role names are not properly sanitized by flag module

This was reported to the security team, but since 'administer permissions' already allows for arbitrary control of a site (in most cases) the Security team considers this to be a normal bug and not a security vulnerability.

Author: Justin C. Klein Keane

Description of Vulnerability:
- - -----------------------------
Drupal (http://drupal.org) is a robust content management system (CMS)
written in PHP and MySQL that provides extensibility through various
third party modules. The Flag module (http://drupal.org/project/flag)
"is a flexible flagging system that is completely customizable by the
administrator. Using this module, the site administrator can provide any
number of flags for nodes, comments, or users. Some possibilities
include bookmarks, marking important, friends, or flag as offensive.
With extensive views integration, you can create custom lists of popular
content or keep tabs on important content."

The Flag module contains several cross site scripting vulnerabilities
because it does not properly sanitize output of role names before
display. Additionally the Flag module contains a SQL injection
vulnerability because it does not properly sanitize variables before
concatenating them into a SQL query.

Systems affected:
- - -----------------
Drupal 6.12 with Flag 6.x-1.1 was tested and shown to be vulnerable.

Impact:
- - -------
XSS vulnerabilities may expose site administrative accounts to
compromise which could lead to web server process compromise. SQL
injection attack can manipulate the Drupal database, compromising data,
exposing credentials, and could allow injection of XSS attacks in node
content.

Mitigating factors:
- - -------------------
The Flag module must be installed. To carry out an role name XSS
exploit against the Flag module the attacker must be able to inject
malicious content into role names, which is possible for authenticated
users with the 'administer permissions' permission. To carry out the
content type XSS exploit against the Flag module the attacker must be
able to inject malicious content into content type names, which is
possible for authenticated users with the 'administer content types'
permission. The SQL injection vulnerability has not yet been shown to
be exploitable.

Technical details:
- ------------------------
The Flag module uses the Drupal user_roles() function from user.module,
which fails to sanitize role names before returning them (this is a
known issue in Drupal 6.12 -
http://lampsecurity.org/drupal-role-xss-vulnerability). On line 416 of
flag.module the $roles variable is composed without sanitizing the
return value of the user_roles() function. This leads to arbitrary HTML
injection. The Flag module also fails to sanitize content type names
returned by the node_get_types() function in flag.module on line 708.

The variables used to construct $result on line 40 of
includes/flag_handler_argument_content_id.inc are not sanitized and
could be used to perform SQL injection.

Patch
- -------
Applying the following patch mitigates these threats.

diff -upr flag/flag.inc flag_fixed/flag.inc
- --- flag/flag.inc     2009-03-14 02:13:54.000000000 -0400
+++ flag_fixed/flag.inc 2009-06-05 13:10:33.000000000 -0400
@@ -227,6 +227,7 @@ class flag_flag {
    }
    // But checkboxes need some massaging:
    $this->roles = array_values(array_filter($this->roles));
+    foreach ($this->roles as $key=>$val) {$this->roles[$key] =
check_plain($val);}
    $this->types = array_values(array_filter($this->types));
    // Clear internal titles cache:
    $this->get_title(NULL, TRUE);
diff -upr flag/flag.module flag_fixed/flag.module
- --- flag/flag.module  2009-03-14 02:13:54.000000000 -0400
+++ flag_fixed/flag.module      2009-06-05 13:02:55.000000000 -0400
@@ -413,6 +413,7 @@ function theme_flag_admin_page($flags, $
    ));

    $roles = array_flip(array_intersect(array_flip(user_roles()),
$flag->roles));
+    foreach ($roles as $key=>$val) {$roles[$key] = check_plain($val);}
    $rows[] = array(
      $flag->name,
      $flag->content_type,
@@ -440,6 +441,7 @@ function theme_flag_admin_page($flags, $
      ));

      $roles = array_flip(array_intersect(array_flip(user_roles()),
$flag->roles));
+    foreach ($roles as $key=>$val) {$roles[$key] = check_plain($val);}
      $rows[] = array(
        $flag->name,
        $flag->module,
@@ -685,11 +687,12 @@ function flag_form(&$form_state, $name,
    '#weight' => 1,
    '#access' => empty($flag->locked['global']),
  );
- -
+  $roles = user_roles(TRUE);
+  foreach ($roles as $key=>$val) $roles[$key]=check_plain($val);
  $form['roles'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Roles that may use this flag'),
- -    '#options' => user_roles(TRUE),
+    '#options' => $roles,
    '#default_value' => $flag->roles,
    '#required' => TRUE,
    '#description' => t('Checking <em>authenticated user</em> will
allow all logged-in users to flag content with this flag. Anonymous
users may not flag content.'),
@@ -702,10 +705,12 @@ function flag_form(&$form_state, $name,
    $form['roles']['#value'] = $flag->roles;
  }

+  $types = node_get_types('names');
+  foreach ($types as $key=>$val) $types[$key] = check_plain($val);
  $form['types'] = array(
    '#type' => 'checkboxes',
    '#title' => t('What nodes this flag may be used on'),
- -    '#options' => node_get_types('names'),
+    '#options' => $types,
    '#default_value' => $flag->types,
    '#description' => t('Check any node types that this flag may be
used on. You must check at least one node type.'),
    '#required' => TRUE,
diff -upr flag/includes/flag_handler_argument_content_id.inc
flag_fixed/includes/flag_handler_argument_content_id.inc
- --- flag/includes/flag_handler_argument_content_id.inc        2008-12-03
09:10:00.000000000 -0500
+++ flag_fixed/includes/flag_handler_argument_content_id.inc    2009-06-05
13:06:28.000000000 -0400
@@ -34,7 +34,7 @@ class flag_handler_argument_content_id e
    $titles = array();
    $placeholders = implode(', ', array_fill(0, sizeof($this->value),
'%d'));

- -    $result = db_query("SELECT o.". $views_info['title field'] ." FROM
{". $views_info['views table'] ."} o WHERE o.". $views_info['join
field'] ." IN ($placeholders)", $this->value);
+    $result = db_query("SELECT o.%s FROM {%s} o WHERE o.%s IN
($placeholders)", $views_info['title field'], $views_info['views
table'], $views_info['join field'], $this->value);
    while ($title = db_fetch_object($result)) {
      $titles[] = check_plain($title->$views_info['title field']);
    }