Perform CRLSet evaluation during Path Building on Windows

net/cert/cert_verify_proc_win.cc

Index: net/cert/cert_verify_proc_win.cc
diff --git a/net/cert/cert_verify_proc_win.cc b/net/cert/cert_verify_proc_win.cc
index f67f8234fc0048b80002f2b4b786f44e925e9410..742a2f00b5fb2b912c42e18983c547d3b9bf6c03 100644
--- a/net/cert/cert_verify_proc_win.cc
+++ b/net/cert/cert_verify_proc_win.cc
@@ -11,6 +11,7 @@
 #include "base/sha1.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/threading/thread_local.h"
 #include "crypto/capi_util.h"
 #include "crypto/scoped_capi_types.h"
 #include "crypto/sha2.h"
@@ -387,11 +388,89 @@ void GetCertPoliciesInfo(
 
 enum CRLSetResult {
   kCRLSetOk,
+  kCRLSetError,
   kCRLSetUnknown,
   kCRLSetRevoked,
 };
 
-// CheckRevocationWithCRLSet attempts to check each element of |chain|
+CRLSetResult CheckRevocationWithCRLSet(CRLSet* crl_set,

[davidben, 2016/01/06 03:37:48]

Documentation comment? It's somewhat unclear what previous_hash is and that it's
actually an input/output parameter. Something like, on input, |*previous_hash|
must either be empty or the hash of |issuer_cert|'s SPKI. On ??? return,
|*previous_hash| will be set to the hash of |subject_cert|'s SPKI.

[Ryan Sleevi, 2016/01/06 04:26:20]

Yeah, this part I threw up a little early to get feedback on, as I'm on triage
rotation today & tomorrow.

+                                       PCCERT_CONTEXT subject_cert,
+                                       PCCERT_CONTEXT issuer_cert,
+                                       std::string* previous_hash) {
+  DCHECK(crl_set);
+  DCHECK(subject_cert);
+
+  // Check to see if |subject_cert|'s SPKI is revoked. The actual revocation
+  // is handled by the SHA-256 hash of the SPKI, so compute that.
+  base::StringPiece der_bytes(
+      reinterpret_cast<const char*>(subject_cert->pbCertEncoded),
+      subject_cert->cbCertEncoded);
+
+  base::StringPiece spki;
+  if (!asn1::ExtractSPKIFromDERCert(der_bytes, &spki)) {
+    NOTREACHED();

[davidben, 2016/01/06 03:37:48]

This NOTREACHED() is because, to have gotten this far, Windows must have been
happy with the certificate, right? But this might fail if Windows' parser is
lenient in any way since we tend to write strict ones.

[Ryan Sleevi, 2016/01/06 04:26:19]

And it'll log, but behave consistently - but we certainly shouldn't reach this
path.

+    previous_hash->clear();
+    return kCRLSetError;
+  }
+  std::string subject_hash = crypto::SHA256HashString(spki);
+
+  CRLSet::Result result = crl_set->CheckSPKI(subject_hash);
+  if (result == CRLSet::REVOKED) {
+    previous_hash->swap(subject_hash);
+    return kCRLSetRevoked;
+  }
+
+  // If no issuer cert is provided, nor a hash of the issuer's SPKI, no
+  // further checks can be done.
+  if (!issuer_cert && previous_hash->empty()) {
+    previous_hash->swap(subject_hash);
+    return kCRLSetUnknown;
+  }
+
+  // Compute the subject's serial.
+  const CRYPT_INTEGER_BLOB* serial_blob =
+      &subject_cert->pCertInfo->SerialNumber;
+  scoped_ptr<uint8_t[]> serial_bytes(new uint8_t[serial_blob->cbData]);
+  // The bytes of the serial number are stored little-endian.

[davidben, 2016/01/06 03:37:48]

...what? There's also the leading 00 bytes and the sign bit and stuff. Per
https://msdn.microsoft.com/en-us/library/windows/desktop/aa377200(v=vs.85).aspx,
apparently Microsoft strips off leading 0x00 and 0xff bytes (which means that
this code can't distinguish negative serial numbers from the corresponding
positive one).

Anyway, CRLSet is actually just fine with all this (positive/negative collision
aside) since it also strips leading zeros, but it probably deserves a mention in
this comment.

[Ryan Sleevi, 2016/01/06 04:26:20]

But negative serials never happen! ;)

[Ryan Sleevi, 2016/01/14 01:13:15]

Confirmed it's a documentation bug; it's talking about the
CertCompareIntegerBlob function.

+  for (unsigned j = 0; j < serial_blob->cbData; j++)
+    serial_bytes[j] = serial_blob->pbData[serial_blob->cbData - j - 1];
+  base::StringPiece serial(reinterpret_cast<const char*>(serial_bytes.get()),
+                           serial_blob->cbData);
+
+  // Compute the issuer's hash. If it was provided (via previous_hash),
+  // use that; otherwise, compute it based on |issuer_cert|.
+  std::string issuer_hash_local;
+  std::string* issuer_hash = previous_hash;
+  if (issuer_hash->empty()) {
+    der_bytes = base::StringPiece(
+        reinterpret_cast<const char*>(issuer_cert->pbCertEncoded),
+        issuer_cert->cbCertEncoded);
+
+    if (!asn1::ExtractSPKIFromDERCert(der_bytes, &spki)) {
+      NOTREACHED();

[davidben, 2016/01/06 03:37:48]

Ditto about NOTREACHED().

+      return kCRLSetError;

[davidben, 2016/01/06 03:37:47]

This codepath doesn't touch *previous_hash which actually messes up the loop
later.

[Ryan Sleevi, 2016/01/06 04:26:20]

D'oh, good spot.

+    }
+
+    issuer_hash_local = crypto::SHA256HashString(spki);
+    issuer_hash = &issuer_hash_local;
+  }
+
+  // Look up by serial & issuer SPKI.
+  result = crl_set->CheckSerial(serial, *issuer_hash);
+  if (result == CRLSet::REVOKED)

[davidben, 2016/01/06 03:37:48]

switch-case, so we catch it all?

[Ryan Sleevi, 2016/01/06 04:26:19]

This was intentional; I think a switch hinders, rather than helps the
readability here.

+    return kCRLSetRevoked;
+
+  previous_hash->swap(subject_hash);

[davidben, 2016/01/06 03:37:48]

How come previous_hash does not get updated on revoke here but does up in line
418?

[Ryan Sleevi, 2016/01/06 04:26:20]

Because revoke terminates processing.

+  if (result == CRLSet::GOOD)
+    return kCRLSetOk;
+  if (result == CRLSet::UNKNOWN)
+    return kCRLSetUnknown;
+
+  NOTREACHED();
+  return kCRLSetError;
+}
+
+// CheckChainRevocationWithCRLSet attempts to check each element of |chain|
 // against |crl_set|. It returns:
 //   kCRLSetRevoked: if any element of the chain is known to have been revoked.
 //   kCRLSetUnknown: if there is no fresh information about the leaf
