Home

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

Mon, 18/06/2007 - 22:14 — Heine

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

  1. // BAD ($from_user is an array of numbers):
  2. 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.

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

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:

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

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

  1. // [...]
  2. function bar($typename, $uid, $og_groups) {
  3.   if (is_array($og_groups)) {
  4.     foreach ($og_groups as $gid) {
  5.       $groups[] = $gid;
  6.     }
  7.     $groups = join(',', $groups);
  8.     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));
  9.   }
  10. }

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']:

  1. node/add/content_foo?gids[]=0) UNION SELECT s.sid FROM sessions s WHERE s.uid = 1 /*
  2. 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:

  1. 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:

  1. function bar($typename, $uid, $og_groups) {
  2.   if (is_array($og_groups)) {
  3.     foreach ($og_groups as $gid) {
  4.       $groups[] = $gid;
  5.     }
  6.     $args = array_merge(array($typename, $uid), $groups);
  7.     $placeholders = implode(',', array_fill(0, count($groups), '%d'));
  8.     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));
  9.   }  
  10. }

Average: 2.5 (26 votes)

Comments

Wed, 27/06/2007 - 14:11 — Arman (not verified)

Hi Heine

Hey,

Aha, hier hang je tegenwoordig rond. Dat is wel wat anders dan repellents!
Ik denk dat je niets meer te maken wilt hebben met iedereen die een collega van je was in je AIO-tijd. Niettemin, was ik benieuwd hoe het met je is en of je ook nog op mijn promotie komt, want je bent van harte uitgenodigd. Gewoon niets aantrekken van Luis (arbeidsimmigrant, daar hebben we er al teveel van in Ned, hahaha).
Wellicht is het ontvangen van een bericht van mij een reden dat de demonen uit je AIO-tijd weeer losgelaten worden. Derhalve, zal ik het hier bij laten. Oh ja, mijn promotie is 24 oktober om 12:45. Niet wetenschappelijk hoogstaand, maar toch een titel. Ik weet die kan je niet eten, maar toch leuk om vrouwen danwel mannen mee te imponeren, hihi!

mit freundlichen Grüssen 8ja, ik hen een Duitse vriendin),

Arman