Title: [95366] trunk
Revision
95366
Author
commit-qu...@webkit.org
Date
2011-09-16 22:16:07 -0700 (Fri, 16 Sep 2011)

Log Message

Make XSSAuditor truncate inline snippets at a reasonable length before comparison
respecting boundaries of multiply urlencoded sequences.
https://bugs.webkit.org/show_bug.cgi?id=68092

Patch by Tom Sepez <tse...@chromium.org> on 2011-09-16
Reviewed by Adam Barth.

Source/WebCore:

Test: http/tests/security/xssAuditor/property-escape-long.html

* html/parser/XSSAuditor.cpp:
(WebCore::XSSAuditor::filterTokenAfterScriptStartTag):
(WebCore::XSSAuditor::eraseDangerousAttributesIfInjected):
(WebCore::XSSAuditor::eraseAttributeIfInjected):
(WebCore::XSSAuditor::decodedSnippetForAttribute):
(WebCore::XSSAuditor::isContainedInRequest):
* html/parser/XSSAuditor.h:

LayoutTests:

* http/tests/security/xssAuditor/property-escape-long-expected.txt: Added.
* http/tests/security/xssAuditor/property-escape-long.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (95365 => 95366)


--- trunk/LayoutTests/ChangeLog	2011-09-17 04:39:45 UTC (rev 95365)
+++ trunk/LayoutTests/ChangeLog	2011-09-17 05:16:07 UTC (rev 95366)
@@ -1,3 +1,14 @@
+2011-09-16  Tom Sepez  <tse...@chromium.org>
+
+        Make XSSAuditor truncate inline snippets at a reasonable length before comparison
+        respecting boundaries of multiply urlencoded sequences.
+        https://bugs.webkit.org/show_bug.cgi?id=68092
+        
+        Reviewed by Adam Barth.
+
+        * http/tests/security/xssAuditor/property-escape-long-expected.txt: Added.
+        * http/tests/security/xssAuditor/property-escape-long.html: Added.
+
 2011-09-16  Dmitry Lomov  <dslo...@google.com>
 
         https://bugs.webkit.org/show_bug.cgi?id=66714

Added: trunk/LayoutTests/http/tests/security/xssAuditor/property-escape-long-expected.txt (0 => 95366)


--- trunk/LayoutTests/http/tests/security/xssAuditor/property-escape-long-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/http/tests/security/xssAuditor/property-escape-long-expected.txt	2011-09-17 05:16:07 UTC (rev 95366)
@@ -0,0 +1,3 @@
+CONSOLE MESSAGE: line 1: Refused to execute a _javascript_ script. Source code of script found within request.
+
+

Added: trunk/LayoutTests/http/tests/security/xssAuditor/property-escape-long.html (0 => 95366)


--- trunk/LayoutTests/http/tests/security/xssAuditor/property-escape-long.html	                        (rev 0)
+++ trunk/LayoutTests/http/tests/security/xssAuditor/property-escape-long.html	2011-09-17 05:16:07 UTC (rev 95366)
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.layoutTestController) {
+  layoutTestController.dumpAsText();
+  layoutTestController.setXSSAuditorEnabled(true);
+}
+</script>
+</head>
+<body>
+<iframe src=""
+</iframe>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (95365 => 95366)


--- trunk/Source/WebCore/ChangeLog	2011-09-17 04:39:45 UTC (rev 95365)
+++ trunk/Source/WebCore/ChangeLog	2011-09-17 05:16:07 UTC (rev 95366)
@@ -1,3 +1,21 @@
+2011-09-16  Tom Sepez  <tse...@chromium.org>
+
+        Make XSSAuditor truncate inline snippets at a reasonable length before comparison
+        respecting boundaries of multiply urlencoded sequences.
+        https://bugs.webkit.org/show_bug.cgi?id=68092
+        
+        Reviewed by Adam Barth.
+
+        Test: http/tests/security/xssAuditor/property-escape-long.html
+
+        * html/parser/XSSAuditor.cpp:
+        (WebCore::XSSAuditor::filterTokenAfterScriptStartTag):
+        (WebCore::XSSAuditor::eraseDangerousAttributesIfInjected):
+        (WebCore::XSSAuditor::eraseAttributeIfInjected):
+        (WebCore::XSSAuditor::decodedSnippetForAttribute):
+        (WebCore::XSSAuditor::isContainedInRequest):
+        * html/parser/XSSAuditor.h:
+
 2011-09-16  Shawn Singh  <shawnsi...@chromium.org>
 
         Remove m_contentsDirty from LayerChromium because it

