Bug 26708 – XSSAuditor false negatives

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Adam Barth]

Gareth Heyes has been look at our XSSAuditor and found a number of false negatives.  We should fix them:

HTML entities:

test.php?x=%3Ca%20href%3D%26%23x0000006a%26%23x61%26%23x76%26%23x61%26%23x73%26%23x63%26%23x72%26%23x69%26%23x70%26%23x74%26%23x3a%26%23x61%26%23x6c%26%23x65%26%23x72%26%23x74%26%23x28%26%23x31%26%23x29%3Etest%3Ca%3E

test.php?x=%3Cimg%20src=1%20onerror=%26%2397%26%23108%26%23101%26%23114%26%23116%26%2340%26%2349%26%2341%3E

Scheme relative paths:

test.php?x=%3Cscript%20src=//businessinfo.co.uk/labs/xss/xss.js%3E%3C/script%3E

iframe + JavaScript URL:

test.php?x=%3Ciframe%20src=javascript:alert(document.domain)%3E

HTTP-Equiv UTF-7:

test.php?x=%3Cmeta%20http-equiv=%22Content-Type%22%20content=%22text/html;%20charset=UTF-7%22%20/%3E%2bADwAcwBjAHIAaQBwAHQAPgBhAGwAZQByAHQAKAAxACkAPAAvAHMAYwByAGkAcAB0AD4-

[Daniel Bates]

Created attachment 31912 [details]
Initial patch for false negatives with tests

Resolves issues with HTML entities, scheme relative paths, and iFrame JavaScript URLs. Fix for the HTTP-Equiv UTF-7 forthcoming.

[Adam Barth]

In general, we shouldn't be duplicating code from other source files.  You should modify the other files to expose the functions you need in their header files.  For example, fixUpChar could be moved to HTMLTokenizer.h (and possibly renamed) and similarly hexDigitValue.

I also wish we didn't have to write our own XSSAuditor::decodeURLHTMLEntities.  Does this function not exist elsewhere?  If not, we should add it to the right file instead of adding it to the auditor directly.  Also for XSSAuditor::findInRequest, we should only search the parent frame if the current frame's URL is about:blank.  That should cover the <iframe src="javascript:..."> case.

Finally, we should do the XSS_AUDITOR_PAGE_HEADERS work in a separate bug / patch because it's not related to these false negatives.

[Daniel Bates]

I will clean up the patch.

I could not find an equivalent function to XSSAuditor::decodeURLHTMLEntities.

(In reply to comment #2)
> In general, we shouldn't be duplicating code from other source files.  You
> should modify the other files to expose the functions you need in their header
> files.  For example, fixUpChar could be moved to HTMLTokenizer.h (and possibly
> renamed) and similarly hexDigitValue.
> 
> I also wish we didn't have to write our own XSSAuditor::decodeURLHTMLEntities. 
> Does this function not exist elsewhere?  If not, we should add it to the right
> file instead of adding it to the auditor directly.  Also for
> XSSAuditor::findInRequest, we should only search the parent frame if the
> current frame's URL is about:blank.  That should cover the <iframe
> src="javascript:..."> case.
> 
> Finally, we should do the XSS_AUDITOR_PAGE_HEADERS work in a separate bug /
> patch because it's not related to these false negatives.
> 

[Daniel Bates]

Created attachment 31972 [details]
Updated patch with tests.

Cleaned up patch. Fixed HTTP-Equiv UTF-7.

I was not sure where to move the method XSSAuditor::decodeHTMLEntities (formally named decodeURLHTMLEntities). Any suggestions?

[Daniel Bates]

Created attachment 31973 [details]
Updated patch with tests.

Removed extraneous header: HTTPHeaderMap.h

[Adam Barth]

Comment on attachment 31973 [details]
Updated patch with tests.

This looks great except for the HTML entities part.  That code really shouldn't be in the XSSAuditor.  It should be shared with the parser.  Can you post a version of the patch without the HTML entities fix?  That way we can get the other issues squared away and focus on the right HTML entities patch.

[Daniel Bates]

Created attachment 31978 [details]
Patch without HTML entities support

Removed support for HTML entities.

[Adam Barth]

Comment on attachment 31978 [details]
Patch without HTML entities support

Thanks Dan.  This looks great.  There are some lines with trailing white space, but I'll fix them when I land the patch.

[Adam Barth]

Will land.

[Adam Barth]

I tweaked the patch slightly before landing.  There isn't any need to store m_parentFrame because you can always get it from m_frame->tree()->parent().

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/http/tests/security/xssAuditor/http-equiv-utf-7-encoded-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/http-equiv-utf-7-encoded.html
Adding         LayoutTests/http/tests/security/xssAuditor/iframe-javascript-url-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/iframe-javascript-url.html
Adding         LayoutTests/http/tests/security/xssAuditor/resources/echo-intertag-default-encode.pl
Adding         LayoutTests/http/tests/security/xssAuditor/script-tag-utf-7-encoded-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/script-tag-utf-7-encoded.html
Adding         LayoutTests/http/tests/security/xssAuditor/script-tag-with-source-relative-scheme-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/script-tag-with-source-relative-scheme.html
Sending        WebCore/ChangeLog
Sending        WebCore/html/HTMLTokenizer.cpp
Sending        WebCore/page/XSSAuditor.cpp
Sending        WebCore/page/XSSAuditor.h
Sending        WebCore/platform/network/ResourceResponseBase.cpp
Sending        WebCore/platform/network/ResourceResponseBase.h
Transmitting file data ................
Committed revision 45312.