potential security issues: output of choices and query generation [#560344]

In a visual review of the module today I found two oddities:

First - 'data' => t($yes_no[$choice]) - seems like it could be a XSS vulnerability. I suggest calling check_plain on this data.

Second - the query being built up like $sql .= ' AND ('. implode(' OR ', $uids) .')'; is not necessarily a vulnerability because the data comes from a known source but it is still bad form to use data like that. Better is to use $placeholders as shown on this page: http://drupal.org/writing-secure-code

I'm reporting these here instead of to the security team because the module doesn't have an official release. When a module has no "official release" any security issues can be discussed in the public.

This review is because the Drupal Association would like to use the module on the association site to help plan meetings. We would like to get the module to a stable and secure state and then ideally get an official release as well.

Thanks for creating a very useful module!

Comments

Comment #1

zeropaper

French

commented 12 November 2009 at 12:37

I'll take a deeper look at that asap. Thanks for the notice by the way.
Just as a clarification.
The variable "$yes_no" is defined like $yes_no = array('no', 'yes');. Wich means it's somehow hardcoded.
But what's not hardcoded is the translation (use of the t() function).

So, are you meaning that the security issue may come from the $choice variable or the use of $yes_no[$choice]?

It would look weird to me to use something like check_plain(t($yes_no[$choice])).
What would be much more "elegant" to me would be to use something like t($yes_no[intval($choice)]).
But I might be wrong...

For the query, you're absolutely right and I'll do the change right now.

I'll made an official release directly after those issues will be clarified.

Comment #2

greggles

he/him

English

Denver, Colorado, USA

commented 12 November 2009 at 14:07

I didn't look where $yes_no came from, so that's probably OK, but as you point out if it uses t() options then those should probably be check_plained.

However - check_plain(t($yes_no[$choice])) is not equivalent to t($yes_no[intval($choice)]) so please don't confuse those.

Comment #3

zeropaper

French

commented 12 November 2009 at 14:31

Thanks for the quick answer,
I will then use check_plain(t($yes_no[$choice])) (in order to be sure).
I'll take now a closer look at the SQL statement building (which is far from being the nicer code I wrote), but as you may see in

  $sql = "SELECT delta, uid FROM {dudel} WHERE nid=%d";
  $args = array($node->nid);
  
  if (!empty($users)) {
    if (is_array($users)) {
      $uids = array();
      foreach ($users as $account) {
        if (is_object($account)) {
          $args[] = $account->uid;
          $uids[] = 'uid=%d';
        }
        elseif (is_numeric($account)) {
          $args[] = $account;
          $uids[] = 'uid=%d';
        }
      }
      $sql .= ' AND ('. implode(' OR ', $uids) .')';
    }
    elseif (is_object($users)) {
      $sql .= ' AND uid=%d';
      $args[] = $users->uid;
    }
  }
  
  $sql .= ' ORDER BY uid';
  $query = db_query($sql, $args);

the uids are passed thru the $args variable. I mean, it also looks overkilled to me but really not unsecure. I guess something like ...uid IN ($placeholder)... could be better (your expertise about point that would be welcome).