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..38c27459ae4286563fadad8812bd8e9be4f5e54e 100644
--- a/net/cert/cert_verify_proc_unittest.cc
+++ b/net/cert/cert_verify_proc_unittest.cc
@@ -107,6 +107,22 @@ bool SupportsDetectingKnownRoots() {
   return true;
 }
 
+// Template helper to load a series of certificate files into a CertificateList.
+// Like CertTestUtil's CreateCertificateListFromFile, except it can load a
+// series of individual certificates (to make the tests clearer).
+template <size_t N>
+void LoadCertificateFiles(const char* const(&cert_files)[N],
+                          CertificateList* certs) {
+  certs->clear();
+  for (size_t i = 0; i < N; ++i) {

[davidben, 2016/01/28 20:52:32]

Optional: This could also be a for each loop if you wanted.

[Ryan Sleevi, 2016/02/05 21:26:03]

? Were you thinking Go?

std::for_each is awful

If you meant a range-based loop (for (const auto& file : cert_files) ), that
feels like an anti-pattern for something like (&cert_files)[N], due to how
cert_files is implicitly castable in such a way to elide N. That is, it relies
more on compiler trickery and seems less legible.

I realize you mentioned it was optional, but I think I'd actually push back on
code if I saw that :)

[davidben, 2016/02/05 21:32:45]

for each loops are hardly a Go innovation...
Agreed.
Yeah, I meant a range-based for loop. I think I've seen that being used on sized
arrays in our code-base plenty. If the [N] got elided, I don't believe the
range-based for loop would build.

I disagree with you that this is trickery, but whatever. This works fine too,
hence the optional.

+    SCOPED_TRACE(cert_files[i]);
+    scoped_refptr<X509Certificate> cert = CreateCertificateChainFromFile(
+        GetTestCertsDirectory(), cert_files[i], X509Certificate::FORMAT_AUTO);
+    ASSERT_TRUE(cert);
+    certs->push_back(cert);
+  }
+}
+
 }  // namespace
 
 class CertVerifyProcTest : public testing::Test {
@@ -1360,6 +1376,203 @@ TEST_F(CertVerifyProcTest, CRLSetLeafSerial) {
                  &verify_result);
   EXPECT_EQ(ERR_CERT_REVOKED, error);
 }
+
+// Tests that revocation by CRLSet functions properly with the certificate
+// immediately before the trust anchor is revoked by that trust anchor, but
+// another version to a different trust anchor exists.
+//
+// The two possible paths are:
+// 1. A(B) -> B(C) -> C(D) -> D(D)
+// 2. A(B) -> B(C) -> C(E) -> E(E)
+//
+// In this test, C(E) is revoked by CRLSet. It is configured to be the more
+// preferable version compared to C(D), once revoked, it should be ignored.
+TEST_F(CertVerifyProcTest, CRLSetRevokedIntermediateSameName) {
+  const char* const kCertificatesToLoad[] = {
+      "multi-root-A-by-B.pem", "multi-root-B-by-C.pem", "multi-root-C-by-D.pem",
+      "multi-root-D-by-D.pem", "multi-root-C-by-E.pem", "multi-root-E-by-E.pem",
+  };
+  CertificateList certs;
+  ASSERT_NO_FATAL_FAILURE(LoadCertificateFiles(kCertificatesToLoad, &certs));
+
+  // Add D and E as trust anchors
+  ScopedTestRoot test_root_D(certs[3].get());  // D-by-D
+  ScopedTestRoot test_root_F(certs[5].get());  // E-by-E
+
+  // Create a chain that sends A(B), B(C), C(E), C(D). The reason that
+  // both C(E) and C(D) are sent are to ensure both certificates are available
+  // for path building. The test
+  // CertVerifyProcTest.VerifyReturnChainFiltersUnrelatedCerts ensures this is
+  // safe to do.
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(certs[1]->os_cert_handle());  // B-by-C
+  intermediates.push_back(certs[4]->os_cert_handle());  // C-by-E
+  intermediates.push_back(certs[2]->os_cert_handle());  // C-by-D
+  scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
+      certs[0]->os_cert_handle(), intermediates);
+  ASSERT_TRUE(cert);
+
+  // Sanity check: Ensure that, without any revocation status, the to-be-revoked
+  // path is preferred.
+  int flags = 0;
+  CertVerifyResult verify_result;
+  int error = Verify(cert.get(), "127.0.0.1", flags, NULL, empty_cert_list_,

[davidben, 2016/01/28 20:52:32]

Nit: nullptr

+                     &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) -> B(C) -> C(E) -> E(E).
+  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("E Root CA", verified_root->subject().common_name);
+
+  // Load a CRLSet that blocks C-by-E.
+  scoped_refptr<CRLSet> crl_set;
+  std::string crl_set_bytes;
+  EXPECT_TRUE(base::ReadFileToString(
+      GetTestCertsDirectory().AppendASCII("multi-root-crlset-C-by-E.raw"),
+      &crl_set_bytes));
+  ASSERT_TRUE(CRLSetStorage::Parse(crl_set_bytes, &crl_set));
+
+  // Verify with the CRLSet. Because C-by-E is revoked, the expected path is
+  // A(B) -> B(C) -> C(D) -> D(D).
+  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);
+
+  // Reverify without the CRLSet, to ensure that CRLSets do not persist between
+  // separate calls. As in the first verification, the expected path is
+  // A(B) -> B(C) -> C(E) -> E(E).
+  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("E Root CA", verified_root->subject().common_name);
+}
+
+// Tests that revocation by CRLSet functions properly when an intermediate is
+// revoked by SPKI. In this case, path building should ignore all certificates
+// with that SPKI, and search for alternatively keyed versions.
+//
+// The two possible paths are:
+// 1. A(B) -> B(C) -> C(D) -> D(D)
+// 2. A(B) -> B(F) -> F(E) -> E(E)
+//
+// The path building algorithm needs to explore B(C) once it discovers that
+// F(E) is revoked, and that there are no valid paths with B(F).
+TEST_F(CertVerifyProcTest, CRLSetRevokedIntermediateCrossIntermediates) {
+  const char* const kCertificatesToLoad[] = {
+      "multi-root-A-by-B.pem", "multi-root-B-by-C.pem", "multi-root-C-by-D.pem",
+      "multi-root-D-by-D.pem", "multi-root-B-by-F.pem", "multi-root-F-by-E.pem",
+      "multi-root-E-by-E.pem",
+  };
+  CertificateList certs;
+  ASSERT_NO_FATAL_FAILURE(LoadCertificateFiles(kCertificatesToLoad, &certs));
+
+  // Add D and E as trust anchors
+  ScopedTestRoot test_root_D(certs[3].get());  // D-by-D
+  ScopedTestRoot test_root_F(certs[6].get());  // E-by-E
+
+  // Create a chain that sends A(B), B(F), F(E), B(C), C(D). The reason that
+  // both B(C) and C(D) are sent are to ensure both certificates are available
+  // for path building. The test
+  // CertVerifyProcTest.VerifyReturnChainFiltersUnrelatedCerts ensures this is
+  // safe to do.
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(certs[4]->os_cert_handle());  // B-by-F
+  intermediates.push_back(certs[5]->os_cert_handle());  // F-by-E
+  intermediates.push_back(certs[1]->os_cert_handle());  // B-by-C
+  intermediates.push_back(certs[2]->os_cert_handle());  // C-by-D
+  scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
+      certs[0]->os_cert_handle(), intermediates);
+  ASSERT_TRUE(cert);
+
+  // Sanity check: Ensure that, without any revocation status, the to-be-revoked
+  // path is preferred.
+  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());
+
+  // The expected path is A(B) -> B(F) -> F(E) -> E(E).
+  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("E Root CA", verified_root->subject().common_name);
+
+  // Load a CRLSet that blocks F.
+  scoped_refptr<CRLSet> crl_set;
+  std::string crl_set_bytes;
+  EXPECT_TRUE(base::ReadFileToString(
+      GetTestCertsDirectory().AppendASCII("multi-root-crlset-F.raw"),
+      &crl_set_bytes));
+  ASSERT_TRUE(CRLSetStorage::Parse(crl_set_bytes, &crl_set));
+
+  // Verify with the CRLSet. Because F is revoked, the expected path is
+  // A(B) -> B(C) -> C(D) -> D(D).
+  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);

