Preventing Injection: Is This Good Enough?

[fr600]

Is the following function good enough to prevent SQL injection?

function clean($data)
{
	$data = trim($data);
	$data = strip_tags($data);
	if(get_magic_quotes_gpc())
	{
		$data = stripslashes($data);
	}
	if(is_numeric($data))
	{
		//$data = intval($data);
		$data = $data+0;
	}
	else
	{
		$data = mysql_real_escape_string($data);
	}
	$data = htmlspecialchars($data);
	return $data;
}

Anyway I can improve it?

[bradgrafelman]

Here's a sort of code critique I came up with as I decomposed your function:

1. $data = trim($data);
   $data = strip_tags($data);

   Neither of those things has anything to do with preventing SQL injection. Instead, you're automatically assuming that a) you'd never want any sort of whitespace on either side of the data string, and b) it's okay to destructively modify the data such that anything that looks like an HTML tag gets removed.

   Those are two assumptions I personally would never make. Instead, if either (or both) of those assumptions were true for any particular datum, I would apply the appropriate escaping/sanitizing mechanisms before I passed it to any SQL-centric cleaning function.

   Before you read much further in this critique, note that I'm assuming this clean() function is meant to be used to prepare data for SQL queries. In other words, it is a "SQL-centric cleaning function" as I described it above.

2. Out of curiosity, can you explain why this:

   	if(is_numeric($data))
   	{
   		//$data = intval($data);
   		$data = $data+0;
   	}
   	else
   	{
   		$data = mysql_real_escape_string($data);
   	}

   is helpful/useful? Specifically, I'm referring to the true branch of the if() statement (and/or why that if() conditional is there at all).

3. $data = htmlspecialchars($data);

   Again, you're assuming that the datum passed in should be treated as if it might later be inserted into an HTML document and that it shouldn't be allowed to contain HTML itself (which will instead be converted into HTML-safe entities).

   As before, these are assumptions that I personally would not make. Otherwise, your clean() function is only good for a specific use case rather than being a generalized "clean()"-ing function.

[fr600]

To be honest, I have no idea what the function does. I'm an amateur and I need to secure my codes. So I visited many websites and found a lot of functions for injection prevention. Not knowing what each of them does specifically, I combined all of them and came up with this.

1. Yes it is meant to be used to prepare data for SQL queries only, nothing else.
2. Found it somewhere.

Since I have no idea of what I'm doing, I'm requesting you to make it better.

[bradgrafelman]

It sounds like what you're after is some magic end-all solution to sanitizing data for SQL queries. Then problem with that is: there isn't one. *

As far as SQL sanitization goes, you really only need to do one of two things. Which one is correct will depend upon what type of data you're expecting. Here's the two cases:

1. The data is a string (note this includes binary data, e.g. an image file). All you need to do is pass it through an escaping mechanism such as [man]mysql_real_escape_string/man (hence where the function got part of its name: '_string').

2. The data isn't a string. Here, a simple way is to just cast the data to the appropriate type to ensure that someone didn't try to pass a string which includes malicious SQL code.

*: Okay, some will say to just use prepared statements and let the driver/library do the escaping for you rather than worrying about learning how to properly escape the data yourself. While this does work, I find this methodology to be grossly dangerous, irresponsible, and just plain stupid.

[laserlight]

bradgrafelman wrote:

Out of curiosity, can you explain why this:

    if(is_numeric($data))
    {
        //$data = intval($data);
        $data = $data+0;
    }
    else
    {
        $data = mysql_real_escape_string($data);
    }

is helpful/useful? Specifically, I'm referring to the true branch of the if() statement (and/or why that if() conditional is there at all).

It is type coercion that handles both integers and floating point numbers: whatever the numeric context of the string, it will then be converted to the appropriate numeric type. The thing is, it is redundant since $data will be placed back into some SQL string afterwards anyway, so keeping it to be of string type (if it was even of string type to begin with) is fine since it is known to be numeric.

[bradgrafelman]

@: It was a rhetorical question. I was hoping that fr600 would explain why he did that (since I was assuming he had written all of the code personally) so that I could hopefully correct any misunderstanding. :p

EDIT: My point would have been that it's actually a lot worse than redundant; it's giving you a false sense of security. For example, say you did this:

$sql = "SELECT isAdmin FROM myTable WHERE id = $_GET[id]";

Oops! We used raw user-supplied data in our query! Better use clean() since it'll make sure to handle numeric data:

$sql = 'SELECT isAdmin FROM myTable WHERE id = ' . clean($_GET['id']);

Oops! We're still vulnerable to SQL injection attacks...

$_GET['id'] = '999 OR 1 = 1 ORDER BY isAdmin DESC'; // assume this was our input

$sql = 'SELECT isAdmin FROM myTable WHERE id = ' . clean($_GET['id']);

$sql now looks like:

SELECT isAdmin FROM myTable WHRE id = 999 OR 1 = 1 ORDER BY isAdmin DESC

which is quite bad.

[fr600]

I now understand a little better. Like I said, I found a lot of examples on the web and I just combined them all and came up with this clean function. Like I said I'm an amateur and I'm here to learn from you guys.

From those examples, someone said using intval would take care of integers. But I can't use it since I'm mostly dealing with decimals. Any cure for that?

Btw, this was my last clean function.

