Drupal Fixes Highly Critical SQL Injection Flaw

An anonymous reader writes Drupal has patched a critical SQL injection vulnerability in version 7.x of the content management system that can allow arbitrary code execution. The flaw lies in an API that is specifically designed to help prevent against SQL injection attacks. "Drupal 7 includes a database abstraction API to ensure that queries executed against the database are sanitized to prevent SQL injection attacks," the Drupal advisory says. "A vulnerability in this API allows an attacker to send specially crafted requests resulting in arbitrary SQL execution. Depending on the content of the requests this can lead to privilege escalation, arbitrary PHP execution, or other attacks."

Comments

Is Drupal 6.x Affected?

[Anonymous Coward]

I've seen no mention of whether or not Drupal 6.x is vulnerable; are they?

Re:Is Drupal 6.x Affected?

[Hewligan]

I've seen no mention of whether or not Drupal 6.x is vulnerable; are they?

No, it won't be affected, as the API involved was introduced in Drupal 7.

Re:Is Drupal 6.x Affected?

[amicusNYCL]

Considering that the API is to help protect against SQL injection though, it's probably fair to say that version 6 is affected by other issues.

Re:Is Drupal 6.x Affected?

[Hewligan]

Considering that the API is to help protect against SQL injection though, it's probably fair to say that version 6 is affected by other issues.

Not really. The API makes it easier for developers to avoid SQL injection vulns. That doesn't mean that code not using it has SQL vulns.

Re:Is Drupal 6.x Affected?

[fluffy99]

I've seen no mention of whether or not Drupal 6.x is vulnerable; are they?

No, it won't be affected, as the API involved was introduced in Drupal 7.

No, but it's certainly an indicator of the quality of code. Don't be surprised if other vulnerabilities are discovered as everyone shifts their attention and starts scrutinizing the rest of the code. The code diff is below. It's a pretty amateurish mistake, and had someone reviewed or tested the original code, they'd have seen it didn't do what it was supposed to. The comments even give you a big hint what the next vulnerability is going could be.

