4

A newbie question here.

I've just read through a short introduction to SQL injection on http://www.breakthesecurity.com/2010/12/hacking-website-using-sql-injection.html?m=1

It says to find a GET request path on a PHP application to see if it is vulnerable or not.

I have a website, with the following query URL.

http://example.com/get_stuff?query=hello&limit=50&offset=0

I'm using a raw query, so according to the site, I'm assuming that my website IS vulnerable to SQL injection.

To test this, I try this.

http://example.com/get_stuff?query=hello&limit=50'&offset=0

(adding a single quotation mark after limit=50)

This prints out the error - as expected and as the website has suggested - meaning that my query is vulnerable.

Now, to safe-guard this, I want to make sure I "escape" the quotes in my queries. I'm using a PHP framework called CodeIgniter, in which there is a function called $this->db->escape().

So I use this "escape" function on the value 50, but now it returns an error saying that "50" is not a valid integer.

Fair enough, the "LIMIT" condition should only receive an integer value.

But my question is, is my query now "SQL-injection" safe?

Improve this question

4 Answers 4

Reset to default
6

Your query is first-order-injection maybe-just-maybe-safe, depending on how CodeIgniter improved from the last time I used it.

Let's be clear on this: no amount of sanitization will prevent you from SQL injection. The true "best fix" for PHP is parametrization, which allows you to take the variables out of the query. Failing that, however, proper sanitization can help. Last time I checked (three years ago), CodeIgniter used mysql_real_escape_string which, while a valid deterrent for most script kiddies, will not stop a seasoned hacker.

The best thing to do is to read up on how CodeIgniter actually does the sanitization/parametrization. If it is just addslashes, avoid it like the plague. There are plenty of other ways to perform SQL injections, by the way - for instance, you can juke character encoding to pass quotes through on most addslashes/mysql_escape_string setups. There are plenty of tutorials on the matter.

Improve this answer
3
  • Technical point: if you are sure that you only need integers, typecast to int to sanitize. $var = (int)$var; is more efficient and faster than mysql_real_escape_string, and is safe (cannot be used for other, more devious, purposes).
    – Sébastien Renauld
    May 8, 2013 at 0:15
  • You definitely want to parametrize all of your SQL. If you can do this in all of your SQL by default in your data access layer code (i.e. in a framework) that's the best. There's no reason, ever, not to write parametrized queries.
    – John
    May 10, 2013 at 15:08
  • @John: old habits die hard. Also, until the PHP team actually gets enough balls to remove the mysql_ list of function... Non-parametrization will persist.
    – Sébastien Renauld
    May 10, 2013 at 15:10
6

No, escaping quotes/double quotes doesn't guarantee, that your web application is not vulnerable. It also depends on SQL-queries you used. I have already seen a lot of examples with addslashes() and escape()-like functions where people do like this:

SELECT * FROM a WHERE id > addslashes(user_input)

This is of course vulnerable, because you don't need a single quote/double quote to perform SQL Injection. It's good to mention, that in same rare cases, addslashes could by bypassed [1].

The most recommended solution to your SQL-Injection problem is forget about string concatenation while creating SQL statements and take a look at parametrized queries/prepared statements. Speaking of CodeIgniter, you should also be interested in Active Record class.

If you are 100% sure, that user always has to type integer (like in your limit variable), then you can try casting: 

$limit = (int)$_GET['limit']

Using casting that way protects you from SQL-Injection ('cos there is no way to put non-integer value into your SQL query).

Oh, and don't forget that other variables could be vulnerable. So make sure to check query, offset and any other variable, which user controls. It's also a good practice to check the behaviour of the web-application, when it gets unexpected data - in this case could be an Array instead of string: get_stuff?query[]=hello&limit[]=50&offset[]=0

Improve this answer
4

What most people don’t understand is that SQL string escape functions are only intended to be used for values that are designated for SQL string literals. This means, you can only put such values inside a SQL string literal like '…' or "…".

These string escape functions are used to avoid user provided input being interpreted as something other than a string value by mistake. This is done by replacing the delimiting quotes with special escape sequences so that they are not interpreted as the ending delimiter but as a literal quote.

Now if your value is intended to be an integer value in SQL like LIMIT requires, escaping the value with string escape functions doesn’t work as it’s obviously not a string literal but an integer literal.

Since it’s not a string, there are no delimiting quotes that needed to be bypassed by the attacker. And as the injection happens in the LIMIT clause, an injected 50 UNION SELECT … might be possible to successfully exploit the vulnerability.

To fix this, make sure the values provided by the user are what you’re expecting. In case of an integer value, check if it’s actually an integer value, either actually a value of the type integer or a valid string representation of the integer value.

Additionally, there are libraries that provide a simple interface to parameterized statements and/or prepared statements where the statement and the parameter values are handled separately and the values automatically get converted to the proper type.

In your case, have a look at How to prevent SQL injection in PHP?

Improve this answer
-3

For non integer values I have been doing this, it may not be the fastest but it has worked for me... preg_replace("/[\"'%()@$.!&?_: #\/-]/","", mysql_real_escape_string($_GET['var']));

While i have never seen anybody suggest using preg_replace I see absolutely no way to get around it to perform an injection attack

Improve this answer
12
  • 2
    I don't think you understand the point of escaping input. Further more, this function would not prevent the sql injection mentioned in this post. (select Password from mysql.user) , another payload would be: union select password from users or simply sleep(30).
    – rook
    May 8, 2013 at 4:25
  • 2
    Actually this is much worse, the preg_replace() makes the mysql_escape_string() vulnerable to sql injection! It allows the attacker to have a backslash at the end the string! Every use of this function is vulnerable to SQLi.
    – rook
    May 8, 2013 at 4:28
  • 1
    how can you pass those arguments when you will not be allowed any spaces?
    – Nisan
    May 8, 2013 at 4:40
  • 1
    @Nisan: character encoding jukes or entities. A space can be seen by MySQL as 0x20. Assuming the hacker can get out of quotes...Your space protection is useless.
    – Sébastien Renauld
    May 8, 2013 at 14:12
  • and how would they 'get out of quotes'? might as well teach me... here is a test page: ragnargray.com/index.php?user=test&pass=test .. I'd like to see
    – Nisan
    May 8, 2013 at 16:12

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .