[PATCH v2 0/4] MR6047: server: Deny opening handles to objects with zero access.

[Paul Gofman (＠gofman)]

--
  v2: server: Skip inaccessible threads in (get_next_thread).
      server: Do not allow zero access mask when opening some objects.
      ntoskrnl.exe/tests: Open directory object with nonzero access in test_permanent().
      ntdll/tests: Add tests for opening some objects with zero access.

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

[PATCH v2 1/4] ntdll/tests: Add tests for opening some objects with zero access.

[Paul Gofman]

From: Paul Gofman

---
 dlls/ntdll/tests/om.c | 82 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/dlls/ntdll/tests/om.c b/dlls/ntdll/tests/om.c
index fb45fe777b7..db767d9d56b 100644
--- a/dlls/ntdll/tests/om.c
+++ b/dlls/ntdll/tests/om.c
@@ -73,6 +73,7 @@ static NTSTATUS (WINAPI *pNtCreateDebugObject)( HANDLE *, ACCESS_MASK, OBJECT_AT
 static NTSTATUS (WINAPI *pNtGetNextThread)(HANDLE process, HANDLE thread, ACCESS_MASK access, ULONG attributes,
                                             ULONG flags, HANDLE *handle);
 static NTSTATUS (WINAPI *pNtOpenProcessToken)(HANDLE,DWORD,HANDLE*);
+static NTSTATUS (WINAPI *pNtOpenThread)(HANDLE *, ACCESS_MASK, const OBJECT_ATTRIBUTES *, const CLIENT_ID * );
 static NTSTATUS (WINAPI *pNtOpenThreadToken)(HANDLE,DWORD,BOOLEAN,HANDLE*);
 static NTSTATUS (WINAPI *pNtDuplicateToken)(HANDLE,ACCESS_MASK,OBJECT_ATTRIBUTES*,BOOLEAN,TOKEN_TYPE,HANDLE*);
 static NTSTATUS (WINAPI *pNtDuplicateObject)(HANDLE,HANDLE,HANDLE,HANDLE*,ACCESS_MASK,ULONG,ULONG);
@@ -3380,6 +3381,85 @@ static void test_object_permanence(void)
     ok( status == STATUS_SUCCESS, "NtSetInformationThread returned %08lx\n", status );
 }