@@ -403,79 +482,32 @@ enum CRLSetResult {
 //       that some EV sites would otherwise take the hit of an OCSP lookup for
 //       no reason.
 //   kCRLSetOk: otherwise.
-CRLSetResult CheckRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
-                                       CRLSet* crl_set) {
-  if (chain->cChain == 0)
-    return kCRLSetOk;
-
-  const PCERT_SIMPLE_CHAIN first_chain = chain->rgpChain[0];
-  const PCERT_CHAIN_ELEMENT* element = first_chain->rgpElement;
+CRLSetResult CheckChainRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
+                                            CRLSet* crl_set) {
+  if (crl_set->IsExpired())
+    return kCRLSetUnknown;

[davidben, 2016/01/06 03:37:48]

This check got moved from the end. It's a change in behavior; if an expired
CRLSet affirmatively says that we should revoke some certs, I would think that
still means we should revoke it. Even though we did, in this case, have to
"unrevoke" something, good connectivity will pick that up, whereas an attacker
might try to suppress downloading a new CRLSet.

(If you disagree, then at least we should change this in all CertVerifyProc
implementations and not have them behave differently.)

[Ryan Sleevi, 2016/01/06 04:26:20]

An attacker can suppress downloading an updated CRLSet to block their
revocation. But I think it's more important that we have the capability to
"undo" something here - as evidenced by the bad revokes of Symantec & Comodo and
the pain they caused.

 
-  const int num_elements = first_chain->cElement;
-  if (num_elements == 0)
+  if (chain->cChain == 0 || chain->rgpChain[0]->cElement == 0)
     return kCRLSetOk;
 
-  // error is set to true if any errors are found. It causes such chains to be
-  // considered as not covered.
-  bool error = false;
-  // last_covered is set to the coverage state of the previous certificate. The
-  // certificates are iterated over backwards thus, after the iteration,
-  // |last_covered| contains the coverage state of the leaf certificate.
-  bool last_covered = false;
+  PCERT_CHAIN_ELEMENT* elements = chain->rgpChain[0]->rgpElement;
+  DWORD num_elements = chain->rgpChain[0]->cElement;
 
-  // We iterate from the root certificate down to the leaf, keeping track of
-  // the issuer's SPKI at each step.
+  bool had_error = false;
+  CRLSetResult result = kCRLSetError;
   std::string issuer_spki_hash;
-  for (int i = num_elements - 1; i >= 0; i--) {
-    PCCERT_CONTEXT cert = element[i]->pCertContext;
-
-    base::StringPiece der_bytes(
-        reinterpret_cast<const char*>(cert->pbCertEncoded),
-        cert->cbCertEncoded);
-
-    base::StringPiece spki;
-    if (!asn1::ExtractSPKIFromDERCert(der_bytes, &spki)) {
-      NOTREACHED();
-      error = true;
-      continue;
-    }
-
-    const std::string spki_hash = crypto::SHA256HashString(spki);
-
-    const CRYPT_INTEGER_BLOB* serial_blob = &cert->pCertInfo->SerialNumber;
-    scoped_ptr<uint8_t[]> serial_bytes(new uint8_t[serial_blob->cbData]);
-    // The bytes of the serial number are stored little-endian.
-    for (unsigned j = 0; j < serial_blob->cbData; j++)
-      serial_bytes[j] = serial_blob->pbData[serial_blob->cbData - j - 1];
-    base::StringPiece serial(reinterpret_cast<const char*>(serial_bytes.get()),
-                             serial_blob->cbData);
-
-    CRLSet::Result result = crl_set->CheckSPKI(spki_hash);
-
-    if (result != CRLSet::REVOKED && !issuer_spki_hash.empty())
-      result = crl_set->CheckSerial(serial, issuer_spki_hash);
-
-    issuer_spki_hash = spki_hash;
-
-    switch (result) {
-      case CRLSet::REVOKED:
-        return kCRLSetRevoked;
-      case CRLSet::UNKNOWN:
-        last_covered = false;
-        continue;
-      case CRLSet::GOOD:
-        last_covered = true;
-        continue;
-      default:
-        NOTREACHED();
-        error = true;
-        continue;
-    }
+  for (DWORD i = num_elements - 1; i >= 0; --i) {
+    PCCERT_CONTEXT subject = elements[i]->pCertContext;
+    result =
+        CheckRevocationWithCRLSet(crl_set, subject, nullptr, &issuer_spki_hash);
+    if (result == kCRLSetRevoked)
+      return result;
+    if (result == kCRLSetError)
+      had_error = true;
   }
-
-  if (error || !last_covered || crl_set->IsExpired())
+  if (had_error)
     return kCRLSetUnknown;
-  return kCRLSetOk;
+  return result;

[davidben, 2016/01/06 03:37:48]

This might warrant a comment like:
  // At this point |result| is the result of the leaf certificate.

(This logic seems a little weird. If our results are {CRLSet::UNKNOWN,
CRLSet::GOOD}, surely we should be returning kCRLSetUnknown? But if we want to
change that, we should change that in all of them concurrently.)

[Ryan Sleevi, 2016/01/06 04:26:19]

I don't understand what you're suggesting we would change - our results are
kCRLSetGood or kCRLSetUnknown

[Ryan Sleevi, 2016/01/14 01:13:15]

I reparsed what you're saying - you're talking about say, a root, intermediate,
leaf which results in {UNKNOWN, UNKNOWN, GOOD} - in this case, returning
kCRLSetOK for the reasons documented above in kCRLSetUnknown - to handle the
case where the intermediate is covered by the CRLSet.

I'm not entirely sure I agree with that decision, but any intermediate
revocations will be done via hard blacklist *anyway*, so it's not the end of the
world?

 }
 
 void AppendPublicKeyHashes(PCCERT_CHAIN_CONTEXT chain,
@@ -551,6 +583,198 @@ bool CheckEV(PCCERT_CHAIN_CONTEXT chain_context,
   return metadata->HasEVPolicyOID(fingerprint, policy_oid);
 }
 
+// Custom revocation provider function that compares incoming certificates with
+// those in CRLSets. This is called BEFORE the default CRL & OCSP handling
+// is invoked (which is handled by the revocation provider function
+// "CertDllVerifyRevocation" in cryptnet.dll)
+BOOL WINAPI
+CertDllVerifyRevocationWithCRLSet(DWORD encoding_type,
+                                  DWORD revocation_type,
+                                  DWORD num_contexts,
+                                  void* rgpvContext[],
+                                  DWORD flags,
+                                  PCERT_REVOCATION_PARA revocation_params,
+                                  PCERT_REVOCATION_STATUS revocation_status);
+
+// Helper class that installs the CRLSet-based Revocation Provider as the
+// default revocation provider. Because it is installed as a function address
+// (meaning only scoped to the process, and not stored in the registry), it
+// will be used before any registry-based providers, including Microsoft's
+// default provider.
+class RevocationInjector {
+ public:
+  CRLSet* GetCRLSet() { return thread_local_crlset.Get(); }
+
+  void SetCRLSet(CRLSet* crl_set) { thread_local_crlset.Set(crl_set); }
+
+ private:
+  friend struct base::DefaultLazyInstanceTraits<RevocationInjector>;
+
+  RevocationInjector() {
+    const CRYPT_OID_FUNC_ENTRY kInterceptFunction[] = {
+        {CRYPT_DEFAULT_OID, &CertDllVerifyRevocationWithCRLSet},
+    };
+    BOOL ok = CryptInstallOIDFunctionAddress(
+        NULL, X509_ASN_ENCODING, CRYPT_OID_VERIFY_REVOCATION_FUNC,
+        arraysize(kInterceptFunction), kInterceptFunction,
+        CRYPT_INSTALL_OID_FUNC_BEFORE_FLAG);
+    DCHECK(ok);
+  }
+
+  ~RevocationInjector() {}
+
+  // As the revocation parameters passed to CertVerifyProc::VerifyInternal
+  // cannot be officially smuggled to the Revocation Provider
+  base::ThreadLocalPointer<CRLSet> thread_local_crlset;

[davidben, 2016/01/06 03:37:48]

Ooh, nice trick. We should use it to lessen some of the globals in nss_ocsp.h
too. (Or replace it with Eric's new thing.) The current arrangement with globals
and stuff is pretty problematic.

[Ryan Sleevi, 2016/01/06 04:26:20]

Um, wat? That doesn't really reduce the globals at all for nss_ocsp - that's how
NSS behaves and requires.

[Ryan Sleevi, 2016/01/06 04:59:00]

I guess I should mention what I'd mentioned to Eric, which is that this is kind
of a gross hack.

I mean, it works, because CertVerifyRevocation is called during chain building,
and presently chain building is single threaded, but that's not an API
guarantee.

I originally had something far grosser (which is more in line with the spec),
which is creating a fake certificate (fake SPKI and everything), fake-signing it
(using one of the old "sha1WithNoEncryption" signature algorithm OIDs, which
triggers a special path in Windows), and sticking a pointer to the CRLSet in an
extension. In order to prevent an attacker from supplying an evil cert w/ the
same extension and parameters, I further had an ASLR-like magic value in the
extension that had to match the per-process magic value before the pointer was
trusted and de-referenced.

Of course, there was even MORE logic to ensure CryptoAPI wouldn't cache this
dummy cert (past the life of the pointer), including signalling the necessary
events on function exit to get CAPI to flush its caches.

As you can tell, this was gross as hell, but is more API conforming. I didn't
want to document all of what the 'didn't work', but I suppose I should document
more about why this is still "Gross Hack", but
"cleaner-though-not-quite-conforming" than the alternative.

[davidben, 2016/01/21 02:37:38]

Hrm, that's true. We would be assuming it calls the fetching callbacks from the
same thread.

My motivation here is that, while NSS's global callback is unavoidable, having
nss_ocsp.h also have a global URLRequestContext and TaskRunner and stuff could
be avoided with your thread-local trick.

I started doing a cleanup of that code a while ago, but didn't get very far. The
way things are right now is totally insane. EnsureNSSHttpIOInit gets called
inside the SSLClientSocket implementations which samples the message loop then,
but this is the wrong message loop anyway because you actually want the message
loop SetURLRequestContextForNSSHttpIO calls. And then the EnsureNSSHttpIOInit
calls happen at yet another point and has a ShutdownNSSHttpIO.

If that stuff could be more local, maybe even switching to Eric's
CertNetFetcherImpl now, I think that would be worthwhile.

Anyway, this isn't actually relevant to this CL.

[Ryan Sleevi, 2016/01/21 02:54:04]

Which NSS's API contract is such that that is not guaranteed. NSS, by way of
NSPR, is allowed (and does) start dedicated threads for some tasks. Not for
fetching, but that's not guaranteed ;)

But yeah, not relevant for this CL :)

+};
+
+// Leaky, as CertVerifyProc workers are themselves leaky.
+base::LazyInstance<RevocationInjector>::Leaky g_revocation_injector =
+    LAZY_INSTANCE_INITIALIZER;
+
+BOOL WINAPI
+CertDllVerifyRevocationWithCRLSet(DWORD encoding_type,
+                                  DWORD revocation_type,
+                                  DWORD num_contexts,
+                                  void* rgpvContext[],
+                                  DWORD flags,
+                                  PCERT_REVOCATION_PARA revocation_params,
+                                  PCERT_REVOCATION_STATUS revocation_status) {
+  PCERT_CONTEXT* cert_contexts = reinterpret_cast<PCERT_CONTEXT*>(rgpvContext);
+  // The dummy CRLSet provider never returns that something is affirmatively
+  // *un*revoked, as this would disable other revocation providers from being
+  // checked for this certificate (much like an OCSP "Good" status would).
+  // Instead, it merely indicates that insufficient information existed to
+  // determine if the certificate was revoked (in the good case), or that a cert
+  // is affirmatively revoked in the event it appears within the CRLSet.
+  // Because of this, set up some basic bookkeeping for the results.
+  CHECK(revocation_status);
+  revocation_status->dwIndex = 0;
+  revocation_status->dwError = static_cast<DWORD>(CRYPT_E_NO_REVOCATION_CHECK);
+  revocation_status->dwReason = 0;
+
+  if (num_contexts == 0) {
+    SetLastError(static_cast<DWORD>(E_INVALIDARG));
+    return FALSE;
+  }
+  if ((GET_CERT_ENCODING_TYPE(encoding_type) != X509_ASN_ENCODING) ||

[davidben, 2016/01/06 03:37:48]

Just to confirm, this one should never happen due to the second parameter to
CryptInstallOIDFunctionAddress, right?

[Ryan Sleevi, 2016/01/06 04:26:20]

Right

+      revocation_type != CERT_CONTEXT_REVOCATION_TYPE) {
+    SetLastError(static_cast<DWORD>(CRYPT_E_NO_REVOCATION_CHECK));
+    return FALSE;
+  }
+
+  // |revocation_params| is an optional structure; to make life simple and avoid
+  // the need to constantly check whether or not it was supplied, create a local
+  // copy. If the caller didn't supply anything, it will be empty; otherwise,
+  // it will be (non-owning) copies of the caller's original params.
+  CERT_REVOCATION_PARA local_params;
+  memset(&local_params, 0, sizeof(local_params));
+  local_params.cbSize = sizeof(local_params);
+  if (revocation_params) {
+    DWORD bytes_to_copy =
+        std::min(revocation_params->cbSize, local_params.cbSize);
+    memcpy(&local_params, revocation_params, bytes_to_copy);
+    local_params.cbSize = sizeof(local_params);

[davidben, 2016/01/06 03:37:48]

Nit: Could also take this and line 672 out and move it after the memset/memcpy
and outside the conditional.

+  }
+
+  PCERT_CONTEXT subject_cert = cert_contexts[0];
+  if (!subject_cert) {
+    SetLastError(static_cast<DWORD>(E_INVALIDARG));
+    return FALSE;
+  }
+
+  // Determine the issuer cert for the incoming cert
+  ScopedPCCERT_CONTEXT issuer_cert;
+  if ((flags & CERT_VERIFY_REV_CHAIN_FLAG) && num_contexts > 1) {
+    // Verifying a chain; issuer is the next cert in the chain.

[davidben, 2016/01/06 03:37:48]

A priori I would have expected this to have expected all the certificates. I
mention this doesn't happen in the review comment, but it seems this should at
least have a comment if not a (D)CHECK or error return or DumpWithoutCrashing or
something to the effect. Or am I misunderstanding and that's something else?

Microsoft's docs also say:
"""
Of these options, a provider MUST support the CERT_VERIFY_REV_CHAIN_FLAG flag.
This flag indicates that all certificates in the certificate chain are to be
validated. When this flag is provided, the calling application must provide the
certificate chain in the rgpvContext[ ] parameter; when processing this
parameter, a revocation provider might assume that this parameter contains the
entire chain with the first one being the end-entity certificate. Examples of
such calling applications include the Authenticode Software Publishing module
and WinVerifyTrust().

When CERT_VERIFY_REV_CHAIN_FLAG is specified, the provider MUST return overall
status versus status of an individual certificate. If it does not have enough
information to make the appropriate requests, it MUST return
CRYPT_E_NO_REVOCATION_CHECK.
"""

[Ryan Sleevi, 2016/01/06 04:26:19]

Self-signed certs and janky-ass third-party invocations (because it's installed
in the process space, this will be called for everything - including things like
WinVerifyTrust and Authenticode)

I suppose I could move line 748 up some to bail early for those cases
Which is what we do. I explained in the commit comment and in the comments here
that we either say "try the next one" or "gtfo". CAPI *has* to call this during
path building - in fact, CAPI *never* calls it with a chain for
CertGetCertificateChain(), because it's already computed the status per-cert
during path building.

I'm not sure I understand what your concern is?

+    issuer_cert.reset(CertDuplicateCertificateContext(cert_contexts[1]));
+  } else if (local_params.pIssuerCert) {
+    // Caller has already supplied the issuer cert via the revocation params;
+    // just use that.
+    issuer_cert.reset(
+        CertDuplicateCertificateContext(local_params.pIssuerCert));
+  } else if (CertCompareCertificateName(subject_cert->dwCertEncodingType,
+                                        &subject_cert->pCertInfo->Subject,
+                                        &subject_cert->pCertInfo->Issuer) &&
+             CryptVerifyCertificateSignatureEx(
+                 NULL, subject_cert->dwCertEncodingType,
+                 CRYPT_VERIFY_CERT_SIGN_SUBJECT_CERT, subject_cert,
+                 CRYPT_VERIFY_CERT_SIGN_ISSUER_CERT, subject_cert, 0,
+                 nullptr)) {
+    // Certificate is self-signed; use it as its own issuer.
+    issuer_cert.reset(CertDuplicateCertificateContext(subject_cert));
+  } else {
+    // Scan the caller-supplied stores first, to try and find the issuer cert.
+    for (DWORD i = 0; i < local_params.cCertStore && !issuer_cert; ++i) {
+      DWORD store_search_flags = CERT_STORE_SIGNATURE_FLAG;
+      PCCERT_CONTEXT previous_cert = nullptr;
+      while ((previous_cert = CertGetIssuerCertificateFromStore(
+                  local_params.rgCertStore[i], subject_cert, previous_cert,
+                  &store_search_flags),
+              previous_cert)) {

[Ryan Sleevi, 2016/01/04 22:33:33]

Note: This was to avoid MSVC complaints about assignment in conditionals

[davidben, 2016/01/06 03:37:48]

I am fond of Go's decl + comparison pattern, but I don't think we do it in C++
much and the comma operator is kind of screwy. Perhaps:

for (;;) {
  PCCERT_CONTEXT previous_cert = ....;
  if (!previous_cert)
    break;

  // Rest of the loop.
}

You can also then write this like...

  for (;;) {
    DWORD store_search_flags = CERT_STORE_SIGNATURE_FLAG;
    PCCERT_CONTEXT previous_cert = .... &store_search_flags ...
    if (!....) {
    }
  }

Specifically this avoids having to reset store_search_flags in line 723.

+        // If a cert is found and meets the criteria, the flag will be reset to
+        // zero. Thus NOT having the bit set is equivalent to having found a
+        // matching certificate.
+        if (!(store_search_flags & CERT_STORE_SIGNATURE_FLAG)) {
+          // No need to dupe; reference is held.
+          issuer_cert.reset(previous_cert);
+          break;
+        }
+        store_search_flags = CERT_STORE_SIGNATURE_FLAG;

[davidben, 2016/01/06 03:37:48]

Looks like previous_cert gets leaked if you hit this codepath. Make it a
ScopedPCCERT_CONTEXT?

[Ryan Sleevi, 2016/01/06 04:26:19]

Line 711 frees it (all the enumerative functions take the previous cert to use
as the iterator context, with null being the 'start enumeration' context)

+      }
+      if (issuer_cert)
+        break;
+      if (GetLastError() == CRYPT_E_SELF_SIGNED) {
+        issuer_cert.reset(CertDuplicateCertificateContext(subject_cert));
+        break;
+      }
+    }
+
+    // At this point, the Microsoft provider opens up the "CA", "Root", and
+    // "SPC" stores to search for the issuer certificate, if not found in the
+    // caller-supplied stores.
+    // TODO(rsleevi): Determine if this is necessary
+  }
+
+  if (!issuer_cert) {
+    revocation_status->dwError = static_cast<DWORD>(CRYPT_E_REVOCATION_OFFLINE);

[davidben, 2016/01/06 03:37:48]

Why OFFLINE and not NO_REVOCATION_CHECK? The docs say:

"""
When CERT_VERIFY_REV_CHAIN_FLAG is specified, the provider MUST return overall
status versus status of an individual certificate. If it does not have enough
information to make the appropriate requests, it MUST return
CRYPT_E_NO_REVOCATION_CHECK.
"""

[Ryan Sleevi, 2016/01/06 04:26:19]

Because it has enough information (e.g. from the certificate or, in this case,
by the fact that we're called) but it's unable to.

CRYPT_E_NO_REVOCATION_CHECK is the no-error try next case
CRYPT_E_REVOCATION_OFFLINE indicates there was an active issue w/ revocation
checking.

+    SetLastError(revocation_status->dwError);
+    return FALSE;
+  }
+
+  // TODO(rsleevi) Do baked-in certificate revocation.

[davidben, 2016/01/06 03:37:48]

super nitpicky nit: comma after close-colon.

[Ryan Sleevi, 2016/01/06 04:26:20]

super nitpicky nit nit: Did you mean colon after close-paren? ;)

+
+  // Do dynamic certificate revocation.
+  CRLSet* crl_set = g_revocation_injector.Get().GetCRLSet();
+  if (!crl_set)
+    return FALSE;
+  if (crl_set->IsExpired())

[davidben, 2016/01/06 03:37:48]

Ditto re comment on line 485. (Also this would be redundant with
CheckRevocationWithCRLSet's check, right?)

[Ryan Sleevi, 2016/01/06 04:26:20]

No - this is executed during path building, and CheckRevocationWithCRLSet
doesn't check IsExpired (you're thinking CheckChainRevocationWithCRLSet, which
does, and which this is the mirror too)

Because CheckChainRev is implemented in terms of CheckRev, I didn't want to have
to check IsExpired in every iteration of the loop of CheckChainRev, hence why I
foisted it out of CheckRev and into here (... hopefully that makes sense)

+    return FALSE;
+
+  std::string unused;

[davidben, 2016/01/06 03:37:48]

Nit: Since this is an input/output parameter, it's not quiiite unused.

[Ryan Sleevi, 2016/01/06 04:26:19]

Is there a concrete suggestion here for what you think is more suitable? I think
"unused" here is entirely accurate in terms of the caller context - even if the
function uses it.

+  CRLSetResult result = CheckRevocationWithCRLSet(crl_set, subject_cert,
+                                                  issuer_cert.get(), &unused);
+  if (result == kCRLSetRevoked) {

[davidben, 2016/01/06 03:37:48]

Nit: Should this be a switch-case just to explicitly list out the cases? It's
not immediately obvious this is what you wanted for kCRLSetError. (Though it's
probably fine?)

[Ryan Sleevi, 2016/01/06 04:26:20]

It is not what we want for kCRLSetError, which the documentation on 475-ish was
meant to cover (which needs to be foisted into enum documentation)

+    revocation_status->dwError = static_cast<DWORD>(CRYPT_E_REVOKED);
+    revocation_status->dwReason = CRL_REASON_UNSPECIFIED;
+    SetLastError(revocation_status->dwError);
+    return FALSE;
+  }
+
+  // The result is ALWAYS FALSE in order to allow the next revocation provider
+  // a chance to examine. The only possible time it is TRUE is when the
+  // certificate is already revoked.

[davidben, 2016/01/06 03:37:48]

It doesn't return TRUE in that case either, right?

[Ryan Sleevi, 2016/01/06 04:26:19]

Comment BUG, thanks :) Revoked gets a FALSE return too, and SetLastError
smuggles that state; TRUE is only used for "it's good"

+  return FALSE;
+}
+
+class ScopedThreadLocalCRLSet {
+ public:
+  explicit ScopedThreadLocalCRLSet(CRLSet* crl_set) {
+    g_revocation_injector.Get().SetCRLSet(crl_set);
+  }
+  ~ScopedThreadLocalCRLSet() { g_revocation_injector.Get().SetCRLSet(nullptr); }
+};
+
 }  // namespace
 
 CertVerifyProcWin::CertVerifyProcWin() {}
