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

net/base/x509_certificate_nss.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/x509_certificate.h"
 
 #include <cert.h>
 #include <cryptohi.h>
 #include <keyhi.h>
 #include <nss.h>
@@ skipping 174 matching lines @@
 
   CERTCertificate* verified_cert = NULL;
   std::vector<CERTCertificate*> verified_chain;
   int i = 0;
   for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
        !CERT_LIST_END(node, cert_list);
        node = CERT_LIST_NEXT(node), ++i) {
     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.
+      //
+      // NOTE: isRoot doesn't mean the certificate is a trust anchor.  It
+      // means the certificate is self-signed.

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

nit: self-issued?

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

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

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.

+      if (node->cert->isRoot) {
+        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;
+        }
+        // Test that |node->cert| is actually a self-signed certificate
+        // whose key is equal to |next_cert|, and not a self-issued
+        // certificate signed by another key of the same CA.
+        //
+        // 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.
+        if (next_cert && SECITEM_ItemsAreEqual(&node->cert->derPublicKey,

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

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.

+                                               &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) {
       case SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION:
         verify_result->has_md5 = true;
         if (i != 0)
           verify_result->has_md5_ca = true;
         break;
       case SEC_OID_PKCS1_MD2_WITH_RSA_ENCRYPTION:
         verify_result->has_md2 = true;
@@ skipping 962 matching lines @@
       *type = kPublicKeyTypeECDSA;
       break;
     default:
       *type = kPublicKeyTypeUnknown;
       *size_bits = 0;
       break;
   }
 }
 
 }  // namespace net