Revert of Perform CRLSet evaluation during Path Building on NSS

net/cert/cert_verify_proc_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/cert/cert_verify_proc_nss.h"
 
 #include <string>
 #include <vector>
 
 #include <cert.h>
@@ skipping 261 matching lines @@
 //   kCRLSetRevoked: if any element of the chain is known to have been revoked.
 //   kCRLSetUnknown: if there is no fresh information about the leaf
 //       certificate in the chain or if the CRLSet has expired.
 //
 //       Only the leaf certificate is considered for coverage because some
 //       intermediates have CRLs with no revocations (after filtering) and
 //       those CRLs are pruned from the CRLSet at generation time. This means
 //       that some EV sites would otherwise take the hit of an OCSP lookup for
 //       no reason.
 //   kCRLSetOk: otherwise.
-CRLSetResult CheckRevocationWithCRLSet(const CERTCertList* cert_list,
+CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
                                        CERTCertificate* root,
                                        CRLSet* crl_set) {
   std::vector<CERTCertificate*> certs;
 
   if (cert_list) {
     for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
          !CERT_LIST_END(node, cert_list);
          node = CERT_LIST_NEXT(node)) {
       certs.push_back(node->cert);
     }
@@ skipping 52 matching lines @@
         error = true;
         continue;
     }
   }
 
   if (error || !last_covered || crl_set->IsExpired())
     return kCRLSetUnknown;
   return kCRLSetOk;
 }
 
-// Arguments for CheckChainRevocationWithCRLSet that are curried within the
-// CERTChainVerifyCallback's isChainValidArg.
-struct CheckChainRevocationArgs {
-  // The CRLSet to evaluate against.
-  CRLSet* crl_set = nullptr;
-
-  // The next callback to invoke, if the CRLSet does not report any errors.
-  CERTChainVerifyCallback* next_callback = nullptr;
-
-  // Indicates that the application callback failure was due to a CRLSet
-  // revocation, rather than due to |next_callback| rejecting it. This is
-  // used to map the error back to the proper caller-visible error code.
-  bool was_revoked = false;
-};
-
-SECStatus CheckChainRevocationWithCRLSet(void* is_chain_valid_arg,
-                                         const CERTCertList* current_chain,
-                                         PRBool* chain_ok) {
-  CHECK(is_chain_valid_arg);
-
-  CheckChainRevocationArgs* args =
-      static_cast<CheckChainRevocationArgs*>(is_chain_valid_arg);
-
-  CRLSetResult crlset_result = kCRLSetUnknown;
-  if (args->crl_set) {
-    crlset_result =
-        CheckRevocationWithCRLSet(current_chain, nullptr, args->crl_set);
-  }
-
-  if (crlset_result == kCRLSetRevoked) {
-    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)
-    return SECSuccess;
-
-  return (*args->next_callback->isChainValid)(
-      args->next_callback->isChainValidArg, current_chain, chain_ok);
-}
-
 // Forward declarations.
 SECStatus RetryPKIXVerifyCertWithWorkarounds(
     CERTCertificate* cert_handle, int num_policy_oids,
     bool cert_io_enabled, std::vector<CERTValInParam>* cvin,
     CERTValOutParam* cvout);
 SECOidTag GetFirstCertPolicy(CERTCertificate* cert_handle);
 
 // Call CERT_PKIXVerifyCert for the cert_handle.
 // Verification results are stored in an array of CERTValOutParam.
 // If |hard_fail| is true, and no policy_oids are supplied (eg: EV is NOT being
@@ skipping 453 matching lines @@
     cache_ocsp_response_from_side_channel_(CERT_GetDefaultCertDB(), cert_handle,
                                            PR_Now(), &ocsp_response_item,
                                            nullptr);
   }
 
   if (!cert->VerifyNameMatch(hostname,
                              &verify_result->common_name_fallback_used)) {
     verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
   }
 
-  // Setup a callback to call into CheckChainRevocationWithCRLSet with the
-  // current CRLSet. If the CRLSet revokes a given chain, |was_revoked|
-  // will be set to true.
-  // The same callback and args are used for every invocation of
-  // PKIXVerifyCert, as CheckChainRevocationWithCRLSet handles resetting
-  // |was_revoked| as necessary.
-  CheckChainRevocationArgs check_chain_revocation_args;
-  check_chain_revocation_args.crl_set = crl_set;
-  check_chain_revocation_args.next_callback = chain_verify_callback;
-
-  CERTChainVerifyCallback crlset_callback;
-  memset(&crlset_callback, 0, sizeof(crlset_callback));
-  crlset_callback.isChainValid = &CheckChainRevocationWithCRLSet;
-  crlset_callback.isChainValidArg =
-      static_cast<void*>(&check_chain_revocation_args);
-
   // Make sure that the cert is valid now.
   SECCertTimeValidity validity = CERT_CheckCertValidTimes(
       cert_handle, PR_Now(), PR_TRUE);
   if (validity != secCertTimeValid)
     verify_result->cert_status |= CERT_STATUS_DATE_INVALID;
 
   CERTValOutParam cvout[3];
   int cvout_index = 0;
   cvout[cvout_index].type = cert_po_certList;
   cvout[cvout_index].value.pointer.chain = NULL;
