Ignore insecure parts of CSP in extensions and allow extension to load

Previously, insecure CSP directive values caused refusal of Chrome to
load the Chrome extension. Now, insecure values are stripped from the
CSP, and a list of detailed warnings is printed at the extensions page.

Renamed ContentSecurityPolicyIsSecure to SanitizeContentSecurityPolicy
and let it return a string (the sanitized CSP) instead of a boolean
that tells whether the CSP was considered secure.

BUG=434773
R=kalman@chromium.org
R=mkwst@chromium.org
TEST=extensions_unittests=ExtensionCSPValidator.*
     unit_tests=ContentSecurityPolicyManifestTest.*:PlatformAppsManifestTest:PlatformAppContentSecurityPolicy

Review URL: https://codereview.chromium.org/747403002

Cr-Commit-Position: refs/heads/master@{#310191}

chrome/common/extensions/manifest_tests/extension_manifests_contentsecuritypolicy_unittest.cc

@@ -3,22 +3,27 @@
 // found in the LICENSE file.
 
 #include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h"
+#include "extensions/common/error_utils.h"
 #include "extensions/common/manifest_constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace errors = extensions::manifest_errors;
+using extensions::ErrorUtils;
 
 class ContentSecurityPolicyManifestTest : public ChromeManifestTest {
 };
 
 TEST_F(ContentSecurityPolicyManifestTest, InsecureContentSecurityPolicy) {
   Testcase testcases[] = {
-    Testcase("insecure_contentsecuritypolicy_1.json",
-        errors::kInsecureContentSecurityPolicy),
-    Testcase("insecure_contentsecuritypolicy_2.json",
-        errors::kInsecureContentSecurityPolicy),
-    Testcase("insecure_contentsecuritypolicy_3.json",
-        errors::kInsecureContentSecurityPolicy),
-  };
-  RunTestcases(testcases, arraysize(testcases), EXPECT_TYPE_ERROR);
+      Testcase(
+          "insecure_contentsecuritypolicy_1.json",
+          ErrorUtils::FormatErrorMessage(errors::kInvalidCSPInsecureValue,
+                                         "http://example.com", "script-src")),
+      Testcase("insecure_contentsecuritypolicy_2.json",
+               ErrorUtils::FormatErrorMessage(errors::kInvalidCSPInsecureValue,
+                                              "'unsafe-inline'", "script-src")),
+      Testcase("insecure_contentsecuritypolicy_3.json",
+               ErrorUtils::FormatErrorMessage(
+                   errors::kInvalidCSPMissingSecureSrc, "object-src"))};
+  RunTestcases(testcases, arraysize(testcases), EXPECT_TYPE_WARNING);
 }

chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc

