Changeset 166202 in webkit

Timestamp:

Mar 24, 2014 4:11:43 PM (10 years ago)

Author:

dbates@webkit.org

Message:

XSS Auditor doesn't block <script> injected before an existing <script>
​https://bugs.webkit.org/show_bug.cgi?id=130475

Merged from Blink (patch by Tom Sepez):
​https://src.chromium.org/viewvc/blink?view=rev&revision=169697

Source/WebCore:

Tests: http/tests/security/xssAuditor/script-tag-expression-follows.html

http/tests/security/xssAuditor/script-tag-near-start.html

• html/parser/XSSAuditor.cpp:

(WebCore::startsHTMLCommentAt):
(WebCore::startsSingleLineCommentAt):
(WebCore::startsMultiLineCommentAt):
(WebCore::startsOpeningScriptTagAt):
(WebCore::XSSAuditor::decodedSnippetForJavaScript):

LayoutTests:

• http/tests/security/xssAuditor/resources/echo-intertag.pl:
• http/tests/security/xssAuditor/script-tag-expression-follows-expected.txt: Added.
• http/tests/security/xssAuditor/script-tag-expression-follows.html: Added.
• http/tests/security/xssAuditor/script-tag-near-start-expected.txt: Added.
• http/tests/security/xssAuditor/script-tag-near-start.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/security/xssAuditor/resources/echo-intertag.pl (modified) (1 diff)
• LayoutTests/http/tests/security/xssAuditor/script-tag-expression-follows-expected.txt (added)
• LayoutTests/http/tests/security/xssAuditor/script-tag-expression-follows.html (added)
• LayoutTests/http/tests/security/xssAuditor/script-tag-near-start-expected.txt (added)
• LayoutTests/http/tests/security/xssAuditor/script-tag-near-start.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/parser/XSSAuditor.cpp (modified) (5 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2014-03-24  Daniel Bates  <dabates@apple.com>
+
+        XSS Auditor doesn't block <script> injected before an existing <script>
+        https://bugs.webkit.org/show_bug.cgi?id=130475
+
+        Merged from Blink (patch by Tom Sepez):
+        https://src.chromium.org/viewvc/blink?view=rev&revision=169697
+
+        * http/tests/security/xssAuditor/resources/echo-intertag.pl:
+        * http/tests/security/xssAuditor/script-tag-expression-follows-expected.txt: Added.
+        * http/tests/security/xssAuditor/script-tag-expression-follows.html: Added.
+        * http/tests/security/xssAuditor/script-tag-near-start-expected.txt: Added.
+        * http/tests/security/xssAuditor/script-tag-near-start.html: Added.
+
 2014-03-24  Brent Fulgham  <bfulgham@apple.com>
 

trunk/LayoutTests/http/tests/security/xssAuditor/resources/echo-intertag.pl

@@ -93 +93 @@
     print "<script>history.replaceState({}, '', '#must-not-appear');</script>\n";
 }
