Restrict app sandbox's CSP to disallow loading web content in them.

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 e6c789f6f8e59fce0d2afdc732efac15d82a37f6..2b62d68b36f44d6becb49d4e1f23c8b947b27bbd 100644
--- a/extensions/common/csp_validator.cc
+++ b/extensions/common/csp_validator.cc
@@ -28,12 +28,21 @@ namespace {
 const char kDefaultSrc[] = "default-src";
 const char kScriptSrc[] = "script-src";
 const char kObjectSrc[] = "object-src";
+const char kFrameSrc[] = "frame-src";
+const char kChildSrc[] = "child-src";
+
+const char kDirectiveSeparator[] = ";";
+
 const char kPluginTypes[] = "plugin-types";
 
 const char kObjectSrcDefaultDirective[] = "object-src 'self';";
 const char kScriptSrcDefaultDirective[] =
     "script-src 'self' chrome-extension-resource:;";
 
+const char kAppSandboxSubframeSrcDefaultDirective[] = "child-src 'self';";
+const char kAppSandboxScriptSrcDefaultDirective[] =
+    "script-src 'self' 'unsafe-inline' 'unsafe-eval';";
+
 const char kSandboxDirectiveName[] = "sandbox";
 const char kAllowSameOriginToken[] = "allow-same-origin";
 const char kAllowTopNavigation[] = "allow-top-navigation";
@@ -55,12 +64,33 @@ const char* const kHashSourcePrefixes[] = {
   "'sha512-"
 };
 
-struct DirectiveStatus {
-  explicit DirectiveStatus(const char* name)
-      : directive_name(name), seen_in_policy(false) {}
+class DirectiveStatus {
+ public:
+  DirectiveStatus(const char* name) { directive_names_.push_back(name); }
+  // Subframe related directives can have multiple directive names: "child-src"
+  // or "frame-src".
+  DirectiveStatus(const char* name1, const char* name2) {
+    directive_names_.push_back(name1);
+    directive_names_.push_back(name2);
+  }
+  bool Matches(const std::string& directive_name) const {
+    for (const auto& directive : directive_names_) {
+      if (directive_name == directive)
+        return true;
+    }
+    return false;
+  }
+  bool seen_in_policy() const { return seen_in_policy_; }
+  void set_seen_in_policy() { seen_in_policy_ = true; }
 
-  const char* directive_name;
-  bool seen_in_policy;
+  std::string name() const {
+    DCHECK(!directive_names_.empty());
+    return directive_names_[0];
+  }
+
+ private:
+  std::vector<const char*> directive_names_;
+  bool seen_in_policy_ = false;
 };
 
 // Returns whether |url| starts with |scheme_and_separator| and does not have a
@@ -199,29 +229,6 @@ void GetSecureDirectiveValues(const std::string& directive_name,
   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,
-                  DirectiveStatus* status,
-                  int options,
-                  std::vector<std::string>* sane_csp_parts,
-                  std::vector<InstallWarning>* warnings) {
-  if (directive_name != status->directive_name)
-    return false;
-
-  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;
-}
-
 // Returns true if the |plugin_type| is one of the fully sandboxed plugin types.
 bool PluginTypeAllowed(const std::string& plugin_type) {
   for (size_t i = 0; i < arraysize(kSandboxedPluginTypes); ++i) {
@@ -259,82 +266,229 @@ bool AllowedToHaveInsecureObjectSrc(
   return false;
 }
 
-}  //  namespace
-
-bool ContentSecurityPolicyIsLegal(const std::string& policy) {
-  // We block these characters to prevent HTTP header injection when
-  // representing the content security policy as an HTTP header.
-  const char kBadChars[] = {',', '\r', '\n', '\0'};
-
-  return policy.find_first_of(kBadChars, 0, arraysize(kBadChars)) ==
-      std::string::npos;
-}
+class CSPEnforcer {
+ public:
+  CSPEnforcer(bool show_missing_csp_warnings)
+      : show_missing_csp_warnings_(show_missing_csp_warnings) {}
+  virtual ~CSPEnforcer() {}
+
+  std::string Enforce(const std::string& policy,
+                      std::vector<InstallWarning>* warnings);
+
+ protected:
+  virtual std::string GetDefaultCSPValue(const DirectiveStatus& status) = 0;
+
+  // Updates the status of a directive |directive_status| with given information
+  // about a directive token. The directive token has name |directive_name| and
+  // its values are in |tokenizer|.
+  //
+  // Returns true if |directive_status| matches |directive_name|.
+  virtual bool VisitAndEnforce(const std::string& directive_name,
+                               DirectiveStatus* directive_status,
+                               base::StringTokenizer* tokenizer,
+                               std::vector<InstallWarning>* warnings) = 0;
+
+  std::vector<DirectiveStatus> directives_;
+  std::vector<std::string> enforced_csp_parts_;
+  const bool show_missing_csp_warnings_;
+};
 
-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, ";", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+std::string CSPEnforcer::Enforce(const std::string& policy,
+                                 std::vector<InstallWarning>* warnings) {

[Devlin, 2016/12/15 00:04:11]

As discussed briefly offline, I still find this pretty hard to grok.  Maybe
parsing, then enforcing (rather than an enforce-as-you-parse) approach would
make this a little more readable.

[lazyboy, 2016/12/15 08:02:35]

I think there are too many cases to consider here which makes things
a bit more complicated.

I also noticed that we just don't want to split by ";" then split
each token by " \t\r\n" [1] right away, because the latter split might be
unnecessary
depending on whether we actually care about this. I think that's why
enforce as we parse style was inspired.

I've made CSPDirectiveToken to express each token and kept all parsing
logic inside that. I've exposed ToString() which we call always: will return
the value without doing [1] (well, I need one tokenizer.GetNext).
I've generalized some more code with base::Bind too.

Using SplitStringPiece to use more StringPieces.

Let's see how you like this version.

+  enforced_csp_parts_.clear();
 
+  // If any directive that we care about isn't explicitly listed in |policy|,
+  // "default-src" fallback is used.
   DirectiveStatus default_src_status(kDefaultSrc);
-  DirectiveStatus script_src_status(kScriptSrc);
-  DirectiveStatus object_src_status(kObjectSrc);
-
-  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");
+
+  // See http://www.w3.org/TR/CSP/#parse-a-csp-policy for parsing algorithm.
+  for (const std::string& directive : base::SplitString(
+           policy, ";", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) {
+    base::StringTokenizer tokenizer(directive, " \t\r\n");
     if (!tokenizer.GetNext())
       continue;
 
     std::string directive_name = base::ToLowerASCII(tokenizer.token_piece());
-    if (UpdateStatus(directive_name, &tokenizer, &default_src_status, options,
-                     &sane_csp_parts, &default_src_csp_warnings))
-      continue;
-    if (UpdateStatus(directive_name, &tokenizer, &script_src_status, options,
-                     &sane_csp_parts, warnings))
+    bool matches_enforcing_directive = false;
+    for (auto& directive_status : directives_) {
+      if (VisitAndEnforce(directive_name, &directive_status, &tokenizer,
+                          warnings)) {
+        matches_enforcing_directive = true;
+        break;
+      }
+    }
+    if (matches_enforcing_directive)
       continue;
-    if (!allow_insecure_object_src &&
-        UpdateStatus(directive_name, &tokenizer, &object_src_status, options,
-                     &sane_csp_parts, warnings))
+
+    if (VisitAndEnforce(directive_name, &default_src_status, &tokenizer,
+                        &default_src_csp_warnings)) {
       continue;
+    }
 
-    // Pass the other CSP directives as-is without further validation.
-    sane_csp_parts.push_back(input + ";");
+    // Keep this directive as is.
+    enforced_csp_parts_.push_back(directive + kDirectiveSeparator);
   }
 
-  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());
+  if (default_src_status.seen_in_policy()) {
+    for (const DirectiveStatus& directive_status : directives_) {
+      if (!directive_status.seen_in_policy()) {
+        // This |directive_status| falls back to "default-src". So warnings from
+        // "default-src" will apply.
+        if (warnings) {
+          warnings->insert(warnings->end(), default_src_csp_warnings.begin(),
+                           default_src_csp_warnings.end());
+        }
+        break;
+      }
     }
   } else {
-    if (!script_src_status.seen_in_policy) {
-      sane_csp_parts.push_back(kScriptSrcDefaultDirective);
-      if (warnings)
+    // Did not see "default-src".
+    // Make sure we cover all sources from |directives_|.
+    for (const DirectiveStatus& directive_status : directives_) {
+      if (directive_status.seen_in_policy())  // Already covered.
+        continue;
+      enforced_csp_parts_.push_back(GetDefaultCSPValue(directive_status));
+
+      if (warnings && show_missing_csp_warnings_) {
         warnings->push_back(CSPInstallWarning(ErrorUtils::FormatErrorMessage(
-            manifest_errors::kInvalidCSPMissingSecureSrc, kScriptSrc)));
+            manifest_errors::kInvalidCSPMissingSecureSrc,
+            directive_status.name())));
+      }
     }
-    if (!object_src_status.seen_in_policy && !allow_insecure_object_src) {
-      sane_csp_parts.push_back(kObjectSrcDefaultDirective);
-      if (warnings)
+  }
+
+  return base::JoinString(enforced_csp_parts_, " ");
+}
+
+class ExtensionCSPEnforcer : public CSPEnforcer {
+ public:
+  ExtensionCSPEnforcer(bool allow_insecure_object_src, int options)
+      : CSPEnforcer(true), options_(options) {
+    directives_.push_back(DirectiveStatus(kScriptSrc));
+    if (!allow_insecure_object_src)
+      directives_.push_back(DirectiveStatus(kObjectSrc));
+  }
+
+ protected:
+  bool VisitAndEnforce(const std::string& directive_name,
+                       DirectiveStatus* directive_status,
+                       base::StringTokenizer* tokenizer,
+                       std::vector<InstallWarning>* warnings) override {
+    if (!directive_status->Matches(directive_name))
+      return false;
+
+    if (!directive_status->seen_in_policy()) {
+      directive_status->set_seen_in_policy();
+      GetSecureDirectiveValues(directive_name, tokenizer, options_,
+                               &enforced_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_,
+                               &enforced_csp_parts_, nullptr);
+    }
+    return true;
+  }
+
+  std::string GetDefaultCSPValue(const DirectiveStatus& status) override {
+    if (status.Matches(kObjectSrc))
+      return kObjectSrcDefaultDirective;
+    DCHECK(status.Matches(kScriptSrc));
+    return kScriptSrcDefaultDirective;
+  }
+
+ private:
+  const int options_;
+};
+
+class AppSandboxPageCSPEnforcer : public CSPEnforcer {
+ public:
+  AppSandboxPageCSPEnforcer() : CSPEnforcer(false) {
+    directives_.push_back(DirectiveStatus(kChildSrc, kFrameSrc));
+    directives_.push_back(DirectiveStatus(kScriptSrc));
+  }
+
+ protected:
+  bool VisitAndEnforce(const std::string& directive_name,
+                       DirectiveStatus* directive_status,
+                       base::StringTokenizer* tokenizer,
+                       std::vector<InstallWarning>* warnings) override {
+    if (!directive_status->Matches(directive_name))
+      return false;
+
+    // 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).
+    bool is_duplicate_directive = directive_status->seen_in_policy();
+    directive_status->set_seen_in_policy();
+
+    enforced_csp_parts_.push_back(directive_name);
+    bool seen_self_or_none = false;
+    while (tokenizer->GetNext()) {
+      std::string source_literal = tokenizer->token();
+      std::string source_lower = base::ToLowerASCII(source_literal);
+
+      // Keyword directive sources are surrounded with quotes, e.g. 'self',
+      // 'sha256-...', 'unsafe-eval', 'nonce-...'. These do not specify a remote
+      // host or '*', so keep them and restrict the rest.
+      if (source_lower.size() > 1u && source_lower[0] == '\'' &&
+          source_lower[source_lower.size() - 1] == '\'') {
+        if (source_lower == "'none'" || source_lower == "'self'")
+          seen_self_or_none |= true;
+        enforced_csp_parts_.push_back(source_lower);
+      } else if (warnings && !is_duplicate_directive) {
         warnings->push_back(CSPInstallWarning(ErrorUtils::FormatErrorMessage(
-            manifest_errors::kInvalidCSPMissingSecureSrc, kObjectSrc)));
+            manifest_errors::kInvalidCSPInsecureValue, source_literal,
+            directive_name)));
+      }
     }
+
+    if (!seen_self_or_none)
+      enforced_csp_parts_.push_back("'self'");
+
+    // Add ";" at the end of the directive.
+    enforced_csp_parts_.back().append(kDirectiveSeparator);
+    return true;
   }
 
