Disallow wildcards from matching top-level registry controlled domains during cert validation.

net/base/x509_certificate.cc

Index: net/base/x509_certificate.cc
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index 915838897e80ac7c5517c19357aa59f706969dbd..d405642ffc2d87b4ca7f61acdc822e9d45cfe589 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -27,6 +27,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/base/pem_tokenizer.h"
+#include "net/base/registry_controlled_domain.h"
 
 namespace net {
 
@@ -507,17 +508,29 @@ bool X509Certificate::VerifyHostname(
   // |reference_domain| is the remainder of |host| after the leading host
   // component is stripped off, but includes the leading dot e.g.
   // "www.f.com" -> ".f.com".
-  // If there is no meaningful domain part to |host| (e.g. it contains no dots)
-  // then |reference_domain| will be empty.
+  // If there is no meaningful domain part to |host| (e.g. it contains no
+  // dots) then |reference_domain| will be empty.
   base::StringPiece reference_host, reference_domain;
   SplitOnChar(reference_name, '.', &reference_host, &reference_domain);
   bool allow_wildcards = false;
   if (!reference_domain.empty()) {
     DCHECK(reference_domain.starts_with("."));
-    // We required at least 3 components (i.e. 2 dots) as a basic protection
-    // against too-broad wild-carding.
-    // Also we don't attempt wildcard matching on a purely numerical hostname.
-    allow_wildcards = reference_domain.rfind('.') != 0 &&
+
+    // Do not allow wildcards for registry controlled domains, so as to
+    // prevent accepting *.com or *.co.uk as valid presented names. For
+    // domains that are unknown (intranet hosts, new TLDs/gTLDs), require at
+    // least three components total - thus assuming all TLDs are

[wtc, 2011/10/21 23:44:51]

Nit: remove "total".

+    // registry-controlled.

[joth, 2011/10/21 09:26:51]

ah gotcha. Took me a little while to connect this statement with the 'true'
param and it's subtle effect: I was looking for code that counted the number of
subcomponents. Maybe just say: "Passing true so that for domains that are
unknown...."

So the consequence of this is if a company uses say .intra for their intranet,
and issue themselves *.intra certificates to their internal servers, they will
no longer be accepted. This is probably a sufficiently broken use case for us
not worry about? (I do recall my previous employer at one time more or less did
this)

[Chris Palmer, 2011/10/21 17:28:36]

Regarding *.intra: I understand this to be perfectly valid, if you resolve it
with private DNS and issue the certs with a private CA. Thus, the "must have
three components" requirement is wrong. The assertion in this patch should be,
"You can't have a wildcard directly underneath a registry-controlled domain."

We do need to keep the list of registry-controlled domains up-to-date, e.g. in
case .intra becomes a real registry-controlled TLD someday (in which case, any
company that uses it is in trouble, through no fault of their own...), but I
understand that is already a requirement for all sorts of reasons and Somebody
Is On It. I think we should/must simply trust that
RegistryControlledDomainService meets its contract.

[Ryan Sleevi, 2011/10/21 22:56:35]

joth: I mentioned this to Palmer yesterday, and is why I uploaded the two
different patchsets and sought clarification.

As it stands today, we already require the three-components, and have for a
while now on Linux (NSS, OpenSSL), and more recently, on OS X (
http://crrev.com/94827 ), and hasn't broken anyone yet.

Likewise, Firefox, by means of NSS, requires the three name components, and
seems to be fine. So I don't think this change will "break" intra - they're
already broken as it is today.

Does this explanation leave you guys comfortable with Patchset 2, or would you
rather patchset 1 - where *.infra, *.rsleevi, and *.foo are all valid?

+    size_t registry_length =
+        RegistryControlledDomainService::GetRegistryLength(reference_name,
+                                                           true);

[joth, 2011/10/21 09:26:51]

GetRegistryLength can return std::string::npos too. We should have already
bailed out up after CanonicalizeHost() if the hostname was invalid, so 
CHECK_LT(registry_length, reference_name.size()) here should be sufficient. OR
we can return false.

[Ryan Sleevi, 2011/10/21 22:56:35]

Where do you see that? It's documented to return 0, and the implementation bits
that I checked all return 0 or a value <= input.size()

[joth, 2011/10/24 09:43:39]

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/base/registry_con...

// Returns std::string::npos if the GURL is invalid or has no
// host (e.g. a file: URL). 
...
//   file:///C:/bar.html             -> std::string::npos (no host)



I think it should be 'impossible' for us to hit it here, so a CHECK seems
reasonable.

+    // Subtract 1 to account for the leading dot in |reference_domain|.
+    bool is_registry_controlled = registry_length != 0 &&
+        registry_length == (reference_domain.size() - 1);
+
+    // Additionally, do not attempt wildcard matching for purely numeric
+    // hostnames.
+    allow_wildcards = !is_registry_controlled &&
         reference_name.find_first_not_of("0123456789.") != std::string::npos;
   }