Bug 30242 - [XSSAuditor] IFrame JavaScript URLs that are URL-encoded twice can by bypass the XSSAuditor
Summary: [XSSAuditor] IFrame JavaScript URLs that are URL-encoded twice can by bypass ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Daniel Bates
URL: http://good.webblaze.org/dbates/xsste...
Keywords: XSSAuditor
Depends on:
Blocks:
 
Reported: 2009-10-08 17:13 PDT by Daniel Bates
Modified: 2009-10-11 23:50 PDT (History)
2 users (show)

See Also:


Attachments
Patch with test cases (15.50 KB, patch)
2009-10-11 16:28 PDT, Daniel Bates
abarth: review-
Details | Formatted Diff | Diff
Patch with test cases (15.68 KB, patch)
2009-10-11 20:56 PDT, Daniel Bates
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-10-08 17:13:50 PDT
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
Comment 1 Daniel Bates 2009-10-08 17:24:31 PDT
#____ 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
Comment 2 Daniel Bates 2009-10-11 16:25:22 PDT
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.
Comment 3 Daniel Bates 2009-10-11 16:28:39 PDT
Created attachment 41007 [details]
Patch with test cases
Comment 4 Adam Barth 2009-10-11 20:21:20 PDT
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.
Comment 5 Daniel Bates 2009-10-11 20:56:29 PDT
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.
Comment 6 Adam Barth 2009-10-11 21:12:22 PDT
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.
Comment 7 Daniel Bates 2009-10-11 23:12:18 PDT
I need to land this manually because of a conflict with the latest revision of FrameLoader.cpp
Comment 8 Daniel Bates 2009-10-11 23:50:33 PDT
Committed r49434: <http://trac.webkit.org/changeset/49434>