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

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

BUG=100442
TEST=net_unittests:X509CertificateNameVerifyTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107075

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107679

[Ryan Sleevi, 2011-10-21 01:19:02]

wtc: For review.

palmer, joth: FYI.

Patchset 1 adopts a looser standard - unknown/unrecognized TLDs are allowed to
have wildcards, but registry controlled domains are not.

Patchset 2 matches the current minimum requirements (at least three components),
but also adds requirements for additional components if a registry-controlled
domain.

Just wanted to make sure that we're in agreement that Patchset 2 is the desired
approach.

wtc, palmer: This change will only affect [Linux, Mac, CrOS]. Currently Win
doesn't use our routines, but uses the CryptoAPI ones. We can adopt Win to do
name checking with these routines, similar to what we did on Mac, but just FYI.

[joth, 2011-10-21 09:26:50]

Thanks for taking this on!

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:523: // registry-controlled.
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)

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:526: true);
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.

[Chris Palmer, 2011-10-21 17:28:35]

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:523: // registry-controlled.
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]

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:523: // registry-controlled.
On 2011/10/21 17:28:36, Chris Palmer wrote:
> 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.

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?

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:526: true);
On 2011/10/21 09:26:51, joth wrote:
> 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.

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()

[palmer, 2011-10-21 23:20:15]

> 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.

Hasn't broken anyone *that we know of* yet. That surprises me, but hey, maybe
it's good. :)

> 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.

Therefore I recommend we continue with that policy, and we find we need to allow
*.intra someday, we can (while still checking with
RegistryControlledDomainService).

Adding cevans and jschuh for their security feelings, if any.

[Ryan Sleevi, 2011-10-21 23:25:00]

On 2011/10/21 23:20:15, Chris P. wrote:
> > 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.
> 
> Hasn't broken anyone *that we know of* yet. That surprises me, but hey, maybe
> it's good. :)
> 
> > 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.
> 
> Therefore I recommend we continue with that policy, and we find we need to
allow
> *.intra someday, we can (while still checking with
> RegistryControlledDomainService).
> 
> Adding cevans and jschuh for their security feelings, if any.

Right, the idea with patchset two is to treat any unknown names as registry
controlled. This fail-close behaviour at least guarantees that if, for some
reason, the RCDS is out of date, we're still "mostly" protected against
negligent/idiotic CAs (is that too redundant? ;-P).

Plus, who really would use *.infra? I mean, one certificate to rule your entire
network? Lord help you protecting that private key from misuse....

[wtc, 2011-10-21 23:44:50]

Patch Set 2 LGTM.

It's better to start strict and loosen later.

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:522: // least three components total - thus
assuming all TLDs are

Nit: remove "total".

[Chris Palmer, 2011-10-21 23:57:40]

Patch Set 2 LGTM also.

[Ryan Sleevi, 2011-10-22 02:55:35]

joth, wtc: Updated the comment. Will land tomorrow if I don't hear any nits.

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

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8362023/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:526: true);
On 2011/10/21 22:56:35, Ryan Sleevi wrote:
> On 2011/10/21 09:26:51, joth wrote:
> > 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.
> 
> Where do you see that?

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.

[commit-bot: I haz the power, 2011-10-25 05:50:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/8362023/14001

[commit-bot: I haz the power, 2011-10-25 07:05:10]

Change committed as 107075

[achuithb, 2011-10-25 08:42:52]

ui tests on mac fail with this change:
http://chromegw.corp.google.com/i/chromium/builders/Mac%2010.6%20Tests%20%28d...

[wtc, 2011-10-25 20:29:09]

Patch Set 4 LGTM.

[Ryan Sleevi, 2011-10-25 22:58:38]

This had to be reverted, most likely because RegistryControlledDomainService is
a Singleton, while Verify() happens on a non-joinable thread.

I'm surprised only OS X in debug tripped this, and that none of the trybots
failed.

As far as I can tell, it's long been unnecessary for RCDS to be a Singleton (see
http://crrev.com/35196), so I'll work on fixing/friending the unittests and
removing it's Singleton-ness.

[wtc, 2011-10-26 02:09:29]

On 2011/10/25 22:58:38, Ryan Sleevi wrote:
> This had to be reverted, most likely because RegistryControlledDomainService
is
> a Singleton, while Verify() happens on a non-joinable thread.

Sorry to hear that.  Let me know when you have a CL ready.  Thanks.

[Ryan Sleevi, 2011-10-26 02:10:01]

On 2011/10/26 02:09:29, wtc wrote:
> On 2011/10/25 22:58:38, Ryan Sleevi wrote:
> > This had to be reverted, most likely because RegistryControlledDomainService
> is
> > a Singleton, while Verify() happens on a non-joinable thread.
> 
> Sorry to hear that.  Let me know when you have a CL ready.  Thanks.

Vandebo already reviewed - getting ready to land
http://codereview.chromium.org/8395025/

[commit-bot: I haz the power, 2011-10-27 20:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/8362023/14001

[commit-bot: I haz the power, 2011-10-27 21:34:16]

Try job failure for 8362023-14001 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-10-27 21:37:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/8362023/14001

[commit-bot: I haz the power, 2011-10-27 22:28:39]

Try job failure for 8362023-14001 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-10-28 01:01:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/8362023/14001

[commit-bot: I haz the power, 2011-10-28 02:32:12]

Change committed as 107679