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
.
Comment | File | Size | Author |
---|---|---|---|
#18 | interdiff.txt | 1.58 KB | pfrenssen |
#18 | 2371141-18-exception_backtrace_xss.patch | 5.78 KB | pfrenssen |
#2 | 2371141-example.txt | 599 bytes | pfrenssen |
#1 | 2371141-xss.png | 45.8 KB | pfrenssen |
Comments
Comment #1
pfrenssenComment #2
pfrenssenHere's the fix and an example that triggers an exception containing unsafe HTML in method arguments.
Comment #3
pfrenssenStarting on a test.
Comment #4
Fabianx CreditAttribution: Fabianx commentedSo where again was our issue to review all SafeMarkup::set() calls ...
Thanks so much pfrenssen for catching that.
Comment #5
pfrenssenComment #6
pfrenssenI'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.
This error ends up in the error.log file in the simpletest folder. It will be picked up here in TestBase::restoreEnvironment():
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.
Comment #7
pfrenssenHere's my work in progress. This is failing atm.
Comment #9
znerol CreditAttribution: znerol commentedThe 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.Comment #10
pfrenssenThanks 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 :)
Comment #12
Fabianx CreditAttribution: Fabianx commentedComment #13
dawehnerI don't know but it seems helpful to document why we need an explicit escaping here.
Comment #14
znerol CreditAttribution: znerol commentedThe
@return
documentation of Error::formatFlattenedBacktrace() says:I think it would be appropriate to change that to:
Alternatively an additional static helper method (e.g.
formatFlattenedBacktraceHTML
) could be added to theError
class (the output even may include the<pre>
wrapper). This then could be savely reused by other modules. Another option would be to extractformatFlattenedBacktraceText
fromformatFlattenedBacktrace
and add the escaping to the original method.Comment #15
pfrenssenHmm 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.
Comment #16
znerol CreditAttribution: znerol commentedWe should at least adapt the
@return
documentation forError::formatFlattenedBacktrace()
because at the moment it suggests to simply wrap the result into a<pre>
.No. This is for the current request only, it does not alter the container.
Comment #17
dawehnerLet's improve the documentation done and get this one in.
Comment #18
pfrenssenUpdated documentation as suggested.
Comment #19
dawehnerThank you
Comment #20
anavarreComment #21
Wim LeersGreat work!
Comment #22
catchCommitted/pushed to 8.0.x, thanks!