sql injection prevention advice

[roly]

hi

i've got some old mainstream sites that were getting hacked with sql injection (only turkish script kiddies), as a temporary measure i removed some functionality of the site to prevent it happening again.

i want to get the sites working properly again. i found this code snippet while i was searching for a solution, is this all i need to prevent an sql injection attack using post or get happening again?

  //This stops SQL Injection in POST vars 
  foreach ($_POST as $key => $value) { 
    $_POST[$key] = mysql_real_escape_string($value); 
  } 

  //This stops SQL Injection in GET vars 
  foreach ($_GET as $key => $value) { 
    $_GET[$key] = mysql_real_escape_string($value); 
  }

as you can tell i'm not a coder so any advice appreciated.

thanks in advance

[Tempest]

Yes, that will help. However, all the values should be validated if you want to stop all potential issues as it will also depend on how the sql statements are done.

You want to do something like this on the post values. And I think it needs to be done after you're connected to the database for mysql_real_escape_string to work.

foreach($_POST as $key => $val){
	if( is_array($val) ){
		for($i = 0; $i < count($val); $i++){
			$_POST[$key][$i] = mysql_real_escape_string(get_magic_quotes_gpc() ? stripslashes($val[$i]) : $val[$i]);
		}
	}else{
		$_POST[$key] = mysql_real_escape_string(get_magic_quotes_gpc() ? stripslashes($val) : $val);
	}
}

[Zyber]

Using mysql_real_escape_string() is a good start but it is far from being enough. Next you should make sure that you use ' around all values in SQL, both strings and numbers. Remove words such as UNION, SELECT, DROP, LOAD, BENCHMARK # -- /* and so on.

There are so many possible ways of injection that it is close to impossible to make it really bulletproof.

You should make a read-only SQL user with only read rights (for that particular database) which you use for your SELECT statements.

Never use the root mysql user.

Then have one user with write-only rights for UPDATE. Delete-only for DELETE. etc

If you are even more paranoid you could lock certain SQL commands to certain tables to mitigate the potential damage.

Doing this approach with multiple sql-connections can be difficult on premade scripts.

I am sure there are many more things to do. But for sure you are not protected if you only rely on that PHP code.

Maybe someone else can chime in with more and better suggestions.

[BestXXXPorn]

Originally Posted by Tempest

Yes, that will help. However, all the values should be validated if you want to stop all potential issues as it will also depend on how the sql statements are done.

You want to do something like this on the post values. And I think it needs to be done after you're connected to the database for mysql_real_escape_string to work.

foreach($_POST as $key => $val){
	if( is_array($val) ){
		for($i = 0; $i < count($val); $i++){
			$_POST[$key][$i] = mysql_real_escape_string(get_magic_quotes_gpc() ? stripslashes($val[$i]) : $val[$i]);
		}
	}else{
		$_POST[$key] = mysql_real_escape_string(get_magic_quotes_gpc() ? stripslashes($val) : $val);
	}
}

Tempest is correct, you'll need to be connected before using that function. It essentially asks MySQL to use its own escaping methods to ensure a given string is escaped properly. The best time to do this would be any time you are selecting or updating/inserting content into the DB that uses user input as a variable... When I mean user input I mean anything that can come from a URL, a FORM, etc...

e.g.

You have pagination that uses GET parameters like: /models?page=1&hair=blue

While the user may not have had the option of typing in "blue" they could still override the GET param with some sort of SQL injection string... so it needs to be escaped.

The proper solution... and here is why the MVC Model/View/Controller design pattern is so damned handy ;) is to escape each field being queried for or updated using the aforementioned real escape string function/method. If your code base is MVC your Model (the database) logic will be split out so adding in this check for each field should be simple. If you're using something procedural or even OO and the Model is not split out correctly you'll be doing a LOT of search/replace action to find every place one of the queries is done and escape each individual field...

I WOULD NOT recommend doing something on every POST field unless each field is not used anywhere else in the code base. Otherwise you'll end up with lots of issues with variables no longer matching was was expected... Imagine $modelName="April O'niel" vs = "April O\'Neil"...

Long story short it can either be a huge pain in the ass or not that big of a deal depending on the competency level of the developer who originally wrote the script :P

Hope that helps clarify that a bit for you ;)

[BestXXXPorn]

Originally Posted by Zyber

