[PATCH v2 0/3] MR6767: WriteProcessMemory() without PROCESS_QUERY_INFORMATION access right.

[eric pouech (＠epo)]

This is an alternate solution to MR 6761 [1]
(actually is based on comment in that MR).

WriteProcessMemory() should succeed when process handle doesn't have
PROCESS_QUERY_INFORMATION access right set (test attached).
Plus some additional cleanup in test for ReadProcessMemory().

Feedback from Proton says that this patch solves the reported game issue.

[1] https://gitlab.winehq.org/wine/wine/-/merge_requests/6761

--
  v2: server: Amend process rights mapping.
      ntdll/tests: Add tests for NtQueryVirtualMemory.
      kernel32/tests: Don't hardcode page size in buffer size.

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

[PATCH v2 1/3] kernel32/tests: Don't hardcode page size in buffer size.

[Eric Pouech]

From: Eric Pouech

Signed-off-by: Eric Pouech 
---
 dlls/kernel32/tests/virtual.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dlls/kernel32/tests/virtual.c b/dlls/kernel32/tests/virtual.c
index 12841133c05..cfbfcac0950 100644
--- a/dlls/kernel32/tests/virtual.c
+++ b/dlls/kernel32/tests/virtual.c
@@ -4368,13 +4368,15 @@ static void test_PrefetchVirtualMemory(void)

static void test_ReadProcessMemory(void)
 {
-    BYTE buf[0x2000];
+    BYTE *buf;
     DWORD old_prot;
     SIZE_T copied;
     HANDLE hproc;
     void *ptr;
     BOOL ret;

+    buf = malloc(2 * si.dwPageSize);
+    ok(buf != NULL, "OOM\n");
     ret = DuplicateHandle(GetCurrentProcess(), GetCurrentProcess(), GetCurrentProcess(),
             &hproc, 0, FALSE, DUPLICATE_SAME_ACCESS);
     ok(ret, "DuplicateHandle failed %lu\n", GetLastError());
@@ -4403,6 +4405,7 @@ static void test_ReadProcessMemory(void)
     ret = VirtualFree(ptr, 0, MEM_RELEASE);
     ok(ret, "VirtualFree failed %lu\n", GetLastError());
     CloseHandle(hproc);
+    free(buf);
 }

START_TEST(virtual)

[PATCH v2 2/3] ntdll/tests: Add tests for NtQueryVirtualMemory.

[Eric Pouech]

From: Eric Pouech

Signed-off-by: Eric Pouech 
---
 dlls/ntdll/tests/info.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/dlls/ntdll/tests/info.c b/dlls/ntdll/tests/info.c
index 7741293d815..d7b84f43b70 100644
--- a/dlls/ntdll/tests/info.c
+++ b/dlls/ntdll/tests/info.c
@@ -2949,6 +2949,7 @@ static void test_queryvirtualmemory(void)
     MEMORY_BASIC_INFORMATION mbi;
     char stackbuf[42];
     HMODULE module;
+    HANDLE process;
     void *user_shared_data = (void *)0x7ffe0000;
     void *buffer[256];
     MEMORY_SECTION_NAME *name = (MEMORY_SECTION_NAME *)buffer;
@@ -2964,6 +2965,16 @@ static void test_queryvirtualmemory(void)
     ok (mbi.Protect == PAGE_READONLY, "mbi.Protect is 0x%lx, expected 0x%x\n", mbi.Protect, PAGE_READONLY);
     ok (mbi.Type == MEM_IMAGE, "mbi.Type is 0x%lx, expected 0x%x\n", mbi.Type, MEM_IMAGE);

