[PATCH v2 0/1] MR11268: crypt32: Avoid conflict between unnamed containers

[Benoît Legat (＠blegat)]

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.
```

--
  v2: crypt32: Avoid conflict between unnamed containers
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11268

[PATCH v2 1/1] crypt32: Avoid conflict between unnamed containers

[Benoît Legat]

From: Benoît Legat <[e-mail]>

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.
---
 dlls/crypt32/pfx.c         | 50 ++++++++++++++++++++++++++--------
 dlls/crypt32/tests/store.c | 56 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 12 deletions(-)

diff --git a/dlls/crypt32/pfx.c b/dlls/crypt32/pfx.c
index b7419e4f446..9d5952e4fcc 100644
--- a/dlls/crypt32/pfx.c
+++ b/dlls/crypt32/pfx.c
@@ -25,12 +25,17 @@
 #include "bcrypt.h"
 #include "ncrypt.h"
 #include "snmp.h"
+#include "rpc.h"
 #include "crypt32_private.h"
 
 #include "wine/debug.h"
 
 WINE_DEFAULT_DEBUG_CHANNEL(crypt);
 
+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 import_key( cert_store_data_t data, DWORD flags )
 {
     HCRYPTPROV prov = 0;
@@ -38,20 +43,36 @@ 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 };
-
-    if (CRYPT32_CALL( import_store_key, ¶ms ) != STATUS_BUFFER_TOO_SMALL) return 0;
+    /* 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 find the second import's key when its
+     * CRYPT_KEY_PROV_INFO is followed back to the container. Follow the
+     * same UuidCreate-via-rpcrt4 pattern that CRYPT_CreateKeyProv
+     * (cert.c) uses for the same problem on the synthesise-a-key path. */
+    HMODULE rpcrt = LoadLibraryW( L"rpcrt4" );
+    UuidCreateFunc uuidCreate;
+    UuidToStringFunc uuidToString;
+    RpcStringFreeFunc rpcStringFree;
+    UUID uuid;
+    unsigned char *uuid_str = NULL;
+
+    if (!rpcrt) return 0;
+    uuidCreate    = (UuidCreateFunc)   GetProcAddress( rpcrt, "UuidCreate" );
+    uuidToString  = (UuidToStringFunc) GetProcAddress( rpcrt, "UuidToStringA" );
+    rpcStringFree = (RpcStringFreeFunc)GetProcAddress( rpcrt, "RpcStringFreeA" );
+    if (!uuidCreate || !uuidToString || !rpcStringFree) goto done;
+    if (uuidCreate( &uuid ) != RPC_S_OK && uuidCreate( &uuid ) != RPC_S_UUID_LOCAL_ONLY) goto done;
+    if (uuidToString( &uuid, &uuid_str ) != RPC_S_OK) goto done;
+
+    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 (!CryptAcquireContextA( &prov, (LPCSTR)uuid_str, MS_ENHANCED_PROV_A, 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( "CryptAcquireContextA failed %08lx\n", GetLastError() );
+        goto done;
     }
 
     params.buf = key = CryptMemAlloc( size );
@@ -60,11 +81,16 @@ 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:
+    if (uuid_str) rpcStringFree( &uuid_str );
+    FreeLibrary( rpcrt );
     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

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/crypt32/pfx.c:
> DWORD size, acquire_flags;
>      void *key;
>      struct import_store_key_params params = { data, NULL, &size };
> -
> -    if (CRYPT32_CALL( import_store_key, ¶ms ) != STATUS_BUFFER_TOO_SMALL) return 0;
> +    /* 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 find the second import's key when its
> +     * CRYPT_KEY_PROV_INFO is followed back to the container. Follow the
> +     * same UuidCreate-via-rpcrt4 pattern that CRYPT_CreateKeyProv
> +     * (cert.c) uses for the same problem on the synthesise-a-key path. */
> +    HMODULE rpcrt = LoadLibraryW( L"rpcrt4" );

It would be better to add rpcrt4 to DELAYIMPORTS in the makefile and create a helper shared by CRYPT_CreateKeyProv() and this function. Please use wide character functions.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11268#note_144424