[PATCH v2 0/3] MR3839: kernel32: Add structure size checks to CreateActCtx().
Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> -- v2: ntdll: Add ACTCTX field limit checks to RtlCreateActivationContext(). kernel32: Add ACTCTX field limit checks to CreateActCtxA(). kernel32/tests: Add some tests for CreateActCtx() with different structure sizes. https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/kernel32/tests/actctx.c | 107 ++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 1a05278fae4..abfbe633c03 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2800,18 +2800,123 @@ static void test_CreateActCtx(void) { static const DWORD flags[] = {LOAD_LIBRARY_AS_DATAFILE, LOAD_LIBRARY_AS_IMAGE_RESOURCE, LOAD_LIBRARY_AS_IMAGE_RESOURCE | LOAD_LIBRARY_AS_DATAFILE}; - CHAR path[MAX_PATH], dir[MAX_PATH], dll[MAX_PATH]; + static const struct + { + DWORD size, flags, error; + } test[] = + { + { FIELD_OFFSET(ACTCTXW, lpSource), 0, ERROR_INVALID_PARAMETER }, + { FIELD_OFFSET(ACTCTXW, wProcessorArchitecture), 0, 0 }, + { FIELD_OFFSET(ACTCTXW, lpAssemblyDirectory), ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID, ERROR_INVALID_PARAMETER }, + { FIELD_OFFSET(ACTCTXW, lpResourceName), ACTCTX_FLAG_RESOURCE_NAME_VALID, ERROR_INVALID_PARAMETER }, + { FIELD_OFFSET(ACTCTXW, hModule), ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID, ERROR_INVALID_PARAMETER }, + }; + char path[MAX_PATH], dir[MAX_PATH], dll[MAX_PATH], source[MAX_PATH]; + WCHAR pathW[MAX_PATH], dirW[MAX_PATH], sourceW[MAX_PATH]; ACTCTXA actctx; + ACTCTXW ctxW; HANDLE handle; int i; GetTempPathA(ARRAY_SIZE(path), path); + strcpy(dir, path); strcat(path, "main_wndcls.manifest"); write_manifest("testdep1.manifest", manifest_wndcls1); write_manifest("testdep2.manifest", manifest_wndcls2); write_manifest("main_wndcls.manifest", manifest_wndcls_main); + GetModuleFileNameA(NULL, source, ARRAY_SIZE(source)); + GetModuleFileNameW(NULL, sourceW, ARRAY_SIZE(sourceW)); + + GetTempPathW(ARRAY_SIZE(pathW), pathW); + wcscpy(dirW, pathW); + wcscat(pathW, L"main_wndcls.manifest"); + + memset(&ctxW, 0, sizeof(ctxW)); + ctxW.cbSize = sizeof(ctxW); + ctxW.dwFlags = ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID | ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID; + ctxW.lpSource = pathW; + ctxW.lpAssemblyDirectory = dirW; + ctxW.lpResourceName = (LPWSTR)124; + ctxW.hModule = GetModuleHandleW(NULL); + handle = CreateActCtxW(&ctxW); + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + for (i = 0; i < ARRAY_SIZE(test); i++) + { + winetest_push_context("%i", i); + ctxW.cbSize = test[i].size; + ctxW.dwFlags = test[i].flags; + SetLastError(0xdeadbeef); + handle = CreateActCtxW(&ctxW); + if (!test[i].error) + { + todo_wine + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + } + else + { + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtx should fail\n"); + ok(test[i].error == GetLastError(), "expected %lu, got %lu\n", test[i].error, GetLastError()); + } + + ctxW.cbSize += sizeof(void *); + if ((ctxW.dwFlags & (ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID)) == ACTCTX_FLAG_RESOURCE_NAME_VALID) + ctxW.lpSource = sourceW; /* source without hModule must point to valid PE */ + SetLastError(0xdeadbeef); + handle = CreateActCtxW(&ctxW); + todo_wine_if(i != 4) + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + winetest_pop_context(); + } + + memset(&actctx, 0, sizeof(actctx)); + actctx.cbSize = sizeof(actctx); + actctx.dwFlags = ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID | ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID; + actctx.lpSource = path; + actctx.lpAssemblyDirectory = dir; + actctx.lpResourceName = (LPSTR)124; + actctx.hModule = GetModuleHandleW(NULL); + handle = CreateActCtxA(&actctx); + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + for (i = 0; i < ARRAY_SIZE(test); i++) + { + winetest_push_context("%i", i); + actctx.cbSize = test[i].size; + actctx.dwFlags = test[i].flags; + SetLastError(0xdeadbeef); + handle = CreateActCtxA(&actctx); + if (!test[i].error) + { + todo_wine + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + } + else + { + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtx should fail\n"); + ok(test[i].error == GetLastError(), "expected %lu, got %lu\n", test[i].error, GetLastError()); + } + + actctx.cbSize += sizeof(void *); + if ((actctx.dwFlags & (ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID)) == ACTCTX_FLAG_RESOURCE_NAME_VALID) + actctx.lpSource = source; /* source without hModule must point to valid PE */ + SetLastError(0xdeadbeef); + handle = CreateActCtxA(&actctx); + todo_wine_if(i != 4) + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + winetest_pop_context(); + } + memset(&actctx, 0, sizeof(ACTCTXA)); actctx.cbSize = sizeof(ACTCTXA); actctx.lpSource = path; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/kernel32/process.c | 10 +++++++++- dlls/kernel32/tests/actctx.c | 2 -- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c index e9e18925911..d56118a0fe3 100644 --- a/dlls/kernel32/process.c +++ b/dlls/kernel32/process.c @@ -422,11 +422,19 @@ HANDLE WINAPI DECLSPEC_HOTPATCH CreateActCtxA( const ACTCTXA *actctx ) TRACE("%p %08lx\n", actctx, actctx ? actctx->dwFlags : 0); - if (!actctx || actctx->cbSize != sizeof(*actctx)) +#define CHECK_LIMIT( field ) (actctx->cbSize >= RTL_SIZEOF_THROUGH_FIELD( ACTCTXA, field )) + if (!actctx || !CHECK_LIMIT( lpSource ) || + ((actctx->dwFlags & ACTCTX_FLAG_PROCESSOR_ARCHITECTURE_VALID) && !CHECK_LIMIT( wProcessorArchitecture )) || + ((actctx->dwFlags & ACTCTX_FLAG_LANGID_VALID) && !CHECK_LIMIT( wLangId )) || + ((actctx->dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) && !CHECK_LIMIT( lpAssemblyDirectory )) || + ((actctx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) && !CHECK_LIMIT( lpResourceName )) || + ((actctx->dwFlags & ACTCTX_FLAG_APPLICATION_NAME_VALID) && !CHECK_LIMIT( lpApplicationName )) || + ((actctx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) && !CHECK_LIMIT( hModule ))) { SetLastError(ERROR_INVALID_PARAMETER); return INVALID_HANDLE_VALUE; } +#undef CHECK_LIMIT actw.cbSize = sizeof(actw); actw.dwFlags = actctx->dwFlags; diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index abfbe633c03..56211c46040 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2895,7 +2895,6 @@ static void test_CreateActCtx(void) handle = CreateActCtxA(&actctx); if (!test[i].error) { - todo_wine ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); } @@ -2910,7 +2909,6 @@ static void test_CreateActCtx(void) actctx.lpSource = source; /* source without hModule must point to valid PE */ SetLastError(0xdeadbeef); handle = CreateActCtxA(&actctx); - todo_wine_if(i != 4) ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/kernel32/tests/actctx.c | 2 -- dlls/ntdll/actctx.c | 12 ++++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 56211c46040..20e5e42b620 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2853,7 +2853,6 @@ static void test_CreateActCtx(void) handle = CreateActCtxW(&ctxW); if (!test[i].error) { - todo_wine ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); } @@ -2868,7 +2867,6 @@ static void test_CreateActCtx(void) ctxW.lpSource = sourceW; /* source without hModule must point to valid PE */ SetLastError(0xdeadbeef); handle = CreateActCtxW(&ctxW); - todo_wine_if(i != 4) ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); diff --git a/dlls/ntdll/actctx.c b/dlls/ntdll/actctx.c index 9379d20b6c1..fbffe691559 100644 --- a/dlls/ntdll/actctx.c +++ b/dlls/ntdll/actctx.c @@ -5261,9 +5261,17 @@ NTSTATUS WINAPI RtlCreateActivationContext( HANDLE *handle, const void *ptr ) TRACE("%p %08lx\n", pActCtx, pActCtx ? pActCtx->dwFlags : 0); - if (!pActCtx || pActCtx->cbSize < sizeof(*pActCtx) || - (pActCtx->dwFlags & ~ACTCTX_FLAGS_ALL)) +#define CHECK_LIMIT( field ) (pActCtx->cbSize >= RTL_SIZEOF_THROUGH_FIELD( ACTCTXW, field )) + if (!pActCtx || (pActCtx->dwFlags & ~ACTCTX_FLAGS_ALL) || + !CHECK_LIMIT( lpSource ) || + ((pActCtx->dwFlags & ACTCTX_FLAG_PROCESSOR_ARCHITECTURE_VALID) && !CHECK_LIMIT( wProcessorArchitecture )) || + ((pActCtx->dwFlags & ACTCTX_FLAG_LANGID_VALID) && !CHECK_LIMIT( wLangId )) || + ((pActCtx->dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) && !CHECK_LIMIT( lpAssemblyDirectory )) || + ((pActCtx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) && !CHECK_LIMIT( lpResourceName )) || + ((pActCtx->dwFlags & ACTCTX_FLAG_APPLICATION_NAME_VALID) && !CHECK_LIMIT( lpApplicationName )) || + ((pActCtx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) && !CHECK_LIMIT( hModule ))) return STATUS_INVALID_PARAMETER; +#undef CHECK_LIMIT if ((pActCtx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) && !pActCtx->lpResourceName) return STATUS_INVALID_PARAMETER; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
You could even use an array of flags and their corresponding sizes.
I've sent an updated version of the patches. Does it look better? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_49270
What's the status of this MR? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_50193
On Wed Nov 8 07:53:23 2023 +0000, Dmitry Timoshkov wrote:
What's the status of this MR? Can I get a review for this MR please?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_51458
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)