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..409e6404114bd0c79df8c69b495c649264ea4299 100644
--- a/extensions/common/csp_validator.cc
+++ b/extensions/common/csp_validator.cc
@@ -28,12 +28,20 @@ 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';";
+
 const char kSandboxDirectiveName[] = "sandbox";
 const char kAllowSameOriginToken[] = "allow-same-origin";
 const char kAllowTopNavigation[] = "allow-top-navigation";
@@ -276,7 +284,7 @@ std::string SanitizeContentSecurityPolicy(
     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);
+      policy, kDirectiveSeparator, base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
 
   DirectiveStatus default_src_status(kDefaultSrc);
   DirectiveStatus script_src_status(kScriptSrc);
@@ -306,7 +314,7 @@ std::string SanitizeContentSecurityPolicy(
       continue;
 
     // Pass the other CSP directives as-is without further validation.
-    sane_csp_parts.push_back(input + ";");
+    sane_csp_parts.push_back(input + kDirectiveSeparator);
   }
 
   if (default_src_status.seen_in_policy) {
@@ -337,6 +345,63 @@ std::string SanitizeContentSecurityPolicy(
   return base::JoinString(sane_csp_parts, " ");
 }
 
+std::string GetEffectiveSandoxedPageCSP(const std::string& policy) {

[Devlin, 2016/12/09 15:46:01]

I kind of wonder if it makes sense to have a more general parsing for CSP in
some kind of object that's easily modifiable.  E.g.,

class CSP {
 public:
  CSP(std::string);
  std::string ToString();
  // accessors and modifiers
};

And then use that both here and in SanitizeCSP.  WDYT?  It seems like it would
be fairly simple and would make a lot of this code more readable, but I haven't
tried coding it up so maybe I'm missing something.

(There may even already be this somewhere?  But I couldn't find it.  Maybe
Charlie knows.)

[Charlie Reis, 2016/12/09 19:55:07]

I haven't seen one, but I agree that avoiding a bunch of custom parsing logic
for each use case is a good idea.  alexmos@ and mkwst@ are more familiar with
CSP and might know.

[alexmos, 2016/12/12 19:04:11]

I haven't seen one either, and I agree it's a good idea to have a common place
for parsing CSP.  We even have a bug filed for this: https://crbug.com/376522;
the motivation there is to move some of CSP enforcement to the browser process. 
mkwst/arthursonzogni are adding another little bit of CSP frame_ancestors
parsing in ancestor_throttle in https://codereview.chromium.org/2488743003/,
which could also use this.

[lazyboy, 2016/12/14 00:49:05]

I've introduced a CSPEnforcer class that can sanitize csp for apps sandbox and
(existing) extensions.

+  // See http://www.w3.org/TR/CSP/#parse-a-csp-policy for parsing algorithm.
+  std::vector<std::string> effective_csp_parts;
+  bool seen_default_source = false;
+  bool seen_subframe_source = false;
+  bool seen_script_source = false;
+  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 (directive_name == kDefaultSrc) {
+      seen_default_source = true;
+    } else if (directive_name == kChildSrc || directive_name == kFrameSrc) {
+      seen_subframe_source = true;
+    } else if (directive_name == kScriptSrc) {
+      seen_script_source = true;
+    } else {
+      // Keep this directive as is.
+      effective_csp_parts.push_back(directive + kDirectiveSeparator);
+      continue;
+    }
+
+    effective_csp_parts.push_back(directive_name);
+    bool seen_self_or_none = false;
+    while (tokenizer.GetNext()) {
+      std::string source_lower = base::ToLowerASCII(tokenizer.token());
+      seen_self_or_none |= source_lower == "'none'" || source_lower == "'self'";
+
+      // 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/09 15:46:01]

When we see an insecure pattern, we could add an install warning to let
developers know that it will be ignored.

[lazyboy, 2016/12/14 00:49:05]

Done.

+        effective_csp_parts.push_back(source_lower);
+      }
+    }
+
+    if (!seen_self_or_none)
+      effective_csp_parts.push_back("'self'");
+
+    // Add ";" at the end of the directive.
+    effective_csp_parts.back().append(kDirectiveSeparator);
+  }
+
+  // If subframes, scripts were not restricted in |effective_csp_parts|, add the
+  // restriction.
+  if (!seen_subframe_source && !seen_default_source)
+    effective_csp_parts.push_back(kAppSandboxSubframeSrcDefaultDirective);
+  if (!seen_script_source && !seen_default_source)
+    effective_csp_parts.push_back(kAppSandboxScriptSrcDefaultDirective);
+
+  return base::JoinString(effective_csp_parts, " ");
+}
+
 bool ContentSecurityPolicyIsSandboxed(
     const std::string& policy, Manifest::Type type) {
   // See http://www.w3.org/TR/CSP/#parse-a-csp-policy for parsing algorithm.