10 ms·
SQL injections vulnerabilities in Stack Overflow PHP questions
- babyrainbow 10y agoPhp is the polio of programming. It catches you when you are a still young to programming, and cripples you for life (Or a very long time)...
- cm2187 10y ago...the VB6 of the open source world!
- willvarfar 10y agoWhat was so wrong about VB6? As a language I rather liked it.
- cm2187 10y agoI liked it too then, but it is a language from another decade, with many design flaws (no concurrency, arguments byref by default, inconsistent syntax (like when to use Set or 0-based vs 1-based collections)) and that doesn't really work as a modern language. And I did my fair share of php too, that was the only way I would develop a website for many years. But I think the similarity with php is not so much in the quality of the language, but rather that it is an old language, which strength (easily accessible to beginners) also makes its weakness (too many beginners who never moved on or improved their coding style, I personally know "professional" php developers who still do not know what a class is, let alone a sql injection vulnerability). And it becomes unkillable just because of the cheer number of developers to who it is the only known way to write code.
- quickben 10y agoFrom a design perspective: Nothing wrong with VB6. It fulfills it business niche rather well.
- WA 10y agoString concatenated SQL queries can be executed in pretty much any language. This has nothing to do with PHP in particular.
- babyrainbow 10y agoIt has everything to do with Php and the attitude that goes with it. Php puts up the appearance that web development is child's play . That in itself is not bad. But what makes it bad is that it provide absolutely no safety by default, in how it does that. This is true with the stuff in the language and outside of it (Documentation, community etc..).
- mootothemax 10y ago>It has everything to do with Php and the attitude that goes with it. Doing a quick search on GitHub gives a good number of similar exploits, several per page of results: C# https://github.com/search?l=C%23&q=sqlcommand+where+username&ref=searchresults&type=Code&utf8= https://github.com/search?l=C%23&q=sqlcommand+where+username... Java https://github.com/search?l=Java&q=sqlcommand+where+username&ref=searchresults&type=Code&utf8= https://github.com/search?l=Java&q=sqlcommand+where+username... Just look at the sheer inventiveness that goes into doing things The Wrong Way; it's not a language-specific problem.
- merb 10y agowell in C#/java you can have User getUser(long id) {}; and you definitly now that if you concat the id with the SQL that it actually is not a vulnerability. Nobody can inject arbitary SQL into the Long/Int variable.
- lsaferite 10y agoAnd in PHP 7.0 (because who wants to use something earlier) you have: function getUser(int $id) : User
- mootothemax 10y ago
- sschueller 10y agoNot really, these are just non parametrierend SQL Statements. It doesn't mean that the variables in the SQL Statements have not been sanitized before hand. [1] It may not be best practice but doesn't automatically mean SQL injection vulnerable. And this is not limited to PHP [1] https://github.com/laurent22/so-sql-injections/issues/1 https://github.com/laurent22/so-sql-injections/issues/1
- nkozyra 10y agoYeah, that was my immediate thought. Something like this: $delete = "DELETE FROM `cart` WHERE id='$id'"; Which is currently among the top three, could be prefaced by the built in mysqli escape function or at minimum cast to (int). This is not a great approach, the query should be prepared, but it's not inherently an injection risk based on that snippet alone. And the underlying functions come from the C library for MySQL. Almost any language's SQL libraries will allow you to send an arbitrary string to the database. This is not a PHP problem by any measure.
- dep_b 10y agoPerhaps it's the whole mindset. Why even use that when you have perfectly fine built in mechanisms for SQL parameters for over a decade? I wouldn't even start writing a query like that. You can do stuff like that too in a .Net / SQL stack for example (or in Django) but somehow everybody defaults to Entity framework or similar solutions that take care of it. I'm way too lazy to deal with lines and lines of sanitizing, raw queries and mysql_blahblahblah anyway so I write wrappers that give me the results from a query and a bunch of parameters in one line. Secure. Simple.
- flomo 10y agoIf you google "PHP MySQL", you can find all sorts of horrible tutorials like [1] teaching you to write SQL injections. So unlike users of other languages, PHP programmers are taught the wrong way first and then are expected to figure it out somehow. [1] http://www.freewebmasterhelp.com/tutorials/phpmysql/7 http://www.freewebmasterhelp.com/tutorials/phpmysql/7
- jorkro 10y agoIt's important to note that the author seems to be flagging injections based on the use of a PHP variable in the query string ( https://github.com/laurent22/so-sql-injections/blob/master/src/AppBundle/InjectionFinder.php#L62 https://github.com/laurent22/so-sql-injections/blob/master/s... ). Although a strong indicator, this does not necessarily imply an SQLi. If the variable is sanitized before the query (for instance, the variable is cast to an int), there is no SQLi to exploit. So it's very possible that these graphs are riddled with false positives.
- cm2187 10y agoIt is more than a strong indicator. But I think the absolute % is missing the point. What I find interesting is the evolution. It doesn't go down.
- Mahn 10y agoPHP has always been easy to pick up, so it's always going to have amateurs. These questions riddled with vulnerabilities are usually asked by people that don't understand what they are doing nor want to learn; it's not really representative of the sector of developers that make a living with PHP.
- cm2187 10y agoHaving a lot of amateurs is one thing. What I am bit concerned about is that the alarming pace of major data leaks, many of which start with an amateurish mistake like this, should increase the awareness to this kind of flaws. The evolution of this chart seems to suggest that this is not the case, at least not for the wider, mainstream community (I am not referring to a minority of MIT/Stanford-educated elite computer scientists).
- s_q_b 10y agoI have had four zero-days affecting 5.5 million devices sitting on a public code repository for two months now, for a project maintained by dozens of corporations employing high-end PHP programmers, who do write PHP for a living. The fixes are code reviewed, but not merged, because the developers don't seem to understand PHP-into-C null string terminator vulnerabilities, or type juggling, or strict comparison, or... I could go on. PHP is unsafe at any speed, because it almost invites arbitrary code execution through a number of vectors. It isn't inherently bad if used correctly, as most Facebook developers will tell you, but the language structure involves quite a number of insecure practices. After all, most programmers don't expect: <?php 0 == "string"; ?> to be true.
- cm2187 10y agoWhat I would also find interesting is to do the same exercise on the top responses.
- askmike 10y agoI haven't seen any examples (just code snippets), but I wouldn't call all unsanitized queries SQL injection vulnerabilities: $sql = "SELECT * FROM inventory_item WHERE product_category_id='$pcid'"; The pcid var might have been checked/cleaned up before this line (might even be part of the same SO answer). ----- Obviously that doesn't mean this is very bad practise, especially at stackoverflow where you know people will just copy and paste your code.
- vomitcuddle 10y agoyeah, unless the snippet also includes a definition for variable $pcid, there is no proof that the code is actually vulnerable - using such simplistic logic, the stats are likely hugely inflated but i don't agree that it should be the poster's responsibility to show their sanitation logic to prevent people from blindly copying-and-pasting their code, if it's not relevant to their question
- chiefalchemist 10y agoPerhaps this is natural? That is, you're looking for help so you're likely to have other flaws in your tool box. Certainly, given the importance, something can be done to prevent relying on the human element to get this right every time. We're how many years into programming and SQL injection is still very real? So much for progress?
- daenney 10y ago> Perhaps this is natural? That is, you're looking for help so you're likely to have other flaws in your tool box. Can you explain what you mean by that? The way I'm reading that you seem to imply "looking for help" means that they have issues all over.
- chiefalchemist 10y agoYou're correct. But let me clarify. I'm not judging. We all have flaws, technical or otherwise. My point was to say, the universe of the sample has a bias of sorts. Kinda like, I have a dog. Well of course it barks. That's all. Make sense? Sorry?
- daenney 10y agoI'm afraid that didn't clarify anything for me. > the universe of the sample has a bias of sorts OK, what? > I have a dog. Well of course it barks. I'm with you there. Dogs bark. There's a correlation between the animal being a dog and it barking. How does that relate to your original post? I have plenty of flaws and gaps in my knowledge and I use SO because there are plenty of great answers on there. Just like there's wonderful threads like these: http://security.stackexchange.com/questions/20803/how-does-ssl-tls-work http://security.stackexchange.com/questions/20803/how-does-s... on other parts of that same network of sites. I don't see how using SO, or any source of information on a topic to help you solve a problem implies or even correlates to not being a capable person in that or other areas in general.
- chiefalchemist 10y ago
- based2 10y agosolution: ci with http://stackoverflow.com/questions/4863969/php-source-code-analyzers http://stackoverflow.com/questions/4863969/php-source-code-a...
- JoshMandel 10y agoStackOverflow accepts edits to posts, if you have the karma for it. Somebody might enjoy writing a bot to propose updates that demonstrate better practices (like prepared SQL statements) for the 80% of bad examples where this is a pretty trivial manipulation.
- oneeyedpigeon 10y agoDoes this site link to the relevant questions, though? Because I can't see it.
- laurent123456 10y agoNot currently, but the data is there so it would be easy to provide a link for each question. Edit: Done - the date is now a link to the question.
- oneeyedpigeon 10y agoThanks very much. I know I've been a bit critical about the implementation in these comments, but it's only because I see such value in the concept, and that's much more important, tbf.
- astrodust 10y agoThere's a small group that does its best to flag questions with comments and steer people towards the right way of doing things. Ninja-editing questions to "fix" problems doesn't teach anyone anything.
- pasta 10y agoI think it's even more shocking they appear in answers. But then again this does only show they don't use prepared statements. You can't tell anything about vulnerabilities.
- niklasber 10y agoI think it's very natural that code in questions is vulnerable to SQL injections as it should be a stripped down example demonstrating the issue that the poster is dealing with, not necessarily a copy-paste of code that's in production.
- tyingq 10y agoIt's finding some things where placeholders aren't a solution.... 29/11/2016 08:23:13: $update = 'UPDATE '.$table.' SET '; You can't use a placeholder for a table name. Using a dynamic table name is pretty standard practice. The reason for that is to allow a table prefix, allowing multiple of the same app within a single database. And, typically, the table name originates from a config file, not web POSTS/GETS or other "taintable" sources.
- oneeyedpigeon 10y agoAlso, one of the examples is: 03/12/2016 19:33:11: $pdo->prepare("UPDATE users SET avatar=? WHERE id=22 ")->execute([$u]) ; Isn't that the correct way to do it?
- DCoder 10y agoThat's what happens when you use Regexes to find problems: https://github.com/laurent22/so-sql-injections/blob/7b9d99c06c5d4e751d48a0d9b6c99ec4fc11d009/src/AppBundle/InjectionFinder.php#L62 https://github.com/laurent22/so-sql-injections/blob/7b9d99c0...
- tyingq 10y agoFound the regex's being used: https://github.com/laurent22/so-sql-injections/blob/master/src/AppBundle/InjectionFinder.php#L62 https://github.com/laurent22/so-sql-injections/blob/master/s... It's pretty basic. For UPDATE, it's matching anything with UPDATE followed by SET, followed by a dollar sign+alphanumeric And, it's specifically going after dynamic table names. Hmm. '/UPDATE\s+.*?\$[a-zA-Z0-9].*?\sSET.*?/i', // UPDATE $table SET ... Edit: You can also escape the scans by using variable names that start with an underscore :) $_dontscanme
- DCoder 10y ago> You can also escape the scans by using variable names that start with an underscore So people who use "WHERE id=" . $_GET['id'] will not get detected?
- dorianm 10y agoNext step could be to auto-edit the answers with proper SQL queries? Most of them look pretty simple to fix
- dorianm 10y agoNext step could be to auto-edit the answers with proper SQL queries? Most of them look pretty simple to fix
- cdevs 10y agoThese are the people we don't hire or if we communicate will with and they seem teachable this is always are first test and step for teaching. I use to be suprised how many college grads come out with a blank face when you ask them about string cleaning or binding and MOST have worked some small internship at a business which means they created a CRUD with plenty of sql injection working for cheap instead of learning. Side story had to yell at a new employee this year for obsessively posting to stack queries like this instead of asking the senior devs. I've asked 5 stackoverflow questions over 6 years and answered 61..if your asking 3 a day you need to rethink things.
- cdevs 10y agoThese are the people we don't hire or if we communicate will with and they seem teachable this is always are first test and step for teaching. I use to be suprised how many college grads come out with a blank face when you ask them about string cleaning or binding and MOST have worked some small internship at a business which means they created a CRUD with plenty of sql injection working for cheap instead of learning. Side story had to yell at a new employee this year for obsessively posting to stack queries like this instead of asking the senior devs. I've asked 5 stackoverflow questions over 6 years and answered 61..if your asking 3 a day you need to rethink things.
- xytop 10y ago!!! PHP MySQL extension doesn't support placeholders so everyone has to escape params before mysql_query and then put escaped params inside query string. Of course if there's used mysqli or PDO then need to use placeholders but in other case - there is actually no choice for developer. So I'd rather not call most of those - "vulnerabilities".
- thisrod 10y agoIn principle, it seems like this could be mitigated by some changes to the language. Has anyone tried this? E.g.: Strings have a dirty flag. Literal strings are initially clean, CGI variables are dirty, argv and file/socket input might be configurable. Function parameters can be declared evaluated. Passing a dirty string as an evaluated argument fails with extreme prejudice. The database primitives enforce this. Maybe sockets have an evaluated flag too, and only clean strings can be written to an evaluated socket. The string primitives propagate dirty flags: clean and dirty concatenate to dirty, and so on. There is an obfuscated way for pdo->prepare to make dirty strings clean, and a special circle of hell for anyone who mentions that in user documentation.
- DCoder 10y agoPerl has such a feature, it's called "taint mode" [1], it marks all variables receiving external data as "tainted" and disallows their usage in sensitive contexts. PHP has an inactive proposal to implement it as well [2]. [1] http://perldoc.perl.org/perlsec.html#Taint-mode http://perldoc.perl.org/perlsec.html#Taint-mode [2] https://wiki.php.net/rfc/taint https://wiki.php.net/rfc/taint
- chriswarbo 10y agoAt a previous job we used custom patches for the PHP interpreter which implemented just such a "dirty flag", among other things. It would also die if you sanitised a clean string, since that indicates that the sanitising logic is dodgy.
- 0hn0 10y agoIt shows only bad practices of PHP and SQL. SO question were asked probably by novice programmers or amateurs. I'm a PHP developer. I use PDO or other data binding ORM/lib (ex. Eloquent ORM).