@@ -89,14 +89,15 @@ TEST_F(PlatformAppsManifestTest, PlatformAppContentSecurityPolicy) {
   EXPECT_EQ(0U, extension->install_warnings().size())
       << "Unexpected warning " << extension->install_warnings()[0].message;
   EXPECT_TRUE(extension->is_platform_app());
-  EXPECT_EQ("default-src 'self' https://www.google.com",
+  EXPECT_EQ("default-src 'self' https://www.google.com;",
             CSPInfo::GetResourceContentSecurityPolicy(extension.get(),
                                                       std::string()));
 
   // But even whitelisted ones must specify a secure policy.
-  LoadAndExpectError(
+  LoadAndExpectWarning(
       "init_platform_app_csp_insecure.json",
-      errors::kInsecureContentSecurityPolicy);
+      ErrorUtils::FormatErrorMessage(errors::kInvalidCSPInsecureValue,
+                                     "http://www.google.com", "default-src"));
 }
 
 TEST_F(PlatformAppsManifestTest, CertainApisRequirePlatformApps) {

chrome/common/extensions/manifest_tests/extension_manifests_sandboxed_unittest.cc

@@ -37,7 +37,7 @@ TEST_F(SandboxedPagesManifestTest, SandboxedPages) {
 
   const char kSandboxedCSP[] = "sandbox allow-scripts allow-forms allow-popups";
   const char kDefaultCSP[] =
-      "script-src 'self' chrome-extension-resource:; object-src 'self'";
+      "script-src 'self' chrome-extension-resource:; object-src 'self';";
   const char kCustomSandboxedCSP[] =
       "sandbox; script-src: https://www.google.com";
 

extensions/common/csp_validator.h

@@ -43,8 +43,13 @@ enum Options {
 // case for extensions. Platform apps disallow it.
 //
 // |options| is a bitmask of Options.
-bool ContentSecurityPolicyIsSecure(
-    const std::string& policy, int options);
+//
+// If |warnings| is not NULL, any validation errors are appended to |warnings|.
+// Returns the sanitized policy.
+std::string SanitizeContentSecurityPolicy(
+    const std::string& policy,
+    int options,
+    std::vector<InstallWarning>* warnings);
 
 // Checks whether the given |policy| enforces a unique origin sandbox as
 // defined by http://www.whatwg.org/specs/web-apps/current-work/multipage/

extensions/common/manifest_constants.cc

@@ -314,10 +314,15 @@ const char kInvalidContentPackSites[] =
     "Invalid value for Content Pack sites - files must be strings.";
 const char kInvalidContentScript[] =
     "Invalid value for 'content_scripts[*]'.";
-const char kInvalidContentSecurityPolicy[] =
-    "Invalid value for 'content_security_policy'.";
 const char kInvalidContentScriptsList[] =
     "Invalid value for 'content_scripts'.";
+const char kInvalidContentSecurityPolicy[] =
+    "Invalid value for 'content_security_policy'.";
+const char kInvalidCSPInsecureValue[] =
+    "Ignored insecure CSP value \"*\" in directive '*'.";
+const char kInvalidCSPMissingSecureSrc[] =
+    "CSP directive '*' must be specified (either explicitly, or implicitly via"
+    " 'default-src') and must whitelist only secure resources.";
 const char kInvalidCss[] =
     "Invalid value for 'content_scripts[*].css[*]'.";
 const char kInvalidCssList[] =
@@ -627,14 +632,6 @@ const char kInvalidWebURLs[] =
     "Invalid value for 'app.urls'.";
 const char kInvalidZipHash[] =
     "Required key 'zip_hash' is missing or invalid.";
-const char kInsecureContentSecurityPolicy[] =
-    "Invalid value for 'content_security_policy': Both 'script-src' and"
-    " 'object-src' directives must be specified (either explicitly, or"
-    " implicitly via 'default-src'), and both must whitelist only secure"
-    " resources. You may include any of the following sources: \"'self'\","
-    " \"'unsafe-eval'\", \"http://127.0.0.1\", \"http://localhost\", or any"
-    " \"https://\" or \"chrome-extension://\" origin. For more information,"
-    " see http://developer.chrome.com/extensions/contentSecurityPolicy.html";
 const char kKeyIsDeprecatedWithReplacement[] =
     "Key \"*\" is deprecated.  Key \"*\" should be used instead.";
 const char kLauncherPagePageRequired[] =

extensions/common/manifest_constants.h

@@ -279,6 +279,8 @@ extern const char kInvalidContentPackSites[];
 extern const char kInvalidContentScript[];
 extern const char kInvalidContentScriptsList[];
 extern const char kInvalidContentSecurityPolicy[];
+extern const char kInvalidCSPInsecureValue[];
+extern const char kInvalidCSPMissingSecureSrc[];
 extern const char kInvalidCss[];
 extern const char kInvalidCssList[];
 extern const char kInvalidDefaultLocale[];

extensions/common/manifest_handlers/csp_info.cc

@@ -9,6 +9,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "extensions/common/csp_validator.h"
+#include "extensions/common/install_warning.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/sandboxed_page_info.h"
 
@@ -18,12 +19,12 @@ namespace keys = manifest_keys;
 namespace errors = manifest_errors;
 
 using csp_validator::ContentSecurityPolicyIsLegal;
-using csp_validator::ContentSecurityPolicyIsSecure;
+using csp_validator::SanitizeContentSecurityPolicy;
 
 namespace {
 
 const char kDefaultContentSecurityPolicy[] =
-    "script-src 'self' chrome-extension-resource:; object-src 'self'";
+    "script-src 'self' chrome-extension-resource:; object-src 'self';";
 
 #define PLATFORM_APP_LOCAL_CSP_SOURCES \
     "'self' data: chrome-extension-resource:"
@@ -31,18 +32,18 @@ const char kDefaultPlatformAppContentSecurityPolicy[] =
     // Platform apps can only use local resources by default.
     "default-src 'self' chrome-extension-resource:;"
     // For remote resources, they can fetch them via XMLHttpRequest.
-    "connect-src *;"
+    " connect-src *;"
     // And serve them via data: or same-origin (blob:, filesystem:) URLs
-    "style-src " PLATFORM_APP_LOCAL_CSP_SOURCES " 'unsafe-inline';"
-    "img-src " PLATFORM_APP_LOCAL_CSP_SOURCES ";"
-    "frame-src " PLATFORM_APP_LOCAL_CSP_SOURCES ";"
-    "font-src " PLATFORM_APP_LOCAL_CSP_SOURCES ";"
+    " style-src " PLATFORM_APP_LOCAL_CSP_SOURCES " 'unsafe-inline';"
+    " img-src " PLATFORM_APP_LOCAL_CSP_SOURCES ";"
+    " frame-src " PLATFORM_APP_LOCAL_CSP_SOURCES ";"
+    " font-src " PLATFORM_APP_LOCAL_CSP_SOURCES ";"
     // Media can be loaded from remote resources since:
     // 1. <video> and <audio> have good fallback behavior when offline or under
     //    spotty connectivity.
     // 2. Fetching via XHR and serving via blob: URLs currently does not allow
     //    streaming or partial buffering.
-    "media-src *;";
+    " media-src *;";
 
 int GetValidatorOptions(Extension* extension) {
   int options = csp_validator::OPTIONS_NONE;
@@ -108,8 +109,10 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
           kDefaultPlatformAppContentSecurityPolicy :
           kDefaultContentSecurityPolicy;
 
-      CHECK(ContentSecurityPolicyIsSecure(content_security_policy,
-                                          GetValidatorOptions(extension)));
+      CHECK_EQ(content_security_policy,
+               SanitizeContentSecurityPolicy(content_security_policy,
+                                             GetValidatorOptions(extension),
+                                             NULL));
       extension->SetManifestData(keys::kContentSecurityPolicy,
                                  new CSPInfo(content_security_policy));
     }
@@ -125,11 +128,14 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
     *error = base::ASCIIToUTF16(errors::kInvalidContentSecurityPolicy);
     return false;
   }
-  if (extension->manifest_version() >= 2 &&
-      !ContentSecurityPolicyIsSecure(content_security_policy,
-                                     GetValidatorOptions(extension))) {
-    *error = base::ASCIIToUTF16(errors::kInsecureContentSecurityPolicy);
-    return false;
+  std::string sanitized_csp;
+  if (extension->manifest_version() >= 2) {
+    std::vector<InstallWarning> warnings;
+    content_security_policy =
+        SanitizeContentSecurityPolicy(content_security_policy,
+                                      GetValidatorOptions(extension),
+                                      &warnings);
+    extension->AddInstallWarnings(warnings);
   }
 
   extension->SetManifestData(keys::kContentSecurityPolicy,