Bug 30242 – [XSSAuditor] IFrame JavaScript URLs that are URL-encoded twice can by bypass the XSSAuditor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

JavaScript URLs are inconsistently encoded/decoded before they are passed to the XSSAuditor. Bug #29523 tried to address this by having the non-decoded JavaScript URL passed to the XSSAuditor, but upon further investigation this URL is still in an encoded form to bug #____.

For example:
<iframe src="javascript: %250Aalert(/XSS/)"></iframe>

Note, the presence and position of the space character is critical in this example because of the "fix" introduced as part of bug #29523. Also, when url decoded "%25" is the '%' character.

And consider this as part of a URL (*), http://good.webblaze.org/dbates/xsstest.php?q=<iframe src="javascript: %250Aalert(/XSS/)"></iframe>

The XSSAuditor::canEvaluateJavaScriptURL(const String& code) is passed code = "%20%0Aalert(/XSS/)" (**).
Looking at the source code part of the JavaScript URL portion of the page URL (*) as decoded by XSSAuditor::findInRequest, we have: "%0Aalert(/XSS/)".
(Actually, we also remove the character '0' as part of removing strings of the form "\0"  that can arise in PHP Magic Quoted code. So, the final decoded page URL used when comparing (**) is: "%Aalert(/XSS)")

A variant of this example can contain a single-line JavaScript comment:
<iframe src="javascript: //%250Aalert(/XSS/)"></iframe>
http://good.webblaze.org/dbates/xsstest.php?q=%3Ciframe%20src=%22javascript:%20//%250Aalert(/XSS/)%22%3E%3C/iframe%3E

[Daniel Bates]

#____ should be #30241
(In reply to comment #0)
> JavaScript URLs are inconsistently encoded/decoded before they are passed to
> the XSSAuditor. Bug #29523 tried to address this by having the non-decoded
> JavaScript URL passed to the XSSAuditor, but upon further investigation this
> URL is still in an encoded form to bug #____.
> 
> For example:
> <iframe src="javascript: %250Aalert(/XSS/)"></iframe>
> 
> Note, the presence and position of the space character is critical in this
> example because of the "fix" introduced as part of bug #29523. Also, when url
> decoded "%25" is the '%' character.
> 
> And consider this as part of a URL (*),
> http://good.webblaze.org/dbates/xsstest.php?q=<iframe src="javascript:
> %250Aalert(/XSS/)"></iframe>
> 
> The XSSAuditor::canEvaluateJavaScriptURL(const String& code) is passed code =
> "%20%0Aalert(/XSS/)" (**).
> Looking at the source code part of the JavaScript URL portion of the page URL
> (*) as decoded by XSSAuditor::findInRequest, we have: "%0Aalert(/XSS/)".
> (Actually, we also remove the character '0' as part of removing strings of the
> form "\0"  that can arise in PHP Magic Quoted code. So, the final decoded page
> URL used when comparing (**) is: "%Aalert(/XSS)")
> 
> A variant of this example can contain a single-line JavaScript comment:
> <iframe src="javascript: //%250Aalert(/XSS/)"></iframe>
> http://good.webblaze.org/dbates/xsstest.php?q=%3Ciframe%20src=%22javascript:%20//%250Aalert(/XSS/)%22%3E%3C/iframe%3E

[Daniel Bates]

Removing dependency on bug #30241 as it may not be obvious or possible come up with a inverse routine to KURL::parse. Moreover, we can normalize the outgoing HTTP parameters by URL-decoding them twice.

[Daniel Bates]

Created attachment 41007 [details]
Patch with test cases

[Adam Barth]

Comment on attachment 41007 [details]
Patch with test cases

You need to initialize m_decodeURLEscapeSequencesTwice in the constructor.

At some point we need to clean up all the boolean flags being passed around.

This coding step is getting nasty.  I wish there was a better way.

[Daniel Bates]

Created attachment 41015 [details]
Patch with test cases

Here is an updated patch.

I agree the code is getting a bit messy with the booleans. Do you want me to try to clean this up now? Otherwise, I would suggest we do a clean up patch after we get this one and bug #27895 resolved.

[Adam Barth]

Comment on attachment 41015 [details]
Patch with test cases

Thanks Dan.  I agree that we should do a cleanup patch after fixing the inline event handler bug.

[Daniel Bates]

I need to land this manually because of a conflict with the latest revision of FrameLoader.cpp

[Daniel Bates]

Committed r49434: <http://trac.webkit.org/changeset/49434>