+static void test_zero_access(void)
+{
+    OBJECT_ATTRIBUTES attr;
+    UNICODE_STRING str;
+    NTSTATUS status;
+    WCHAR name[256];
+    CLIENT_ID cid;
+    HANDLE h1, h2;
+
+    swprintf( name, ARRAY_SIZE(name), L"\Sessions\%u\BaseNamedObjects\test_object", NtCurrentTeb()->Peb->SessionId );
+    pRtlInitUnicodeString( &str, name );
+    InitializeObjectAttributes( &attr, &str, 0, 0, NULL );
+
+    status = pNtCreateEvent( &h1, 0, &attr, NotificationEvent, FALSE );
+    ok( !status, "got %#lx.\n", status );
+
+    status = pNtOpenEvent( &h2, EVENT_ALL_ACCESS, &attr );
+    ok( !status, "got %#lx.\n", status );
+    CloseHandle( h2 );
+
+    status = NtOpenEvent(&h2, 0, &attr);
+    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
+    if (!status) CloseHandle( h2 );
+
+    InitializeObjectAttributes( &attr, &str, OBJ_INHERIT, 0, NULL );
+    status = NtOpenEvent(&h2, 0, &attr);
+    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
+    if (!status) CloseHandle( h2 );
+
+    status = pNtDuplicateObject( GetCurrentProcess(), h1, GetCurrentProcess(), &h2, 0, 0, 0 );
+    ok( !status, "got %#lx.\n", status );
+    CloseHandle( h2 );
+    status = pNtDuplicateObject( GetCurrentProcess(), h1, GetCurrentProcess(), &h2, EVENT_ALL_ACCESS, 0, 0 );
+    ok( !status, "got %#lx.\n", status );
+    CloseHandle( h2 );
+
+    CloseHandle( h1 );
+
+    InitializeObjectAttributes( &attr, &str, 0, 0, NULL );
+    status = pNtCreateMutant( &h1, 0, &attr, FALSE );
+    ok( !status, "got %#lx.\n", status );
+    status = NtOpenMutant(&h2, 0, &attr);
+    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
+    if (!status) CloseHandle( h2 );
+    CloseHandle( h1 );
+
+    status = pNtCreateTimer( &h1, 0, &attr, NotificationTimer );
+    ok( !status, "got %#lx.\n", status );
+    status = pNtOpenTimer( &h2, 0, &attr );
+    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
+    if (!status) CloseHandle( h2 );
+    CloseHandle( h1 );
+
+    status = NtGetNextThread(GetCurrentProcess(), NULL, 0, 0, 0, &h1);
+    todo_wine ok( status == STATUS_NO_MORE_ENTRIES, "got %#lx.\n", status );
+    if (!status) CloseHandle( h1 );
+
+    InitializeObjectAttributes( &attr, NULL, 0, 0, NULL );
+    cid.UniqueProcess = ULongToHandle( GetCurrentProcessId() );
+    cid.UniqueThread  = 0;
+    status = pNtOpenProcess( &h1, 0, &attr, &cid );
+    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
+    if (!status) CloseHandle( h1 );
+
+    InitializeObjectAttributes( &attr, NULL, 0, 0, NULL );
+    cid.UniqueProcess = 0;
+    cid.UniqueThread  = ULongToHandle( GetCurrentThreadId() );
+    status = pNtOpenThread( &h1, 0, &attr, &cid );
+    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
+    if (!status) CloseHandle( h1 );
+
+    InitializeObjectAttributes( &attr, &str, OBJ_OPENIF, 0, NULL );
+    swprintf( name, ARRAY_SIZE(name), L"\Sessions\%u", NtCurrentTeb()->Peb->SessionId );
+    RtlInitUnicodeString( &str, name );
+    pNtOpenDirectoryObject( &h1, 0, &attr );
+    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
+    if (!status) CloseHandle( h1 );
+}
+
 START_TEST(om)
 {
     HMODULE hntdll = GetModuleHandleA("ntdll.dll");
@@ -3424,6 +3504,7 @@ START_TEST(om)
     pNtDuplicateToken       =  (void *)GetProcAddress(hntdll, "NtDuplicateToken");
     pNtDuplicateObject      =  (void *)GetProcAddress(hntdll, "NtDuplicateObject");
     pNtCompareObjects       =  (void *)GetProcAddress(hntdll, "NtCompareObjects");
+    pNtOpenThread           =  (void *)GetProcAddress(hntdll, "NtOpenThread");

test_null_in_object_name();
     test_case_sensitive();
@@ -3444,4 +3525,5 @@ START_TEST(om)
     test_object_identity();
     test_query_directory();
     test_object_permanence();
+    test_zero_access();
 }

[PATCH v2 2/4] ntoskrnl.exe/tests: Open directory object with nonzero access in test_permanent().

[Paul Gofman]

From: Paul Gofman

---
 dlls/ntoskrnl.exe/tests/driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c
index 751f2acff96..7ddbad36fee 100644
--- a/dlls/ntoskrnl.exe/tests/driver.c
+++ b/dlls/ntoskrnl.exe/tests/driver.c
@@ -2289,7 +2289,7 @@ static void test_permanence(void)
     ok(!status, "got %#lx\n", status);

attr.Attributes = 0;
-    status = ZwOpenDirectoryObject( &handle, 0, &attr );
+    status = ZwOpenDirectoryObject( &handle, DIRECTORY_ALL_ACCESS, &attr );
     ok(!status, "got %#lx\n", status);
     status = ZwMakeTemporaryObject( handle );
     ok(!status, "got %#lx\n", status);
@@ -2303,7 +2303,7 @@ static void test_permanence(void)
     status = ZwCreateDirectoryObject( &handle, GENERIC_ALL, &attr );
     ok(!status, "got %#lx\n", status);
     attr.Attributes = OBJ_PERMANENT;
