Heine

  • Home
  • Drupal
  • About
Home

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));
  }  
}
  • Drupal
  • Security
  • Planet Drupal

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.

Recent posts

  • Teampassword manager's password generator is biased
  • Other vectors for SA-CORE-2014-005?
  • Lazy loading: hook_hook_info is for hook owners only.
  • "Always offline" problem in EA's Origin due to antivirus
  • From bug to exploit - Bakery SSO
more

Security reviews

I provide security reviews of custom code, contributed modules, themes and entire sites via LimoenGroen.

Contact us for a quote.

Follow @ustima

Copyright © 2021 by Heine Deelstra. All rights reserved.

  • Home
  • Drupal
  • About