Live data from Hacker News

SQL Injection Galore

github.com

11–20 of 88 posts

Re: SQL Injection Galore

#12

We actually clean the GET and POST arrays in an include when the page gets requested, if you need anything unescaped you specifically have to request it from a different array. The code looks something like this: $_GET = array_map ( 'strip_tags', $_GET ); $_GET = array_map ( 'mysql_real_escape_string', $_GET ); Although there is a bit more to it than just this.

Not convinced this is a good idea.

Your devs will learn to trust $_GET "because it's cleaned by our include".

They'll never sanitise input themselves "because it's cleaned by our include".

Code that they write for other projects will trust $_GET "because it's cleaned by our include" - except it's not because this is a 3rd party script.

Code you import from other projects will be double-escaping.

Also mysql_real_escape_string is deprecated.

Stop treating SQL queries as strings. They're code. You wouldn't write code by concatenating strings with user input would you?

Use prepared statements.

edit: missed the part when you said that you clean $_POST as well. I was wondering "What do you do when you need to submit markup?" Now I know that it's magic. The $_REQUEST array is actually the unsanitised array, whereas $_POST is the sanitised one. Of course! Isn't it obvious!

Sigh.

Re: SQL Injection Galore

#13
Seems like it would be possible to create a "linter" for PHP and Ruby that would detect at least basic SQL injection vulnerabilities?

Re: SQL Injection Galore

#15

We actually clean the GET and POST arrays in an include when the page gets requested, if you need anything unescaped you specifically have to request it from a different array. The code looks something like this: $_GET = array_map ( 'strip_tags', $_GET ); $_GET = array_map ( 'mysql_real_escape_string', $_GET ); Although there is a bit more to it than just this.

This is exactly what NOT to do.

The mysql_real_escape_string part is what PHP did with its "magic_quotes" feature. This feature has been removed for good reasons :

- All your variables are cluttered with \' everywhere, so you have to unescape them before doing anything other than using the variable in a SQL query. And re-escape them after that. You will forget to do it.

- It doesn't protect you if you expect the variable to be numeric, and use it in a query without enclosing it in quotes:

    // do not do this
    mysql_query("SELECT do_not_do_this WHERE x = $var");
    // $var could be `0 OR 1 = 1`, even after escaping
- Also, you never know if a variable is actually escaped or not. e.g. in the following code, is "$var" escaped ?

    $var = get_data_from_db_or_other_source();
    // do not do this
    mysql_query("SELECT do_not_do_this WHERE x = '$var'");
- You've "protected" yourself from sql injection and html injection (very wrongly, btw). What about shell injection, css injection, javascript injection, protocol injection ? Are you going to also apply escapeshellarg() on all $_GET variables ?

---

Also, you shouldn't be using strip_tags at all:

- It doesn't strips quote characters, so you would be vulnerable to html parameter injection, if you used a variable "sanitized" like this in a html parameter.

- It alters the data is some non-reversible way. What if you genuinely want to display (not render or "execute") some html code in a blog post on your web site ?

You should be using htmlspecialchars() instead.

---

What to do:

- escape, do not "filter" or "sanitize"

- escape data just in time, not ahead of time (e.g. escape your variable just before using it in a mysql query, or just before outputing it in a html document)

- prefer prepared statements

- use the right escape function depending for the context in which the variable is used: mysql_real_escape_string for mysql query, htmlspecialchars for html, escapeshellarg for command line arguments, etc.

Re: SQL Injection Galore

#16
post #15

We actually clean the GET and POST arrays in an include when the page gets requested, if you need anything unescaped you specifically have to request it from a different array. The code looks something like this: $_GET = array_map ( 'strip_tags', $_GET ); $_GET = array_map ( 'mysql_real_escape_string', $_GET ); Although there is a bit more to it than just this.

This is exactly what NOT to do. The mysql_real_escape_string part is what PHP did with its "magic_quotes" feature. This feature has been removed for good reasons : - All your variables are cluttered with \' everywhere, so you have to unescape them before doing anything other than using the variable in a SQL query. And re-escape them after that. You will forget to do it. - It doesn't protect you if you expect the vari…

> You should be using htmlspecialchars() instead.

Without trying to be too much of an arrogant arsehole, surely they should be using a proper language. That is the real core cause of the problem here isn't it?

Re: SQL Injection Galore

#17
post #12

We actually clean the GET and POST arrays in an include when the page gets requested, if you need anything unescaped you specifically have to request it from a different array. The code looks something like this: $_GET = array_map ( 'strip_tags', $_GET ); $_GET = array_map ( 'mysql_real_escape_string', $_GET ); Although there is a bit more to it than just this.

Not convinced this is a good idea. Your devs will learn to trust $_GET "because it's cleaned by our include". They'll never sanitise input themselves "because it's cleaned by our include". Code that they write for other projects will trust $_GET "because it's cleaned by our include" - except it's not because this is a 3rd party script. Code you import from other projects will be double-escaping. Also mysql_real_escap…

Yeah. It's just the wrong level of abstraction to deal with this. It's not even hard to do properly anyway.

Re: SQL Injection Galore

#18
post #15

We actually clean the GET and POST arrays in an include when the page gets requested, if you need anything unescaped you specifically have to request it from a different array. The code looks something like this: $_GET = array_map ( 'strip_tags', $_GET ); $_GET = array_map ( 'mysql_real_escape_string', $_GET ); Although there is a bit more to it than just this.

This is exactly what NOT to do. The mysql_real_escape_string part is what PHP did with its "magic_quotes" feature. This feature has been removed for good reasons : - All your variables are cluttered with \' everywhere, so you have to unescape them before doing anything other than using the variable in a SQL query. And re-escape them after that. You will forget to do it. - It doesn't protect you if you expect the vari…

Fair point, it's actually not my code and I've always been generally skeptical about its safety. I have found flaws in it before.

Thanks for the input, I'll look at changing it.

Re: SQL Injection Galore

#19
post #15

Earlier quoted context omitted.

This is exactly what NOT to do. The mysql_real_escape_string part is what PHP did with its "magic_quotes" feature. This feature has been removed for good reasons : - All your variables are cluttered with \' everywhere, so you have to unescape them before doing anything other than using the variable in a SQL query. And re-escape them after that. You will forget to do it. - It doesn't protect you if you expect the vari…

> You should be using htmlspecialchars() instead. Without trying to be too much of an arrogant arsehole, surely they should be using a proper language. That is the real core cause of the problem here isn't it?

Nothing specific to the language here. Mentioned functions do exactly what their names imply.

Just like ruby or python, PHP has great templating languages, support for prepared statements, etc.

Re: SQL Injection Galore

#20
post #19

Earlier quoted context omitted.

> You should be using htmlspecialchars() instead. Without trying to be too much of an arrogant arsehole, surely they should be using a proper language. That is the real core cause of the problem here isn't it?

Nothing specific to the language here. Mentioned functions do exactly what their names imply. Just like ruby or python, PHP has great templating languages, support for prepared statements, etc.

This is often said, but these problems are practically endemic in the PHP 'community'. I can't help but think that the lack of proper documentation or basic explanations is responsible.
Post reply on HN