rsaenh: fix critical bug in read_key_container

[Mounir IDRASSI]

Hi,
We have found a critical bug in rsaenh dll. This bug prevents
CryptAcquireContext from correctly loading the key associated with the
container from the registry, and thus CryptGetUserKey become unusable.
This bug is located in the internal function read_key_container.
Attached with this email is a patch correcting this bug and adding a
test that demonstrate it.

Cheers,
Mounir IDRASSI
IDRIX - Cryptography and IT Security Experts
http://www.idrix.fr
> From 8da78d912c0a709a641ae4082ee6df270f644a08 Mon Sep 17 00:00:00 2001

From: Mounir IDRASSI 
Date: Mon, 7 May 2007 17:04:46 +0200
Subject: rsaenh: fix critical bug in read_key_container

---
 dlls/rsaenh/rsaenh.c       |   11 +++++++----
 dlls/rsaenh/tests/rsaenh.c |   19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/dlls/rsaenh/rsaenh.c b/dlls/rsaenh/rsaenh.c
index 2ace2d5..7723157 100644
--- a/dlls/rsaenh/rsaenh.c
+++ b/dlls/rsaenh/rsaenh.c
@@ -1050,6 +1050,7 @@ static HCRYPTPROV read_key_container(PCHAR pszContainerName, DWORD dwFlags, PVTa
     KEYCONTAINER *pKeyContainer;
     HCRYPTPROV hKeyContainer;
     DATA_BLOB blobIn, blobOut;
+    HCRYPTKEY hCryptKey;

sprintf(szRSABase, RSAENH_REGKEY, pszContainerName);

@@ -1089,8 +1090,9 @@ static HCRYPTPROV read_key_container(PCHAR pszContainerName, DWORD dwFlags, PVTa
                     if (CryptUnprotectData(&blobIn, NULL, NULL, NULL, NULL, 
                          (dwFlags & CRYPT_MACHINE_KEYSET) ? CRYPTPROTECT_LOCAL_MACHINE : 0, &blobOut))
                     {
-                        RSAENH_CPImportKey(hKeyContainer, blobOut.pbData, blobOut.cbData, 0, 0,
-                                           &pKeyContainer->hKeyExchangeKeyPair);
+                        if(RSAENH_CPImportKey(hKeyContainer, blobOut.pbData, blobOut.cbData, 0, 0,
+                                           &hCryptKey))
+			    pKeyContainer->hKeyExchangeKeyPair = hCryptKey;
                         HeapFree(GetProcessHeap(), 0, blobOut.pbData);
                     }
                 }
@@ -1113,8 +1115,9 @@ static HCRYPTPROV read_key_container(PCHAR pszContainerName, DWORD dwFlags, PVTa
                     if (CryptUnprotectData(&blobIn, NULL, NULL, NULL, NULL, 
                          (dwFlags & CRYPT_MACHINE_KEYSET) ? CRYPTPROTECT_LOCAL_MACHINE : 0, &blobOut))
                     {
-                        RSAENH_CPImportKey(hKeyContainer, blobOut.pbData, blobOut.cbData, 0, 0,
-                                           &pKeyContainer->hSignatureKeyPair);
+                        if(RSAENH_CPImportKey(hKeyContainer, blobOut.pbData, blobOut.cbData, 0, 0,
+                                           &hCryptKey))
+			    pKeyContainer->hSignatureKeyPair = hCryptKey;
                         HeapFree(GetProcessHeap(), 0, blobOut.pbData);
                     }
                 }
diff --git a/dlls/rsaenh/tests/rsaenh.c b/dlls/rsaenh/tests/rsaenh.c
index fe27f4b..0832e0f 100644
--- a/dlls/rsaenh/tests/rsaenh.c
+++ b/dlls/rsaenh/tests/rsaenh.c
@@ -1603,6 +1603,25 @@ static void test_null_provider(void)

CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL,
      CRYPT_DELETEKEYSET);
+    
+	
+    /* test for the bug in accessing the user key in a container
+     */
+    result = CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL,
+     CRYPT_NEWKEYSET);
+    ok(result, "CryptAcquireContext failed: %08x\n", GetLastError());
+    result = CryptGenKey(prov, AT_KEYEXCHANGE, 0, &key);
+    ok(result, "CryptGenKey with AT_KEYEXCHANGE failed with error %08x\n", GetLastError());
+    CryptDestroyKey(key); 
+    CryptReleaseContext(prov,0);
+    result = CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL,0);
+    ok(result, "CryptAcquireContext failed: 0x%08x\n", GetLastError());    
+    result = CryptGetUserKey(prov, AT_KEYEXCHANGE, &key);
+    ok (result, "CryptGetUserKey failed with error %08x\n", GetLastError());    
+    CryptDestroyKey(key); 
+    
+    CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL,
+     CRYPT_DELETEKEYSET);          
 }

START_TEST(rsaenh)

[Stefan Dösinger]

Hi,
Patches should go to [e-mail] :-)

[Mounir IDRASSI]

yes but I prefer sending patches first to the devel list for comments.
Once no major issue is raised, I send it to the patches list. I think
this helps prevent polluting the patches list.

Mounir IDRASSI
IDRIX - Cryptography and IT Security Experts
http://www.idrix.fr

Stefan Dösinger wrote:
> Hi,
> Patches should go to [e-mail] :-)

[Vitaliy Margolen]

No, that adds more pollution to the place which should not have it.
Please send all patches to [e-mail]. If you think that
all of your patches need work, then don't send them at all. Do this
extra work. And when you have a patch that _you_ think is as good as it
can be, send it to [e-mail].

Vitaliy Margolen.

Mounir IDRASSI wrote:
> yes but I prefer sending patches first to the devel list for comments.
> Once no major issue is raised, I send it to the patches list. I think
> this helps prevent polluting the patches list.
>
> Mounir IDRASSI
> IDRIX - Cryptography and IT Security Experts
> http://www.idrix.fr
>
> Stefan Dösinger wrote:
> > Hi,
> > Patches should go to [e-mail] :-)

[Mounir IDRASSI]

I'll take you advice from now and I apologize for any annoyance caused
by my previous emails.
I would also thank all the people who took time to review our patches
and sent me their comments/remarks.

Regards,
Mounir IDRASSI
IDRIX - Cryptography and IT Security Experts
http://www.idrix.fr

Vitaliy Margolen wrote:
> No, that adds more pollution to the place which should not have it.
> Please send all patches to [e-mail]. If you think that
> all of your patches need work, then don't send them at all. Do this
> extra work. And when you have a patch that _you_ think is as good as it
> can be, send it to [e-mail].
>
> Vitaliy Margolen.