Bug 66585 – XSS filter bypass via document.write(location.href) and fragments

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

[Adam Barth]

http://code.google.com/p/chromium/issues/detail?id=76796

VULNERABILITY DETAILS
There is a way to bypass anti-XSS filter for DOM XSS exploiting a "window.location.href".

Considering a typical URL:

scheme://domain:port/path?query_string#fragment_id

Browsers encode correctly both "path" and "query_string", but not the "fragment_id". 
Explorer does not encode neither "query_string".
So if used "fragment_id" the vector is also not logged on Web Server.

VERSION
Chrome Version: 10.0.648.134 (Official Build 77917) beta

REPRODUCTION CASE
This is an xss_location.html page:

[[
<script type="text/javascript" language="javascript"> 
	document.write( window.location.href );
</script>
]]

The attack vector is:
xss_location.html?#<script>alert('XSS');</script>

* PoC:
For your convenience, a minimalist PoC is located on:
http://security.onofri.org/xss_location.html?#<script>alert('XSS');</script>

* References
 - DOM Based Cross Site Scripting or XSS of the Third Kind - http://www.webappsec.org/projects/articles/071105.shtml

[Daniel Bates]

I am unable to reproduce this issue using either Safari- or Chrome-  for Mac,  Version 5.1 (7534.48.3, r93670) and 14.0.835.109 beta, respectively.

Are there any additional details?

I did notice that Chrome doesn't visibly encode the fragment portion of the URL which is consistent with both the description of this bug as well as reiterated in <http://code.google.com/p/chromium/issues/detail?id=76796#c7>. When running the example in Chrome, we attempt to construct the HTML Script Element. The XSS Auditor detects this and scrubs the contents of the <script>.

In comparison, in Safari the fragment portion of the URL is encoded and hence we don't interpret as HTML markup.

For completeness, we have some test coverage for DOM-based XSS via window.location. For example, <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/xssAuditor/dom-write-location.html>.

[Adam Barth]

I also can't reproduce.  Maybe we should add a test a test and close this issue?  I tried this variation as well:

http://security.onofri.org/xss_location.html?#<script>alert('XS%41S');</script>

[Daniel Bates]

