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..f2076c6ad09681d3cbe83a40c5fd93c0533abeb5 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 {
 
@@ -38,30 +39,59 @@ struct DirectiveStatus {
   bool is_secure;
 };
 
+static bool isAllowedHttpsUrl(const std::string& url) {

[not at google - send to devlin, 2014/08/19 15:05:58]

This function is inside an anonymous namespace, static unnecessary.

Also "isHttpsUrlAllowed" would read more naturally.

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

Done.

+  if (!StartsWithASCII(url, "https://", true))
+    return false;
+
+  // Length of "https://"
+  size_t start_of_host = 8;

[not at google - send to devlin, 2014/08/19 15:05:58]

How about:

const char* kHttps = "https://";
if (!StartsWithASCII(..))
  return false;

size_t start_of_host = strlen(kHttps);

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

Done.

+
+  size_t end_of_host = url.find("/", start_of_host);
+  if (end_of_host == std::string::npos)
+    end_of_host = url.size();
+
+  bool isWildcardSubdomain = end_of_host > start_of_host + 2 &&

[not at google - send to devlin, 2014/08/19 15:05:58]

is_wildcard_subdomain

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

Done.

+      url[start_of_host] == '*' && url[start_of_host + 1] == '.';
+  if (isWildcardSubdomain)
+    start_of_host += 2;

[not at google - send to devlin, 2014/08/19 15:05:58]

All of this manual URL parsing is making me nervous.

Mike any idea if we have wildcard-aware URL parsing library available on the
browser? I had a look at GURL but it escapes "*" as "%2A" which is pretty
awkward.

But maybe less awkward than doing this by hand? Of course if you were to do some
sort of un-escaping of "%2A" in the URL it would prevent legitimate uses of
"%2A" but I don't even know if that's a valid hostname anyway.

[not at google - send to devlin, 2014/08/19 15:07:21]

(We do have URLPattern but I'm not sure I trust it for parsing CSP directives).

+

[not at google - send to devlin, 2014/08/19 15:05:57]

^ I didn't notice that most of this code was basically moved from below.

+  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 (!isWildcardSubdomain)
+    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;
-      }
-    }

[not at google - send to devlin, 2014/08/19 15:05:57]

Isn't this a regression insofar as earlier we could disallow wildcard on
http/chrome as well, but now we only disallow on https? Could you make
isAllowedHttpsUrl agnostic of the scheme (like, find the first occurrence of ":"
rather than "https:") then continue to apply that check here?

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

Fixed.

http://* has never been allowed by the way.

-
     // We might need to relax this whitelist over time.
     if (source == "'self'" ||
         source == "'none'" ||
@@ -71,7 +101,7 @@ bool HasOnlySecureTokens(base::StringTokenizer& tokenizer,
         LowerCaseEqualsASCII(source, "http://localhost") ||
         StartsWithASCII(source, "http://127.0.0.1:", false) ||
         StartsWithASCII(source, "http://localhost:", false) ||
-        StartsWithASCII(source, "https://", true) ||
+        isAllowedHttpsUrl(source) ||
         StartsWithASCII(source, "chrome://", true) ||
         StartsWithASCII(source,
                         std::string(extensions::kExtensionScheme) +