[PATCH v2 0/5] MR7377: crypt32: Resync system store when using chain engine cache.
-- v2: crypt32: Resync world store for default engines in get_chain_engine(). crypt32: Only sync registry store if registry has changed. crypt32: Do not temporary delete contexts in I_CertUpdateStore(). crypt32: Lock store in MemStore_deleteContext(). crypt32/tests: Add test for chain engine cache update. https://gitlab.winehq.org/wine/wine/-/merge_requests/7377
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 | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/dlls/crypt32/regstore.c b/dlls/crypt32/regstore.c index 218a7ea3269..bee906e422a 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,22 @@ 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; + EnterCriticalSection(&store->cs); 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); + } + LeaveCriticalSection(&store->cs); break; } case CERT_STORE_CTRL_COMMIT: @@ -553,6 +564,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
v2: - take regstore critical section in CRYPT_RegControl() / CERT_STORE_CTRL_RESYNC to avoid potentially racing resyncs ending up with not really up to date state with registry notifications consumed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7377#note_95284
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7377
participants (3)
-
Hans Leidekker (＠hans) -
Paul Gofman
-
Paul Gofman (＠gofman)