Live data from Hacker News

SQL Injection Galore

github.com

71–80 of 88 posts

Re: SQL Injection Galore

#71
post #55

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 am not sure that is enough, I mean if you look at the supposed results many of them can't actually be actacked (like the on that tests that Id is a number before it is used) but a semi stupid script wouldn't catch those.

It's good enough to make people aware that you are watching their every move :)

Re: SQL Injection Galore

#72
post #67

What's more concerning is the amount of people choosing not to use parameterized queries with mysqli: https://github.com/search?q=extension%3Aphp+mysqli_query+%24...

Have you tried using mysqli's implementation of parameterized queries in real, live code?

I mean, sure, it works, but it's ugly and inconvenient. PDO does it much, much better.

Re: SQL Injection Galore

#73

I find it incredible that there are 86,453 results for this, and 58,123 results for execution of a parameter as a command! Github should set up a little warning for people when they log in to their account if their repos match any of a set of obvious security anti-patterns, because this would be fertile ground for exploits. They could expand it later to some kind of code-review bot which does automatic code reviews l…

That would be great, but I would hope it could be turned off. I have plenty of code on GitHub that's clearly marked as just something I put up in a few hours for personal use only, where I don't care if there's vulnerabilities or exploits. The code was written in the true spirit of hacking, fast and dirty in order to solve an immediate need.

It shouldn't really matter if it's only for personal use, it's not a whole lot of extra effort to follow basic security guidelines.

Re: SQL Injection Galore

#74

To be clear, this list is not all exploitable. It only shows php files that use both GET variables and the mysql_query function. Not all of these variable are being fed unescaped into a query, nor are they necessarily used in a query at all.

Agreed. I find posts like these disingenuous. What do learn from this list? I think we all know that SQL injection is a serious problem and a very common mistake already. What this list seems to do is just serve as a high horse we can all get on and proceed to shame and look down our nose at people who: - Use raw $_GET variables in their code - Use $_GET in MySQL queries - Use mysql_real_escape_string instead of prep…

I don't see any purpose in this other than to either look for open source applications to exploit in the wild or to just pick on a really easy target to make us all feel superior.

You could do a similar search for common anti-patterns in almost every language you can think of, so this is not specific to PHP, just perhaps a bit more common due to the low-bar for picking up PHP.

A useful aim of this sort of post to me is to underline that people should not do this in a script exposed to the internet, and if people read comments on searches like this, they'll find out why. I don't think it's appropriate to use variables in query strings even for learners as it's teaching bad habits - I have personally seen scripts go into production based on bad examples at a former place of work (in perl, not php), allowing spammers to use a contact form. You can guarantee that for each of these common vulnerabilities there are scripts in the wild scanning websites and trying to exploit them.

A surprising number of people still do make mistakes like this, so it does more good than harm to point that out I think, and certainly doesn't make me feel superior, it makes me wonder which vulnerabilities I've missed in my own code.

Re: SQL Injection Galore

#75
post #50
post #33

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.

It is a small window, however, wouldn't it be better to filter the values into an easily identifiable 'clean' variable? The code is still using $_GET, and while it may have been filtered above me, I have no indication of that - versus - $foo->cleaned('var') - where I can reasonably assume it is clean.

True, I couldnt agree more

Re: SQL Injection Galore

#76
Instead of policing every line of code you can also mitigate SQL injection by restricting the access of the database handle the user-facing queries are using

* All read-only queries use a read-only (SELECT only) database account. Injecting; DROP TABLES or INSERT, UPDATE, etc (DML commands) just error out.

* User accounts and logins are stored in a different database, so only the code responsible for login and registration can access those tables/database.

* All queries should use prepared statements which effectively treats all injected text as just text rather than database commands.

Re: SQL Injection Galore

#78
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?

I'm trying very hard to think of a language that won't, by default, allow you to throw user-generated HTML onto an HTML page if it allows you to create an HTML page at all that incorporated user-generated (or, in fact, program- or programmer-generated) strings. Angle brackets, etc., are only situationally bad. (And let us separate language from framework here; the languages used in templating frameworks will almost always allow you to emit HTML should you choose to do so.)

Re: SQL Injection Galore

#79

I find it incredible that there are 86,453 results for this, and 58,123 results for execution of a parameter as a command! Github should set up a little warning for people when they log in to their account if their repos match any of a set of obvious security anti-patterns, because this would be fertile ground for exploits. They could expand it later to some kind of code-review bot which does automatic code reviews l…

I think this is a great idea, and an area where someone as big as Github could make a MASSIVE difference. A CodeSmell metric! Allow contributions to add extra smells to langauges, and then it looks after itself.

A modern, open, polyglot lint would be of great benefit to the world. Unfortunately, it's something that gets way too little attention. Open-source static analysis projects always seem to flounder.

Re: SQL Injection Galore

#80

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?

I'm trying very hard to think of a language that won't, by default, allow you to throw user-generated HTML onto an HTML page if it allows you to create an HTML page at all that incorporated user-generated (or, in fact, program- or programmer-generated) strings. Angle brackets, etc., are only situationally bad. (And let us separate language from framework here; the languages used in templating frameworks will almost a…

It's true a turing complete language has no way of stopping you, but there are ways around this that many frameworks and languages have adopted. Taint mode, for example.
Post reply on HN