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..2a23e9b990adfc07fa7c26d4466730acf4cb04f3 100644
--- a/extensions/common/csp_validator.cc
+++ b/extensions/common/csp_validator.cc
@@ -8,10 +8,13 @@
 
 #include <vector>
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/error_utils.h"
@@ -28,12 +31,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 +67,33 @@ const char* const kHashSourcePrefixes[] = {
   "'sha512-"
 };
 
-struct DirectiveStatus {
-  explicit DirectiveStatus(const char* name)
-      : directive_name(name), seen_in_policy(false) {}
+class DirectiveStatus {

[Devlin, 2016/12/20 17:36:38]

Class comments

[lazyboy, 2016/12/22 03:07:30]

Done.

+ public:
+  DirectiveStatus(const std::string& name) { directive_names_.push_back(name); }

[Devlin, 2016/12/20 17:36:38]

Could we just do:
DirectiveStatus(std::initializer_list<const char*> directives) {
  directive_names_.assign(directives.begin(), directives.end());
}
(or maybe just ": directive_names_(directives.begin(), directives.end())" works
too?)

Then to construct
DirectiveStatus({"script-src"});
DirectiveStatus({"script-src", "child-src"});

[lazyboy, 2016/12/22 03:07:29]

Nice, done.

+  // Subframe related directives can have multiple directive names: "child-src"
+  // or "frame-src".
+  DirectiveStatus(const std::string& name1, const std::string& name2) {
+    directive_names_.push_back(name1);
+    directive_names_.push_back(name2);
+  }
+  bool Matches(const std::string& directive_name) const {

[Devlin, 2016/12/20 17:36:38]

\n
function comments

[lazyboy, 2016/12/22 03:07:30]

Done.

+    for (const auto& directive : directive_names_) {
+      if (!base::CompareCaseInsensitiveASCII(directive_name, directive))
+        return true;
+    }
+    return false;
+  }

[Devlin, 2016/12/20 17:36:39]

\n

[lazyboy, 2016/12/22 03:07:29]

Done.

+  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<std::string> directive_names_;
+  bool seen_in_policy_ = false;

[Devlin, 2016/12/20 17:36:38]

if we don't need to copy this, DISALLOW_COPY_AND_ASSIGN

[Devlin, 2016/12/20 17:36:39]

member comments

[lazyboy, 2016/12/22 03:07:29]

Done.

[lazyboy, 2016/12/22 03:07:30]

Done.

 };
 
 // Returns whether |url| starts with |scheme_and_separator| and does not have a
@@ -124,7 +157,7 @@ bool isNonWildcardTLD(const std::string& url,
 }
 
 // Checks whether the source is a syntactically valid hash.
-bool IsHashSource(const std::string& source) {
+bool IsHashSource(const base::StringPiece& source) {

[Devlin, 2016/12/20 17:36:38]

usually we don't pass StringPiece by const&, because it's nearly as cheap to
copy as to pass the ref.

[lazyboy, 2016/12/22 03:07:30]

Done.

   size_t hash_end = source.length() - 1;
   if (source.empty() || source[hash_end] != '\'') {

[Devlin, 2016/12/20 17:36:38]

since you're here, source.back()?

[lazyboy, 2016/12/22 03:07:29]

Done.

     return false;
@@ -151,14 +184,14 @@ 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_literal = tokenizer->token();
+std::string GetSecureDirectiveValues(
+    int options,
+    const std::string& directive_name,
+    const std::vector<base::StringPiece>& directive_values,
+    std::vector<InstallWarning>* warnings) {
+  std::vector<std::string> sane_csp_parts(1, directive_name);
+  for (const base::StringPiece& directive_value : directive_values) {
+    base::StringPiece source_literal = directive_value;

[Devlin, 2016/12/20 17:36:38]

May as well just have a mutable variable in the for loop
for (base::StringPiece directive_value : directive_values)

[lazyboy, 2016/12/22 03:07:30]

Done.

     std::string source_lower = base::ToLowerASCII(source_literal);
     bool is_secure_csp_token = false;
 
@@ -186,40 +219,50 @@ void GetSecureDirectiveValues(const std::string& directive_name,
     }
 
     if (is_secure_csp_token) {
-      sane_csp_parts->push_back(source_literal);
+      sane_csp_parts.push_back(source_literal.as_string());
     } else if (warnings) {
       warnings->push_back(CSPInstallWarning(ErrorUtils::FormatErrorMessage(
-          manifest_errors::kInvalidCSPInsecureValue, source_literal,
+          manifest_errors::kInvalidCSPInsecureValue, source_literal.as_string(),
           directive_name)));
     }
   }
   // 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(';');
+  sane_csp_parts.back().push_back(kDirectiveSeparator);
+  return base::JoinString(sane_csp_parts, " ");
 }
 
-// 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;
+std::string GetAppSandboxSecureDirectiveValues(

[Devlin, 2016/12/20 17:36:38]

function comments

[lazyboy, 2016/12/22 03:07:30]

Done.

+    const std::string& directive_name,
+    const std::vector<base::StringPiece>& directive_values,
+    std::vector<InstallWarning>* warnings) {
+  std::vector<std::string> sane_csp_parts(1, directive_name);
+  bool seen_self_or_none = false;
+  for (const base::StringPiece& directive_value : directive_values) {
+    base::StringPiece source_literal = directive_value;

[Devlin, 2016/12/20 17:36:38]

ditto

[lazyboy, 2016/12/22 03:07:29]

Done.

+    std::string source_lower = base::ToLowerASCII(source_literal);
 
-  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);
+    // 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] == '\'') {

[Devlin, 2016/12/20 17:36:39]

source_lower.back()

[lazyboy, 2016/12/22 03:07:30]

Done.

+      if (source_lower == "'none'" || source_lower == "'self'")
+        seen_self_or_none |= true;

[Devlin, 2016/12/20 17:36:39]

optional nit: could one-line this:
seen_self_or_none |= source_lower == "'none'" || source_lower == "'self'";

[lazyboy, 2016/12/22 03:07:30]

Done.

+      sane_csp_parts.push_back(source_lower);
+    } else if (warnings) {
+      warnings->push_back(CSPInstallWarning(ErrorUtils::FormatErrorMessage(
+          manifest_errors::kInvalidCSPInsecureValue, source_literal.as_string(),
+          directive_name)));
+    }
   }
-  return true;
+
+  if (!seen_self_or_none)
+    sane_csp_parts.push_back("'self'");

[Devlin, 2016/12/20 17:36:38]

comment why we do this.

[lazyboy, 2016/12/22 03:07:30]

Done.

+
+  sane_csp_parts.back().push_back(kDirectiveSeparator);
+  return base::JoinString(sane_csp_parts, " ");
 }
 
 // Returns true if the |plugin_type| is one of the fully sandboxed plugin types.
@@ -259,6 +302,191 @@ bool AllowedToHaveInsecureObjectSrc(
   return false;
 }
 
+using SecureDirectiveValueFunction = base::Callback<std::string(
+    const std::string& directive_name,
+    const std::vector<base::StringPiece>& directive_values,
+    std::vector<InstallWarning>* warnings)>;
+
+class CSPDirectiveToken {

[Devlin, 2016/12/20 17:36:39]

comments for this class + for methods

[lazyboy, 2016/12/22 03:07:30]

Done.

+ public:
+  explicit CSPDirectiveToken(base::StringPiece directive_token)
+      : directive_token_(directive_token),
+        parsed_(false),
+        tokenizer_(directive_token.begin(), directive_token.end(), " \t\r\n") {
+    is_empty_ = !tokenizer_.GetNext();
+    if (!is_empty_)
+      directive_name_ = tokenizer_.token();
+  }
+  bool MatchAndUpdateStatus(DirectiveStatus* status,
+                            const SecureDirectiveValueFunction& secure_function,
+                            std::vector<InstallWarning>* warnings) {
+    if (is_empty_ || !status->Matches(directive_name_))
+      return false;
+
+    EnsureTokenPartsParsed();
+
+    bool is_duplicate_directive = status->seen_in_policy();
+    status->set_seen_in_policy();
+
+    secure_value_ = secure_function.Run(
+        directive_name_, directive_values_,
+        // 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). Therefore, set warnings
+        // param to nullptr.
+        is_duplicate_directive ? nullptr : warnings);
+    return true;
+  }
+  std::string ToString() {
+    if (secure_value_)
+      return secure_value_.value();
+    // This token didn't require modification.
+    return base::StringPrintf("%s%c", directive_token_.as_string().c_str(),

[Devlin, 2016/12/20 17:36:39]

doesn't base::StringPiece::as_string().c_str() == base::StringPiece::data()? 
(well, in the sense of the value of the cstring, not pointer equality)

[lazyboy, 2016/12/22 03:07:30]

.data() isn't \0 terminated. So can't use.

+                              kDirectiveSeparator);
+  }
+
+ private:
+  void EnsureTokenPartsParsed() {
+    if (!parsed_) {
+      while (tokenizer_.GetNext())
+        directive_values_.push_back(tokenizer_.token_piece());
+      parsed_ = true;
+    }
+  }
+
+  base::StringPiece directive_token_;
+  std::string directive_name_;
+  std::vector<base::StringPiece> directive_values_;
+
+  base::Optional<std::string> secure_value_;
+
+  bool is_empty_;
+  bool parsed_;
+  base::CStringTokenizer tokenizer_;
+
+  DISALLOW_COPY_AND_ASSIGN(CSPDirectiveToken);
+};
+
+class CSPEnforcer {

[Devlin, 2016/12/20 17:36:38]

comments

[lazyboy, 2016/12/22 03:07:30]

Done.

+ public:
+  CSPEnforcer(bool show_missing_csp_warnings,
+              const SecureDirectiveValueFunction& secure_function)
+      : show_missing_csp_warnings_(show_missing_csp_warnings),
+        secure_function_(secure_function) {}
+  virtual ~CSPEnforcer() {}
+
+  std::string Enforce(const std::string& policy,
+                      std::vector<InstallWarning>* warnings);
+
+ protected:
+  virtual std::string GetDefaultCSPValue(const DirectiveStatus& status) = 0;
+
+  std::vector<DirectiveStatus> directives_;
+  std::vector<std::string> enforced_csp_parts_;
+  const bool show_missing_csp_warnings_;
+  const SecureDirectiveValueFunction secure_function_;
+};

[Devlin, 2016/12/20 17:36:38]

DISALLOW_COPY_AND_ASSIGN

[lazyboy, 2016/12/22 03:07:29]

Done.

+
+std::string CSPEnforcer::Enforce(const std::string& policy,
+                                 std::vector<InstallWarning>* warnings) {

[Devlin, 2016/12/20 17:36:38]

Maybe DCHECK(!directives_.empty())

[lazyboy, 2016/12/22 03:07:30]

Done.

+  std::vector<std::string> enforced_csp_parts;
+
+  // If any directive that we care about isn't explicitly listed in |policy|,
+  // "default-src" fallback is used.
+  DirectiveStatus default_src_status(kDefaultSrc);
+  std::vector<InstallWarning> default_src_csp_warnings;
+
+  for (const base::StringPiece& directive_token : base::SplitStringPiece(
+           policy, ";", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
+    CSPDirectiveToken csp_directive_token(directive_token);
+    bool matches_enforcing_directive = false;
+    for (auto& directive_status : directives_) {
+      if (csp_directive_token.MatchAndUpdateStatus(
+              &directive_status, secure_function_, warnings)) {
+        matches_enforcing_directive = true;
+        break;
+      }
+    }
+    if (!matches_enforcing_directive) {
+      csp_directive_token.MatchAndUpdateStatus(
+          &default_src_status, secure_function_, &default_src_csp_warnings);
+    }
+
+    enforced_csp_parts.push_back(csp_directive_token.ToString());
+  }
+
+  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 {
+    // 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,
+            directive_status.name())));
+      }
+    }
+  }
+
+  return base::JoinString(enforced_csp_parts, " ");
+}
+
+class ExtensionCSPEnforcer : public CSPEnforcer {
+ public:
+  ExtensionCSPEnforcer(bool allow_insecure_object_src, int options)
+      : CSPEnforcer(true, base::Bind(&GetSecureDirectiveValues, options)) {
+    directives_.push_back(DirectiveStatus(kScriptSrc));
+    if (!allow_insecure_object_src)
+      directives_.push_back(DirectiveStatus(kObjectSrc));
+  }
+
+ protected:
+  std::string GetDefaultCSPValue(const DirectiveStatus& status) override {
+    if (status.Matches(kObjectSrc))
+      return kObjectSrcDefaultDirective;
+    DCHECK(status.Matches(kScriptSrc));
+    return kScriptSrcDefaultDirective;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExtensionCSPEnforcer);
+};
+
+class AppSandboxPageCSPEnforcer : public CSPEnforcer {
+ public:
+  AppSandboxPageCSPEnforcer()
+      : CSPEnforcer(false, base::Bind(&GetAppSandboxSecureDirectiveValues)) {
+    directives_.push_back(DirectiveStatus(kChildSrc, kFrameSrc));
+    directives_.push_back(DirectiveStatus(kScriptSrc));
+  }
+
+ protected:
+  std::string GetDefaultCSPValue(const DirectiveStatus& status) override {
+    if (status.Matches(kChildSrc))
+      return kAppSandboxSubframeSrcDefaultDirective;
+    DCHECK(status.Matches(kScriptSrc));
+    return kAppSandboxScriptSrcDefaultDirective;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AppSandboxPageCSPEnforcer);
+};
+
 }  //  namespace
 
 bool ContentSecurityPolicyIsLegal(const std::string& policy) {
@@ -267,7 +495,7 @@ bool ContentSecurityPolicyIsLegal(const std::string& policy) {
   const char kBadChars[] = {',', '\r', '\n', '\0'};
 
   return policy.find_first_of(kBadChars, 0, arraysize(kBadChars)) ==
-      std::string::npos;
+         std::string::npos;
 }
 
 std::string SanitizeContentSecurityPolicy(
@@ -278,63 +506,17 @@ std::string SanitizeContentSecurityPolicy(
   std::vector<std::string> directives = base::SplitString(
       policy, ";", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
 
-  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");
-    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))
-      continue;
-    if (!allow_insecure_object_src &&
-        UpdateStatus(directive_name, &tokenizer, &object_src_status, options,
-                     &sane_csp_parts, warnings))
-      continue;
-
-    // Pass the other CSP directives as-is without further validation.
-    sane_csp_parts.push_back(input + ";");
-  }
-
-  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)));
-    }
-  }
+  ExtensionCSPEnforcer csp_enforcer(allow_insecure_object_src, options);
+  return csp_enforcer.Enforce(policy, warnings);
+}
 
-  return base::JoinString(sane_csp_parts, " ");
+std::string GetEffectiveSandoxedPageCSP(const std::string& policy,
+                                        std::vector<InstallWarning>* warnings) {
+  AppSandboxPageCSPEnforcer csp_enforcer;
+  return csp_enforcer.Enforce(policy, warnings);
 }
 
 bool ContentSecurityPolicyIsSandboxed(