There seems to be an SQL injection error in my PHP code

Question

include "../admin/site.php"; // Setup db connection. $appid = -1; if (is_string($_GET["id"])) { $id = mysql_real_escape_string($_GET["id"]); $sql = "select * from version where id=$id"; $ver = mysql_query($sql); if ($id > 0 && $ver && mysql_num_rows($ver)) { $appid = mysql_result($ver, 0, "AppID"); $app = DLookUp("apps", "name", "id=$appid"); $name = mysql_result($ver, 0, "Name"); $notes = mysql_result($ver, 0, "Notes"); } else $app = "No version by that ID"; } else $app = "No ID"; /* some html snipped */ if (isset($app) && isset($name)) echo $app . " v" . $name; else echo "v###"; /* some html snipped */ if (isset($appid)) { $url = "/" . DLookUp("apps", "Page", "id=$appid"); echo "<a href=\"$url\">Up</a> to $app..."; } if (isset($notes)) echo $notes; 

Somehow this code allows someone to see the entire contents of my database. I would think mysql_real_escape_string would prevent such an attack? I could overlay $ id on an integer that should fix the problem, but I want to understand what I did wrong, so I am not repeating my mistake.

Answer 1

I think part of the problem is that you are not using quotes around $id , so an attacker could send a value for id, which is 1 OR 1=1 , and the SQL being executed:

 select * from version where id=1 OR 1=1 

mysql_real_escape_string() only prints NULL, newlines and quotation marks (\ x00, \ n, \ r, \, ', "and \ x1a), so this helps a little if you don't provide this variable.

Answer 2

You (at least) need to specify $id , even if the column is an integer

 select * from version where id="{$id}" 

But if the source is $ _GET ["id"] using

 $id = "xxx\" OR 1=1"; select * from version where id="{$id}" <-- this become a vulnerable as it eval to select * from version where id="xxx" OR 1=1 

You should consider binding a parameter:

 select * from version where id :=id 

Or at least cast an integer $id = (int)$_GET["id"] ,
you can compare with a whole column, right? (forced to zero when $ _GET ["id"] is not an integer)

Take a look at this: -

• Someone hacked into my database - how?
• Does mysql_real_escape_string () really fully protect against SQL injection?

And always include the id of the database link when working with the mysql_ * function
(you can have several database connections on one page)

Answer 3

Perhaps mysql_real_escape_string could not register certain multibyte encoded characters. Your attacker is probably using a multibyte sequence that is improperly escaped for your specific db encoding.

The best way to prevent an injection is to use prepared statements.

Answer 4

I believe that there are several possibilities:

Since there are no quotes in $id , an attacker can send a value for id that is 1 OR 1 = 1, and the SQL that is executed:

 select * from version where id=1 OR 1=1 

Check if there is an integer:

 if (intval($_GET['id']) > [some condition]) //do something 

Type Caste it:

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

Last but not least, I would use prepared statements

 <?php $query = "select * from version where id=?"; $stmt = $dbh->prepare($query); $stmt->execute(array($_GET['id']); ?> 

Answer 5

The only problem I saw is your id

I would check if this is really an integer.

 if (is_int($_GET['id'])) { 

You can continue ... This will assure you that id is indeed an integer.