[SOLVED] Would this be open to SQL injection?

[toyfruit]

Hi all, I'm new to PHP and have heard about SQL injection and just wanted to see if the kind of thing I am working on should be protected, or if it isn't going to cause me any problems, and if so, what would you suggest to reduce the risk of injection?

 

Basically, I am passing an id through the URL which is then caught on the next page, put into a variable and then that variable is used to grab results from a database. The id should always be a number, but I currently don't have anything to check for that.

 

So for example the URL might be:

www.mysite.com/mypage?cat_id=4

 

The code on the collecting page would be something like:

 

 <?php
   // get the category id from the URL
   $cat_id = $_GET['cat_id'];
$result = mysql_query("SELECT * FROM category WHERE id = $cat_id", $connection);
while ($row = mysql_fetch_array($result)){
	echo $row["category_name"] . "<br />";
}	
?>

[redarrow]

any id in a url is sql injection........................

 

put the id in a session ok............

[JonnoTheDev]

Yes

$cat_id must be cleaned.

 

In the least use mysql_real_escape_string($cat_id);

[Mchl]

At least check if cat_id is numeric.

 

Best thing would be to use prepared statement, but you'd need to shift from mysql extension to mysqli extension.

 

<?php
  // get the category id from the URL
  if (is_numeric($_GET['cat_id'])) $cat_id = $_GET['cat_id'];
$result = mysql_query("SELECT * FROM category WHERE id = $cat_id", $connection);
while ($row = mysql_fetch_array($result)){
	echo $row["category_name"] . "<br />";
}	
?>

[redarrow]

dosent matter what u do if the url has got a visable id there still a chance off mysql injection......

[toyfruit]

OK thanks ... so if there is a risk of injection by using an id in a url, then how do I disguise the id in the first place? I need to pass a value from one page to another without there being a risk of malicious injection.

I have a session open, so could I store that value in the session, and if so how would I go about doing that?

 

Thanks for all your responses so far, its been very useful

[aschk]

Unfortunately redarrow i'm unconvinced that ANY id in a url is a viable target for SQL injection.

toyfruit there is nothing wrong with what you have done, just make sure to escape the input, so as neil.johnson suggested use mysql_real_escape_string on your user input (any $_GET/$_POST param).

 

Thus i would expect your script to look like this:

<?php
  // get the category id from the URL
  $cat_id = mysql_real_escape_string($_GET['cat_id']);
  if($cat_id > 0)
   $result = mysql_query("SELECT * FROM category WHERE id = $cat_id", $connection);
   while ($row = mysql_fetch_array($result)){
     echo $row["category_name"] . "<br />";
   }
  }
?>

 

I challenge redarrow to break this.

 

Also, sanitizing your input (as Mchl suggested) is useful as there's little point in running your SQL unless the cat_id that is being sent is a numeric, therefore using is_numeric() or > 0 will perform this check. > 0 is probably a better check as you don't want a negative cat_id

[JonnoTheDev]

You dont need to disguise the url param but using a mod rewrite does help as it limits what characters are accepted in the url param. Just remember the golden rule: always filter input and escape output

[aschk]

I'd like to reiterate what neil has just said.

1) Filter input

2) Escape output

 

These rules are simple, and a highly effective mechanism for protecting your site.

[toyfruit]

This has been really useful - thankyou.

 

I think I understand what you mean by 'filter input' (I assume this is what we have just discussed), but what do you mean by 'escaping output'? I realize this may be slightly off topic, but would be interested in knowing what you're referring to.

[redarrow]

if i no another id i want to get to just use the cat_id with database inputs

 

<?php
  // get the category id from the URL
  $cat_id = mysql_real_escape_string($_GET['cat_id']);
  if($cat_id > 0)
   $result = mysql_query("SELECT * FROM category WHERE id = $cat_id", $connection);
   while ($row = mysql_fetch_array($result)){
     echo $row["category_name"] . "<br />";
   }
  }
?>

[Mchl]

I've seen recently a somewhat intriguing way of protecting urls from sql injection

http://myhost/script.php?id=1&idmd5=c4ca4238a0b923820dcc509a6f75849b

 

 

