Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(166)

Unified Diff: net/cert/cert_verify_proc_win.cc

Issue 1557133002: Perform CRLSet evaluation during Path Building on Windows (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: More tests Created 4 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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..37cf19be9029bd04118bc4a2570409dbc2fa644b 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"
@@ -386,12 +387,130 @@ void GetCertPoliciesInfo(
}
enum CRLSetResult {
- kCRLSetOk,
+ // Indicates an error happened while attempting to determine CRLSet status.
+ // For example, if the certificate's SPKI could not be extracted.
+ kCRLSetError,
+
+ // Indicates there is no fresh information about the certificate, or if the
+ // CRLSet has expired.
+ // In the case of certificate chains, this is only returned if the leaf
+ // certificate is not covered by the CRLSet; this is because some
+ // intermediates are fully covered, but after filtering, the issuer's CRL
+ // is empty and thus omitted from the CRLSet. Since online checking is
+ // performed for EV certificates when this status is returned, this would
+ // result in needless online lookups for certificates known not-revoked.
kCRLSetUnknown,
+
+ // Indicates that the certificate (or a certificate in the chain) has been
+ // revoked.
kCRLSetRevoked,
+
+ // The certificate (or certificate chain) has no revocations.
+ kCRLSetOk,
};
-// CheckRevocationWithCRLSet attempts to check each element of |chain|
+// Determines if |subject_cert| is revoked within |crl_set|,
+// storing the SubjectPublicKeyInfo hash of |subject_cert| in
+// |*previous_hash|.
+//
+// CRLSets store revocations by both SPKI and by the tuple of Issuer SPKI
+// Hash & Serial. While |subject_cert| contains enough information to check
+// for SPKI revocations, to determine the issuer's SPKI, either |issuer_cert|
+// must be supplied, or the hash of the issuer's SPKI provided in
+// |previous_hash|. If |issuer_cert| is omitted, and |previous_hash| is empty,
davidben 2016/01/21 02:37:39 Nit: Most of these |previous_hash|s should probabl
+// only SPKI checks are performed.
+//
+// To avoid recomputing SPKI hashes, the hash of |subject_cert| is stored in
+// |*previous_hash|. This allows chaining revocation checking, by starting
+// at the root and iterating to the leaf, supplying |previous_hash| each time.
+//
+// In the event of a parsing error, |previous_hash| is cleared, to prevent the
+// wrong Issuer&Serial tuple from being used.
+CRLSetResult CheckRevocationWithCRLSet(CRLSet* crl_set,
+ 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)) {
+ // Should not reach here; it indicates Windows accepted something that
+ // could not even be remotely parsed.
+ NOTREACHED();
+ previous_hash->clear();
+ return kCRLSetError;
+ }
+ std::string subject_hash = crypto::SHA256HashString(spki);
davidben 2016/01/21 02:37:39 Nit: I'd probably take lines 436 through 450 and p
+
+ CRLSet::Result result = crl_set->CheckSPKI(subject_hash);
+ if (result == CRLSet::REVOKED) {
+ previous_hash->swap(subject_hash);
davidben 2016/01/21 02:37:39 This is inconsistent with line 500's CRLSet::REVOK
Ryan Sleevi 2016/01/21 02:54:04 I agree, we shouldn't update it here, but I disagr
+ 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.
+ // Note: While MSDN implies that bytes are stripped from this serial,
+ // they are not - only CertCompareIntegerBlob actually removes bytes.
+ for (unsigned j = 0; j < serial_blob->cbData; j++)
davidben 2016/01/21 02:37:39 Nit: unsigned -> DWORD, to match cbData's type?
Ryan Sleevi 2016/01/21 02:54:04 Yeah, good point; copy pasta.
+ 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|.
davidben 2016/01/21 02:37:39 Nit: I would take this block and switch it with th
Ryan Sleevi 2016/01/21 02:54:04 I find that (the folding) substantially less reada
+ 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)) {
+ // Should not reach here; it indicates Windows accepted something that
+ // could not even be remotely parsed.
+ NOTREACHED();
+ previous_hash->clear();
+ return kCRLSetError;
+ }
+
+ 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)
+ return kCRLSetRevoked;
+
+ previous_hash->swap(subject_hash);
+ 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 +522,29 @@ 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;
-
- const int num_elements = first_chain->cElement;
- if (num_elements == 0)
+CRLSetResult CheckChainRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
+ CRLSet* crl_set) {
+ 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 = 0; i < num_elements; ++i) {
+ PCCERT_CONTEXT subject = elements[num_elements - i - 1]->pCertContext;
davidben 2016/01/21 02:37:39 Optional: If you want to avoid the num_elements -
Ryan Sleevi 2016/01/21 02:54:04 I intentionally wanted to avoid relying on "stupid
+ 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 || crl_set->IsExpired())
return kCRLSetUnknown;
- return kCRLSetOk;
+ return result;
}
void AppendPublicKeyHashes(PCCERT_CHAIN_CONTEXT chain,
@@ -551,6 +620,243 @@ 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;
+};
+
+// 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);
davidben 2016/01/21 02:37:39 Nit: newline here? Comments that aren't preceded b
Ryan Sleevi 2016/01/21 02:54:05 Agreed.
+ // 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 || !cert_contexts[0]) {
+ SetLastError(static_cast<DWORD>(E_INVALIDARG));
+ return FALSE;
+ }
+
+ if ((GET_CERT_ENCODING_TYPE(encoding_type) != X509_ASN_ENCODING) ||
davidben 2016/01/21 02:37:39 Style nit: Unnecessary parens. (Alternatively, mis
+ revocation_type != CERT_CONTEXT_REVOCATION_TYPE) {
+ SetLastError(static_cast<DWORD>(CRYPT_E_NO_REVOCATION_CHECK));
+ return FALSE;
+ }
+
+ // No revocation checking possible if there is no associated
+ // CRLSet.
+ CRLSet* crl_set = g_revocation_injector.Get().GetCRLSet();
+ if (!crl_set)
+ 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));
+ if (revocation_params) {
+ DWORD bytes_to_copy = std::min(revocation_params->cbSize,
+ static_cast<DWORD>(sizeof(local_params)));
+ memcpy(&local_params, revocation_params, bytes_to_copy);
+ }
+ local_params.cbSize = sizeof(local_params);
+
+ PCERT_CONTEXT subject_cert = cert_contexts[0];
+
+ if ((flags & CERT_VERIFY_REV_CHAIN_FLAG) && num_contexts > 1) {
Ryan Sleevi 2016/01/15 01:12:55 I just went ahead and did the iterative code, alth
+ // Verifying a chain; first verify from the last certificate in the
+ // chain to the first, and then leave the last certificate (which
+ // is presumably self-issued, although it may simply be a trust
+ // anchor) as the |subject_cert| in order to scan for more
+ // revocations.
+ std::string issuer_hash;
+ PCCERT_CONTEXT issuer_cert = nullptr;
+ for (DWORD i = num_contexts; i > 0; --i) {
davidben 2016/01/21 02:37:39 Optional: Another option is to write this as: f
+ subject_cert = cert_contexts[i - 1];
+ if (!subject_cert) {
davidben 2016/01/21 02:37:39 Why does this function check for NULL while CheckC
Ryan Sleevi 2016/01/21 02:54:04 Yes. CertVerifyRevocation is caller-exposed, and n
+ SetLastError(static_cast<DWORD>(E_INVALIDARG));
+ return FALSE;
+ }
+ CRLSetResult result = CheckRevocationWithCRLSet(
+ crl_set, subject_cert, issuer_cert, &issuer_hash);
+ if (result == kCRLSetRevoked) {
+ revocation_status->dwIndex = i - 1;
+ revocation_status->dwError = static_cast<DWORD>(CRYPT_E_REVOKED);
+ revocation_status->dwReason = CRL_REASON_UNSPECIFIED;
+ SetLastError(revocation_status->dwError);
+ return FALSE;
+ }
+ issuer_cert = subject_cert;
+ }
+ // Verified all certificates from the trust anchor to the leaf, and none
+ // were explicitly revoked. Now do a second pass to attempt to determine
+ // the issuer for cert_contexts[num_contexts - 1], so that the
+ // Issuer SPKI+Serial can be checked for that certificate.
+ //
+ // This code intentionally ignores the flag
+ subject_cert = cert_contexts[num_contexts - 1];
+ // Reset local_params.pIssuerCert, since it would contain the issuer
+ // for cert_contexts[0].
+ local_params.pIssuerCert = nullptr;
+ // Fixup the revocation index to point to this cert (in the event it is
+ // revoked). If it isn't revoked, this will be done undone later.
+ revocation_status->dwIndex = num_contexts - 1;
+ }
+
+ // Determine the issuer cert for the incoming cert
+ ScopedPCCERT_CONTEXT issuer_cert;
+ if (local_params.pIssuerCert &&
davidben 2016/01/21 02:37:39 [I love how they don't say anything useful about t
+ CryptVerifyCertificateSignatureEx(
+ NULL, subject_cert->dwCertEncodingType,
+ CRYPT_VERIFY_CERT_SIGN_SUBJECT_CERT, subject_cert,
+ CRYPT_VERIFY_CERT_SIGN_ISSUER_CERT,
+ const_cast<PCERT_CONTEXT>(local_params.pIssuerCert), 0, nullptr)) {
+ // 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) {
+ PCCERT_CONTEXT previous_cert = nullptr;
+ for (;;) {
+ DWORD store_search_flags = CERT_STORE_SIGNATURE_FLAG;
+ previous_cert = CertGetIssuerCertificateFromStore(
+ local_params.rgCertStore[i], subject_cert, previous_cert,
+ &store_search_flags);
+ if (!previous_cert)
+ break;
+ // 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;
+ }
+ }
+ 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. It is unclear whether that is necessary here.
+ }
+
+ if (!issuer_cert) {
+ // Rather than return CRYPT_E_NO_REVOCATION_CHECK (indicating everything
+ // is fine to try the next provider), return CRYPT_E_REVOCATION_OFFLINE.
+ // This propogates up to the caller as an error while checking revocation,
+ // which is the desired intent if there are certificates that cannot
+ // be checked.
+ revocation_status->dwIndex = 0;
+ revocation_status->dwError = static_cast<DWORD>(CRYPT_E_REVOCATION_OFFLINE);
+ SetLastError(revocation_status->dwError);
+ return FALSE;
+ }
+
+ std::string unused;
+ CRLSetResult result = CheckRevocationWithCRLSet(crl_set, subject_cert,
+ issuer_cert.get(), &unused);
+ if (result == kCRLSetRevoked) {
+ 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 difference is whether or not an error is
+ // indicated via dwError (and SetLastError()).
+ // Reset the error index so that Windows does not believe this code has
+ // examined the entire chain and found no issues until the last cert (thus
+ // skipping other revocation providers).
+ revocation_status->dwIndex = 0;
+ 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 +884,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 +931,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;
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
+ // 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 +975,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 +989,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;

Powered by Google App Engine
This is Rietveld 408576698