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

extensions/common/csp_validator.cc

Index: extensions/common/csp_validator.cc
diff --git a/extensions/common/csp_validator.cc b/extensions/common/csp_validator.cc
index 3224ad6e1324d3348bb6d30be6372e2d982aa8a9..e3dd2253c37d63d74264a2702c84cf4f13377179 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 {
@@ -22,6 +25,9 @@ namespace {
 const char kDefaultSrc[] = "default-src";
 const char kScriptSrc[] = "script-src";
 const char kObjectSrc[] = "object-src";
+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";
@@ -29,14 +35,10 @@ const char kAllowTopNavigation[] = "allow-top-navigation";
 
 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
@@ -54,12 +56,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
@@ -106,8 +102,12 @@ bool isNonWildcardTLD(const std::string& url,
   return registry_length != 0;
 }
 
-bool HasOnlySecureTokens(base::StringTokenizer& tokenizer,
-                         int options) {
+void GetSecureDirectiveValues(const std::string& directive_name,
+                              base::StringTokenizer& tokenizer,
+                              int options,
+                              std::vector<std::string>& csp_parts,
+                              std::vector<std::string>* csp_warnings) {
+  csp_parts.push_back(directive_name);

[not at google - send to devlin, 2014/12/01 19:19:31]

Only pass const references. If you need to modify a reference, pass it as a
pointer instead.

[robwu, 2014/12/02 23:42:09]

Done.

   while (tokenizer.GetNext()) {
     std::string source = tokenizer.token();
     base::StringToLowerASCII(&source);
@@ -128,31 +128,47 @@ bool HasOnlySecureTokens(base::StringTokenizer& tokenizer,
                          url::kStandardSchemeSeparator,
                          false) ||
         StartsWithASCII(source, "chrome-extension-resource:", true)) {
+      csp_parts.push_back(source);
       continue;
     }
 
     if (options & OPTIONS_ALLOW_UNSAFE_EVAL) {
-      if (source == "'unsafe-eval'")
+      if (source == "'unsafe-eval'") {
+        csp_parts.push_back(source);
         continue;
+      }
+    }

[not at google - send to devlin, 2014/12/01 19:19:31]

I think this would be slightly easier to read if you used a bool "safe"
variable, setting to true rather than continuing. That would mean
- instead of continue, "safe = true"
- at the end, it would be 
if (safe) {
  csp_parts->push_back(source);
} else if (csp_warnings) {
  csp_warnings->push_back(...);
}

[robwu, 2014/12/02 23:42:09]

Done.

+    if (csp_warnings) {
+      csp_warnings->push_back(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.
+  csp_parts.back().push_back(';');
 }
 
 // Returns true if |directive_name| matches |status.directive_name|.
 bool UpdateStatus(const std::string& directive_name,
                   base::StringTokenizer& tokenizer,
                   DirectiveStatus* status,
-                  int options) {
-  if (status->seen_in_policy)
-    return false;
+                  int options,
+                  std::vector<std::string>& csp_parts,

[not at google - send to devlin, 2014/12/01 19:19:31]

Same comment re reference vs pointer.

[robwu, 2014/12/02 23:42:08]

Done.

+                  std::vector<std::string>* csp_warnings) {
   if (directive_name != status->directive_name)
     return false;
-  status->seen_in_policy = true;
-  status->is_secure = HasOnlySecureTokens(tokenizer, options);
+
+  if (!status->seen_in_policy) {
+    status->seen_in_policy = true;
+    GetSecureDirectiveValues(directive_name, tokenizer, options, csp_parts,
+                             csp_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, csp_parts,
+                             NULL);
+  }
   return true;
 }
 
@@ -168,7 +184,9 @@ bool ContentSecurityPolicyIsLegal(const std::string& policy) {
 }
 
 bool ContentSecurityPolicyIsSecure(const std::string& policy,
-                                   int options) {
+                                   int options,
+                                   std::string* sanitized_csp,

[not at google - send to devlin, 2014/12/01 19:19:31]

I'm worried that the way the code has been modified allows silently allowing an
insecure CSP, if callers are not careful. That is, if the caller passes in a
NULL |sanitized_csp| and |warnings| -- perfectly normal, if they don't care -
but |policy| was "sanitized" and this method ends up returning true - bad things
could happen.

Renaming this method to EvaluateContentSecurityPolicy would help, or even
SanitizeContentSecurityPolicy since that's basically what it does now. I don't
even know why it needs to return a bool at all, it could just return the
sanitized string (caller can detect whether it's the same as the old string).

I'm not sure how much of this is a reasonable change to make at this stage.

[robwu, 2014/12/02 23:42:08]

SanitizeContentSecurityPolicy sgtm. I'll do that in a separate patch set to make
review easier (search & replace + fix spacing & comments).
The "sanitized CSP" does not need to be equal to the input CSP. E.g.
"default-src<SP><SP>'self'" becomes "default-src<SP>'self';" (note: a space
disappeared and a semicolon was added).
This method is only used in two places, we can drop the bool and use CHECK_EQ
instead of CHECK.
Perfectly reasonable.

+                                   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);
@@ -177,6 +195,9 @@ bool ContentSecurityPolicyIsSecure(const std::string& policy,
   DirectiveStatus script_src_status(kScriptSrc);
   DirectiveStatus object_src_status(kObjectSrc);
 
+  std::vector<std::string> csp_parts;

[not at google - send to devlin, 2014/12/01 19:19:31]

sane_csp? (basically what this is generating).

[robwu, 2014/12/02 23:42:09]

Done. (sane_csp_parts)

+  std::vector<std::string> csp_warnings;
+  std::vector<std::string> default_src_csp_warnings;

[not at google - send to devlin, 2014/12/01 19:19:31]

Can you just directly pass around |warnings|?

[robwu, 2014/12/02 23:42:08]

Instead of csp_warnings? Yes, especially after I drop the boolean return value.

[not at google - send to devlin, 2014/12/03 20:33:05]

Yes that is what I meant; don't even declare a separate |csp_warnings| variable.

   for (size_t i = 0; i < directives.size(); ++i) {
     std::string& input = directives[i];
     base::StringTokenizer tokenizer(input, " \t\r\n");
@@ -186,31 +207,56 @@ 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))
+    if (UpdateStatus(directive_name, tokenizer, &default_src_status, options,
+                     csp_parts, &default_src_csp_warnings))
       continue;
-    if (UpdateStatus(directive_name, tokenizer, &script_src_status, options))
+    if (UpdateStatus(directive_name, tokenizer, &script_src_status, options,
+                     csp_parts, &csp_warnings))
       continue;
-    if (UpdateStatus(directive_name, tokenizer, &object_src_status, options))
+    if (!(options & OPTIONS_ALLOW_INSECURE_OBJECT_SRC) &&
+        UpdateStatus(directive_name, tokenizer, &object_src_status, options,
+                     csp_parts, &csp_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 (!(options & OPTIONS_ALLOW_INSECURE_OBJECT_SRC))
-      return false;
+    // Pass the other CSP directives as-is without further validation.
+    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.
+      csp_warnings.insert(csp_warnings.end(),
+                          default_src_csp_warnings.begin(),
+                          default_src_csp_warnings.end());
+    }
+  } else {
+    if (!script_src_status.seen_in_policy) {
+      csp_parts.push_back(kScriptSrcDefaultDirective);
+      csp_warnings.push_back(ErrorUtils::FormatErrorMessage(
+          manifest_errors::kInvalidCSPMissingSecureSrc, kScriptSrc));
+    }
+    if (!object_src_status.seen_in_policy &&
+        !(options & OPTIONS_ALLOW_INSECURE_OBJECT_SRC)) {
+      csp_parts.push_back(kObjectSrcDefaultDirective);
+      csp_warnings.push_back(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);
+  if (sanitized_csp)
+    *sanitized_csp = JoinString(csp_parts, ' ');
+  if (csp_warnings.empty())
+    return true;
+
+  if (warnings) {
+    for (auto csp_warning : csp_warnings) {

[not at google - send to devlin, 2014/12/01 19:19:31]

To avoid string copy I think you need const auto&

[robwu, 2014/12/02 23:42:08]

Done.

+      warnings->push_back(
+          InstallWarning(csp_warning, manifest_keys::kContentSecurityPolicy));
+    }
+  }
+  return false;
 }
 
 bool ContentSecurityPolicyIsSandboxed(