[PATCH] crypt32: CertGetCertificateContextProperty should not convert from store to certificate format CERT_KEY_PROV_INFO_PROP_ID property.

[Dmitry Timoshkov]

CRYPT_ReadContextProp() already does that when reading from the store.

I also took the opportunity to clean up CERT_KEY_PROV_INFO_PROP_ID helpers.

I apologize for the lack of proper testing previously that led to introducing
broken code, and only a partial fix in a recent patch.

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 dlls/crypt32/cert.c            | 85 +++++++++++-----------------------
 dlls/crypt32/crypt32_private.h | 27 ++++++++---
 dlls/crypt32/serialize.c       |  9 ++--
 3 files changed, 50 insertions(+), 71 deletions(-)

diff --git a/dlls/crypt32/cert.c b/dlls/crypt32/cert.c
index ef871155b9..f4fc5a62c6 100644
--- a/dlls/crypt32/cert.c
+++ b/dlls/crypt32/cert.c
@@ -536,67 +536,43 @@ static BOOL CertContext_GetProperty(cert_t *cert, DWORD dwPropId,
     return ret;
 }
 
-/* 64-bit compatible layout, so that 64-bit crypt32 is able to read
- * the structure saved by 32-bit crypt32.
- */
-typedef struct
-{
-    ULONG64 pwszContainerName;
-    ULONG64 pwszProvName;
-    DWORD dwProvType;
-    DWORD dwFlags;
-    DWORD cProvParam;
-    ULONG64 rgProvParam;
-    DWORD dwKeySpec;
-} store_CRYPT_KEY_PROV_INFO;
-
-typedef struct
-{
-    DWORD dwParam;
-    ULONG64 pbData;
-    DWORD cbData;
-    DWORD dwFlags;
-} store_CRYPT_KEY_PROV_PARAM;
-
-void CRYPT_FixKeyProvInfoPointers(PCRYPT_KEY_PROV_INFO buf)
+void CRYPT_ConvertKeyProvInfo(const struct store_CRYPT_KEY_PROV_INFO *store, CRYPT_KEY_PROV_INFO *info)
 {
-    CRYPT_KEY_PROV_INFO info;
-    store_CRYPT_KEY_PROV_INFO *store = (store_CRYPT_KEY_PROV_INFO *)buf;
     BYTE *p = (BYTE *)(store + 1);
 
     if (store->pwszContainerName)
     {
-        info.pwszContainerName = (LPWSTR)((BYTE *)store + store->pwszContainerName);
-        p += (lstrlenW(info.pwszContainerName) + 1) * sizeof(WCHAR);
+        info->pwszContainerName = (LPWSTR)((BYTE *)store + store->pwszContainerName);
+        p += (lstrlenW(info->pwszContainerName) + 1) * sizeof(WCHAR);
     }
     else
-        info.pwszContainerName = NULL;
+        info->pwszContainerName = NULL;
 
     if (store->pwszProvName)
     {
-        info.pwszProvName = (LPWSTR)((BYTE *)store + store->pwszProvName);
-        p += (lstrlenW(info.pwszProvName) + 1) * sizeof(WCHAR);
+        info->pwszProvName = (LPWSTR)((BYTE *)store + store->pwszProvName);
+        p += (lstrlenW(info->pwszProvName) + 1) * sizeof(WCHAR);
     }
     else
-        info.pwszProvName = NULL;
+        info->pwszProvName = NULL;
 
-    info.dwProvType = store->dwProvType;
-    info.dwFlags = store->dwFlags;
-    info.dwKeySpec = store->dwKeySpec;
-    info.cProvParam = store->cProvParam;
+    info->dwProvType = store->dwProvType;
+    info->dwFlags = store->dwFlags;
+    info->dwKeySpec = store->dwKeySpec;
+    info->cProvParam = store->cProvParam;
 
-    if (info.cProvParam)
+    if (info->cProvParam)
     {
         DWORD i;
 
-        info.rgProvParam = (CRYPT_KEY_PROV_PARAM *)p;
+        info->rgProvParam = (CRYPT_KEY_PROV_PARAM *)p;
 
         for (i = 0; i < store->cProvParam; i++)
         {
             CRYPT_KEY_PROV_PARAM param;
-            store_CRYPT_KEY_PROV_PARAM *store_param;
+            struct store_CRYPT_KEY_PROV_PARAM *store_param;
 
-            store_param = (store_CRYPT_KEY_PROV_PARAM *)p;
+            store_param = (struct store_CRYPT_KEY_PROV_PARAM *)p;
             p += sizeof(*store_param);
 
             param.dwParam = store_param[i].dwParam;
@@ -605,16 +581,14 @@ void CRYPT_FixKeyProvInfoPointers(PCRYPT_KEY_PROV_INFO buf)
             param.pbData = param.cbData ? p : NULL;
             p += store_param[i].cbData;
 
-            memcpy(&info.rgProvParam[i], ¶m, sizeof(param));
+            memcpy(&info->rgProvParam[i], ¶m, sizeof(param));
         }
     }
     else
-        info.rgProvParam = NULL;
+        info->rgProvParam = NULL;
 
-    TRACE("%s,%s,%u,%08x,%u,%p,%u\n", debugstr_w(info.pwszContainerName), debugstr_w(info.pwszProvName),
-          info.dwProvType, info.dwFlags, info.cProvParam, info.rgProvParam, info.dwKeySpec);
-
-    *buf = info;
+    TRACE("%s,%s,%u,%08x,%u,%p,%u\n", debugstr_w(info->pwszContainerName), debugstr_w(info->pwszProvName),
+          info->dwProvType, info->dwFlags, info->cProvParam, info->rgProvParam, info->dwKeySpec);
 }
 
 BOOL WINAPI CertGetCertificateContextProperty(PCCERT_CONTEXT pCertContext,
@@ -649,12 +623,6 @@ BOOL WINAPI CertGetCertificateContextProperty(PCCERT_CONTEXT pCertContext,
              sizeof(keyContext.hCryptProv));
         break;
     }
