Bug 26807 – Crashes on 3 layout tests when XSS auditor is enabled

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

The following tests crash when the XSS auditor is enabled:

fast/loader/javascript-url-encoding-2.html
fast/loader/opaque-base-url.html
http/tests/security/postMessage/javascript-page-still-sends-origin.html

I'll attach the full backtrace.

[Gustavo Noronha (kov)]

Created attachment 32014 [details]
back trace

[Adam Barth]

From the test names, it looks like we might be handling the encoding of documents created by JavaScript URLs correctly.  Dan would you like to look into this, or would you like me to?

[Daniel Bates]

I'll take a look.

(In reply to comment #2)
> From the test names, it looks like we might be handling the encoding of
> documents created by JavaScript URLs correctly.  Dan would you like to look
> into this, or would you like me to?
> 

[Daniel Bates]

Created attachment 32023 [details]
temporary workaround

Temporary workaround.

I have not been able to reproduce the crash for the test: http/tests/security/postMessage/javascript-page-still-sends-origin.html

With respect to the other cases: fast/loader/javascript-url-encoding-2.html and fast/loader/opaque-base-url.html, these eventually cause the FrameLoader::m_decoder to be nullified and never set again before the XSSAuditor is called. So, frame->document()->decoder() is a null pointer and we were dereferencing it; hence the crashes.

Both of these tests call FrameLoader::executeIfJavaScriptURL, which calls FrameLoader::write(const String& str). Unlike in FrameLoader::write(const char* str, int len, bool flush), the field FrameLoader::m_decoder is never set in FrameLoader::write(const String& str). Moreover, it looks like FrameLoader::executeIfJavaScriptURL is only method that calls FrameLoader::write(const String& str).

I am unclear of the purpose of FrameLoader::write(const String& str). Why doesn't it just call FrameLoader::write(const char* str, int len, bool flush) with the appropriate arguments? I take it that it has something to do with parsing a strict document? I was not sure if I should set m_decoder (in a similar fashion) as in FrameLoader::write(const char* str, int len, bool flush), it seems kinda weird that someone left this out. Any suggestions?

[Adam Barth]

Comment on attachment 32023 [details]
temporary workaround

> +    if (!resultDecoded.isEmpty()) {
> +        if (!allowControlCharacters)
> +            resultDecoded.removeCharacters(&isControlCharacter);
> +        result = resultDecoded;
> +    }

This should just be an && instead of a nested if.

I think the patch is probably the right approach for the moment.  I'm surprised that JavaScript URLs don't inherit the charset of their creator.  We should file another bug to investigate this.  The easiest test is probably to have a UTF-7 encoded parent create an <iframe> with a javascript URL an some UTF-7 encoded tags and see what happens.

[Daniel Bates]

Created attachment 32138 [details]
Patch

Fixes the issue by checking whether frame->document()->decoder() is null.

[Adam Barth]

Comment on attachment 32138 [details]
Patch

Thanks!

[Adam Barth]

Sending        WebCore/ChangeLog
Sending        WebCore/page/XSSAuditor.cpp
Transmitting file data ..
Committed revision 45445.