Make XSSAuditor extract meaningful snippet from script blocks for comparison
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Sep 2011 02:28:31 +0000 (02:28 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Sep 2011 02:28:31 +0000 (02:28 +0000)
against the URL when checking for reflection.  Avoids getting caugh up in
trailing comments.
https://bugs.webkit.org/show_bug.cgi?id=68094

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

Source/WebCore:

Tests: http/tests/security/xssAuditor/script-tag-with-trailing-comment.html
       http/tests/security/xssAuditor/script-tag-with-trailing-comment2.html
       http/tests/security/xssAuditor/script-tag-with-trailing-comment3.html

* html/parser/XSSAuditor.cpp:
(WebCore::XSSAuditor::filterTokenAfterScriptStartTag):
(WebCore::XSSAuditor::extractCodeFragment):
* html/parser/XSSAuditor.h:

LayoutTests:

* http/tests/security/xssAuditor/resources/echo-intertag.pl:
* http/tests/security/xssAuditor/script-tag-with-trailing-comment-expected.txt: Added.
* http/tests/security/xssAuditor/script-tag-with-trailing-comment.html: Added.
* http/tests/security/xssAuditor/script-tag-with-trailing-comment2-expected.txt: Added.
* http/tests/security/xssAuditor/script-tag-with-trailing-comment2.html: Added.
* http/tests/security/xssAuditor/script-tag-with-trailing-comment3-expected.txt: Added.
* http/tests/security/xssAuditor/script-tag-with-trailing-comment3.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@95774 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/security/xssAuditor/resources/echo-intertag.pl
LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment.html [new file with mode: 0644]
LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment2-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment2.html [new file with mode: 0644]
LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment3-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment3.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/parser/XSSAuditor.cpp
Source/WebCore/html/parser/XSSAuditor.h

index 874a129..9c7b4ea 100644 (file)
@@ -1,3 +1,20 @@
+2011-09-22  Tom Sepez  <tsepez@chromium.org>
+
+        Make XSSAuditor extract meaningful snippet from script blocks for comparison
+        against the URL when checking for reflection.  Avoids getting caugh up in
+        trailing comments.
+        https://bugs.webkit.org/show_bug.cgi?id=68094
+
+        Reviewed by Adam Barth.
+
+        * http/tests/security/xssAuditor/resources/echo-intertag.pl:
+        * http/tests/security/xssAuditor/script-tag-with-trailing-comment-expected.txt: Added.
+        * http/tests/security/xssAuditor/script-tag-with-trailing-comment.html: Added.
+        * http/tests/security/xssAuditor/script-tag-with-trailing-comment2-expected.txt: Added.
+        * http/tests/security/xssAuditor/script-tag-with-trailing-comment2.html: Added.
+        * http/tests/security/xssAuditor/script-tag-with-trailing-comment3-expected.txt: Added.
+        * http/tests/security/xssAuditor/script-tag-with-trailing-comment3.html: Added.
+
 2011-09-22  Ben Wells  <benwells@chromium.org>
 
         Rebaseline for bug 65583 (path based border radius drawing on skia) part 5
index 9561bd6..ab5e802 100755 (executable)
@@ -35,6 +35,9 @@ print $cgi->param('q');
 if ($cgi->param('clutter')) {
     print $cgi->param('clutter');
 }
+if ($cgi->param('q2')) {
+    print $cgi->param('q2');
+}
 if ($cgi->param('notifyDone')) {
     print "<script>\n";
     print "if (window.layoutTestController)\n";
diff --git a/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment-expected.txt b/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment-expected.txt
new file mode 100644 (file)
index 0000000..513e2f8
--- /dev/null
@@ -0,0 +1,3 @@
+CONSOLE MESSAGE: line 1: Refused to execute a JavaScript script. Source code of script found within request.
+
+
diff --git a/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment.html b/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment.html
new file mode 100644 (file)
index 0000000..1dffe8d
--- /dev/null
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.layoutTestController) {
+  layoutTestController.dumpAsText();
+  layoutTestController.setXSSAuditorEnabled(true);
+}
+</script>
+</head>
+<body>
+<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?clutter=%20<i><b>&q=<script>/*&q2=*/alert(String.fromCharCode(0x58,0x53,0x53))</script>">
+</iframe>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment2-expected.txt b/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment2-expected.txt
new file mode 100644 (file)
index 0000000..8b06a47
--- /dev/null
@@ -0,0 +1,5 @@
+CONSOLE MESSAGE: line 1: Refused to execute a JavaScript script. Source code of script found within request.
+
+CONSOLE MESSAGE: line 1: Refused to execute a JavaScript script. Source code of script found within request.
+
diff --git a/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment2.html b/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment2.html
new file mode 100644 (file)
index 0000000..285de8d
--- /dev/null
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.layoutTestController) {
+  layoutTestController.dumpAsText();
+  layoutTestController.setXSSAuditorEnabled(true);
+}
+</script>
+</head>
+<body>
+<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?clutter=<i><b>&q=<script>//&q2=%0aalert(String.fromCharCode(0x58,0x53,0x53))</script>">
+</iframe>
+<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?clutter=<i><b>&q=<script>x=1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1*1//&q2=%0aalert(String.fromCharCode(0x58,0x53,0x53))</script>">
+</iframe>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment3-expected.txt b/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment3-expected.txt
new file mode 100644 (file)
index 0000000..513e2f8
--- /dev/null
@@ -0,0 +1,3 @@
+CONSOLE MESSAGE: line 1: Refused to execute a JavaScript script. Source code of script found within request.
+
+
diff --git a/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment3.html b/LayoutTests/http/tests/security/xssAuditor/script-tag-with-trailing-comment3.html
new file mode 100644 (file)
index 0000000..753e3ed
--- /dev/null
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.layoutTestController) {
+  layoutTestController.dumpAsText();
+  layoutTestController.setXSSAuditorEnabled(true);
+}
+</script>
+</head>
+<body>
+<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?clutter=%20<i><b>&q=<script>%20%0a<!--&q2=%0aalert(String.fromCharCode(0x58,0x53,0x53))//--></script>">
+</iframe>
+</body>
+</html>
index c1eb510..5e7287b 100644 (file)
@@ -1,3 +1,21 @@
+2011-09-22  Tom Sepez  <tsepez@chromium.org>
+
+        Make XSSAuditor extract meaningful snippet from script blocks for comparison
+        against the URL when checking for reflection.  Avoids getting caugh up in
+        trailing comments.
+        https://bugs.webkit.org/show_bug.cgi?id=68094
+
+        Reviewed by Adam Barth.
+
+        Tests: http/tests/security/xssAuditor/script-tag-with-trailing-comment.html
+               http/tests/security/xssAuditor/script-tag-with-trailing-comment2.html
+               http/tests/security/xssAuditor/script-tag-with-trailing-comment3.html
+
+        * html/parser/XSSAuditor.cpp:
+        (WebCore::XSSAuditor::filterTokenAfterScriptStartTag):
+        (WebCore::XSSAuditor::extractCodeFragment):
+        * html/parser/XSSAuditor.h:
+
 2011-09-22  Nate Chapin  <japhet@chromium.org>
 
         Remove didReceiveAuthenticationChallenge() from SubresourceLoaderClient.
