Hi, We have found few bugs in the MS Enhanced CSP implementation in wine (rsaenh.dll) and you'll find attached a patch that corrects them. Here is a description of the problems we found: - In RSAENH_CPAcquireContext : when specifying CRYPT_DELETEKEYSET, the function always deletes the container for the local user, while it should check the CRYPT_MACHINE_KEYSET flag to see if it should be deleted on the local machine or for the local user. - In RSAENH_CPGetProvParam: The following mandatory parameters were not supported : PP_UNIQUE_CONTAINER, PP_PROVTYPE, PP_KEYSPEC, PP_KEYSET_TYPE, PP_KEYSTORAGE.
In order to support the PP_KEYSTORAGE parameter, we added three defined to wincrypt.h.
Cheers, Mounir IDRASSI IDRIX - Cryptography and IT Security Experts http://www.idrix.fr
From a7240539a2e3c83291c7cf206d55f1e4e7e75803 Mon Sep 17 00:00:00 2001
From: Mounir IDRASSI mounir.idrassi@idrix.fr Date: Sat, 5 May 2007 19:01:57 +0200 Subject: rsaenh: fix bugs in RSAENH_CPAcquireContext and RSAENH_CPGetProvParam
--- dlls/rsaenh/rsaenh.c | 32 +++++++++++++++++++++++++++++++- include/wincrypt.h | 5 +++++ 2 files changed, 36 insertions(+), 1 deletions(-)
diff --git a/dlls/rsaenh/rsaenh.c b/dlls/rsaenh/rsaenh.c index 2ace2d5..9f55760 100644 --- a/dlls/rsaenh/rsaenh.c +++ b/dlls/rsaenh/rsaenh.c @@ -1473,7 +1473,13 @@ BOOL WINAPI RSAENH_CPAcquireContext(HCRYPTPROV *phProv, LPSTR pszContainer, SetLastError(NTE_BAD_KEYSET_PARAM); return FALSE; } else { - if (!RegDeleteKeyA(HKEY_CURRENT_USER, szRegKey)) { + HKEY hRootKey; + if (dwFlags & CRYPT_MACHINE_KEYSET) + hRootKey = HKEY_LOCAL_MACHINE; + else + hRootKey = HKEY_CURRENT_USER; + + if (!RegDeleteKeyA(hRootKey, szRegKey)) { SetLastError(ERROR_SUCCESS); return TRUE; } else { @@ -2899,6 +2905,7 @@ BOOL WINAPI RSAENH_CPGetProvParam(HCRYPTPROV hProv, DWORD dwParam, BYTE *pbData, switch (dwParam) { case PP_CONTAINER: + case PP_UNIQUE_CONTAINER:/* MSDN says we can return the same value as PP_CONTAINER */ return copy_param(pbData, pdwDataLen, (CONST BYTE*)pKeyContainer->szName, strlen(pKeyContainer->szName)+1);
@@ -2915,6 +2922,29 @@ BOOL WINAPI RSAENH_CPGetProvParam(HCRYPTPROV hProv, DWORD dwParam, BYTE *pbData, dwTemp = CRYPT_IMPL_SOFTWARE; return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp));
+ case PP_PROVTYPE: + dwTemp = PROV_RSA_FULL; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + + case PP_KEYSPEC: + dwTemp = AT_SIGNATURE | AT_KEYEXCHANGE; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + + case PP_KEYSET_TYPE: + dwTemp = pKeyContainer->dwFlags & CRYPT_MACHINE_KEYSET; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + + case PP_KEYSTORAGE: + dwTemp = GetVersion(); + /* for Windows NT, 95,98, Me, return CRYPT_PSTORE | CRYPT_UI_PROMPT | CRYPT_SEC_DESCR + * for the others, return CRYPT_SEC_DESCR + */ + if(dwTemp < 0x80000000 && ((dwTemp & 0x000000FF) != 0x00000004)) + dwTemp = CRYPT_SEC_DESCR; + else + dwTemp = CRYPT_PSTORE | CRYPT_UI_PROMPT | CRYPT_SEC_DESCR; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + case PP_VERSION: dwTemp = 0x00000200; return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); diff --git a/include/wincrypt.h b/include/wincrypt.h index 2219bd5..c897e96 100644 --- a/include/wincrypt.h +++ b/include/wincrypt.h @@ -1613,6 +1613,11 @@ static const WCHAR MS_SCARD_PROV_W[] = { 'M','i','c','r','o','s','o',' #define PP_KEYSPEC 39 #define PP_ENUMEX_SIGNING_PROT 40
+/* Values returned by CryptGetProvParam of PP_KEYSTORAGE */ +#define CRYPT_SEC_DESCR 0x00000001 +#define CRYPT_PSTORE 0x00000002 +#define CRYPT_UI_PROMPT 0x00000004 + /* Crypt{Get/Set}KeyParam */ #define KP_IV 1 #define KP_SALT 2
Mounir IDRASSI schreef:
We have found few bugs in the MS Enhanced CSP implementation in wine (rsaenh.dll) and you'll find attached a patch that corrects them.
Hello Mounir,
Bug fixes are always welcome in wine. But fixing bugs is one thing, but it is also very recommended to add tests to verify the behavior is correct. I don't see any tests added in this patch so it is hard to verify. Please also update dlls/rsaenh/tests with some simple tests if possible, it will make it more likely that the code will be accepted.
Cheers, Maarten
Hi, Thanks Maarte for your comment. Please find attached the patch augmented with some tests that illustrate what has been corrected.
Mounir IDRASSI IDRIX - Cryptography and IT Security Experts http://www.idrix.fr
Maarten Lankhorst wrote:
I don't see any tests added in this patch so it is hard to verify. Please also update dlls/rsaenh/tests with some simple tests if possible, it will make it more likely that the code will be accepted.
Cheers, Maarte
From 33a9a1f1573299c9b83c2d699dd4ac31f097a682 Mon Sep 17 00:00:00 2001
From: Mounir IDRASSI mounir.idrassi@idrix.fr Date: Sun, 6 May 2007 21:36:08 +0200 Subject: rsaenh: fix bugs in RSAENH_CPAcquireContext and RSAENH_CPGetProvParam
--- dlls/rsaenh/rsaenh.c | 32 ++++++++++++++++++++++- dlls/rsaenh/tests/rsaenh.c | 61 ++++++++++++++++++++++++++++++++++++++++--- include/wincrypt.h | 5 +++ 3 files changed, 92 insertions(+), 6 deletions(-)
diff --git a/dlls/rsaenh/rsaenh.c b/dlls/rsaenh/rsaenh.c index 2ace2d5..488b90c 100644 --- a/dlls/rsaenh/rsaenh.c +++ b/dlls/rsaenh/rsaenh.c @@ -1473,7 +1473,13 @@ BOOL WINAPI RSAENH_CPAcquireContext(HCRYPTPROV *phProv, LPSTR pszContainer, SetLastError(NTE_BAD_KEYSET_PARAM); return FALSE; } else { - if (!RegDeleteKeyA(HKEY_CURRENT_USER, szRegKey)) { + HKEY hRootKey; + if (dwFlags & CRYPT_MACHINE_KEYSET) + hRootKey = HKEY_LOCAL_MACHINE; + else + hRootKey = HKEY_CURRENT_USER; + + if (!RegDeleteKeyA(hRootKey, szRegKey)) { SetLastError(ERROR_SUCCESS); return TRUE; } else { @@ -2899,6 +2905,7 @@ BOOL WINAPI RSAENH_CPGetProvParam(HCRYPTPROV hProv, DWORD dwParam, BYTE *pbData, switch (dwParam) { case PP_CONTAINER: + case PP_UNIQUE_CONTAINER:/* MSDN says we can return the same value as PP_CONTAINER */ return copy_param(pbData, pdwDataLen, (CONST BYTE*)pKeyContainer->szName, strlen(pKeyContainer->szName)+1);
@@ -2906,6 +2913,29 @@ BOOL WINAPI RSAENH_CPGetProvParam(HCRYPTPROV hProv, DWORD dwParam, BYTE *pbData, return copy_param(pbData, pdwDataLen, (CONST BYTE*)pKeyContainer->szProvName, strlen(pKeyContainer->szProvName)+1);
+ case PP_PROVTYPE: + dwTemp = PROV_RSA_FULL; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + + case PP_KEYSPEC: + dwTemp = AT_SIGNATURE | AT_KEYEXCHANGE; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + + case PP_KEYSET_TYPE: + dwTemp = pKeyContainer->dwFlags & CRYPT_MACHINE_KEYSET; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + + case PP_KEYSTORAGE: + dwTemp = GetVersion(); + /* for Windows NT, 95,98, Me, return CRYPT_PSTORE | CRYPT_UI_PROMPT | CRYPT_SEC_DESCR + * for the others, return CRYPT_SEC_DESCR + */ + if(dwTemp < 0x80000000 && ((dwTemp & 0x000000FF) != 0x00000004)) + dwTemp = CRYPT_SEC_DESCR; + else + dwTemp = CRYPT_PSTORE | CRYPT_UI_PROMPT | CRYPT_SEC_DESCR; + return copy_param(pbData, pdwDataLen, (CONST BYTE*)&dwTemp, sizeof(dwTemp)); + case PP_SIG_KEYSIZE_INC: case PP_KEYX_KEYSIZE_INC: dwTemp = 8; diff --git a/dlls/rsaenh/tests/rsaenh.c b/dlls/rsaenh/tests/rsaenh.c index fe27f4b..dd7013b 100644 --- a/dlls/rsaenh/tests/rsaenh.c +++ b/dlls/rsaenh/tests/rsaenh.c @@ -1480,7 +1480,8 @@ static void test_null_provider(void) HCRYPTPROV prov; HCRYPTKEY key; BOOL result; - DWORD keySpec, dataLen; + DWORD keySpec, dataLen,dwParam; + char szName[MAX_PATH];
result = CryptAcquireContext(NULL, szContainer, NULL, 0, 0); ok(!result && GetLastError() == NTE_BAD_PROV_TYPE, @@ -1513,10 +1514,23 @@ static void test_null_provider(void) CRYPT_VERIFYCONTEXT); ok(result, "CryptAcquireContext failed: %08x\n", GetLastError()); if (!result) return; + /* Test provider parameters getter */ + dataLen = sizeof(dwParam); + result = CryptGetProvParam(prov, PP_PROVTYPE, (LPBYTE)&dwParam, &dataLen, 0); + ok(result && dataLen == sizeof(dwParam) && dwParam == PROV_RSA_FULL, + "Expected PROV_RSA_FULL, got 0x%08X\n",dwParam); + dataLen = sizeof(dwParam); + result = CryptGetProvParam(prov, PP_KEYSET_TYPE, (LPBYTE)&dwParam, &dataLen, 0); + ok(result && dataLen == sizeof(dwParam) && dwParam == 0, + "Expected 0, got 0x%08X\n",dwParam); + dataLen = sizeof(dwParam); + result = CryptGetProvParam(prov, PP_KEYSTORAGE, (LPBYTE)&dwParam, &dataLen, 0); + ok(result && dataLen == sizeof(dwParam) && (dwParam & CRYPT_SEC_DESCR), + "Expected CRYPT_SEC_DESCR to be set, got 0x%08X\n",dwParam); + dataLen = sizeof(keySpec); result = CryptGetProvParam(prov, PP_KEYSPEC, (LPBYTE)&keySpec, &dataLen, 0); - if (result) - ok(keySpec == (AT_KEYEXCHANGE | AT_SIGNATURE), + ok(result && keySpec == (AT_KEYEXCHANGE | AT_SIGNATURE), "Expected AT_KEYEXCHANGE | AT_SIGNATURE, got %08x\n", keySpec); /* Even though PP_KEYSPEC says both AT_KEYEXCHANGE and AT_SIGNATURE are * supported, you can't get the keys from this container. @@ -1558,9 +1572,21 @@ static void test_null_provider(void) if (!result) return; dataLen = sizeof(keySpec); result = CryptGetProvParam(prov, PP_KEYSPEC, (LPBYTE)&keySpec, &dataLen, 0); - if (result) - ok(keySpec == (AT_KEYEXCHANGE | AT_SIGNATURE), + ok(keySpec == (AT_KEYEXCHANGE | AT_SIGNATURE), "Expected AT_KEYEXCHANGE | AT_SIGNATURE, got %08x\n", keySpec); + /* PP_CONTAINER parameter */ + dataLen = sizeof(szName); + result = CryptGetProvParam(prov, PP_CONTAINER, (LPBYTE)szName, &dataLen, 0); + ok(result && dataLen == strlen(szContainer)+1 && strcmp(szContainer,szName) == 0, + "failed getting PP_CONTAINER. result = %s. Error 0x%08X. returned length = %d\n", + (result)? "TRUE":"FALSE",GetLastError(),dataLen); + /* PP_UNIQUE_CONTAINER parameter */ + dataLen = sizeof(szName); + result = CryptGetProvParam(prov, PP_UNIQUE_CONTAINER, (LPBYTE)szName, &dataLen, 0); + ok(result && dataLen == strlen(szContainer)+1 && strcmp(szContainer,szName) == 0, + "failed getting PP_CONTAINER. result = %s. Error 0x%08X. returned length = %d\n", + (result)? "TRUE":"FALSE",GetLastError(),dataLen); + result = CryptGetUserKey(prov, AT_KEYEXCHANGE, &key); ok(!result && GetLastError() == NTE_NO_KEY, "Expected NTE_NO_KEY, got %08x\n", GetLastError()); @@ -1603,6 +1629,31 @@ static void test_null_provider(void)
CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL, CRYPT_DELETEKEYSET); + + /* test the machine key set */ + CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL, + CRYPT_DELETEKEYSET|CRYPT_MACHINE_KEYSET); + result = CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL, + CRYPT_NEWKEYSET|CRYPT_MACHINE_KEYSET); + ok(result, "CryptAcquireContext with CRYPT_MACHINE_KEYSET failed: %08x\n", GetLastError()); + /* check PP_KEYSET_TYPE parameter */ + dataLen = sizeof(dwParam); + result = CryptGetProvParam(prov, PP_KEYSET_TYPE, (LPBYTE)&dwParam, &dataLen, 0); + ok(result && dataLen == sizeof(dwParam) && dwParam == CRYPT_MACHINE_KEYSET, + "Expected CRYPT_MACHINE_KEYSET, got 0x%08X\n",dwParam); + CryptReleaseContext(prov, 0); + result = CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL, + CRYPT_MACHINE_KEYSET); + ok(result, "CryptAcquireContext with CRYPT_MACHINE_KEYSET failed: %08x\n", GetLastError()); + CryptReleaseContext(prov,0); + result = CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL, + CRYPT_DELETEKEYSET|CRYPT_MACHINE_KEYSET); + ok(result, "CryptAcquireContext with CRYPT_DELETEKEYSET|CRYPT_MACHINE_KEYSET failed: %08x\n", + GetLastError()); + result = CryptAcquireContext(&prov, szContainer, NULL, PROV_RSA_FULL, + CRYPT_MACHINE_KEYSET); + ok(!result && GetLastError() == NTE_BAD_KEYSET , + "Expected NTE_BAD_KEYSET, got %08x\n", GetLastError()); }
START_TEST(rsaenh) diff --git a/include/wincrypt.h b/include/wincrypt.h index 2219bd5..2aaa89b 100644 --- a/include/wincrypt.h +++ b/include/wincrypt.h @@ -1613,6 +1613,11 @@ static const WCHAR MS_SCARD_PROV_W[] = { 'M','i','c','r','o','s','o',' #define PP_KEYSPEC 39 #define PP_ENUMEX_SIGNING_PROT 40
+/* Values returned by CryptGetProvParam of PP_KEYSTORAGE */ +#define CRYPT_SEC_DESCR 0x00000001 +#define CRYPT_PSTORE 0x00000002 +#define CRYPT_UI_PROMPT 0x00000004 + /* Crypt{Get/Set}KeyParam */ #define KP_IV 1 #define KP_SALT 2
On Sa, 2007-05-05 at 19:16 +0200, Mounir IDRASSI wrote:
--- a/include/wincrypt.h +++ b/include/wincrypt.h
+/* Values returned by CryptGetProvParam of PP_KEYSTORAGE */ +#define CRYPT_SEC_DESCR 0x00000001 +#define CRYPT_PSTORE 0x00000002 +#define CRYPT_UI_PROMPT 0x00000004
I have no Idea about the code, but the missing defines should be a seperate patch.
Your idention looks broken. Wine is using 4 Space, when possible.
Thanks for your reply. I'm rewriting the patches and sending them back.
Mounir IDRASSI IDRIX - Cryptography and IT Security Experts http://www.idrix.fr
Detlef Riekenberg wrote:
I have no Idea about the code, but the missing defines should be a seperate patch.
Your idention looks broken. Wine is using 4 Space, when possible.
On Mo, 2007-05-07 at 18:29 +0200, Mounir IDRASSI wrote:
Thanks for your reply. I'm rewriting the patches and sending them back.
Mounir IDRASSI IDRIX - Cryptography and IT Security Experts http://www.idrix.fr
Detlef Riekenberg wrote:
I have no Idea about the code, but the missing defines should be a seperate patch.
Your idention looks broken. Wine is using 4 Space, when possible.
An Idention of 4, while using Space... (as you can see in the other Code)
---example 1 -- #define PP_KEYSPEC 39 #define PP_ENUMEX_SIGNING_PROT 40
/* Values returned by CryptGetProvParam of PP_KEYSTORAGE */ #define CRYPT_SEC_DESCR 0x00000001 #define CRYPT_PSTORE 0x00000002 #define CRYPT_UI_PROMPT 0x00000004
/* Crypt{Get/Set}KeyParam */ #define KP_IV 1 #define KP_SALT 2 ------
---example 2 -- #define PP_KEYSPEC 39 #define PP_ENUMEX_SIGNING_PROT 40
/* Values returned by CryptGetProvParam of PP_KEYSTORAGE */ #define CRYPT_SEC_DESCR 0x00000001 #define CRYPT_PSTORE 0x00000002 #define CRYPT_UI_PROMPT 0x00000004
/* Crypt{Get/Set}KeyParam */ #define KP_IV 1 #define KP_SALT 2 ------
---example 3 -- #define PP_KEYSPEC 39 #define PP_ENUMEX_SIGNING_PROT 40
/* Values returned by CryptGetProvParam of PP_KEYSTORAGE */ #define CRYPT_SEC_DESCR 0x00000001 #define CRYPT_PSTORE 0x00000002 #define CRYPT_UI_PROMPT 0x00000004
/* Crypt{Get/Set}KeyParam */ #define KP_IV 1 #define KP_SALT 2 ------
Sorry for not type my Message so clean as needed. (I'm silent now ...)
-
Detlef Riekenberg -
Maarten Lankhorst -
Mounir IDRASSI