SQL Injection Galore

[grey-area]

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 looking for common antipatterns, off-by-one errors, and anything else that is easy to detect automatically. There are quite a few tools like this for analysis of desktop apps, but code for web apps are not checked as much in a systematic way.

Are there any external services which do this already for public github repos?

[tomorgan]

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.

[freehunter]

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.

[seivan]

For Ruby.. this is probably not as bad, and some of them are probably harmless, but still.

* Apartment.destroy(params[:id])

https://github.com/search?q=extension%3Arb+path%3A%2Fapp%2Fc...

I've found stuff like this on startups who charges people money - scary.

Some of them require authentication but not authorization In other words, they require login, but not WHO you're logged in as.

[Xylakant]

Your example looks much like AR and AR will sanitize the given parameter at least in this case so that's a non-issue here. Same would be true if you'd call a prepared statement in PHP and hand it a GET parameter. The problem with the given PHP samples is that they do neither: The take the unsafe user-provided parameter and use string concatenation to build a query - the textbook example of an SQL injection vulnerability.

[cruise02]

Next time someone asks you "How do I find open source projects to contribute to?" this is a good place to start.

[diminoten]

This is brilliant, and doesn't necessarily have to be exploits. You could just search for any anti-pattern and fix it.

I'm a little sad I hadn't thought to do this already...

[fjcaetano]

At first I thought "this is so mean" when I saw the search query, but with your comment comes a silver lining...

[yuvadam]

Bear with me on this one -

All the code is already public on Github, so everyone can see these gaping security holes, right?

What if some honest, global actor could mass-commit a fix for all these repos in one fell swoop? For example, replace all references to:

    $_GET

with:

    some_safe_sanitizer($_GET)

All affected repos win a free fix, and the world becomes a better place due to having less security bugs. Essentially, what would the next abstraction layer, over all Github repository objects, look like? Ponder that.

[user24]

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 not being escaped, etc). Also that extension is deprecated in favour of prepared statements.

Also, some of the dodgy code is using $_GET vars for things other than values, like field and table names (!!!!) which would need a more significant refactoring in order to fix.

However it would be a great little problem to work on, and you could probably write a bot to fix 80% of the code pretty easily.

The other issue is how many people will understand and accept the pull requests...

[1] http://php.net/manual/en/pdo.prepared-statements.php

[2] http://php.net/manual/en/function.mysql-real-escape-string.p...

[crazygringo]

Because there are plenty of times when that would break things. Consider:

    $table_name = $_GET['from'];
    if (!in_array($table_name, $VALID_TABLES)) {
      die('Invalid table name');
    }
    $set_to = (int)$_GET['set_to'];
    mysql_query("INSERT INTO $table_name (value) " .
      "VALUES ($set_to);

Quoting/escaping the table name in this case would break the code. (Backticks will work, but that's another story.) The variable has already been sanitized immediately before, so there's no injection possibility.

The case with $set_to is still pretty bad form, but it has been sanitized above by casting to an int.

Not saying this is great code, just pointing out the fact that there's no possible way to do a mass-commit fix.

[snorkel]

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.

[qu4z-2]

So instead of policing every line of code we should just use prepared statements everywhere?

[a904guy]

I always preferred the remote code execution search myself personally...

https://github.com/search?q=extension%3Aphp+exec+%24_GET&typ...

[fjcaetano]

Holy shit! Look at this! This is hilarious! https://github.com/bratliff/engconf/blob/0b8f003edc5f5d25fe1...

[a904guy]

CGI Python? Happens... https://github.com/search?q=extension%3Apy+os.system+%22impo...

[a904guy]

Django... https://github.com/search?q=extension%3Apy+os.system+%22requ...

[harrytuttle]

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.

[mullethunter]

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.

[dave1010uk]

Could you share some of these?

[tomjen3]

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.

[brokentone]

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.

[bpatrianakos]

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 prepared statements or otherwise properly sanitizing input - Use PHP in general

We already know we should all be using prepared statements or combining data sanitizing methods with an ORM and to not trust raw user input and that its cool to hate on PHP. So I ask again, what are we learning here? That there's a ton of programmers who are doing sloppy incompetent work? Not news.

Furthermore, I think context is important. I'm not ashamed at all to say out loud that I've done these sorts of things knowingly and purposefully despite knowing it's not safe or correct. Why? Maybe I was creating simple examples to teach others the basics of getting user input and working with it in a database. Of course you'd warn people not to do things like that in a production environment but you show it that way anyway to get across some basic ideas without throwing too much at people. Or what if I were writing a simple series or scripts or small app that would only be used by myself or a select group of people? What if that application needed to be done in a hurry and was only for internal use and not accessible to the outside world? Lots of things can always go wrong but at a certain point you have to be able to trust someone and its not always better to be right than happy. I'm sure if anyone wants to be pedantic they can poke holes in my examples but the point is that these sorts of sins aren't always so awful depending on the context. 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.

[hatu]

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

[Rygu]

It's been posted. :) https://news.ycombinator.com/item?id=5805025

[kronholm]

Some 40.000 results with the eval() function: https://github.com/search?q=extension%3Aphp+eval+%24_GET&typ...

[OliverLassen]

Yeah, the same for Classic ASP: https://github.com/search?q=extension%3Aasp+execute+request&...

[peterwwillis]

GitHub Dorking: The successor to Google Dorking.

[api]

In other news: it's 2013 and people are still not using query builders.

[c0n5pir4cy]

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.

[fooyc]

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.

[user24]

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.

[ohwp]

I think this is bad practice. You should not check your input but your output. So clean up the var that goes into the database.

[pearjuice]

This is a duplicate.

https://news.ycombinator.com/item?id=5805025

[gedrap]

What I find equally worrying and sad is the fact that mysql_* extension is so widely used although it has been deprecated 3 years ago.

[martin_]

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...

[ygra]

This could be in part because the procedural interface of mysqli is deliberately designed to be vyer similar (if not identical) to the mysql one. So maybe they just continued writing their code that way but replaced mysql by mysqli because someone told them mysqli was better and supported (which it is, but probably for other reasons).

I wonder whether a similar search for the mysqli OO interface yields the same amount of unsanitised queries. (Assumption: People using the OO interface looked through the documentation more closely and maybe understand the ways in how mysqli is better than mysql. But that's just an unfounded assumption.)

[McGlockenshire]

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.

[Ryoku]

Wow. This has just turned into a great example for one of my classes. I don't think there's a better way (other than hands-on examples) to show the severity and widespread of such security holes.

[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.

[dwrowe]

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.