CSP: Enable whitelisting of external JavaScript via hashes

third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp

Index: third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
index 86f819efed137e93a3c26eb22bd2d840b3417a89..d817ba0b6164329da9088603989affab4fb624aa 100644
--- a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
@@ -4,6 +4,7 @@
 
 #include "core/frame/csp/CSPDirectiveList.h"
 
+#include "core/frame/SubresourceIntegrity.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/frame/csp/SourceListDirective.h"
 #include "platform/loader/fetch/ResourceRequest.h"
@@ -165,7 +166,7 @@ TEST_F(CSPDirectiveListTest, AllowScriptFromSourceNoNonce) {
         createList(test.list, ContentSecurityPolicyHeaderTypeReport);
     EXPECT_EQ(test.expected,
               directiveList->allowScriptFromSource(
-                  scriptSrc, String(), ParserInserted,
+                  scriptSrc, String(), IntegrityMetadataSet(), ParserInserted,
                   ResourceRequest::RedirectStatus::NoRedirect,
                   SecurityViolationReportingPolicy::SuppressReporting));
 
@@ -174,7 +175,7 @@ TEST_F(CSPDirectiveListTest, AllowScriptFromSourceNoNonce) {
         createList(test.list, ContentSecurityPolicyHeaderTypeEnforce);
     EXPECT_EQ(test.expected,
               directiveList->allowScriptFromSource(
-                  scriptSrc, String(), ParserInserted,
+                  scriptSrc, String(), IntegrityMetadataSet(), ParserInserted,
                   ResourceRequest::RedirectStatus::NoRedirect,
                   SecurityViolationReportingPolicy::SuppressReporting));
   }
@@ -222,8 +223,8 @@ TEST_F(CSPDirectiveListTest, AllowFromSourceWithNonce) {
                    ContentSecurityPolicyHeaderTypeReport);
     EXPECT_EQ(test.expected,
               directiveList->allowScriptFromSource(
-                  resource, String(test.nonce), ParserInserted,
-                  ResourceRequest::RedirectStatus::NoRedirect,
+                  resource, String(test.nonce), IntegrityMetadataSet(),
+                  ParserInserted, ResourceRequest::RedirectStatus::NoRedirect,
                   SecurityViolationReportingPolicy::SuppressReporting));
 
     // Enforce 'script-src'
@@ -231,8 +232,8 @@ TEST_F(CSPDirectiveListTest, AllowFromSourceWithNonce) {
                                ContentSecurityPolicyHeaderTypeEnforce);
     EXPECT_EQ(test.expected,
               directiveList->allowScriptFromSource(
-                  resource, String(test.nonce), ParserInserted,
-                  ResourceRequest::RedirectStatus::NoRedirect,
+                  resource, String(test.nonce), IntegrityMetadataSet(),
+                  ParserInserted, ResourceRequest::RedirectStatus::NoRedirect,
                   SecurityViolationReportingPolicy::SuppressReporting));
 
     // Report-only 'style-src'
@@ -258,8 +259,8 @@ TEST_F(CSPDirectiveListTest, AllowFromSourceWithNonce) {
                                ContentSecurityPolicyHeaderTypeReport);
     EXPECT_EQ(test.expected,
               directiveList->allowScriptFromSource(
-                  resource, String(test.nonce), ParserInserted,
-                  ResourceRequest::RedirectStatus::NoRedirect,
+                  resource, String(test.nonce), IntegrityMetadataSet(),
+                  ParserInserted, ResourceRequest::RedirectStatus::NoRedirect,
                   SecurityViolationReportingPolicy::SuppressReporting));
     EXPECT_EQ(test.expected,
               directiveList->allowStyleFromSource(
@@ -272,8 +273,8 @@ TEST_F(CSPDirectiveListTest, AllowFromSourceWithNonce) {
                                ContentSecurityPolicyHeaderTypeEnforce);
     EXPECT_EQ(test.expected,
               directiveList->allowScriptFromSource(
-                  resource, String(test.nonce), ParserInserted,
-                  ResourceRequest::RedirectStatus::NoRedirect,
+                  resource, String(test.nonce), IntegrityMetadataSet(),
+                  ParserInserted, ResourceRequest::RedirectStatus::NoRedirect,
                   SecurityViolationReportingPolicy::SuppressReporting));
     EXPECT_EQ(test.expected,
               directiveList->allowStyleFromSource(
@@ -283,6 +284,99 @@ TEST_F(CSPDirectiveListTest, AllowFromSourceWithNonce) {
   }
 }
 
+TEST_F(CSPDirectiveListTest, AllowScriptFromSourceWithHash) {
+  struct TestCase {
+    const char* list;
+    const char* url;
+    const char* integrity;
+    bool expected;
+  } cases[] = {
+      // Doesn't affect lists without hashes.
+      {"https://example.com", "https://example.com/file", "sha256-yay", true},
+      {"https://example.com", "https://example.com/file", "sha256-boo", true},
+      {"https://example.com", "https://example.com/file", "", true},
+      {"https://example.com", "https://not.example.com/file", "sha256-yay",
+       false},
+      {"https://example.com", "https://not.example.com/file", "sha256-boo",
+       false},
+      {"https://example.com", "https://not.example.com/file", "", false},
+
+      // Doesn't affect URLs that match the whitelist.
+      {"https://example.com 'sha256-yay'", "https://example.com/file",
+       "sha256-yay", true},
+      {"https://example.com 'sha256-yay'", "https://example.com/file",
+       "sha256-boo", true},
+      {"https://example.com 'sha256-yay'", "https://example.com/file", "",
+       true},
+
+      // Does affect URLs that don't match the whitelist.
+      {"https://example.com 'sha256-yay'", "https://not.example.com/file",
+       "sha256-yay", true},
+      {"https://example.com 'sha256-yay'", "https://not.example.com/file",
+       "sha256-boo", false},
+      {"https://example.com 'sha256-yay'", "https://not.example.com/file", "",
+       false},
+
+      // Both algorithm and digest must match.
+      {"'sha256-yay'", "https://a.com/file", "sha384-yay", false},
+
+      // Sha-1 is not supported, but -384 and -512 are.
+      {"'sha1-yay'", "https://a.com/file", "sha1-yay", false},
+      {"'sha384-yay'", "https://a.com/file", "sha384-yay", true},
+      {"'sha512-yay'", "https://a.com/file", "sha512-yay", true},
+
+      // Unknown hash algorithms don't work.
+      {"'asdf256-yay'", "https://a.com/file", "asdf256-yay", false},
+
+      // Additional whitelisted hashes in the CSP don't interfere.
+      {"'sha256-yay' 'sha384-boo'", "https://a.com/file", "sha256-yay", true},
+      {"'sha256-yay' 'sha384-boo'", "https://a.com/file", "sha384-boo", true},
+
+      // All integrity hashes must appear in the CSP (and match).
+      {"'sha256-yay'", "https://a.com/file", "sha256-yay sha384-boo", false},
+      {"'sha384-boo'", "https://a.com/file", "sha256-yay sha384-boo", false},
+      {"'sha256-yay' 'sha384-boo'", "https://a.com/file",
+       "sha256-yay sha384-yay", false},
+      {"'sha256-yay' 'sha384-boo'", "https://a.com/file",
+       "sha256-boo sha384-boo", false},
+      {"'sha256-yay' 'sha384-boo'", "https://a.com/file",
+       "sha256-yay sha384-boo", true},
+
+      // At least one integrity hash must be present.
+      {"'sha256-yay'", "https://a.com/file", "", false},
+  };
+
+  for (const auto& test : cases) {
+    SCOPED_TRACE(testing::Message()
+                 << "List: `" << test.list << "`, URL: `" << test.url << "`");

[Mike West, 2017/04/04 13:59:46]

You'll probably want to add `test.integrity` in here as well.

[Marc Treib, 2017/04/04 14:17:21]

Good catch, done.

+    KURL resource = KURL(KURL(), test.url);
+
+    IntegrityMetadataSet integrityMetadata;
+    ASSERT_EQ(SubresourceIntegrity::IntegrityParseValidResult,
+              SubresourceIntegrity::parseIntegrityAttribute(test.integrity,
+                                                            integrityMetadata));
+
+    // Report-only 'script-src'
+    Member<CSPDirectiveList> directiveList =
+        createList(String("script-src ") + test.list,
+                   ContentSecurityPolicyHeaderTypeReport);
+    EXPECT_EQ(test.expected,

[Mike West, 2017/04/04 13:59:46]

Shouldn't this be `EXPECT_TRUE`, if we're only in reporting mode?

[Marc Treib, 2017/04/04 14:17:21]

Hm, good question. I just copied this from AllowFromSourceWithNonce above. In
fact, it looks like all the Allow* tests here treat enfore and report-only
exactly the same. I guess the report-only thing is just handled on another
level? Or something is seriously broken here...

[Marc Treib, 2017/04/04 14:29:49]

I'm confused now. There's ContentSecurityPolicyHeaderType (with values Report or
Enforce) which is a property of the CSPDirectiveList. This is what we're setting
here. But then there's also SecurityViolationReportingPolicy (with values
SuppressReporting or Report), which is a param to allowScriptFromSource, and
we're always passing SuppressReporting in the tests. That has the effect that
CSPDirectiveList::allowScriptFromSource calls CSPDirectiveList::checkSource
rather than CSPDirectiveList::checkSourceAndReportViolation, and checkSource
doesn't care about the HeaderType, only ..AndReportViolation does.

I'm not sure how all of that fits together to make sense. What are those two
enums?

+              directiveList->allowScriptFromSource(
+                  resource, String(), integrityMetadata, ParserInserted,
+                  ResourceRequest::RedirectStatus::NoRedirect,
+                  SecurityViolationReportingPolicy::SuppressReporting));
+
+    // Enforce 'script-src'
+    directiveList = createList(String("script-src ") + test.list,
+                               ContentSecurityPolicyHeaderTypeEnforce);
+    EXPECT_EQ(test.expected,
+              directiveList->allowScriptFromSource(
+                  resource, String(), integrityMetadata, ParserInserted,
+                  ResourceRequest::RedirectStatus::NoRedirect,
+                  SecurityViolationReportingPolicy::SuppressReporting));
+  }
+}
+
 TEST_F(CSPDirectiveListTest, allowRequestWithoutIntegrity) {
   struct TestCase {
     const char* list;