@@ skipping 17 matching lines @@
       (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED);
   if (check_revocation)
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
 
   ScopedCERTCertList trust_anchors;
   if (!additional_trust_anchors.empty()) {
     trust_anchors.reset(
         CertificateListToCERTCertList(additional_trust_anchors));
   }
 
-  SECStatus status =
-      PKIXVerifyCert(cert_handle, check_revocation, false, cert_io_enabled,
-                     NULL, 0, trust_anchors.get(), &crlset_callback, cvout);
+  SECStatus status = PKIXVerifyCert(cert_handle,
+                                    check_revocation,
+                                    false,
+                                    cert_io_enabled,
+                                    NULL,
+                                    0,
+                                    trust_anchors.get(),
+                                    chain_verify_callback,
+                                    cvout);
 
   if (status == SECSuccess &&
       (flags & CertVerifier::VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS) &&
       !IsKnownRoot(cvout[cvout_trust_anchor_index].value.pointer.cert)) {
     // TODO(rsleevi): Optimize this by supplying the constructed chain to
     // libpkix via cvin. Omitting for now, due to lack of coverage in upstream
     // NSS tests for that feature.
     scoped_cvout.Clear();
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
-    status = PKIXVerifyCert(cert_handle, true, true, cert_io_enabled, NULL, 0,
-                            trust_anchors.get(), &crlset_callback, cvout);
+    status = PKIXVerifyCert(cert_handle,
+                            true,
+                            true,
+                            cert_io_enabled,
+                            NULL,
+                            0,
+                            trust_anchors.get(),
+                            chain_verify_callback,
+                            cvout);
   }
 
   if (status == SECSuccess) {
     AppendPublicKeyHashes(cvout[cvout_cert_list_index].value.pointer.chain,
                           cvout[cvout_trust_anchor_index].value.pointer.cert,
                           &verify_result->public_key_hashes);
 
     verify_result->is_issued_by_known_root =
         IsKnownRoot(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);
   }
 
   CRLSetResult crl_set_result = kCRLSetUnknown;
   if (crl_set) {
-    if (status == SECSuccess) {
-      // Reverify the returned chain; NSS should have already called
-      // CheckChainRevocationWithCRLSet prior to returning, but given the
-      // edge cases (self-signed certs that are trusted; cached chains;
-      // unreadable code), this is more about defense in depth than
-      // functional necessity.
-      crl_set_result = CheckRevocationWithCRLSet(
-          cvout[cvout_cert_list_index].value.pointer.chain,
-          cvout[cvout_trust_anchor_index].value.pointer.cert, crl_set);
-      if (crl_set_result == kCRLSetRevoked) {
-        PORT_SetError(SEC_ERROR_REVOKED_CERTIFICATE);
-        status = SECFailure;
-      }
-    } else if (PORT_GetError() == SEC_ERROR_APPLICATION_CALLBACK_ERROR &&
-               check_chain_revocation_args.was_revoked) {
-      // If a CRLSet was supplied, and the error was an application callback
-      // error, then it was directed through the CRLSet code and that
-      // particular chain was revoked.
+    crl_set_result = CheckRevocationWithCRLSet(
+        cvout[cvout_cert_list_index].value.pointer.chain,
+        cvout[cvout_trust_anchor_index].value.pointer.cert,
+        crl_set);
+    if (crl_set_result == kCRLSetRevoked) {
       PORT_SetError(SEC_ERROR_REVOKED_CERTIFICATE);
+      status = SECFailure;
     }
   }
 
   if (status != SECSuccess) {
     int err = PORT_GetError();
     LOG(ERROR) << "CERT_PKIXVerifyCert for " << hostname
                << " failed err=" << err;
     // CERT_PKIXVerifyCert rerports the wrong error code for
     // expired certificates (NSS bug 491174)
     if (err == SEC_ERROR_CERT_NOT_VALID &&
@@ skipping 12 matching lines @@
     return MapCertStatusToNetError(verify_result->cert_status);
 
   if ((flags & CertVerifier::VERIFY_EV_CERT) && is_ev_candidate) {
     check_revocation |=
         crl_set_result != kCRLSetOk &&
         cert_io_enabled &&
         (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY);
     if (check_revocation)
       verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
 
-    if (VerifyEV(cert_handle, flags, crl_set, check_revocation, metadata,
-                 ev_policy_oid, trust_anchors.get(), &crlset_callback)) {
+    if (VerifyEV(cert_handle,
+                 flags,
+                 crl_set,
+                 check_revocation,
+                 metadata,
+                 ev_policy_oid,
+                 trust_anchors.get(),
+                 chain_verify_callback)) {
       verify_result->cert_status |= CERT_STATUS_IS_EV;
     }
   }
 
   return OK;
 }
 
 int CertVerifyProcNSS::VerifyInternal(
     X509Certificate* cert,
     const std::string& hostname,
     const std::string& ocsp_response,
     int flags,
     CRLSet* crl_set,
     const CertificateList& additional_trust_anchors,
     CertVerifyResult* verify_result) {
   return VerifyInternalImpl(cert, hostname, ocsp_response, flags, crl_set,
                             additional_trust_anchors,
                             NULL,  // chain_verify_callback
                             verify_result);
 }
 
 }  // namespace net