1

I've a website that hacked today. Server logs returned something like this as hacker's tries:

www.site.com/notifications.php?PID=7&id=999999.9%20union%20all%20select%20%28select%20distinct%20concat%280x7e%2C0x27%2Cunhex%28Hex%28cast%28schema_name%20as%20char%29%29%29%2C0x27%2C0x7e%29%20from%20%60information_schema%60.schemata%20limit%201%2C1%29%2C0x31303235343830303536%2C0x31303235343830303536%2C0x31303235343830303536--

But I've used mysql_real_escape_string() in my code: 

if (isset($_GET['id']) && $_GET['id'] != '') {
   $id = mysql_real_escape_string($_GET['id']); 
} else {
   $id = '';
}

if ($id == '') {
   $stmt = "SELECT * FROM tbln13 ORDER BY id DESC"; 
} else {
   $stmt = "SELECT * FROM tbln13 WHERE id = $id";
}

$NewsResult = mysql_query($stmt) or die (mysql_error());

Why my website could not prevent this attack?

Improve this question
7
  • 4
    This is common sense. You're using deprecated functions, no prepared statements. All that's needed is bobby tables Commented Dec 27, 2013 at 19:29
  • 4
    WHERE id = $id";.... not quoted, so you're not treating it as a string; yet you're still escaping it as though it was a string Commented Dec 27, 2013 at 19:30
  • mysql_real_escape_string is for, as the name says, escaping special characters in a string, it will not make integers safe Commented Dec 27, 2013 at 19:38
  • 1
    This is a wake-up call to anyone still using the awful mysql_query interface. Throw that code in the garbage before it burns you again and write it properly using PDO and placeholders. Commented Dec 27, 2013 at 19:51
  • You have other questions you posted that's related to PDO; why didn't you stick with that instead of using mysql_* functions? THAT, is what I don't get. It was only a matter of time till this happened. This, being one of those questions --- Now, you really need to read this post carefully => stackoverflow.com/questions/60174/… and this one => owasp.org/index.php/Top_10_2013-Top_10 and STOP using mysql_* functions, once and for all. Commented Dec 27, 2013 at 22:00

5 Answers 5

Reset to default
8

Because escape_string add slashes and such to quotes. You didn't have any quotes in your query, or the string they submitted.

Your query doesn't have a STRING in it, it appears to expect an int. If you expected an integer, you should have verified it was an int, or forced it to an int, before using it in a query. Escaping a value as a string, then using it as an int, won't work.

Switch to prepared statements in MySQLi or PDO.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

The sql injected query looks like this

SELECT * FROM tbln13 WHERE id = 999999.9 
union all select 
 (select distinct concat(0x7e,0x27,unhex(Hex(cast(schema_name as char))),0x27,0x7e) 
  from `information_schema`.schemata 
  limit 1,1), 
 0x31303235343830303536, 0x31303235343830303536, 0x31303235343830303536--

as you see, you were injected because you have just allowed this!

You expected a number but you didn't check for it! So you got the number and something more.

You should have checked the $id variable for what you expected, which is the number. This is what I would use:

if (!preg_match('/^\d+$/', $id))
    die("ERROR: invalid id"); // error, don't continue
Improve this answer

1 Comment

why the downvote? The explanation is not clear? This method is 100% correct.
1

Use prepared statements, that will, in most cases, prevent SQL injections.

A simple and comprehensible guide to prepared statements can be found in this website:

Bobby Tables

More over you should stop using MYSQL, it's outdated and will be removed in future implementations. Use MySQLi or PDO instead.

Improve this answer

Comments

1

Because your escaped variable is not a string therefore it is not inside quotes in your query. If you want a quick fix you can change your query to:

$stmt = "SELECT * FROM tbln13 WHERE id = '$id'";

It is not standard use for numeric comparison but should work.

Improve this answer

3 Comments

A "quick fix" is what created this problem in the first place. The last thing that's needed is another one.
Sometimes fixes like this can save the day. Especially on PHP since it is not a strongly typed language...
Do not take chances with SQL queries. A single unescaped parameter could destroy your company or career. Quick fixes do not belong anywhere near this kind of code.
0

As mentioned by others, you should ditch deprecated mysql_* functions and instead used prepared statements via mysqli or PDO.

Even then you should also be validating your input, because just using prepared statements will not help you identify whether you have input values that are valid. You would ideally make sure all your input is valid before even attempting a prepared statement. In this case, this validation could be as simple as this:

$id = filter_input(INPUT_GET, 'id', FILTER_VALIDATE_INT);
if (false === $id) {
   // you do not have a valid integer value passed. Do something.
} else {
   // continue with your prepared statement
}
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.