[PATCH 0/4] MR6047: server: Deny opening handles to objects with zero access.
From: Paul Gofman <pgofman(a)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
From: Paul Gofman <pgofman(a)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
From: Paul Gofman <pgofman(a)codeweavers.com> --- 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 +- 6 files changed, 25 insertions(+), 17 deletions(-) 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
From: Paul Gofman <pgofman(a)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
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) === 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. === debian11 (build log) === WineRunWineTest.pl:error: The task timed out === debian11b (64 bit WoW report) === advapi32: security.c:5546: Test succeeded inside todo block: OpenMutex should fail security.c:5548: Test succeeded inside todo block: wrong error 5 security.c:5546: Test succeeded inside todo block: OpenMutex should fail security.c:5548: Test succeeded inside todo block: wrong error 5 security.c:5546: Test succeeded inside todo block: OpenMutex should fail security.c:5548: Test succeeded inside todo block: wrong error 5 security.c:5546: Test succeeded inside todo block: OpenMutex should fail security.c:5548: Test succeeded inside todo block: wrong error 5 security.c:5546: Test succeeded inside todo block: OpenMutex should fail security.c:5548: Test succeeded inside todo block: wrong error 5 security.c:5603: Test succeeded inside todo block: OpenEvent should fail security.c:5605: Test succeeded inside todo block: wrong error 5 security.c:5603: Test succeeded inside todo block: OpenEvent should fail security.c:5605: Test succeeded inside todo block: wrong error 5 security.c:5603: Test succeeded inside todo block: OpenEvent should fail security.c:5605: Test succeeded inside todo block: wrong error 5 security.c:5603: Test succeeded inside todo block: OpenEvent should fail security.c:5605: Test succeeded inside todo block: wrong error 5 security.c:5603: Test succeeded inside todo block: OpenEvent should fail security.c:5605: Test succeeded inside todo block: wrong error 5 kernel32: sync.c:251: Test succeeded inside todo block: OpenMutex succeeded sync.c:253: Test succeeded inside todo block: wrong error 5 user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000019B00DC, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
Fixes "Knowledge, or know Lady" (Steam game id 2786680) closing itself after 10 minutes of runtime (relying on OpenEvent() with zero access mask to fail). Patch 2 works around already present test failures which are reproducible here with Wine (denied access for ZwMakeTemporaryObject() when handled is opened with 0 access). Although patch 3 would add a bit more to it (ZwOpenDirectoryObject would not work with zero access). From the userspace ZwOpenDirectoryObject works identical to NtOpenDirectoryObject and doesn't allow opening directory with zero access). "Special" ntoskrnl functions (like ObOpenObjectByPointer) have explicit control where a handle should be opened in user mode or, in kernel mode (which is the only ObOpenObjectByPointer currently supports) it bypasses any access right checks. That was one of the reasons I concluded introducing an explicit alloc_handle_user_open() is better than trying to change alloc_handle() to check for the ask (which is also used with certain "no_type" objects with zero access). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6047#note_75856
participants (3)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)