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)
@@ -192,8 +192,30 @@
     if (i == 0) {
       verified_cert = node->cert;
     } else {
+      // Because of an NSS bug, CERT_PKIXVerifyCert may chain a self-signed
+      // certificate of a root CA to another certificate of the same root CA
+      // key.  Detect that error and ignore the root CA certificate.
+      // See https://bugzilla.mozilla.org/show_bug.cgi?id=721288.
+      if (node->cert->isRoot) {

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

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)

+        CERTCertListNode* next_node = CERT_LIST_NEXT(node);
+        CERTCertificate* next_cert;
+        if (!CERT_LIST_END(next_node, cert_list)) {
+          next_cert = next_node->cert;
+        } else {
+          next_cert = root_cert;
+        }
+        // This SECITEM_ItemsAreEqual test should be unnecessary, implied
+        // by node->cert->isRoot and the fact that NSS has verified the
+        // signature of node->cert with the public key of next_cert.  But I
+        // added the test to make it easier to prove the code is correct.

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

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.

+        if (next_cert && SECITEM_ItemsAreEqual(&node->cert->derPublicKey,
+                                               &next_cert->derPublicKey)) {
+          continue;
+        }
+      }
       verified_chain.push_back(node->cert);
     }
+
     SECAlgorithmID& signature = node->cert->signature;
     SECOidTag oid_tag = SECOID_FindOIDTag(&signature.algorithm);
     switch (oid_tag) {