[SOLVED] Would this be open to SQL injection?

[lanmonkey]

Lots of issues surrounding sanitisation have been discussed thus far in this thread but I think that in this instance all you need to do is 2 things:

 

1> if you know that id should always be a number (an integer to be exact) then 'type cast' it which will force the value if $_GET['id'] to be an integer regardless of what type is originally.  Any non-numerical characters will be chopped off.

 

2> use mysql_real_escape_string before putting the value of $_GET['id'] anywhere near a database, this will simply escape (put a slash in front of) any dangerous SQL characters.

 

Heres all you need:

 

 

$cat_id = (int) mysql_real_escape_string($_GET['cat_id']);

[Mchl]

See the following for the source of my confusion:

<?php

if($str = "4" > 0){
echo "1st woop";
}

if(is_numeric($str)){
echo "2nd woop";
}

?>

 

Oh my! That's a nice one XD

 

[aschk]

I know, great isn't it!

For any of those reading this, i shall explain what is happening, in the "if" construct

$str = 4 > 0

is NOT worked out as you might want (or expect) due to the order of precedence of the operators in question.

It actually works out something like this:

4 > 0 ...is true

$str = true

if(true)... it worked

 

Thus, the problem in the my brilliant bit of php. In order to correct the issue i should have used parenthesis, e.g.

<?php

if(($str = "4") > 0){
echo "1st woop";
}

if(is_numeric($str)){
echo "2nd woop";
}

?>

 

et voila, now we have expected behaviour

[KevinM1]

Lots of issues surrounding sanitisation have been discussed thus far in this thread but I think that in this instance all you need to do is 2 things:

 

1> if you know that id should always be a number (an integer to be exact) then 'type cast' it which will force the value if $_GET['id'] to be an integer regardless of what type is originally.  Any non-numerical characters will be chopped off.

 

Are you sure that non-integers will be truncated and not converted to their ASCII value?  I don't have time to test it myself at the moment, but that immediately came to mind when I read this.  Probably because I've been spending too much time in C-family-land lately.

 

If I know that the range of acceptable incoming URL query values is small, I tend to create a whitelist of those acceptable values and test against that.  Anything not on the list causes either an error message or (in most of my cases) the user to be redirected to the homepage.

 

Regardless, the OP should be ready to handle any errors that stem from an invalid URL query value.

[JonnoTheDev]

In terms of redirection after bad url params you are better using a 404 header rather than redirecting to lets say the home page. Ive seen (and done) it before where you can create a page of spoof links that contain bad params and end up all redirecting to a certain page. Google comes along and sees all links hitting the same page and you end up in the supplimental results for duplicate content. This is just a note if your site requires SEO treatment of course.

[toyfruit]

Guys (and/or gals),

This has been really informative and useful, thankyou. Thanks very much for the code examples too, its really helped to see things in context.

[lanmonkey]

 

Are you sure that non-integers will be truncated and not converted to their ASCII value? 

 

positive, try the following:

 

$var = '12ab34';

 

echo (int) $var; //will output '12'