A few places in core use the following construct:

[...] WHERE nid IN(%s)', implode(',', $nodes));

Because %s is not surrounded by quotes, it can be interpreted as SQL (not just data).

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));

The above code is from a real life example. $groups is a parameter passed to the function. When the caller changed to pass $_REQUEST['gid'] this became the site of an SQL injection vulnerability with such URLs as:

?gids[]=0) UNION SELECT s.sid FROM sessions s WHERE s.uid = 1 /*

In our case, $nodes is an array of nids and this does not constitute a vulnerability. It is very brittle however as this code relies on the caller to remain secure. It is therefore all to easy to introduce vulnerabilities.

The attached patch simply replaces IN (%s) with IN (%d, %d, ...)

CommentFileSizeAuthor
#14 in-s-D6-167284-14.patch8.46 KBpwolanin
#7 in-s-hardening_2.patch6.95 KBpwolanin
in-s-hardening.patch6.94 KBheine

Comments

heine’s picture

heine commented
Status: Active » Needs review
pwolanin’s picture

pwolanin commented
Status: Needs review » Active

An important cleanup for 5.x

It would be nice to have a more robust solution for 6.x. For example, make %s wrap its return value with single quotes (as %b already does).

Perhaps also a "multiple integer" placeholder for 6.x for _db_query_callback to simplify this sort of common call? For example %dd:


case '%dd':
  // Use tab , ; and : as tokenizing characters 
  $tok = strtok(array_shift($args), "\t,;:");
  $ret = array();
  while ($tok !== FALSE) {
      $ret[] = (int)$tok;
      $tok = strtok("\t,;:");
  }
  return implode(',', $ret);
pwolanin’s picture

pwolanin commented
Status: Active » Needs review
moshe weitzman@drupal.org’s picture

moshe weitzman@drupal.org commented

Nice example :)

Could you add a helper function like below and use it throughout? Is easier IMO than remembering the incantation yourself.


function drupal_placeholders($arguments, $type = "'%s'") {
  return implode(',', array_fill(0, count($arguments), $type);
}
pwolanin’s picture

pwolanin commented

see: http://drupal.org/node/167649 for a D6 issue for Moshe's suggestion or a another fix. This issue is D5, so I think a new APi function is out of the scope.

moshe weitzman’s picture

moshe weitzman commented

there is nothing wrong with *adding* a helper function to an existing drupal release. it is part of fixing this issue in a clean fashion.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work
StatusFileSize
new in-s-hardening_2.patch6.95 KB

there is an error in the taxonomy module part of this:

warning: array_fill() [function.array-fill]: Number of elements must be positive in modules/taxonomy/taxonomy.module on line 1372.

revised patch attached.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

rechecked - should be good to go.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to 5.x

kkaefer’s picture

kkaefer commented
Version: 5.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

This needs to go into 6 as well!

pwolanin’s picture

pwolanin commented

Yes - I was holding off on 6.x until the various split patches were done. I think we can proceed now.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Remove IN (%s) - a vulnerability waiting to happen. » Regression: Remove IN (%s) - a vulnerability waiting to happen.
Priority: Normal » Critical
pwolanin’s picture

pwolanin commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new in-s-D6-167284-14.patch8.46 KB

ok, this patch adds a helper function as Moshe wanted, and catches the same instances of IN(%s) in D6.

Also, a little doxygen cleanup in database.inc to get a single line description for a couple related functions.

Zothos’s picture

Zothos commented

Someone has a hint for me, how to test this patch? I think its critical but and i wanne help to get this thing in, unfortunately i have no idea how to test this issue on my test domain. XD

pwolanin’s picture

pwolanin commented

@Zothos - the security hole is a theoretical one, rather than one that's obviously exploitable. So, in terms of testing this patch - nothing should break or be different after applying the patch. Especially the node admin form actions at admin/content/node, per-role block permissions, and general functioning of taxonomy pages and user permissions.

Zothos’s picture

Zothos commented

I am sorry. Then my thoughts about this issue were completely wrong.

drewish’s picture

drewish commented
Status: Needs review » Reviewed & tested by the community

Looks real good. Tested:
- that the admin/build/block and user/* blocks work correctly.
- publishing, un-publishing, promoting, demoting, making sticky and unsticking both a single node and multiple nodes.
- the taxonomy/term/* page select nodes correctly.
- did some basic tests on user_access.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Sounds good, looks good. Committed. Thanks!

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Fixed » Needs work

Actually, this needs to be documented in the upgrade docs, so leaving this CNW.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Component: other » documentation
pwolanin’s picture

pwolanin commented
Status: Needs work » Fixed

done: http://drupal.org/node/114774#placeholder_helper

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)