[PATCH 0/4] MR7112: crypt32, cryptnet: OCSP revocation check improvements.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/decode.c | 2 +- dlls/crypt32/tests/encode.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/dlls/crypt32/decode.c b/dlls/crypt32/decode.c index cfdeef5380a..44877a6e9d6 100644 --- a/dlls/crypt32/decode.c +++ b/dlls/crypt32/decode.c @@ -6722,7 +6722,7 @@ static BOOL WINAPI CRYPT_AsnDecodeOCSPBasicResponse(DWORD dwCertEncodingType, { ASN_SEQUENCEOF, offsetof(OCSP_BASIC_RESPONSE_INFO, cResponseEntry), CRYPT_AsnDecodeOCSPBasicResponseEntriesArray, MEMBERSIZE(OCSP_BASIC_RESPONSE_INFO, cResponseEntry, cExtension), TRUE, TRUE, offsetof(OCSP_BASIC_RESPONSE_INFO, rgResponseEntry) }, - { ASN_CONTEXT | ASN_CONSTRUCTOR, offsetof(OCSP_BASIC_RESPONSE_INFO, cExtension), + { ASN_CONTEXT | ASN_CONSTRUCTOR | 1, offsetof(OCSP_BASIC_RESPONSE_INFO, cExtension), CRYPT_AsnDecodeCertExtensions, FINALMEMBERSIZE(OCSP_BASIC_RESPONSE_INFO, cExtension), TRUE, TRUE, offsetof(OCSP_BASIC_RESPONSE_INFO, rgExtension), 0 }, }; diff --git a/dlls/crypt32/tests/encode.c b/dlls/crypt32/tests/encode.c index b5db9cf7d0d..c6c461999e6 100644 --- a/dlls/crypt32/tests/encode.c +++ b/dlls/crypt32/tests/encode.c @@ -8847,6 +8847,7 @@ static const BYTE ocsp_basic_signed_response_with_cert[] = static void test_decodeOCSPBasicSignedResponseInfo(DWORD dwEncoding) { OCSP_BASIC_SIGNED_RESPONSE_INFO *info; + OCSP_BASIC_RESPONSE_INFO *b; DWORD size; BOOL ret; @@ -8875,12 +8876,25 @@ static void test_decodeOCSPBasicSignedResponseInfo(DWORD dwEncoding) ok(!info->SignatureInfo.cCertEncoded, "got %lu\n", info->SignatureInfo.cCertEncoded); ok(!info->SignatureInfo.rgCertEncoded, "got %p\n", info->SignatureInfo.rgCertEncoded); + + + ret = CryptDecodeObjectEx(dwEncoding, OCSP_BASIC_RESPONSE, info->ToBeSigned.pbData, info->ToBeSigned.cbData, + CRYPT_DECODE_ALLOC_FLAG, NULL, &b, &size); + ok(ret, "got %08lx\n", GetLastError()); + ok(!b->cExtension, "got %lu.\n", b->cExtension); + LocalFree(b); LocalFree(info); size = 0; ret = CryptDecodeObjectEx(dwEncoding, OCSP_BASIC_SIGNED_RESPONSE, ocsp_basic_signed_response_with_cert, sizeof(ocsp_basic_signed_response_with_cert), CRYPT_DECODE_ALLOC_FLAG, NULL, &info, &size); ok(ret, "got %08lx\n", GetLastError()); + + ret = CryptDecodeObjectEx(dwEncoding, OCSP_BASIC_RESPONSE, info->ToBeSigned.pbData, info->ToBeSigned.cbData, + CRYPT_DECODE_ALLOC_FLAG, NULL, &b, &size); + ok(ret, "got %08lx\n", GetLastError()); + ok(b->cExtension == 1, "got %lu.\n", b->cExtension); + LocalFree(b); LocalFree(info); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7112
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/decode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/crypt32/decode.c b/dlls/crypt32/decode.c index 44877a6e9d6..2deacc1cb15 100644 --- a/dlls/crypt32/decode.c +++ b/dlls/crypt32/decode.c @@ -6507,7 +6507,7 @@ static BOOL CRYPT_AsnDecodeOCSPBasicResponseEntry(const BYTE *pbEncoded, DWORD c { ASN_CONTEXT | ASN_CONSTRUCTOR, offsetof(OCSP_BASIC_RESPONSE_ENTRY, NextUpdate), CRYPT_AsnDecodeOCSPNextUpdate, sizeof(FILETIME), TRUE, FALSE, 0, 0 }, - { ASN_CONTEXT | ASN_CONSTRUCTOR /* FIXME */, offsetof(OCSP_BASIC_RESPONSE_ENTRY, cExtension), + { ASN_CONTEXT | ASN_CONSTRUCTOR | 1 /* FIXME */, offsetof(OCSP_BASIC_RESPONSE_ENTRY, cExtension), CRYPT_AsnDecodeCertExtensions, FINALMEMBERSIZE(OCSP_BASIC_RESPONSE_ENTRY, cExtension), TRUE, TRUE, offsetof(OCSP_BASIC_RESPONSE_ENTRY, rgExtension), 0 }, }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7112
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/cryptnet/cryptnet_main.c | 72 ++++++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 19 deletions(-) diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c index 1068dd26868..ed4add512e6 100644 --- a/dlls/cryptnet/cryptnet_main.c +++ b/dlls/cryptnet/cryptnet_main.c @@ -2028,11 +2028,12 @@ static DWORD handle_ocsp_response(const CERT_INFO *cert, const CERT_INFO *issuer static DWORD verify_cert_revocation_with_ocsp(const CERT_CONTEXT *cert, const WCHAR *base_url, const CERT_REVOCATION_PARA *revpara, FILETIME *next_update) { - HINTERNET ses, con, req = NULL; + HINTERNET ses = NULL, con = NULL, req = NULL; BYTE *request_data = NULL, *response_data = NULL; - DWORD size, flags, status, request_len, response_len, count, ret = CRYPT_E_REVOCATION_OFFLINE; + DWORD size, status, request_len, response_len, count, ret = CRYPT_E_REVOCATION_OFFLINE; + DWORD flags = INTERNET_FLAG_KEEP_CONNECTION; URL_COMPONENTSW comp; - WCHAR *url; + WCHAR *url = NULL; if (!revpara || !revpara->pIssuerCert) { @@ -2043,8 +2044,11 @@ static DWORD verify_cert_revocation_with_ocsp(const CERT_CONTEXT *cert, const WC return CRYPT_E_REVOCATION_OFFLINE; url = build_request_url(base_url, request_data, request_len); - LocalFree(request_data); - if (!url) return CRYPT_E_REVOCATION_OFFLINE; + if (!url) + { + ret = CRYPT_E_REVOCATION_OFFLINE; + goto done; + } memset(&comp, 0, sizeof(comp)); comp.dwStructSize = sizeof(comp); @@ -2052,31 +2056,33 @@ static DWORD verify_cert_revocation_with_ocsp(const CERT_CONTEXT *cert, const WC comp.dwUrlPathLength = ~0u; if (!InternetCrackUrlW(url, 0, 0, &comp)) { - free(url); - return CRYPT_E_REVOCATION_OFFLINE; + ret = CRYPT_E_REVOCATION_OFFLINE; + goto done; } switch (comp.nScheme) { case INTERNET_SCHEME_HTTP: - flags = 0; break; case INTERNET_SCHEME_HTTPS: - flags = INTERNET_FLAG_SECURE; + flags |= INTERNET_FLAG_SECURE; break; default: FIXME("scheme %u not supported\n", comp.nScheme); - free(url); - return ERROR_NOT_SUPPORTED; + ret = ERROR_NOT_SUPPORTED; + goto done; } - if (!(ses = InternetOpenW(L"CryptoAPI", 0, NULL, NULL, 0))) return GetLastError(); + if (!(ses = InternetOpenW(L"CryptoAPI", 0, NULL, NULL, 0))) + { + ret = GetLastError(); + goto done; + } comp.lpszHostName[comp.dwHostNameLength] = 0; if (!(con = InternetConnectW(ses, comp.lpszHostName, comp.nPort, NULL, NULL, INTERNET_SERVICE_HTTP, 0, 0))) { - free(url); - InternetCloseHandle(ses); - return GetLastError(); + ret = GetLastError(); + goto done; } comp.lpszHostName[comp.dwHostNameLength] = '/'; if (!(req = HttpOpenRequestW(con, NULL, comp.lpszUrlPath, NULL, NULL, NULL, flags, 0)) || @@ -2084,13 +2090,40 @@ static DWORD verify_cert_revocation_with_ocsp(const CERT_CONTEXT *cert, const WC size = sizeof(status); if (!HttpQueryInfoW(req, HTTP_QUERY_STATUS_CODE | HTTP_QUERY_FLAG_NUMBER, &status, &size, NULL)) goto done; - if (status != HTTP_STATUS_OK) + if (status == HTTP_STATUS_OK) { - WARN("request status %lu\n", status); - goto done; + size = sizeof(response_len); + if (!HttpQueryInfoW(req, HTTP_QUERY_FLAG_NUMBER | HTTP_QUERY_CONTENT_LENGTH, &response_len, &size, 0) || + !response_len || !(response_data = malloc(response_len)) || + !InternetReadFile(req, response_data, response_len, &count) || count != response_len) goto done; + + ret = handle_ocsp_response(cert->pCertInfo, revpara->pIssuerCert->pCertInfo, response_data, response_len, + next_update); } + if (ret == ERROR_SUCCESS || ret == CRYPT_E_REVOKED) goto done; - size = sizeof(response_len); + WARN("GET OCSP request failed, status %lu, ret %#lx, retrying with POST.\n", status, ret); + InternetCloseHandle(req); + req = NULL; + free(response_data); + response_data = NULL; + memset(&comp, 0, sizeof(comp)); + comp.dwStructSize = sizeof(comp); + comp.dwHostNameLength = ~0u; + comp.dwUrlPathLength = ~0u; + if (!InternetCrackUrlW(base_url, 0, 0, &comp)) + { + ret = CRYPT_E_REVOCATION_OFFLINE; + goto done; + } + flags &= ~INTERNET_FLAG_KEEP_CONNECTION; + if (!(req = HttpOpenRequestW(con, L"POST", comp.lpszUrlPath, NULL, NULL, NULL, flags, 0)) || + !HttpSendRequestW(req, L"Content-Type: application/ocsp-request\0", -1, request_data, request_len)) goto done; + if (status != HTTP_STATUS_OK) + { + WARN("request status %lu.\n", status); + goto done; + } if (!HttpQueryInfoW(req, HTTP_QUERY_FLAG_NUMBER | HTTP_QUERY_CONTENT_LENGTH, &response_len, &size, 0) || !response_len || !(response_data = malloc(response_len)) || !InternetReadFile(req, response_data, response_len, &count) || count != response_len) goto done; @@ -2099,6 +2132,7 @@ static DWORD verify_cert_revocation_with_ocsp(const CERT_CONTEXT *cert, const WC next_update); done: + LocalFree(request_data); free(url); free(response_data); InternetCloseHandle(req); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7112
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/cryptnet/cryptnet_main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c index ed4add512e6..1200416af58 100644 --- a/dlls/cryptnet/cryptnet_main.c +++ b/dlls/cryptnet/cryptnet_main.c @@ -2161,7 +2161,15 @@ static DWORD verify_cert_revocation_from_aia_ext(const CRYPT_DATA_BLOB *value, c { const WCHAR *url = aia->rgAccDescr[i].AccessLocation.pwszURL; TRACE("OCSP URL = %s\n", debugstr_w(url)); - error = verify_cert_revocation_with_ocsp(cert, url, pRevPara, next_update); + if (dwFlags & CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION) + { + TRACE("Cache only revocation, returning CRYPT_E_REVOCATION_OFFLINE.\n"); + error = CRYPT_E_REVOCATION_OFFLINE; + } + else + { + error = verify_cert_revocation_with_ocsp(cert, url, pRevPara, next_update); + } } else { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7112
This helps Sea of Thieves and Master Chief Collection which timeout in Xbox login window on some instances (probably dependent on region / servers which might be using different Xbox servers and certificates). This is technically a regression although probably not from the latest development cycle. So I am not entirely sure if this is for code freeze, there are some rather extensive changes but leaving it to Hans for final judgement. There are a few things going on there. Firstly, the certificates revocation checks through OCSP always fail, which is not fatal for login window functioning but causes the revocation requests not to be cached but evaluated each time (making the core issue, when reproducible, always happen on consequent attempts). There are two problems: 1. For one of the two concerned certificates, OCSP server (http://oneocsp.microsoft.com/ocsp) returns bogus OCSP response (the data length is one byte short for the outermost ASN sequence). This response can't be decoded by openssl program and also is marked as malformed by Wireshark. That only happens when OCSP request is sent using GET http verb and works fine with POST. The attached test program (which has problematic certificate data embedded) allows to see what happens on Windows when recording the traffic with Wineshark. Windows sents request with GET just like we do, and also receives similar bogus OCSP response. But then Windows falls back to POST request and gets the normal response. Patch 3 implements that (keep alive connection on first GET request is also the case on Windows). 2. Then, the two certificates (one of those is the same as in p. 1. embedded in the attached test program) fails to parse (while now it is correct). That is addressed by patch 2 (see [1], Page 9, SingleResponse definition, there is tag "[1]" for singleExtensions but current implementation assumes "[0]"). Patch 1 addresses similar issue with different response extensions and has embedded test. I didn't embedded for patch 2 because it is a bit involved generating such a certificate with openssl and the change corresponds to spec, and it is also tested by attached test program (while may try to generate such a test certificate if that is deemed necessary, or include MS server response in Wine test for parsing if it is not deemed undesirable). Then, the core problem I think which might be the major trigger of the issues on some instances is that the certificate revocation check is invoked with CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION, but we ignore that in OCSP (thus it might be taking a lot of time and lead to app's timeout if OCSP server is unavailable while the app explicitly requests cache-only check). This looks sort of flaky for embedded test because we'd need some dedicated test OCSP server for that, OCSP servers in the wild tend to change (e. g., that happened with the currently present revocation OCSP check, the server replies with "access denied" OCSP response while the test still succeeds through CRL fallback). This is also tested in the attached program (which manually clears certificate revocation check cache). 1. http://oneocsp.microsoft.com/ocsp [revoccheck.c](/uploads/118e1d3e30b0f6c72505bdfa6d72fc57/revoccheck.c) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112#note_91614
Then, the core problem I think which might be the major trigger of the issues on some instances is that the certificate revocation check is invoked with CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION, but we ignore that in OCSP
Wouldn't respecting the cache-only flag cause side effects though (as seen in https://bugs.winehq.org/show_bug.cgi?id=56559 and Rustup/Discord updater)? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112#note_91633
Bug 56559 is about verification using CRLs, not OCSP. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112#note_91634
I guess you mean that the introduction of OCSP support caused the regression. Before that we would fall back to checking a cached CRL. The first version of OCSP support didn't have cache support and when it was added we didn't honor the CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION flag. Patches look good but I think this should wait until the code freeze is over. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112#note_91635
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112
I guess you mean that the introduction of OCSP support caused the regression.
I think so, yes. It is reported to work fine on the old Proton versions. I didn't bisect exactly but from how these changes help that looks most likely. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112#note_91649
On Wed Jan 8 16:33:10 2025 +0000, Aida Jonikienė wrote:
Then, the core problem I think which might be the major trigger of the issues on some instances is that the certificate revocation check is invoked with CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION, but we ignore that in OCSP Wouldn't respecting the cache-only flag cause side effects though (as seen in https://bugs.winehq.org/show_bug.cgi?id=56559 and Rustup/Discord updater)? Besides what Hans mentioned (the bug concerns CRL verification which I am not touching here and not OCSP) it seems weird if the application uses CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION and then depends firmly on that check to succeed (as in not returning _REVOCATION_OFFLINE). The dependency on a certificate being frequently checked outside of the app is weird by itself, wonder what that cert is so it is supposed to be guaranteed to be checked just by Windows boot. Even if certificate is frequently checked (even, e. g., by Windows itself on boot), the cached revocation status can be invalidated any time (due to expiration or due to cache cleanup, the cache is AppDataLow), so that should be sometimes failing on Windows too. So it seems to me that the bug (if still valid??) could use a bit more exploration. Maybe something else is going wrong. First check would be clearing certificates revocation cache on Windows right before running the program (e. g., between the lines of this: https: //mssec.wordpress.com/2013/04/09/delete-local-crl-cache-in-windows/). If success really depends solely on the cert revocation check being present in cache (which I doubt a bit and suspect there might be more to it) and that certificate is really something Windows always checks on boot at least then maybe the correct (while unfortunate) solution would be to think of some way of pre-populating the cache of select certificates on Wine boot. Or the program is just frequently broken on Windows too.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112#note_91651
On Wed Jan 8 16:33:10 2025 +0000, Paul Gofman wrote:
Besides what Hans mentioned (the bug concerns CRL verification which I am not touching here and not OCSP) it seems weird if the application uses CERT_VERIFY_CACHE_ONLY_BASED_REVOCATION and then depends firmly on that check to succeed (as in not returning _REVOCATION_OFFLINE). The dependency on a certificate being frequently checked outside of the app is weird by itself, wonder what that cert is so it is supposed to be guaranteed to be checked just by Windows boot. Even if certificate is frequently checked (even, e. g., by Windows itself on boot), the cached revocation status can be invalidated any time (due to expiration or due to cache cleanup, the cache is AppDataLow), so that should be sometimes failing on Windows too. So it seems to me that the bug (if still valid??) could use a bit more exploration. Maybe something else is going wrong. First check would be clearing certificates revocation cache on Windows right before running the program (e. g., between the lines of this: https://mssec.wordpress.com/2013/04/09/delete-local-crl-cache-in-windows/). If success really depends solely on the cert revocation check being present in cache (which I doubt a bit and suspect there might be more to it) and that certificate is really something Windows always checks on boot at least then maybe the correct (while unfortunate) solution would be to think of some way of pre-populating the cache of select certificates on Wine boot. Or the program is just frequently broken on Windows too. I took a look at Bug 56559 and found that the actual problem seems to be in CertVerifyCertificateChainPolicy() (not handling the related flags specified by the launcher to ignore revocation errors), while revocation errors themselves can easily happen on Windows too. I dropped a comment there and probably going to prepare a separate patch for that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7112#note_91662
participants (4)
-
Aida Jonikienė -
Hans Leidekker (＠hans)
-
Paul Gofman
-
Paul Gofman (＠gofman)