function clean($data)
{
	$data = trim($data);
	$data = strip_tags($data);
	if(get_magic_quotes_gpc())
	{
		$data = stripslashes($data);
	}
	if(is_numeric($data))
	{
		$data = $data+0;
	}
	$data = mysql_real_escape_string($data);
	return $data;
}

[Bjom]

It's quite a good idea to get to understand the code you are using.

Apart from that I would suggest that you do not rely on all that real escape string stuff but use

a) sprintf to assemble your SQL statements.

b) use mysqli and prepared statements to execute your sql

read up on it there:

sprintf
mysqli
prepared statements

play around with it, you'll pick it up quick.

Bjom

[NogDog]

To avoid the problem BPAT was alluding to, you can let the function "decide" whether the value needs to be quoted, e.g.:

function clean($data)
{
	if(get_magic_quotes_gpc())
	{
		$data = stripslashes($data);
	}
	if(!is_numeric($data))
	{
		$data = "'" . mysql_real_escape_string($data) . "'";
	}
	return $data;
}

// Sample usage
$sql = "SELECT * FROM table_name WHERE id=" . clean($_POST['id'])
     . " AND col_1=" . clean($_POST['col1'];

Note that where I typed the sample SQL string, no quotes were put around either value -- even though I expect the "col1" value to be a string -- as the clean() function will now handle it for me when needed.

Another option is to use sprintf() to force the numeric type as applicable (e.g. via the %d or %f place-holders), in which case you could choose not to have the function add the quotes around strings if you prefer not to:

function clean($data)
{
	if(get_magic_quotes_gpc())
	{
		$data = stripslashes($data);
	}
	if(!is_numeric($data))
	{
		$data = mysql_real_escape_string($data);
	}
	return $data;
}

// Sample usage
$sql = sprintf(
   "SELECT * FROM table_name WHERE id=%d AND col1='%s'",
   clean($_POST['id']),
   clean($_POST['col1'])
);

Or you could just use mysqli (or PDO) and prepared statements with bound parameters, and be done with all this fun. 😉

[fr600]

Sorry for late reply. Thank you all so much. I'll try and learn what you guys recommended. If I get stuck, I know where I can ask the experts some questions. 🙂

[dysback]

I don't understand you guys.
Why You don't simply use PDO extension.
With PDO there's no need to worry about SQL injection.

[bradgrafelman]

dysback;10982521 wrote:

Why You don't simply use PDO extension.

I suspect it's mostly because of unfamiliarity, but it's also possible that the PDO extension simply isn't available or feasible to use on the project at hand.

dysback;10982521 wrote:

With PDO there's no need to worry about SQL injection.

That's true only if you use prepared statements. Personally, the only time I would ever bother with the overhead of a prepared statement is if I plan on repeating the statement with different parameters. If it's only a one-time execution, why bother?

And besides, isn't it also important to know how prepared statements goes about properly escaping the data? Or would you rather remain blissfully ignorant and never know how to do things properly and just hope that there is always a tool/library/person available to do it for you?

[laserlight]

bradgrafelman wrote:

If it's only a one-time execution, why bother?

Because:

dysback wrote:

there's no need to worry about SQL injection.

Admittedly a bit of an exaggeration, but it is certainly a step closer to that.

bradgrafelman wrote:

And besides, isn't it also important to know how prepared statements goes about properly escaping the data?

To some extent, I'd say yes. However, learning how to use an escaping function teaches nothing about how prepared statements handle string input, and learning how to cast does not do much better in that direction either.

bradgrafelman wrote:

Or would you rather remain blissfully ignorant and never know how to do things properly and just hope that there is always a tool/library/person available to do it for you?

So you are saying that prepared statements are not a proper way to do things? Besides, if you advocate this, then you should not immediately advocate the use of say, mysql_real_escape_string. Rather, you should first recommend the implementation of a mysql_real_escape_string equivalent that uses fundamental constructs to implement the escaping rules, and only suggest the use of mysql_real_escape_string after the person knows how mysql_real_escape_string "goes about properly escaping the data".

[bradgrafelman]

laserlight;10982598 wrote:

So you are saying that prepared statements are not a proper way to do things?

Certainly not, but I don't think they're the only answer either. Plus, there are some situations where you can't even used a parameterized variable in a prepared statement, e.g.:

SELECT * FROM ?;
SELECT foo FROM bar ORDER BY sortCol ?;

and so on. If you're of the mindset that "there's no need to worry about SQL injection" and you run into one of those situations, then you might be in trouble.

laserlight;10982598 wrote:

Besides, if you advocate this, then you should not immediately advocate the use of say, mysql_real_escape_string. Rather, you should first recommend the implementation of a mysql_real_escape_string equivalent that uses fundamental constructs to implement the escaping rules, and only suggest the use of mysql_real_escape_string after the person knows how mysql_real_escape_string "goes about properly escaping the data".

There's one problem with either of our solutions. Proper handling of data is a topic I could fill an entire post (or more) with tips and tricks and just general knowledge. However, the "newbies" who don't have a formal education in such matters are simply looking for the shortest article/response out there that appears to make their code work. If I filled every post I ever made that offers advice about how data handling should be done properly, I'd most likely exceed the character count every time (not to mention the fact that no one would probably bother to read all of it).