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.

Review URL: http://codereview.chromium.org/9271060

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@119595 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 61 insertions, 0 deletions

diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index fdd65fd..165c3ca 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -192,8 +192,32 @@ void GetCertChainInfo(CERTCertList* cert_list,
     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) {
+        // NOTE: isRoot doesn't mean the certificate is a trust anchor.  It
+        // means the certificate is self-signed.  Here we assume isRoot only
+        // implies the certificate is self-issued.
+        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.
+        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) {
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index b7df5da..00f3cd1 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -708,6 +708,43 @@ TEST(X509CertificateTest, RejectWeakKeys) {
   TestRootCerts::GetInstance()->Clear();
 }
 
+// Test for bug 108514.
+// The certificate will expire on 2012-07-20. The test will still
+// pass if error == ERR_CERT_DATE_INVALID.  TODO(rsleevi): generate test
+// certificates for this unit test.  http://crbug.com/111730
+TEST(X509CertificateTest, ExtraneousMD5RootCert) {
+  FilePath certs_dir = GetTestCertsDirectory();
+
+  scoped_refptr<X509Certificate> server_cert =
+      ImportCertFromFile(certs_dir, "images_etrade_wallst_com.pem");
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), server_cert);
+
+  scoped_refptr<X509Certificate> intermediate_cert =
+      ImportCertFromFile(certs_dir, "globalsign_orgv1_ca.pem");
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), intermediate_cert);
+
+  scoped_refptr<X509Certificate> md5_root_cert =
+      ImportCertFromFile(certs_dir, "globalsign_root_ca_md5.pem");
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), md5_root_cert);
+
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(intermediate_cert->os_cert_handle());
+  intermediates.push_back(md5_root_cert->os_cert_handle());
+  scoped_refptr<X509Certificate> cert_chain =
+      X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
+                                        intermediates);
+
+  CertVerifyResult verify_result;
+  int flags = 0;
+  int error = cert_chain->Verify("images.etrade.wallst.com", flags, NULL,
+                                 &verify_result);
+  if (error != OK)
+    EXPECT_EQ(ERR_CERT_DATE_INVALID, error);
+
+  EXPECT_FALSE(verify_result.has_md5);
+  EXPECT_FALSE(verify_result.has_md5_ca);
+}
+
 // Test for bug 94673.
 TEST(X509CertificateTest, GoogleDigiNotarTest) {
   FilePath certs_dir = GetTestCertsDirectory();