Extensions: Enforce secure extension page CSP for extensions using csp dictionary.

This CL enforces a secure extension page CSP for any extension using the
"content_security_policy" dictionary key and for all manifest V3 extensions. The
restrictions placed are same as those for secure isolated world CSPs. This
should facilitate preventing these extensions from loading remote scripts.

BUG=993530

Change-Id: I66191e7688a772eb0cf8321f09370d6dba4b0cb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1753470
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688230}

extensions/common/csp_validator.cc

@@ -663,8 +663,9 @@ bool ContentSecurityPolicyIsSandboxed(
   return seen_sandbox;
 }
 
-bool IsSecureIsolatedWorldCSP(const std::string& isolated_world_csp,
-                              base::string16* error) {
+bool DoesCSPDisallowRemoteCode(const std::string& content_security_policy,
+                               base::StringPiece manifest_key,
+                               base::string16* error) {
   DCHECK(error);
 
   struct DirectiveMapping {
@@ -687,7 +688,7 @@ bool IsSecureIsolatedWorldCSP(const std::string& isolated_world_csp,
   };
 
   // Populate |directive_mappings|.
-  CSPParser csp_parser(isolated_world_csp);
+  CSPParser csp_parser(content_security_policy);
   for (DirectiveMapping* mapping : directive_mappings) {
     // Find the first matching directive. As per
     // http://www.w3.org/TR/CSP/#parse-a-csp-policy, duplicate directive names
@@ -723,12 +724,11 @@ bool IsSecureIsolatedWorldCSP(const std::string& isolated_world_csp,
   // "default-src".
   fallback_if_necessary(&worker_src_mapping, script_src_mapping);
 
-  auto is_secure_directive = [](const DirectiveMapping& mapping,
-                                base::string16* error) {
+  auto is_secure_directive = [manifest_key](const DirectiveMapping& mapping,
+                                            base::string16* error) {
     if (!mapping.directive) {
       *error = ErrorUtils::FormatErrorMessageUTF16(
-          manifest_errors::kInvalidCSPMissingSecureSrc,
-          manifest_keys::kContentSecurityPolicy_IsolatedWorldPath,
+          manifest_errors::kInvalidCSPMissingSecureSrc, manifest_key,
           mapping.status.name());
       return false;
     }
@@ -746,8 +746,7 @@ bool IsSecureIsolatedWorldCSP(const std::string& isolated_world_csp,
       return true;
 
     *error = ErrorUtils::FormatErrorMessageUTF16(
-        manifest_errors::kInvalidCSPInsecureValueError,
-        manifest_keys::kContentSecurityPolicy_IsolatedWorldPath, *it,
+        manifest_errors::kInvalidCSPInsecureValueError, manifest_key, *it,
         mapping.status.name());
     return false;
   };

extensions/common/csp_validator.h

@@ -132,10 +132,11 @@ std::string GetEffectiveSandoxedPageCSP(const std::string& policy,
 bool ContentSecurityPolicyIsSandboxed(
     const std::string& policy, Manifest::Type type);
 
-// Returns whether the given |isolated_world_csp| is secure. If not, populates
-// |error|.
-bool IsSecureIsolatedWorldCSP(const std::string& isolated_world_csp,
-                              base::string16* error);
+// Returns whether the given |content_security_policy| prevents remote scripts.
+// If not, populates |error|.
+bool DoesCSPDisallowRemoteCode(const std::string& content_security_policy,
+                               base::StringPiece manifest_key,
+                               base::string16* error);
 
 }  // namespace csp_validator
 

extensions/common/csp_validator_unittest.cc

@@ -616,19 +616,17 @@ TEST(ExtensionCSPValidator, ParseCSP) {
   }
 }
 
-TEST(ExtensionCSPValidator, IsSecureIsolatedWorldCSP) {
-  auto insecure_value_error = [](const std::string& directive,
-                                 const std::string& value) {
+TEST(ExtensionCSPValidator, DoesCSPDisallowRemoteCode) {
+  const char* kManifestKey = "dummy_key";
+  auto insecure_value_error = [kManifestKey](const std::string& directive,
+                                             const std::string& value) {
     return ErrorUtils::FormatErrorMessage(
         extensions::manifest_errors::kInvalidCSPInsecureValueError,
-        extensions::manifest_keys::kContentSecurityPolicy_IsolatedWorldPath,
-        value, directive);
+        kManifestKey, value, directive);
   };
 
-  auto missing_secure_src_error = [](const std::string& directive) {
-    return MissingSecureSrcWarning(
-        extensions::manifest_keys::kContentSecurityPolicy_IsolatedWorldPath,
-        directive);
+  auto missing_secure_src_error = [kManifestKey](const std::string& directive) {
+    return MissingSecureSrcWarning(kManifestKey, directive);
   };
 
   struct {
@@ -657,8 +655,8 @@ TEST(ExtensionCSPValidator, IsSecureIsolatedWorldCSP) {
   for (const auto& test_case : test_cases) {
     SCOPED_TRACE(test_case.policy);
     base::string16 error;
-    bool result = extensions::csp_validator::IsSecureIsolatedWorldCSP(
-        test_case.policy, &error);
+    bool result = extensions::csp_validator::DoesCSPDisallowRemoteCode(
+        test_case.policy, kManifestKey, &error);
     EXPECT_EQ(test_case.expected_error.empty(), result);
     EXPECT_EQ(base::ASCIIToUTF16(test_case.expected_error), error);
   }

extensions/common/manifest_handlers/csp_info.cc

@@ -33,8 +33,9 @@ const char kDefaultContentSecurityPolicy[] =
     "object-src 'self' blob: filesystem:;";
 
 const char kDefaultIsolatedWorldCSP_BypassMainWorld[] = "";
-const char kDefaultIsolatedWorldCSP_Secure[] =
-    "script-src 'self'; object-src 'self'; worker-src 'self'";
+
+// The default secure CSP to be used in order to prevent remote scripts.
+const char kDefaultSecureCSP[] = "script-src 'self'; object-src 'self';";
 
 const char kDefaultSandboxedPageContentSecurityPolicy[] =
     "sandbox allow-scripts allow-forms allow-popups allow-modals; "
@@ -96,6 +97,17 @@ const base::Value* GetManifestPath(const Extension* extension,
   return extension->manifest()->Get(path, &value) ? value : nullptr;
 }
 
+const char* GetDefaultExtensionPagesCSP(Extension* extension,
+                                        bool secure_only) {
+  if (secure_only)
+    return kDefaultSecureCSP;
+
+  if (extension->GetType() == Manifest::TYPE_PLATFORM_APP)
+    return kDefaultPlatformAppContentSecurityPolicy;
+
+  return kDefaultContentSecurityPolicy;
+}
+
 }  // namespace
 
 CSPInfo::CSPInfo(std::string extension_pages_csp)
@@ -176,8 +188,10 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
       return ParseCSPDictionary(extension, error);
   }
 
-  if (!ParseExtensionPagesCSP(extension, error, key, csp))
+  if (!ParseExtensionPagesCSP(extension, error, key, false /* secure_only */,
+                              csp)) {
     return false;
+  }
 
   if (!ParseSandboxCSP(extension, error, keys::kSandboxedPagesCSP,
                        GetManifestPath(extension, keys::kSandboxedPagesCSP))) {
@@ -192,6 +206,7 @@ bool CSPHandler::ParseCSPDictionary(Extension* extension,
                                     base::string16* error) {
   if (!ParseExtensionPagesCSP(
           extension, error, keys::kContentSecurityPolicy_ExtensionPagesPath,
+          true /* secure_only */,
           GetManifestPath(extension,
                           keys::kContentSecurityPolicy_ExtensionPagesPath))) {
     return false;
@@ -215,9 +230,13 @@ bool CSPHandler::ParseExtensionPagesCSP(
     Extension* extension,
     base::string16* error,
     base::StringPiece manifest_key,
+    bool secure_only,
     const base::Value* content_security_policy) {
-  if (!content_security_policy)
-    return SetDefaultExtensionPagesCSP(extension, manifest_key);
+  if (!content_security_policy) {
+    return SetExtensionPagesCSP(
+        extension, manifest_key, secure_only,
+        GetDefaultExtensionPagesCSP(extension, secure_only));
+  }
 
   if (!content_security_policy->is_string()) {
     *error = GetInvalidManifestKeyError(manifest_key);
@@ -231,18 +250,24 @@ bool CSPHandler::ParseExtensionPagesCSP(
     return false;
   }
 
+  if (secure_only) {
+    if (!csp_validator::DoesCSPDisallowRemoteCode(content_security_policy_str,
+                                                  manifest_key, error)) {
+      return false;
+    }
+    SetExtensionPagesCSP(extension, manifest_key, secure_only,
+                         content_security_policy_str);
+    return true;
+  }
+
   std::vector<InstallWarning> warnings;
-  // TODO(crbug.com/914224): For manifest V3, instead of sanitizing the
-  // extension provided csp value and raising install warnings, see if we want
-  // to raise errors and prevent the extension from loading.
   std::string sanitized_content_security_policy = SanitizeContentSecurityPolicy(
       content_security_policy_str, manifest_key.as_string(),
       GetValidatorOptions(extension), &warnings);
   extension->AddInstallWarnings(std::move(warnings));
 
-  extension->SetManifestData(
-      keys::kContentSecurityPolicy,
-      std::make_unique<CSPInfo>(std::move(sanitized_content_security_policy)));
+  SetExtensionPagesCSP(extension, manifest_key, secure_only,
+                       std::move(sanitized_content_security_policy));
   return true;
 }
 
@@ -253,7 +278,7 @@ bool CSPHandler::ParseIsolatedWorldCSP(Extension* extension,
   const base::Value* isolated_world_csp = GetManifestPath(extension, key);
 
   if (!isolated_world_csp) {
-    SetIsolatedWorldCSP(extension, kDefaultIsolatedWorldCSP_Secure);
+    SetIsolatedWorldCSP(extension, kDefaultSecureCSP);
     return true;
   }
 
@@ -268,8 +293,11 @@ bool CSPHandler::ParseIsolatedWorldCSP(Extension* extension,
     return false;
   }
 
-  if (!csp_validator::IsSecureIsolatedWorldCSP(isolated_world_csp_str, error))
+  if (!csp_validator::DoesCSPDisallowRemoteCode(
+          isolated_world_csp_str,
+          manifest_keys::kContentSecurityPolicy_IsolatedWorldPath, error)) {
     return false;
+  }
 
   SetIsolatedWorldCSP(extension, isolated_world_csp_str);
   return true;
@@ -306,22 +334,24 @@ bool CSPHandler::ParseSandboxCSP(Extension* extension,
   return true;
 }
 
-bool CSPHandler::SetDefaultExtensionPagesCSP(Extension* extension,
-                                             base::StringPiece manifest_key) {
-  // TODO(abarth): Should we continue to let extensions override the
-  //               default Content-Security-Policy?
-  const char* content_security_policy =
-      extension->GetType() == Manifest::TYPE_PLATFORM_APP
-          ? kDefaultPlatformAppContentSecurityPolicy
-          : kDefaultContentSecurityPolicy;
-
-  DCHECK_EQ(content_security_policy,
-            SanitizeContentSecurityPolicy(
-                content_security_policy, manifest_key.as_string(),
-                GetValidatorOptions(extension), nullptr));
+bool CSPHandler::SetExtensionPagesCSP(Extension* extension,
+                                      base::StringPiece manifest_key,
+                                      bool secure_only,
+                                      std::string content_security_policy) {
+  if (secure_only) {
+    base::string16 error;
+    DCHECK(csp_validator::DoesCSPDisallowRemoteCode(content_security_policy,
+                                                    manifest_key, &error));
+  } else {
+    DCHECK_EQ(content_security_policy,
+              SanitizeContentSecurityPolicy(
+                  content_security_policy, manifest_key.as_string(),
+                  GetValidatorOptions(extension), nullptr));
+  }
+
   extension->SetManifestData(
       keys::kContentSecurityPolicy,
-      std::make_unique<CSPInfo>(content_security_policy));
+      std::make_unique<CSPInfo>(std::move(content_security_policy)));
   return true;
 }
 

extensions/common/manifest_handlers/csp_info.h

@@ -76,6 +76,7 @@ class CSPHandler : public ManifestHandler {
   bool ParseExtensionPagesCSP(Extension* extension,
                               base::string16* error,
                               base::StringPiece manifest_key,
+                              bool secure_only,
                               const base::Value* content_security_policy);
 
   // Parses the content security policy specified in the manifest for isolated
@@ -89,9 +90,11 @@ class CSPHandler : public ManifestHandler {
                        base::StringPiece manifest_key,
                        const base::Value* sandbox_csp);
 
-  // Sets the default CSP value for the extension.
-  bool SetDefaultExtensionPagesCSP(Extension* extension,
-                                   base::StringPiece manifest_key);
+  // Helper to set the extension pages content security policy manifest data.
+  bool SetExtensionPagesCSP(Extension* extension,
+                            base::StringPiece manifest_key,
+                            bool secure_only,
+                            std::string content_security_policy);
 
   // Helper to set the isolated world content security policy manifest data.
   void SetIsolatedWorldCSP(Extension* extension,

extensions/common/manifest_handlers/csp_info_unittest.cc

@@ -29,8 +29,7 @@ const char kDefaultExtensionPagesCSP[] =
     "script-src 'self' blob: filesystem:; "
     "object-src 'self' blob: filesystem:;";
 const char kDefaultIsolatedWorldCSP_BypassMainWorld[] = "";
-const char kDefaultIsolatedWorldCSP_Secure[] =
-    "script-src 'self'; object-src 'self'; worker-src 'self'";
+const char kDefaultSecureCSP[] = "script-src 'self'; object-src 'self';";
 
 }  // namespace
 
@@ -122,10 +121,10 @@ TEST_F(CSPInfoUnitTest, CSPDictionary_ExtensionPages) {
   struct {
     const char* file_name;
     const char* csp;
-  } cases[] = {
-      {"csp_dictionary_valid.json", "default-src 'none';"},
-      {"csp_empty_valid.json", "script-src 'self'; object-src 'self';"},
-      {"csp_empty_dictionary_valid.json", kDefaultExtensionPagesCSP}};
+  } cases[] = {{"csp_dictionary_valid_1.json", "default-src 'none'"},
+               {"csp_dictionary_valid_2.json",
+                "worker-src 'self'; script-src; default-src 'self'"},
+               {"csp_empty_dictionary_valid.json", kDefaultSecureCSP}};
 
   // Verify that keys::kContentSecurityPolicy key can be used as a dictionary on
   // trunk.
@@ -162,7 +161,18 @@ TEST_F(CSPInfoUnitTest, CSPDictionary_ExtensionPages) {
                      keys::kContentSecurityPolicy_ExtensionPagesPath)),
         Testcase("csp_invalid_3.json",
                  GetInvalidManifestKeyError(
-                     keys::kContentSecurityPolicy_ExtensionPagesPath))};
+                     keys::kContentSecurityPolicy_ExtensionPagesPath)),
+        Testcase(
+            "csp_missing_src.json",
+            ErrorUtils::FormatErrorMessage(
+                errors::kInvalidCSPMissingSecureSrc,
+                keys::kContentSecurityPolicy_ExtensionPagesPath, "script-src")),
+        Testcase("csp_insecure_src.json",
+                 ErrorUtils::FormatErrorMessage(
+                     errors::kInvalidCSPInsecureValueError,
+                     keys::kContentSecurityPolicy_ExtensionPagesPath,
+                     "'unsafe-eval'", "worker-src")),
+    };
     RunTestcases(testcases, base::size(testcases), EXPECT_TYPE_ERROR);
   }
 }
@@ -180,7 +190,7 @@ TEST_F(CSPInfoUnitTest, CSPDictionary_Sandbox) {
     const char* expected_csp;
   } success_cases[] = {
       {"sandbox_dictionary_1.json", "/test", kCustomSandboxedCSP},
-      {"sandbox_dictionary_1.json", "/index", kDefaultExtensionPagesCSP},
+      {"sandbox_dictionary_1.json", "/index", kDefaultSecureCSP},
       {"sandbox_dictionary_2.json", "/test", kDefaultSandboxedPageCSP},
       {"sandbox_dictionary_2.json", "/index", kCustomExtensionPagesCSP},
   };
@@ -216,12 +226,11 @@ TEST_F(CSPInfoUnitTest, CSPDictionary_IsolatedWorlds) {
     const char* file_name;
     const char* expected_csp;
   } success_cases[] = {
-      {"isolated_world_csp_dictionary_default_v2.json",
-       kDefaultIsolatedWorldCSP_Secure},
+      {"isolated_world_csp_dictionary_default_v2.json", kDefaultSecureCSP},
       {"isolated_world_csp_no_dictionary_default_v2.json",
        kDefaultIsolatedWorldCSP_BypassMainWorld},
-      {"csp_dictionary_empty_v3.json", kDefaultIsolatedWorldCSP_Secure},
-      {"csp_dictionary_missing_v3.json", kDefaultIsolatedWorldCSP_Secure},
+      {"csp_dictionary_empty_v3.json", kDefaultSecureCSP},
+      {"csp_dictionary_missing_v3.json", kDefaultSecureCSP},
       {"isolated_world_csp_valid.json",
        "script-src 'self'; object-src http://localhost:80;"}};
 
@@ -273,10 +282,10 @@ TEST_F(CSPInfoUnitTest, CSPDictionaryMandatoryForV3) {
     const std::string* isolated_world_csp =
         CSPInfo::GetIsolatedWorldCSP(*extension);
     ASSERT_TRUE(isolated_world_csp);
-    EXPECT_EQ(kDefaultIsolatedWorldCSP_Secure, *isolated_world_csp);
+    EXPECT_EQ(kDefaultSecureCSP, *isolated_world_csp);
     EXPECT_EQ(kDefaultSandboxedPageCSP,
               CSPInfo::GetSandboxContentSecurityPolicy(extension.get()));
-    EXPECT_EQ(kDefaultExtensionPagesCSP,
+    EXPECT_EQ(kDefaultSecureCSP,
               CSPInfo::GetExtensionPagesCSP(extension.get()));
   }
 }

extensions/common/manifest_test.cc

@@ -12,6 +12,7 @@
 #include "base/path_service.h"
 #include "base/strings/pattern.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "extensions/common/extension_l10n_util.h"
 #include "extensions/common/extension_paths.h"
@@ -269,6 +270,9 @@ void ManifestTest::RunTestcases(const Testcase* testcases,
 
 void ManifestTest::RunTestcase(const Testcase& testcase,
                                         ExpectType type) {
+  SCOPED_TRACE(base::StringPrintf("Testing file '%s'",
+                                  testcase.manifest_filename_.c_str()));
+
   switch (type) {
     case EXPECT_TYPE_ERROR:
       LoadAndExpectError(testcase.manifest_filename_.c_str(),

extensions/test/data/manifest_tests/csp_dictionary_valid_2.json

+{
+  "name": "test",
+  "version": "0.1",
+  "manifest_version": 2,
+  "content_security_policy": {
+    "extension_pages" : "worker-src 'self'; script-src; default-src 'self'"
+  }
+}

extensions/test/data/manifest_tests/csp_insecure_src.json

+{
+  "name": "test",
+  "version": "0.1",
+  "manifest_version": 2,
+  "content_security_policy": {
+    "extension_pages" : "default-src 'none'; worker-src 'unsafe-eval';"
+  }
+}

extensions/test/data/manifest_tests/sandbox_csp_with_dictionary.json

@@ -7,6 +7,6 @@
     "content_security_policy": "sandbox;"
   },
   "content_security_policy" : {
-    "extension_pages" : "script-src google.com;"
+    "extension_pages" : "default-src;"
   }
 }