[PATCH 0/1] MR8350: crypt32: Handle missing attributes in CDecodeSignedMsg_GetParam.

[Yuxuan Shui (＠yshui)]

* * *

While investigating another problem, I found we are handling empty attributes list differently compared native.

But the actual problem is this, shouldn't `CRYPT_SizeOfAttributes` add `sizeof(CRYPT_ATTRIBUTES)` to `size`? Basically we return `size == 0` when the attributes list is empty, so the caller allocates zero bytes and call `GetParam` again, and we write out-of-bound trying to `out->cAttrs = in->cAttrs` in `CRYPT_ConstructAttributes`. (Of course with this MR this problem no longer show up, but the question still stands.)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8350

[PATCH 1/1] crypt32: Handle missing attributes in CDecodeSignedMsg_GetParam.

[Yuxuan Shui]

From: Yuxuan Shui <yshui(a)codeweavers.com>

---
 dlls/crypt32/msg.c       | 4 ++++
 dlls/crypt32/tests/msg.c | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/dlls/crypt32/msg.c b/dlls/crypt32/msg.c
index 4d49aebe4b6..d4ff73373c7 100644
--- a/dlls/crypt32/msg.c
+++ b/dlls/crypt32/msg.c
@@ -3245,6 +3245,8 @@ static BOOL CDecodeSignedMsg_GetParam(CDecodeMsg *msg, DWORD dwParamType,
         {
             if (dwIndex >= msg->u.signed_data.info->cSignerInfo)
                 SetLastError(CRYPT_E_INVALID_INDEX);
+            else if (!msg->u.signed_data.info->rgSignerInfo[dwIndex].AuthAttrs.cAttr)
+                SetLastError(CRYPT_E_ATTRIBUTES_MISSING);
             else
                 ret = CRYPT_CopyAttr(pvData, pcbData,
                  &msg->u.signed_data.info->rgSignerInfo[dwIndex].AuthAttrs);
@@ -3257,6 +3259,8 @@ static BOOL CDecodeSignedMsg_GetParam(CDecodeMsg *msg, DWORD dwParamType,
         {
             if (dwIndex >= msg->u.signed_data.info->cSignerInfo)
                 SetLastError(CRYPT_E_INVALID_INDEX);
+            else if (!msg->u.signed_data.info->rgSignerInfo[dwIndex].UnauthAttrs.cAttr)
+                SetLastError(CRYPT_E_ATTRIBUTES_MISSING);
             else
                 ret = CRYPT_CopyAttr(pvData, pcbData,
                  &msg->u.signed_data.info->rgSignerInfo[dwIndex].UnauthAttrs);
diff --git a/dlls/crypt32/tests/msg.c b/dlls/crypt32/tests/msg.c
index fa48d978e17..0c7bbdd26d8 100644
--- a/dlls/crypt32/tests/msg.c
+++ b/dlls/crypt32/tests/msg.c
@@ -2808,6 +2808,11 @@ static void test_decode_msg_get_param(void)
         compare_signer_info((CMSG_SIGNER_INFO *)buf, &signer);
         CryptMemFree(buf);
     }
+    size = 0;
+    ret = CryptMsgGetParam(msg, CMSG_SIGNER_UNAUTH_ATTR_PARAM, 0, NULL, &size);
+    ok(!ret, "CryptMsgGetParam succeeded unexpectedly\n");
+    ok(GetLastError() == CRYPT_E_ATTRIBUTES_MISSING, "unexpected error %08lx\n", GetLastError());
+    ok(size == 0, "unexpected size: %lu\n", size);
     /* Getting the CMS signer info of a PKCS7 message is possible. */
     size = 0;
     ret = CryptMsgGetParam(msg, CMSG_CMS_SIGNER_INFO_PARAM, 0, NULL, &size);
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8350

[Bernhard Übelacker]

This seems to be related to:

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57656

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8350#note_106925

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/crypt32/tests/msg.c:
> compare_signer_info((CMSG_SIGNER_INFO *)buf, &signer);
>          CryptMemFree(buf);
>      }
> +    size = 0;
> +    ret = CryptMsgGetParam(msg, CMSG_SIGNER_UNAUTH_ATTR_PARAM, 0, NULL, &size);
> +    ok(!ret, "CryptMsgGetParam succeeded unexpectedly\n");
> +    ok(GetLastError() == CRYPT_E_ATTRIBUTES_MISSING, "unexpected error %08lx\n", GetLastError());

You should set last error to a known value before calling CryptMsgGetParam() to confirm that this function sets it: `SetLastError(0xdeadbeef);`

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8350#note_106990