Report by fbingha, Feb 20 (3 days ago) at http://code.google.com/p/chromium/issues/detail?id=98787 Say you have an editor and in your page source you have a script reference, say <script type="text/javascript" src="https://www.vbulletin.com/forum/clientscript/vbulletin-core.js?v=4110c"></script> Now say, within the editor you type: src="https://www.vbulletin.com" Or even better, your editor has an image in it like <img src="https://www.vbulletin.com/me.png" /> Now you preview, and bang, javascript is dead because Chrome insists on ANY usage of src="yourdomain" appearing in the request means that an XSS has been triggered. Previously I would had to have this in my request to trigger the problem: src="https://www.vbulletin.com/forum/clientscript/vbulletin-core.js?v=4110c"> Now you've pared it down to just this src="https://www.vbulletin.com">
Created attachment 128551 [details] Patch + tests This is the most straightforward way of preventing these false positives. Please double-check that we don't care about these "protected" attributes without an injected tag.
Comment on attachment 128551 [details] Patch + tests View in context: https://bugs.webkit.org/attachment.cgi?id=128551&action=review > Source/WebCore/html/parser/XSSAuditor.cpp:338 > + if (isContainedInRequest("<script")) What about "<ScriPt" ? IMHO, it would be better if we grabbed the characters to match from the m_cachedSnippet.
IsContainedInRequest() should handle the case issues. Also there's an ASSERT(hasName(token, embedTag)). But we'll take it from token.name
Created attachment 128580 [details] Patch + some refactoring.
Comment on attachment 128580 [details] Patch + some refactoring. View in context: https://bugs.webkit.org/attachment.cgi?id=128580&action=review > Source/WebCore/html/parser/XSSAuditor.cpp:502 > + size_t start = decodedSnippet.find("<"); How can this fail to be the first character?
(In reply to comment #5) > (From update of attachment 128580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128580&action=review > > > Source/WebCore/html/parser/XSSAuditor.cpp:502 > > + size_t start = decodedSnippet.find("<"); > > How can this fail to be the first character? Wasn't sure, nor was I taking any chances...
Comment on attachment 128580 [details] Patch + some refactoring. View in context: https://bugs.webkit.org/attachment.cgi?id=128580&action=review This looks great. > Source/WebCore/html/parser/XSSAuditor.cpp:316 > + if (isContainedInRequest(m_cachedDecodedSnippet) && isContainedInRequest(decodedSnippetForJavascript(token))) { Javascript => JavaScript
Here's an interesting case: <script/src="http://attacker.com/foo.js"></script> I think we get that subtly wrong in this patch, actually. The attacker can inject: <script/src="http://attacker.com/foo and let the rest of the page terminate the request. We'll wait for an HTMLSpace to terminate the token name, which could include characters that aren't in the request.
Rather than searching for an HTMLSpace to terminate the token name, why don't we just grab a fixed number of characters equal to one plus the length of the token's name (to account for the "<")?
Created attachment 128592 [details] Patch w/ fixed char grab
Comment on attachment 128592 [details] Patch w/ fixed char grab Ok. We should also add a test for the <script/src thing do we don't forget it in the future.
Attachment 128592 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Source/WebCore/html/parser/XSSAuditor.cpp:495: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/html/parser/XSSAuditor.cpp:500: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 128837 [details] Patch for landing
Comment on attachment 128837 [details] Patch for landing Clearing flags on attachment: 128837 Committed r108881: <http://trac.webkit.org/changeset/108881>
All reviewed patches have been landed. Closing bug.