diff --git a/includes/database/database.inc b/includes/database/database.inc
index f78098b..01b6385 100644
--- a/includes/database/database.inc
+++ b/includes/database/database.inc
@@ -736,7 +736,7 @@ abstract class DatabaseConnection extends PDO { // to expand it out into a comma-delimited set of placeholders.
          foreach (array_filter($args, 'is_array') as $key => $data) {
              $new_keys = array();
- foreach ($data as $i => $value) {
+ foreach (array_values($data) as $i => $value) { // This assumes that there are no other placeholders that use the same // name. For example, if the array placeholder is defined as :example // and there is already an :example_2 placeholder, this will generate

Re:Is Drupal 6.x Affected?

[Anonymous Coward]

No, but it's certainly an indicator of the quality of code. Don't be surprised if other vulnerabilities are discovered as everyone shifts their attention and starts scrutinizing the rest of the code.

But... but... but... it's open source! It must have been reviewed by hundreds of people by now. Surely there's no more bugs.

Yeah right.

But seriously, if you think you can see what the next bug is going to be, or you think there's a code quality issue, why don't you go and submit a patch to the code rather than being snarky about it here.

Heh

[bunratty]

The flaw lies in an API that is specifically designed to help prevent against SQL injection attacks.

You had one job!

Re:Heh

[Anonymous Coward]

Even when code tries to do one thing, and tries to do it well, mistakes can happen.

So how can anyone think that something like systemd, which tries to do absolutely everything it seems, is a good idea?

Re:Heh

[Anonymous Coward]

So were they "sanitizing" full-text queries instead of using prepared statements? That's like polishing a turd. No mater how hard you polish, you still have a turd.

Re:Heh

[Spy Handler]

Kind of like a flu vaccine that give you... the flu

Re:Heh

[Hewligan]

No, it's more like they were enforcing parametrised queries on top of an API that allows non-parametrised queries.

Re:Heh

[amicusNYCL]

It looks like a feature where you could supply one placeholder in a prepared statement, but give it an array of values, and it would expand the placeholders to fit the array. So if the query was like this:

SELECT * FROM table WHERE id IN (:idlist)

and you passed an array with 3 values for idlist, it would replace the query like this:

SELECT * FROM table WHERE id IN (:idlist_1, :idlist_2, :idlist_3) ... then use the values in the array as the three values for those placeholders. It looks like the old code was using the keys from the data array, so instead of appending someting like "_1", it would append the actual key. So an attacker could put SQL code into the array keys and it would stick those (unchanged) into the query.

Here is the old code (without comments):

foreach (array_filter($args, 'is_array') as $key => $data) {
            $new_keys = array();
            foreach ($data as $i => $value) {
                $new_keys[$key . '_' . $i] = $value;
            }
            $query = preg_replace('#' . $key . '\b#', implode(', ', array_keys($new_keys)), $query);

And the new code:

foreach (array_filter($args, 'is_array') as $key => $data) {
            $new_keys = array();
            foreach (array_values($data) as $i => $value) {
                $new_keys[$key . '_' . $i] = $value;
            }
            $query = preg_replace('#' . $key . '\b#', implode(', ', array_keys($new_keys)), $query);

array_values will return an array with numeric indexes, which is what removes the vulnerability.

Re:Heh

[Anonymous Coward]

You can't get the flu from the vaccine as the virus is deactivated. You get the flu from a version of the virus not in the vaccine and thus were not innoculated against. It's no different to the fact that you don't get the measles from a measles vaccine.

Re:Heh

[Anonymous Coward]

You can't get the flu from the vaccine as the virus is deactivated. You get the flu from a version of the virus not in the vaccine and thus were not innoculated against. It's no different to the fact that you don't get the measles from a measles vaccine.

That's what I thought. But EVERY SINGLE TIME I get a flu shot, sure enough, within 24 hours, I HAVE THE FLU!

Re:Heh

[Anonymous Coward]

There is nothing to think. You don't get the flu from a deactivated virus. That is simply a fact.

Re:Heh

[Anonymous Coward]

I had heard about kids who got polio vaccine and infected their parents who were not inoculated, though a reference to this in google eludes me right now :(

Re:Heh

[Anonymous Coward]

No, what you get are side effects of your body reacting to the newly introduced antigens. These can include fever, aches, and pains. You are interpreting these as the flu, but that is not what you are getting. Unless you get severe side effects, these should be gone within a day or two and is generally preferable to the actual flu. If the side effects are very pronounced in you, by all means skip the vaccine.

Re:Heh

[Anonymous Coward]

I think it was that Bobby Tables kid who designed the API

Re:Heh

[Anonymous Coward]

Since the first way is the natural way to write the loop, the flaw lies in PHP. In fact, I think the underlying vulnerability is the fact that PHP doesn't have (and enforce) separate types for arrays and maps.

Re:Heh

[dotancohen]

If this were a map, say in Python, then the programmer would have to supply the value $i (or in Python, just i) with an ++$i (or in Python i+=1). This can be done in PHP too, so there is no disadvantage to what PHP supports. The problem here is that the programmer is putting dynamic code in the SQL query without sanitizing it first. So what if it is supposed to be variables that are not supposed to be affected by the user? The first rule of preventing SQL injection is to use ZERO outside string variables, even those ostensibly created by your own code. If the data _or metadata_ (i.e. array keys) came in through a function argument, then it is NOT CLEAN.

Of course, the "natural way" to write code is often riddled with buffer overflows, SQL injection, and other naive security issues. This is why you hire a programmer with experience, just as with any other profession. There is no end to the problems with PHP, but this particular bug is not one of them.

This is LITERALLY ...

[Anonymous Coward]

... the definition of irony.

Re: This is LITERALLY ...

[Anonymous Coward]

This is literally an EXAMPLE of irony.

Re: This is LITERALLY ...

[gargleblast]

Well, that depends on:

(a) your definition of literally: (in a literal sense vs. virtually), and

(b) your definition of definition: (explanation vs. perfect example).

The irony of this situation is left as an exercise for the reader.

It's not that hard to do it right

[Art3x]

I understand database abstration layers that let you write:

db_query('select * from table where id = 3')

instead of:

mysql_query('select * from table where id = 3')
or
pgsql_query('select * from table where id = 3')

But I'm not sure I understand why you would want even more abstraction that lets you write:

db_select('*').from('table').where({ id: 3 })

---

Sealing against SQL injection isn't that hard. Don't ever write:

select * from table where id = $id

If you see a dollar sign in an SQL string, it should catch your eye. Instead use parametric queries whenever you can:

select * from table where id = ? or
select * from table where id = $1 or
select * from table where id = :id or
whatever your programming language's syntax is.

Maybe variables in queries are unavoidable, if you have some kind of query building code:

if ($x) {
    $table = 'x';
} else {
    $table = 'y';
}

$q = db_prepare("select * from $table where id = ?");

Does anyone have a better way to build up queries?

Re:It's not that hard to do it right

[Georules]

Prepared statements, stored procedures.

Re:It's not that hard to do it right

[Hewligan]

This does enable precisely the parametric queries you're talking about.

Re:It's not that hard to do it right

[Anonymous Coward]

This only works for a rather static query structures. What if you have a GUI that allows you to dynamically add clauses to the query ? How can you handle that with prepared statements ?

Re:It's not that hard to do it right

[dotancohen]

Go google for "php prepared statements in clause" and see how the very first result is a "solution" that is vulerable to SQL injection.

Re:It's not that hard to do it right

[Anonymous Coward]

ROFL! It's true!

Re:It's not that hard to do it right

[WaffleMonster]

Sealing against SQL injection isn't that hard. Don't ever write:

select * from table where id = $id

Does anyone have a better way to build up queries?

The forbidden example above looks to be the easiest and most readable of all the variants you have provided...

SQL context aware eval() routines with safe default marshaling assumptions are relatively trivial to write.

Much better to give people what they want rather than forcing them to use parameterized semantics where not ideal. If web platforms did this from the beginning CVE databases would be much lighter than they have become.

Re:It's not that hard to do it right

[Anonymous Coward]

Yes: _properly_ escape everything that isn't defined in the code. Remove anything that looks suspicious (ie, in PHP: str_ireplace("DROP *.*", "", $userInput))

I'm just saying... oh, and set the security on your SQL users correctly, too. I'd only let DROP statements exist for highly privileged users (ie, root@localhost), or very specific IP addresses.

Proper Database security can save your datASS in case someone does throw something they shouldn't.

Re: It's not that hard to do it right

[Aethedor]

You might want to take a look how the Banshee PHP framework deals with SQL. With its SQL driver and the security_audit script, it's really hard to have an SQL injection error in your code.

Re: It's not that hard to do it right

[Anonymous Coward]

It's not hard at all. A bit complex at times, maybe, but definitely not hard.

Re: It's not that hard to do it right

[Anonymous Coward]

I never understood what's so hard about use of prepared statements. In the days of positional-parameters-only APIs it used to a bit more cumbersome, but certainly not rocket surgery...

Re:It's not that hard to do it right

[Anonymous Coward]

I use a similar style because it simplifies object-relational mapping in SQLalchemy. It's handy to reduce the code complexity when your data sets are trivial (e.g. using sqlite as a glorified internal file format).

Re:It's not that hard to do it right

[Alioth]

People can write equally vulnerable code in Python or Java or Ruby. The root cause is building SQL queries out of strings instead of using prepared parameterized statements (which I believe PHP has supported for a while -- not as long as Python or Perl or Java or Ruby, after all PHP has those god awful mysql_something functions instead of having something like perl's DBI from the get-go).

I think if you're building queries out of strings you're doing it wrong and asking for an SQL injection vulnerability. From looking at the thread it seems that it was a query that used a list, I think it would have been better to find some other method.

Re:It's not that hard to do it right

[Anonymous Coward]

Sure but in Java you have things like Spring Framework, Hibernate, Java EE standards that have been around for a decade and they are rock-solid foundations to build upon. You really have to think very hard (or very little...) to write SQL injection-vulnerable code in Java these days... While in PHP it seems every year there is a new fashion show of frameworks, eveyone will recommend you something else plus you can see from this article that there are people homebrewing abstraction layers for executing SQL. In 2014 (or whenever Drupal 7 came out).

Re:It's not that hard to do it right

[AqD]

Just dynamically append SQL which uses parameters and then assign the parameters. Generate the parameter names by automatic means if you have to.

That being said, SQL in text form is really stupid and it's even more stupid that ORMs have to construct it from syntax trees (criteria expressions / linq), only for it to be re-parsed into syntax tree later inside the database.

Re:It's not that hard to do it right

[Anonymous Coward]

Except with a huge security hole. Thus making it dangerous to use.

Re:It's not that hard to do it right

[benjymouse]

People can write equally vulnerable code in Python or Java or Ruby.

Nonsensical. Yes, given enough effort, one can certainly write equally vulnerable code in Python or Java or Ruby. That does not prove *anything*

This particular vulnerability is directly triggered by a extremely poor PHP design decision: To conflate arrays and hashtables. The Drupal developers wrote some code that on the surface looks sorta ok. But it assumes that the passed array has numerical indexes.

But in their wisdom, PHP designers decided that separate data structures were too complex for programmers to understand. Alas, arrays as hashtables are the same, since one could view an array as "just" a hashtable that happens to use integers as keys.

The code in question assumed that it could retrieve the "position" of a value in the array and use that. Only, when the position was text with PHP or SQL attack code it led to a vulnerability.

There is NO way to compare that to vulnerabilities created by Python, Java or Ruby developers. Given the exact same lines of thought - which are not at all "out there" the same way of thinking woul NOT have led to a serious vulnerability in any of those languages.

PHP is just bad, bad design. And the bad design is dangerous.

Re:It's not that hard to do it right

[Anonymous Coward]

PHP is just bad, bad design. And the bad design is dangerous.

True.

And yet here in the real world there are places and situations in which it must be used, for a multitude of reasons, some of which are worse than others.

Re:It's not that hard to do it right

[amicusNYCL]

SQL context aware eval() routines with safe default marshaling assumptions are relatively trivial to write.

Could you post a trivial example of one?

Re:It's not that hard to do it right

[amicusNYCL]

Sure but in Java you have things like Spring Framework, Hibernate, Java EE standards that have been around for a decade and they are rock-solid foundations to build upon.

To be fair, the mysqli extension in PHP which supports prepared statements has also been around for over a decade. But you can still go and find any number of tutorials teaching people how to write vulnerable queries by concatenating strings and using the deprecated mysql extension, and you can go to any PHP forum and find people posting questions about code which uses the same. And when you try to teach those people how to do it the correct way, roughly 95% of the time their response is along the lines of "I just need to make it work, then I'll learn about prepared statements." It's a failure of the programmers and tutorials far more than it is a failure of the language. It would be fantastic if PHP outright removed the mysql extension and the mysqli_query function, but that would break a ton of existing applications. And, even so, even when you point people to tutorials about prepared statements they gloss over everything and come back with code like:

$mysqli->prepare('SELECT * FROM table WHERE id=' . $_GET['id']);

Look, I used a prepared statement!

Like I said, it's a failure of the programmers who want the quick and easy way instead of the correct way.

Re:It's not that hard to do it right

[Anonymous Coward]

Just don't use C. It's been so broken for so many years, these vulnerabilities in major C projects proving this time and time again that I am astonished that people still want to use this language/software... Besides, at least where I live, C programmers are a commodity, they come a dime a dozen so how is it a good career choice?

Hmmm... yep, works just as well.

Stored procs?

[Morpeth]

Maybe I'm missing something, but aren't injection attacks a non-issue with parameterized stored procs?

I'm unclear on what their API is doing, but it can't just allowing people to insert anything in a query can it?

Re:Stored procs?

[Elminster Aumar]

I know how to use parameterized inputs (I also know how to spell it). Oh, I also use PHP... So I think you need to reconsider using blanket statements that stress your impulse assumptions.

Re:Stored procs?

[Anonymous Coward]

The problem here is in code to allow you to use lists as parameters. Since you can't pass a list as a parameter to a stored proc, that won't help you.

In order to pass a list, you have to have a query with a list of parameters (one for each item in your input list). It was the creation of this list of parameters that caused the vulnerability.

dom

Re:Stored procs?

[Anonymous Coward]

Concatenate the list into a string in the client, pass it in to the stored proc, then the stored proc parses the input param back out into a list again, which you can run through either as a temp table, cursor, or what have you.
Clumsy, but it works.

Heh

[Anonymous Coward]

Well. Glad I didn't waste time updating to v. 7.31.

Quick, patch whitehouse.gov!!!

[ls671]

Quick, patch whitehouse.gov ;-)

http://radar.oreilly.com/2009/...

http://buytaert.net/whitehouse...

More seriously, I assume they also run some kind of WAF that would catch the attempt even if drupal wasn't yet patched since I do and I am much much smaller.

It's not that hard to do it right

[Anonymous Coward]

Just don't use PHP. It's been so broken for so many years, these vulnerabilities in major PHP projects proving this time and time again that I am astonished that people still want to use this language/software... Besides, at least where I live, PHP programmers are a commodity, they come a dime a dozen so how is it a good career choice? Learn Python and/or Java and/or Ruby and/or C and/or C++ - there are so many great languages/frameworks out there, why would you want to waste your life on PHP which is a laughing stock pretty much everywhere outside PHP communities?

CMSes are crap anyway

[Anonymous Coward]

Seriously, why would one use a CMS for a serious project? They limit you more than they help you if you hit their limits. Just pick some barebone framework, like Laravel 4 (or 5) and start doing your mojo.

To the people bitching about PHP, I can only say that it's here to stay. The syntax is simple, the technology has loads of helpful functionality and the latest frameworks (PHP-FIG compatible ones) like Symfony, Laravel, Zend, Fuel and Aura are thousand times better than anything Drupal and/or Wordpress can bring to the table, using modern techniques and design patterns. Security flaws happen because of programmer incompetence, not because of a language. Claiming that a language is not secure is just stupid and proves how much of an expert you are. Everything can be abstracted. Even the most flawed of systems. Also are you implying that projects written in C++/Java/Ruby does not get hacked just because they use the said tech?

You do realize that Facebook's frontend is powered by PHP because to them it is just cheaper to have a dev team optimizing HHVM and tons of PHP devs buildings things right? (Recently moving from PHP to "Hack"). Go ahead and SQL inject facebook smartass :)

Re: Drupal 6, or how NOT to code

[Anonymous Coward]

The Irony Dept. just called; they said your shipment of fail just arrived...