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:
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.
$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:
$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']:
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:
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:
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:29This 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.