Drupal Fixes Highly Critical SQL Injection Flaw 54
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."
Is Drupal 6.x Affected? (Score:1)
I've seen no mention of whether or not Drupal 6.x is vulnerable; are they?
Re:Is Drupal 6.x Affected? (Score:4, Informative)
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: (Score:3)
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: (Score:2)
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: (Score:3)
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/include
Heh (Score:5, Funny)
You had one job!
Re: (Score:2, Insightful)
Re: (Score:3)
No, it's more like they were enforcing parametrised queries on top of an API that allows non-parametrised queries.
Re:Heh (Score:5, Informative)
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: (Score:1)
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: (Score:2)
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, e
Re: (Score:1)
Kind of like a flu vaccine that give you... the flu
Re: (Score:2)
Well, that depends on:
(a) your definition of literally: (in a literal sense vs. virtually) [merriam-webster.com], and
(b) your definition of definition: (explanation vs. perfect example) [merriam-webster.com].
The irony of this situation is left as an exercise for the reader.
It's not that hard to do it right (Score:5, Interesting)
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 :id or
select * from table where id = $1 or
select * from table where id =
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: (Score:1)
Re: (Score:1)
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: (Score:3)
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: (Score:2)
This does enable precisely the parametric queries you're talking about.
Re: (Score:2)
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: (Score:2)
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 (Score:2)
Re: (Score:3)
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. Fr
Re: (Score:2)
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 o
Re: (Score:2)
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 t
Stored procs? (Score:2)
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: (Score:2)
Quick, patch whitehouse.gov!!! (Score:2)
Quick, patch whitehouse.gov ;-)
http://radar.oreilly.com/2009/... [oreilly.com]
http://buytaert.net/whitehouse... [buytaert.net]
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.