[PATCH 0/5] MR7377: crypt32: Resync system store when using chain engine cache.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/tests/chain.c | 66 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index 5522ab4f01b..7310027284c 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -3506,6 +3506,8 @@ static void checkChainStatus(PCCERT_CHAIN_CONTEXT chain, } } +/* Tuesday, May 1, 2007 */ +static SYSTEMTIME may2007 = { 2007, 5, 2, 1, 0, 0, 0, 0 }; /* Wednesday, Oct 1, 2007 */ static SYSTEMTIME oct2007 = { 2007, 10, 1, 1, 0, 0, 0, 0 }; /* Wednesday, Oct 28, 2009 */ @@ -5506,6 +5508,65 @@ static void test_VerifyCertChainPolicy_flags(void) CertCloseStore(store, 0); } +static void test_chain_engine_cache_update(void) +{ + CERT_CHAIN_PARA para = { 0 }; + PCCERT_CHAIN_CONTEXT chain; + PCCERT_CONTEXT cert, cert2; + FILETIME filetime; + HCERTSTORE store, store2; + BOOL ret; + + cert = CertCreateCertificateContext(X509_ASN_ENCODING, selfSignedCert, sizeof(selfSignedCert)); + ok(!!cert, "got NULL.\n"); + + SystemTimeToFileTime(&may2007, &filetime); + + ret = CertGetCertificateChain(HCCE_CURRENT_USER, cert, &filetime, NULL, ¶, CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY, NULL, &chain); + ok(ret, "got error %#lx.\n", GetLastError()); + ok(chain->TrustStatus.dwErrorStatus == CERT_TRUST_IS_UNTRUSTED_ROOT, "got %#lx.\n", chain->TrustStatus.dwErrorStatus); + CertFreeCertificateChain(chain); + + store2 = CertOpenStore(CERT_STORE_PROV_SYSTEM_W, 0, 0, CERT_SYSTEM_STORE_CURRENT_USER, L"Root"); + ok(!!store2, "got NULL.\n"); + + store = CertOpenStore(CERT_STORE_PROV_SYSTEM_W, 0, 0, CERT_SYSTEM_STORE_CURRENT_USER, L"Root"); + ok(!!store, "got NULL.\n"); + ret = CertAddCertificateContextToStore(store, cert, CERT_STORE_ADD_ALWAYS, NULL); + ok(ret, "got error %#lx.\n", GetLastError()); + + cert2 = CertFindCertificateInStore(store2, X509_ASN_ENCODING, 0, CERT_FIND_EXISTING, cert, NULL); + ok(!cert2, "got NULL.\n"); + CertFreeCertificateContext(cert2); + + ret = CertGetCertificateChain(HCCE_CURRENT_USER, cert, &filetime, NULL, ¶, CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY, NULL, &chain); + ok(ret, "got error %#lx.\n", GetLastError()); + todo_wine ok(!chain->TrustStatus.dwErrorStatus, "got %#lx.\n", chain->TrustStatus.dwErrorStatus); + CertFreeCertificateChain(chain); + + ret = CertCloseStore(store2, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); + + ret = CertCloseStore(store, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); + + ret = CertGetCertificateChain(HCCE_CURRENT_USER, cert, &filetime, NULL, ¶, CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY, NULL, &chain); + ok(ret, "got error %#lx.\n", GetLastError()); + todo_wine ok(!chain->TrustStatus.dwErrorStatus, "got %#lx.\n", chain->TrustStatus.dwErrorStatus); + CertFreeCertificateChain(chain); + + store = CertOpenStore(CERT_STORE_PROV_SYSTEM_W, 0, 0, CERT_SYSTEM_STORE_CURRENT_USER, L"Root"); + ok(!!store, "got NULL.\n"); + cert2 = CertFindCertificateInStore(store, X509_ASN_ENCODING, 0, CERT_FIND_EXISTING, cert, NULL); + ok(!!cert2, "got NULL.\n"); + ret = CertDeleteCertificateFromStore(cert2); + ok(ret, "got error %#lx.\n", GetLastError()); + ret = CertCloseStore(store, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); + + CertFreeCertificateContext(cert); +} + START_TEST(chain) { testCreateCertChainEngine(); @@ -5513,4 +5574,9 @@ START_TEST(chain) testGetCertChain(); test_CERT_CHAIN_PARA_cbSize(); test_VerifyCertChainPolicy_flags(); + if (winetest_interactive) + { + /* This test will pop up user confirmation dialogs on Windows. */ + test_chain_engine_cache_update(); + } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7377
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/store.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index 537f7821013..ee07ad58994 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -214,12 +214,14 @@ static context_t *MemStore_enumContext(WINE_MEMSTORE *store, struct list *list, static BOOL MemStore_deleteContext(WINE_MEMSTORE *store, context_t *context) { + EnterCriticalSection(&store->cs); if (!context->deleted_from_store) { context->deleted_from_store = TRUE; if (!context->ref) memstore_free_context(context); } + LeaveCriticalSection(&store->cs); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7377
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/crypt32_private.h | 10 ++++- dlls/crypt32/store.c | 68 +++++++++++++++++++++++++--------- 2 files changed, 58 insertions(+), 20 deletions(-) diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h index 6548dd36ab0..f5918ef0359 100644 --- a/dlls/crypt32/crypt32_private.h +++ b/dlls/crypt32/crypt32_private.h @@ -254,7 +254,10 @@ typedef BOOL (WINAPI *SetContextPropertyFunc)(const void *context, DWORD dwPropID, DWORD dwFlags, const void *pvData); typedef BOOL (WINAPI *SerializeElementFunc)(const void *context, DWORD dwFlags, BYTE *pbElement, DWORD *pcbElement); -typedef BOOL (WINAPI *DeleteContextFunc)(const void *contex); +typedef BOOL (WINAPI *DeleteContextFromStoreFunc)(const void *contex); +typedef const void * (*FindContextByContextFunc)(HCERTSTORE store, const void *context); +typedef const void * (WINAPI *DuplicateContextFunc)(const void *context); +typedef void (WINAPI *FreeContextFunc)(const void *context); /* An abstract context (certificate, CRL, or CTL) interface */ typedef struct _WINE_CONTEXT_INTERFACE @@ -267,7 +270,10 @@ typedef struct _WINE_CONTEXT_INTERFACE GetContextPropertyFunc getProp; SetContextPropertyFunc setProp; SerializeElementFunc serialize; - DeleteContextFunc deleteFromStore; + DeleteContextFromStoreFunc deleteFromStore; + FindContextByContextFunc findContextByContext; + DuplicateContextFunc duplicateContext; + FreeContextFunc freeContext; } WINE_CONTEXT_INTERFACE; extern const WINE_CONTEXT_INTERFACE *pCertInterface; diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index ee07ad58994..0a96a337eab 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -37,6 +37,29 @@ WINE_DEFAULT_DEBUG_CHANNEL(crypt); +static const void *cert_find_context_by_context(HCERTSTORE store, const void *context) +{ + BYTE hash[20]; + DWORD size = sizeof(hash); + CRYPT_HASH_BLOB blob = { sizeof(hash), hash }; + const CERT_CONTEXT *cert = context; + + if (!CertGetCertificateContextProperty(cert, CERT_HASH_PROP_ID, hash, &size)) + return NULL; + + return CertFindCertificateInStore(store, cert->dwCertEncodingType, 0, CERT_FIND_SHA1_HASH, &blob, NULL); +} + +static const void *crl_find_context_by_context(HCERTSTORE store, const void *context) +{ + return CertFindCRLInStore(store, 0, 0, CRL_FIND_EXISTING, context, NULL); +} + +static const void *ctl_find_context_by_context(HCERTSTORE store, const void *context) +{ + return CertFindCTLInStore(store, 0, 0, CTL_FIND_EXISTING, context, NULL); +} + static const WINE_CONTEXT_INTERFACE gCertInterface = { (CreateContextFunc)CertCreateCertificateContext, (AddContextToStoreFunc)CertAddCertificateContextToStore, @@ -46,7 +69,10 @@ static const WINE_CONTEXT_INTERFACE gCertInterface = { (GetContextPropertyFunc)CertGetCertificateContextProperty, (SetContextPropertyFunc)CertSetCertificateContextProperty, (SerializeElementFunc)CertSerializeCertificateStoreElement, - (DeleteContextFunc)CertDeleteCertificateFromStore, + (DeleteContextFromStoreFunc)CertDeleteCertificateFromStore, + cert_find_context_by_context, + (DuplicateContextFunc)CertDuplicateCertificateContext, + (FreeContextFunc)CertFreeCertificateContext, }; const WINE_CONTEXT_INTERFACE *pCertInterface = &gCertInterface; @@ -59,7 +85,10 @@ static const WINE_CONTEXT_INTERFACE gCRLInterface = { (GetContextPropertyFunc)CertGetCRLContextProperty, (SetContextPropertyFunc)CertSetCRLContextProperty, (SerializeElementFunc)CertSerializeCRLStoreElement, - (DeleteContextFunc)CertDeleteCRLFromStore, + (DeleteContextFromStoreFunc)CertDeleteCRLFromStore, + crl_find_context_by_context, + (DuplicateContextFunc)CertDuplicateCRLContext, + (FreeContextFunc)CertFreeCRLContext, }; const WINE_CONTEXT_INTERFACE *pCRLInterface = &gCRLInterface; @@ -72,7 +101,10 @@ static const WINE_CONTEXT_INTERFACE gCTLInterface = { (GetContextPropertyFunc)CertGetCTLContextProperty, (SetContextPropertyFunc)CertSetCTLContextProperty, (SerializeElementFunc)CertSerializeCTLStoreElement, - (DeleteContextFunc)CertDeleteCTLFromStore, + (DeleteContextFromStoreFunc)CertDeleteCTLFromStore, + ctl_find_context_by_context, + (DuplicateContextFunc)CertDuplicateCTLContext, + (FreeContextFunc)CertFreeCTLContext, }; const WINE_CONTEXT_INTERFACE *pCTLInterface = &gCTLInterface; @@ -118,24 +150,24 @@ BOOL WINAPI I_CertUpdateStore(HCERTSTORE store1, HCERTSTORE store2, DWORD unk0, warned = TRUE; } - /* Poor-man's resync: empty first store, then add everything from second - * store to it. - */ for (i = 0; i < ARRAY_SIZE(interfaces); i++) { - const void *context; + const void *context, *context2; - do { - context = interfaces[i]->enumContextsInStore(store1, NULL); - if (context) - interfaces[i]->deleteFromStore(context); - } while (context); - do { - context = interfaces[i]->enumContextsInStore(store2, context); - if (context) - interfaces[i]->addContextToStore(store1, context, - CERT_STORE_ADD_ALWAYS, NULL); - } while (context); + context = NULL; + while ((context = interfaces[i]->enumContextsInStore(store1, context))) + { + if ((context2 = interfaces[i]->findContextByContext(store2, context))) + { + interfaces[i]->freeContext(context2); + continue; + } + context2 = interfaces[i]->duplicateContext(context); + interfaces[i]->deleteFromStore(context2); + } + context = NULL; + while ((context = interfaces[i]->enumContextsInStore(store2, context))) + interfaces[i]->addContextToStore(store1, context, CERT_STORE_ADD_REPLACE_EXISTING, NULL); } return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7377
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/regstore.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/dlls/crypt32/regstore.c b/dlls/crypt32/regstore.c index 218a7ea3269..fc3e85854ce 100644 --- a/dlls/crypt32/regstore.c +++ b/dlls/crypt32/regstore.c @@ -43,6 +43,7 @@ typedef struct _WINE_REGSTOREINFO struct list certsToDelete; struct list crlsToDelete; struct list ctlsToDelete; + HANDLE key_modified_event; } WINE_REGSTOREINFO; void CRYPT_HashToStr(const BYTE *hash, LPWSTR asciiHash) @@ -324,6 +325,7 @@ static void WINAPI CRYPT_RegCloseStore(HCERTSTORE hCertStore, DWORD dwFlags) CRYPT_RegFlushStore(store, FALSE); RegCloseKey(store->key); + CloseHandle(store->key_modified_event); store->cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&store->cs); CryptMemFree(store); @@ -461,13 +463,20 @@ static BOOL WINAPI CRYPT_RegControl(HCERTSTORE hCertStore, DWORD dwFlags, { case CERT_STORE_CTRL_RESYNC: { - HCERTSTORE memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, 0, - CERT_STORE_CREATE_NEW_FLAG, NULL); + HCERTSTORE memStore; + DWORD ret; CRYPT_RegFlushStore(store, FALSE); - CRYPT_RegReadFromReg(store->key, memStore, CERT_STORE_ADD_REPLACE_EXISTING); - I_CertUpdateStore(store->memStore, memStore, 0, 0); - CertCloseStore(memStore, 0); + if ((ret = WaitForSingleObject(store->key_modified_event, 0)) != WAIT_TIMEOUT) + { + memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, 0, CERT_STORE_CREATE_NEW_FLAG, NULL); + if (RegNotifyChangeKeyValue(store->key, TRUE, REG_NOTIFY_CHANGE_NAME | REG_NOTIFY_CHANGE_LAST_SET, + store->key_modified_event, TRUE)) + ERR("RegNotifyChangeKeyValue failed.\n"); + CRYPT_RegReadFromReg(store->key, memStore, CERT_STORE_ADD_REPLACE_EXISTING); + I_CertUpdateStore(store->memStore, memStore, 0, 0); + CertCloseStore(memStore, 0); + } break; } case CERT_STORE_CTRL_COMMIT: @@ -553,6 +562,10 @@ WINECRYPT_CERTSTORE *CRYPT_RegOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags, list_init(®Info->certsToDelete); list_init(®Info->crlsToDelete); list_init(®Info->ctlsToDelete); + regInfo->key_modified_event = CreateEventW(NULL, FALSE, FALSE, NULL); + if (RegNotifyChangeKeyValue(regInfo->key, TRUE, REG_NOTIFY_CHANGE_NAME | REG_NOTIFY_CHANGE_LAST_SET, + regInfo->key_modified_event, TRUE)) + ERR("RegNotifyChangeKeyValue failed.\n"); CRYPT_RegReadFromReg(regInfo->key, regInfo->memStore, CERT_STORE_ADD_ALWAYS); regInfo->dirty = FALSE; provInfo.cbSize = sizeof(provInfo); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7377
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/chain.c | 4 ++-- dlls/crypt32/tests/chain.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/crypt32/chain.c b/dlls/crypt32/chain.c index e7c7afe9147..b99b5729332 100644 --- a/dlls/crypt32/chain.c +++ b/dlls/crypt32/chain.c @@ -173,7 +173,7 @@ static CertificateChainEngine *get_chain_engine(HCERTCHAINENGINE handle, BOOL al if(default_cu_engine != handle) CertFreeCertificateChainEngine(handle); } - + CertControlStore(default_cu_engine->hWorld, 0, CERT_STORE_CTRL_RESYNC, NULL); return default_cu_engine; } @@ -187,7 +187,7 @@ static CertificateChainEngine *get_chain_engine(HCERTCHAINENGINE handle, BOOL al if(default_lm_engine != handle) CertFreeCertificateChainEngine(handle); } - + CertControlStore(default_lm_engine->hWorld, 0, CERT_STORE_CTRL_RESYNC, NULL); return default_lm_engine; } diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index 7310027284c..4ea91e1bba4 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -5552,7 +5552,7 @@ static void test_chain_engine_cache_update(void) ret = CertGetCertificateChain(HCCE_CURRENT_USER, cert, &filetime, NULL, ¶, CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY, NULL, &chain); ok(ret, "got error %#lx.\n", GetLastError()); - todo_wine ok(!chain->TrustStatus.dwErrorStatus, "got %#lx.\n", chain->TrustStatus.dwErrorStatus); + ok(!chain->TrustStatus.dwErrorStatus, "got %#lx.\n", chain->TrustStatus.dwErrorStatus); CertFreeCertificateChain(chain); store = CertOpenStore(CERT_STORE_PROV_SYSTEM_W, 0, 0, CERT_SYSTEM_STORE_CURRENT_USER, L"Root"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7377
This fixes Battle.net error displaying error when running initial setup (which also prevents auto start of Battle.net after install but then installed Battle.net still works). This is related to the same Battle.net ephemeral certificate added to root store, but now that hits yet a different issue during initial setup. CertGetCertificateChain() is using cached chain engines in Wine which has root store open only once when CertGetCertificateChain() is first called with this engine. Thus, if root store is updated after CertGetCertificateChain() was once called that update won't be seen in the consequently called CertGetCertificateChain() in the same process. Battle.net setup seems to always add the certificate before checking that it works with CertGetCertificateChain() but depending probably on some timing or else factors another CertGetCertificateChain() may be called before that on a different thread which "latches" root store cache in the state before the cert in question is added. I didn't debug why it wasn't happening before or what this sequence depends on because CertGetCertificateChain() should obviously use up to date root store (that is also confir med by the included test). Patches "crypt32: Lock store in MemStore_deleteContext().", "crypt32: Do not temporary delete contexts in I_CertUpdateStore()." should ensure that we can safely update registry stores in the cached engines without additional locking while those may be concurrently used in CertGetCertificateChain() from another thread. The access to underlying mem store is synchronized, and updating or deleting certificate from I_CertUpdateStore should not be breaking existing in-progress enumerations after 489d95d3bb48e6febc8b1ee6fbbe1d7ed0181ace . Before "crypt32: Do not temporary delete contexts in I_CertUpdateStore()." temporary deletion could fail racing CertGetCertificateChain as some existing certs could be gone missing. While if we delete only what we need to delete and update those with need to be updated or added the concurrent enumeration should behave correctly. Without "crypt32: Only sync registry store if registry has changed." adding CertControlStore(..., CERT_STORE_CTRL_RESYNC, ...); to get_chain_engine() would be as good as just getting rid of the cache (which is probably there for reasons; reading and de-serializing all the certs from registry takes a lot of time). The remaining todo_wine in test is for the case when CertGetCertificateChain() is called after cert was added to root store but before that store was explicitly flushed or closed. It seems like that behaves differently on Windows and probably writes to registry right away without waiting for explicit flush on close, but this looks unrelated to the issue and patches concerned here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7377#note_95280
participants (2)
-
Paul Gofman -
Paul Gofman (＠gofman)