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?

devblack
  • 11
  • 4
ericbae
  • 149
  • 1
  • 1
  • 3

4 Answers4

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.

Sébastien Renauld
  • 685
  • 3
  • 8
  • 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 08 '13 at 00: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 '13 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 '13 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

p____h
  • 1,527
  • 7
  • 11
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?

Community
  • 1
Gumbo
  • 2,003
  • 1
  • 13
  • 17
-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

Nisan
  • 25
  • 2
  • 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 08 '13 at 04: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 08 '13 at 04:28
  • 1
    how can you pass those arguments when you will not be allowed any spaces? – Nisan May 08 '13 at 04: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 08 '13 at 14:12
  • and how would they 'get out of quotes'? might as well teach me... here is a test page: http://ragnargray.com/index.php?user=test&pass=test .. I'd like to see – Nisan May 08 '13 at 16:12
  • @Nisan Actually getting out of the quotes is the **easy part**, because you are giving the attacker a backslash which is as dangerous as a quote mark (after-all that is the point of escaping!). You are also giving the attacker `()` because there is a bug in your regex. [Exploiting sql injection without spaces is nothing new](http://websec.wordpress.com/2010/03/19/exploiting-hard-filtered-sql-injections/). The problem I am having is actually terminating the query. Security experts use parametrized queries, which avoids this problem. – rook May 08 '13 at 17:25
  • Remove the preg_replace it only makes this system less secure because it undermines your ability to escape control characters. You even accidentally modified this function to produce single quotes `'`... You are attempting to make a change to security function used by everyone and you are breaking it entirely. Why not rely upon a proven security system? – rook May 08 '13 at 17:26
  • I don't see where you have gained access?? I don't think you've even 'got out of the quotes'... – Nisan May 08 '13 at 17:44
  • I do see where the equal sign(=) and the vbar (|) should be added, but I'll wait on you to break what's there because I am not so sure it can be done. Add the = and | to the list and i'm pretty sure it can't be. but ... – Nisan May 08 '13 at 17:55
  • so you can't crack it can you... anybody? I really don't think I needed rep taken away for this... – Nisan May 08 '13 at 22:24
  • @Nisan: read the article that Rook provided. Your filter is in the list. – Sébastien Renauld May 10 '13 at 12:40
  • How does this: if(if((name)like(0x61646D696E),1,0),if(mid((password),1,1)like(0x61),id,0),0);%00 ... pass my filter? – Nisan May 11 '13 at 14:35