9 ms·
SQL Injection Galore
- Rygu 13y agoIt's been posted. :) https://news.ycombinator.com/item?id=5805025 https://news.ycombinator.com/item?id=5805025
- a904guy 13y agoI always preferred the remote code execution search myself personally... https://github.com/search?q=extension%3Aphp+exec+%24_GET&type=Code&ref=searchresults https://github.com/search?q=extension%3Aphp+exec+%24_GET&typ...
- a904guy 13y agoCGI Python? Happens... https://github.com/search?q=extension%3Apy+os.system+%22import+cgi%22&type=Code&ref=searchresults https://github.com/search?q=extension%3Apy+os.system+%22impo...
- a904guy 13y agoDjango... https://github.com/search?q=extension%3Apy+os.system+%22request.GET%22&type=Code&ref=searchresults https://github.com/search?q=extension%3Apy+os.system+%22requ...
- fjcaetano 13y agoThe difference is that SQL injection will only happen when using raw queries. System (as you mentioned) or EXEC injections, however, may get out of hand.
- ris 13y agoI only found one exploitable example browsing the first few pages, whereas the majority of the OP's results looked fairly exploitable.
- fjcaetano 13y agoHoly shit! Look at this! This is hilarious! https://github.com/bratliff/engconf/blob/0b8f003edc5f5d25fe1feecd9f39d107958ddd11/staging/wp-content/plugins/ezpz-one-click-backup/functions/ezpz-archive-cmd.php https://github.com/bratliff/engconf/blob/0b8f003edc5f5d25fe1...
- krapp 13y agoOh. 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.
- spyder 13y agoAnd it has been already exploited: http://wordpress.org/support/topic/plugin-ezpz-one-click-backup-possible-security-flaw http://wordpress.org/support/topic/plugin-ezpz-one-click-bac...
- grey-area 13y agoI 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 13y agoI 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.
- nknighthb 13y agoA 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.
- freehunter 13y agoThat 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 13y agoMaybe dirty, hackish code for personal use only shouldn't make it out into the public ;-)
- yuvadam 13y agoBear 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 13y agoTo 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.php http://php.net/manual/en/function.mysql-real-escape-string.p...
- seivan 13y agoFor 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%2Fcontrollers%2F+.destroy&type=Code&ref=searchresults 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 13y agoYour 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 13y agoHe'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.
- c0n5pir4cy 13y agoWe 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.
- user24 13y agoNot 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.
- troels 13y agoYeah. It's just the wrong level of abstraction to deal with this. It's not even hard to do properly anyway.
- fooyc 13y agoThis 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.
- OliverLassen 13y agoYeah, the same for Classic ASP: https://github.com/search?q=extension%3Aasp+execute+request&type=Code&ref=searchresults https://github.com/search?q=extension%3Aasp+execute+request&...
- hatu 13y agoSeems like it would be possible to create a "linter" for PHP and Ruby that would detect at least basic SQL injection vulnerabilities?
- _ZeD_ 13y agopoor, poor Bobby Tables...
- harrytuttle 13y agoI 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 13y agoCould you share some of these?
- harrytuttle 13y agoNot 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.
- mullethunter 13y agoI 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.
- tomjen3 13y agoI 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.
- harrytuttle 13y agoIt's good enough to make people aware that you are watching their every move :)
- peterwwillis 13y agoGitHub Dorking: The successor to Google Dorking.
- foogs 13y agomy 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 13y agoIt 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 13y agoTrue, I couldnt agree more
- cruise02 13y agoNext time someone asks you "How do I find open source projects to contribute to?" this is a good place to start.
- fjcaetano 13y agoAt first I thought "this is so mean" when I saw the search query, but with your comment comes a silver lining...
- diminoten 13y agoThis 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...
- gedrap 13y agoWhat I find equally worrying and sad is the fact that mysql_* extension is so widely used although it has been deprecated 3 years ago.
- api 13y agoIn other news: it's 2013 and people are still not using query builders.
- Ryoku 13y agoWow. 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.
- pearjuice 13y agoThis is a duplicate. https://news.ycombinator.com/item?id=5805025 https://news.ycombinator.com/item?id=5805025
- LolWolf 13y agoOh, God, why.
- brokentone 13y agoTo 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 13y agoAgreed. 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.
- grey-area 13y agoI 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.
- martin_ 13y agoWhat'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_GET&type=Code&ref=searchresults https://github.com/search?q=extension%3Aphp+mysqli_query+%24...
- McGlockenshire 13y agoHave 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.
- ygra 13y agoThis 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.)
- snorkel 13y agoInstead 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 13y agoSo instead of policing every line of code we should just use prepared statements everywhere?
- kronholm 13y agoSome 40.000 results with the eval() function: https://github.com/search?q=extension%3Aphp+eval+%24_GET&type=Code&ref=searchresults https://github.com/search?q=extension%3Aphp+eval+%24_GET&typ...