Mysql injection attacks

[Mr Chris]

Hello People,

 

Been reading up on these and trying to understand them more.  Say I have a file called page.php?id=12345

 

and when users hit the page I run this code in the background:

 

$id = $_GET['id'];
$query = "UPDATE tbl SET live = '1' WHERE id = '".$id."'"; 

 

That page is not open to any attack right?  Even though i'm using $_GET.

 

Am I right in thinking that attacks only happen on online forms.  So for example there is no way an attacker could somehow output all the data in my table tbl

 

 

Thank yo

[vungee]

Your example is prone to sql injection as variable $id can be escaped. Make sure you clean your variables with a mysql_real_escape_string.

 

http://php.net/manual/en/function.mysql-real-escape-string.php

 

$id = mysql_real_escape_string($_GET['id']);

[rwwd]

Wrong. $_GET is set through the URL, so this is the easiest way of getting into the script.

$id = strip_tags($_GET['id']);
$query = "UPDATE `tbl` SET `live` = '1' WHERE id = ".$id." "; 

 

And as the value being passed is an integer, you don't need to quote it in the equals part either, you only do this with strings, quoting them turns them into strings (or at least they get treated as strings by the sql engine)

Your example is prone to sql injection as variable $id can be escaped.

Providing the example given is for page values or ID numbers, mysql_real_escape_string() would be pointless, though this is good for string values, as all the function does is add \ to ' like \' to make them sql safe.

 

But you can be too overcautious, but personally I think this is a good thing.

 

Rw

[vungee]

If you choose not to use quotes then validate your data ($id) to an integer (ie. type cast). If it is a string, you will throw a mysql syntax error.

[Mr Chris]

Thanks for the repies guys, but i'm confused, so I need to use:

 

$id = strip_tags($_GET['id']);

or

$id = mysql_real_escape_string($_GET['id']);

[Pikachu2000]

No, you'd just need to cast that as an integer, since the value is expected to be an integer. Then in the query string, leave $id unquoted.

$id = (int) $_GET['id'];

$query = "UPDATE `tbl` SET `live` = 1 WHERE id = $id"; 

 

 

[vungee]

Thanks for the repies guys, but i'm confused, so I need to use:

 

$id = strip_tags($_GET['id']);

or

$id = mysql_real_escape_string($_GET['id']);

 

If $id is always an integer, then add a check to type cast it to an integer and no escape is needed... even though you can to be on the safe side. If $id can be a string then you will want to escape it using mysql_real_escape_string.

[Andy-H]

No, the data is expected to be an integer, and used as such, so you simply need to typecast it as int.

 

$id = (int)$_GET['id'];

http://php.net/manual/en/language.types.type-juggling.php

 

However, if the data is expected to be a string, and used as one, then yopu should use mysql_real_escape_string to escape characters (with a backslash '\') that could be harmfull if injected into an SQL query, such as ( " ' ", apostrophes).

 

When outputting data, to protect against XSS/JS injection, use stripslashes and htmlentities with the ENT_QUOTES flag.

[rwwd]

You really need to add basic error checking too, so that the $_GET['id'] has a default state:-

if(isset($_GET['id']) && ctype_digit($_GET['id'])){
//assign
$id = (int)$_GET['id'];//just use typecasting to force whole numbers here
}
else{
$id = 0;//default is zero
}

 

Now you have a basic checking mechanism to detect any attempts, though you can change the ctype_ function to other functions depending on context of application.

 

Rw

[Andy-H]

You really need to add basic error checking too, so that the $_GET['id'] has a default state:-

if(isset($_GET['id']) && ctype_digit($_GET['id'])){
//assign
$id = (int)$_GET['id'];//just use typecasting to force whole numbers here
}
else{
$id = 0;//default is zero
}

 

Now you have a basic checking mechanism to detect any attempts, though you can change the ctype_ function to other functions depending on context of application.

 

Rw

 

ctype_digit isn't neccessary here, typecasting will do the trick, however, I agree, all user-inputted data should be deemed untrustworthy and validated, for example, checking that the user has permission to delete the record with the submitted id. And, as rwwd suggested, checking $_GET['id'] isset

[Mchl]

And just to demonstrate how the original code can exploited:

page.php?id=12345' OR 1;--

so your query will be

UPDATE tbl SET live = '1' WHERE id = '12345' OR 1;--'

which will set live=1 for all rows in database

[ignace]

$id = (int)$_GET['id'];

 

Is the right way. Type-casting will convert anything that it can't type-cast properly to 0 and as an auto_increment primary key won't be 0 you know that whenever it's 0 then it was invalid. For more information on type-casting and how PHP handles all this stuff:

 

http://php.net/manual/en/language.types.php