In a pagecallback I executed the following with <script>alert('xss')</script> as script code:

$result = db_select('node', 'n')->fields('n', array('nid', 'title'))->orderby('title', ASC; SELECT [inject script code here] uid FROM users;)->execute();

And got a nice popup on the error screen.

Output was:

<h1>Additional uncaught exception thrown while handling exception.</h1><h2>Original</h2><p>PDOException: SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active.  Consider using PDOStatement::fetchAll().  Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.: SELECT b.*
FROM 
{block} b
WHERE  (b.theme = :db_condition_placeholder_0) AND (b.status = :db_condition_placeholder_1) 
ORDER BY b.region ASC, b.weight ASC, b.module ASC; Array
(
    [:db_condition_placeholder_0] => garland
    [:db_condition_placeholder_1] => 1
)
 in _block_load_blocks() (line 658 of D:\www\core7\modules\block\block.module).</p><h2>Additional</h2><p>PDOException: SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active.  Consider using PDOStatement::fetchAll().  Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.: INSERT INTO {watchdog} (uid, type, message, variables, severity, link, location, referer, hostname, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => php
    [:db_insert_placeholder_2] => %type: %message in %function (line %line of %file).
    [:db_insert_placeholder_3] => a:6:{s:5:"%type";s:12:"PDOException";s:8:"%message";s:573:"SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active.  Consider using PDOStatement::fetchAll().  Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.: SELECT b.*
FROM 
{block} b
WHERE  (b.theme = :db_condition_placeholder_0) AND (b.status = :db_condition_placeholder_1) 
ORDER BY b.region ASC, b.weight ASC, b.module ASC; Array
(
    [:db_condition_placeholder_0] => garland
    [:db_condition_placeholder_1] => 1
)
";s:9:"%function";s:20:"_block_load_blocks()";s:5:"%file";s:39:"D:\www\core7\modules\block\block.module";s:5:"%line";i:658;s:14:"severity_level";i:3;}
    [:db_insert_placeholder_4] => 3
    [:db_insert_placeholder_5] => 
    [:db_insert_placeholder_6] => http://l/core7/scratch/dbtest/ASC; SELECT <script>alert('xss')</script>uid FROM users;
    [:db_insert_placeholder_7] => 
    [:db_insert_placeholder_8] => 127.0.0.1
    [:db_insert_placeholder_9] => 1276715728
)
 in dblog_watchdog() (line 131 of D:\www\core7\modules\dblog\dblog.module).</p><hr /><h1>Uncaught exception thrown in session handler.</h1><p>PDOException: SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active.  Consider using PDOStatement::fetchAll().  Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.: INSERT INTO {sessions} (uid, cache, hostname, session, timestamp, sid) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5) ON DUPLICATE KEY UPDATE uid=VALUES(uid), cache=VALUES(cache), hostname=VALUES(hostname), session=VALUES(session), timestamp=VALUES(timestamp); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => 0
    [:db_insert_placeholder_2] => 127.0.0.1
    [:db_insert_placeholder_3] => batches|a:1:{i:1;b:1;}
    [:db_insert_placeholder_4] => 1276715728
    [:db_insert_placeholder_5] => X
)
 in _drupal_session_write() (line 177 of D:\www\core7\includes\session.inc).</p><hr />

As you can see, the placeholder_6 [:db_insert_placeholder_6] => http://l/core7/scratch/dbtest/ASC; SELECT <script>alert('xss')</script>uid FROM users; contains non-escaped data.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

FileSize
523 bytes

I wasn't able to reproduce the exact error, but I think it only happens when the exception is displayed with the new _drupal_render_exception_safe() function, which uses strtr() instead of t(). Because when there is a single PDO exception handled by the maintenance theme, t() is used and the message is check_plain'ed.

So, what about this simple change?

Berdir’s picture

Status: Active » Needs review
jbrown’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

It's pretty much the most awesome thing ever that a function called _drupal_render_exception_safe() contains a security vulnerability. :P~

Can we please get a test for this?

Heine’s picture

I've been looking at this function a bit more, and _drupal_render_exception_safe is only used to output HTML from a plaintext error message. That means check_plain is appropriate. The patch is good, though we might want to escape the before inserting them in the future (and maybe factor this out of t()).

When starting the thread I wanted to propose the function for most misleading functionname of the year, until I realized it is _exception_safe .

jbrown’s picture

Yes - the 'safe' refers to the exceptions - it has a 'no throw guarantee':

http://en.wikipedia.org/wiki/Exception_handling#Exception_safety

Dave Reid’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.14 KB

Patch attached for review that uses the error handler tests to test for the escaping. Drupal is pretty awesome

Berdir’s picture

Status: Needs review » Needs work

I don't think that actually tests the change in this patch.

_drupal_render_exception_safe() is only called under very special circumstances: http://api.drupal.org/api/function/_drupal_render_exception_safe/7

To be able to actually test the change in this patch, it would need to go (for example) through an exception thrown in a shutdown function. And I think that needs to happen in a test module so that we can actually verify that check_plain() is called correctly.

There is already a test callback for drupal_register_shutdown_function here: http://api.drupal.org/api/function/system_test_page_shutdown_functions/7.

Se we could for example do something like 'throw new Exception("Drupal is awesome");' and verify what's displayed then.

This would actually be a test for _drupal_render_exception_safe() too, I don't think we have any tests for that currently.

Crell’s picture

Subscribe.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.32 KB

Made a patch of #8, not sure if the inline comments are OK... I made sure that the test fails without the added check_plain().

I'm wondering if we should incorporate the tests from #7 anyway. They do not verify the bug reported here, but it's probably a valid test nevertheless.

davidburns’s picture

Status: Needs review » Reviewed & tested by the community

applied and tested locally. looks good.

User concern is double escaping, at this point it's not really a concern this isn't presented to the user.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/simpletest/tests/system_test.module
@@ -303,5 +303,11 @@ function _system_test_second_shutdown_function($arg1, $arg2) {
+  throw new Exception('Drupal is <blink>awesome</blink>');

Let's end sentences with a point (.).

Can we get a quick re-roll?

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.33 KB

just a quick re-roll based on #12

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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