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}

1 files changed, 104 insertions, 80 deletions

diff --git a/extensions/common/csp_validator.cc b/extensions/common/csp_validator.cc
index 6895b31..371d7f8 100644
--- a/extensions/common/csp_validator.cc
+++ b/extensions/common/csp_validator.cc
@@ -11,6 +11,9 @@
 #include "base/strings/string_util.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/error_utils.h"
+#include "extensions/common/install_warning.h"
+#include "extensions/common/manifest_constants.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 
 namespace extensions {
@@ -24,6 +27,10 @@ const char kScriptSrc[] = "script-src";
 const char kObjectSrc[] = "object-src";
 const char kPluginTypes[] = "plugin-types";
 
+const char kObjectSrcDefaultDirective[] = "object-src 'self';";
+const char kScriptSrcDefaultDirective[] =
+    "script-src 'self' chrome-extension-resource:;";
+
 const char kSandboxDirectiveName[] = "sandbox";
 const char kAllowSameOriginToken[] = "allow-same-origin";
 const char kAllowTopNavigation[] = "allow-top-navigation";
@@ -38,14 +45,10 @@ const char* const kSandboxedPluginTypes[] = {
 
 struct DirectiveStatus {
   explicit DirectiveStatus(const char* name)
-    : directive_name(name)
-    , seen_in_policy(false)
-    , is_secure(false) {
-  }
+      : directive_name(name), seen_in_policy(false) {}
 
   const char* directive_name;
   bool seen_in_policy;
-  bool is_secure;
 };
 
 // Returns whether |url| starts with |scheme_and_separator| and does not have a
@@ -63,12 +66,6 @@ bool isNonWildcardTLD(const std::string& url,
   if (end_of_host == std::string::npos)
     end_of_host = url.size();
 
-  // A missing host such as "chrome-extension://" is invalid, but for backwards-
-  // compatibility, accept such CSP parts. They will be ignored by Blink anyway.
-  // TODO(robwu): Remove this special case once crbug.com/434773 is fixed.
-  if (start_of_host == end_of_host)
-    return true;
-
   // Note: It is sufficient to only compare the first character against '*'
   // because the CSP only allows wildcards at the start of a directive, see
   // host-source and host-part at http://www.w3.org/TR/CSP2/#source-list-syntax
@@ -115,11 +112,20 @@ bool isNonWildcardTLD(const std::string& url,
   return registry_length != 0;
 }
 
-bool HasOnlySecureTokens(base::StringTokenizer& tokenizer,
-                         int options) {
-  while (tokenizer.GetNext()) {
-    std::string source = tokenizer.token();
+InstallWarning CSPInstallWarning(const std::string& csp_warning) {
+  return InstallWarning(csp_warning, manifest_keys::kContentSecurityPolicy);
+}
+
+void GetSecureDirectiveValues(const std::string& directive_name,
+                              base::StringTokenizer* tokenizer,
+                              int options,
+                              std::vector<std::string>* sane_csp_parts,
+                              std::vector<InstallWarning>* warnings) {
+  sane_csp_parts->push_back(directive_name);
+  while (tokenizer->GetNext()) {
+    std::string source = tokenizer->token();
     base::StringToLowerASCII(&source);
+    bool is_secure_csp_token = false;
 
     // We might need to relax this whitelist over time.
     if (source == "'self'" ||
@@ -137,52 +143,45 @@ bool HasOnlySecureTokens(base::StringTokenizer& tokenizer,
                          url::kStandardSchemeSeparator,
                          false) ||
         StartsWithASCII(source, "chrome-extension-resource:", true)) {
-      continue;
+      is_secure_csp_token = true;
+    } else if ((options & OPTIONS_ALLOW_UNSAFE_EVAL) &&
+               source == "'unsafe-eval'") {
+      is_secure_csp_token = true;
     }
 
-    if (options & OPTIONS_ALLOW_UNSAFE_EVAL) {
-      if (source == "'unsafe-eval'")
-        continue;
+    if (is_secure_csp_token) {
+      sane_csp_parts->push_back(source);
+    } else if (warnings) {
+      warnings->push_back(CSPInstallWarning(ErrorUtils::FormatErrorMessage(
+          manifest_errors::kInvalidCSPInsecureValue, source, directive_name)));
     }
-
-    return false;
   }
-
-  return true;  // Empty values default to 'none', which is secure.
+  // End of CSP directive that was started at the beginning of this method. If
+  // none of the values are secure, the policy will be empty and default to
+  // 'none', which is secure.
+  sane_csp_parts->back().push_back(';');
 }
 
 // Returns true if |directive_name| matches |status.directive_name|.
 bool UpdateStatus(const std::string& directive_name,
-                  base::StringTokenizer& tokenizer,
+                  base::StringTokenizer* tokenizer,
                   DirectiveStatus* status,
-                  int options) {
-  if (status->seen_in_policy)
-    return false;
+                  int options,
+                  std::vector<std::string>* sane_csp_parts,
+                  std::vector<InstallWarning>* warnings) {
   if (directive_name != status->directive_name)
     return false;
-  status->seen_in_policy = true;
-  status->is_secure = HasOnlySecureTokens(tokenizer, options);
-  return true;
-}
-
-// Parses the plugin-types directive and returns the list of mime types
-// specified in |plugin_types|.
-bool ParsePluginTypes(const std::string& directive_name,
-                      base::StringTokenizer& tokenizer,
-                      std::vector<std::string>* plugin_types) {
-  DCHECK(plugin_types);
-
-  if (directive_name != kPluginTypes || !plugin_types->empty())
-    return false;
 
-  while (tokenizer.GetNext()) {
-    std::string mime_type = tokenizer.token();
-    base::StringToLowerASCII(&mime_type);
-    // Since we're comparing the mime types to a whitelist, we don't check them
-    // for strict validity right now.
-    plugin_types->push_back(mime_type);
+  if (!status->seen_in_policy) {
+    status->seen_in_policy = true;
+    GetSecureDirectiveValues(directive_name, tokenizer, options, sane_csp_parts,
+                             warnings);
+  } else {
+    // Don't show any errors for duplicate CSP directives, because it will be
+    // ignored by the CSP parser (http://www.w3.org/TR/CSP2/#policy-parsing).
+    GetSecureDirectiveValues(directive_name, tokenizer, options, sane_csp_parts,
+                             NULL);
   }
-
   return true;
 }
 
@@ -201,20 +200,26 @@ bool PluginTypeAllowed(const std::string& plugin_type) {
 // the set specified in kSandboxedPluginTypes.
 bool AllowedToHaveInsecureObjectSrc(
     int options,
-    const std::vector<std::string>& plugin_types) {
+    const std::vector<std::string>& directives) {
   if (!(options & OPTIONS_ALLOW_INSECURE_OBJECT_SRC))
     return false;
 
-  // plugin-types must be specified.
-  if (plugin_types.empty())
-    return false;
-
-  for (const auto& plugin_type : plugin_types) {
-    if (!PluginTypeAllowed(plugin_type))
-      return false;
+  for (size_t i = 0; i < directives.size(); ++i) {
+    const std::string& input = directives[i];
+    base::StringTokenizer tokenizer(input, " \t\r\n");
+    if (!tokenizer.GetNext())
+      continue;
+    if (!LowerCaseEqualsASCII(tokenizer.token(), kPluginTypes))
+      continue;
+    while (tokenizer.GetNext()) {
+      if (!PluginTypeAllowed(tokenizer.token()))
+        return false;
+    }
+    // All listed plugin types are whitelisted.
+    return true;
   }
-
-  return true;
+  // plugin-types not specified.
+  return false;
 }
 
 }  //  namespace
@@ -228,8 +233,10 @@ bool ContentSecurityPolicyIsLegal(const std::string& policy) {
       std::string::npos;
 }
 
-bool ContentSecurityPolicyIsSecure(const std::string& policy,
-                                   int options) {
+std::string SanitizeContentSecurityPolicy(
+    const std::string& policy,
+    int options,
+    std::vector<InstallWarning>* warnings) {
   // See http://www.w3.org/TR/CSP/#parse-a-csp-policy for parsing algorithm.
   std::vector<std::string> directives;
   base::SplitString(policy, ';', &directives);
@@ -238,8 +245,11 @@ bool ContentSecurityPolicyIsSecure(const std::string& policy,
   DirectiveStatus script_src_status(kScriptSrc);
   DirectiveStatus object_src_status(kObjectSrc);
 
-  std::vector<std::string> plugin_types;
+  bool allow_insecure_object_src =
+      AllowedToHaveInsecureObjectSrc(options, directives);
 
+  std::vector<std::string> sane_csp_parts;
+  std::vector<InstallWarning> default_src_csp_warnings;
   for (size_t i = 0; i < directives.size(); ++i) {
     std::string& input = directives[i];
     base::StringTokenizer tokenizer(input, " \t\r\n");
@@ -249,33 +259,47 @@ bool ContentSecurityPolicyIsSecure(const std::string& policy,
     std::string directive_name = tokenizer.token();
     base::StringToLowerASCII(&directive_name);
 
-    if (UpdateStatus(directive_name, tokenizer, &default_src_status, options))
-      continue;
-    if (UpdateStatus(directive_name, tokenizer, &script_src_status, options))
+    if (UpdateStatus(directive_name, &tokenizer, &default_src_status, options,
+                     &sane_csp_parts, &default_src_csp_warnings))
       continue;
-    if (UpdateStatus(directive_name, tokenizer, &object_src_status, options))
+    if (UpdateStatus(directive_name, &tokenizer, &script_src_status, options,
+                     &sane_csp_parts, warnings))
       continue;
-    if (ParsePluginTypes(directive_name, tokenizer, &plugin_types))
+    if (!allow_insecure_object_src &&
+        UpdateStatus(directive_name, &tokenizer, &object_src_status, options,
+                     &sane_csp_parts, warnings))
       continue;
-  }
 
-  if (script_src_status.seen_in_policy && !script_src_status.is_secure)
-    return false;
-
-  if (object_src_status.seen_in_policy && !object_src_status.is_secure) {
-    // Note that this does not fully check the object-src source list for
-    // validity but Blink will do this anyway.
-    if (!AllowedToHaveInsecureObjectSrc(options, plugin_types))
-      return false;
+    // Pass the other CSP directives as-is without further validation.
+    sane_csp_parts.push_back(input + ";");
   }
 
-  if (default_src_status.seen_in_policy && !default_src_status.is_secure) {
-    return script_src_status.seen_in_policy &&
-           object_src_status.seen_in_policy;
+  if (default_src_status.seen_in_policy) {
+    if (!script_src_status.seen_in_policy ||
+        !object_src_status.seen_in_policy) {
+      // Insecure values in default-src are only relevant if either script-src
+      // or object-src is omitted.
+      if (warnings)
+        warnings->insert(warnings->end(),
+                         default_src_csp_warnings.begin(),
+                         default_src_csp_warnings.end());
+    }
+  } else {
+    if (!script_src_status.seen_in_policy) {
+      sane_csp_parts.push_back(kScriptSrcDefaultDirective);
+      if (warnings)
+        warnings->push_back(CSPInstallWarning(ErrorUtils::FormatErrorMessage(
+            manifest_errors::kInvalidCSPMissingSecureSrc, kScriptSrc)));
+    }
+    if (!object_src_status.seen_in_policy && !allow_insecure_object_src) {
+      sane_csp_parts.push_back(kObjectSrcDefaultDirective);
+      if (warnings)
+        warnings->push_back(CSPInstallWarning(ErrorUtils::FormatErrorMessage(
+            manifest_errors::kInvalidCSPMissingSecureSrc, kObjectSrc)));
+    }
   }
 
-  return default_src_status.seen_in_policy ||
-      (script_src_status.seen_in_policy && object_src_status.seen_in_policy);
+  return JoinString(sane_csp_parts, ' ');
 }
 
 bool ContentSecurityPolicyIsSandboxed(