-    status = ZwOpenDirectoryObject( &handle2, 0, &attr );
+    status = ZwOpenDirectoryObject( &handle2, DIRECTORY_ALL_ACCESS, &attr );
     ok(status == STATUS_SUCCESS, "got %#lx\n", status);
     status = ZwClose( handle2 );
     ok(!status, "got %#lx\n", status);

[PATCH v2 3/4] server: Do not allow zero access mask when opening some objects.

[Paul Gofman]

From: Paul Gofman

---
 dlls/advapi32/tests/security.c |  4 ----
 dlls/kernel32/tests/sync.c     |  2 --
 dlls/ntdll/tests/om.c          | 21 +++++++--------------
 dlls/user32/tests/winstation.c |  4 ++++
 server/handle.c                | 11 ++++++++++-
 server/handle.h                |  2 ++
 server/process.c               |  2 +-
 server/thread.c                |  2 +-
 8 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 0917b144648..84a6139b3a8 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -5542,9 +5542,7 @@ static void test_mutex_security(HANDLE token)

SetLastError(0xdeadbeef);
         dup = OpenMutexA(0, FALSE, "WineTestMutex");
-        todo_wine
         ok(!dup, "OpenMutex should fail\n");
-        todo_wine
         ok(GetLastError() == ERROR_ACCESS_DENIED, "wrong error %lu\n", GetLastError());
     }

@@ -5599,9 +5597,7 @@ static void test_event_security(HANDLE token)

SetLastError(0xdeadbeef);
         dup = OpenEventA(0, FALSE, "WineTestEvent");
-        todo_wine
         ok(!dup, "OpenEvent should fail\n");
-        todo_wine
         ok(GetLastError() == ERROR_ACCESS_DENIED, "wrong error %lu\n", GetLastError());
     }

diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c
index c06ced47298..abadc4b1397 100644
--- a/dlls/kernel32/tests/sync.c
+++ b/dlls/kernel32/tests/sync.c
@@ -247,9 +247,7 @@ static void test_mutex(void)

SetLastError(0xdeadbeef);
     hOpened = OpenMutexA(0, FALSE, "WineTestMutex");
-    todo_wine
     ok(hOpened == NULL, "OpenMutex succeeded\n");
-    todo_wine
     ok(GetLastError() == ERROR_ACCESS_DENIED, "wrong error %lu\n", GetLastError());

SetLastError(0xdeadbeef);
diff --git a/dlls/ntdll/tests/om.c b/dlls/ntdll/tests/om.c
index db767d9d56b..fd23038500a 100644
--- a/dlls/ntdll/tests/om.c
+++ b/dlls/ntdll/tests/om.c
@@ -3402,13 +3402,11 @@ static void test_zero_access(void)
     CloseHandle( h2 );

status = NtOpenEvent(&h2, 0, &attr);
-    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
-    if (!status) CloseHandle( h2 );
+    ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );

InitializeObjectAttributes( &attr, &str, OBJ_INHERIT, 0, NULL );
     status = NtOpenEvent(&h2, 0, &attr);
-    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
-    if (!status) CloseHandle( h2 );
+    ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );

status = pNtDuplicateObject( GetCurrentProcess(), h1, GetCurrentProcess(), &h2, 0, 0, 0 );
     ok( !status, "got %#lx.\n", status );
@@ -3423,15 +3421,13 @@ static void test_zero_access(void)
     status = pNtCreateMutant( &h1, 0, &attr, FALSE );
     ok( !status, "got %#lx.\n", status );
     status = NtOpenMutant(&h2, 0, &attr);
-    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
-    if (!status) CloseHandle( h2 );
+    ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
     CloseHandle( h1 );

status = pNtCreateTimer( &h1, 0, &attr, NotificationTimer );
     ok( !status, "got %#lx.\n", status );
     status = pNtOpenTimer( &h2, 0, &attr );
