Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

SQL Injection Galore

github.com

Post reply on HN
Print view View original

31–40 of 88 posts

Re: SQL Injection Galore

#31

Post by jtreminio »

hahainternet wrote: post #16
fooyc wrote: 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?

php isn't going to hold your hand, if you want to write awful code or don't know any better then god help you.

That said, all the mysql_* functions have been officially deprecated and we steer newbies to using pdo and named parameters.

Re: SQL Injection Galore

#33

Post by foogs »

my 2 cents: 1) At the first place, yes, it does look like these are sureshot SQL injections. 2) However, we are looking through just a tiny window. There could be filter chains executed long before this code that would sanitize the request parameters before they are consumed anywhere else in the codebase.

Re: SQL Injection Galore

#35

Post by orf »

readme wrote: post #30
user24 wrote: post #10

Earlier quoted context omitted.

To do it properly you'd want to use prepared statements[1] which requires a non-trivial, though not particularly complicated, syntax change. So your global actor would have to parse the PHP in a rather more intelligent way that just a string replace. You could just use mysql_real_escape_string [2] but that's less secure than prepared statements, and may break some things (eg if the code is relying on certain things n…

I know the PHP/PDO way to do it, is prepared statements. I'm not able to see why the parent's suggestion to just escape the strings is not a valid solution from a security perspective.

because you also have integer based SQL injection. Escaping strings isn't a complete fix.

Re: SQL Injection Galore

#36

Post by anoncow »

readme wrote: post #30
user24 wrote: post #10

Earlier quoted context omitted.

To do it properly you'd want to use prepared statements[1] which requires a non-trivial, though not particularly complicated, syntax change. So your global actor would have to parse the PHP in a rather more intelligent way that just a string replace. You could just use mysql_real_escape_string [2] but that's less secure than prepared statements, and may break some things (eg if the code is relying on certain things n…

I know the PHP/PDO way to do it, is prepared statements. I'm not able to see why the parent's suggestion to just escape the strings is not a valid solution from a security perspective.

>that's less secure than prepared statements, and may break some things (eg if the code is relying on certain things not being escaped, etc)

Re: SQL Injection Galore

#37

Post by [deleted] »

readme wrote: post #30
user24 wrote: post #10

Earlier quoted context omitted.

To do it properly you'd want to use prepared statements[1] which requires a non-trivial, though not particularly complicated, syntax change. So your global actor would have to parse the PHP in a rather more intelligent way that just a string replace. You could just use mysql_real_escape_string [2] but that's less secure than prepared statements, and may break some things (eg if the code is relying on certain things n…

I know the PHP/PDO way to do it, is prepared statements. I'm not able to see why the parent's suggestion to just escape the strings is not a valid solution from a security perspective.

[deleted]

Re: SQL Injection Galore

#38

Post by user24 »

orf wrote: post #35
readme wrote: post #30

Earlier quoted context omitted.

I know the PHP/PDO way to do it, is prepared statements. I'm not able to see why the parent's suggestion to just escape the strings is not a valid solution from a security perspective.

because you also have integer based SQL injection. Escaping strings isn't a complete fix.

Yep. In fact the typical SQL injection example is " 0 OR 1=1 ".

Re: SQL Injection Galore

#39

Post by dave1010uk »

harrytuttle wrote: post #24

I have commit hooks on our repository that look for things like this and prevent the user committing it! We've got 30 odd rules so far that have saved us from all sorts of pain from exception swallowing to adding test ignores as well.

Could you share some of these?

Re: SQL Injection Galore

#40

Post by mullethunter »

harrytuttle wrote: post #24

I have commit hooks on our repository that look for things like this and prevent the user committing it! We've got 30 odd rules so far that have saved us from all sorts of pain from exception swallowing to adding test ignores as well.

I second this. I manage a couple of teams and one is dealing with legacy a PHP stack where they've been refactoring things like SQL injection risks and whatnot. Having something as an automated second set of eyes would be brilliant.
Post reply on HN