[PATCH] crypt32: Store CRYPT_KEY_PROV_INFO in a platform independent way.
This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property from/to the certificate storage independent from architecture the crypt32.dll code being executed on. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/crypt32/cert.c | 162 +++++++++++++++++++++++++++++--------------- 1 file changed, 109 insertions(+), 53 deletions(-) diff --git a/dlls/crypt32/cert.c b/dlls/crypt32/cert.c index 488b84001c..5b7e031338 100644 --- a/dlls/crypt32/cert.c +++ b/dlls/crypt32/cert.c @@ -520,38 +520,90 @@ static BOOL CertContext_GetProperty(cert_t *cert, DWORD dwPropId, return ret; } -void CRYPT_FixKeyProvInfoPointers(PCRYPT_KEY_PROV_INFO info) -{ - DWORD i, containerLen, provNameLen; - LPBYTE data = (LPBYTE)info + sizeof(CRYPT_KEY_PROV_INFO); - - if (info->pwszContainerName) - { - info->pwszContainerName = (LPWSTR)data; - containerLen = (lstrlenW(info->pwszContainerName) + 1) * sizeof(WCHAR); - data += containerLen; +/* 64-bit compatible definitions */ +typedef struct +{ + DWORD pwszContainerName; + DWORD pad1; + DWORD pwszProvName; + DWORD pad2; + DWORD dwProvType; + DWORD dwFlags; + DWORD cProvParam; + DWORD pad3; + DWORD rgProvParam; + DWORD pad4; + DWORD dwKeySpec; + DWORD pad5; +} store_CRYPT_KEY_PROV_INFO; + +typedef struct +{ + DWORD dwParam; + DWORD pad1; + DWORD pbData; + DWORD pad2; + DWORD cbData; + DWORD dwFlags; +} store_CRYPT_KEY_PROV_PARAM; + +void CRYPT_FixKeyProvInfoPointers(PCRYPT_KEY_PROV_INFO buf) +{ + 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)p; + p += (lstrlenW(info.pwszContainerName) + 1) * sizeof(WCHAR); } + else + info.pwszContainerName = NULL; - if (info->pwszProvName) + if (store->pwszProvName) { - info->pwszProvName = (LPWSTR)data; - provNameLen = (lstrlenW(info->pwszProvName) + 1) * sizeof(WCHAR); - data += provNameLen; + info.pwszProvName = (LPWSTR)p; + p += (lstrlenW(info.pwszProvName) + 1) * sizeof(WCHAR); } + else + info.pwszProvName = NULL; + + info.dwProvType = store->dwProvType; + info.dwFlags = store->dwFlags; + info.dwKeySpec = store->dwKeySpec; + info.cProvParam = store->cProvParam; - if (info->cProvParam) + if (info.cProvParam) { - info->rgProvParam = (PCRYPT_KEY_PROV_PARAM)data; - data += info->cProvParam * sizeof(CRYPT_KEY_PROV_PARAM); + DWORD i; - for (i = 0; i < info->cProvParam; i++) + info.rgProvParam = (CRYPT_KEY_PROV_PARAM *)p; + + for (i = 0; i < store->cProvParam; i++) { - info->rgProvParam[i].pbData = data; - data += info->rgProvParam[i].cbData; + CRYPT_KEY_PROV_PARAM param; + store_CRYPT_KEY_PROV_PARAM *store_param; + + store_param = (store_CRYPT_KEY_PROV_PARAM *)p; + p += sizeof(*store_param); + + param.dwParam = store_param[i].dwParam; + param.dwFlags = store_param[i].dwFlags; + param.cbData = store_param[i].cbData; + param.pbData = param.cbData ? p : NULL; + p += store_param[i].cbData; + + 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; } BOOL WINAPI CertGetCertificateContextProperty(PCCERT_CONTEXT pCertContext, @@ -606,72 +658,76 @@ BOOL WINAPI CertGetCertificateContextProperty(PCCERT_CONTEXT pCertContext, * data, but whose pointers are uninitialized. * Upon return, to contains a contiguous copy of from, packed in the following * order: - * - CRYPT_KEY_PROV_INFO + * - store_CRYPT_KEY_PROV_INFO * - pwszContainerName * - pwszProvName - * - rgProvParam[0]... + * - store_CRYPT_KEY_PROV_PARAM[0] + * - store_CRYPT_KEY_PROV_PARAM[0].data + * - ... */ -static void CRYPT_CopyKeyProvInfo(PCRYPT_KEY_PROV_INFO to, - const CRYPT_KEY_PROV_INFO *from) +static void CRYPT_CopyKeyProvInfo(store_CRYPT_KEY_PROV_INFO *to, const CRYPT_KEY_PROV_INFO *from) { DWORD i; - LPBYTE nextData = (LPBYTE)to + sizeof(CRYPT_KEY_PROV_INFO); + BYTE *p; + store_CRYPT_KEY_PROV_PARAM *param; + + p = (BYTE *)(to + 1); if (from->pwszContainerName) { - to->pwszContainerName = (LPWSTR)nextData; - lstrcpyW(to->pwszContainerName, from->pwszContainerName); - nextData += (lstrlenW(from->pwszContainerName) + 1) * sizeof(WCHAR); + to->pwszContainerName = p - (BYTE *)to; + lstrcpyW((LPWSTR)p, from->pwszContainerName); + p += (lstrlenW(from->pwszContainerName) + 1) * sizeof(WCHAR); } else - to->pwszContainerName = NULL; + to->pwszContainerName = 0; + if (from->pwszProvName) { - to->pwszProvName = (LPWSTR)nextData; - lstrcpyW(to->pwszProvName, from->pwszProvName); - nextData += (lstrlenW(from->pwszProvName) + 1) * sizeof(WCHAR); + to->pwszProvName = p - (BYTE *)to; + lstrcpyW((LPWSTR)p, from->pwszProvName); + p += (lstrlenW(from->pwszProvName) + 1) * sizeof(WCHAR); } else - to->pwszProvName = NULL; + to->pwszProvName = 0; + to->dwProvType = from->dwProvType; to->dwFlags = from->dwFlags; to->cProvParam = from->cProvParam; - to->rgProvParam = (PCRYPT_KEY_PROV_PARAM)nextData; - nextData += to->cProvParam * sizeof(CRYPT_KEY_PROV_PARAM); to->dwKeySpec = from->dwKeySpec; + for (i = 0; i < to->cProvParam; i++) { - memcpy(&to->rgProvParam[i], &from->rgProvParam[i], - sizeof(CRYPT_KEY_PROV_PARAM)); - to->rgProvParam[i].pbData = nextData; - memcpy(to->rgProvParam[i].pbData, from->rgProvParam[i].pbData, - from->rgProvParam[i].cbData); - nextData += from->rgProvParam[i].cbData; + param = (store_CRYPT_KEY_PROV_PARAM *)p; + p += sizeof(*param); + + param->dwParam = from->rgProvParam[i].dwParam; + param->cbData = from->rgProvParam[i].cbData; + param->dwFlags = from->rgProvParam[i].dwFlags; + memcpy(p, from->rgProvParam[i].pbData, from->rgProvParam[i].cbData); + p += from->rgProvParam[i].cbData; } } 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; BOOL ret; - LPBYTE buf = NULL; - DWORD size = sizeof(CRYPT_KEY_PROV_INFO), i, containerSize, provNameSize; if (info->pwszContainerName) - containerSize = (lstrlenW(info->pwszContainerName) + 1) * sizeof(WCHAR); - else - containerSize = 0; + size += (lstrlenW(info->pwszContainerName) + 1) * sizeof(WCHAR); if (info->pwszProvName) - provNameSize = (lstrlenW(info->pwszProvName) + 1) * sizeof(WCHAR); - else - provNameSize = 0; - size += containerSize + provNameSize; + size += (lstrlenW(info->pwszProvName) + 1) * sizeof(WCHAR); + for (i = 0; i < info->cProvParam; i++) - size += sizeof(CRYPT_KEY_PROV_PARAM) + info->rgProvParam[i].cbData; + size += sizeof(store_CRYPT_KEY_PROV_PARAM) + info->rgProvParam[i].cbData; + buf = CryptMemAlloc(size); if (buf) { - CRYPT_CopyKeyProvInfo((PCRYPT_KEY_PROV_INFO)buf, info); + CRYPT_CopyKeyProvInfo((store_CRYPT_KEY_PROV_INFO *)buf, info); ret = ContextPropertyList_SetProperty(properties, CERT_KEY_PROV_INFO_PROP_ID, buf, size); CryptMemFree(buf); -- 2.26.2
Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property from/to the certificate storage independent from architecture the crypt32.dll code being executed on.
Is there anything that could be improved to make this patch accepted? -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property from/to the certificate storage independent from architecture the crypt32.dll code being executed on.
Is there anything that could be improved to make this patch accepted?
That data structure looks strange. Maybe you could explain why it needs to use the 64-bit layout, and why you are achieving that with padding instead of using 64-bit types? -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property from/to the certificate storage independent from architecture the crypt32.dll code being executed on.
Is there anything that could be improved to make this patch accepted?
That data structure looks strange. Maybe you could explain why it needs to use the 64-bit layout, and why you are achieving that with padding instead of using 64-bit types?
64-bit layout is used so that 64-bit crypt32 is able to read the structure saved by 32-bit crypt32, currently this leads to a memory corruption. There's no reason to not use 64-bit types, thanks. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property from/to the certificate storage independent from architecture the crypt32.dll code being executed on.
Is there anything that could be improved to make this patch accepted?
That data structure looks strange. Maybe you could explain why it needs to use the 64-bit layout, and why you are achieving that with padding instead of using 64-bit types?
64-bit layout is used so that 64-bit crypt32 is able to read the structure saved by 32-bit crypt32, currently this leads to a memory corruption. There's no reason to not use 64-bit types, thanks.
That's better, but as long as you are defining your own structure, I don't see why using simply DWORDs wouldn't work just as well, as long as you use the same structure on 32- and 64-bit. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property from/to the certificate storage independent from architecture the crypt32.dll code being executed on.
Is there anything that could be improved to make this patch accepted?
That data structure looks strange. Maybe you could explain why it needs to use the 64-bit layout, and why you are achieving that with padding instead of using 64-bit types?
64-bit layout is used so that 64-bit crypt32 is able to read the structure saved by 32-bit crypt32, currently this leads to a memory corruption. There's no reason to not use 64-bit types, thanks.
That's better, but as long as you are defining your own structure, I don't see why using simply DWORDs wouldn't work just as well, as long as you use the same structure on 32- and 64-bit.
Obviously DWORDs won't work for pointers, using ULONG64 allows to avoid reallocation when reading CRYPT_KEY_PROV_INFO from the certificate storage, and just fix up the pointers in place as current code is doing. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property from/to the certificate storage independent from architecture the crypt32.dll code being executed on.
Is there anything that could be improved to make this patch accepted?
That data structure looks strange. Maybe you could explain why it needs to use the 64-bit layout, and why you are achieving that with padding instead of using 64-bit types?
64-bit layout is used so that 64-bit crypt32 is able to read the structure saved by 32-bit crypt32, currently this leads to a memory corruption. There's no reason to not use 64-bit types, thanks.
That's better, but as long as you are defining your own structure, I don't see why using simply DWORDs wouldn't work just as well, as long as you use the same structure on 32- and 64-bit.
Obviously DWORDs won't work for pointers, using ULONG64 allows to avoid reallocation when reading CRYPT_KEY_PROV_INFO from the certificate storage, and just fix up the pointers in place as current code is doing.
I feel it would be cleaner to do that with some sort of union, and use the correct types and names for what is being stored. In particular keeping names like 'pwszContainerName' for things that are not pointers isn't very meaningful. But of course the existing code is already not great in that respect... -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
> This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property > from/to the certificate storage independent from architecture the crypt32.dll > code being executed on.
Is there anything that could be improved to make this patch accepted?
That data structure looks strange. Maybe you could explain why it needs to use the 64-bit layout, and why you are achieving that with padding instead of using 64-bit types?
64-bit layout is used so that 64-bit crypt32 is able to read the structure saved by 32-bit crypt32, currently this leads to a memory corruption. There's no reason to not use 64-bit types, thanks.
That's better, but as long as you are defining your own structure, I don't see why using simply DWORDs wouldn't work just as well, as long as you use the same structure on 32- and 64-bit.
Obviously DWORDs won't work for pointers, using ULONG64 allows to avoid reallocation when reading CRYPT_KEY_PROV_INFO from the certificate storage, and just fix up the pointers in place as current code is doing.
I feel it would be cleaner to do that with some sort of union, and use the correct types and names for what is being stored. In particular keeping names like 'pwszContainerName' for things that are not pointers isn't very meaningful. But of course the existing code is already not great in that respect...
My main intention was to reuse current code, even it's so much ugly, that's why I decided to keep using such a structure, and just adapt it for mixed 32/64-bit case. If you feel strongly about rewriting it, then current code should be completely replaced. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
>> This makes reading and writing of CRYPT_KEY_PROV_INFO certificate property >> from/to the certificate storage independent from architecture the crypt32.dll >> code being executed on. > > Is there anything that could be improved to make this patch accepted?
That data structure looks strange. Maybe you could explain why it needs to use the 64-bit layout, and why you are achieving that with padding instead of using 64-bit types?
64-bit layout is used so that 64-bit crypt32 is able to read the structure saved by 32-bit crypt32, currently this leads to a memory corruption. There's no reason to not use 64-bit types, thanks.
That's better, but as long as you are defining your own structure, I don't see why using simply DWORDs wouldn't work just as well, as long as you use the same structure on 32- and 64-bit.
Obviously DWORDs won't work for pointers, using ULONG64 allows to avoid reallocation when reading CRYPT_KEY_PROV_INFO from the certificate storage, and just fix up the pointers in place as current code is doing.
I feel it would be cleaner to do that with some sort of union, and use the correct types and names for what is being stored. In particular keeping names like 'pwszContainerName' for things that are not pointers isn't very meaningful. But of course the existing code is already not great in that respect...
My main intention was to reuse current code, even it's so much ugly, that's why I decided to keep using such a structure, and just adapt it for mixed 32/64-bit case. If you feel strongly about rewriting it, then current code should be completely replaced.
That would be a good idea, if you feel like doing it. But I'll commit your patch in the meantime. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Dmitry Timoshkov