@@ -578,6 +802,10 @@ int CertVerifyProcWin::VerifyInternal(
     CRLSet* crl_set,
     const CertificateList& additional_trust_anchors,
     CertVerifyResult* verify_result) {
+  // Ensure the Revocation Provider has been installed and configured for this
+  // CRLSet.
+  ScopedThreadLocalCRLSet thread_local_crlset(crl_set);
+
   PCCERT_CONTEXT cert_handle = cert->os_cert_handle();
   if (!cert_handle)
     return ERR_UNEXPECTED;
@@ -621,33 +849,41 @@ int CertVerifyProcWin::VerifyInternal(
     }
   }
 
-  // We can set CERT_CHAIN_RETURN_LOWER_QUALITY_CONTEXTS to get more chains.
-  DWORD chain_flags = CERT_CHAIN_CACHE_END_CERT |
-                      CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT;
+  // Revocation checking is always enabled, in order to enable CRLSets to be
+  // evaluated as part of a revocation provider. However, when the caller did
+  // not explicitly request revocation checking (which is to say, online
+  // revocation checking), then only enable cached results. This disables OCSP
+  // and CRL fetching, but still allows the revocation provider to be called.
+  // Note: The root cert is also checked for revocation status, so that CRLSets
+  // will cover revoked SPKIs.
+  DWORD chain_flags = CERT_CHAIN_REVOCATION_CHECK_CHAIN;

