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

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

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

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?