[PATCH 0/1] MR10439: crypt32: Protect certificate context from repeated releases.
From: Dmitry Timoshkov <dmitry@baikal.ru> A buggy application does something like this: PCCERT_CONTEXT cert, prev = NULL; while ((cert = CertEnumCertificatesInStore(store, prev))) { do_something_with_cert(cert); CertFreeCertificateContext(cert); prev = cert; } CertCloseStore(store); <= assert(!cert->ref) beacuse cert->ref == -1 which leads to a crash because of an assert(). Similar code works under Windows, however it's not clear how this could be properly added as a test case because of potential use after free. Also, adding a 'prev->ref' check to Context_Release() doesn't seem to be correct since Context_Release() is used outside of the lock in other callers. Signed-off-by: Dmitry Timoshkov <dmitry@baikal.ru> --- dlls/crypt32/store.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index 54b16ab8fa1..bca4dfd2986 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -219,7 +219,7 @@ static context_t *MemStore_enumContext(WINE_MEMSTORE *store, struct list *list, EnterCriticalSection(&store->cs); if (prev) { next = list_next(list, &prev->u.entry); - Context_Release(prev); + if (prev->ref) Context_Release(prev); }else { next = list_head(list); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10439
Do you still see this after 3b47d25b0b45ea103f8127209503f9687cea9d4a? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135778
On Fri Apr 10 07:10:02 2026 +0000, Hans Leidekker wrote:
Do you still see this after 3b47d25b0b45ea103f8127209503f9687cea9d4a? Yes, that fix is not related to the described here behaviour of a buggy application.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135780
On Fri Apr 10 07:10:02 2026 +0000, Dmitry Timoshkov wrote:
Yes, that fix is not related to the described here behaviour of a buggy application. What is the call sequence that leads to this crash?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135781
On Fri Apr 10 07:12:13 2026 +0000, Hans Leidekker wrote:
What is the call sequence that leads to this crash? Quoting https://gitlab.winehq.org/wine/wine/-/merge_requests/10439/diffs?commit_id=0...
A buggy application does something like this: PCCERT_CONTEXT cert, prev = NULL; while ((cert = CertEnumCertificatesInStore(store, prev))) { do_something_with_cert(cert); CertFreeCertificateContext(cert); prev = cert; } CertCloseStore(store); <= assert(!cert->ref) beacuse cert->ref == -1 which leads to a crash because of an assert(). Similar code works under Windows, however it's not clear how this could be properly added as a test case because of potential use after free. Also, adding a 'prev->ref' check to Context_Release() doesn't seem to be correct since Context_Release() is used outside of the lock in other callers. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135783
On Fri Apr 10 07:19:39 2026 +0000, Dmitry Timoshkov wrote:
Quoting https://gitlab.winehq.org/wine/wine/-/merge_requests/10439/diffs?commit_id=0... A buggy application does something like this: PCCERT_CONTEXT cert, prev = NULL; while ((cert = CertEnumCertificatesInStore(store, prev))) { do_something_with_cert(cert); CertFreeCertificateContext(cert); prev = cert; } CertCloseStore(store); <= assert(!cert->ref) beacuse cert->ref == -1 which leads to a crash because of an assert(). Similar code works under Windows, however it's not clear how this could be properly added as a test case because of potential use after free. Also, adding a 'prev->ref' check to Context_Release() doesn't seem to be correct since Context_Release() is used outside of the lock in other callers. Can you provide more detail? This translation of your example crashes on Windows:
static void test_crash(void)
{
PCCERT_CONTEXT cert, prev = NULL;
HCERTSTORE store = CertOpenSystemStoreW( 0, L"My" );
while ((cert = CertEnumCertificatesInStore( store, prev )))
{
trace( "%p\n", cert );
CertFreeCertificateContext( cert );
prev = cert;
}
CertCloseStore( store, 0 );
}
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135785
On Fri Apr 10 07:52:57 2026 +0000, Hans Leidekker wrote:
Can you provide more detail? This translation of your example crashes on Windows: ``` static void test_crash(void) { PCCERT_CONTEXT cert, prev = NULL; HCERTSTORE store = CertOpenSystemStoreW( 0, L"My" ); while ((cert = CertEnumCertificatesInStore( store, prev ))) { trace( "%p\n", cert ); CertFreeCertificateContext( cert ); prev = cert; } CertCloseStore( store, 0 ); } ``` [enum_store.tar.7z](/uploads/0f768f14d34423da91fe100f038f4572/enum_store.tar.7z) Attached test should replicate the assert().
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135813
On Fri Apr 10 12:46:02 2026 +0000, Dmitry Timoshkov wrote:
[enum_store.tar.7z](/uploads/0f768f14d34423da91fe100f038f4572/enum_store.tar.7z) Attached test should replicate the assert(). This appears to be specific to the PKCS7 store. I don't see a crash if I substitute a file or registry store.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135938
On Mon Apr 13 08:35:10 2026 +0000, Hans Leidekker wrote:
This appears to be specific to the PKCS7 store. I don't see a crash if I substitute a file or registry store. How would you suggest to fix it?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135940
On Mon Apr 13 08:57:45 2026 +0000, Dmitry Timoshkov wrote:
How would you suggest to fix it? This suggests that we are either failing to reference the contexts somewhere in the PKCS7 path or releasing them too early.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135941
On Mon Apr 13 09:08:25 2026 +0000, Hans Leidekker wrote:
This suggests that we are either failing to reference the contexts somewhere in the PKCS7 path or releasing them too early. This is clearly an application bug, and crashing in the case for file or registry store confirms that. I'd guess that the application developers didn't test their code with something different than PKCS7 store, otherwise they would notice the bug and fixed it. It looks like Windows has a different code path for a PKCS7 store, while in Wine we have the unified code for all types of stores, and making the PKCS7 store not crash on close would mean a lot of code duplication for little gain, so I'd propose to accept this patch.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_135942
What would be the best way to proceed here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_137076
On Mon Apr 13 09:17:49 2026 +0000, Dmitry Timoshkov wrote:
This is clearly an application bug, and crashing in the case for file or registry store confirms that. I'd guess that the application developers didn't test their code with something different than PKCS7 store, otherwise they would notice the bug and fixed it. It looks like Windows has a different code path for a PKCS7 store, while in Wine we have the unified code for all types of stores, and making the PKCS7 store not crash on close would mean a lot of code duplication for little gain, so I'd propose to accept this patch. Like I said, this suggests that we are either failing to reference the contexts somewhere in the PKCS7 path or releasing them too early. So we need to investigate that and fix it if this hypothesis turns out to be correct.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_137077
On Mon Apr 20 08:36:47 2026 +0000, Hans Leidekker wrote:
Like I said, this suggests that we are either failing to reference the contexts somewhere in the PKCS7 path or releasing them too early. So we need to investigate that and fix it if this hypothesis turns out to be correct. You seem to not agree that it's the application bug? Why?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_137080
On Mon Apr 20 08:42:41 2026 +0000, Dmitry Timoshkov wrote:
You seem to not agree that it's the application bug? Why? Hans, I'd appreciate to hear your opinion on this.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_138168
On Wed Apr 29 10:16:22 2026 +0000, Dmitry Timoshkov wrote:
Hans, I'd appreciate to hear your opinion on this. Just in case to make things more clear about the application bug: https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cer... in the Remarks section states: The returned pointer is freed when passed as the pPrevCertContext parameter on a subsequent call. Otherwise, the pointer must be freed by calling CertFreeCertificateContext. A non-NULL pPrevCertContext passed to CertEnumCertificatesInStore is always freed even for an error.
Under Windows the sample code crashes for "My" store, but doesn't crash for an PKCS7 store, that probably implies that PKCS7 code path is different in Windows implementation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10439#note_138180
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Hans Leidekker (＠hans)