Bug 66585

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>.

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>

(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.

Thanks Dan.

Created attachment 105073 [details] LayoutTest

Comment on attachment 105073 [details] LayoutTest Clearing flags on attachment: 105073 Committed r93745: <http://trac.webkit.org/changeset/93745>

All reviewed patches have been landed. Closing bug.

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

(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?

(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.

> 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.

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>.

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.

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().

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.

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

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.

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

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.

> 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.

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.)

Created attachment 105814 [details] Prev patch plus renamed length variable. Sure, why not.

Comment on attachment 105814 [details] Prev patch plus renamed length variable. Clearing flags on attachment: 105814 Committed r94225: <http://trac.webkit.org/changeset/94225>

All reviewed patches have been landed. Closing bug.