Using mysql_real_escape_string() is a good start but it is far from being enough. Next you should make sure that you use ' around all values in SQL, both strings and numbers. Remove words such as UNION, SELECT, DROP, LOAD, BENCHMARK # -- /* and so on.

There are so many possible ways of injection that it is close to impossible to make it really bulletproof.

You should make a read-only SQL user with only read rights (for that particular database) which you use for your SELECT statements.

Never use the root mysql user.

Then have one user with write-only rights for UPDATE. Delete-only for DELETE. etc

If you are even more paranoid you could lock certain SQL commands to certain tables to mitigate the potential damage.

Doing this approach with multiple sql-connections can be difficult on premade scripts.

I am sure there are many more things to do. But for sure you are not protected if you only rely on that PHP code.

Maybe someone else can chime in with more and better suggestions.

Zyber is also correct...

Best practices dictate a MySQL user should ONLY have the finite privileges it requires. All other privileges should be revoked for that user. Those privileges should also only apply to the DB(s) that user needs to access (they should not be global privileges).

[Zyber]

You should sanitize $_GET, $_POST and $_COOKIE and also be paranoid about values from HTTP Headers (if you use them in SQL, for example HTTP_REFERER)

Good luck ;)

[Zyber]

And one more thing, make sure that neither PHP nor MySQL is allowed to give error messages. No error message should be sent to the screen. It makes it harder for the attacker to see what he is doing as he will have to operate blindly.

[Zyber]

You can also move some of the logic to PHP instead of MySQL. For example you can compare values in PHP arrays instead of sending them to MySQL.

SQL for authentication (unsafe)

SELECT 1 FROM users WHERE username='$username' AND password='$password'

The below is safer as the user-input is never sent to MySQL.

SQL to preload PHP array

SELECT username, password FROM users

Authentication in PHP

if ($passwords[$username] == $password){
 return true;
}
else {
 return false;
}
unset($passwords);

This is just an example to give you the idea

[fris]

http://www.netlobo.com/preventing_mysql_injection.html

[Varius]

Originally Posted by Zyber

Next you should make sure that you use ' around all values in SQL, both strings and numbers. Remove words such as UNION, SELECT, DROP, LOAD, BENCHMARK # -- /* and so on.

This part I have to disagree with. There are cases you need those words or characters.

Also for putting quotes around integers, that is not a great idea either. While MySQL has advanced to the point it doesn't make a huge speed difference anymore, it still does slow-down queries on extremely large tables or involving complex joins and it still slows queries down quite a bit in PostgreSQL.

The proper thing to do would just be to properly sanitize all variables that are involved in your queries. If everything is sanitized, there will be no injection. For example, instead of putting quotes around an integer like age, you can verify it's of the proper type and within a specific range.

The rest of your advice I can agree with though

[Zyber]

Originally Posted by Varius

The proper thing to do would just be to properly sanitize all variables that are involved in your queries. If everything is sanitized, there will be no injection. For example, instead of putting quotes around an integer like age, you can verify it's of the proper type and within a specific range.

I agree with this. Properly validated integers should be safe to use without quotes.

[Zyber]

Oh, I almost forgot one important thing.

MySQL accepts both ASCII and HEX values. Someone could write their payload as 0x6e6f7468696e67206865726520746f20736565

That string could contain anything...

Such strings could possibly go undetected by type-validation as all the ASCII chars look harmless.

Making proper sanitation is really hard, and there is always the risk that you have missed something.

In my opinion one should be extra paranoid with databases. They are like Swiss cheese..

[rowan]

Quick tip: if the value you're expecting should be an integer, you can force PHP to treat it as such like this:

$val = 0 + $_POST["variable"];

[BestXXXPorn]

Originally Posted by rowan

Quick tip: if the value you're expecting should be an integer, you can force PHP to treat it as such like this:

$val = 0 + $_POST["variable"];

Ouch... no don't do that and if you really must typecast instead use

$val = intval($_POST['variable']);

And always use single quotes unless you need special escaped chars... Every time you use double quotes for a string in PHP you cause the string interpreter to parse the entire string looking for variables, special chars, etc... Using single quotes instead ensures that doesn't happen...

[Argos88]

if you use wordpress, this is prevented by default on their CMS.

.

[Tempest]

Originally Posted by BestXXXPorn

