Quality of purchased code

[billuk]

I recently purchased a package written in PHP. I have been checking the code for quality.

This is an example of one part of the code:

$status_new = $_POST['status_new'];
mysql_query("UPDATE users SET user_status='$status_new' WHERE user_id='$user_info[user_id]' LIMIT 1");

Now is it me, or is that horrifically dodgy? Taking a user submitted variable and dumping it straight into a SQL code. Ideal for a SQL injection surely? Also the none enclosed array ref aint to hot.

Or am I barking up the wrong tree?

[jatar_k]

nope, you are exactly right, I would change the query line to

mysql_query("UPDATE users SET user_status='$status_new' WHERE user_id='" . $user_info['user_id'] . "' LIMIT 1");

to be honest I would change it to

$q = "UPDATE users SET user_status='$status_new' WHERE user_id='" . $user_info['user_id'] . "' LIMIT 1";
mysql_query($q);

and that user data needs some cleaning

it's great that you are actually looking through this billuk, most people just use it however they get it.

[billuk]

Hi jatar_k, thanks for your speedy reply and thanks for confirming things. I try to check as much as possible, paranoia is the best policy!

I was going to use the mysql_escape_string wrapped around the user input.

eg.

$q = "UPDATE users SET user_status='$status_new' WHERE user_id='" . mysql_escape_string($user_info['user_id']) . "' LIMIT 1"; 
mysql_query($q); 

This does protect things to a certain extent, doesn't it?

[jatar_k]

use mysql_real_escape_string [php.net] as the other is deprecated.

Remember that the "real" version needs an open connection to the db to work as it takes into account the charset

it does help a little, depending on what data you expect there are a lot of other tests that could be done

[billuk]

Ok many thanks, I'll look into it :)

[WesleyC]

If user_id is an autoincrement ID column in the database (as I expect it is), then the value you want for that column will always be an integer. Rather than using the slightly slower mysql_real_escape_string, you could simply force it to an integer value using intval($user_info['user_id']).

The programmer may have been assuming the server had some nonstandard PHP settings like magic quotes, or he may have done cleaning on the entire $_POST array. You might make sure this is the case before you go too far--I've run into several programmers that use the (annoying, but at least secure) practice of sanitizing all $_POST, $_GET, and $_COOKIE variables at the top of their code and then just assigning all the clean values back into those arrays.

[billuk]

Hi WesleyC, thanks for your post. Ive double checked and you are quite right, there is a check/clean on the $_POST array which I missed as it's hidden away in an included file. Saves me ripping their code to pieces any more than I have to!

[vincevincevince]

Check that check/clean on the POST really really well. In particular, pass through Multibyte strings, including some which start with a single-byte character. Put through really long strings (as long as you can get through). Be sure they get escaped to the very end of the string. Don't forget to include linebreaks in your test strings.

Here are some of the other key characters to test, according to php.net:

\x00, \n, \r, \, ', " and \x1a

All of those should be escaped, whether single-byte or multi-byte.

Find the hacks which worked against mysql_escape_string() (easy enough to find) and test them against this homespun escaping function.

[jatar_k]

as an aside

>> sanitizing all $_POST, $_GET, and $_COOKIE variables at the top of their code and then just assigning all the clean values back into those arrays.

really foolish practice

[vincevincevince]

mysqli is a good way around this due to the built-in escaping, but you still need to do your own sanitising.

I've encountered many websites which don't bother to check that the retun of a <select> field is one of the options on the list.

[Habtom]

jatar_k

really foolish practice

Not that I have done it, but I always thought that was a good idea.

Can you tell me why you think it is not a good practice?

[vincevincevince]

Can you tell me why you think it is not a good practice?

Here are mine:

You get used to not sanitising and then do it somewhere else without your 'fix-all-first' function

No matter what you say, magic_quotes is a source of posterior-pain - don't replicate it now we're finally free from it!

Other code which you include() or otherwise link to will get confused and everything will get double-escaped

I don't like the idea of you saving all that typing

[jatar_k]

what V3 said

good practice would be to write cleaned information to a new array or set of variables and deal with that. Destroying the original data is bad form for the exact reason that billuk didn't realize it was getting cleaned. Also what happens if you forget to include your cleaning function? You would still be working with the $_POST array having no clue that you are using bad data.

I also don't agree with changing data submitted by the user, send it back to them to fix. (I'm not saying that's happening but if it is..)