Implement support for fetching CRLs based on the CRL distribution point extension
Description
Environment
is related to
Activity
Scott Cantor January 31, 2011 at 10:24 AM
I don't think it needs fixing. AFAIK, OpenSSL still checks any other CRLs it has, it just doesn't require them, so it doesn't hurt anything.
Former user January 31, 2011 at 10:22 AMEdited
I just realized that when checkRevocation="entityOnly" (a setting I don't consider that sensible), we might end up with fetching CRLs we don't really need. Not sure if you think it's worth fixing... it would mean breaking out of the "for (int i = 0; i < sk_X509_num(untrusted); ++i)" loop after the first pass, IINM.
Scott Cantor January 31, 2011 at 10:09 AM
Fixed remaining bugs and cleaned up the return paths a bit, should be final.
Former user January 31, 2011 at 9:46 AMEdited
Oh, and I just found a stupid off-by-one error I introduced with the patch from Saturday:
@@ -376,7 +376,7 @@
const char* cdpuri = (const char*)gen->d.ia5->data;
auto_ptr<XSECCryptoX509CRL> crl(getRemoteCRLs(cdpuri, log));
if (crl.get() && crl->getProviderName()==DSIGConstants::s_unicodeStrPROVOpenSSL &&
(isFreshCRL(crl.get()) || (ii == sk_DIST_POINT_num(dps) && iii == sk_GENERAL_NAME_num(dp->distpoint->name.fullname)))) {
+ (isFreshCRL(crl.get()) || (ii == sk_DIST_POINT_num(dps)1 && iii == sk_GENERAL_NAME_num(dp>distpoint->name.fullname)-1))) {
// owned by store
X509_STORE_add_crl(store, X509_CRL_dup(static_cast<OpenSSLCryptoX509CRL*>(crl.get())->getOpenSSLX509CRL()));
log.debug("added CRL issued by (%s)", crlissuer.c_str());
(this is meant to add the CRL if it's the last CRLDP we could find - even if it doesn't pass our "freshness check")
Former user January 31, 2011 at 7:39 AMEdited
Thanks for the timezone fix, Z is for Zulu/UTC, yes. (The Linux man page for timegm suggests avoiding timegm for portability reasons [and replacing with some getenv/setenv/tzset dance instead], but I think it's good enough for getCRLTime).
While testing, I actually found another, rather serious issue: so far, revocation checking wasn't really being applied, even if one of the X509_V_FLAG_CRL_CHECK or X509_V_FLAG_CRL_CHECK_ALL flags was set! The reason for this is that the code currently has
X509_STORE_set_flags(store, fullCRLChain ? (X509_V_FLAG_CRL_CHECK|X509_V_FLAG_CRL_CHECK_ALL) : (X509_V_FLAG_CRL_CHECK));
but this is only set after calling X509_STORE_CTX_init, which means that it doesn't apply to the subsequent verification with x509_verify_cert... (basically, the X509_STORE_CTX has its own flags). Setting the flag on the store after having called X509_STORE_CTX_init basically means that CRLs are ignored (even if they have been added to the store).
The attached patch should fix this problem, by switching to X509_STORE_CTX_set_flags(); I also suggest to remove the check for the empty crlissuers as OpenSSL will complain about missing CRLs anyway.
The PKIX trust engine currently supports CRLs which are either embedded in the signature or statically configured in a CredentialResolver element. A useful enhancement would be support for the CRL distribution point (CRLDP) extension, i.e., fetch CRLs based from URIs which are included in the certs themselves.
The attached patch is a proof of concept / skeleton for adding such a feature to the PKIX trust engine. Noteworthy points:
CRLDP is implemented as a "fallback": we only attempt retrieval from the network if no CRL (for a specific issuer) is available yet. Also, enabling CRLDP support only makes sense if the trust engine specifies mandatory revocation checking (see also )
to prevent "random" CRLDP URIs from being followed, we want to make sure that we first verify whether the certs provided in the chain validate (w/o revocation checking), and only if that's the case we should attempt fetching CRLs from those URIs
I suggest to limit CRLDP support to HTTP URIs, for the time being. Also, as a further short-circuit, the patch currently stops looping over all CRLDPs after the first HTTP URI has been found (there are CAs which put in multiple CRLDPs, for reasons of redundancy)
caching isn't addressed in this implementation - but is something which should definitely be considered. For a backing file name, (a hash of) the issuer name could be used, and before re-fetching a CRL from the network we should probably check for the freshness of the CRL we currently have (through X509_CRL_get_lastUpdate() and X509_CRL_get_nextUpdate()).