Description of Vulnerability:

The Data module contains multiple Cross Site Scripting (XSS) vulnerabilities because it fails to sanitize table descriptions, field names or labels before display. This results in multiple stored XSS as well as DOM based XSS vulnerabilities. Drupal site users with the ability to create or edit tables using the Data module could inject arbitrary HTML into administrative pages.

The Data module also contains numerous SQL injection vulnerabilities because it fails to sanitize values for table names or column names before invoking SQL statements. This allows users with the ability to create or edit tables managed by the Data module to perform SQL injection attacks.

Systems affected:

Drupal 6.20 with Data 6.x-1.0-alpha14 was tested and shown to be vulnerable.

Impact

User could inject arbitrary scripts into pages affecting site users. This could result in administrative account compromise leading to web server process compromise. A more likely scenario would be for an attacker to inject hidden content (such as iframes, applets, or embedded objects) that would attack client browsers in an attempt to compromise site users' machines. This vulnerability could also be used to launch cross site request forgery (XSRF) attacks against the site that could have other unexpected consequences.

Mitigating factors:

In order to exploit this vulnerability the attacker must have credentials to an authorized account that has been assigned the permissions to administer or edit in the Data module. This could be accomplished via social engineering, brute force password guessing, or abuse or legitimate credentials.

CVE-Identifiers

  • CVE-2011-2714 for the XSS-vulnerabilities.
  • CVE-2011-2715 for the SQL-injection vulnerabilities.

Comments

greggles’s picture

Subscribe.

kbk’s picture

Priority:Normal» Critical
emackn’s picture

StatusFileSize
new5.75 KB

Added check_plain to the table data when using drupal_set_title().
Updated the data_search.module to take more care with fields and table names.

Should apply to alpha14 or DRUPAL-6--1 branch.

see comment #4 for new patch.

emackn’s picture

Assigned:Unassigned» emackn
Status:Active» Needs review

figures.. found something else. ;)

emackn’s picture

StatusFileSize
new16.26 KB

OK, I think I got all of it.

greggles’s picture

Status:Needs review» Needs work

There's a few items:

<?php
+    t('Are you sure you would like to revert table !table? This will reset all information about this table its definition in code. This action cannot be undone.', array('!table' => check_plain($table->get('name')))),
?>

Would be better as:

<?php
+    t('Are you sure you would like to revert table @table? This will reset all information about this table its definition in code. This action cannot be undone.', array('2table' => $table->get('name'))),
?>
emackn’s picture

Status:Needs work» Needs review
StatusFileSize
new19.08 KB

Here is an updated patch with the suggested changes. Also found more uses of !table with t(), so I updated those as well.

alex_b’s picture

Status:Needs review» Needs work

- Around // Update meta data. the patch check_plains stuff that actually goes to the database.
- Where possible I would have the DataTable object handle escaping - so that $table->addIndex(db_escape_table($new['name'])); is not necessary.
- Do tests complete?

kbk’s picture

Bump. I don't think there has been an advisory for this: http://drupal.org/security/contrib

alex_b’s picture

#9: There are no security advisories for alpha or beta modules.

hms’s picture

Two CVE-identifiers have been assigned for these security issues:

CVE-2011-2714 for the XSS-vulnerabilities.
CVE-2011-2715 for the SQL-injection vulnerabilities.

In my opinion Drupal security advisory -policy could be improved. Advisory should be released if there is production users for alpha-versions and no stable-version is available of module in question. Please note that I don't have any idea how many alpha-releases there is in the modules overall. :)

greggles’s picture

@hms, how do you suggest the security team determine if there are production users? It's an impossible task making your suggestion also impossible.

I suggest instead that production sites not use alpha/dev code unless they are willing to audit it.

ianchan’s picture

subscribe

budda’s picture

So are the changes in comment #7 going in to the code and rolling a new release?

joachim’s picture

Version:6.x-1.0-alpha14» 6.x-1.x-dev
StatusFileSize
new13 KB
-    $row[] = $table->get('name');
+    $row[] = check_plain($table->get('name'));

This doesn't seem like the best approach to me.
Surely we should be cleaning up the data table in the DataTable class's constructor, and then we know it's okay to use it wherever?

I've applied this patch to the new 7.x branch, with the following changes:

- changes to data_node, data_search, data_taxonomy failed completely and those modules have not been updated to 7 anyway. So I've skipped those; further work needed.
- drupal_set_title() doesn't need sanitizing on D7, it check_plain()s itself
- made the change to the DataTable class outlined above.

- #1056470 by emackn, joachim: Fixed multiple SQL injection and XSS vulnerabilities.

Leaving as needs work on D6 -- if someone can take care of the suggested change and reroll I'll commit it.

joachim’s picture

Status:Needs work» Fixed

> Surely we should be cleaning up the data table in the DataTable class's constructor, and then we know it's okay to use it wherever?

On second thoughts, that would break tables which people have managed to create with insecure names (not sure if that's possible but eh).

In the interests of #1355346: Create stable release of Data for 6.x, I'm going to commit the patch in #7 and we can tweak things later.

- #1056470 by emackn: Fixed multiple SQL injection and XSS vulnerabilities.

Status:Fixed» Closed (fixed)

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

mr.baileys’s picture

Issue summary:View changes

Updated issue summary.