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)- stackoverflow.com/questions/1220182/… — davidsleeps
– davidsleeps, Commented Dec 7, 2011 at 2:22
shouldn't you put quotes around the $id? - $sql = "select * from version where id='$id'"; — Aaron W.
– Aaron W., Commented Dec 7, 2011 at 2:25
Not putting this as an answer because I might be wrong, but what if someone passes id as 1 OR 1=1 — jprofitt
– jprofitt, Commented Dec 7, 2011 at 2:26
Good question, solicited numerous, production worthy responses. +1 — Mike Purcell
– Mike Purcell, Commented Dec 7, 2011 at 3:35

Elias Dorneles · Accepted Answer · 2011-12-07 02:34:18Z

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 7, 2011 at 2:34

Elias Dorneles

24.2k12 gold badges91 silver badges112 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Chris Over a year ago

This is true. Either make sure id is an int, or put quotes around it.

fret Over a year ago

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 :[

Mike Purcell Over a year ago

@fret: Putting quotes around ints is not the best approach, imo you should do something like $id = intval($id);

footy Over a year ago

@elijunior isnt PDO and Prepared statements the best approach? Why use this old method when php5 is such a common place?

Community · Accepted Answer · 2017-05-23 12:20:12Z

4

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)

edited May 23, 2017 at 12:20

CommunityBot

11 silver badge

answered Dec 7, 2011 at 2:37

ajreal

47.4k11 gold badges97 silver badges119 bronze badges

2 Comments

fret Over a year ago

The casting to int is definitely a good way to go when you can. I'll keep that in mind, thanks.

ajreal Over a year ago

@fret hi, bind parameter is proper way, and keep in mind the casting only works for integer/numeric column

Matt H · Accepted Answer · 2011-12-07 02:33:53Z

1

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.

answered Dec 7, 2011 at 2:33

Matt H

6,5502 gold badges32 silver badges34 bronze badges

Comments

footy · Accepted Answer · 2011-12-07 02:42:10Z

1

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']);
?>

answered Dec 7, 2011 at 2:42

footy

5,95713 gold badges54 silver badges98 bronze badges

Comments

Insecurefarm · Accepted Answer · 2011-12-07 02:33:38Z

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 7, 2011 at 2:33

Insecurefarm

4116 silver badges16 bronze badges

3 Comments

No Results Found Over a year ago

$_GET data will always be a string type.

footy Over a year ago

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

Yes Barry Over a year ago

is_numeric() would still result to true, though.

Collectives™ on Stack Overflow

Apparently there is an SQL injection bug in my PHP code

5 Answers 5

4 Comments

2 Comments

Comments

Comments

3 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

4 Comments

2 Comments

Comments

Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related