[PATCH 1/4] cryptnet: Specify half of the remaining timeout for each CRL if CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG was specified.
Instead of wasting the whole timeout on the first CRL. Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/cryptnet/cryptnet_main.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c index 06c468bd655..565302957d6 100644 --- a/dlls/cryptnet/cryptnet_main.c +++ b/dlls/cryptnet/cryptnet_main.c @@ -1539,22 +1539,24 @@ static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB * if (urlArray) { - DWORD j, retrievalFlags = 0, startTime, endTime, timeout; + 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 ((dwFlags & CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG) && pRevPara && pRevPara->cbSize >= RTL_SIZEOF_THROUGH_FIELD(CERT_REVOCATION_PARA, dwUrlRetrievalTimeout)) - { - startTime = GetTickCount(); - endTime = startTime + pRevPara->dwUrlRetrievalTimeout; timeout = pRevPara->dwUrlRetrievalTimeout; - } - else - endTime = timeout = 0; + + /* 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 */ @@ -1568,19 +1570,17 @@ static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB * if (ret) { error = verify_cert_revocation_with_crl_online(cert, crl, pTime, pRevStatus); - if (!error && timeout) - { - DWORD time = GetTickCount(); - - if ((int)(endTime - time) <= 0) - error = ERROR_TIMEOUT; - else - timeout = endTime - time; - } 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 (GetLastError() == ERROR_TIMEOUT) + timeout /= 2; error = CRYPT_E_REVOCATION_OFFLINE; + } } CryptMemFree(urlArray); } -- 2.30.2
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/cryptnet/cryptnet_main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c index 565302957d6..f4088021a0e 100644 --- a/dlls/cryptnet/cryptnet_main.c +++ b/dlls/cryptnet/cryptnet_main.c @@ -1547,8 +1547,7 @@ static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB * if (dwFlags & CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION) retrievalFlags |= CRYPT_CACHE_ONLY_RETRIEVAL; - if ((dwFlags & CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG) && pRevPara - && pRevPara->cbSize >= RTL_SIZEOF_THROUGH_FIELD(CERT_REVOCATION_PARA, dwUrlRetrievalTimeout)) + 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 @@ -1577,7 +1576,7 @@ static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB * /* 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 (GetLastError() == ERROR_TIMEOUT) + if ((dwFlags & CERT_VERIFY_REV_ACCUMULATIVE_TIMEOUT_FLAG) && GetLastError() == ERROR_TIMEOUT) timeout /= 2; error = CRYPT_E_REVOCATION_OFFLINE; } -- 2.30.2
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> --- dlls/cryptnet/cryptnet_main.c | 115 ++++++++++++++++++---------------- 1 file changed, 60 insertions(+), 55 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; } -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) +/* 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 error = ERROR_SUCCESS, cbUrlArray; + DWORD retrieve_flags = 0; + const CRL_CONTEXT *crl; + DWORD i; - if (CRYPT_GetUrlFromCRLDistPointsExt(value, NULL, &cbUrlArray, NULL, NULL)) + 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) { - CRYPT_URL_ARRAY *urlArray = CryptMemAlloc(cbUrlArray); + if (CryptRetrieveObjectByUrlW(array->rgwszUrl[i], CONTEXT_OID_CRL, retrieve_flags, + timeout, (void **)&crl, NULL, NULL, NULL, NULL)) + return crl; - 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; - - 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; + /* 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; } - else - error = GetLastError(); + + return NULL; +} + +static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB *value, const CERT_CONTEXT *cert, + FILETIME *time, DWORD flags, const CERT_REVOCATION_PARA *params, CERT_REVOCATION_STATUS *status) +{ + 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, url_array, &url_array_size, NULL, NULL)) + { + CryptMemFree(url_array); + return GetLastError(); + } + + if (params && params->cbSize >= RTL_SIZEOF_THROUGH_FIELD(CERT_REVOCATION_PARA, dwUrlRetrievalTimeout)) + timeout = params->dwUrlRetrievalTimeout; + + if (!(crl = retrieve_crl_from_dist_points(url_array, flags, timeout))) + { + CryptMemFree(url_array); + return CRYPT_E_REVOCATION_OFFLINE; + } + + error = verify_cert_revocation_with_crl_online(cert, crl, time, status); + + CertFreeCRLContext(crl); + CryptMemFree(url_array); return error; } -- 2.30.2
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- According to my limited understanding of certificates, this is safe, but I'd certainly appreciate a second opinion. The part about nextUpdate is informed by [1]. [1] https://security.stackexchange.com/questions/20958/x-509-crls-next-update dlls/cryptnet/Makefile.in | 2 +- dlls/cryptnet/cryptnet_main.c | 127 ++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 1 deletion(-) diff --git a/dlls/cryptnet/Makefile.in b/dlls/cryptnet/Makefile.in index 8482882cdce..1e97f26d9df 100644 --- a/dlls/cryptnet/Makefile.in +++ b/dlls/cryptnet/Makefile.in @@ -1,6 +1,6 @@ MODULE = cryptnet.dll IMPORTLIB = cryptnet -IMPORTS = crypt32 +IMPORTS = crypt32 shell32 ole32 DELAYIMPORTS = wininet EXTRADLLFLAGS = -mno-cygwin diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c index b1e2829d881..2f4e2d895f7 100644 --- a/dlls/cryptnet/cryptnet_main.c +++ b/dlls/cryptnet/cryptnet_main.c @@ -21,6 +21,7 @@ #define NONAMELESSUNION #define CERT_REVOCATION_PARA_HAS_EXTRA_FIELDS +#include <share.h> #include <stdio.h> #include <stdarg.h> @@ -31,6 +32,9 @@ #include "wininet.h" #include "objbase.h" #include "wincrypt.h" +#include "initguid.h" +#include "knownfolders.h" +#include "shlobj.h" #include "wine/debug.h" @@ -1514,6 +1518,124 @@ BOOL WINAPI CryptRetrieveObjectByUrlW(LPCWSTR pszURL, LPCSTR pszObjectOid, return ret; } +/* Store successful revocation checks (whether the certificate was revoked or + * not) in an on-disk cache. This is not because of network latency—we already + * have a cache for that—but rather because parsing very large CRLs can take a + * long time (at the time of writing, 20 MB CRLs have been seen in the wild and + * can take several hundred milliseconds) and applications expect chain building + * to be much faster. + * + * The cache is treated as invalid once we pass the nextUpdate field of the CRL. + * This isn't quite what the field is meant for (it's rather meant to specify a + * later bound for the next time the CRL will be reissued, and doesn't prescribe + * a date by which the CRL is invalid; see RFC 5280 § 5.1.2.5) but it's the way + * it's used in practice. + * + * The location of the cache roughly matches Windows, but the file name and + * contents do not. + */ + +static const char revocation_cache_signature[] = "Wine cached revocation"; + +static FILE *open_cached_revocation_file(const CRYPT_INTEGER_BLOB *serial, const WCHAR *mode, int sharing) +{ + WCHAR path[MAX_PATH]; + WCHAR *appdata_path; + DWORD len, i; + HRESULT hr; + + if (FAILED(hr = SHGetKnownFolderPath(&FOLDERID_LocalAppDataLow, 0, NULL, &appdata_path))) + { + ERR("Failed to get LocalAppDataLow path, hr %#x.\n", hr); + return INVALID_HANDLE_VALUE; + } + + len = swprintf(path, ARRAY_SIZE(path), L"%s\\Microsoft\\CryptnetUrlCache\\Content\\", appdata_path); + CoTaskMemFree(appdata_path); + + if (len + serial->cbData * 2 * sizeof(WCHAR) > ARRAY_SIZE(path) - 1) + { + WARN("Serial length exceeds static buffer; not caching.\n"); + return INVALID_HANDLE_VALUE; + } + + SHCreateDirectoryExW(NULL, path, NULL); + + for (i = 0; i < serial->cbData; ++i) + { + swprintf(path + len, 3, L"%02x", serial->pbData[i]); + len += 2; + } + + return _wfsopen(path, mode, sharing); +} + +static BOOL find_cached_revocation_status(const CRYPT_INTEGER_BLOB *serial, + const FILETIME *time, CERT_REVOCATION_STATUS *status) +{ + char buffer[sizeof(revocation_cache_signature)]; + FILETIME update_time; + FILE *file; + int len; + + if (!(file = open_cached_revocation_file(serial, L"r", _SH_DENYWR))) + return FALSE; + + if ((len = fread(buffer, 1, sizeof(buffer), file)) != sizeof(buffer) + || memcmp(buffer, revocation_cache_signature, len)) + { + ERR("Invalid cache signature.\n"); + fclose(file); + return FALSE; + } + + if (fread(&update_time, sizeof(update_time), 1, file) != 1) + { + ERR("Failed to read update time.\n"); + fclose(file); + return FALSE; + } + + if (CompareFileTime(time, &update_time) > 0) + { + TRACE("Cached revocation status is potentially out of date.\n"); + fclose(file); + return FALSE; + } + + if (fread(&status->dwError, sizeof(status->dwError), 1, file) != 1) + { + ERR("Failed to read error code.\n"); + fclose(file); + return FALSE; + } + + if (status->dwError == CERT_E_REVOKED && fread(&status->dwReason, sizeof(status->dwReason), 1, file) != 1) + { + ERR("Failed to read revocation reason.\n"); + fclose(file); + return FALSE; + } + + TRACE("Using cached status %#x, reason %#x.\n", status->dwError, status->dwReason); + return TRUE; +} + +static void cache_revocation_status(const CRYPT_INTEGER_BLOB *serial, + const FILETIME *time, const CERT_REVOCATION_STATUS *status) +{ + FILE *file; + + if (!(file = open_cached_revocation_file(serial, L"w", _SH_DENYRW))) + return; + fwrite(revocation_cache_signature, 1, sizeof(revocation_cache_signature), file); + fwrite(time, sizeof(*time), 1, file); + fwrite(&status->dwError, sizeof(status->dwError), 1, file); + if (status->dwError == CERT_E_REVOKED) + fwrite(&status->dwReason, sizeof(status->dwReason), 1, file); + fclose(file); +} + static DWORD verify_cert_revocation_with_crl_online(const CERT_CONTEXT *cert, const CRL_CONTEXT *crl, FILETIME *pTime, CERT_REVOCATION_STATUS *pRevStatus) { @@ -1591,6 +1713,8 @@ static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB * error = verify_cert_revocation_with_crl_online(cert, crl, time, status); + cache_revocation_status(&cert->pCertInfo->SerialNumber, &crl->pCrlInfo->NextUpdate, status); + CertFreeCRLContext(crl); CryptMemFree(url_array); return error; @@ -1663,6 +1787,9 @@ static DWORD verify_cert_revocation(const CERT_CONTEXT *cert, FILETIME *pTime, DWORD error = ERROR_SUCCESS; PCERT_EXTENSION ext; + if (find_cached_revocation_status(&cert->pCertInfo->SerialNumber, pTime, pRevStatus)) + return pRevStatus->dwError; + if ((ext = CertFindExtension(szOID_CRL_DIST_POINTS, cert->pCertInfo->cExtension, cert->pCertInfo->rgExtension))) { -- 2.30.2
participants (1)
-
Zebediah Figura