Align cross-platform behaviors for CRLSets

On Windows and macOS, when a CRLSet is used to revoke a
certificate, the chain originally constructed by the OS
verifier is made available, as well as having the hashes
for that chain computed in
CertVerifyResult.public_key_hashes.

For Linux/ChromeOS, due to a bug in NSS's libpkix's
memoization of certificate paths during the chain building
process, combined with how Chromium implemented CRLSets
using an application-verifier callback, the constructed
chain was forgotten and not placed into the
CertVerifyResult.

Align the platforms to ensure that the CertVerifyResult
is populated with the (revoked) chain information. This
can be used with the ssl_error_assistant in //chrome to
provide additional error messaging for entries in CRLSets.

Bug: 989220
Change-Id: If4bf73d3548b0dec60980070ea7fa4c28edb0f08
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1727446
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682584}

net/cert/cert_verify_proc_nss.cc

@@ -175,14 +175,13 @@ CertStatus MapCertErrorToCertStatus(int err) {
   return MapNetErrorToCertStatus(net_error);
 }
 
-// Saves some information about the certificate chain cert_list in
-// *verify_result.  The caller MUST initialize *verify_result before calling
-// this function.
+// Extracts the certificate chain from |cert_list| (and optionally |root_cert|)
+// into an X509Certificate. If this fails, returns nullptr.
 // Note that cert_list[0] is the end entity certificate.