[Ryan Sleevi, 2016/01/04 22:33:33]

I stopped supplying CACHE_END_CERT for two reasons:
1) It never did anything (the cert cached is keyed off of the hAdditionalStore
to CertGetCertificateChain, so we've been disabling the Windows cache for some
time)
2) To force revocation checks as best as possible (note: AFAICT, Windows still
still perform revocation checks even for cached certs, but... can't find
anything documented that says that)

   bool rev_checking_enabled =
       (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED);
-
   if (rev_checking_enabled) {
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
   } else {
     chain_flags |= CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY;
   }
 
-  // For non-test scenarios, use the default HCERTCHAINENGINE, NULL, which
-  // corresponds to HCCE_CURRENT_USER and is is initialized as needed by
-  // crypt32. However, when testing, it is necessary to create a new
-  // HCERTCHAINENGINE and use that instead. This is because each
-  // HCERTCHAINENGINE maintains a cache of information about certificates
-  // encountered, and each test run may modify the trust status of a
-  // certificate.
+  // By default, use the default HCERTCHAINENGINE (aka HCCE_CURRENT_USER). When
+  // running tests, use a dynamic HCERTCHAINENGINE. All of the status and cache
+  // of verified certificates and chains is tied to the HCERTCHAINENGINE. As
+  // each invocation may have changed the set of known roots, invalid the cache
+  // between runs.
+  //
+  // This is not the most efficient means of doing so; it's possible to mark the
+  // Root store used by TestRootCerts as changed, via CertControlStore with the