Ouch... no don't do that and if you really must typecast instead use

$val = intval($_POST['variable']);

And always use single quotes unless you need special escaped chars... Every time you use double quotes for a string in PHP you cause the string interpreter to parse the entire string looking for variables, special chars, etc... Using single quotes instead ensures that doesn't happen...

Agreed with the single quotes.. Far too much code out there that uses double quotes.

As for the int thing, let's all argue about how best to handle them lol...

I actually do a preg_match on all values that are supposed to be pure integers so I can catch invalid data. i.e.:

preg_match('/^[+-]?\d+$/', (string)$value)

[CYF]

someone forward this thread to CCBill

[Varius]

Originally Posted by Tempest

Agreed with the single quotes.. Far too much code out there that uses double quotes.

As for the int thing, let's all argue about how best to handle them lol...

I actually do a preg_match on all values that are supposed to be pure integers so I can catch invalid data. i.e.:

preg_match('/^[+-]?\d+$/', (string)$value)

Speaking of preg_match, to everyone out there not using the latest version yet, if you have been using ereg you need to stop and start using preg_match, as ereg has become deprecated.

same goes for the split() function, which is very common across people's code so better off with explode in most cases.

[roly]

Originally Posted by Zyber

Using mysql_real_escape_string() is a good start but it is far from being enough. Next you should make sure that you use ' around all values in SQL, both strings and numbers. Remove words such as UNION, SELECT, DROP, LOAD, BENCHMARK # -- /* and so on.

There are so many possible ways of injection that it is close to impossible to make it really bulletproof.

You should make a read-only SQL user with only read rights (for that particular database) which you use for your SELECT statements.

Never use the root mysql user.

Then have one user with write-only rights for UPDATE. Delete-only for DELETE. etc

If you are even more paranoid you could lock certain SQL commands to certain tables to mitigate the potential damage.

Doing this approach with multiple sql-connections can be difficult on premade scripts.

I am sure there are many more things to do. But for sure you are not protected if you only rely on that PHP code.

Maybe someone else can chime in with more and better suggestions.

A big thanks for everyones input its a huge help

I've got a lot of sites using variations of this (very old) script, its not suported anymore and i don't have the time to find an alternative and switch to that. so i've just got to try and patch it up as best as possible. they were using something along the lines of this to hack the script:

page.php?id=-999999999+union+select+concat(login,0x3a,password),1,2,3,4,5+from+adminlogin/

so if i implement some of the recommendations above and also remove union, select, etc as well, hopefully i should be a bit safer.

[mstach]

PHP's website has good examples for you.

php.net/manual/en/security.database.sql-injection.php

[Zyber]

Originally Posted by roly

A big thanks for everyones input its a huge help

I've got a lot of sites using variations of this (very old) script, its not suported anymore and i don't have the time to find an alternative and switch to that. so i've just got to try and patch it up as best as possible. they were using something along the lines of this to hack the script:

page.php?id=-999999999+union+select+concat(login,0x3a,password),1,2,3,4,5+from+adminlogin/

so if i implement some of the recommendations above and also remove union, select, etc as well, hopefully i should be a bit safer.

Glad we could all help

CONCAT is also a naughty word as you found out.

[Tempest]

Originally Posted by roly

A big thanks for everyones input its a huge help

I've got a lot of sites using variations of this (very old) script, its not suported anymore and i don't have the time to find an alternative and switch to that. so i've just got to try and patch it up as best as possible. they were using something along the lines of this to hack the script:

page.php?id=-999999999+union+select+concat(login,0x3a,password),1,2,3,4,5+from+adminlogin/

so if i implement some of the recommendations above and also remove union, select, etc as well, hopefully i should be a bit safer.

If you validate/sanatize the input variables you'll get rid of those types of instances. That's a perfect example of why I check that all integer values (especially id type values) are true integers before using them.

[Zyber]

A safer practise is to use parameterized SQL queries. It also has performance benefits.

The negative thing is that makes your code more complex, and thus less pretty.

See example here (note MySQLi and not MySQL)
http://www.php.net/manual/en/mysqli.prepare.php

[Babaganoosh]

Use a mysql class. I've been using the mysql class at ricocheting.com/code/php/mysql-database-class-wrapper-v3 and modified it a little bit to add some functionality that I needed. It's good protection for lazy coders like me.