Bug 78836

Summary: XSSAuditor bypass with <svg> tags and html-entities.
Product: WebKit Reporter: Thomas Sepez <tsepez>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
First cut at Patch -- needs more analysis.
none
Patch for initial review (needs more testing before commit).
abarth: review+
Patch + indent fix. none

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.