Bug 53205

Created attachment 80272 [details] Patch

Comment on attachment 80272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80272&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:3102 > + 977E2E0D12F0FC9C00C13379 /* create-html-entity-table in Resources */ = {isa = PBXBuildFile; fileRef = 977E2E0A12F0FC9C00C13379 /* create-html-entity-table */; }; Did you intend for this change to be part of this patch? > Source/WebCore/WebCore.xcodeproj/project.pbxproj:9493 > + 977E2E0A12F0FC9C00C13379 /* create-html-entity-table */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = "create-html-entity-table"; path = "parser/create-html-entity-table"; sourceTree = "<group>"; }; Ditto. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:15930 > + 977E2E0A12F0FC9C00C13379 /* create-html-entity-table */, Ditto. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22579 > + 977E2E0D12F0FC9C00C13379 /* create-html-entity-table in Resources */, Ditto. > Source/WebCore/html/parser/XSSFilter.cpp:2 > + * Copyright (C) 2010 Adam Barth. All Rights Reserved. Nit: 2010 => 2011. > Source/WebCore/html/parser/XSSFilter.cpp:40 > +namespace { Out of curiosity, how did you come to the decision to use an anonymous namespace as opposed to declaring all of the functions in this namespace as static? > Source/WebCore/html/parser/XSSFilter.cpp:45 > + if (length > 7) Where does the 7 come from? Notice, "http://" is seven characters in length. Won't this lead to false positives? Consider a web page search.php with <script src="http://www.example.com/script.js"></script>. Suppose we request search.php?q=http://www.apple.com. Then we will clear the value of the src attribute of the <script> in decodeURL() since "http://" is a substring of "http://www.apple.com"; => we won't load/execute the external script? This is a false positive. Moreover, this changes seems like it would also cause false positives in other HTML tags, including <img> and <base>. > Source/WebCore/html/parser/XSSFilter.cpp:52 > + if (name.size() < 5) Where does the 5 come from? I take it that this was chosen with respect to the shortest onX attribute, oncut. Hence, I would suggest we have a comment that describes how we chose 5. > Source/WebCore/html/parser/XSSFilter.cpp:91 > + HTMLToken::AttributeList::const_iterator iter = token.attributes().begin() You're missing a semicolon at the end of this line. > Source/WebCore/html/parser/XSSFilter.cpp:104 > + // FIXME: We should grab one character before the name also. OK; If we consider at least one or more characters that precede the attribute that will reduce the number of false positives with respect to matching the substring "http://" (without quotes). > Source/WebCore/html/parser/XSSFilter.h:2 > + * Copyright (C) 2010 Adam Barth. All Rights Reserved. Nit: 2010 => 2011.

Comment on attachment 80272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80272&action=review >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:3102 >> + 977E2E0D12F0FC9C00C13379 /* create-html-entity-table in Resources */ = {isa = PBXBuildFile; fileRef = 977E2E0A12F0FC9C00C13379 /* create-html-entity-table */; }; > > Did you intend for this change to be part of this patch? Probably not. :) >> Source/WebCore/html/parser/XSSFilter.cpp:40 >> +namespace { > > Out of curiosity, how did you come to the decision to use an anonymous namespace as opposed to declaring all of the functions in this namespace as static? I always forget which one we're supposed to use. All I remember is that whichever one I pick, someone tells me I picked the wrong one. ): >> Source/WebCore/html/parser/XSSFilter.cpp:45 >> + int length = attribute.m_valueRange.m_end - attribute.m_valueRange.m_start; >> + if (length > 7) > > Where does the 7 come from? Notice, "http://" is seven characters in length. Won't this lead to false positives? > > Consider a web page search.php with <script src="http://www.example.com/script.js"></script>. Suppose we request search.php?q=http://www.apple.com. Then we will clear the value of the src attribute of the <script> in decodeURL() since "http://" is a substring of "http://www.apple.com"; => we won't load/execute the external script? This is a false positive. > > Moreover, this changes seems like it would also cause false positives in other HTML tags, including <img> and <base>. I just made up the number 7. I can remove it from this patch, if you like. It's certainly putting the cart in front of the horse at this point. w.r.t. your comments about false positives, my plan is to use more context than we've used previously. In your script example, the snippet we'll be looking for it ' src="http://' (or however much of the attribute value we grab). My hope is that will help a lot to reduce false positives. We can do this easily in this design because we're integrated with the parser and we can get the characters in the input for each token (and each part of the token). >> Source/WebCore/html/parser/XSSFilter.cpp:52 >> + if (name.size() < 5) > > Where does the 5 come from? I take it that this was chosen with respect to the shortest onX attribute, oncut. Hence, I would suggest we have a comment that describes how we chose 5. Yeah, 5 matches what IE does here too. Definitely worth explaining! >> Source/WebCore/html/parser/XSSFilter.cpp:91 >> + HTMLToken::AttributeList::const_iterator iter = token.attributes().begin() > > You're missing a semicolon at the end of this line. Wow, how does that even compile? >> Source/WebCore/html/parser/XSSFilter.cpp:104 >> + // FIXME: We should grab one character before the name also. > > OK; If we consider at least one or more characters that precede the attribute that will reduce the number of false positives with respect to matching the substring "http://" (without quotes). We can't take too many characters before the name because then we won't catch attribute splitting: <button id=$foo> where $foo=" onclick=alert(1)"

Created attachment 80364 [details] Patch

Comment on attachment 80364 [details] Patch Thanks for making the changes. This looks good to me. r=me

Comment on attachment 80364 [details] Patch Thanks Dan!

Comment on attachment 80364 [details] Patch Rejecting attachment 80364 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2 Last 500 characters of output: eeded at 15969 (offset 2 lines). Hunk #4 succeeded at 22454 with fuzz 1 (offset 4 lines). Hunk #5 succeeded at 25151 (offset 4 lines). patching file Source/WebCore/html/parser/HTMLDocumentParser.cpp patching file Source/WebCore/html/parser/HTMLDocumentParser.h patching file Source/WebCore/html/parser/XSSFilter.cpp patching file Source/WebCore/html/parser/XSSFilter.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Daniel Bates', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/7606385

Created attachment 80483 [details] Patch for landing

Comment on attachment 80483 [details] Patch for landing Rejecting attachment 80483 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2 Last 500 characters of output: 504. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/WebCore.vcproj/WebCore.vcproj.rej patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj patching file Source/WebCore/html/parser/HTMLDocumentParser.cpp patching file Source/WebCore/html/parser/HTMLDocumentParser.h patching file Source/WebCore/html/parser/XSSFilter.cpp patching file Source/WebCore/html/parser/XSSFilter.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/7651074

Committed r76980: <http://trac.webkit.org/changeset/76980>

http://trac.webkit.org/changeset/76980 might have broken Leopard Intel Release (Tests)