The selection statement in protected_node_requirements() is vulnerable to SQL injection. I'm not filing this with the security team because all permissions are defined be modules, however it's possible that a permission might accidentally cause a problem.

    $likes = array();
    foreach ( $perms as $perm ) {
      $likes[] = "perm LIKE '%" . $perm . "%'";
    }
    $sql = "SELECT rid FROM {permission} WHERE " . implode( ' OR ', $likes );
    $roles = db_affected_rows( db_query( $sql ) );

Since perm is not escaped in any way, it's possible that a permission may inaccurately be matched or end the SQL statement accidentally (such as if the permission contains a single quote), causing a SQL error.

Instead this should be:

    $likes = array();
    foreach ($perms as $perm) {
      $likes[] = "perm LIKE '%%s%'";
    }
    $sql = "SELECT rid FROM {permission} WHERE " . implode(' OR ', $likes);
    $roles = db_affected_rows(db_query($sql, $perms));

There's also a function specifically for this kind of replacement, but I couldn't figure out how it would apply in this situation: http://api.drupal.org/db_placeholders.

Comments

AlexisWilke’s picture

Assigned: Unassigned » AlexisWilke
Status: Active » Fixed

I used your code because I like it. 8-)

But that was not an SQL injection because:

The $perm contents are taken from protected_node_perm() which is limited to:

(a) the module permissions (and we know we don't inject anything bad here); and

(b) node type names which are limited to [a-z0-9_]+

So all names were under control and could not generate an SQL injection. This being said, fixing such things is always a good idea (in case someone was thinking to do a copy + paste...)

Thank you.
Alexis Wilke

See:
http://drupalcode.org/project/protected_node.git/commit/e2efd11

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.