-  return base::JoinString(sane_csp_parts, " ");
+  std::string GetDefaultCSPValue(const DirectiveStatus& status) override {
+    if (status.Matches(kChildSrc))
+      return kAppSandboxSubframeSrcDefaultDirective;
+    DCHECK(status.Matches(kScriptSrc));
+    return kAppSandboxScriptSrcDefaultDirective;
+  }
+};
+
+}  //  namespace
+
+bool ContentSecurityPolicyIsLegal(const std::string& policy) {
+  // We block these characters to prevent HTTP header injection when
+  // representing the content security policy as an HTTP header.
+  const char kBadChars[] = {',', '\r', '\n', '\0'};
+
+  return policy.find_first_of(kBadChars, 0, arraysize(kBadChars)) ==
+         std::string::npos;
+}
+
+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, kDirectiveSeparator, base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+
+  bool allow_insecure_object_src =
+      AllowedToHaveInsecureObjectSrc(options, directives);
+
+  ExtensionCSPEnforcer csp_enforcer(allow_insecure_object_src, options);
+  return csp_enforcer.Enforce(policy, warnings);
+}
+
+std::string GetEffectiveSandoxedPageCSP(const std::string& policy,
+                                        std::vector<InstallWarning>* warnings) {
+  AppSandboxPageCSPEnforcer csp_enforcer;
+  return csp_enforcer.Enforce(policy, warnings);
 }
 
 bool ContentSecurityPolicyIsSandboxed(