Perform CRLSet evaluation during Path Building on Windows

net/cert/cert_verify_proc_unittest.cc

Index: net/cert/cert_verify_proc_unittest.cc
diff --git a/net/cert/cert_verify_proc_unittest.cc b/net/cert/cert_verify_proc_unittest.cc
index bdeec339d02cf1d75a7037d6f21392027bab3aad..4f88f1c9331918eaf9cf9f3a3cafc7291311c756 100644
--- a/net/cert/cert_verify_proc_unittest.cc
+++ b/net/cert/cert_verify_proc_unittest.cc
@@ -1360,6 +1360,309 @@ TEST_F(CertVerifyProcTest, CRLSetLeafSerial) {
                  &verify_result);
   EXPECT_EQ(ERR_CERT_REVOKED, error);
 }
+
+// Test that when multiple paths exist for a certificate to be verified, and
+// one of them is revoked by a CRLSet, that the alternate and still valid
+// paths are considered.
+// This tests when the leaf (A) is issued by B, which is issued by C. Two
+// different versions of C exist, but both versions are signed by a trust
+// anchor (D and F, respectively).
+TEST_F(CertVerifyProcTest, CRLSetRevokedIntermediateSameName) {
+  // Load the supplemental certificates for path building (B, C) into memory.
+  CertificateList ca_cert_list = CreateCertificateListFromFile(
+      GetTestCertsDirectory(), "multi-root-chain1.pem",

[davidben, 2016/01/21 02:37:39]

Seeing as the tests themselves disassemble the pre-made chain files anyway, it
seems better to put each individual certificate in its own file so I don't have
to go back and check that multi-root-chain1's 3rd element is indeed
such-and-such. It's bad enough that I have to look back at my notes to see what
A, B, etc., are.

+      X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(4U, ca_cert_list.size());
+
+  // Add the "D" test root.
+  ScopedTestRoot test_root(ca_cert_list[3].get());
+
+  CertificateList cert_list = CreateCertificateListFromFile(
+      GetTestCertsDirectory(), "multi-root-chain3.pem",
+      X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(4U, cert_list.size());
+
+  // Add the "F" test root.
+  ScopedTestRoot second_test_root(cert_list[3].get());
+
+  // Create a certificate chain that sends
+  // A -> B -> C3 -> C
+  // This is way of ensuring that both versions of C are supplied as
+  // supplemental certificates to be verified. The
+  // CertVerifyProcTest.VerifyReturnChainFiltersUnrelatedCerts test ensures that
+  // these should not negatively impact chain building, as they would otherwise
+  // be treated as 'ignorable' certificates.
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(cert_list[1]->os_cert_handle());
+  intermediates.push_back(cert_list[2]->os_cert_handle());
+  intermediates.push_back(ca_cert_list[2]->os_cert_handle());
+  scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
+      cert_list[0]->os_cert_handle(), intermediates);
+  ASSERT_TRUE(cert.get());
+
+  // Verify that, without any revocation, things verify successfully and
+  // prefer the newer (to be revoked) certificate.
+  int flags = 0;
+  CertVerifyResult verify_result;
+  int error = Verify(cert.get(), "127.0.0.1", flags, NULL, empty_cert_list_,
+                     &verify_result);
+  ASSERT_EQ(OK, error);
+  ASSERT_EQ(0U, verify_result.cert_status);
+  ASSERT_TRUE(verify_result.verified_cert.get());
+
+  // Ensure the first / newest chain provided is preferred - as this is the
+  // chain that will subsequently be revoked.
+  // The expected path is A -> B -> C3 -> F
+  const X509Certificate::OSCertHandles& verified_intermediates =
+      verify_result.verified_cert->GetIntermediateCertificates();
+  ASSERT_EQ(3U, verified_intermediates.size());
+  scoped_refptr<X509Certificate> verified_root =
+      X509Certificate::CreateFromHandle(verified_intermediates[2],
+                                        X509Certificate::OSCertHandles());
+  ASSERT_TRUE(verified_root.get());
+  EXPECT_EQ("F Root CA", verified_root->subject().common_name);

[davidben, 2016/01/21 02:37:39]

It seems these tests could do with a few helper functions to make them less
long. It's a bit difficult to read through them all. (In comparison, the tests
we recently added for alt-chains in BoringSSL are a lot easier to read.
https://boringssl.googlesource.com/boringssl/+/master/crypto/x509/x509_test.c...
)

[Ryan Sleevi, 2016/01/21 02:54:04]

This is strongly discouraged by our internal testing style guide. You can find
it on the TOTT archives ;)