(In reply to comment #2)
> I also can't reproduce.  Maybe we should add a test a test and close this issue?

Actually, we already have a test case that covers this issue:
<http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/xssAuditor/anchor-url-dom-write-location.html>.

>  I tried this variation as well:
> 
> http://security.onofri.org/xss_location.html?#<script>alert('XS%41S');</script>

I'll add a test for this variation.

[Adam Barth]

Thanks Dan.

[Daniel Bates]

Created attachment 105073 [details]
LayoutTest

[WebKit Review Bot]

Comment on attachment 105073 [details]
LayoutTest

Clearing flags on attachment: 105073

Committed r93745: <http://trac.webkit.org/changeset/93745>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Actually, Tom managed to repro this issue.  Instant was causing me not to be able to repro it before.

[Daniel Bates]

(In reply to comment #8)
> Actually, Tom managed to repro this issue.  Instant was causing me not to be able to repro it before.

Can you elaborate?

[Daniel Bates]

(In reply to comment #9)
> (In reply to comment #8)
> > Actually, Tom managed to repro this issue.  Instant was causing me not to be able to repro it before.
> 
> Can you elaborate?

Never mind I was able to reproduce this issue with both Chrome for Mac 13.0.782.215 and 14.0.835.109 beta using:

http://security.onofri.org/xss_location.html?#<script>alert('XS%41S');</script>

I am unable to reproduce this issue with either shipping Mac Safari 5.1 or Mac Nightly r93707.

[Adam Barth]

> I am unable to reproduce this issue with either shipping Mac Safari 5.1 or Mac Nightly r93707.

That could be related to different URL handling in KURL and GURL.  Tom's thoughts are that we should try decoding these strings repeatedly until they stop changing, which should be robust to this sort of issue.

[Daniel Bates]

Created attachment 105098 [details]
DRT Test Case

I was able to reproduce the issue with Safari when I load a URL that has a similar form as the example in an <iframe>.

[Thomas Sepez]

Interestingly, the layout test that was introduced for this case is passing.  Here's what I'm observing:

The xss auditor bypass as reported requires that location.hash return a value of "#<script>alert('XS%41')</script>" to JS which is then passed to document.write().

Chrome (as distributed) and Safari (but only when launched from a fresh build via Tools/Script/run-safari") return this string.

Safari (as installed officially on my mac from cupertino) and (somehow) dumpRenderTree (as built) instead return "#%3cscript%3ealert('XS%41S')%3e/script%3c".

In order make this fire at all, the LayoutTest's .html file does document.write(unescape(...)).  The unescape introduces the needed <> punctuation, but also gets rid of the "%41", which must persist for this example to work.  Without a "%41" in the url, there isn't the conflict between xssauditor's decode() of URL and the page with the %41.

So ...  now I need to determine (if and) why say, alert(document.location) gives the different results for official safari vs. chrome vs. built safari vs. dumprendertree.

[Daniel Bates]

Additional remarks:

As I remarked in comment 1, Safari URL encodes the fragment portion of the URL for the main frame load. In particular, '<', and '>' and URL encoded. So, #<script>alert('XS%41S');</script> becomes #%3Cscript%3Ealert('XS%41S');%3C/script%3E.

From debugging the DRT test case using an Apple Mac WebKit build, we don't encode the fragment portion of the <iframe>'s src attribute using the same encoding algorithm as we do for a main frame load. In particular, when we construct a KURL object from the value of the <iframe>'s src attribute in SubframeLoader::requestFrame() (http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubframeLoader.cpp?rev=85395#L81), we ultimately call escapeAndAppendFragment() (See remark (*)) in KURL.cpp which only escapes ASCII characters whose value is < 32 (Space) and >= 127 (DEL): <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/KURL.cpp?rev=89336#L1017>.

For comparison, we use appendEscapingBadChars() in KURL.cpp to escape unsafe characters in the query string portion of the URL. And appendEscapingBadChars() escapes every isBadChar() character c \notin {'%', '?', '\t', '\n', '\r'}.

(*) Notice that HTMLFrameElementBase::openURL() calls SubframeLoader::requestFrame(); => SubframeLoader::completeURL() ;=> Document::completeURL(); => KURL(const KURL& base, const String& relative, const TextEncoding&) ;=> KURL::init(const KURL& base, const String& relative, const TextEncoding& encoding) ;=> KURL::parse(const char* url, const String* originalString); => escapeAndAppendFragment().

[Thomas Sepez]

Ok, if that's the case, then we should be able to build a test that illustrates the issue if the unescape() is removed and a %41 is present in the hash.

[Thomas Sepez]

Created attachment 105521 [details]
Revised test case showing the issue.

[Thomas Sepez]

Created attachment 105806 [details]
Patch plus test case changes from prev attachment.

Reduces both the url/post parameters and the body snippet to a minimal form before making a comparison for a reflected XSS.

[Adam Barth]

Comment on attachment 105806 [details]
Patch plus test case changes from prev attachment.

View in context: https://bugs.webkit.org/attachment.cgi?id=105806&action=review

This looks great.  Below are just some style nits.

> Source/WebCore/html/parser/XSSAuditor.cpp:119
> +static String fullyDecodeString(const String& string,
> +                                const TextResourceDecoder* decoder)

WebKit usually keeps function declarations to one line.

> Source/WebCore/html/parser/XSSAuditor.cpp:121
> +    size_t workingLen;

workingLen => workingLength

WebKit likes variable names make from complete words.

> Source/WebCore/html/parser/XSSAuditor.cpp:129
> +      CString workingStringUTF8 = workingString.utf8();
> +      String decodedString = decoder->encoding().decode(

4-space indent, pls.

> Source/WebCore/html/parser/XSSAuditor.cpp:130
> +          workingStringUTF8.data(), workingStringUTF8.length());

Also, this should be on one line.  There is no 80 column line limit in WebKit.

> Source/WebCore/html/parser/XSSAuditor.cpp:469
> +      return false;

4-space indent

[Thomas Sepez]

Created attachment 105811 [details]
Patch from prev attachment with corrected indent style.

Sorry about that.  Looks like my editor settings were still set to "2" on this box.
Other question:  is there a way to prove this is sufficiently performant?  Thanks.

[Adam Barth]

> Other question:  is there a way to prove this is sufficiently performant?  Thanks.

There are two approaches:

1) You can run the PLT.  You can either do this manually or watch http://build.chromium.org/f/chromium/perf/dashboard/overview.html once you patch rolls into Chromium.

2) We have some XSS auditor performance tests, but they're pretty minimal:

http://trac.webkit.org/browser/trunk/PerformanceTests/XSSAuditor

I don't think this patch will cause a performance problem, though.  The common case is that we'll decode the same number of times as before.

[Adam Barth]

Comment on attachment 105811 [details]
Patch from prev attachment with corrected indent style.

View in context: https://bugs.webkit.org/attachment.cgi?id=105811&action=review

> Source/WebCore/html/parser/XSSAuditor.cpp:125
> +    } while (workingString.length() < workingLength);

After this program point, workingLength no longer reflects the length of workingString.  That's fine, but slightly confusing.  Maybe add a comment?  (Or maybe it's not worth worrying about.)

[Thomas Sepez]

Created attachment 105814 [details]
Prev patch plus renamed length variable.

Sure, why not.

[WebKit Review Bot]

Comment on attachment 105814 [details]
Prev patch plus renamed length variable.

Clearing flags on attachment: 105814

Committed r94225: <http://trac.webkit.org/changeset/94225>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.