Why is this SQL code insecure?

Question

I just had a friend of mine look over some code I'm using to get data from my database, and he tells me it's very unsecure and that SQL injection is serious shit.

Here's the code I'm using now:

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

He tells me that the solution is to change that code into:

$id = intval($_GET['id']);
$result = mysql_query("SELECT * FROM news WHERE id = $id") or die("err0r");

My code somehow (according to my friend) makes any user able to edit content in my database:

http://mydomain.com/?p=news&id=38;DROP TABLE tablename;

Could someone explain exactly what he means?

Thank you and have a pleasant day

Take a look here: http://stackoverflow.com/questions/332365/xkcd-sql-injection-please-explain — Swati, May 12 '11 at 15:54

score 4 · Answer 1 · answered May 12 '11 at 15:52

If you don't try to force id to an integer, anything could run on your database. DROP TABLE commands, for example. Of course, someone could still try to force a very large integer here and cause an overflow, I don't if that could do anything more than cause a relatively minor error.

See here: http://xkcd.com/327/ for a classic illustration.

As an aside, if you set up a test server, you could see what really happens with your code if you navigate to the URL http://mydomain.com/?p=news&id=38;DROP TABLE tablename;

Nothing bad at all - but that doesn't mean it's not an important thing to do. — Grim..., May 12 '11 at 15:58

score 4 · Answer 2 · answered May 12 '11 at 15:58

If you don't validate "id" and make sure it is an integer, someone could set id to (say) "1;drop table news;" which would get sent to the database as is and could possibly delete the "news" table. If you do a validation to make sure that id is an integer you should be fine. Also as an additional level of precaution you may want to make sure that the database user that the script uses only has the exact level of permissions needed to do what is needed -- nothing more. For instance, if your program has no intention of deleting tables, make sure that the database user does not have this privilege.

score 3 · Answer 3 · answered May 12 '11 at 15:53

That URL would make $id = '38;DROP TABLE tablename;' The first semi-colon would end the current query, causing the second one to be run - which would drop your table.

Wikipedia has a good explanation: http://en.wikipedia.org/wiki/SQL_injection

As does this: http://xkcd.com/327/ =]

You should also start thinking about what you'll do when you need input that isn't a number, so intval won't help. Having a function to sanatize your inputs is always handy - just don't forget to use it, and never trust anything!

score 3 · Accepted Answer · answered May 12 '11 at 16:01

Consider what would happen if someone posted http://www.mydomain.com?id=1; DROP TABLE news

Older versions of PHP tried to automatically protect against this sort of thing by automatically escaping all the input variables; ie by adding slashes to any quote characters in the input so that they wouldn't break a SQL query. This is no longer the default in current versions of PHP since it caused a lot of other issues.

However in your case, you haven't even quoted the variable in the SQL query, so even that protection wouldn't have helped you since a hacker wouldn't have needed to include any quotes to hack you.

The intval() solution will indeed help you in this specific case, but it won't help in others (eg if you need to handle a string variable).

The correct solution:

You should use the mysql_real_escape_string() function on all variables that are to be passed into the SQL query to prevent any possible hack attacks, like so:
```
$escaped_id = mysql_real_escape_string($id);
```
You should enclose all variables in the SQL string in quotes, like this:
```
$result = mysql_query("SELECT * FROM news WHERE id = '$escaped_id'");
```

By the way... This comic strip is a very popular thing to link to to demonstrate the problem. ;-)

I'm gonna try this, it seems like you know what you're talking about =) Thanks man. — MstrQKN, May 12 '11 at 16:19
mysql_real_escape_string is a good start, but it has problems, and probably shouldn't be used. More here: http://stackoverflow.com/questions/5414731/are-mysql-real-escape-string-and-mysql-escape-string-sufficient-for-app-secur — Grim..., May 12 '11 at 16:34
@Grim - indeed, but the *real* solution (rewriting the whole app using a different DB library such as PDO) is probably too big a leap to be suggesting for this question. — Spudley, May 12 '11 at 17:36

score -1 · Answer 5 · answered May 12 '11 at 15:53

-1

It makes sure that $id is a number.

answered May 12 '11 at 15:53

James C

14,047
1
34
43

Why is this SQL code insecure?

5 Answers5

Linked