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

List overview All Threads

newer

older

[PATCH v2 0/4] MR6047: server:...

[PATCH v2 0/7] MR6041: Cmd engine...

Paul Gofman (＠gofman)

10 Jul 2024 10 Jul '24

11:06 p.m.

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

Show replies by date

Paul Gofman

10 Jul 10 Jul

11:06 p.m.

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

From: Paul Gofman pgofman@codeweavers.com

--- 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(); }

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

Paul Gofman

11:06 p.m.

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

From: Paul Gofman pgofman@codeweavers.com

--- 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..6283803d31d 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, GENERIC_ALL, &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, GENERIC_ALL, &attr ); ok(status == STATUS_SUCCESS, "got %#lx\n", status); status = ZwClose( handle2 ); ok(!status, "got %#lx\n", status);

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

Paul Gofman

11:06 p.m.

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

From: Paul Gofman pgofman@codeweavers.com

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 ); } }

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

Paul Gofman

11:06 p.m.

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

From: Paul Gofman pgofman@codeweavers.com

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

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

Marvin

11 Jul 11 Jul

12:19 a.m.

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

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

Your paranoid android.

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