+
+  // Now test by blocking the C3-by-F intermediate, which should result
+  // in a path of A -> B -> C -> D
+  scoped_refptr<CRLSet> crl_set;
+  std::string crl_set_bytes;
+  EXPECT_TRUE(base::ReadFileToString(
+      GetTestCertsDirectory().AppendASCII("multi-root-crlset-C3.raw"),
+      &crl_set_bytes));
+  ASSERT_TRUE(CRLSetStorage::Parse(crl_set_bytes, &crl_set));
+
+  error = Verify(cert.get(), "127.0.0.1", flags, crl_set.get(),
+                 empty_cert_list_, &verify_result);
+  ASSERT_EQ(OK, error);
+  ASSERT_EQ(0U, verify_result.cert_status);
+  ASSERT_TRUE(verify_result.verified_cert.get());
+
+  const X509Certificate::OSCertHandles& new_verified_intermediates =
+      verify_result.verified_cert->GetIntermediateCertificates();
+  ASSERT_EQ(3U, new_verified_intermediates.size());
+  verified_root = X509Certificate::CreateFromHandle(
+      new_verified_intermediates[2], X509Certificate::OSCertHandles());
+  ASSERT_TRUE(verified_root.get());
+  EXPECT_EQ("D Root CA", verified_root->subject().common_name);
+
+  // Now reverify the cert without the CRLSet, to ensure that the CRLSet does
+  // not persist in between independent calls.
+  error = Verify(cert.get(), "127.0.0.1", flags, NULL, empty_cert_list_,
+                 &verify_result);
+  ASSERT_EQ(OK, error);
+  ASSERT_EQ(0U, verify_result.cert_status);
+  ASSERT_TRUE(verify_result.verified_cert.get());
+
+  // The expected path is A -> B -> C3 -> F
+  const X509Certificate::OSCertHandles& final_verified_intermediates =
+      verify_result.verified_cert->GetIntermediateCertificates();
+  ASSERT_EQ(3U, final_verified_intermediates.size());
+  verified_root = X509Certificate::CreateFromHandle(
+      final_verified_intermediates[2], X509Certificate::OSCertHandles());
+  ASSERT_TRUE(verified_root.get());
+  EXPECT_EQ("F Root CA", verified_root->subject().common_name);
+}
+
+// Test that when multiple paths exist for a certificate to be verified, and
+// one of them is revoked by a CRLSet, that the alternate and still valid
+// paths are considered.
+// This tests when the leaf (A) is issued by B. Two versions of B
+// exist - one that goes to G and one that goes to C. Both G and C
+// are signed by trust anchors (F and D/E/F, respectively). This tests
+// the ability of path building to explore an edge where the immediate
+// certificate is not revoked, but the issuer is (in this case, G) - and thus
+// needs to backtrack to find the alternative path (B -> C -> D)
+TEST_F(CertVerifyProcTest, CRLSetRevokedIntermediateCrossIntermediates) {
+  // Load the supplemental certificates for path building (B, C) into memory.
+  CertificateList ca_cert_list = CreateCertificateListFromFile(
+      GetTestCertsDirectory(), "multi-root-chain1.pem",
+      X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(4U, ca_cert_list.size());
+
+  // Add the "D" test root.
+  ScopedTestRoot test_root(ca_cert_list[3].get());
+
+  CertificateList cert_list = CreateCertificateListFromFile(
+      GetTestCertsDirectory(), "multi-root-chain4.pem",
+      X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(4U, cert_list.size());
+
+  // Add the "F" test root.
+  ScopedTestRoot second_test_root(cert_list[3].get());
+
+  // Create a certificate chain that sends
+  // A -> B2 -> G -> B -> C
+  // This is way of ensuring that both B and C are supplied as supplemental
+  // certificates to be verified. The
+  // CertVerifyProcTest.VerifyReturnChainFiltersUnrelatedCerts test ensures that
+  // these should not negatively impact chain building, as they would otherwise
+  // be treated as 'ignorable' certificates.
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(cert_list[1]->os_cert_handle());
+  intermediates.push_back(cert_list[2]->os_cert_handle());
+  intermediates.push_back(ca_cert_list[1]->os_cert_handle());
+  intermediates.push_back(ca_cert_list[2]->os_cert_handle());
+  scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
+      cert_list[0]->os_cert_handle(), intermediates);
+  ASSERT_TRUE(cert.get());
+
+  // Verify that, without any revocation, things verify successfully and
+  // prefer the newer (to be revoked) certificate.
+  int flags = 0;
+  CertVerifyResult verify_result;
+  int error = Verify(cert.get(), "127.0.0.1", flags, NULL, empty_cert_list_,
+                     &verify_result);
+  ASSERT_EQ(OK, error);
+  ASSERT_EQ(0U, verify_result.cert_status);
+  ASSERT_TRUE(verify_result.verified_cert.get());
+
+  // Ensure the first / newest chain provided is preferred - as this is the
+  // chain that will subsequently be revoked.
+  // The expected path is A -> B2 -> G -> F
+  const X509Certificate::OSCertHandles& verified_intermediates =
+      verify_result.verified_cert->GetIntermediateCertificates();
+  ASSERT_EQ(3U, verified_intermediates.size());
+  scoped_refptr<X509Certificate> verified_root =
+      X509Certificate::CreateFromHandle(verified_intermediates[2],
+                                        X509Certificate::OSCertHandles());
+  ASSERT_TRUE(verified_root.get());
+  EXPECT_EQ("F Root CA", verified_root->subject().common_name);
+  scoped_refptr<X509Certificate> verified_intermediate =
+      X509Certificate::CreateFromHandle(verified_intermediates[1],
+                                        X509Certificate::OSCertHandles());
+  ASSERT_TRUE(verified_intermediate.get());
+  EXPECT_EQ("G CA", verified_intermediate->subject().common_name);
+
+  // Now test by blocking the G-by-F intermediate, which should result
+  // in a path of A -> B -> C -> D
+  scoped_refptr<CRLSet> crl_set;
+  std::string crl_set_bytes;
+  EXPECT_TRUE(base::ReadFileToString(
+      GetTestCertsDirectory().AppendASCII("multi-root-crlset-G.raw"),
+      &crl_set_bytes));
+  ASSERT_TRUE(CRLSetStorage::Parse(crl_set_bytes, &crl_set));
+
+  error = Verify(cert.get(), "127.0.0.1", flags, crl_set.get(),
+                 empty_cert_list_, &verify_result);
+  ASSERT_EQ(OK, error);
+  ASSERT_EQ(0U, verify_result.cert_status);
+  ASSERT_TRUE(verify_result.verified_cert.get());
+
+  const X509Certificate::OSCertHandles& new_verified_intermediates =
+      verify_result.verified_cert->GetIntermediateCertificates();
+  ASSERT_EQ(3U, new_verified_intermediates.size());
+  verified_root = X509Certificate::CreateFromHandle(
+      new_verified_intermediates[2], X509Certificate::OSCertHandles());
+  ASSERT_TRUE(verified_root.get());
+  EXPECT_EQ("D Root CA", verified_root->subject().common_name);
+
+  // Now reverify the cert without the CRLSet, to ensure that the CRLSet does
+  // not persist in between independent calls.
+  error = Verify(cert.get(), "127.0.0.1", flags, NULL, empty_cert_list_,
+                 &verify_result);
+  ASSERT_EQ(OK, error);
+  ASSERT_EQ(0U, verify_result.cert_status);
+  ASSERT_TRUE(verify_result.verified_cert.get());
+
+  const X509Certificate::OSCertHandles& final_verified_intermediates =
+      verify_result.verified_cert->GetIntermediateCertificates();
+  ASSERT_EQ(3U, final_verified_intermediates.size());
+  verified_root = X509Certificate::CreateFromHandle(
+      final_verified_intermediates[2], X509Certificate::OSCertHandles());
+  ASSERT_TRUE(verified_root.get());
+  EXPECT_EQ("F Root CA", verified_root->subject().common_name);
+}
+
+// Test that when multiple paths exist for a certificate to be verified, and
+// one of them is revoked by a CRLSet, that the alternate and still valid
+// paths are considered.
+// This tests a variety of situations in which a CA has cyclicly certified
+// the certificates (e.g. Root J signed Root K, and Root K signed Root J).
+// When both of these roots are present in the trust store (J and K), but
+// one is revoked (K), paths should build and terminate in J.
+// This tests when the leaf (A) is issued by B. Two versions of B
+// exist - one that goes to G and one that goes to C. Both G and C
+// are signed by trust anchors (F and D/E/F, respectively). This tests
+// the ability of path building to explore an edge where the immediate
+// certificate is not revoked, but the issuer is (in this case, G) - and thus
+// needs to backtrack to find the alternative path (B -> C -> D)
+TEST_F(CertVerifyProcTest, CRLSetRevokedCyclicPathBuilding) {
+  // Load the supplemental certificates for path building (B, C) into memory.
+  CertificateList ca_cert_list = CreateCertificateListFromFile(
+      GetTestCertsDirectory(), "multi-root-chain5.pem",
+      X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(3U, ca_cert_list.size());
+
+  struct TestData {
+    // The name of the chain file to load.
+    const char* const filename;
+    // The expected length of the (parsed, validated) chain.
+    const size_t chain_length;
+  } kTestChains[] = {
+      {"multi-root-chain6.pem", 4},
+      {"multi-root-chain7.pem", 5},
+      {"multi-root-chain8.pem", 6},
+  };
+  for (const auto& test_case : kTestChains) {
+    SCOPED_TRACE(test_case.filename);
+
+    CertificateList cert_list = CreateCertificateListFromFile(
+        GetTestCertsDirectory(), test_case.filename,
+        X509Certificate::FORMAT_AUTO);
+    ASSERT_EQ(test_case.chain_length, cert_list.size());
+
+    // Add the "J" test root.
+    ScopedTestRoot test_root(ca_cert_list[2].get());
+
+    // Add the chain root as a test root
+    ScopedTestRoot second_test_root(cert_list.back().get());
+
+    // Create a certificate chain that sends everything but the last
+    // certificate.
+    cert_list.pop_back();
+    X509Certificate::OSCertHandles intermediates;
+    for (size_t i = 1; i < cert_list.size(); ++i) {
+      intermediates.push_back(cert_list[i]->os_cert_handle());
+    }
+    scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
+        cert_list[0]->os_cert_handle(), intermediates);
+    ASSERT_TRUE(cert.get());
+
+    // Verify that, without any revocation, the certificate verifies
+    // successfully.
+    int flags = 0;
+    CertVerifyResult verify_result;
+    int error = Verify(cert.get(), "127.0.0.1", flags, NULL, empty_cert_list_,
+                       &verify_result);
+    ASSERT_EQ(OK, error);
+    ASSERT_EQ(0U, verify_result.cert_status);
+    ASSERT_TRUE(verify_result.verified_cert.get());
+
+    // Now test by blocking K, which should result in a path of H -> I -> J
+    scoped_refptr<CRLSet> crl_set;
+    std::string crl_set_bytes;
+    EXPECT_TRUE(base::ReadFileToString(
+        GetTestCertsDirectory().AppendASCII("multi-root-crlset-K.raw"),
+        &crl_set_bytes));
+    ASSERT_TRUE(CRLSetStorage::Parse(crl_set_bytes, &crl_set));
+
+    error = Verify(cert.get(), "127.0.0.1", flags, crl_set.get(),
+                   empty_cert_list_, &verify_result);
+    ASSERT_EQ(OK, error);
+    ASSERT_EQ(0U, verify_result.cert_status);
+    ASSERT_TRUE(verify_result.verified_cert.get());
+
+    const X509Certificate::OSCertHandles& new_verified_intermediates =
+        verify_result.verified_cert->GetIntermediateCertificates();
+    ASSERT_EQ(2U, new_verified_intermediates.size());
+    scoped_refptr<X509Certificate> verified_root =
+        X509Certificate::CreateFromHandle(new_verified_intermediates[1],
+                                          X509Certificate::OSCertHandles());
+    ASSERT_TRUE(verified_root.get());
+    EXPECT_EQ("J Root CA", verified_root->subject().common_name);
+  }
+}
+
 #endif
 
 enum ExpectedAlgorithms {