-    case CERT_KEY_PROV_INFO_PROP_ID:
-        ret = CertContext_GetProperty(cert, dwPropId, pvData,
-         pcbData);
-        if (ret && pvData)
-            CRYPT_FixKeyProvInfoPointers(pvData);
-        break;
     default:
         ret = CertContext_GetProperty(cert, dwPropId, pvData,
          pcbData);
@@ -676,11 +644,11 @@ BOOL WINAPI CertGetCertificateContextProperty(PCCERT_CONTEXT pCertContext,
  * - store_CRYPT_KEY_PROV_PARAM[0].data
  * - ...
  */
-static void CRYPT_CopyKeyProvInfo(store_CRYPT_KEY_PROV_INFO *to, const CRYPT_KEY_PROV_INFO *from)
+static void CRYPT_CopyKeyProvInfo(const CRYPT_KEY_PROV_INFO *from, struct store_CRYPT_KEY_PROV_INFO *to)
 {
     DWORD i;
     BYTE *p;
-    store_CRYPT_KEY_PROV_PARAM *param;
+    struct store_CRYPT_KEY_PROV_PARAM *param;
 
     p = (BYTE *)(to + 1);
 
@@ -710,7 +678,7 @@ static void CRYPT_CopyKeyProvInfo(store_CRYPT_KEY_PROV_INFO *to, const CRYPT_KEY
 
     for (i = 0; i < to->cProvParam; i++)
     {
-        param = (store_CRYPT_KEY_PROV_PARAM *)p;
+        param = (struct store_CRYPT_KEY_PROV_PARAM *)p;
         p += sizeof(*param);
 
         param->dwParam = from->rgProvParam[i].dwParam;
@@ -726,7 +694,7 @@ static BOOL CertContext_SetKeyProvInfoProperty(CONTEXT_PROPERTY_LIST *properties
  const CRYPT_KEY_PROV_INFO *info)
 {
     BYTE *buf;
-    DWORD size = sizeof(store_CRYPT_KEY_PROV_INFO), i;
+    DWORD size = sizeof(struct store_CRYPT_KEY_PROV_INFO), i;
     BOOL ret;
 
     if (info->pwszContainerName)
@@ -735,14 +703,13 @@ static BOOL CertContext_SetKeyProvInfoProperty(CONTEXT_PROPERTY_LIST *properties
         size += (lstrlenW(info->pwszProvName) + 1) * sizeof(WCHAR);
 
     for (i = 0; i < info->cProvParam; i++)
-        size += sizeof(store_CRYPT_KEY_PROV_PARAM) + info->rgProvParam[i].cbData;
+        size += sizeof(struct store_CRYPT_KEY_PROV_PARAM) + info->rgProvParam[i].cbData;
 
     buf = CryptMemAlloc(size);
     if (buf)
     {
-        CRYPT_CopyKeyProvInfo((store_CRYPT_KEY_PROV_INFO *)buf, info);
-        ret = ContextPropertyList_SetProperty(properties,
-         CERT_KEY_PROV_INFO_PROP_ID, buf, size);
+        CRYPT_CopyKeyProvInfo(info, (struct store_CRYPT_KEY_PROV_INFO *)buf);
+        ret = ContextPropertyList_SetProperty(properties, CERT_KEY_PROV_INFO_PROP_ID, buf, size);
         CryptMemFree(buf);
     }
     else
diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h
index c552bdf949..d5e547136d 100644
--- a/dlls/crypt32/crypt32_private.h
+++ b/dlls/crypt32/crypt32_private.h
@@ -370,13 +370,28 @@ BOOL CRYPT_ReadSerializedStoreFromFile(HANDLE file, HCERTSTORE store) DECLSPEC_H
 BOOL CRYPT_ReadSerializedStoreFromBlob(const CRYPT_DATA_BLOB *blob,
  HCERTSTORE store) DECLSPEC_HIDDEN;
 
-/* Fixes up the pointers in info, where info is assumed to be a
- * CRYPT_KEY_PROV_INFO, followed by its container name, provider name, and any
- * provider parameters, in a contiguous buffer, but where info's pointers are
- * assumed to be invalid.  Upon return, info's pointers point to the
- * appropriate memory locations.
+/* 64-bit compatible layout, so that 64-bit crypt32 is able to read
+ * the structure saved by 32-bit crypt32.
  */
-void CRYPT_FixKeyProvInfoPointers(PCRYPT_KEY_PROV_INFO info) DECLSPEC_HIDDEN;
+struct store_CRYPT_KEY_PROV_INFO
+{
+    DWORD64 pwszContainerName;
+    DWORD64 pwszProvName;
+    DWORD dwProvType;
+    DWORD dwFlags;
+    DWORD cProvParam;
+    DWORD64 rgProvParam;
+    DWORD dwKeySpec;
+};
+
+struct store_CRYPT_KEY_PROV_PARAM
+{
+    DWORD dwParam;
+    ULONG64 pbData;
+    DWORD cbData;
+    DWORD dwFlags;
+};
+void CRYPT_ConvertKeyProvInfo(const struct store_CRYPT_KEY_PROV_INFO *src, CRYPT_KEY_PROV_INFO *dst) DECLSPEC_HIDDEN;
 
 struct store_CERT_KEY_CONTEXT
 {
diff --git a/dlls/crypt32/serialize.c b/dlls/crypt32/serialize.c
index 7f7a4bc369..b8430c18ec 100644
--- a/dlls/crypt32/serialize.c
+++ b/dlls/crypt32/serialize.c
@@ -272,12 +272,9 @@ static BOOL CRYPT_ReadContextProp(
             break;
         case CERT_KEY_PROV_INFO_PROP_ID:
         {
-            PCRYPT_KEY_PROV_INFO info =
-             (PCRYPT_KEY_PROV_INFO)pbElement;
-
-            CRYPT_FixKeyProvInfoPointers(info);
-            ret = contextInterface->setProp(context,
-             hdr->propID, 0, pbElement);
+            CRYPT_KEY_PROV_INFO info;
+            CRYPT_ConvertKeyProvInfo((struct store_CRYPT_KEY_PROV_INFO *)pbElement, &info);
+            ret = contextInterface->setProp(context, hdr->propID, 0, &info);
             break;
         }
         case CERT_KEY_CONTEXT_PROP_ID:
-- 
2.26.2

[PATCH] crypt32: CertGetCertificateContextProperty should not convert from store to certificate format CERT_KEY_PROV_INF

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=79571

Your paranoid android.


=== debiant (32 bit report) ===

crypt32:
Unhandled exception: page fault on read access to 0x00000030 in 32-bit code (0x7b02a438).

Report validation errors:
crypt32:cert crashed (c0000005)

=== debiant (32 bit Chinese:China report) ===

crypt32:
Unhandled exception: page fault on read access to 0x00000030 in 32-bit code (0x7b02a438).

Report validation errors:
crypt32:cert crashed (c0000005)

=== debiant (32 bit WoW report) ===

crypt32:
Unhandled exception: page fault on read access to 0x00000030 in 32-bit code (0x7b02a438).

Report validation errors:
crypt32:cert crashed (c0000005)

=== debiant (64 bit WoW report) ===

crypt32:
Unhandled exception: page fault on read access to 0x00000030 in 32-bit code (0x7b02a438).

Report validation errors:
crypt32:cert crashed (c0000005)