-    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
-    if (!status) CloseHandle( h2 );
+    ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
     CloseHandle( h1 );

status = NtGetNextThread(GetCurrentProcess(), NULL, 0, 0, 0, &h1);
@@ -3442,22 +3438,19 @@ static void test_zero_access(void)
     cid.UniqueProcess = ULongToHandle( GetCurrentProcessId() );
     cid.UniqueThread  = 0;
     status = pNtOpenProcess( &h1, 0, &attr, &cid );
-    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
-    if (!status) CloseHandle( h1 );
+    ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );

InitializeObjectAttributes( &attr, NULL, 0, 0, NULL );
     cid.UniqueProcess = 0;
     cid.UniqueThread  = ULongToHandle( GetCurrentThreadId() );
     status = pNtOpenThread( &h1, 0, &attr, &cid );
-    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
-    if (!status) CloseHandle( h1 );
+    ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );

InitializeObjectAttributes( &attr, &str, OBJ_OPENIF, 0, NULL );
     swprintf( name, ARRAY_SIZE(name), L"\Sessions\%u", NtCurrentTeb()->Peb->SessionId );
     RtlInitUnicodeString( &str, name );
     pNtOpenDirectoryObject( &h1, 0, &attr );
-    todo_wine ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
-    if (!status) CloseHandle( h1 );
+    ok( status == STATUS_ACCESS_DENIED, "got %#lx.\n", status );
 }

START_TEST(om)
diff --git a/dlls/user32/tests/winstation.c b/dlls/user32/tests/winstation.c
index bcbb55ee918..9294db4934f 100644
--- a/dlls/user32/tests/winstation.c
+++ b/dlls/user32/tests/winstation.c
@@ -172,6 +172,10 @@ static void test_handles(void)
     else if (le == ERROR_ACCESS_DENIED)
         win_skip( "Not enough privileges for CreateWindowStation\n" );

+    w2 = OpenWindowStationA("winsta0", TRUE, 0 );
+    ok( !w2, "got non-NULL.\n" );
+    ok( GetLastError() == ERROR_ACCESS_DENIED, "got %ld.\n", GetLastError() );
+
     w2 = OpenWindowStationA("winsta0", TRUE, WINSTA_ALL_ACCESS );
     ok( w2 != 0, "OpenWindowStation failed\n" );
     ok( w2 != w1, "OpenWindowStation returned default handle\n" );
diff --git a/server/handle.c b/server/handle.c
index ef243e06e0b..7d19402ef7d 100644
--- a/server/handle.c
+++ b/server/handle.c
@@ -288,6 +288,15 @@ obj_handle_t alloc_handle( struct process *process, void *ptr, unsigned int acce
     return alloc_handle_entry( process, ptr, access, attr );
 }

+/* allocate handle for opening an object by userspace request */
+obj_handle_t alloc_handle_user_open( struct process *process, void *obj, unsigned int access, unsigned int attr )
+{
+    if (access) return alloc_handle( process, obj, access, attr );
+
+    set_error( STATUS_ACCESS_DENIED );
+    return 0;
+}
+
 /* allocate a global handle for an object, incrementing its refcount */
 /* return the handle, or 0 on error */
 static obj_handle_t alloc_global_handle_no_access_check( void *obj, unsigned int access )
