Suggestions

I'm a new user, but I have some suggestions.
Usually when I install a web app, only like to put the parts the browser actually requests withing a directory apache serves.

IE - with sphider, the unpacked zip file is in
/srv/mydomain/sphider-1.3.4
whis is NOT in the document root.

Then I copy the pages that actually get served into the web root and set it to include the original directory.

That's a little bit difficult with sphider - because there are many places where it uses a variable for includes set to ./foo - IE

$settings_dir = "./settings";

What you probably should do is this - at the top of all the php files that get served, have

ini_set("include_path", ".";

Then - you can do

$settings_dir = "settings";

Note the lack of of the ./

That way - people like me who just want to copy search.php into our web root can change the include_path to -

ini_set("include_path", "/srv/mydomain/sphider-1.3.4";

and it will just work. Well, there are a few other changes that need to be made to separate the served pages from the non served pages, but that's the main one. I'm having to find all the relative path includes and change them because the package wasn't set up to make the served pages physically separate from it's includes. There's good reason for separating them, things like the database and admin password should NOT be in the web root where a mis-configuration of the web server could result in the server sending them as plain text - and the directories the admin interface has write permission to should not be in the web root because you never want the server to have a server writable directory in the web root (you use wrappers to fetch the data from writable directories if needed).

I'm rambling, maybe I'll make a patch of the changes I'm making to make it flawlessly work, but setting the include path so you can get rid of relative include paths in the code would be a start.

I think it would be a great idea for the author of this script to create an SVN repository for community updates and contributions. Having to weed through the forum for a patch here and a patch there is a horrible way to fix bugs.

Mike

I'm definitely going to make at least one patch, probably two.

The definite patch is for making it easier to separate the served pages from the archive.

The possible patch will be for the setting/conf.php file.

Currently to update the settings, the application wants the web server to have WRITE permission to a file that many other files read.

The right way to do it is in a database table. Read the default settings from the conf file, and then do a database query to update the settings that are changed. The database query can be done at the end of the conf.php file so that nothing else needs to be altered (other than the admin/configset.php file - which will need to do an sql update instead of all the fwrites)

I don't think I'll be using the web interface to updating that file though, so I don't know if I will.

Patch for mysql injection

--- search.php 2009-01-26 12:20:42.000000000 -0800
+++ search.php.new 2009-01-26 12:21:53.000000000 -0800
@@ -11,23 +11,23 @@
//extract(getHttpVars());



if (isset($_GET['query']))

- $query = $_GET['query'];

+ $query = mysql_real_escape_string($_GET['query']);

if (isset($_GET['search']))

- $search = $_GET['search'];

+ $search = mysql_real_escape_string($_GET['search']);

if (isset($_GET['domain']))

- $domain = $_GET['domain'];

+ $domain = mysql_real_escape_string($_GET['domain']);

if (isset($_GET['type']))

- $type = $_GET['type'];

+ $type = mysql_real_escape_string($_GET['type']);

if (isset($_GET['catid']))

- $catid = $_GET['catid'];

+ $catid = mysql_real_escape_string($_GET['catid']);

if (isset($_GET['category']))

- $category = $_GET['category'];

+ $category = mysql_real_escape_string($_GET['category']);

if (isset($_GET['results']))

- $results = $_GET['results'];

+ $results = mysql_real_escape_string($_GET['results']);

if (isset($_GET['start']))

- $start = $_GET['start'];

+ $start = mysql_real_escape_string($_GET['start']);

if (isset($_GET['adv']))

- $adv = $_GET['adv'];

+ $adv = mysql_real_escape_string($_GET['adv']);





$include_dir = "./include";

Thanks for this,

I'm not a coder so I don't understand exactly what your patch does but I believe it'll prevent possible SQL injections via the search form. Am I right?

I'm not sure that sphider was vulnerable, it may clean the data in the includes functions (which I haven't gone through yet), but it's good practice to always clean any input the php script eats from an external source with mysql_real_escape_string if it is going to be used for a database query, and it's best to clean it as soon as the script gets the variable.

Okay,

Well, sounds like good practice, prevention for just in case instead of finding out the hard way. Keep up the good work!

Greetings.

Well Sphider 1.3.4 does have a possible vulnerability in the SQL injection department. The script simply adds slashes to escape query variables, which has been proven to be easily circumvented through the use of multi-byte characters. SQL variables must be escaped with the PHP function mysql_real_escape_string, which is completely lacking in the script.

Mike

EDIT:

I didn't mean that the script is poorly written, just that on certain MySQL character sets (such as GBK), the php stripslashes function can be ineffective in protecting against injection attacks. With a potentially multi-lingual script as Sphider, it COULD be a problem.



Edited 1 time(s). Last edit at 01/29/2009 10:34PM by mikez.