Apparently there is an SQL injection bug 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 is allowing someone to see the entire contents of my database. I would've thought that mysql_real_escape_string would prevent that sort of attack? I could cast $id to an integer which should fix the issue, but I want to understand what I did wrong here, so I don't keep repeating my mistake.

this is only a possible duplicate (haven't raised a flag)- http://stackoverflow.com/questions/1220182/does-mysql-real-escape-string-fully-protect-against-sql-injection — davidsleeps, Dec 07 '11 at 02:22
shouldn't you put quotes around the `$id`? - `$sql = "select * from version where id='$id'";` — Aaron W., Dec 07 '11 at 02:25
Not putting this as an answer because I might be wrong, but what if someone passes id as `1 OR 1=1` — jprofitt, Dec 07 '11 at 02:26
Good question, solicited numerous, production worthy responses. +1 — Mike Purcell, Dec 07 '11 at 03:35

score 5 · Accepted Answer · answered Dec 07 '11 at 02:34

5

I think part of the problem is that you aren't using quotes around $id, so an attacker may send a value for id that is 1 OR 1=1 and the SQL executed would be:

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

mysql_real_escape_string() escapes just NULLs, newlines, and quotes (\x00, \n, \r, \, ', " and \x1a), so it is of little help if you don't quote the variable.

answered Dec 07 '11 at 02:34

Elias Dorneles

22,556
11
85
107

This is true. Either make sure id is an int, or put quotes around it. – Chris Dec 07 '11 at 02:36
I think this is it. I've confirmed with at least one sql injection testing tool that putting quotes around the $id does indeed fix the flaw. Now to go and check about 200 other php pages :[ – fret Dec 07 '11 at 03:10
1

@fret: Putting quotes around ints is not the best approach, imo you should do something like `$id = intval($id);` – Mike Purcell Dec 07 '11 at 03:32
@elijunior isnt PDO and Prepared statements the best approach? Why use this old method when php5 is such a common place? – footy Dec 23 '11 at 03:14

score 4 · Answer 2 · edited May 23 '17 at 12:20

You (at least) need to quote the $id even the column is an integer

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

But, if the supply $_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 bind the parameter :

select * from version where id :=id

Or at least doing the casting to integer $id = (int)$_GET["id"],
you are suppose to compare to integer column, aren't you?
(force it to zero when the $_GET["id"] is not an integer)

Take a look on this :-

And always enclose the database link identifier when dealing with mysql_* function
(you might have multiple database connections in a single page)

The casting to int is definitely a good way to go when you can. I'll keep that in mind, thanks. — fret, Dec 07 '11 at 03:11
@fret hi, bind parameter is proper way, and keep in mind the casting only works for integer/numeric column — ajreal, Dec 07 '11 at 03:13

score 1 · Answer 3 · answered Dec 07 '11 at 02:33

It's possible for mysql_real_escape_string to fail to account for certain multi-byte encoded characters. Your attacker is likely using a multi-byte sequence that is improperly escaped for your db's particular charset.

The best way to prevent injection is through the use of prepared statements.

score 1 · Answer 4 · answered Dec 07 '11 at 02:42

My take on this is that there are multiple possibilities:

As there are no quotes around $id an attacker may send a value for id that is 1 OR 1=1 and the SQL executed would be:

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

Check if its an Integer:

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

Type Caste it:

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

Lastly but not the least, I would use prepared statements

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

score 0 · Answer 5 · answered Dec 07 '11 at 02:33

0

The only problem i could see is with your ID

I would check if its really an integer.

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

You can proceed... That will assure you that the id is really a integer.

answered Dec 07 '11 at 02:33

Insecurefarm

391
5
16

1

`$_GET` data will always be a string type. – Wesley Murch Dec 07 '11 at 02:37
you have to caste it before getting an integer value out of `$_GET` values as its always an integer, But if you do caste it the statement is always true, so its an irony and this answer cannot be true – footy Dec 07 '11 at 02:43
`is_numeric()` would still result to true, though. – Yes Barry Dec 07 '11 at 02:45

Apparently there is an SQL injection bug in my PHP code

5 Answers5

Linked