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

1–10 of 88 posts

Re: SQL Injection Galore

#6

Post by 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?

Re: SQL Injection Galore

#7

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

Re: SQL Injection Galore

#8

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

Re: SQL Injection Galore

#9

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

Re: SQL Injection Galore

#10

Post by user24 »

yuvadam wrote: post #7

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,…

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

Post reply on HN