[PATCH v2 0/3] MR3839: kernel32: Add structure size checks to CreateActCtx().

[Dmitry Timoshkov (＠dmitry)]

Signed-off-by: Dmitry Timoshkov

--
  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

[PATCH v2 1/3] kernel32/tests: Add some tests for CreateActCtx() with different structure sizes.

[Dmitry Timoshkov]

From: Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov 
---
 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;

[PATCH v2 2/3] kernel32: Add ACTCTX field limit checks to CreateActCtxA().

[Dmitry Timoshkov]

From: Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov 
---
 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);

[PATCH v2 3/3] ntdll: Add ACTCTX field limit checks to RtlCreateActivationContext().

[Dmitry Timoshkov]

From: Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov 
---
 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;

[Dmitry Timoshkov (＠dmitry)]

> 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?

[Dmitry Timoshkov]

What's the status of this MR?

[Dmitry Timoshkov (＠dmitry)]

What's the status of this MR?

[Dmitry Timoshkov (＠dmitry)]

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?