IsolatedWorldCSP: Don't expose capability to specify CSP for isolated…

… worlds.

Don't expose the capability to specify an isolated world CSP to manifest
V3 extensions. Instead use a strict default CSP which disallows remotely
hosted code.

We might look at adding this back again if we see there's demand for
this and are confident at the robustness of the IsolatedWorldCSP
implementation.

BUG=896041

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

extensions/common/manifest_constants.cc

@@ -37,8 +37,6 @@ const char kContentScripts[] = "content_scripts";
 const char kContentSecurityPolicy[] = "content_security_policy";
 const char kContentSecurityPolicy_ExtensionPagesPath[] =
     "content_security_policy.extension_pages";
-const char kContentSecurityPolicy_IsolatedWorldPath[] =
-    "content_security_policy.isolated_world";
 const char kContentSecurityPolicy_SandboxedPagesPath[] =
     "content_security_policy.sandbox";
 const char kConvertedFromUserScript[] = "converted_from_user_script";

extensions/common/manifest_handlers/csp_info.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/no_destructor.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
@@ -32,8 +33,6 @@ const char kDefaultContentSecurityPolicy[] =
     "script-src 'self' blob: filesystem:; "
     "object-src 'self' blob: filesystem:;";
 
-const char kDefaultIsolatedWorldCSP_BypassMainWorld[] = "";
-
 // The default secure CSP to be used in order to prevent remote scripts.
 const char kDefaultSecureCSP[] = "script-src 'self'; object-src 'self';";
 
