CSP: Ignore individual invalid source-expressions instead of discarding

the entire directive.

Bug: 1035949
Change-Id: Ic8c897a38abb9677b03179f8d6c8bf912eb858b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1986623Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Lucas Gadani <lfg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729765}

services/network/public/cpp/content_security_policy.cc

@@ -232,11 +232,6 @@ mojom::CSPSourceListPtr ParseFrameAncestorsSourceList(
   base::StringPiece value = base::TrimString(
       frame_ancestors_value, base::kWhitespaceASCII, base::TRIM_ALL);
 
-  std::vector<mojom::CSPSourcePtr> sources;
-
-  if (frame_ancestors_value.empty())
-    return {};
-
   auto directive = mojom::CSPSourceList::New();
 
   if (base::EqualsCaseInsensitiveASCII(value, "'none'"))
@@ -262,7 +257,9 @@ mojom::CSPSourceListPtr ParseFrameAncestorsSourceList(
     }
 
     // Parsing error.
-    return {};
+    // Ignore this source-expression.
+    // TODO(lfg): Emit a warning to the user when parsing an invalid
+    // expression.
   }
 
   return directive;
@@ -273,7 +270,7 @@ mojom::CSPSourceListPtr ParseFrameAncestorsSourceList(
 // TODO(lfg): The report-to should be treated as a single token according to the
 // spec, but this implementation accepts multiple endpoints
 // https://crbug.com/916265.
-bool ParseReportDirective(const GURL& request_url,
+void ParseReportDirective(const GURL& request_url,
                           base::StringPiece value,
                           bool using_reporting_api,
                           std::vector<std::string>* report_endpoints) {
@@ -293,16 +290,16 @@ bool ParseReportDirective(const GURL& request_url,
     } else {
       GURL url = request_url.Resolve(uri);
 
+      // TODO(lfg): Emit a warning when parsing an invalid reporting URL.
       if (!url.is_valid())
-        return false;
+        continue;
       report_endpoints->push_back(url.spec());
     }
   }
-  return true;
 }
 
 // Parses the frame-ancestor directive of a Content-Security-Policy header.
-bool ParseFrameAncestors(
+void ParseFrameAncestors(
     const mojom::ContentSecurityPolicyPtr& content_security_policy_ptr,
     base::StringPiece frame_ancestors_value) {
   // A frame-ancestors directive has already been parsed. Skip further
@@ -311,7 +308,7 @@ bool ParseFrameAncestors(
   if (FindDirective(mojom::CSPDirective::Name::FrameAncestors,
                     &(content_security_policy_ptr->directives))) {
     // TODO(arthursonzogni, lfg): Should a warning be fired to the user here?
-    return true;
+    return;
   }
 
   auto source_list = ParseFrameAncestorsSourceList(frame_ancestors_value);
@@ -319,16 +316,14 @@ bool ParseFrameAncestors(
   // TODO(lfg): Emit a warning to the user when parsing an invalid
   // expression.
   if (!source_list)
-    return false;
+    return;
 
   content_security_policy_ptr->directives.push_back(mojom::CSPDirective::New(
       mojom::CSPDirective::Name::FrameAncestors, std::move(source_list)));
-
-  return true;
 }
 
 // Parses the report-uri directive of a Content-Security-Policy header.
-bool ParseReportEndpoint(
+void ParseReportEndpoint(
     const mojom::ContentSecurityPolicyPtr& content_security_policy_ptr,
     const GURL& base_url,
     base::StringPiece header_value,
@@ -336,16 +331,10 @@ bool ParseReportEndpoint(
   // A report-uri directive has already been parsed. Skip further directives per
   // https://www.w3.org/TR/CSP3/#parse-serialized-policy.
   if (!content_security_policy_ptr->report_endpoints.empty())
-    return true;
+    return;
 
-  if (!ParseReportDirective(base_url, header_value, using_reporting_api,
-                            &(content_security_policy_ptr->report_endpoints))) {
-    // TODO(lfg): Emit a warning to the user when parsing an invalid
-    // expression.
-    return false;
-  }
-
-  return true;
+  ParseReportDirective(base_url, header_value, using_reporting_api,
+                       &(content_security_policy_ptr->report_endpoints));
 }
 
 }  // namespace
@@ -353,27 +342,24 @@ bool ParseReportEndpoint(
 ContentSecurityPolicy::ContentSecurityPolicy() = default;
 ContentSecurityPolicy::~ContentSecurityPolicy() = default;
 
-bool ContentSecurityPolicy::Parse(const GURL& base_url,
+void ContentSecurityPolicy::Parse(const GURL& base_url,
                                   const net::HttpResponseHeaders& headers) {
   size_t iter = 0;
   std::string header_value;
   while (headers.EnumerateHeader(&iter, "content-security-policy",
                                  &header_value)) {
-    if (!Parse(base_url, network::mojom::ContentSecurityPolicyType::kEnforce,
-               header_value))
-      return false;
+    Parse(base_url, network::mojom::ContentSecurityPolicyType::kEnforce,
+          header_value);
   }
   iter = 0;
   while (headers.EnumerateHeader(&iter, "content-security-policy-report-only",
                                  &header_value)) {
-    if (!Parse(base_url, network::mojom::ContentSecurityPolicyType::kReport,
-               header_value))
-      return false;
+    Parse(base_url, network::mojom::ContentSecurityPolicyType::kReport,
+          header_value);
   }
-  return true;
 }
 
-bool ContentSecurityPolicy::Parse(
+void ContentSecurityPolicy::Parse(
     const GURL& base_url,
     network::mojom::ContentSecurityPolicyType type,
     base::StringPiece header_value) {
@@ -388,13 +374,8 @@ bool ContentSecurityPolicy::Parse(
     content_security_policy_ptr->type = type;
 
     auto frame_ancestors = directives.find("frame-ancestors");
-    if (frame_ancestors != directives.end()) {
-      if (!ParseFrameAncestors(content_security_policy_ptr,
-                               frame_ancestors->second)) {
-        content_security_policy_ptr.reset();
-        return false;
-      }
-    }
+    if (frame_ancestors != directives.end())
+      ParseFrameAncestors(content_security_policy_ptr, frame_ancestors->second);
 
     auto report_endpoints = directives.find("report-to");
     if (report_endpoints != directives.end()) {
@@ -407,19 +388,14 @@ bool ContentSecurityPolicy::Parse(
     }
 
     if (report_endpoints != directives.end()) {
-      if (!ParseReportEndpoint(
-              content_security_policy_ptr, base_url, report_endpoints->second,
-              content_security_policy_ptr->use_reporting_api)) {
-        content_security_policy_ptr.reset();
-        return false;
-      }
+      ParseReportEndpoint(content_security_policy_ptr, base_url,
+                          report_endpoints->second,
+                          content_security_policy_ptr->use_reporting_api);
     }
 
     content_security_policies_.push_back(
         std::move(content_security_policy_ptr));
   }
-
-  return true;
 }
 
 }  // namespace network

services/network/public/cpp/content_security_policy.h

@@ -30,10 +30,10 @@ class COMPONENT_EXPORT(NETWORK_CPP) ContentSecurityPolicy {
   // requesting |request_url|. The |request_url| is used for violation
   // reporting, as specified in
   // https://w3c.github.io/webappsec-csp/#report-violation.
-  bool Parse(const GURL& request_url, const net::HttpResponseHeaders& headers);
+  void Parse(const GURL& request_url, const net::HttpResponseHeaders& headers);
 
   // Parses a Content-Security-Policy |header|.
-  bool Parse(const GURL& base_url,
+  void Parse(const GURL& base_url,
              network::mojom::ContentSecurityPolicyType type,
              base::StringPiece header);
 

services/network/public/cpp/content_security_policy_unittest.cc

@@ -28,26 +28,21 @@ struct ExpectedResult {
 
 struct TestData {
   std::string header;
-  bool is_valid;
   ExpectedResult expected_result;
 
-  TestData(const std::string& header, ExpectedResult expected_result)
-      : header(header), is_valid(true), expected_result(expected_result) {}
-  TestData(const std::string& header) : header(header), is_valid(false) {}
+  TestData(const std::string& header,
+           ExpectedResult expected_result = ExpectedResult())
+      : header(header), expected_result(expected_result) {}
 };
 
-static void TestCSPParser(const std::string& header,
-                          const ExpectedResult* expected_result) {
+static void TestFrameAncestorsCSPParser(const std::string& header,
+                                        const ExpectedResult* expected_result) {
   scoped_refptr<net::HttpResponseHeaders> headers(
       new net::HttpResponseHeaders("HTTP/1.1 200 OK"));
   headers->AddHeader("Content-Security-Policy: frame-ancestors " + header);
   ContentSecurityPolicy policy;
   policy.Parse(GURL("https://example.com/"), *headers);
 
-  if (!expected_result) {
-    EXPECT_EQ(0U, policy.content_security_policies().size());
-    return;
-  }
   auto& frame_ancestors =
       policy.content_security_policies()[0]->directives[0]->source_list;
   EXPECT_EQ(frame_ancestors->sources.size(),
@@ -142,8 +137,10 @@ TEST(ContentSecurityPolicy, ParseFrameAncestors) {
       {"'self'", {{}, true, false}},
       {"*", {{}, false, true}},
 
-      // Invalid 'none'
-      {"example.com 'none'"},
+      // Invalid 'none'. This is an invalid expression according to the CSP
+      // grammar, but it is accepted because the parser ignores individual
+      // invalid source-expressions.
+      {"example.com 'none'", {{{"", "example.com"}}}},
 
       // Other.
       {"*:*", {{{"", "", url::PORT_UNSPECIFIED, "", true, true}}}},
@@ -168,7 +165,7 @@ TEST(ContentSecurityPolicy, ParseFrameAncestors) {
   };
 
   for (auto& test : test_data)
-    TestCSPParser(test.header, test.is_valid ? &test.expected_result : nullptr);
+    TestFrameAncestorsCSPParser(test.header, &test.expected_result);
 }
 
 TEST(ContentSecurityPolicy, ParseMultipleDirectives) {

services/network/url_loader.cc

@@ -1010,10 +1010,9 @@ void URLLoader::OnResponseStarted(net::URLRequest* url_request, int net_error) {
           network::features::kOutOfBlinkFrameAncestors)) {
     // Parse the Content-Security-Policy headers.
     ContentSecurityPolicy policy;
-    if (url_request_->response_headers() &&
-        policy.Parse(url_request_->url(), *url_request_->response_headers())) {
-      response_->content_security_policy = policy.TakeContentSecurityPolicy();
-    }
+    if (url_request_->response_headers())
+      policy.Parse(url_request_->url(), *url_request_->response_headers());
+    response_->content_security_policy = policy.TakeContentSecurityPolicy();
   }
 
   if (base::FeatureList::IsEnabled(features::kCrossOriginIsolation)) {

third_party/blink/renderer/core/fetch/fetch_response_data.cc

@@ -335,26 +335,21 @@ mojom::blink::FetchAPIResponsePtr FetchResponseData::PopulateFetchAPIResponse(
     if (HeaderList()->Get("content-security-policy",
                           content_security_policy_header)) {
       network::ContentSecurityPolicy policy;
-      if (policy.Parse(request_url,
-                       network::mojom::ContentSecurityPolicyType::kEnforce,
-                       StringUTF8Adaptor(content_security_policy_header)
-                           .AsStringPiece())) {
-        response->content_security_policy =
-            ConvertToBlink(policy.TakeContentSecurityPolicy());
-      }
+      policy.Parse(
+          request_url, network::mojom::ContentSecurityPolicyType::kEnforce,
+          StringUTF8Adaptor(content_security_policy_header).AsStringPiece());
+      response->content_security_policy =
+          ConvertToBlink(policy.TakeContentSecurityPolicy());
     }
     if (HeaderList()->Get("content-security-policy-report-only",
                           content_security_policy_header)) {
       network::ContentSecurityPolicy policy;
-      if (policy.Parse(request_url,
-                       network::mojom::ContentSecurityPolicyType::kReport,
-                       StringUTF8Adaptor(content_security_policy_header)
-                           .AsStringPiece())) {
-        auto blink_policies =
-            ConvertToBlink(policy.TakeContentSecurityPolicy());
-        for (auto& policy : blink_policies)
-          response->content_security_policy.push_back(std::move(policy));
-      }
+      policy.Parse(
+          request_url, network::mojom::ContentSecurityPolicyType::kReport,
+          StringUTF8Adaptor(content_security_policy_header).AsStringPiece());
+      auto blink_policies = ConvertToBlink(policy.TakeContentSecurityPolicy());
+      for (auto& policy : blink_policies)
+        response->content_security_policy.push_back(std::move(policy));
     }
   }
   return response;