+    module = GetModuleHandleA( "ntdll.dll" );
+    process = NULL;
+    status = NtDuplicateObject(NtCurrentProcess(), NtCurrentProcess(), NtCurrentProcess(),
+                               &process, PROCESS_VM_OPERATION | PROCESS_VM_WRITE, 0, 0);
+    ok(status == STATUS_SUCCESS, "DuplicateHandle failed\n");
+    status = pNtQueryVirtualMemory(process, module, MemoryBasicInformation, &mbi, sizeof(MEMORY_BASIC_INFORMATION), &readcount);
+    todo_wine
+    ok( status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %08lx\n", status);
+    CloseHandle(process);
+
     module = GetModuleHandleA( "ntdll.dll" );
     status = pNtQueryVirtualMemory(NtCurrentProcess(), pNtQueryVirtualMemory, MemoryBasicInformation, &mbi, sizeof(MEMORY_BASIC_INFORMATION), &readcount);
     ok( status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %08lx\n", status);

[PATCH v2 3/3] server: Amend process rights mapping.

[Eric Pouech]

From: Eric Pouech

PROCESS_VM_OPERATION | PROCESS_VM_WRITE grants
PROCESS_QUERY_LIMITED_INFORMATION rights.

Signed-off-by: Eric Pouech 
---
 server/process.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/server/process.c b/server/process.c
index 49f5c75005f..f386239919d 100644
--- a/server/process.c
+++ b/server/process.c
@@ -805,6 +805,8 @@ static unsigned int process_map_access( struct object *obj, unsigned int access
     access = default_map_access( obj, access );
     if (access & PROCESS_QUERY_INFORMATION) access |= PROCESS_QUERY_LIMITED_INFORMATION;
     if (access & PROCESS_SET_INFORMATION) access |= PROCESS_SET_LIMITED_INFORMATION;
+    if ((access & (PROCESS_VM_OPERATION | PROCESS_VM_WRITE)) == (PROCESS_VM_OPERATION | PROCESS_VM_WRITE))
+        access |= PROCESS_QUERY_LIMITED_INFORMATION;
     return access;
 }

[PATCH v2 3/3] server: Amend process rights mapping.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=149476

Your paranoid android.

=== w864 (32 bit report) ===

ntdll:
info.c:2975: Test failed: Expected STATUS_SUCCESS, got c0000022

=== w1064v1507 (32 bit report) ===

ntdll:
info.c:2975: Test failed: Expected STATUS_SUCCESS, got c0000022

=== w7pro64 (64 bit report) ===

ntdll:
info.c:2975: Test failed: Expected STATUS_SUCCESS, got c0000022

=== w864 (64 bit report) ===

ntdll:
info.c:2975: Test failed: Expected STATUS_SUCCESS, got c0000022

=== w1064v1507 (64 bit report) ===

ntdll:
info.c:2975: Test failed: Expected STATUS_SUCCESS, got c0000022

=== debian11 (32 bit report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11 (32 bit ar:MA report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11 (32 bit de report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11 (32 bit fr report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11 (32 bit he:IL report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11 (32 bit hi:IN report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11 (32 bit ja:JP report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11 (32 bit zh:CN report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11b (32 bit WoW report) ===

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

=== debian11b (64 bit WoW report) ===

d3d9:
d3d9ex.c:3168: Test failed: Expected message 0x3 for window 0x1, but didn't receive it, i=0.

ntdll:
info.c:2975: Test succeeded inside todo block: Expected STATUS_SUCCESS, got 00000000

[eric pouech (＠epo)]

On Tue Nov  5 09:37:05 2024 +0000, Alexandre Julliard wrote:
> We should first make sure that NtQueryVirtualMemory behaves correctly. A
> quick test suggests that it should succeed for PROCESS_VM_OPERATION|PROCESS_VM_WRITE.

Pushed V2:
- tests now apply to NtQueryVirtualMemory (instead of WriteProcessMemory)
- fix now just amends process access rights in server (tested on Win10 that the granted access rights include PROCESS_QUERY_LIMITED_INFORMATION)

[Dmitry Timoshkov (＠dmitry)]

On Tue Nov  5 09:37:05 2024 +0000, eric pouech wrote:
> Pushed V2:
> - tests now apply to NtQueryVirtualMemory (instead of WriteProcessMemory)
> - fix now just amends process access rights in server (tested on Win10
>
> that the granted access rights include PROCESS_QUERY_LIMITED_INFORMATION)

It should be possible to add a test for this. See dlls/advapi32/tests/security.c,test_process_access() for an example.