Proposed commit message:

Issue #2371141 by pfrenssen, znerol: Fixed XSS vulnerability when displaying exception backtrace.

In DefaultExceptionSubscriber::onHtml() the entire backtrace is marked as safe before it is passed to drupal_set_message(). Since the backtrace outputs the contents of method arguments this opens up a XSS vulnerability if any of the arguments contain unsafe HTML.

Example to replicate the bug: install the standard profile, apply the 2371141-example.txt patch, log in as UID1 and visit admin/structure/views/view/files/edit/block_1.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

Issue summary: View changes
FileSize
45.8 KB
pfrenssen’s picture

Here's the fix and an example that triggers an exception containing unsafe HTML in method arguments.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Starting on a test.

Fabianx’s picture

So where again was our issue to review all SafeMarkup::set() calls ...

Thanks so much pfrenssen for catching that.

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

I'm having trouble writing a test for this. My test case is very simple: I'm throwing an exception in a custom controller, then let HttpKernel handle the request and check if its output is correctly escaped.

I'm hitting a problem though, for some bizarre reason the call to error_log() in ExceptionLoggingSubscriber::onError() is not caught by the custom error handler that is set in TestBase.

  public function onError(GetResponseForExceptionEvent $event) {
    // ...
    $is_critical = !$exception instanceof HttpExceptionInterface || $exception->getStatusCode() >= 500;
    if ($is_critical) {
      // ... at this point if I check set_error_handler() it is set correctly to \Drupal\system\Tests\Routing\ExceptionHandlingTest::errorHandler().
      // ... but for some bizarre reason the following line is not handled by it.
      error_log(sprintf('Uncaught PHP Exception %s: "%s" at %s line %s', get_class($exception), $exception->getMessage(), $exception->getFile(), $exception->getLine()));
    }
  }

This error ends up in the error.log file in the simpletest folder. It will be picked up here in TestBase::restoreEnvironment():

    // In case a fatal error occurred that was not in the test process read the
    // log to pick up any fatal errors.
    simpletest_log_read($this->testId, $this->databasePrefix, get_class($this));

At this point my best guess is to implement a tearDown() and remove that line from the error log. That will make the test green.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Active » Needs review
FileSize
3.44 KB

Here's my work in progress. This is failing atm.

Status: Needs review » Needs work

The last submitted patch, 7: 2371141-7-exception_backtrace_xss.patch, failed testing.

znerol’s picture

The exception is logged by the exception.logger (class: \Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber). It might be possible to disable the subscriber by removing it from the event dispatcher before throwing the exception.

$event_dispatcher = \Drupal::service('event_dispatcher');
$exception_logger = \Drupal::service('exception.logger');
$event_dispatcher->removeSubscriber($exception_logger);
pfrenssen’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests +DrupalCamp Ghent 2014
FileSize
4.08 KB
4.9 KB

Thanks a lot @znerol, your suggestion works perfectly!

I have updated the patch. If this gets committed, please provide credit to znerol for his invaluable help :)

The last submitted patch, 10: 2371141-10-exception_backtrace_xss-test_only.patch, failed testing.

Fabianx’s picture

Issue summary: View changes
dawehner’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
@@ -140,7 +140,7 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
         // Generate a backtrace containing only scalar argument values.
-        $message .= '<pre class="backtrace">' . Error::formatFlattenedBacktrace($backtrace) . '</pre>';
+        $message .= '<pre class="backtrace">' . SafeMarkup::escape(Error::formatFlattenedBacktrace($backtrace)) . '</pre>';
       }

I don't know but it seems helpful to document why we need an explicit escaping here.

znerol’s picture

The @return documentation of Error::formatFlattenedBacktrace() says:

A plain-text line-wrapped string ready to be put inside <pre>

I think it would be appropriate to change that to:

An unescaped plain-text line-wrapped string. The string needs to be run through SafeMarkup::escape when rendering it as HTML.

Alternatively an additional static helper method (e.g. formatFlattenedBacktraceHTML) could be added to the Error class (the output even may include the <pre> wrapper). This then could be savely reused by other modules. Another option would be to extract formatFlattenedBacktraceText from formatFlattenedBacktrace and add the escaping to the original method.

pfrenssen’s picture

Hmm that seems a bit much, it's evident that plain text strings should be escaped before displaying them in HTML. I like @dawehner's idea to just document it inline.

I was wondering, is it necessary to add the subscriber back after removing it? In case additional tests are added later, some potential test failures might not be detected if they are not logged.

znerol’s picture

Hmm that seems a bit much

We should at least adapt the @return documentation for Error::formatFlattenedBacktrace() because at the moment it suggests to simply wrap the result into a <pre>.

I was wondering, is it necessary to add the subscriber back after removing it?

No. This is for the current request only, it does not alter the container.

dawehner’s picture

Status: Needs review » Needs work

Let's improve the documentation done and get this one in.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
5.78 KB
1.58 KB

Updated documentation as suggested.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

anavarre’s picture

Issue tags: +Security
Wim Leers’s picture

Great work!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 55b8558 on 8.0.x
    Issue #2371141 by pfrenssen: Fixed XSS vulnerability when displaying...

Status: Fixed » Closed (fixed)

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