Modified: trunk/Source/WebCore/html/parser/XSSAuditor.cpp (95365 => 95366)


--- trunk/Source/WebCore/html/parser/XSSAuditor.cpp	2011-09-17 04:39:45 UTC (rev 95365)
+++ trunk/Source/WebCore/html/parser/XSSAuditor.cpp	2011-09-17 05:16:07 UTC (rev 95366)
@@ -309,7 +309,8 @@
     // FIXME: We probably want to grab only the first few characters of the
     //        contents of the script element.
     int end = token.endIndex() - token.startIndex();
-    if (isContainedInRequest(m_cachedSnippet + snippetForRange(token, start, end))) {
+    String snippet = m_cachedSnippet + snippetForRange(token, start, end);
+    if (isContainedInRequest(fullyDecodeString(snippet, m_parser->document()->decoder()))) {
         token.eraseCharacters();
         token.appendToCharacter(' '); // Technically, character tokens can't be empty.
         return true;
@@ -440,7 +441,29 @@
         bool valueContainsJavaScriptURL = isInlineEventHandler ? false : containsJavaScriptURL(attribute.m_value);
         if (!isInlineEventHandler && !valueContainsJavaScriptURL)
             continue;
-        if (!isContainedInRequest(snippetForAttribute(token, attribute)))
+        // Beware of trailing characters which came from the page itself, not the 
+        // injected vector. Excluding the terminating character covers common cases
+        // where the page immediately ends the attribute, but doesn't cover more
+        // complex cases where there is other page data following the injection. 
+        // Generally, these won't parse as _javascript_, so the injected vector
+        // typically excludes them from consideration via a single-line comment or
+        // by enclosing them in a string literal terminated later by the page's own
+        // closing punctuation. Since the snippet has not been parsed, the vector
+        // may also try to introduce these via entities. As a result, we'd like to
+        // stop before the first "//", the first entity, or the first quote not
+        // immediately following the first equals sign (taking whitespace into
+        // consideration). To keep things simpler, we don't try to distinguish
+        // between entity-introducing amperands vs. other uses, nor do we bother to
+        // check for a second slash for a comment, stoping instead on any ampersand
+        // or slash.
+        String decodedSnippet = decodedSnippetForAttribute(token, attribute);
+        size_t position;
+        if ((position = decodedSnippet.find("=")) != notFound
+            && (position = decodedSnippet.find(isNotHTMLSpace, position + 1)) != notFound
+            && (position = decodedSnippet.find(isTerminatingCharacter, isHTMLQuote(decodedSnippet[position]) ? position + 1 : position)) != notFound) {
+            decodedSnippet.truncate(position);
+        }
+        if (!isContainedInRequest(decodedSnippet))
             continue;
         token.eraseValueOfAttribute(i);
         if (valueContainsJavaScriptURL)
@@ -455,7 +478,7 @@
     size_t indexOfAttribute;
     if (findAttributeWithName(token, attributeName, indexOfAttribute)) {
         const HTMLToken::Attribute& attribute = token.attributes().at(indexOfAttribute);
-        if (isContainedInRequest(snippetForAttribute(token, attribute))) {
+        if (isContainedInRequest(decodedSnippetForAttribute(token, attribute))) {
             if (attributeName == srcAttr && isSameOriginResource(String(attribute.m_value.data(), attribute.m_value.size())))
                 return false;
             if (attributeName == http_equivAttr && !isDangerousHTTPEquiv(String(attribute.m_value.data(), attribute.m_value.size())))
@@ -476,46 +499,23 @@
     return m_parser->sourceForToken(token).substring(start, end - start);
 }
 
-String XSSAuditor::snippetForAttribute(const HTMLToken& token, const HTMLToken::Attribute& attribute)
+String XSSAuditor::decodedSnippetForAttribute(const HTMLToken& token, const HTMLToken::Attribute& attribute)
 {
+    const size_t kMaximumSnippetLength = 100;
+
     // The range doesn't inlcude the character which terminates the value. So,
     // for an input of |name="value"|, the snippet is |name="value|. For an
     // unquoted input of |name=value |, the snippet is |name=value|.
     // FIXME: We should grab one character before the name also.
     int start = attribute.m_nameRange.m_start - token.startIndex();
     int end = attribute.m_valueRange.m_end - token.startIndex();
-    String snippet = snippetForRange(token, start, end);
-
-    // Beware of trailing characters which came from the page itself, not the 
-    // injected vector. Excluding the terminating character covers common cases
-    // where the page immediately ends the attribute, but doesn't cover more
-    // complex cases where there is other page data following the injection. 
-    // Generally, these won't parse as _javascript_, so the injected vector
-    // typically excludes them from consideration via a single-line comment or
-    // by enclosing them in a string literal terminated later by the page's own
-    // closing punctuation. Since the snippet has not been parsed, the vector
-    // may also try to introduce these via entities. As a result, we'd like to
-    // stop before the first "//", the first entity, or the first quote not
-    // immediately following the first equals sign (taking whitespace into
-    // consideration). To keep things simpler, we don't try to distinguish
-    // between entity-introducing amperands vs. other uses, nor do we bother to
-    // check for a second slash for a comment, stoping instead on any ampersand
-    // or slash.
-    size_t position;
-    if ((position = snippet.find("=")) != notFound
-        && (position = snippet.find(isNotHTMLSpace, position + 1)) != notFound
-        && (position = snippet.find(isTerminatingCharacter, isHTMLQuote(snippet[position]) ? position + 1 : position)) != notFound) {
-        snippet.truncate(position);
-    }
-
-    return snippet;
+    String decodedSnippet = fullyDecodeString(snippetForRange(token, start, end), m_parser->document()->decoder());
+    decodedSnippet.truncate(kMaximumSnippetLength);
+    return decodedSnippet;
 }
 
-bool XSSAuditor::isContainedInRequest(const String& snippet)
+bool XSSAuditor::isContainedInRequest(const String& decodedSnippet)
 {
-    ASSERT(!snippet.isEmpty());
-    TextResourceDecoder* decoder = m_parser->document()->decoder();
-    String decodedSnippet = fullyDecodeString(snippet, decoder);
     if (decodedSnippet.isEmpty())
         return false;
     if (m_decodedURL.find(decodedSnippet, 0, false) != notFound)

Modified: trunk/Source/WebCore/html/parser/XSSAuditor.h (95365 => 95366)


--- trunk/Source/WebCore/html/parser/XSSAuditor.h	2011-09-17 04:39:45 UTC (rev 95365)
+++ trunk/Source/WebCore/html/parser/XSSAuditor.h	2011-09-17 05:16:07 UTC (rev 95366)
@@ -67,7 +67,7 @@
     bool eraseAttributeIfInjected(HTMLToken&, const QualifiedName&, const String& replacementValue = String());
 
     String snippetForRange(const HTMLToken&, int start, int end);
-    String snippetForAttribute(const HTMLToken&, const HTMLToken::Attribute&);
+    String decodedSnippetForAttribute(const HTMLToken&, const HTMLToken::Attribute&);
 
     bool isContainedInRequest(const String&);
     bool isSameOriginResource(const String& url);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to