[PATCH v3 0/1] MR11268: crypt32: Avoid conflict between unnamed containers
Part 5 of my attempt to run the [Niko Home Control programming software](https://appdb.winehq.org/objectManager.php?sClass=application&iId=21635) under Wine. Fixes the following bug: 1. First PFXImport: CryptAcquireContextW(NULL container, CRYPT_NEWKEYSET) -\> CSP creates container "blegat" (the default user name) and imports key K1 into it. set_key_prov_info then writes pwszContainerName="blegat" onto cert1. 2. Second PFXImport: same call fails with NTE_EXISTS (the default container already exists), the existing retry opens it without CRYPT_NEWKEYSET, then CryptImportKey writes K2 into that container, overwriting K1. set_key_prov_info writes pwszContainerName="blegat" onto cert2. 3. State: container "blegat" holds K2. Cert1's CRYPT_KEY_PROV_INFO.pwszContainerName still says "blegat". Anyone asking "what's cert1's private key?" reads its container name, opens "blegat", and gets K2, the second cert's key. I followed the UUID creation pattern from `CRYPT_CreateKeyProv` to stay consistent with existing code in Wine. I added a test, without the change, the tests fail with ``` store.c:1107: Test marked todo: CertOpenStore failed: 00000006 store.c:505: Test marked todo: Cert was not saved in AppData at 3 (3) store.c:2139: Test marked todo: Store registration (dwFlags=00040000) failed, last error 0 store.c:2143: Tests skipped: Nothing to test without registered store at 00040000 store.c:2139: Test marked todo: Store registration (dwFlags=00090000) failed, last error 0 store.c:2143: Tests skipped: Nothing to test without registered store at 00090000 store.c:389: Test marked todo: file store -> system store: expected size 188, got 156 store.c:398: Test marked todo: file store -> system store: unexpected value store.c:2864: Test marked todo: Unexpected cert3 store.c:3583: Test failed: two PFX imports collided into the same container L"blegat" â the second import would have overwritten the first cert's private key 0020:store: 651 tests executed (7 marked as todo, 0 as flaky, 1 failure), 2 skipped. ``` And after the fix, the tests pass ``` store.c:1107: Test marked todo: CertOpenStore failed: 00000006 store.c:505: Test marked todo: Cert was not saved in AppData at 3 (3) store.c:2139: Test marked todo: Store registration (dwFlags=00040000) failed, last error 0 store.c:2143: Tests skipped: Nothing to test without registered store at 00040000 store.c:2139: Test marked todo: Store registration (dwFlags=00090000) failed, last error 0 store.c:2143: Tests skipped: Nothing to test without registered store at 00090000 store.c:389: Test marked todo: file store -> system store: expected size 188, got 156 store.c:398: Test marked todo: file store -> system store: unexpected value store.c:2864: Test marked todo: Unexpected cert3 0020:store: 651 tests executed (7 marked as todo, 0 as flaky, 0 failures), 2 skipped. ``` -- v3: crypt32: Avoid conflict between unnamed containers https://gitlab.winehq.org/wine/wine/-/merge_requests/11268
From: Benoît Legat <benoit.legat@gmail.com> 1. PFXImport #1: CryptAcquireContextW(NULL container, CRYPT_NEWKEYSET) -> CSP creates container "blegat" (the default user name) and imports key K1 into it. set_key_prov_info then writes pwszContainerName="blegat" onto cert1. 2. PFXImport #2: same call fails with NTE_EXISTS (the default container already exists), the existing retry opens it without CRYPT_NEWKEYSET, then CryptImportKey writes K2 into that container, overwriting K1. set_key_prov_info writes pwszContainerName="blegat" onto cert2. 3. State: container "blegat" holds K2. Cert1's CRYPT_KEY_PROV_INFO.pwszContainerName still says "blegat". Anyone asking "what's cert1's private key?" reads its container name, opens "blegat", and gets K2, the second cert's key. Create GUID using UuidCreate, UuidToStringW and RpcStringFreeW from rpcrt4 --- dlls/crypt32/Makefile.in | 2 +- dlls/crypt32/cert.c | 64 +++++++++++++--------------------- dlls/crypt32/crypt32_private.h | 5 +++ dlls/crypt32/pfx.c | 29 +++++++++------ dlls/crypt32/tests/store.c | 56 +++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 52 deletions(-) diff --git a/dlls/crypt32/Makefile.in b/dlls/crypt32/Makefile.in index 9af443bfb36..9a0254150aa 100644 --- a/dlls/crypt32/Makefile.in +++ b/dlls/crypt32/Makefile.in @@ -3,7 +3,7 @@ MODULE = crypt32.dll UNIXLIB = crypt32.so IMPORTLIB = crypt32 IMPORTS = user32 advapi32 bcrypt ncrypt -DELAYIMPORTS = cryptnet +DELAYIMPORTS = cryptnet rpcrt4 UNIX_LIBS = $(SECURITY_LIBS) UNIX_CFLAGS = $(GNUTLS_CFLAGS) diff --git a/dlls/crypt32/cert.c b/dlls/crypt32/cert.c index c37432822a2..19a3e482c16 100644 --- a/dlls/crypt32/cert.c +++ b/dlls/crypt32/cert.c @@ -3759,53 +3759,37 @@ static void CRYPT_MakeCertInfo(PCERT_INFO info, const CRYPT_DATA_BLOB *pSerialNu } } -typedef RPC_STATUS (RPC_ENTRY *UuidCreateFunc)(UUID *); -typedef RPC_STATUS (RPC_ENTRY *UuidToStringFunc)(UUID *, unsigned char **); -typedef RPC_STATUS (RPC_ENTRY *RpcStringFreeFunc)(unsigned char **); - -static HCRYPTPROV CRYPT_CreateKeyProv(void) +WCHAR *CRYPT32_AllocateUniqueContainerName(void) { - HCRYPTPROV hProv = 0; - HMODULE rpcrt = LoadLibraryW(L"rpcrt4"); + UUID uuid; + RPC_WSTR uuid_str = NULL; + WCHAR *ret = NULL; + RPC_STATUS status; - if (rpcrt) - { - UuidCreateFunc uuidCreate = (UuidCreateFunc)GetProcAddress(rpcrt, - "UuidCreate"); - UuidToStringFunc uuidToString = (UuidToStringFunc)GetProcAddress(rpcrt, - "UuidToStringA"); - RpcStringFreeFunc rpcStringFree = (RpcStringFreeFunc)GetProcAddress( - rpcrt, "RpcStringFreeA"); - - if (uuidCreate && uuidToString && rpcStringFree) - { - UUID uuid; - RPC_STATUS status = uuidCreate(&uuid); + status = UuidCreate( &uuid ); + if (status != RPC_S_OK && status != RPC_S_UUID_LOCAL_ONLY) return NULL; + if (UuidToStringW( &uuid, &uuid_str ) != RPC_S_OK) return NULL; - if (status == RPC_S_OK || status == RPC_S_UUID_LOCAL_ONLY) - { - unsigned char *uuidStr; + if ((ret = CryptMemAlloc( (lstrlenW( (WCHAR *)uuid_str ) + 1) * sizeof(WCHAR) ))) + lstrcpyW( ret, (WCHAR *)uuid_str ); + RpcStringFreeW( &uuid_str ); + return ret; +} - status = uuidToString(&uuid, &uuidStr); - if (status == RPC_S_OK) - { - BOOL ret = CryptAcquireContextA(&hProv, (LPCSTR)uuidStr, - MS_DEF_PROV_A, PROV_RSA_FULL, CRYPT_NEWKEYSET); +static HCRYPTPROV CRYPT_CreateKeyProv(void) +{ + HCRYPTPROV hProv = 0; + WCHAR *container = CRYPT32_AllocateUniqueContainerName(); - if (ret) - { - HCRYPTKEY key; + if (!container) return 0; - ret = CryptGenKey(hProv, AT_SIGNATURE, 0, &key); - if (ret) - CryptDestroyKey(key); - } - rpcStringFree(&uuidStr); - } - } - } - FreeLibrary(rpcrt); + if (CryptAcquireContextW( &hProv, container, MS_DEF_PROV_W, PROV_RSA_FULL, CRYPT_NEWKEYSET )) + { + HCRYPTKEY key; + if (CryptGenKey( hProv, AT_SIGNATURE, 0, &key )) + CryptDestroyKey( key ); } + CryptMemFree( container ); return hProv; } diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h index 7b6bdfa9df7..9de0c9f9548 100644 --- a/dlls/crypt32/crypt32_private.h +++ b/dlls/crypt32/crypt32_private.h @@ -26,6 +26,11 @@ BOOL CNG_ImportPubKey(CERT_PUBLIC_KEY_INFO *pubKeyInfo, BCRYPT_KEY_HANDLE *key); BOOL cng_prepare_signature(const char *alg_oid, BYTE *encoded_sig, DWORD encoded_sig_len, BYTE **sig_value, DWORD *sig_len); +/* Returns a freshly-allocated, NUL-terminated UUID string suitable for use + * as a CSP key container name. Caller frees with CryptMemFree. Returns + * NULL on failure. */ +WCHAR *CRYPT32_AllocateUniqueContainerName(void); + /* a few asn.1 tags we need */ #define ASN_BOOL (ASN_UNIVERSAL | ASN_PRIMITIVE | 0x01) #define ASN_BITSTRING (ASN_UNIVERSAL | ASN_PRIMITIVE | 0x03) diff --git a/dlls/crypt32/pfx.c b/dlls/crypt32/pfx.c index b7419e4f446..a4a493e41ba 100644 --- a/dlls/crypt32/pfx.c +++ b/dlls/crypt32/pfx.c @@ -38,20 +38,23 @@ static HCRYPTPROV import_key( cert_store_data_t data, DWORD flags ) DWORD size, acquire_flags; void *key; struct import_store_key_params params = { data, NULL, &size }; + /* Use a unique container name per import. With a NULL container + + * CRYPT_NEWKEYSET the CSP falls back to its per-user default container + * ("<USERNAME>"), so a second PFX import overwrites the first import's + * private key inside that shared container; any cert from the first + * import still held will then dereference (via CRYPT_KEY_PROV_INFO) + * to the second import's key. */ + WCHAR *container = CRYPT32_AllocateUniqueContainerName(); - if (CRYPT32_CALL( import_store_key, ¶ms ) != STATUS_BUFFER_TOO_SMALL) return 0; + if (!container) return 0; + + if (CRYPT32_CALL( import_store_key, ¶ms ) != STATUS_BUFFER_TOO_SMALL) goto done; acquire_flags = (flags & CRYPT_MACHINE_KEYSET) | CRYPT_NEWKEYSET; - if (!CryptAcquireContextW( &prov, NULL, MS_ENHANCED_PROV_W, PROV_RSA_FULL, acquire_flags )) + if (!CryptAcquireContextW( &prov, container, MS_ENHANCED_PROV_W, PROV_RSA_FULL, acquire_flags )) { - if (GetLastError() != NTE_EXISTS) return 0; - - acquire_flags &= ~CRYPT_NEWKEYSET; - if (!CryptAcquireContextW( &prov, NULL, MS_ENHANCED_PROV_W, PROV_RSA_FULL, acquire_flags )) - { - WARN( "CryptAcquireContextW failed %08lx\n", GetLastError() ); - return 0; - } + WARN( "CryptAcquireContextW failed %08lx\n", GetLastError() ); + goto done; } params.buf = key = CryptMemAlloc( size ); @@ -60,11 +63,15 @@ static HCRYPTPROV import_key( cert_store_data_t data, DWORD flags ) { WARN( "CryptImportKey failed %08lx\n", GetLastError() ); CryptReleaseContext( prov, 0 ); + prov = 0; CryptMemFree( key ); - return 0; + goto done; } CryptDestroyKey( cryptkey ); CryptMemFree( key ); + +done: + CryptMemFree( container ); return prov; } diff --git a/dlls/crypt32/tests/store.c b/dlls/crypt32/tests/store.c index 10b861675ca..81da1fad4e4 100644 --- a/dlls/crypt32/tests/store.c +++ b/dlls/crypt32/tests/store.c @@ -3541,6 +3541,61 @@ static void test_PFXImportCertStore(void) CertCloseStore( store, 0 ); } +static void test_PFXImportCertStore_unique_containers(void) +{ + /* Two persistent (CRYPT_USER_KEYSET) PFX imports of the *same* PKCS#12 + * blob must land in two *distinct* CSP key containers; otherwise the + * second import overwrites the first import's private key inside the + * shared container, and any cert still held from the first import + * then dereferences (via CRYPT_KEY_PROV_INFO) to the second import's + * key. The container-name format is internal to the importer; only + * its uniqueness across calls is observable to applications, and that + * uniqueness is what this test checks. */ + + CRYPT_DATA_BLOB pfx = { sizeof(pfxdata), (BYTE *)pfxdata }; + BYTE buf1[512], buf2[512]; + CRYPT_KEY_PROV_INFO *info1 = (CRYPT_KEY_PROV_INFO *)buf1; + CRYPT_KEY_PROV_INFO *info2 = (CRYPT_KEY_PROV_INFO *)buf2; + HCERTSTORE store1, store2; + const CERT_CONTEXT *cert1, *cert2; + DWORD size; + BOOL ret; + + store1 = PFXImportCertStore( &pfx, NULL, CRYPT_EXPORTABLE | CRYPT_USER_KEYSET ); + ok( store1 != NULL, "first PFXImportCertStore failed: %lu\n", GetLastError() ); + store2 = PFXImportCertStore( &pfx, NULL, CRYPT_EXPORTABLE | CRYPT_USER_KEYSET ); + ok( store2 != NULL, "second PFXImportCertStore failed: %lu\n", GetLastError() ); + if (!store1 || !store2) goto done; + + cert1 = CertFindCertificateInStore( store1, X509_ASN_ENCODING, 0, CERT_FIND_ANY, NULL, NULL ); + cert2 = CertFindCertificateInStore( store2, X509_ASN_ENCODING, 0, CERT_FIND_ANY, NULL, NULL ); + ok( cert1 != NULL, "cert1 lookup failed: %08lx\n", GetLastError() ); + ok( cert2 != NULL, "cert2 lookup failed: %08lx\n", GetLastError() ); + if (!cert1 || !cert2) goto done_close; + + size = sizeof(buf1); + ret = CertGetCertificateContextProperty( cert1, CERT_KEY_PROV_INFO_PROP_ID, info1, &size ); + ok( ret, "cert1 has no KEY_PROV_INFO: %08lx\n", GetLastError() ); + size = sizeof(buf2); + ret = CertGetCertificateContextProperty( cert2, CERT_KEY_PROV_INFO_PROP_ID, info2, &size ); + ok( ret, "cert2 has no KEY_PROV_INFO: %08lx\n", GetLastError() ); + if (!info1->pwszContainerName || !info2->pwszContainerName) goto done_certs; + + ok( wcscmp( info1->pwszContainerName, info2->pwszContainerName ) != 0, + "two PFX imports collided into the same container %s — the second " + "import would have overwritten the first cert's private key\n", + wine_dbgstr_w( info1->pwszContainerName ) ); + +done_certs: + CertFreeCertificateContext( cert1 ); + CertFreeCertificateContext( cert2 ); +done_close: + CertCloseStore( store1, 0 ); + CertCloseStore( store2, 0 ); +done: + ; +} + static void test_PFXExportCertStoreEx(void) { HCERTSTORE store, store2; @@ -3769,6 +3824,7 @@ START_TEST(store) test_I_UpdateStore(); test_PFXImportCertStore(); + test_PFXImportCertStore_unique_containers(); test_PFXExportCertStoreEx(); test_CryptQueryObject(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11268
On Mon Jun 29 14:35:32 2026 +0000, Benoît Legat wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/11268/diffs?diff_id=278226&start_sha=e1f28705b481509d11230152619ef643e66b177b#984b53f695649e171b4f0fa11c26b8ea77b94b26_54_45) Good idea, just made the change, squashing it directly into the existing commit
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11268#note_144440
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11268
participants (3)
-
Benoît Legat -
Benoît Legat (＠blegat)
-
Hans Leidekker (＠hans)