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(CERTCertList* cert_list,
+CRLSetResult CheckRevocationWithCRLSet(const 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;
 }
 
+scoped_refptr<X509Certificate> CreateCertFromCERTList(
+    const CERTCertList* cert_list) {
+  X509Certificate::OSCertHandles intermediates;
+  for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
+       !CERT_LIST_END(node, cert_list); node = CERT_LIST_NEXT(node)) {
+    intermediates.push_back(node->cert);
+  }
+  if (intermediates.empty())
+    return nullptr;
+
+  X509Certificate::OSCertHandle leaf = intermediates.front();
+  intermediates.erase(intermediates.begin());

[eroman, 2016/02/25 03:12:02]

Could have avoided the erase().

[Ryan Sleevi, 2016/03/01 22:44:57]

I'm not sure your remark here, or what you're suggesting.

As best I can tell, you're suggesting to track leaf (CERT_LIST_HEAD) vs the rest
of the intermediates, to avoid the erase? If so, that seems significantly more
complicated logic, but perhaps I'm missing something obvious.

[eroman, 2016/03/02 00:05:07]

In the construction of intermediates vector above, you could have avoiding
pushing the leaf node in since it is just popped here.

Not worth it if these are small vectors and you think it will complicate your
code.

+#if defined(OS_IOS)
+  return x509_util_ios::CreateCertFromNSSHandles(leaf, intermediates);
+#else
+  return X509Certificate::CreateFromHandle(leaf, intermediates);
+#endif
+}
+
+// Arguments for CheckChainRevocationWithCRLSet that are curried within the
+// CERTChainVerifyCallback's isChainValidArg.
+struct CheckChainRevocationArgs {
+  CRLSet* crl_set;
+  CERTChainVerifyCallback* next_callback;
+  scoped_refptr<X509Certificate> revoked_chain;
+};
+
+SECStatus CheckChainRevocationWithCRLSet(void* is_chain_valid_arg,
+                                         const CERTCertList* current_chain,
+                                         PRBool* chain_ok) {
+  if (!is_chain_valid_arg) {

[eroman, 2016/02/25 03:12:02]

Is this actually reachable?

[Ryan Sleevi, 2016/03/01 22:44:57]

It's NSS; the guarantees the API provides are undocumented.

That said, moved it to a CHECK

+    *chain_ok = PR_FALSE;
+    return SECFailure;
+  }
+
+  CheckChainRevocationArgs* args =
+      static_cast<CheckChainRevocationArgs*>(is_chain_valid_arg);
+
+  CRLSetResult crlset_result = kCRLSetUnknown;
+  if (args->crl_set)

[eroman, 2016/02/25 03:12:01]

curlies

+    crlset_result =
+        CheckRevocationWithCRLSet(current_chain, nullptr, args->crl_set);
+
+  if (crlset_result == kCRLSetRevoked) {
+    args->revoked_chain = CreateCertFromCERTList(current_chain);
+    *chain_ok = PR_FALSE;
+    return SECSuccess;
+  }
+  args->revoked_chain = nullptr;
+
+  *chain_ok = PR_TRUE;
+  if (!args->next_callback || !args->next_callback->isChainValid)

[eroman, 2016/02/25 03:12:02]

Is it expected that isChainValid can be NULL, or is the second part just
defensive?

[Ryan Sleevi, 2016/03/01 22:44:57]

Yes, it is; this matches the internal checks of NSS with respect to invoking its
callbacks. This is due to how the structure behaves.

+    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;
   }
 
+  CheckChainRevocationArgs check_chain_revocation_args;
+  check_chain_revocation_args.crl_set = crl_set;
+  check_chain_revocation_args.next_callback = chain_verify_callback;
+  check_chain_revocation_args.revoked_chain = nullptr;

[eroman, 2016/02/25 03:12:01]

could have omitted this unless you are trying to be explicit in naming all the
members.

[Ryan Sleevi, 2016/03/01 22:44:58]

crl_set and next_callback are pointer types; they aren't guaranteed to be
zero-initialized without an explicit constructor. But hey, C++11 means we can
use default initializers - which I did

[eroman, 2016/03/02 00:05:07]

I was referring to revoked_chain, which is a scoped_refptr<>.

That is default initialized 4 lines earlier.
So it is guaranteed to already be nullptr.

Shrug, no longer relevant in new patchset.

+
+  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(),
-                                    chain_verify_callback,
-                                    cvout);
+  SECStatus status =
+      PKIXVerifyCert(cert_handle, check_revocation, false, cert_io_enabled,
+                     NULL, 0, trust_anchors.get(), &crlset_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(),
-                            chain_verify_callback,
-                            cvout);
+    status = PKIXVerifyCert(cert_handle, true, true, cert_io_enabled, NULL, 0,
+                            trust_anchors.get(), &crlset_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) {
-    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) {
+    if (status == SECSuccess) {
+      crl_set_result = CheckRevocationWithCRLSet(

[eroman, 2016/02/25 03:12:02]

Wasn't this already called by PKIXVerifyCert() on the constructed path, why call
it again?

[Ryan Sleevi, 2016/03/01 22:44:57]

As with the Windows code, we're taking a defense in depth approach.

But to directly answer your question: It's supposed to be, yes. But I don't
trust NSS.

+          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.revoked_chain) {

[eroman, 2016/02/25 03:12:02]

It doesn't look like the revoked_chain is every consumed, except for its
existence.
Why not just use a boolean then?

[Ryan Sleevi, 2016/03/01 22:44:58]

You're right; I was originally going to sub in the revoked chain since it would
have passed all the verification checks, but given that we can't presently
guarantee the behaviour that a invalid chain will be available across all
platforms, that was an unnecessary optimization. Switched to a bool.

+      // 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.
       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(),
-                 chain_verify_callback)) {
+    if (VerifyEV(cert_handle, flags, crl_set, check_revocation, metadata,
+                 ev_policy_oid, trust_anchors.get(), &crlset_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