[davidben, 2016/01/06 03:37:48]

Root -> root? Or is this a Windows proper noun?

[Ryan Sleevi, 2016/01/06 04:26:20]

It's the name of a specific Logical Store ("Root")

+  // CERT_STORE_CTRL_NOTIFY_CHANGE / CERT_STORE_CTRL_RESYNC, but that's more
+  // complexity for what is test-only code.
   ScopedHCERTCHAINENGINE chain_engine(NULL);
   if (TestRootCerts::HasInstance())
     chain_engine.reset(TestRootCerts::GetInstance()->GetChainEngine());
 
   ScopedPCCERT_CONTEXT cert_list(cert->CreateOSCertChainForCert());
 
+  // Add stapled OCSP response data, which will be preferred over online checks
+  // and used when in cache-only mode.
   if (!ocsp_response.empty()) {
-    // Attach the OCSP response to the chain.
     CRYPT_DATA_BLOB ocsp_response_blob;
     ocsp_response_blob.cbData = ocsp_response.size();
     ocsp_response_blob.pbData =
@@ -657,10 +893,7 @@ int CertVerifyProcWin::VerifyInternal(
         CERT_SET_PROPERTY_IGNORE_PERSIST_ERROR_FLAG, &ocsp_response_blob);
   }
 
-  PCCERT_CHAIN_CONTEXT chain_context;
-  // IE passes a non-NULL pTime argument that specifies the current system
-  // time.  IE passes CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT as the
-  // chain_flags argument.
+  PCCERT_CHAIN_CONTEXT chain_context = nullptr;
   if (!CertGetCertificateChain(
            chain_engine,
            cert_list.get(),
@@ -674,9 +907,13 @@ int CertVerifyProcWin::VerifyInternal(
     return MapSecurityError(GetLastError());
   }
 
+  // Perform a second check with CRLSets. Although the Revocation Provider
+  // should have prevented invalid paths from being built, the behaviour and
+  // timing of how a Revocation Provider is invoked is not well documented. This
+  // is just defense in depth.
   CRLSetResult crl_set_result = kCRLSetUnknown;
   if (crl_set)
-    crl_set_result = CheckRevocationWithCRLSet(chain_context, crl_set);
+    crl_set_result = CheckChainRevocationWithCRLSet(chain_context, crl_set);
 
   if (crl_set_result == kCRLSetRevoked) {
     verify_result->cert_status |= CERT_STATUS_REVOKED;