Injection Help

[Swarfega]

Hey so I've got this small "Game-Corner" part of my website, loading the Games through $_GET id=number, and it's currently a possible spot to do an SQL injection, and I want to stop this, but I can't.

 

I've personally tried to inject it to see how I would stop it myself, but no matter how hard I try, I can't manage to use the vulnerability properly.

 

 

http://domainname.nu/game.php?id=8 <<< Results in loading game

 

http://domainname.nu...e.php?id=8' <<< Results with:

 

Warning: mysql_num_rows(): supplied argument is not a valid MySQL result resource in /public_html/user/game.php on line 246
Invalid Game ID. Click Here to continue.

 

Query + Line 246:

 

$sql = "SELECT * FROM games WHERE id='".$_GET['id']."'";
$query = mysql_query($sql);
$rowcount = mysql_num_rows($query); [b]<<<< Line 246[/b]

 

 

I know using PDO is much more efficent, but I'm still learning PDO and I'd like a temporary fix for the injection issue until I'm more comfortable with PDO.

 

 

Any ideas?

Edited January 15, 2013 by Swarfega

[Swarfega]

$l = 0;
switch($_GET['id']) {
case 1:
$l = 1;
break;
case 2:
$l = 2;
break;
case 3:
$l = 3;
break;
case 4:
$l = 4;
break;
case 5:
$l = 5;
break;
case 6:
$l = 6;
break;
case 7:
$l = 7;
break;
case 8:
$l = 8;
break;
case 9:
$l = 9;
break;
case 10:
$l = 10;
break;
case 11:
$l = 11;
break;
case 12:
$l = 12;
break;
}


$sql = "SELECT * FROM games WHERE id=$l";
$query = mysql_query($sql);
$rowcount = mysql_num_rows($query);

 

Doing this stops the injection possibility but isn't a very smooth way to go by it I'm guessing.

Edited January 15, 2013 by Swarfega

[stijn0713]

i'm not a specialist but i think a normal mysql_real_escape_string() should be quite ok.

 

foreach ($_POST as $key => $value) {

if(get_magic_quotes_gpc()) $_POST[$key]=stripslashes($value);

$_POST[$key] = mysql_real_escape_string($value);

}

[Christian F.]

No need for that switch-case block, as this is sufficient:

$l = intval ($_GET['id']);

 

Also, mysql_real_escape_string () is the wrong function to use, as this is not a string value. Not to mention the shotgun-approach, to using it on the entire $_POST array. Escaping via MRES should only be done the very instant you add the values to the SQL query, so that you do not modify variables you will (or might be) using later on.

[Swarfega]

No need for that switch-case block, as this is sufficient:

$l = intval ($_GET['id']);

 

Also, mysql_real_escape_string () is the wrong function to use, as this is not a string value. Not to mention the shotgun-approach, to using it on the entire $_POST array. Escaping via MRES should only be done the very instant you add the values to the SQL query, so that you do not modify variables you will (or might be) using later on.

 

I have never heard of intval Method before, I'll look it up!

 

Also, I had the same issue with Profile.php?uid=XX (Total Reg Count over 1000 so 1000 possible SQL Injections) but on this one I went with an experiment:

 

$_GET['uid'] = str_replace("'", "", $_GET['uid']);

 

So it replaces all ' with nothing-ness, and it's working. Is that an OK way to treat the Profile in this matter?

 

 

I'm going to do some reading on the intval now!

[cpd]

Before Christians suggestion I'd validate the integer as intval() returns 0 on failure which could, in theory, give unexpected results.

 

mysql_real_escape_string() is most definitely not sufficient security. In this case it shouldn't matter though as the integer shouldn't be wrapped in apostrophes.

 

And to your last post Swarfega, no that's a bad idea. Casting is what you should be doing and is effectively what intval() does.

Edited January 15, 2013 by cpd

[Swarfega]

Before Christians suggestion I'd validate the integer as intval() returns 0 on failure which could, in theory, give unexpected results.

 

mysql_real_escape_string() is most definitely not sufficient security. In this case it shouldn't matter though as the integer shouldn't be wrapped in apostrophes.

 

And to your last post Swarfega, no that's a bad idea. Casting is what you should be doing and is effectively what intval() does.

 

Yes, I read up a little bit on intval and I get what you mean.

 

It's working on most of the site, but when it comes to the individual-user Albums displaying enlarged-pictures, it does not.

 

&targetid=x&albumid=x&imageid=18

^^^^^^ Displays Image properly

 

&targetid=x&albumid=x&imageid='18

^^^^^^ Does not display any image

 

and instead gives this

 

Warning: mysql_num_rows(): supplied argument is not a valid MySQL result resource in public_html/user/album/visa.php on line 561

Warning: mysql_num_rows(): supplied argument is not a valid MySQL result resource in public_html/user/album/visa.php on line 566

Warning: mysql_fetch_array(): supplied argument is not a valid MySQL result resource in public_html/user/album/visa.php on line 637

Using $imid = intval($_GET['imageid'])

before the query and using $imid in the query.

Edited January 15, 2013 by Swarfega

[cpd]

If you echo $imid you'll notice it equals 0 (zero). This is because '18 (apostrophe 18) cannot be evaluated to a number therefore intval() fails and returns 0.

 

I assume you then attempt to query your database using 0. This is why I said you need to validate before using intval()

Edited January 15, 2013 by cpd

[Swarfega]

Thanks, I got it working after doing what you said. Was more or less concerned about someone injecting in order to release all the database data publically on some random forum.

 

Going to spend my night implementing this into a Site of 357 PHP Files, woo, funtimes!

[Christian F.]

Normally the validation is the query itself, as AUTO_INCREMENTS always start with 1. Even if you do not have a row with the ID of 0, it should still return a valid result (of 0 rows).

The fact that you get errors tells me that there's something else that's wrong in your code, but since you haven't posted the part of the script that uses the $imid variable, we can't help out on that.

[stijn0713]

Sorry for my mistake btw...