-print $cgi->param('q');
+print $cgi->param('q'); # XSS reflected here.
+if ($cgi->param('script-expression-follows')) {
+    print "\n <script>42;</script>\n";
+}
 if ($cgi->param('clutter')) {
     print $cgi->param('clutter');

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-03-24  Daniel Bates  <dabates@apple.com>
+
+        XSS Auditor doesn't block <script> injected before an existing <script>
+        https://bugs.webkit.org/show_bug.cgi?id=130475
+
+        Merged from Blink (patch by Tom Sepez):
+        https://src.chromium.org/viewvc/blink?view=rev&revision=169697
+
+        Tests: http/tests/security/xssAuditor/script-tag-expression-follows.html
+               http/tests/security/xssAuditor/script-tag-near-start.html
+
+        * html/parser/XSSAuditor.cpp:
+        (WebCore::startsHTMLCommentAt):
+        (WebCore::startsSingleLineCommentAt):
+        (WebCore::startsMultiLineCommentAt):
+        (WebCore::startsOpeningScriptTagAt):
+        (WebCore::XSSAuditor::decodedSnippetForJavaScript):
+
 2014-03-24  Brent Fulgham  <bfulgham@apple.com>
 

trunk/Source/WebCore/html/parser/XSSAuditor.cpp

@@ -42 +42 @@
 #include "TextResourceDecoder.h"
 #include "XLinkNames.h"
+#include <wtf/ASCIICType.h>
 #include <wtf/MainThread.h>
 
@@ -88 +89 @@
 static bool startsHTMLCommentAt(const String& string, size_t start)
 {
-    return (start + 3 < string.length() && string[start] == '<' && string[start+1] == '!' && string[start+2] == '-' && string[start+3] == '-');
+    return (start + 3 < string.length() && string[start] == '<' && string[start + 1] == '!' && string[start + 2] == '-' && string[start + 3] == '-');
 }
 
 static bool startsSingleLineCommentAt(const String& string, size_t start)
 {
-    return (start + 1 < string.length() && string[start] == '/' && string[start+1] == '/');
+    return (start + 1 < string.length() && string[start] == '/' && string[start + 1] == '/');
 }
 
 static bool startsMultiLineCommentAt(const String& string, size_t start)
 {
-    return (start + 1 < string.length() && string[start] == '/' && string[start+1] == '*');
+    return (start + 1 < string.length() && string[start] == '/' && string[start + 1] == '*');
+}
+
+static bool startsOpeningScriptTagAt(const String& string, size_t start)
+{
+    return start + 6 < string.length() && string[start] == '<'
+        && WTF::toASCIILowerUnchecked(string[start + 1]) == 's'
+        && WTF::toASCIILowerUnchecked(string[start + 2]) == 'c'
+        && WTF::toASCIILowerUnchecked(string[start + 3]) == 'r'
+        && WTF::toASCIILowerUnchecked(string[start + 4]) == 'i'
+        && WTF::toASCIILowerUnchecked(string[start + 5]) == 'p'
+        && WTF::toASCIILowerUnchecked(string[start + 6]) == 't';
 }
 
@@ -622 +634 @@
     size_t endPosition = string.length();
     size_t foundPosition = notFound;
+    size_t lastNonSpacePosition = notFound;
 
     // Skip over initial comments to find start of code.
@@ -650 +663 @@
     String result;
     while (startPosition < endPosition && !result.length()) {
-        // Stop at next comment (using the same rules as above for SVG/XML vs HTML), when we 
-        // encounter a comma, or when we  exceed the maximum length target. The comma rule
-        // covers a common parameter concatenation case performed by some webservers.
-        // After hitting the length target, we can only stop at a point where we know we are
-        // not in the middle of a %-escape sequence. For the sake of simplicity, approximate
-        // not stopping inside a (possibly multiply encoded) %-esacpe sequence by breaking on
-        // whitespace only. We should have enough text in these cases to avoid false positives.
+        // Stop at next comment (using the same rules as above for SVG/XML vs HTML), when we encounter a comma,
+        // when we hit an opening <script> tag, or when we exceed the maximum length target. The comma rule
+        // covers a common parameter concatenation case performed by some web servers.
+        lastNonSpacePosition = notFound;
         for (foundPosition = startPosition; foundPosition < endPosition; foundPosition++) {
             if (!request.shouldAllowCDATA) {
@@ -668 +678 @@
                 }
             }
-            if (string[foundPosition] == ',' || (foundPosition > startPosition + kMaximumFragmentLengthTarget && isHTMLSpace(string[foundPosition]))) {
+            if (string[foundPosition] == ',')
+                break;
+
+            if (lastNonSpacePosition != notFound && startsOpeningScriptTagAt(string, foundPosition)) {
+                foundPosition = lastNonSpacePosition;
                 break;
             }
+
+            if (foundPosition > startPosition + kMaximumFragmentLengthTarget) {
+                // After hitting the length target, we can only stop at a point where we know we are
+                // not in the middle of a %-escape sequence. For the sake of simplicity, approximate
+                // not stopping inside a (possibly multiply encoded) %-escape sequence by breaking on
+                // whitespace only. We should have enough text in these cases to avoid false positives.
+                if (isHTMLSpace(string[foundPosition]))
+                    break;
+            }
+
+            if (!isHTMLSpace(string[foundPosition]))
+                lastNonSpacePosition = foundPosition;
         }