| Summary: | XSSAuditor bypass with remote script ending in ? character | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Thomas Sepez <tsepez> | ||||||||
| Component: | WebCore Misc. | Assignee: | Thomas Sepez <tsepez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, dbates, webkit.review.bot | ||||||||
| Priority: | P2 | Keywords: | XSSAuditor | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 66579 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Thomas Sepez
2011-10-17 12:22:56 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. Created attachment 111338 [details]
Patch plus test case.
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. 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. Created attachment 111345 [details]
Much easier to understand patch.
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 Created attachment 111358 [details]
Much easier to understand patch with better changelog format.
Comment on attachment 111358 [details]
Much easier to understand patch with better changelog format.
Thanks Tom for the updated patch.
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> All reviewed patches have been landed. Closing bug. |