Check cert->isRoot to skip extraneous root certificates in certificate

Check cert->isRoot to skip extraneous root certificates in certificate
chains.

NSS bug 721288 causes CERT_PKIXVerifyCert to continue extending the
certificate chain after it has reached a root certificate.  Detect that
bug and ignore such extraneous root certificates in certificate chains
when checking for weak signature algorithms.

R=rsleevi@chromium.org
BUG=108514
TEST=a new unit test (to be added) that uses the certificate chain sent
by https://images.etrade.wallst.com/ during SSL handshake.

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

[wtc, 2012-01-26 03:10:22]

I will add the unit test tomorrow.  In the interest of
time, I'd like you to review the fix first.

https://chromiumcodereview.appspot.com/9271060/diff/1/net/base/x509_certifica...
File net/base/x509_certificate_nss.cc (right):

https://chromiumcodereview.appspot.com/9271060/diff/1/net/base/x509_certifica...
net/base/x509_certificate_nss.cc:202: if (node->cert->isRoot && root_cert &&

The isRoot member of the NSS CERTCertificate structure is
initialized here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...

It uses the cert_IsRootCert function, which does exactly
what I was planning to check to work around the NSS bug:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...

So the solution turned out to be much simpler than I expected.
For extra assurance, I also check that node->cert->derSubject
and root_cert->derSubject are the same.

[Ryan Sleevi, 2012-01-26 04:36:27]

https://chromiumcodereview.appspot.com/9271060/diff/1/net/base/x509_certifica...
File net/base/x509_certificate_nss.cc (right):

https://chromiumcodereview.appspot.com/9271060/diff/1/net/base/x509_certifica...
net/base/x509_certificate_nss.cc:202: if (node->cert->isRoot && root_cert &&
On 2012/01/26 03:10:23, wtc wrote:
> 
> The isRoot member of the NSS CERTCertificate structure is
> initialized here:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...
> 
> It uses the cert_IsRootCert function, which does exactly
> what I was planning to check to work around the NSS bug:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...
> 
> So the solution turned out to be much simpler than I expected.
> For extra assurance, I also check that node->cert->derSubject
> and root_cert->derSubject are the same.

I think I'd be interested in seeing a unit test. In the case of this particular
chain, my understanding was that node->cert->isRoot is false, because the MD5
version is /not/ in the trusted root store. This is why NSS keeps continuing to
build the chain, ultimately terminating in root_cert.

It also fails to take into consideration DER name canonicalization issues, which
libpkix /does/ handle properly.

What do you think of this alternative implementation:

std::vector<CERTCertificate*> returned_chain;
for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
     !CERT_LIST_END(node, cert_list);
     node = CERT_LIST_NEXT(node)) {
  returned_chain.push_back(node->cert);
}
if (root_cert)
  returned_chain.push_back(root_cert);

std::vector<CERTCertificate*> verified_chain;
for (size_t j = 0; j < returned_chain.size() - 1; ++j) {
  if (CERT_CompareName(&returned_chain[j]->subject,
                       &returned_chain[j+1]->subject) &&
      SECITEM_ItemsAreEqual(&returned_chain[j]->derPublicKey,
                            &returned_chain[j+1]->derPublicKey)) {
    // Same subject, same public key. Assume NSS was being
    // stupid here. If the chain was successfully validated
    // when both j and j + 1 are present, it should be valid
    // with just j + 1 present, since j + 1 would be a valid
    // signer for j - 1 (j + 1 will always be /less/
    // constrained than j, in PKIX terms, so it's always
    // as-valid-or-moreso).
    continue;
  }
  verified_chain.push_back(returned_chain[i]);
}
if (root_cert)
  verified_chain.pop_back();

// Now loop over verified_chain to examine signatures.



With the above logic, we should see that the MD5-root-but-not subject is the
same as the SHA1-true-root's, and the MD5-root-but-not's SPKI is the same as the
SHA1-true-root's, so the MD5-root-but-not is pruned from |verified_chain|.

This should case false positives for the deprecated algorithm code, and I
believe was correct. I'd be happy to reason with you about this in person, since
it's still something I'm a little nervous with, but less nervous than the
current change.

[wtc, 2012-01-27 02:54:04]

rsleevi: thanks for your comments.  Patch Set 2 implements
your suggestion.

I am relying on node->cert->isRoot to skip the new code
in common cases.  I omitted the
      CERT_CompareName(&returned_chain[j]->subject,
                       &returned_chain[j+1]->subject)
check you suggested because it is implied by
node->cert->isRoot (which ensures returned_chain[j]->subject
and returned_chain[j]->issuer are the same, see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...
)
and the certificate name chaining check performed by NSS.

http://codereview.chromium.org/9271060/diff/1/net/base/x509_certificate_nss.cc
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/9271060/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:202: if (node->cert->isRoot && root_cert &&
On 2012/01/26 04:36:27, Ryan Sleevi wrote:
>
> I think I'd be interested in seeing a unit test.

I will add a unit test tomorrow.

> In the case of this particular
> chain, my understanding was that node->cert->isRoot is false, because the MD5
> version is /not/ in the trusted root store.

No, node->cert->isRoot is true.

NSS does not determine cert->isRoot based on whether the
cert is in the trusted root store.  NSS determines
cert->isRoot entirely by inspecting the cert's contents:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...

http://mxr.mozilla.org/security/ident?i=cert_IsRootCert

[Ryan Sleevi, 2012-01-27 02:59:21]

Functionally LGTM, two comment nits below.

http://codereview.chromium.org/9271060/diff/7001/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/9271060/diff/7001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:199: if (node->cert->isRoot) {
Might be worth adding a comment here that isRoot only compares that subject &
issuer are equal, not that it's self-signed nor that it's a trust anchor. This
seems a tricky piece to get right (as evidenced by the fact that I still missed
it, even with the code pointers you provided)

http://codereview.chromium.org/9271060/diff/7001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:210: // added the test to make it easier to
prove the code is correct.
nit: Drop the last sentence?

Would it be worth explaining why

// Test that |node->cert| is actually a self-signed certificate
// whose key is equal to |next_cert|, and not a cross-signed
// cert whose subject/issuer names match.

[wtc, 2012-01-27 21:09:08]

sleevi: Thank you for your comments.  Please review Patch
Set 3.

I added the comments you suggested, except that I use the
terms "self-signed", "self-issued", and "cross-certificates"
strictly as defined in PKIX.

I also added the unit test I promised.

[Ryan Sleevi, 2012-01-27 21:45:37]

Great. Like we discussed, I think this is fine/correct - tests look good, so I
hope they pass!

A few comments below, but nothing requiring re-review, as long as trybots are
happy.

Note, I don't have net/ OWNERs yet, so you'll need to TBR it someone else (or
use your OWNERS power to grant me OWNERS :-p)

http://codereview.chromium.org/9271060/diff/9002/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/9271060/diff/9002/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:201: // means the certificate is self-signed.
nit: self-issued?

This is to match comments in line 211, and that isRoot doesn't perform a sig
check (AFAICT)

http://codereview.chromium.org/9271060/diff/9002/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:217: if (next_cert &&
SECITEM_ItemsAreEqual(&node->cert->derPublicKey,
This isn't for checking the signature of node->cert

This is checking the signature of [node - 1]->cert with the key of [node + 1],
by ensuring that the key of node->cert is the same key of [node + 1]->cert.

http://codereview.chromium.org/9271060/diff/9002/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/9271060/diff/9002/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:731: int error =
cert_chain->Verify("images.etrade.wallst.com", flags, NULL,
Can you file a bug and assign to me, and TODO here, to generate some internal
certs for this, so that we're not tied to real certs and their expiration dates?

It shouldn't take long to whip up something.

[wtc, 2012-01-27 22:58:13]

rsleevi: Patch Set 4 addressed your review comments.  I also
verified the new unit test will still pass after certificate
expiration by modifying PR_Now to return a future time.

http://codereview.chromium.org/9271060/diff/9002/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/9271060/diff/9002/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:201: // means the certificate is self-signed.

On 2012/01/27 21:45:37, Ryan Sleevi wrote:
> nit: self-issued?

isRoot means self-signed as defined in PKIX (RFC 5280).

isRoot checks two things:
1. The subject and issuer fields are the same.  This is the
definition of "self-issued" in PKIX.
2. The cert is signed by the public key in the cert.  If a
self-issued cert also has this property, it is called
self-signed in PKIX.

I think you do not trust how NSS tests property 2.  So I've
updated this comment to point out that we assume isRoot
only implies self-issued, and removed the extra comment below.

[commit-bot: I haz the power, 2012-01-28 00:55:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/9271060/13001

[commit-bot: I haz the power, 2012-01-28 01:26:05]

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

[commit-bot: I haz the power, 2012-01-28 02:10:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/9271060/13001

[commit-bot: I haz the power, 2012-01-28 08:42:21]

Change committed as 119595