MySQL Injection

[elite311]

I have done a bunch of reading and I just want to make sure I am doing this correctly to protect against injection. I'm hoping someone can confirm if this is the correct way to protect against injection.

 

if(isset($_POST['updateit']))
{
// Protect against injection
$site = mysql_real_escape_string($_POST[job]);
$avguse = mysql_real_escape_string($_POST[avguse]);
$id = mysql_real_escape_string($_POST[id]);

// Update database
$db->query("UPDATE assets
			SET pmcount = '$avguse',
			updatedby = '{$_SESSION['username']}',
			updateddate = NOW()
			WHERE id = '$id'")
			or die(mysql_error());

// Redirect page						
header("Location: locationinfo.php?id=$site&updated=1");
exit();
}

[Jessica]

is pmcount supposed to be a number? if so you can just cast it as an int, and not quote it.

Same with $id.

[elite311]

Yes just a number, I'm not sure I understand what you mean by cast it in as an int

[Jessica]

|

|

V

[elite311]

I'm a little confused, I'll do more reading on this though. As far as my script is now though it would protect against injection the way it is currently?

[xyph]

Associative array strings need to be quoted.

 

You should be coding with error reporting on - the code you posted should be giving you undefined constant notices.

[Christian F.]

You have a good start there, with a only few minor issues.

 

As noted by Josi, when you have numerical data you should use intval (), floatval () or %d with sprintf (). This will ensure that the data that's sent to the database is actually a number, and not a string which can cause some odd results.

Also, $_POST[id] (etc) is wrong, unless you have defined a constant named id which contains the name of the index you want to find. Somehow I doubt this, and thus you should be using quotes around the index as it is a string. In other words $_POST['id']. This is true for all associative array indices. If you have turned on error reporting, as xyph demonstrates in his signature, then you would have seen this.

 

Now, onto the real meat, namely the escaping itself. You'll notice that I mentioned sprintf () in the list of functions that should be used with numerical data, and it is in fact my preferred method. I'll get to that in a bit, but first let me explain a bit more about when to apply output escaping and why.

The proper time to apply output escaping is just before you send the output to the third party system, ensuring that whatever you send is indeed recognized as data and not as commands. As such you don't really have a need to save the escaped variables, as re-use is very rare. Not only is there no real need to save them, but applying output escaping to data that's being used in other parts of the code (or other systems) can cause problems. Reason for this is that output escaping applies extra characters to the string, causing the potential meta-characters to be escaped and thus rendered harmless. Problem with that, is that every system has their own set of meta-characters and escaping methods. So if you send output that's escaped for one system to another, then the result will not be what you want (and in some cases potentially harmful).

 

Now, this is why I prefer using sprintf (), as it allows me to construct the output string while simultaneously applying escaping. Without making a mess out of the code, mutating the data for later processing, or duplicating the data in multiple variables. As a quick example, a SQL query with one numerical value and one string:

$query = "SELECT `id`, `name` FROM `users` WHERE `username` = '%s' AND `status` = %d";
$query = sprintf ($query, $db->real_escape_string ($username), $id);

In this example I've instructed sprintf () to treat the second parameter as a string, after applying escaping to it, and the third parameter as a numerical value. Since "%d" only accepts numerical data, I don't have to apply any further escaping to it as number are numbers and everything else is discarded.

 

So, a good start, but some holes in your understanding still. Hopefully less holes now that you've read my post.

 

PS: What you should have done in your script, where you currently have the escaping, is to validate the input. This is something you'll always need to do when accepting input from users, and is a separate concern from output escaping. They do tend to go hand in hand though, as you normally want to save/send the user-generated input somewhere, and thus need to escape it.

A good rule of thumb is that input validation always happens first, and output escaping last.

[fenway]

Unless you're using bind parameters, it's always better to quote strings -- particularly if you're writing raw SQL via PHP.

[elite311]

Thank you all for the replies it has helped a lot with my understanding, but Particularly thank you ChristianF for taking the time to explain all of that. I am defiantly going to read up on that sprintf function as well as due some reading on validating form input and error checking.

 

Again thanks everyone for all the help so far, this is a great place for a beginner like myself to learn from people who really understand this stuff.

[gizmola]

There's nothing wrong with what ChristianF provided, and some good information there.  As with most languages, there are numerous ways to skin the same cat. 

 

However, what jesirose was suggesting in regards to integers specifically, is that you can typecast the parameter that must be an integer.  It is fairly well known that typecasting is the technique that performs the best, since it's a language construct and not a function that has to be called.  I don't want to oversell it because in one script the difference between something that takes milliseconds is negligible, but you might as well learn about it. 

 

From looking at your sql statement, it looks as if you actually have 2 columns that are probably integers in the database -- id and avguse.

 

$avguse = (int)$_POST[avguse];
$id = (int)$_POST[id];

 

If you experiment with this, you'll find that no matter what you put into the url parameters, the variables will be safely converted to integers.  Of course when you put in strings, in most cases they will be converted to 0, so you want to make sure your query works acceptably even when zeros are passed.

[Christian F.]

You're most welcome, Elite, I'm glad I could be of assistance.