PHP $_GET['id'] and security

Question

I have links on my webpage like this: http://test.com/index.php?function=news&id=88

So whenever I put a ' after 88, I get the following error: Warning: mysql_fetch_row() expects parameter 1 to be resource, boolean given in ... line 588

So I read about mysql_real_escape_string(), but I'm getting the ID not posting and I have no clue how should I prevent getting this error.

function news()
{
$query = mysql_query("SELECT * FROM news WHERE id=".$_GET['id']."");
while($news = mysql_fetch_row($query)) 
{
    ...
}
}

It means that something in your code is taking $_GET['id'] and using it directly in a query. At best, it can stop your queries working. At worst, it will let someone run their own SQL code on your server. — andrewsi, Jul 09 '13 at 16:34
Use a prepared statement instead of trying to escape. See http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php — MrCode, Jul 09 '13 at 16:35
[The Great Escapism (Or: What You Need To Know To Work With Text Within Text)](http://kunststube.net/escapism/) — deceze, Jul 09 '13 at 16:36
Good thing you didn't try to go to: `http://test.com/index.php?function=news&id=0; DROP TABLE news; --` :-P — gen_Eric, Jul 09 '13 at 16:37
@RocketHazmat that won't work because multiple queries is not supported, other SQL injections will work though. — MrCode, Jul 09 '13 at 16:39
@MrCode: How about: `http://test.com/index.php?function=news&id='' OR id IN (DROP TABLE news); -- `? :-) I know that multiple queries won't work, I was just pointing out the injection. :-P — gen_Eric, Jul 09 '13 at 16:40
@RocketHazmat still won't work, you can't have `drop` inside an `IN` clause, also you don't need the the `; --` because OP's value isn't quoted in the query, you don't need to kill any remaining quotes. — MrCode, Jul 09 '13 at 16:45
@RocketHazmat :) you make the right point though that that SQLi is there. The best you can do with that SQLi is extract the entire database through it. It would take some time but can be done and would be just as devastating as dropping it, if there's interesting/sensitive data. — MrCode, Jul 09 '13 at 17:25

score 5 · Answer 1 · edited Jul 09 '13 at 16:43

5

The easy way is to cast the id to integer, if the id is an integer that is:

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

But it's strongly recomended to use pdo or mysqli with prepared statements:

http://php.net/manual/en/book.pdo.php

http://php.net/manual/en/book.mysqli.php

edited Jul 09 '13 at 16:43

Fosfor

331
2
3
15

answered Jul 09 '13 at 16:34

lexmihaylov

717
3
8

Well, it works, at least I don't get errors anymore. – fame Jul 09 '13 at 16:40

score 1 · Answer 2 · answered Jul 09 '13 at 16:38

1

You can do a redirect whenever mysql_fetch_row() don't return anything (i.e. because there is no id 89)

Something like:

if (!$row = mysql_fetch_row($result)) {
    header(Your error page);
}

answered Jul 09 '13 at 16:38

Fosfor

331
2
3
15

The problem is SQL injection. He entered `88'` as the ID, and that broke the query. – gen_Eric Jul 09 '13 at 16:39

George Cummins · Answer 3 · 2013-07-09T16:44:19.497

Your code assumes that the query was successful without checking. For debugging purposes, add an 'or die(mysql_error())' line to the end of the mysql_query() statement.

$query = mysql_query("SELECT * FROM news WHERE id=".$_GET['id']."") or die( mysql_query() );

For more robust error handling in production applications, check the value of $query and log an error if it is false.

if (false === $query ) {
    // Log error and/or notify an administrator
} 
else { 
    while($news = mysql_fetch_row($query)) ...

As pointed out in other answers, you should ensure that the value of the id parameter is an integer since your query assumes that it will be. You can do this by casting:

(int)$_GET['id']

or via more robust type checking

if ( !is_numeric( $_GET['id'] ) ) {
    // Take appropriate action
}
else {
    // Create and execute the query

The problem is SQL injection. He entered `88'` as the ID, and that broke the query. — gen_Eric, Jul 09 '13 at 16:44
I've added a reference to that error to the question. I believe that the rest is relevant because it will help to reveal such errors in the future. — George Cummins, Jul 09 '13 at 16:45

score 0 · Answer 4 · answered Jul 09 '13 at 16:41

Warning: mysql_fetch_row() expects parameter 1 to be resource

This means the the $result = mysql_query(....); call you made before the mysql_fetch_row() failed and resulted FALSE instead of a Resource ( i.e. a handle to the query result );

Look at the query, post it if possible, that is where your problem is.

PHP $_GET['id'] and security

4 Answers4