@@ -644,7 +653,7 @@ obj_handle_t open_object( struct process *process, obj_handle_t parent, unsigned

if ((obj = open_named_object( root, ops, name, attributes )))
     {
-        handle = alloc_handle( process, obj, access, attributes );
+        handle = alloc_handle_user_open( process, obj, access, attributes );
         release_object( obj );
     }
     if (root) release_object( root );
diff --git a/server/handle.h b/server/handle.h
index 1d02e040258..ab8ee382297 100644
--- a/server/handle.h
+++ b/server/handle.h
@@ -38,6 +38,8 @@ extern obj_handle_t alloc_handle( struct process *process, void *obj,
                                   unsigned int access, unsigned int attr );
 extern obj_handle_t alloc_handle_no_access_check( struct process *process, void *ptr,
                                                   unsigned int access, unsigned int attr );
+extern obj_handle_t alloc_handle_user_open( struct process *process, void *obj,
+                                            unsigned int access, unsigned int attr );
 extern unsigned int close_handle( struct process *process, obj_handle_t handle );
 extern struct object *get_handle_obj( struct process *process, obj_handle_t handle,
                                       unsigned int access, const struct object_ops *ops );
diff --git a/server/process.c b/server/process.c
index 155dc050d95..488c9a66417 100644
--- a/server/process.c
+++ b/server/process.c
@@ -1442,7 +1442,7 @@ DECL_HANDLER(open_process)
     reply->handle = 0;
     if (process)
     {
-        reply->handle = alloc_handle( current->process, process, req->access, req->attributes );
+        reply->handle = alloc_handle_user_open( current->process, process, req->access, req->attributes );
         release_object( process );
     }
 }
diff --git a/server/thread.c b/server/thread.c
index 6542e1584ab..0ba34511cd5 100644
--- a/server/thread.c
+++ b/server/thread.c
@@ -1477,7 +1477,7 @@ DECL_HANDLER(open_thread)
     reply->handle = 0;
     if (thread)
     {
-        reply->handle = alloc_handle( current->process, thread, req->access, req->attributes );
+        reply->handle = alloc_handle_user_open( current->process, thread, req->access, req->attributes );
         release_object( thread );
     }
 }

[PATCH v2 4/4] server: Skip inaccessible threads in (get_next_thread).

[Paul Gofman]

From: Paul Gofman

---
 dlls/ntdll/tests/om.c |  3 +--
 server/thread.c       | 10 +++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/dlls/ntdll/tests/om.c b/dlls/ntdll/tests/om.c
index fd23038500a..bb6cf9059e1 100644
--- a/dlls/ntdll/tests/om.c
+++ b/dlls/ntdll/tests/om.c
@@ -3431,8 +3431,7 @@ static void test_zero_access(void)
     CloseHandle( h1 );

status = NtGetNextThread(GetCurrentProcess(), NULL, 0, 0, 0, &h1);
-    todo_wine ok( status == STATUS_NO_MORE_ENTRIES, "got %#lx.\n", status );
-    if (!status) CloseHandle( h1 );
+    ok( status == STATUS_NO_MORE_ENTRIES, "got %#lx.\n", status );

InitializeObjectAttributes( &attr, NULL, 0, 0, NULL );
     cid.UniqueProcess = ULongToHandle( GetCurrentProcessId() );
diff --git a/server/thread.c b/server/thread.c
index 0ba34511cd5..b3cf550349a 100644
--- a/server/thread.c
+++ b/server/thread.c
@@ -2023,9 +2023,13 @@ DECL_HANDLER(get_next_thread)
         thread = LIST_ENTRY( ptr, struct thread, entry );
         if (thread->process == process)
         {
-            reply->handle = alloc_handle( current->process, thread, req->access, req->attributes );
-            release_object( process );
-            return;
+            reply->handle = alloc_handle_user_open( current->process, thread, req->access, req->attributes );
+            if (get_error() != STATUS_ACCESS_DENIED)
+            {
+                release_object( process );
+                return;
+            }
+            clear_error();
         }
         ptr = req->flags ? list_prev( &thread_list, &thread->entry )
                          : list_next( &thread_list, &thread->entry );

[PATCH v2 4/4] server: Skip inaccessible threads in (get_next_thread).

[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=146995

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w7u_adm (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w7u_el (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w8 (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w8adm (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

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

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

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

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w1064v1809 (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w1064_tsign (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w10pro64 (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w10pro64_en_AE_u8 (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w11pro64 (32 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

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

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

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

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

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

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w1064v1809 (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w1064_2qxl (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w1064_adm (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w1064_tsign (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w10pro64 (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w10pro64_ar (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w10pro64_ja (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w10pro64_zh_CN (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.

=== w11pro64_amd (64 bit report) ===

ntoskrnl.exe:
Fatal: test 'driver' does not exist.