And then in the script.php

if (md5($_GET['id']) == $_GET['idmd5']) {
// OK
}

 

It's pretty clever I must admitt, but I don't think it's really needed...

 

[edit]

 

After a moment thought. It must've been somewhat more sophisticated tha what I've just written (because what I've written is just some obscurity). I just don't remember details...

[redarrow]

md5 correct best use mod_rewite theo more powerfull

[aschk]

Red i'm not 100% sure i know what you mean? Can you elaborate with some code as to how you would break this, or what input you would supply that would break this?

 

 

if i no another id i want to get to just use the cat_id with database inputs

 

<?php
  // get the category id from the URL
  $cat_id = mysql_real_escape_string($_GET['cat_id']);
  if($cat_id > 0)
   $result = mysql_query("SELECT * FROM category WHERE id = $cat_id", $connection);
   while ($row = mysql_fetch_array($result)){
     echo $row["category_name"] . "<br />";
   }
  }
?>

 

As a side note: surely the md5 method is m00t? It's just the md5 of the ID supplied, so surely if you're attemping injection you would just supply the md5 of your id argument? e.g.

http://myhost/script.php?id=(SELECT COUNT(*) FROM users)&idmd5=d4078dbb41cbddf476109a21e9c09005

 

All i had to do was calculate the md5 of my argument...

[Minase]

here is what i do use for checking numeric ,SQL injection free

 

function numeric($str)
{
    return (!ereg("^[0-9\.]+$", $str)) ? false : true;
}

$value = $_GET['value'];

if (numeric($value) == 1) {
$test = $_GET['value'];
} else {
$test = // whatever you want;
}

 

so aschk do you think this is vulnerable to SQL injection?

i dont think

no monkey business

[Mchl]

 

As a side note: surely the md5 method is m00t? It's just the md5 of the ID supplied, so surely if you're attemping injection you would just supply the md5 of your id argument? e.g.

http://myhost/script.php?id=(SELECT COUNT(*) FROM users)&idmd5=d4078dbb41cbddf476109a21e9c09005

 

All i had to do was calculate the md5 of my argument...

 

In the form I presented it, it's a moot. I can't remember details of what it was like

[aschk]

Ah dang, maybe if you find that post again you can link it here so we can all have a look. Sounds interesting though, providing the hash of something. Maybe it just provides the hash based on a random session variable and not the ID? Like salting the value?

 

Minase, very sexy, nice method. Complex way of doing > 0 though , BUT highlights well, the usage of filtering.

 

Also, as a side note is_numeric() only works on numerics (although not entirely obvious), as it doesn't do any type conversion for you. So in actual fact I believe all $_GET parameters are strings, so is_numeric($_GET['cat_id']) would in fact fail is the value was 5 ...

[Minase]

//edit lol misunderstood what you told

 

nvm

[Mchl]

is_numeric —  Finds whether a variable is a number or a numeric string

 

One problem with is, that

is_numeric("0123");

returns true

But try

INSERT INTO table (id) VALUES (0123);

 

[xoligy]

Sorry to ask this but its on the same kind of situation is this any good at stopping sql injections?

 

if (!$cgi['page']){
         $cgi['page']=is_numeric(1);
}

or should i add mysql_real_escape_string on the front of $cgi['page']?

[Mchl]

is_numeric doesn't work like that...

 

And what do you want to insert into database?

[xoligy]

aaah ok nevermind then, and i just stumbled on something else i gotta figure out after ive had some kip i think

[toyfruit]

Just wondered if someone could quickly explain the 'escape output' comment from earlier? I understand the 'filter input' part now, but would like some clarification on 'escape output'.

 

Thanks all

[aschk]

Escaping output is as simple as turning all user generated content into a safe format (to avoid XSS).

In other words:

<?php
// Escape our output.
echo htmlentities($output);
?>

[aschk]

Mchl: i was doing something weird with is_numeric() earlier which for one reason or another made me think that is_numeric only too numerics (not strings)... DOH!

 

See the following for the source of my confusion:

<?php

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

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

?>