Disallow non-subdomain wildcards in the extension's CSP

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 d19c7f27ef803c519421858fa31f4171a208b5c4..3e60c459fc919c548ca93d5f9b717f7f04c38400 100644
--- a/extensions/common/csp_validator.cc
+++ b/extensions/common/csp_validator.cc
@@ -11,6 +11,7 @@
 #include "base/strings/string_util.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 
 namespace extensions {
 
@@ -26,6 +27,8 @@ const char kSandboxDirectiveName[] = "sandbox";
 const char kAllowSameOriginToken[] = "allow-same-origin";
 const char kAllowTopNavigation[] = "allow-top-navigation";
 
+const char kHttpSchemeAndSeparator[] = "https://";
+
 struct DirectiveStatus {
   explicit DirectiveStatus(const char* name)
     : directive_name(name)
@@ -38,30 +41,80 @@ struct DirectiveStatus {
   bool is_secure;
 };
 
+bool isNonWildcardScheme(const std::string& url,
+                         const std::string& scheme_and_separator) {
+  if (!StartsWithASCII(url, scheme_and_separator, true))
+    return false;
+
+  // Disallow host-less schemes such as "https://".
+  if (url == scheme_and_separator)
+    return false;
+
+  const size_t scheme_and_separator_len = scheme_and_separator.length();
+  // 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/CSP11/#source-list-syntax

[Mike West, 2014/08/19 16:53:07]

Nit: Can you change this link to the editor's draft[1] (or the latest WD[2]).

[1]: https://w3c.github.io/webappsec/specs/content-security-policy/
[2]: http://www.w3.org/TR/CSP2/

+  if (url[scheme_and_separator_len] != '*')
+    return true;
+
+  // If a wildcard is set, then it must be followed by a dot to qualify as
+  // a subdomain wildcard.
+  return url.length() > scheme_and_separator_len &&
+      url[scheme_and_separator_len + 1] == '.';
+}
+
+bool isHttpsUrlAllowed(const std::string& url) {
+  if (!StartsWithASCII(url, kHttpSchemeAndSeparator, true))
+    return false;
+
+  size_t start_of_host = strlen(kHttpSchemeAndSeparator);
+
+  size_t end_of_host = url.find("/", start_of_host);
+  if (end_of_host == std::string::npos)
+    end_of_host = url.size();
+
+  bool is_wildcard_subdomain = end_of_host > start_of_host + 2 &&
+      url[start_of_host] == '*' && url[start_of_host + 1] == '.';
+  if (is_wildcard_subdomain)
+    start_of_host += 2;
+
+  size_t start_of_port = url.rfind(":", end_of_host);
+  if (start_of_port != std::string::npos && start_of_port > start_of_host) {
+    bool is_valid_port = false;
+    // Do a quick sanity check. The following check could mistakenly flag
+    // ":123456" or ":****" as valid, but that does not matter because the
+    // relaxing CSP directive will just be ignored by Blink.
+    for (size_t i = start_of_port + 1; i < end_of_host; ++i) {
+      is_valid_port = IsAsciiDigit(url[i]) || url[i] == '*';
+      if (!is_valid_port)
+        break;
+    }
+    if (is_valid_port)
+      end_of_host = start_of_port;
+  }
+
+  std::string host(url, start_of_host, end_of_host - start_of_host);
+  // Global wildcards are not allowed.
+  if (host.empty() || host.find("*") != std::string::npos)
+    return false;
+
+  if (!is_wildcard_subdomain)
+    return true;
+
+  // Wildcards on subdomains of a TLD are not allowed.
+  size_t registry_length = net::registry_controlled_domains::GetRegistryLength(
+      host,
+      net::registry_controlled_domains::INCLUDE_UNKNOWN_REGISTRIES,
+      net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+  return registry_length != 0;
+}
+
 bool HasOnlySecureTokens(base::StringTokenizer& tokenizer,
                          Manifest::Type type) {
   while (tokenizer.GetNext()) {
     std::string source = tokenizer.token();
     base::StringToLowerASCII(&source);
 
-    // Don't alow whitelisting of all hosts. This boils down to:
-    //   1. Maximum of 2 '*' characters.
-    //   2. Each '*' is either followed by a '.' or preceded by a ':'
-    int wildcards = 0;
-    size_t length = source.length();
-    for (size_t i = 0; i < length; ++i) {
-      if (source[i] == L'*') {
-        wildcards++;
-        if (wildcards > 2)
-          return false;
-
-        bool isWildcardPort = i > 0 && source[i - 1] == L':';
-        bool isWildcardSubdomain = i + 1 < length && source[i + 1] == L'.';
-        if (!isWildcardPort && !isWildcardSubdomain)
-          return false;
-      }
-    }
-
     // We might need to relax this whitelist over time.
     if (source == "'self'" ||
         source == "'none'" ||
@@ -69,14 +122,13 @@ bool HasOnlySecureTokens(base::StringTokenizer& tokenizer,
         LowerCaseEqualsASCII(source, "blob:") ||
         LowerCaseEqualsASCII(source, "filesystem:") ||
         LowerCaseEqualsASCII(source, "http://localhost") ||
-        StartsWithASCII(source, "http://127.0.0.1:", false) ||
-        StartsWithASCII(source, "http://localhost:", false) ||
-        StartsWithASCII(source, "https://", true) ||
-        StartsWithASCII(source, "chrome://", true) ||
-        StartsWithASCII(source,
-                        std::string(extensions::kExtensionScheme) +
-                            url::kStandardSchemeSeparator,
-                        true) ||
+        StartsWithASCII(source, "http://127.0.0.1:", true) ||
+        StartsWithASCII(source, "http://localhost:", true) ||

[robwu, 2014/08/19 16:31:39]

Drive-by fix. source has already been transformed to lowercase a few lines
before, so a case-sensitive comparison is sufficient.

+        isHttpsUrlAllowed(source) ||
+        isNonWildcardScheme(source, "chrome://") ||
+        isNonWildcardScheme(source,
+                            std::string(extensions::kExtensionScheme) +
+                            url::kStandardSchemeSeparator) ||
         StartsWithASCII(source, "chrome-extension-resource:", true)) {
       continue;
     }