Summary: | [chromium] XSS Auditor bypass via javascript url and control characters | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Thomas Sepez <tsepez> | ||||||
Component: | WebCore Misc. | Assignee: | Thomas Sepez <tsepez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | XSSAuditor | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Thomas Sepez
2012-02-21 15:28:49 PST
Created attachment 128061 [details]
Patch.
Comment on attachment 128061 [details]
Patch.
This isn't the right approach... There should be a function already that does this work called something like protocolIs or protocolIsJavaScript...
Take a look at allowSettingJavaScriptURL in JSHTMLFrameElementCustom.cpp, for example. allowSettingSrcToJavaScriptUrl allowSettingFrameSrcToJavascriptUrl() does the check: if (protocolIsJavaScript(stripLeadingAndTrailingHTMLSpaces(value))) { ... If I'm reading this right, stripLeadingAndTrailingHTMLSpaces() [HTMLParserIdioms.cpp] doesn't remove control-characters or internal HTML spaces. protocolIsJavaScript() [KURL.cpp] uses protocolIs() which asserts on c <= 0x20 in DEBUG builds but nothing otherwise. So it looks like this check is hosed in the same was as XSSauditor if the (specific to chromium, right?) code allows  and the like -- Unless there is a higher-level check. Flipping to component security while I cobble together an example see if there's actually a hole here. Ah. KURL vs. KURLgoogle again. KURLgoogle's protocolIs() will handle the ctrl characters, so no UXSS hole on setting frame.location. Good. So we can use protocolIsJavascript(), but test will need to be chromium-only. Created attachment 128349 [details]
Patch.
Patch using Adam's suggested function. Also, the test now uses an <a href=""> rather than an <iframe src=""> since iframe src currently isn't exploitable. It flunks an origin test on chromium -- which is why this needs to be href in an a tag. Still waiting full testing completion.
Comment on attachment 128349 [details] Patch. Clearing flags on attachment: 128349 Committed r108653: <http://trac.webkit.org/changeset/108653> All reviewed patches have been landed. Closing bug. |