
SQL Injection Galore - ericmathison
https://github.com/search?p=3&q=extension:php+mysql_query+%24_GET&ref=searchresults&type=Code
======
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?

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

~~~
ygra
Maybe dirty, hackish code for personal use only shouldn't make it out into the
public ;-)

~~~
freehunter
I would expect any code marked "quick hack to solve [obscure problem], not for
production!" to be pretty clear.

If someone is encountering the same problem, the code is there for them to
use. If they happen to use it in production, that's hardly my fault.

~~~
mesozoic
Technically if the code doesn't have an open source license attached then it
is still their copyright and isn't up for you to use even if on Github.

------
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...](https://github.com/search?q=extension%3Arb+path%3A%2Fapp%2Fcontrollers%2F+.destroy&type=Code&ref=searchresults)

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.

~~~
sdevlin
He's citing this as a potential cross-authorization vulnerability, not SQL
injection.

The form he's demonstrating is a common misstep in Rails. Instead of writing
something like:

    
    
      current_user.apartments.destroy(params[:id])
    

The programmer wrote:

    
    
      Apartment.destroy(params[:id])
    

Meaning that the apartment lookup is not being done within the context of the
current user, it's global. This means an attacker can delete other users'
apartments with crafted URLs.

When I'm testing Rails applications, I always grep the source for something
like:

    
    
      /[A-Z][A-Za-z0-9_]\.(find|destroy|...)/
    

To find vulnerabilities of exactly this kind.

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

------
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](http://php.net/manual/en/pdo.prepared-statements.php)

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

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

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

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

------
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...](https://github.com/search?q=extension%3Aphp+exec+%24_GET&type=Code&ref=searchresults)

~~~
fjcaetano
Holy shit! Look at this! This is hilarious!
[https://github.com/bratliff/engconf/blob/0b8f003edc5f5d25fe1...](https://github.com/bratliff/engconf/blob/0b8f003edc5f5d25fe1feecd9f39d107958ddd11/staging/wp-
content/plugins/ezpz-one-click-backup/functions/ezpz-archive-cmd.php)

~~~
krapp
Oh. And it's for wordpress. Isn't that just fucking wonderful. I would guess
looking at the age of the account and the complete lack of documentation that
it's a personal project he never really intended to get much scrutiny. I'm
sure if someone looked at my github they could find some bad code too. Not
that bad though.

Edit - made an issue.

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

~~~
dave1010uk
Could you share some of these?

~~~
harrytuttle
Not really at the moment. I've been slowly refactoring it into a publishable
chunk of code but progress is slow. I will publish it on github when it is
done.

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

~~~
seandougall
I totally get what you're saying, but this right here is kind of the problem:

> Maybe I was creating simple examples to teach others the basics of getting
> user input and working with it in a database.

I'm one of the countless people who learned PHP off of examples that do just
that, for clarity, and it was years before I learned that it was not meant to
be a real-world example. Sites that offer such examples (I'm looking at you,
w3schools) are probably responsible for 90% of all the bad code that shows up
on lists like this, because the exchange amounts to:

Student: "How do I do XYZ?"

Teacher: "It's easy! Just do 123."

Student: "Thanks!" <leaves, closes door>

Teacher (to empty room): "Haha, just kidding. You should really forget that
entirely and do 456 instead."

I don't think it's valuable to learn about mysql_query("delete from `tbl`
where id = {$_GET['id']}") at all in the first place, if you just have to
unlearn it and start doing it the right way later. It doesn't actually
simplify anything.

------
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](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...](https://github.com/search?q=extension%3Aphp+eval+%24_GET&type=Code&ref=searchresults)

------
OliverLassen
Yeah, the same for Classic ASP:
[https://github.com/search?q=extension%3Aasp+execute+request&...](https://github.com/search?q=extension%3Aasp+execute+request&type=Code&ref=searchresults)

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

~~~
hahainternet
> 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?

~~~
tudborg
You can be an idiot in any language, including PHP. Blaming the tool is
exactly what gets you in to this mess in the first place.

The fault is with the programmer. No one else.

~~~
hahainternet
It's true that you can be an idiot in any language, but I have written
production code in every 'high level' language of this type and only PHP seems
to suffer from this problem. Take a random mail form, even has its own domain:
[http://jemsmailform.com/](http://jemsmailform.com/)

Finding the obvious problems I leave as an exercise.

~~~
user24
I kindly direct you to search for "formmail.pl".

------
pearjuice
This is a duplicate.

[https://news.ycombinator.com/item?id=5805025](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...](https://github.com/search?q=extension%3Aphp+mysqli_query+%24_GET&type=Code&ref=searchresults)

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

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

~~~
foogs
True, I couldnt agree more

------
LolWolf
Oh, God, why.

------
_ZeD_
poor, poor Bobby Tables...

