SQL injections vulnerabilities in Stack Overflow PHP questions
laurent22.github.io
laurent22.github.io
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.
But I think the absolute % is missing the point. What I find interesting is the evolution. It doesn't go down.
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).
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.
nobody would expect that
Note that PHP also has a JavaScript-style triple-equals comparison, which does not attempt type conversion and does not exhibit this bizarre behaviour.
This occurs if PHP thinks both strings could be numbers in scientific notation. "0e123" == "00e45"
Is BAD. Imagine if a car was made with the same "design".
A tool is BAD if the user must patch to overcome the inherent behaviour it show.
As for the framework, that's RDK-B. http://rdkcentral.com http://code.rdkcentral.com https://github.com/rdkcmf
The deeper you look, the worse it gets. Those php issues are very trivial, first glance type stuff. Some need a bit of a twist to make exploitable, but another will strip the encryption right off the hidden network.
I have others I wish to disclose, but I can't seem to get them to respond to my requests for a PoC. Quite frankly, I'm shocked that I can't seem to get anyone to realize how serious the impact of an RCE vulnerability in a framework fielded that widely truly is.
If you find any of more serious things I'm talking about on your own, wait for the vendors to fix them. Please don't brick the world.
They should still be fixed, but I believe these bugs are no longer an issue in PHP after 5.3.
Anecdote: I was integrating a paid Magento extension (which charges about $1000 for a license) into an existing shop, and some poor code in their templates made me look deeper into their main code. Six hours later, I had:
* 4 different ways to read any file on the server that PHP can read,
* 5 different ways to upload any file to a certain directory on the server, including one that lets you overwrite the webserver's security configuration for that directory and then execute uploaded PHP code,
* a way to delete certain things the administrator has created in the backend,
* a way to overwrite other customers' uploaded files,
* a way to edit other customers' information, which lets you then XSS/XSRF those customers when they view that information.
I have reviewed other Magento extensions in the past, and I found many poor ones, but nothing as atrocious as this.
Probably you encountered this in the days of Magento Connect. Our new app store (Marketplace) actually hosts the code and has static code tests for many things, including OWASP items.
This extension was purchased through Magento Connect just a few months ago. I'm glad to hear the situation is improving now.
For those that use a framework for PHP they're also insulated from these problems, they have better examples to work from, but the PHP community is full of people that think they're too good for frameworks, or that frameworks are an obstacle to understanding.
Those people are the cause of so much damage.
It may not be best practice but doesn't automatically mean SQL injection vulnerable.
And this is not limited to PHP
$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.
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.
The biggest trouble with these questions is that people are learning many aspects of web development without understanding the separate concerns they should be aware of when heading to production code. It's difficult to (a) avoid over-answering and explaining everything while also (b) avoid giving insecure advice
That's a completely different issue and not an SQL injection error though.
Personally, I don't think introducing undefined behavior is ever good advice, even if it is technically correct advice.
All of this really just gets at my original point, though: there are so many things for new developers to learn that it is extremely difficult to provide good answers on a forum like SO. That doesn't diminish the value, but we have to be aware of, accept, and make clear this fact.
OTOH it is entirely unnecessary to quote integers (and the escaping function would not quote it before injecting it), so outlook not so good.
I've seen lots of people do that though I don't know their rationale. Maybe they just don't know SQL
It's not necessary in any DB, SQL specifies coercion of any value when the expected data type is unambiguous (and in a bunch of other cases, SQL is pretty weakly typed all in all).
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.
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?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 query("DELETE FROM foo WHERE id=1");$id=2;
In other words, it's flagging any line that begins with "DELETE ... FROM" and then later has a variable. Now, that might be a fairly strong correlation, but it's definitely far from perfect. $query="SELECT whatever from foo WHERE".
"id=$bar ".
"AND " .
"field=$value";- it's currently skipping variable names with underscores, and it's matching variable names that start with numbers, which isn't a valid variable name
- the false positives coming from ->execute($whatever) could be fixed by not searching after a > character. I don't think that would create any notable amount of false negatives.
Like, for example, changing:
'/UPDATE\s+.*?\sSET\s.*?\$[a-zA-Z0-9].*?/i'
To '/UPDATE\s+.*?\sSET\s[^>]*?\$[a-zA-Z_]/i'... or by splitting the SQL string over multiple lines. Or building it up by concatenating strings. Let's stop this now, we've demonstrated that the code is very brittle :)
So people who use
"WHERE id=" . $_GET['id']
will not get detected?Not just php either. Ruby/Rails Active Record, for example, supports it.
And I imagine dynamic, variable bound table names are probably in every ORM in just about every language.
Another scenario I just thought of: your schema is more or less dynamic. For example, Drupal lets developers/site administrators create entities with custom fields, each field gets its own DB table, and to load an entity you need to
SELECT * FROM node
LEFT OUTER JOIN field_data_field_name_1 ON ..
LEFT OUTER JOIN field_data_field_name_2 ON ..
...This is precisely how PHP programmers think: that MySQL is somehow the gold standard in the databases world.
Table prefixes are just the stupid way of emulating database schemas where it's not supported, which is only MySQL (SQLite doesn't support this either, but it was never meant for web applications anyway).
Edit: And, for what it's worth, the affinity for Mysql in PHP code is most likely because both are there, by default, in shared hosting environments. I don't personally know any PHP devs that find Mysql to be a "gold standard". Most popular PHP apps and frameworks support Postrgres as well.
Yeah, two databases. One that was designed as an embedded storage engine and was never intended for web applications and the other that historically was probably the shittiest one (silently damaging unicode data, silently damaging foreign keys in some circumstances, silently ignoring CHECK constraints, and using non-ACID data store unless asked otherwise, though the last one was fixed just few years back) and should have died long time ago.
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.
Edit: Done - the date is now a link to the question.
$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.
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
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.
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.
PHP has an inactive proposal to implement it as well [2].
It would also die if you sanitised a clean string, since that indicates that the sanitising logic is dodgy.
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?
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.
That's all. Make sense? Sorry?
> 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-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.
You're disheveled. You're shirt isn't tucked in. So you're not going to know you're fly is down as well.
The basic question is your shirt. The SQLi issue is your fly.
Put another way again :) Is it reasonabke to expect someone struggling with a SQL question to get SQLi concerns right? Fuck me. They can't get the query. Do you really think they're thinking about injection?
There! Any better? That's all I got. :)
Python, Perl, Ruby, Java, Node, they all have respect for best practices and libraries that make coding SQL queries safely quite easy and pleasant.
PHP is a whole different beast. People are allergic to package managers, to using "third party code", to using frameworks, even to reading documentation. At times it's amazing how aggressively backwards it can be.
I'm not trying to characterize the entire PHP community, there are large parts of it with people trying hard to do the right thing, but there's also this vast wasteland of incompetent people publishing tutorials that are so dangerously wrong with even more people willing to defend these tutorials as being educational and useful.
Progress means learning from your mistakes, but some would prefer everyone repeat them, painfully if necessary, to promote "education".
The only problem with PHP is that it has the easiest barrier to entry - or perhaps more accurately, no barrier to entry whatsoever. The kind of developer you're describing would never have written a line of code if PHP didn't exist. Python, Perl, Ruby, and Java just wouldn't have been an entry point into programming for them. Node is different, as like PHP, it is accessible to frontend developers who want to branch off into backend as an "addon" rather than as a primary endeavor.
I remember considerable resistance on the part of the community to the very idea of object-oriented problem, something that massively limits uptake on things like PDO and is the driving reason behind mysqli having a bizarre mixed mode, OO or procedural based on preference.
Likewise, PHP tutorials, references, and guides kept pushing the absolutely garbage mysql_query interface on people. I don't know how many books published post-PDO insisted on using this approach because it was "easier" or whatever, and some books don't even touch on SQL injection bugs, it's like they don't exist, even while the examples lack escaping of any sort and would crash on certain kinds of input.
> The kind of developer you're describing would never have written a line of code if PHP didn't exist.
Bullshit. These are people that want to learn to program, and as they look around they see PHP is popular, it's easy to get support for, and hosting it is easy. It's got a lot of positive points.
I've seen people with zero technical experience pick up Ruby and build a Rails app inside of months, they get productive very quickly. Same could be said for Node with the right mentoring, though the async stuff is a little harder to get lined up. Client-side apps aren't all that bad, though.
PHP has a lot of good things going for it, but too much of the community is absolutely in the dark about it. There's almost zero outreach being done.
The first line of the file is "require( dirname(__FILE__) . '/wp-load.php' );". a) "require" is a statement, not a function; it should not have parentheses around it at all. b) Oh. my. god. Their coding standard require spaces inside parentheses; I have never seen this in any language, even from legacy codebases or esoteric developers - it's incomprehensible that some senior developer managed to enforce this on the entire company. c) Oh, mixed PHP class and HTML; a 1000 line file for a login script. RUN! SERIOUSLY, RUN AWAY!
After this many years, backwards compatibility is not an excuse. WordPress should be avoided at all costs, unless it is the only way you believe you can build and sustain your business. If you have the choice to take any other avenue, take it. The developers working there have no clue, and/or are content with a status quo that is 20 years behind acceptable standards.
Yeah. You're right. Ugly.