index 4e56c4d..5575c1c 100644 (file)
@@ -80,6 +80,26 @@ static bool isHTMLQuote(UChar c)
     return (c == '"' || c == '\'');
 }
 
+static bool isHTMLNewline(UChar c)
+{
+    return (c == '\n' || c == '\r');
+}
+
+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] == '-');
+}    
+
+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 hasName(const HTMLToken& token, const QualifiedName& name)
 {
     return equalIgnoringNullity(token.name(), static_cast<const String&>(name.localName()));
@@ -305,15 +325,16 @@ bool XSSAuditor::filterTokenAfterScriptStartTag(HTMLToken& token)
         return false;
     }
 
-    int start = 0;
-    // FIXME: We probably want to grab only the first few characters of the
-    //        contents of the script element.
-    int end = token.endIndex() - token.startIndex();
-    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;
+    TextResourceDecoder* decoder = m_parser->document()->decoder();
+    if (isContainedInRequest(fullyDecodeString(m_cachedSnippet, decoder))) {
+        int start = 0;
+        int end = token.endIndex() - token.startIndex();
+        String snippet = snippetForJavaScript(snippetForRange(token, start, end));
+        if (isContainedInRequest(fullyDecodeString(snippet, decoder))) {
+            token.eraseCharacters();
+            token.appendToCharacter(' '); // Technically, character tokens can't be empty.
+            return true;
+        }
     }
     return false;
 }
@@ -537,4 +558,50 @@ bool XSSAuditor::isSameOriginResource(const String& url)
     return (m_parser->document()->url().host() == resourceURL.host() && resourceURL.query().isEmpty());
 }
 
+
+String XSSAuditor::snippetForJavaScript(const String& string)
+{
+    const size_t kMaximumFragmentLengthTarget = 100;
+
+    size_t startPosition = 0;
+    size_t endPosition = string.length();
+    size_t foundPosition = notFound;
+
+    // Skip over initial comments to find start of code.
+    while (startPosition < endPosition) {
+        while (startPosition < endPosition && isHTMLSpace(string[startPosition]))
+            startPosition++;
+        if (startsHTMLCommentAt(string, startPosition) || startsSingleLineCommentAt(string, startPosition)) {
+            while (startPosition < endPosition && !isHTMLNewline(string[startPosition]))
+                startPosition++;
+        } else if (startsMultiLineCommentAt(string, startPosition)) {
+            if ((foundPosition = string.find("*/", startPosition)) != notFound)
+                startPosition = foundPosition + 2;
+            else
+                startPosition = endPosition;
+        } else
+            break;
+    }
+
+    // Stop at next comment or when we exceed the maximum length target. 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. A simple way to do this is to break on whitespace only.                
+    for (foundPosition = startPosition; foundPosition < endPosition; foundPosition++) {
+        if (startsSingleLineCommentAt(string, foundPosition) || startsMultiLineCommentAt(string, foundPosition)) {
+            endPosition = foundPosition + 2;
+            break;
+        }
+        if (startsHTMLCommentAt(string, foundPosition)) {
+            endPosition = foundPosition + 4;
+            break;
+        }
+        if (foundPosition > startPosition + kMaximumFragmentLengthTarget && isHTMLSpace(string[foundPosition])) {
+            endPosition = foundPosition;
+            break;
+        }
+    }
+    
+    return string.substring(startPosition, endPosition - startPosition);
 }
+
+} // namespace WebCore
index 9e1a7c0..2583a19 100644 (file)
@@ -67,6 +67,7 @@ private:
     bool eraseAttributeIfInjected(HTMLToken&, const QualifiedName&, const String& replacementValue = String());
 
     String snippetForRange(const HTMLToken&, int start, int end);
+    String snippetForJavaScript(const String&);
     String decodedSnippetForAttribute(const HTMLToken&, const HTMLToken::Attribute&);
 
     bool isContainedInRequest(const String&);