Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#13 | check_plain_with_test.patch | 2.33 KB | dawehner |
#10 | check_plain_with_test.patch | 2.32 KB | Berdir |
#7 | 829484-check_plain_display_safe.patch | 3.14 KB | Dave Reid |
#1 | check_plain_display_safe.patch | 523 bytes | Berdir |
Comments
Comment #1
BerdirI 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?
Comment #2
BerdirComment #3
jbrown CreditAttribution: jbrown commentedLooks good.
Comment #4
webchickIt'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?
Comment #5
Heine CreditAttribution: Heine commentedI'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 .
Comment #6
jbrown CreditAttribution: jbrown commentedYes - the 'safe' refers to the exceptions - it has a 'no throw guarantee':
http://en.wikipedia.org/wiki/Exception_handling#Exception_safety
Comment #7
Dave ReidPatch attached for review that uses the error handler tests to test for the escaping. Drupal is pretty awesome
Comment #8
BerdirI 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.
Comment #9
Crell CreditAttribution: Crell commentedSubscribe.
Comment #10
BerdirMade 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.
Comment #11
davidburnsapplied 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.
Comment #12
Dries CreditAttribution: Dries commentedLet's end sentences with a point (.).
Can we get a quick re-roll?
Comment #13
dawehnerjust a quick re-roll based on #12
Comment #14
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.