-void GetCertChainInfo(CERTCertList* cert_list,
-                      CERTCertificate* root_cert,
-                      CertVerifyResult* verify_result) {
-  DCHECK(cert_list);
+scoped_refptr<X509Certificate> GetCertChainInfo(CERTCertList* cert_list,
+                                                CERTCertificate* root_cert) {
+  if (!cert_list)
+    return nullptr;
 
   CERTCertificate* verified_cert = NULL;
   std::vector<CERTCertificate*> verified_chain;
@@ -223,13 +222,8 @@ void GetCertChainInfo(CERTCertList* cert_list,
   if (root_cert)
     verified_chain.push_back(root_cert);
 
-  scoped_refptr<X509Certificate> verified_cert_with_chain =
-      x509_util::CreateX509CertificateFromCERTCertificate(verified_cert,
+  return x509_util::CreateX509CertificateFromCERTCertificate(verified_cert,
                                                              verified_chain);
-  if (verified_cert_with_chain)
-    verify_result->verified_cert = std::move(verified_cert_with_chain);
-  else
-    verify_result->cert_status |= CERT_STATUS_INVALID;
 }
 
 // Returns true if the given certificate is one of the additional trust anchors.
@@ -345,6 +339,8 @@ struct CheckChainRevocationArgs {
   // The CRLSet to evaluate against.
   CRLSet* crl_set = nullptr;
 
+  ScopedCERTCertList chain;
+
   // The next callback to invoke, if the CRLSet does not report any errors.
   CERTChainVerifyCallback* next_callback = nullptr;
 
@@ -362,6 +358,9 @@ SECStatus CheckChainRevocationWithCRLSet(void* is_chain_valid_arg,
   CheckChainRevocationArgs* args =
       static_cast<CheckChainRevocationArgs*>(is_chain_valid_arg);
 
+  args->was_revoked = false;
+  args->chain.reset();
+
   CRLSetResult crlset_result = kCRLSetUnknown;
   if (args->crl_set) {
     crlset_result =
@@ -369,11 +368,25 @@ SECStatus CheckChainRevocationWithCRLSet(void* is_chain_valid_arg,
   }
 
   if (crlset_result == kCRLSetRevoked) {
+    // Record the current chain; as an application callback, libpkix will try
+    // to build a better chain, if possible, or otherwise unwind the path graph
+    // and forget that it found a potentially-valid, but application-rejected
+    // chain. For ease with later functions, this is implemented by duplicating
+    // the CERTCertList, which will take ownership of the certs inside it.
+    args->chain.reset(CERT_NewCertList());
+    for (CERTCertListNode* node = CERT_LIST_HEAD(current_chain);
+         !CERT_LIST_END(node, current_chain); node = CERT_LIST_NEXT(node)) {
+      if (CERT_AddCertToListTail(args->chain.get(),
+                                 CERT_DupCertificate(node->cert)) !=
+          SECSuccess) {
+        args->chain.reset();
+        break;
+      }
+    }
     args->was_revoked = true;
     *chain_ok = PR_FALSE;
     return SECSuccess;
   }
-  args->was_revoked = false;
 
   *chain_ok = PR_TRUE;
   if (!args->next_callback || !args->next_callback->isChainValid)
@@ -656,6 +669,9 @@ void AppendPublicKeyHashesAndTestKnownRoot(CERTCertList* cert_list,
                                            bool* known_root) {
   *known_root = false;
 
+  if (!cert_list)
+    return;
+
   // First, traverse the list to build the list of public key hashes, in order
   // of leaf to root.
   for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
@@ -909,6 +925,13 @@ int CertVerifyProcNSS::VerifyInternalImpl(
         cvout[cvout_cert_list_index].value.pointer.chain,
         cvout[cvout_trust_anchor_index].value.pointer.cert, &hashes,
         &known_root);
+  } else if (status == SECFailure &&
+             PORT_GetError() == SEC_ERROR_APPLICATION_CALLBACK_ERROR &&
+             check_chain_revocation_args.was_revoked) {
+    AppendPublicKeyHashesAndTestKnownRoot(
+        check_chain_revocation_args.chain.get(), nullptr, &hashes, &known_root);
+    // Restore the error (which may have been erased).
+    PORT_SetError(SEC_ERROR_APPLICATION_CALLBACK_ERROR);
   }
 
   if (status == SECSuccess &&
@@ -925,21 +948,40 @@ int CertVerifyProcNSS::VerifyInternalImpl(
           cvout[cvout_cert_list_index].value.pointer.chain,
           cvout[cvout_trust_anchor_index].value.pointer.cert, &hashes,
           &known_root);
+    } else if (status == SECFailure &&
+               PORT_GetError() == SEC_ERROR_APPLICATION_CALLBACK_ERROR &&
+               check_chain_revocation_args.was_revoked) {
+      AppendPublicKeyHashesAndTestKnownRoot(
+          check_chain_revocation_args.chain.get(), nullptr, &hashes,
+          &known_root);
+      // Restore the error (which may have been erased).
+      PORT_SetError(SEC_ERROR_APPLICATION_CALLBACK_ERROR);
     }
   }
 
-  if (status == SECSuccess) {
+  if (status == SECSuccess ||
+      (status == SECFailure &&
+       PORT_GetError() == SEC_ERROR_APPLICATION_CALLBACK_ERROR &&
+       check_chain_revocation_args.was_revoked)) {
     verify_result->public_key_hashes = hashes;
     verify_result->is_issued_by_known_root = known_root;
 
+    if (status == SECFailure) {
+      verify_result->verified_cert =
+          GetCertChainInfo(check_chain_revocation_args.chain.get(), nullptr);
+      // Restore the error (which may have been erased).
+      PORT_SetError(SEC_ERROR_APPLICATION_CALLBACK_ERROR);
+    } else {
+      verify_result->verified_cert =
+          GetCertChainInfo(cvout[cvout_cert_list_index].value.pointer.chain,
+                           cvout[cvout_trust_anchor_index].value.pointer.cert);
       verify_result->is_issued_by_additional_trust_anchor =
           IsAdditionalTrustAnchor(
               trust_anchors.get(),
               cvout[cvout_trust_anchor_index].value.pointer.cert);
-
-    GetCertChainInfo(cvout[cvout_cert_list_index].value.pointer.chain,
-                     cvout[cvout_trust_anchor_index].value.pointer.cert,
-                     verify_result);
+    }
+    if (!verify_result->verified_cert)
+      verify_result->cert_status |= CERT_STATUS_INVALID;
   }
 
   CRLSetResult crl_set_result = kCRLSetUnknown;

net/cert/cert_verify_proc_unittest.cc

@@ -2686,6 +2686,48 @@ TEST_P(CertVerifyProcInternalTest, CRLSetLeafSerial) {
   EXPECT_THAT(error, IsError(ERR_CERT_REVOKED));
 }
 
+TEST_P(CertVerifyProcInternalTest, CRLSetRootReturnsChain) {
+  if (!SupportsCRLSet()) {
+    LOG(INFO) << "Skipping test as verifier doesn't support CRLSet";
+    return;
+  }
+
+  CertificateList ca_cert_list =
+      CreateCertificateListFromFile(GetTestCertsDirectory(), "root_ca_cert.pem",
+                                    X509Certificate::FORMAT_AUTO);
+  ASSERT_EQ(1U, ca_cert_list.size());
+  ScopedTestRoot test_root(ca_cert_list[0].get());
+
+  scoped_refptr<X509Certificate> leaf = CreateCertificateChainFromFile(
+      GetTestCertsDirectory(), "ok_cert_by_intermediate.pem",
+      X509Certificate::FORMAT_AUTO);
+  ASSERT_TRUE(leaf);
+  ASSERT_EQ(1U, leaf->intermediate_buffers().size());
+
+  int flags = 0;
+  CertVerifyResult verify_result;
+  int error =
+      Verify(leaf.get(), "127.0.0.1", flags, CRLSet::BuiltinCRLSet().get(),
+             CertificateList(), &verify_result);
+  EXPECT_THAT(error, IsOk());
+
+  // Test revocation of the root itself.
+  scoped_refptr<CRLSet> crl_set;
+  std::string crl_set_bytes;
+  ASSERT_TRUE(base::ReadFileToString(
+      GetTestCertsDirectory().AppendASCII("crlset_by_root_spki.raw"),
+      &crl_set_bytes));
+  ASSERT_TRUE(CRLSet::Parse(crl_set_bytes, &crl_set));
+
+  error = Verify(leaf.get(), "127.0.0.1", flags, crl_set.get(),
+                 CertificateList(), &verify_result);
+  EXPECT_THAT(error, IsError(ERR_CERT_REVOKED));
+
+  EXPECT_EQ(3u, verify_result.public_key_hashes.size());
+  ASSERT_TRUE(verify_result.verified_cert);
+  EXPECT_EQ(2u, verify_result.verified_cert->intermediate_buffers().size());
+}
+
 // Tests that CertVerifyProc implementations apply CRLSet revocations by
 // subject.
 TEST_P(CertVerifyProcInternalTest, CRLSetRevokedBySubject) {

net/data/ssl/scripts/generate-test-certs.sh

@@ -543,6 +543,14 @@ python crlsetutil.py -o ../certificates/crlset_by_leaf_spki.raw \
 }
 CRLBYLEAFSPKI
 
+## Block a root cert directly by SPKI
+python crlsetutil.py -o ../certificates/crlset_by_root_spki.raw \
+<<CRLBYROOTSPKI
+{
+  "BlockedBySPKI": ["../certificates/root_ca_cert.pem"]
+}
+CRLBYROOTSPKI
+
 ## Block a leaf cert by issuer-hash-and-serial (ok_cert.pem == serial 3, by
 ## virtue of the serial file and ordering above.
 python crlsetutil.py -o ../certificates/crlset_by_root_serial.raw \