XSSAuditor bypass with script tag and expression following injection point

Source/core/html/parser/XSSAuditor.cpp

 /*
  * Copyright (C) 2011 Adam Barth. All Rights Reserved.
  * Copyright (C) 2011 Daniel Bates (dbates@intudata.com).
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 25 matching lines @@
 #include "core/html/HTMLParamElement.h"
 #include "core/html/parser/HTMLDocumentParser.h"
 #include "core/html/parser/HTMLParserIdioms.h"
 #include "core/html/parser/TextResourceDecoder.h"
 #include "core/html/parser/XSSAuditorDelegate.h"
 #include "core/loader/DocumentLoader.h"
 #include "core/frame/Settings.h"
 #include "platform/JSONValues.h"
 #include "platform/network/FormData.h"
 #include "platform/text/DecodeEscapeSequences.h"
+#include "wtf/ASCIICType.h"
 #include "wtf/MainThread.h"
 
 namespace {
 
 // SecurityOrigin::urlWithUniqueSecurityOrigin() can't be used cross-thread, or we'd use it instead.
 const char kURLWithUniqueOrigin[] = "data:,";
 
 } // namespace
 
 namespace WebCore {
@@ skipping 48 matching lines @@
 static bool startsSingleLineCommentAt(const String& string, size_t start)
 {
     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] == '*');
 }
 
+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'

[dbates, 2014/03/20 17:39:35]

Nit: There should be space characters on either side of the addition operator
per (2) of section Spacing in the Blink Code Style Guidelines,
<http://www.chromium.org/blink/coding-style#TOC-Spacing>.

Notice that this spacing issue appears twice in this line and four more times
below.

+        && WTF::toASCIILowerUnchecked(string[start+3]) == 'r' && WTF::toASCIILowerUnchecked(string[start+4]) == 'i'
+        && WTF::toASCIILowerUnchecked(string[start+5]) == 'p' && WTF::toASCIILowerUnchecked(string[start+6]) == 't';
+}
+
 // If other files need this, we should move this to core/html/parser/HTMLParserIdioms.h
 template<size_t inlineCapacity>
 bool threadSafeMatch(const Vector<UChar, inlineCapacity>& vector, const QualifiedName& qname)
 {
     return equalIgnoringNullity(vector, qname.localName().impl());
 }
 
 static bool hasName(const HTMLToken& token, const QualifiedName& name)
 {
     return threadSafeMatch(token.name(), name);
@@ skipping 518 matching lines @@
     }
     return canonicalize(decodedSnippet);
 }
 
 String XSSAuditor::decodedSnippetForJavaScript(const FilterTokenRequest& request)
 {
     String string = request.sourceTracker.sourceForToken(request.token);
     size_t startPosition = 0;
     size_t endPosition = string.length();
     size_t foundPosition = kNotFound;
+    size_t lastNonSpacePosition = kNotFound;
 
     // Skip over initial comments to find start of code.
     while (startPosition < endPosition) {
         while (startPosition < endPosition && isHTMLSpace<UChar>(string[startPosition]))
             startPosition++;
 
         // Under SVG/XML rules, only HTML comment syntax matters and the parser returns
         // these as a separate comment tokens. Having consumed whitespace, we need not look
         // further for these.
         if (request.shouldAllowCDATA)
             break;
 
         // Under HTML rules, both the HTML and JS comment synatx matters, and the HTML
         // comment ends at the end of the line, not with -->.
         if (startsHTMLCommentAt(string, startPosition) || startsSingleLineCommentAt(string, startPosition)) {
             while (startPosition < endPosition && !isJSNewline(string[startPosition]))
                 startPosition++;
         } else if (startsMultiLineCommentAt(string, startPosition)) {
             if (startPosition + 2 < endPosition && (foundPosition = string.find("*/", startPosition + 2)) != kNotFound)
                 startPosition = foundPosition + 2;
             else
                 startPosition = endPosition;
         } else
             break;
     }
 
     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
+        // 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

[dbates, 2014/03/20 17:39:35]

Nit: There are two space characters that precede the word "exceed". 

         // covers a common parameter concatenation case performed by some webservers.

[dbates, 2014/03/20 17:39:35]

Nit: Although this isn't part of the patch, "webservers" should be "web
servers".

-        // 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.
+        lastNonSpacePosition = kNotFound;
         for (foundPosition = startPosition; foundPosition < endPosition; foundPosition++) {
             if (!request.shouldAllowCDATA) {
                 if (startsSingleLineCommentAt(string, foundPosition) || startsMultiLineCommentAt(string, foundPosition)) {
                     foundPosition += 2;
                     break;
                 }
                 if (startsHTMLCommentAt(string, foundPosition)) {
                     foundPosition += 4;
                     break;
                 }
             }
-            if (string[foundPosition] == ',' || (foundPosition > startPosition + kMaximumFragmentLengthTarget && isHTMLSpace<UChar>(string[foundPosition]))) {
+            if (string[foundPosition] == ',')
+                break;
+
+            if (lastNonSpacePosition != kNotFound && startsOpeningScriptTagAt(string, foundPosition)) {
+                foundPosition = lastNonSpacePosition;

[dbates, 2014/03/20 17:39:35]

The coverage included in this patch is sufficient. For your consideration, I
suggest we added a test where q :='<script>"<script>"%3Balert(/XSS/)' (without
single quotes) to test how we handle matching the start of an opening script tag
near the beginning of the injection string.

                 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) %-esacpe sequence by breaking on

[dbates, 2014/03/20 17:39:35]

Nit: esacpe => escape

+                // whitespace only. We should have enough text in these cases to avoid false positives.
+                if (isHTMLSpace<UChar>(string[foundPosition]))
+                    break;
+            }
+
+            if (!isHTMLSpace<UChar>(string[foundPosition]))
+                lastNonSpacePosition = foundPosition;
         }
 
         result = canonicalize(fullyDecodeString(string.substring(startPosition, foundPosition - startPosition), m_encoding));
         startPosition = foundPosition + 1;
     }
     return result;
 }
 
 bool XSSAuditor::isContainedInRequest(const String& decodedSnippet)
 {
@@ skipping 29 matching lines @@
 
 bool XSSAuditor::isSafeToSendToAnotherThread() const
 {
     return m_documentURL.isSafeToSendToAnotherThread()
         && m_decodedURL.isSafeToSendToAnotherThread()
         && m_decodedHTTPBody.isSafeToSendToAnotherThread()
         && m_httpBodyAsString.isSafeToSendToAnotherThread();
 }
 
 } // namespace WebCore