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

net/base/x509_certificate_nss.cc

Index: net/base/x509_certificate_nss.cc
===================================================================
--- net/base/x509_certificate_nss.cc	(revision 119153)
+++ net/base/x509_certificate_nss.cc	(working copy)
@@ -194,6 +194,17 @@
     } else {
       verified_chain.push_back(node->cert);
     }
+
+    // Because of an NSS bug, CERT_PKIXVerifyCert may chain one self-signed
+    // certificate of a root CA to another self-signed certificate of the
+    // same root CA.  Detect that error and ignore the root CA certificate.
+    // See https://bugzilla.mozilla.org/show_bug.cgi?id=721288.
+    if (node->cert->isRoot && root_cert &&

[wtc, 2012/01/26 03:10:23]

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]

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]

I will add a unit test tomorrow.
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

+        SECITEM_ItemsAreEqual(&node->cert->derSubject,
+                              &root_cert->derSubject)) {
+      continue;
+    }
+
     SECAlgorithmID& signature = node->cert->signature;
     SECOidTag oid_tag = SECOID_FindOIDTag(&signature.algorithm);
     switch (oid_tag) {