@@ -125,13 +124,25 @@ const std::string& CSPInfo::GetExtensionPagesCSP(const Extension* extension) {
 
 // static
 const std::string* CSPInfo::GetIsolatedWorldCSP(const Extension& extension) {
-  // TODO(crbug.com/1005978): This should be only called for extensions which
-  // can have isolated worlds. Figure out the case of TYPE_USER_SCRIPT and add
-  // DCHECK(csp_info).
-  CSPInfo* csp_info = static_cast<CSPInfo*>(
-      extension.GetManifestData(keys::kContentSecurityPolicy));
+  if (extension.manifest_version() >= 3) {
+    // The isolated world will use its own CSP which blocks remotely hosted
+    // code.
+    static const base::NoDestructor<std::string> default_isolated_world_csp(
+        kDefaultSecureCSP);
+    return default_isolated_world_csp.get();
+  }
+
+  Manifest::Type type = extension.GetType();
+  bool bypass_main_world_csp = type == Manifest::TYPE_PLATFORM_APP ||
+                               type == Manifest::TYPE_EXTENSION ||
+                               type == Manifest::TYPE_LEGACY_PACKAGED_APP;
+  if (!bypass_main_world_csp) {
+    // The isolated world will use the main world CSP.
+    return nullptr;
+  }
 
-  return csp_info ? &csp_info->isolated_world_csp : nullptr;
+  // The isolated world will bypass the main world CSP.
+  return &base::EmptyString();
 }
 
 // static
@@ -161,26 +172,24 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
                         : keys::kContentSecurityPolicy;
 
   // The "content_security_policy" manifest key can either be a string or a
-  // dictionary of the format
+  // dictionary of the format.
   // "content_security_policy" : {
   //     "extension_pages": "",
   //     "sandbox": "",
-  //     "isolated_world": ""
   //  }
   const base::Value* csp = GetManifestPath(extension, key);
-  const int kManifestVersion3 = 3;
 
-  // TODO(crbug.com/914224): Remove the channel check once support for isolated
-  // world CSP is implemenented.
+  // TODO(karandeepb): Remove the channel check since we don't plan to support
+  // the CSP dictionary for Manifest V2.
   bool csp_dictionary_supported =
       extension->GetType() == Manifest::TYPE_EXTENSION &&
-      (extension->manifest_version() >= kManifestVersion3 ||
+      (extension->manifest_version() >= 3 ||
        GetCurrentChannel() == version_info::Channel::UNKNOWN);
 
   if (csp_dictionary_supported) {
     // CSP key as dictionary is mandatory for manifest v3 (and above)
     // extensions.
-    if (extension->manifest_version() >= kManifestVersion3) {
+    if (extension->manifest_version() >= 3) {
       if (csp && !csp->is_dict()) {
         *error = GetInvalidManifestKeyError(key);
         return false;
@@ -203,7 +212,6 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
     return false;
   }
 
-  SetIsolatedWorldCSP(extension, kDefaultIsolatedWorldCSP_BypassMainWorld);
   return true;
 }
 
@@ -223,9 +231,8 @@ bool CSPHandler::ParseCSPDictionary(Extension* extension,
                  extension, keys::kContentSecurityPolicy_ExtensionPagesPath)) &&
          ParseSandboxCSP(
              extension, error, keys::kContentSecurityPolicy_SandboxedPagesPath,
-             GetManifestPath(
-                 extension, keys::kContentSecurityPolicy_SandboxedPagesPath)) &&
-         ParseIsolatedWorldCSP(extension, error);
+             GetManifestPath(extension,
+                             keys::kContentSecurityPolicy_SandboxedPagesPath));
 }
 
 bool CSPHandler::ParseExtensionPagesCSP(
@@ -273,38 +280,6 @@ bool CSPHandler::ParseExtensionPagesCSP(
   return true;
 }
 
-bool CSPHandler::ParseIsolatedWorldCSP(Extension* extension,
-                                       base::string16* error) {
-  const char* key = keys::kContentSecurityPolicy_IsolatedWorldPath;
-
-  const base::Value* isolated_world_csp = GetManifestPath(extension, key);
-
-  if (!isolated_world_csp) {
-    SetIsolatedWorldCSP(extension, kDefaultSecureCSP);
-    return true;
-  }
-
-  if (!isolated_world_csp->is_string()) {
-    *error = GetInvalidManifestKeyError(key);
-    return false;
-  }
-
-  const std::string& isolated_world_csp_str = isolated_world_csp->GetString();
-  if (!ContentSecurityPolicyIsLegal(isolated_world_csp_str)) {
-    *error = GetInvalidManifestKeyError(key);
-    return false;
-  }
-
-  if (!csp_validator::DoesCSPDisallowRemoteCode(
-          isolated_world_csp_str,
-          manifest_keys::kContentSecurityPolicy_IsolatedWorldPath, error)) {
-    return false;
-  }
-
-  SetIsolatedWorldCSP(extension, isolated_world_csp_str);
-  return true;
-}
-
 bool CSPHandler::ParseSandboxCSP(Extension* extension,
                                  base::string16* error,
                                  base::StringPiece manifest_key,
@@ -357,15 +332,6 @@ bool CSPHandler::SetExtensionPagesCSP(Extension* extension,
   return true;
 }
 
-void CSPHandler::SetIsolatedWorldCSP(Extension* extension,
-                                     std::string isolated_world_csp) {
-  // By now we must have parsed the extension page CSP.
-  CSPInfo* csp_info = static_cast<CSPInfo*>(
-      extension->GetManifestData(keys::kContentSecurityPolicy));
-  DCHECK(csp_info);
-  csp_info->isolated_world_csp = std::move(isolated_world_csp);
-}
-
 void CSPHandler::SetSandboxCSP(Extension* extension, std::string sandbox_csp) {
   CHECK(csp_validator::ContentSecurityPolicyIsSandboxed(sandbox_csp,
                                                         extension->GetType()));

extensions/common/manifest_handlers/csp_info.h

@@ -25,9 +25,6 @@ struct CSPInfo : public Extension::ManifestData {
   // cross-site scripting and other vulnerabilities.
   std::string extension_pages_csp;
 
-  // Content security policy to be used for extension isolated worlds.
-  std::string isolated_world_csp;
-
   // Content Security Policy that should be used to enforce the sandbox used
   // by sandboxed pages (guaranteed to have the "sandbox" directive without the
   // "allow-same-origin" token).
@@ -41,9 +38,7 @@ struct CSPInfo : public Extension::ManifestData {
   static const std::string& GetExtensionPagesCSP(const Extension* extension);
 
   // Returns the Content Security Policy to be used for extension isolated
-  // worlds or null if there is no defined CSP. Note that for extensions,
-  // platform apps and legacy packaged apps, a default CSP is used even if the
-  // manifest didn't specify one, so null shouldn't be returned for those cases.
+  // worlds or null if there is no defined CSP.
   static const std::string* GetIsolatedWorldCSP(const Extension& extension);
 
   // Returns the extension's Content Security Policy for the sandboxed pages.
@@ -79,10 +74,6 @@ class CSPHandler : public ManifestHandler {
                               bool secure_only,
                               const base::Value* content_security_policy);
 
-  // Parses the content security policy specified in the manifest for isolated
-  // worlds.
-  bool ParseIsolatedWorldCSP(Extension* extension, base::string16* error);
-
   // Parses the content security policy specified in the manifest for sandboxed
   // pages. This should be called after ParseExtensionPagesCSP.
   bool ParseSandboxCSP(Extension* extension,
@@ -96,10 +87,6 @@ class CSPHandler : public ManifestHandler {
                             bool secure_only,
                             std::string content_security_policy);
 
-  // Helper to set the isolated world content security policy manifest data.
-  void SetIsolatedWorldCSP(Extension* extension,
-                           std::string isolated_world_csp);
-
   // Helper to set the sandbox content security policy manifest data.
   void SetSandboxCSP(Extension* extension, std::string sandbox_csp);
 

extensions/common/manifest_handlers/csp_info_unittest.cc

@@ -28,7 +28,6 @@ const char kDefaultSandboxedPageCSP[] =
 const char kDefaultExtensionPagesCSP[] =
     "script-src 'self' blob: filesystem:; "
     "object-src 'self' blob: filesystem:;";
-const char kDefaultIsolatedWorldCSP_BypassMainWorld[] = "";
 const char kDefaultSecureCSP[] = "script-src 'self'; object-src 'self';";
 
 }  // namespace
@@ -112,6 +111,12 @@ TEST_F(CSPInfoUnitTest, CSPStringKey) {
   EXPECT_EQ("script-src 'self'; default-src 'none';",
             CSPInfo::GetExtensionPagesCSP(extension.get()));
 
+  // Manifest V2 extensions bypass the main world CSP in their isolated worlds.
+  const std::string* isolated_world_csp =
+      CSPInfo::GetIsolatedWorldCSP(*extension);
+  ASSERT_TRUE(isolated_world_csp);
+  EXPECT_TRUE(isolated_world_csp->empty());
+
   RunTestcase(Testcase("csp_invalid_1.json", GetInvalidManifestKeyError(
                                                  keys::kContentSecurityPolicy)),
               EXPECT_TYPE_ERROR);
@@ -219,49 +224,6 @@ TEST_F(CSPInfoUnitTest, CSPDictionary_Sandbox) {
   RunTestcases(testcases, base::size(testcases), EXPECT_TYPE_ERROR);
 }
 
-TEST_F(CSPInfoUnitTest, CSPDictionary_IsolatedWorlds) {
-  ScopedCurrentChannel channel(version_info::Channel::UNKNOWN);
-
-  struct {
-    const char* file_name;
-    const char* expected_csp;
-  } success_cases[] = {
-      {"isolated_world_csp_dictionary_default_v2.json", kDefaultSecureCSP},
-      {"isolated_world_csp_no_dictionary_default_v2.json",
-       kDefaultIsolatedWorldCSP_BypassMainWorld},
-      {"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;"}};
-
-  for (const auto& test_case : success_cases) {
-    SCOPED_TRACE(test_case.file_name);
-    scoped_refptr<Extension> extension =
-        LoadAndExpectSuccess(test_case.file_name);
-    ASSERT_TRUE(extension);
-
-    const std::string* csp = CSPInfo::GetIsolatedWorldCSP(*extension);
-    ASSERT_TRUE(csp);
-    EXPECT_EQ(test_case.expected_csp, *csp);
-  }
-
-  const char* key = keys::kContentSecurityPolicy_IsolatedWorldPath;
-  Testcase invalid_cases[] = {
-      {"isolated_world_csp_invalid_type.json", GetInvalidManifestKeyError(key)},
-      {"isolated_world_csp_missing_src.json",
-       ErrorUtils::FormatErrorMessage(
-           errors::kInvalidCSPMissingSecureSrc,
-           keys::kContentSecurityPolicy_IsolatedWorldPath, "script-src")},
-      {"isolated_world_csp_insecure_src.json",
-       ErrorUtils::FormatErrorMessage(
-           manifest_errors::kInvalidCSPInsecureValueError,
-           manifest_keys::kContentSecurityPolicy_IsolatedWorldPath,
-           "google.com", "object-src")},
-  };
-
-  RunTestcases(invalid_cases, base::size(invalid_cases), EXPECT_TYPE_ERROR);
-}
-
 // Ensures that using a dictionary for the keys::kContentSecurityPolicy manifest
 // key is mandatory for manifest v3 extensions and that defaults are applied
 // correctly.
@@ -281,6 +243,7 @@ TEST_F(CSPInfoUnitTest, CSPDictionaryMandatoryForV3) {
         CSPInfo::GetIsolatedWorldCSP(*extension);
     ASSERT_TRUE(isolated_world_csp);
     EXPECT_EQ(kDefaultSecureCSP, *isolated_world_csp);
+
     EXPECT_EQ(kDefaultSandboxedPageCSP,
               CSPInfo::GetSandboxContentSecurityPolicy(extension.get()));
     EXPECT_EQ(kDefaultSecureCSP,