Invalid Content Security Policy sources should generate console warnings.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Aug 2012 22:36:58 +0000 (22:36 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Aug 2012 22:36:58 +0000 (22:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=93599

Patch by Mike West <mkwst@chromium.org> on 2012-08-09
Reviewed by Adam Barth.

Source/WebCore:

Given a Content Security Policy directive, we're currently correctly
ignoring sources that we can't parse: "'slef'", "http:///", etc. have
no effect on the protected resource's active policy. We're not, however
telling the developer what we're doing, which can lead to confusion.
This patch adds a console warning whenever an invalid source expression
is encountered while parsing a directive in order to make WebKit's
behavior more transparent to the developer.

There should be no functional change as a result of this patch: the
policy should be parsed exactly as before, it should simply be more
verbose.

Test: http/tests/security/contentSecurityPolicy/source-list-parsing-07.html

* page/ContentSecurityPolicy.cpp:
(WebCore::CSPSourceList::parse):
    Two changes: First, if 'parseSource' returns false, then log a
    warning to the developer's console. Second, if both the source and
    host are empty, but parsing succeeded, then we know we're dealing
    with either a wildcard- or keyword-source that was properly handled
    inside 'parseSource', so jump to the next source.
(WebCore::CSPSourceList::parseSource):
    Wildcard- and keyword-sources now return 'true'. They parsed
    correctly, after all.
(WebCore::ContentSecurityPolicy::reportInvalidSourceExpression):
    Log a console warning if an invalid source expression is present in
    a CSP directive's value.
(WebCore):
* page/ContentSecurityPolicy.h:

LayoutTests:

* http/tests/security/contentSecurityPolicy/source-list-parsing-02-expected.txt:
* http/tests/security/contentSecurityPolicy/source-list-parsing-02.html:
    Move two invalid source tests out into 07.
* http/tests/security/contentSecurityPolicy/source-list-parsing-07-expected.txt: Added.
* http/tests/security/contentSecurityPolicy/source-list-parsing-07.html: Added.
    Many invalid source tests, all of which should generate warnings.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-02-expected.txt
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-02.html
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-07-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-07.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/ContentSecurityPolicy.cpp
Source/WebCore/page/ContentSecurityPolicy.h

index 9650ef2..12134fe 100644 (file)
@@ -1,3 +1,17 @@
+2012-08-09  Mike West  <mkwst@chromium.org>
+
+        Invalid Content Security Policy sources should generate console warnings.
+        https://bugs.webkit.org/show_bug.cgi?id=93599
+
+        Reviewed by Adam Barth.
+
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-02-expected.txt:
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-02.html:
+            Move two invalid source tests out into 07.
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-07-expected.txt: Added.
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-07.html: Added.
+            Many invalid source tests, all of which should generate warnings.
+
 2012-08-09  Csaba Osztrogon√°c  <ossy@webkit.org>
 
         [Qt] Unreviewed gardening, paint the bots green.
index db917d2..6a70d0d 100644 (file)
@@ -1,6 +1,4 @@
-CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src https://127.?.0.1:*".
-
-None of these scripts should execute even though there are parse errors in the policy.
+These scripts should execute even though there are parse errors in the policy.
 
 
 
@@ -18,13 +16,3 @@ PASS
 Frame: '<!--framePath //<!--frame2-->-->'
 --------
 PASS
-
---------
-Frame: '<!--framePath //<!--frame3-->-->'
---------
-PASS
-
---------
-Frame: '<!--framePath //<!--frame4-->-->'
---------
-PASS
index 9c74af2..4f4aff3 100644 (file)
@@ -4,8 +4,6 @@
 <script src='resources/multiple-iframe-test.js'></script>
 <script>
 var tests = [
-    ['no',  'script-src https://127.?.0.1:*', 'resources/script.js'],
-    ['yes', 'script-src https://127.0.0.1:\t*  ', 'resources/script.js'],
     ['yes', 'script-src\thttp://127.0.0.1:8000', 'resources/script.js'],
     ['yes', 'script-src http://127.0.0.1:8000   \t ', 'resources/script.js'],
     ['yes', 'script-src http://127.0.0.1:* ', 'resources/script.js']
@@ -14,5 +12,5 @@ var tests = [
 </head>
 <body onload="test()">
   <p>
-    None of these scripts should execute even though there are parse errors in the policy.
+    These scripts should execute even though there are parse errors in the policy.
   </p>
diff --git a/LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-07-expected.txt b/LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-07-expected.txt
new file mode 100644 (file)
index 0000000..c4c49cf
--- /dev/null
@@ -0,0 +1,65 @@
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: 'http:/'. It will be ignored.
+CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src http:/".
+
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: 'http:/127.0.0.1'. It will be ignored.
+CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src http:/127.0.0.1".
+
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: 'http:///127.0.0.1'. It will be ignored.
+CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src http:///127.0.0.1".
+
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: 'http://127.0.0.1:/'. It will be ignored.
+CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src http://127.0.0.1:/".
+
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: 'https://127.?.0.1:*'. It will be ignored.
+CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src https://127.?.0.1:*".
+
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: 'https://127.0.0.1:'. It will be ignored.
+CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src https://127.0.0.1:".
+
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: ''slef''. It will be ignored.
+CONSOLE MESSAGE: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src 'slef'".
+
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains an invalid source: 'https://127.0.0.1:'. It will be ignored.
+Invalid source expressions should log a console warning, and be ignored.
+
+
+
+--------
+Frame: '<!--framePath //<!--frame0-->-->'
+--------
+PASS
+
+--------
+Frame: '<!--framePath //<!--frame1-->-->'
+--------
+PASS
+
+--------
+Frame: '<!--framePath //<!--frame2-->-->'
+--------
+PASS
+
+--------
+Frame: '<!--framePath //<!--frame3-->-->'
+--------
+PASS
+
+--------
+Frame: '<!--framePath //<!--frame4-->-->'
+--------
+PASS
+
+--------
+Frame: '<!--framePath //<!--frame5-->-->'
+--------
+PASS
+
+--------
+Frame: '<!--framePath //<!--frame6-->-->'
+--------
+PASS
+
+--------
+Frame: '<!--framePath //<!--frame7-->-->'
+--------
+PASS
diff --git a/LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-07.html b/LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-07.html
new file mode 100644 (file)
index 0000000..23a53ad
--- /dev/null
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src='resources/multiple-iframe-test.js'></script>
+<script>
+var tests = [
+    ['no', 'script-src http:/', 'resources/script.js'],
+    ['no', 'script-src http:/127.0.0.1', 'resources/script.js'],
+    ['no', 'script-src http:///127.0.0.1', 'resources/script.js'],
+    ['no', 'script-src http://127.0.0.1:/', 'resources/script.js'],
+    ['no', 'script-src https://127.?.0.1:*', 'resources/script.js'],
+    ['no', 'script-src https://127.0.0.1:', 'resources/script.js'],
+    ['no', 'script-src \'slef\'', 'resources/script.js'],
+    ['yes','script-src https://127.0.0.1:\t*   ', 'resources/script.js'],
+];
+</script>
+</head>
+<body onload="test()">
+  <p>
+    Invalid source expressions should log a console warning, and be ignored.
+  </p>
index 5340d9d..2d1849d 100644 (file)
@@ -1,3 +1,40 @@
+2012-08-09  Mike West  <mkwst@chromium.org>
+
+        Invalid Content Security Policy sources should generate console warnings.
+        https://bugs.webkit.org/show_bug.cgi?id=93599
+
+        Reviewed by Adam Barth.
+
+        Given a Content Security Policy directive, we're currently correctly
+        ignoring sources that we can't parse: "'slef'", "http:///", etc. have
+        no effect on the protected resource's active policy. We're not, however
+        telling the developer what we're doing, which can lead to confusion.
+        This patch adds a console warning whenever an invalid source expression
+        is encountered while parsing a directive in order to make WebKit's
+        behavior more transparent to the developer.
+
+        There should be no functional change as a result of this patch: the
+        policy should be parsed exactly as before, it should simply be more
+        verbose.
+
+        Test: http/tests/security/contentSecurityPolicy/source-list-parsing-07.html
+
+        * page/ContentSecurityPolicy.cpp:
+        (WebCore::CSPSourceList::parse):
+            Two changes: First, if 'parseSource' returns false, then log a
+            warning to the developer's console. Second, if both the source and
+            host are empty, but parsing succeeded, then we know we're dealing
+            with either a wildcard- or keyword-source that was properly handled
+            inside 'parseSource', so jump to the next source.
+        (WebCore::CSPSourceList::parseSource):
+            Wildcard- and keyword-sources now return 'true'. They parsed
+            correctly, after all.
+        (WebCore::ContentSecurityPolicy::reportInvalidSourceExpression):
+            Log a console warning if an invalid source expression is present in
+            a CSP directive's value.
+        (WebCore):
+        * page/ContentSecurityPolicy.h:
+
 2012-08-06  Nat Duca  <nduca@chromium.org>
 
         [chromium] Expose CCGraphicsContext as WebCompositorOutputSurface
index 078a5f7..55faf87 100644 (file)
@@ -264,12 +264,18 @@ void CSPSourceList::parse(const UChar* begin, const UChar* end)
         bool portHasWildcard = false;
 
         if (parseSource(beginSource, position, scheme, host, port, path, hostHasWildcard, portHasWildcard)) {
+            // Wildcard hosts and keyword sources ('self', 'unsafe-inline',
+            // etc.) aren't stored in m_list, but as attributes on the source
+            // list itself.
+            if (scheme.isEmpty() && host.isEmpty())
+                continue;
             if (scheme.isEmpty())
                 scheme = m_policy->securityOrigin()->protocol();
             if (!path.isEmpty())
                 m_policy->reportIgnoredPathComponent(m_directiveName, String(beginSource, position - beginSource), path);
             m_list.append(CSPSource(scheme, host, port, hostHasWildcard, portHasWildcard));
-        }
+        } else
+            m_policy->reportInvalidSourceExpression(m_directiveName, String(beginSource, position - beginSource));
 
         ASSERT(position == end || isASCIISpace(*position));
      }
@@ -288,22 +294,22 @@ bool CSPSourceList::parseSource(const UChar* begin, const UChar* end,
 
     if (end - begin == 1 && *begin == '*') {
         addSourceStar();
-        return false;
+        return true;
     }
 
     if (equalIgnoringCase("'self'", begin, end - begin)) {
         addSourceSelf();
-        return false;
+        return true;
     }
 
     if (equalIgnoringCase("'unsafe-inline'", begin, end - begin)) {
         addSourceUnsafeInline();
-        return false;
+        return true;
     }
 
     if (equalIgnoringCase("'unsafe-eval'", begin, end - begin)) {
         addSourceUnsafeEval();
-        return false;
+        return true;
     }
 
     const UChar* position = begin;
@@ -1310,6 +1316,12 @@ void ContentSecurityPolicy::reportIgnoredPathComponent(const String& directiveNa
     logToConsole(message);
 }
 
+void ContentSecurityPolicy::reportInvalidSourceExpression(const String& directiveName, const String& source) const
+{
+    String message = makeString("The source list for Content Security Policy directive '", directiveName, "' contains an invalid source: '", source, "'. It will be ignored.");
+    logToConsole(message);
+}
+
 void ContentSecurityPolicy::logToConsole(const String& message, const String& contextURL, const WTF::OrdinalNumber& contextLine, PassRefPtr<ScriptCallStack> callStack) const
 {
     m_scriptExecutionContext->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, contextURL, contextLine.oneBasedInt(), callStack);
index a6b6027..51f3869 100644 (file)
@@ -97,8 +97,9 @@ public:
     void gatherReportURIs(DOMStringList&) const;
 
     void reportDuplicateDirective(const String&) const;
-    void reportInvalidNonce(const String&) const;
     void reportIgnoredPathComponent(const String& directiveName, const String& completeSource, const String& path) const;
+    void reportInvalidNonce(const String&) const;
+    void reportInvalidSourceExpression(const String& directiveName, const String& source) const;
     void reportUnrecognizedDirective(const String&) const;
     void reportViolation(const String& directiveText, const String& consoleMessage, const KURL& blockedURL, const Vector<KURL>& reportURIs, const String& header, const String& contextURL = String(), const WTF::OrdinalNumber& contextLine = WTF::OrdinalNumber::beforeFirst(), PassRefPtr<ScriptCallStack> = 0) const;