Bug 78836 - XSSAuditor bypass with <svg> tags and html-entities.
: XSSAuditor bypass with <svg> tags and html-entities.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-16 13:35 PST by Thomas Sepez
Modified: 2012-02-22 14:21 PST (History)
3 users (show)

See Also:


Attachments
First cut at Patch -- needs more analysis. (17.90 KB, patch)
2012-02-17 16:19 PST, Thomas Sepez
no flags Details | Formatted Diff | Diff
Patch for initial review (needs more testing before commit). (22.85 KB, patch)
2012-02-21 13:53 PST, Thomas Sepez
abarth: review+
Details | Formatted Diff | Diff
Patch + indent fix. (22.85 KB, patch)
2012-02-22 12:54 PST, Thomas Sepez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Sepez 2012-02-16 13:35:03 PST
Originally reported by masatokinugawa at http://code.google.com/p/chromium/issues/detail?id=114641

Reflected vectors are of the form:

?xss=%3Csvg%3E%3Cscript%3E//%26%23x0A%3Balert(1)%3C/script%3E
?xss=%3Csvg%3E%3Cscript%3E//%26%23x0D%3Balert(1)%3C/script%3E
?xss=%3Csvg%3E%3Cscript%3E//%26%23x2028%3Balert(1)%3C/script%3E
?xss=%3Csvg%3E%3Cscript%3E//%26%23x2029%3Balert(1)%3C/script%3E
?xss=%3Csvg%3E%3Cscript%3E/**%26%23x2F%3Balert(1)%3C/script%3E

SVG is XML and hence the entities get decoded during parsing.
Comment 1 Thomas Sepez 2012-02-16 13:51:12 PST
Probably the right way to fix this is for xssauditor to know when it is in an SVG block vs. an ordinary script block, and apply html entity decoding to match the HTML vs. XML expectations.  Trying to always html entity decode will open up vulns in the HTML case.
Comment 2 Thomas Sepez 2012-02-17 16:19:58 PST
Created attachment 127672 [details]
First cut at Patch -- needs more analysis.
Comment 3 Thomas Sepez 2012-02-21 13:53:38 PST
Created attachment 128037 [details]
Patch for initial review (needs more testing before commit).
Comment 4 Thomas Sepez 2012-02-21 14:03:23 PST
So the idea in patch #2 is that the state of initial token / after scrip tag goes away, and is replaced by a count of the depth of the script blocks (which is cheaper than trying to keep a stack of tags).
Comment 5 Adam Barth 2012-02-21 14:19:55 PST
Comment on attachment 128037 [details]
Patch for initial review (needs more testing before commit).

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

This looks fantastic.  Thanks.

> Source/WebCore/html/parser/XSSAuditor.cpp:586
> +          break;

bad indent
Comment 6 Thomas Sepez 2012-02-22 12:54:14 PST
Created attachment 128269 [details]
Patch + indent fix.

Let's try to land this to avoid merging around it.
Comment 7 WebKit Review Bot 2012-02-22 14:21:00 PST
Comment on attachment 128269 [details]
Patch + indent fix.

Clearing flags on attachment: 128269

Committed r108551: <http://trac.webkit.org/changeset/108551>
Comment 8 WebKit Review Bot 2012-02-22 14:21:05 PST
All reviewed patches have been landed.  Closing bug.