[davidben, 2016/01/28 20:52:32]

Could the existence of C(E) being used in a previous test cause us to chain to E
instead of D? A(B) -> B(C) -> C(E) -> E(E). Or is everything deterministic or
isolated enough to always pick C(D) over C(E)?

[Ryan Sleevi, 2016/02/05 21:26:03]

I'm not sure I understand your question. Line 1532 asserts the proper result
precisely to ensure C(D) is not returned until C(E) is revoked - that's
literally the cause for the complexity, to ensure that C(E) is otherwise picked
first, unless it's revoked.

[davidben, 2016/02/05 21:32:45]

*C*(E), not F(E). This tests revokes F(E) and doesn't ever touch C(E) at all.
I'm asking if any of our verifiers would introduce some non-determinism if they
ran the previous test first which does use C(E).

[Ryan Sleevi, 2016/02/05 21:39:26]

That's hard to answer, I think, in that you're asking me to predict the future?
If they did, we'd adjust to understand that cause of non-determinism and respond
appropriately, and this test failing would be that signal.

[davidben, 2016/02/05 21:46:48]

I'm just asking if they do that right now. I agree that the test would probably
notice, although it does mean an "E Root CA" check is kind of ambiguous. *shrug*

Actually... why not do:

EXPECT_TRUE(X509Certificate::IsSameOSCert(
                certs[0]->os_cert_handle(),
                verify_result.verified_cert->os_cert_handle()));
ASSERT_EQ(3u, new_verified_intermediates.size());
EXPECT_TRUE(X509Certificate::IsSameOSCert(certs[1]->os_cert_handle(),
                                          new_verified_intermediates[0]));
EXPECT_TRUE(X509Certificate::IsSameOSCert(certs[2]->os_cert_handle(),
                                          new_verified_intermediates[1]));
EXPECT_TRUE(X509Certificate::IsSameOSCert(certs[3]->os_cert_handle(),
                                          new_verified_intermediates[2]));

[Ryan Sleevi, 2016/02/05 21:52:11]

Right, but I'm not sure that ambiguity is a problem in this test, or needs to be
controlled here.

I don't want to make a strong check that _prevents_ such non-determinism, I
simply want to ensure that the problem path isn't found. If this test failed, it
means there's non-determinism to resolve, but it could still be functioning
correctly.

A more detailed test would be to make sure that intermediate-1 is not F(E),
which would handle if C(E) was chosen - but I *do* want to capture
non-determinism if there is any remaining.

I'll take this as optional, so if you wanna give your LG... :)

+
+  // Reverify without the CRLSet, to ensure that CRLSets do not persist between
+  // separate calls. As in the first verification, the expected path is
+  // A(B) -> B(F) -> F(E) -> E(E).
+  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("E Root CA", verified_root->subject().common_name);
+}
+
 #endif
 
 enum ExpectedAlgorithms {