Zebediah Figura : cryptnet: Check only the first successfully retrieved CRL in verify_cert_revocation_from_dist_points_ext().
Module: wine Branch: master Commit: 13a349540ae0230f83466c04e003209656a889c0 URL: https://source.winehq.org/git/wine.git/?a=commit;h=13a349540ae0230f83466c04e... Author: Zebediah Figura <zfigura(a)codeweavers.com> Date: Thu Jul 22 00:21:06 2021 -0500 cryptnet: Check only the first successfully retrieved CRL in verify_cert_revocation_from_dist_points_ext().
From RFC 5280 ยง 4.2.1.13:
If the DistributionPointName contains multiple values, each name describes a different mechanism to obtain the same CRL. For example, the same CRL could be available for retrieval through both LDAP and HTTP. Steam attempts to validate a certificate containing what are apparently two different mirrored URLs to the same 20 MB CRL, which currently takes over 400ms to parse in Wine. According to my reading of the RFC, we should only need to parse one of them, cutting the time in half. Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> Signed-off-by: Alexandre Julliard <julliard(a)winehq.org> --- dlls/cryptnet/cryptnet_main.c | 111 ++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 53 deletions(-) diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c index f4088021a0e..b1e2829d881 100644 --- a/dlls/cryptnet/cryptnet_main.c +++ b/dlls/cryptnet/cryptnet_main.c @@ -1528,66 +1528,71 @@ static DWORD verify_cert_revocation_with_crl_online(const CERT_CONTEXT *cert, return ERROR_SUCCESS; } +/* Try to retrieve a CRL from any one of the specified distribution points. */ +static const CRL_CONTEXT *retrieve_crl_from_dist_points(const CRYPT_URL_ARRAY *array, + DWORD verify_flags, DWORD timeout) +{ + DWORD retrieve_flags = 0; + const CRL_CONTEXT *crl; + DWORD i; + + if (verify_flags & CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION) + retrieve_flags |= CRYPT_CACHE_ONLY_RETRIEVAL; + + /* Yes, this is a weird algorithm, but the documentation for + * CERT_CHAIN_REVOCATION_ACCUMULATIVE_TIMEOUT specifies this, and + * tests seem to bear it out for CertVerifyRevocation() as well. */ + if (verify_flags & CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG) + timeout /= 2; + + for (i = 0; i < array->cUrl; ++i) + { + if (CryptRetrieveObjectByUrlW(array->rgwszUrl[i], CONTEXT_OID_CRL, retrieve_flags, + timeout, (void **)&crl, NULL, NULL, NULL, NULL)) + return crl; + + /* We don't check the current time here. This may result in less + * accurate timeouts, but this too seems to be true of Windows. */ + if ((verify_flags & CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG) && GetLastError() == ERROR_TIMEOUT) + timeout /= 2; + } + + return NULL; +} + static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB *value, const CERT_CONTEXT *cert, - FILETIME *pTime, DWORD dwFlags, const CERT_REVOCATION_PARA *pRevPara, CERT_REVOCATION_STATUS *pRevStatus) + FILETIME *time, DWORD flags, const CERT_REVOCATION_PARA *params, CERT_REVOCATION_STATUS *status) { - DWORD error = ERROR_SUCCESS, cbUrlArray; + DWORD url_array_size, error; + CRYPT_URL_ARRAY *url_array; + const CRL_CONTEXT *crl; + DWORD timeout = 0; + + if (!CRYPT_GetUrlFromCRLDistPointsExt(value, NULL, &url_array_size, NULL, NULL)) + return GetLastError(); + + if (!(url_array = CryptMemAlloc(url_array_size))) + return ERROR_OUTOFMEMORY; - if (CRYPT_GetUrlFromCRLDistPointsExt(value, NULL, &cbUrlArray, NULL, NULL)) + if (!CRYPT_GetUrlFromCRLDistPointsExt(value, url_array, &url_array_size, NULL, NULL)) { - CRYPT_URL_ARRAY *urlArray = CryptMemAlloc(cbUrlArray); + CryptMemFree(url_array); + return GetLastError(); + } - if (urlArray) - { - DWORD j, retrievalFlags = 0, timeout = 0; - BOOL ret; - - ret = CRYPT_GetUrlFromCRLDistPointsExt(value, urlArray, - &cbUrlArray, NULL, NULL); - if (dwFlags & CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION) - retrievalFlags |= CRYPT_CACHE_ONLY_RETRIEVAL; - - if (pRevPara && pRevPara->cbSize >= RTL_SIZEOF_THROUGH_FIELD(CERT_REVOCATION_PARA, dwUrlRetrievalTimeout)) - timeout = pRevPara->dwUrlRetrievalTimeout; - - /* Yes, this is a weird algorithm, but the documentation for - * CERT_CHAIN_REVOCATION_ACCUMULATIVE_TIMEOUT specifies this, and - * tests seem to bear it out for CertVerifyRevocation() as well. */ - if (dwFlags & CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG) - timeout /= 2; - - if (!ret) - error = GetLastError(); - /* continue looping if one was offline; break if revoked or timed out */ - for (j = 0; (!error || error == CRYPT_E_REVOCATION_OFFLINE) && j < urlArray->cUrl; j++) - { - PCCRL_CONTEXT crl; + if (params && params->cbSize >= RTL_SIZEOF_THROUGH_FIELD(CERT_REVOCATION_PARA, dwUrlRetrievalTimeout)) + timeout = params->dwUrlRetrievalTimeout; - ret = CryptRetrieveObjectByUrlW(urlArray->rgwszUrl[j], - CONTEXT_OID_CRL, retrievalFlags, timeout, (void **)&crl, - NULL, NULL, NULL, NULL); - if (ret) - { - error = verify_cert_revocation_with_crl_online(cert, crl, pTime, pRevStatus); - CertFreeCRLContext(crl); - } - else - { - /* We don't check the current time here. This may result in - * less accurate timeouts, but this too seems to be true of - * Windows. */ - if ((dwFlags & CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG) && GetLastError() == ERROR_TIMEOUT) - timeout /= 2; - error = CRYPT_E_REVOCATION_OFFLINE; - } - } - CryptMemFree(urlArray); - } - else - error = ERROR_OUTOFMEMORY; + if (!(crl = retrieve_crl_from_dist_points(url_array, flags, timeout))) + { + CryptMemFree(url_array); + return CRYPT_E_REVOCATION_OFFLINE; } - else - error = GetLastError(); + + error = verify_cert_revocation_with_crl_online(cert, crl, time, status); + + CertFreeCRLContext(crl); + CryptMemFree(url_array); return error; }
participants (1)
-
Alexandre Julliard