IN (%s); a security vulnerability waiting to happen

Heine —Tue, 2007/06/19 - 00:14

I've written before about (ab)using %s in IN clauses such as:

// BAD ($from_user is an array of numbers):
db_query("SELECT t.s FROM {table} t WHERE t.field IN (%s)", implode(',', $from_user));

The problem with such a query is that you open the possibility that a code path develops where unfiltered content ends up in the variable and thus in your query.
Here's a simplified, real life example where this happened during a bugfix. foo() is a function that runs only on the creation of new nodes and calls bar() to fetch a node id to redirect (== display) to.

function foo() {
$nid = bar($node->type, $node->uid, $node->og_groups);
// Display nid further downstream
}

Obviously, $node->og_groups won't work for new nodes, and, since the URL to create these nodes was carefully crafted, it could be replaced by:

function foo() {
$nid = bar($node->type, $node->uid, $_REQUEST['gids']);
// Display nid further downstream
}

Bug solved. But before closing the issue, let's take a look at bar, to ensure it is still secure.

// [...]
function bar($typename, $uid, $og_groups) {
if (is_array($og_groups)) {
foreach ($og_groups as $gid) {
$groups[] = $gid;
}
$groups = join(',', $groups);
return db_result(db_query("SELECT n.nid FROM {node} n INNER JOIN {og_ancestry} oga ON n.nid = oga.nid WHERE n.uid = %d AND n.type = '%s' AND oga.group_nid IN (%s)", $uid, $typename, $groups));
}
}

O no! After the bugfix, we're passing in user-supplied data, and have an SQL injection vulnerability on our hands, as one can simply manipulate $_REQUEST['gids']:

node/add/content_foo?gids[]=0) UNION SELECT s.sid FROM sessions s WHERE s.uid = 1 /*
encoded: node/add/content_foo?gids[]=0)%20UNION%20SELECT%20s.sid%20FROM%20sessions%20s%20WHERE%20s.uid%20=%201%20%2F*

Which results in the following query:

SELECT n.nid FROM node n INNER JOIN og_ancestry oga ON n.nid = oga.nid WHERE n.uid = [uid] AND n.type = 'content_foo' AND oga.group_nid IN (0) UNION SELECT s.sid FROM sessions s WHERE s.uid = 1 /*)

The UNION will place the session id of the administrative user in $nid, and, since this is subsequently displayed to the user, he/she is able to take over the session, effectively becoming the all-powerful user 1.

The rewritten function that is no longer vulnerable:

function bar($typename, $uid, $og_groups) {
if (is_array($og_groups)) {
foreach ($og_groups as $gid) {
$groups[] = $gid;
}
$args = array_merge(array($typename, $uid), $groups);
$placeholders = implode(',', array_fill(0, count($groups), '%d'));
return db_result(db_query("SELECT n.nid FROM {node} n INNER JOIN {og_ancestry} oga ON n.nid = oga.nid WHERE n.uid = %d AND n.type = '%s' AND oga.group_nid IN ($placeholders)", $args));
}
}

Comments

Update

Submitted by Heine on Tue, 2011/01/11 - 11:29

This post was written in 2007. Drupal 6 got the function db_placeholders to make this much easier.

Drupal 7 finally supports secure expansion of arrays. See DBTNG: Static queries for details.

Heine