Perform CRLSet evaluation during Path Building on NSS

net/cert/cert_verify_proc_nss.cc

Index: net/cert/cert_verify_proc_nss.cc
diff --git a/net/cert/cert_verify_proc_nss.cc b/net/cert/cert_verify_proc_nss.cc
index ca8e8116f0ef521f61539386abbc59dd57da2f21..9db660bb9c9d5ddf733c0f74d98f2e91b3fe33e0 100644
--- a/net/cert/cert_verify_proc_nss.cc
+++ b/net/cert/cert_verify_proc_nss.cc
@@ -279,7 +279,7 @@ enum CRLSetResult {
 //       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;
@@ -352,6 +352,64 @@ CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
   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,
@@ -825,6 +883,17 @@ int CertVerifyProcNSS::VerifyInternalImpl(
     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);
@@ -862,15 +931,9 @@ int CertVerifyProcNSS::VerifyInternalImpl(
         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) &&
@@ -880,15 +943,8 @@ int CertVerifyProcNSS::VerifyInternalImpl(
     // 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) {
@@ -910,13 +966,20 @@ int CertVerifyProcNSS::VerifyInternalImpl(
 
   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;
     }
   }
 
@@ -949,14 +1012,8 @@ int CertVerifyProcNSS::VerifyInternalImpl(
     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;
     }
   }