[PATCH v2 1/3] ntdll/tests: Add tests for DebugPort* info query with security checks.
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntdll/tests/info.c | 190 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 189 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/tests/info.c b/dlls/ntdll/tests/info.c index 6c01ee092d4..4d6104b7502 100644 --- a/dlls/ntdll/tests/info.c +++ b/dlls/ntdll/tests/info.c @@ -46,6 +46,8 @@ static NTSTATUS (WINAPI * pNtQueryObject)(HANDLE, OBJECT_INFORMATION_CLASS, void static NTSTATUS (WINAPI * pNtCreateDebugObject)( HANDLE *, ACCESS_MASK, OBJECT_ATTRIBUTES *, ULONG ); static NTSTATUS (WINAPI * pNtSetInformationDebugObject)(HANDLE,DEBUGOBJECTINFOCLASS,PVOID,ULONG,ULONG*); static NTSTATUS (WINAPI * pDbgUiConvertStateChangeStructure)(DBGUI_WAIT_STATE_CHANGE*,DEBUG_EVENT*); +static HANDLE (WINAPI * pDbgUiGetThreadDebugObject)(void); +static void (WINAPI * pDbgUiSetThreadDebugObject)(HANDLE); static BOOL is_wow64; @@ -99,6 +101,8 @@ static void InitFunctionPtrs(void) NTDLL_GET_PROC(NtSetInformationDebugObject); NTDLL_GET_PROC(NtGetCurrentProcessorNumber); NTDLL_GET_PROC(DbgUiConvertStateChangeStructure); + NTDLL_GET_PROC(DbgUiGetThreadDebugObject); + NTDLL_GET_PROC(DbgUiSetThreadDebugObject); pIsWow64Process = (void *)GetProcAddress(hkernel32, "IsWow64Process"); if (!pIsWow64Process || !pIsWow64Process( GetCurrentProcess(), &is_wow64 )) is_wow64 = FALSE; @@ -2024,6 +2028,146 @@ static void test_query_process_debug_port(int argc, char **argv) ok(ret, "CloseHandle failed, last error %#x.\n", GetLastError()); } +static void subtest_query_process_debug_port_custom_dacl(int argc, char **argv, ACCESS_MASK access, PSID sid) +{ + HANDLE old_debug_obj, debug_obj; + OBJECT_ATTRIBUTES attr; + SECURITY_DESCRIPTOR sd; + union { + ACL acl; + DWORD buffer[(sizeof(ACL) + + (offsetof(ACCESS_ALLOWED_ACE, SidStart) + SECURITY_MAX_SID_SIZE) + + sizeof(DWORD) - 1) / sizeof(DWORD)]; + } acl; + char cmdline[MAX_PATH]; + PROCESS_INFORMATION pi; + STARTUPINFOA si; + DEBUG_EVENT ev; + NTSTATUS status; + BOOL ret; + + if (!InitializeAcl(&acl.acl, sizeof(acl), ACL_REVISION)) + { + ok(0, "Failed to initialise ACL: %u\n", GetLastError()); + return; + } + + if (!AddAccessAllowedAce(&acl.acl, ACL_REVISION, access, sid)) + { + ok(0, "Failed to add ACE: %u\n", GetLastError()); + return; + } + + if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) + { + ok(0, "Failed to initialise security descriptor: %u\n", GetLastError()); + return; + } + + if (!SetSecurityDescriptorDacl(&sd, TRUE, &acl.acl, FALSE)) + { + ok(0, "Failed to set DACL: %u\n", GetLastError()); + return; + } + + InitializeObjectAttributes(&attr, NULL, 0, NULL, &sd); + status = NtCreateDebugObject(&debug_obj, MAXIMUM_ALLOWED, &attr, DEBUG_KILL_ON_CLOSE); + ok(SUCCEEDED(status), "Failed to create debug object: %#010x\n", status); + if (!SUCCEEDED(status)) return; + + old_debug_obj = pDbgUiGetThreadDebugObject(); + pDbgUiSetThreadDebugObject(debug_obj); + + sprintf(cmdline, "%s %s %s %u", argv[0], argv[1], "debuggee:dbgport", access); + + memset(&si, 0, sizeof(si)); + si.cb = sizeof(si); + ret = CreateProcessA(NULL, cmdline, NULL, NULL, FALSE, + DEBUG_PROCESS, NULL, NULL, &si, &pi); + ok(ret, "CreateProcess failed, last error %#x.\n", GetLastError()); + if (!ret) goto close_debug_obj; + + do + { + ret = WaitForDebugEvent(&ev, INFINITE); + ok(ret, "WaitForDebugEvent failed, last error %#x.\n", GetLastError()); + if (!ret) break; + + ret = ContinueDebugEvent(ev.dwProcessId, ev.dwThreadId, DBG_CONTINUE); + ok(ret, "ContinueDebugEvent failed, last error %#x.\n", GetLastError()); + if (!ret) break; + } while (ev.dwDebugEventCode != EXIT_PROCESS_DEBUG_EVENT); + + wait_child_process(pi.hProcess); + ret = CloseHandle(pi.hThread); + ok(ret, "CloseHandle failed, last error %#x.\n", GetLastError()); + ret = CloseHandle(pi.hProcess); + ok(ret, "CloseHandle failed, last error %#x.\n", GetLastError()); + +close_debug_obj: + pDbgUiSetThreadDebugObject(old_debug_obj); + NtClose(debug_obj); +} + +static TOKEN_OWNER *get_current_owner(void) +{ + TOKEN_OWNER *owner; + ULONG length = 0; + HANDLE token; + BOOL ret; + + ret = OpenProcessToken(GetCurrentProcess(), TOKEN_ALL_ACCESS, &token); + ok(ret, "Failed to get process token: %u\n", GetLastError()); + + ret = GetTokenInformation(token, TokenOwner, NULL, 0, &length); + ok(!ret && GetLastError() == ERROR_INSUFFICIENT_BUFFER, + "GetTokenInformation failed: %u\n", GetLastError()); + ok(length != 0, "Failed to get token owner information length: %u\n", GetLastError()); + + owner = HeapAlloc(GetProcessHeap(), 0, length); + ret = GetTokenInformation(token, TokenOwner, owner, length, &length); + ok(ret, "Failed to get token owner information: %u)\n", GetLastError()); + + CloseHandle(token); + return owner; +} + +static void test_query_process_debug_port_custom_dacl(int argc, char **argv) +{ + static const ACCESS_MASK all_access_masks[] = { + GENERIC_ALL, + DEBUG_ALL_ACCESS, + STANDARD_RIGHTS_REQUIRED | SYNCHRONIZE, + }; + TOKEN_OWNER *owner; + int i; + + if (!pDbgUiSetThreadDebugObject) + { + skip("DbgUiGetThreadDebugObject not found\n"); + return; + } + + if (!pDbgUiGetThreadDebugObject) + { + skip("DbgUiSetThreadDebugObject not found\n"); + return; + } + + owner = get_current_owner(); + + for (i = 0; i < ARRAY_SIZE(all_access_masks); i++) + { + ACCESS_MASK access = all_access_masks[i]; + + winetest_push_context("debug object access %08x", access); + subtest_query_process_debug_port_custom_dacl(argc, argv, access, owner->Owner); + winetest_pop_context(); + } + + HeapFree(GetProcessHeap(), 0, owner); +} + static void test_query_process_priority(void) { PROCESS_PRIORITY_CLASS priority[2]; @@ -3371,6 +3515,45 @@ static void test_process_instrumentation_callback(void) "Got unexpected status %#x.\n", status ); } +static void test_debuggee_dbgport(int argc, char **argv) +{ + NTSTATUS status, expect_status; + DWORD_PTR debug_port = 0xdeadbeef; + DWORD debug_flags = 0xdeadbeef; + HANDLE handle; + ACCESS_MASK access; + + if (argc < 2) + { + ok(0, "insufficient arguments for child process\n"); + return; + } + + access = strtoul(argv[1], NULL, 0); + winetest_push_context("debug object access %08x", access); + + status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessDebugPort, + &debug_port, sizeof(debug_port), NULL ); + todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) + ok( !status, "NtQueryInformationProcess ProcessDebugPort failed, status %#x.\n", status ); + todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) + ok( debug_port == ~(DWORD_PTR)0, "Expected port %#lx, got %#lx.\n", ~(DWORD_PTR)0, debug_port ); + + status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessDebugFlags, + &debug_flags, sizeof(debug_flags), NULL ); + todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) + ok( !status, "NtQueryInformationProcess ProcessDebugFlags failed, status %#x.\n", status ); + + expect_status = access ? STATUS_SUCCESS : STATUS_ACCESS_DENIED; + status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessDebugObjectHandle, + &handle, sizeof(handle), NULL ); + todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) + ok( status == expect_status, "NtQueryInformationProcess ProcessDebugObjectHandle expected status %#x, actual %#x.\n", expect_status, status ); + if (SUCCEEDED( status )) NtClose( handle ); + + winetest_pop_context(); +} + START_TEST(info) { char **argv; @@ -3379,7 +3562,11 @@ START_TEST(info) InitFunctionPtrs(); argc = winetest_get_mainargs(&argv); - if (argc >= 3) return; /* Child */ + if (argc >= 3) + { + if (strcmp(argv[2], "debuggee:dbgport") == 0) test_debuggee_dbgport(argc - 2, argv + 2); + return; /* Child */ + } /* NtQuerySystemInformation */ test_query_basic(); @@ -3413,6 +3600,7 @@ START_TEST(info) test_query_process_vm(); test_query_process_times(); test_query_process_debug_port(argc, argv); + test_query_process_debug_port_custom_dacl(argc, argv); test_query_process_priority(); test_query_process_handlecount(); test_query_process_wow64(); -- 2.31.1
Today, Wine uses NtQueryInformationProcess/ProcessDebugPort to detect whether the current process is being debugged. If it is, the process issues a breakpoint to yield control to the debugger. Some debuggers (e.g. latest CDB) appear to create debug handles with restricted DACL, which causes querying debug port to fail with STATUS_ACCESS_DENIED. This results in the debuggee erroneously skipping the initial breakpoint. Fix this by not requiring DEBUG_ALL_ACCESS when opening the debug port object. Instead, use MAXIMUM_ALLOWED for the access mask. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52184 Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntdll/tests/info.c | 4 ---- server/process.c | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/dlls/ntdll/tests/info.c b/dlls/ntdll/tests/info.c index 4d6104b7502..a821dada902 100644 --- a/dlls/ntdll/tests/info.c +++ b/dlls/ntdll/tests/info.c @@ -3534,20 +3534,16 @@ static void test_debuggee_dbgport(int argc, char **argv) status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessDebugPort, &debug_port, sizeof(debug_port), NULL ); - todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) ok( !status, "NtQueryInformationProcess ProcessDebugPort failed, status %#x.\n", status ); - todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) ok( debug_port == ~(DWORD_PTR)0, "Expected port %#lx, got %#lx.\n", ~(DWORD_PTR)0, debug_port ); status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessDebugFlags, &debug_flags, sizeof(debug_flags), NULL ); - todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) ok( !status, "NtQueryInformationProcess ProcessDebugFlags failed, status %#x.\n", status ); expect_status = access ? STATUS_SUCCESS : STATUS_ACCESS_DENIED; status = pNtQueryInformationProcess( GetCurrentProcess(), ProcessDebugObjectHandle, &handle, sizeof(handle), NULL ); - todo_wine_if(access != DEBUG_ALL_ACCESS && access != GENERIC_ALL) ok( status == expect_status, "NtQueryInformationProcess ProcessDebugObjectHandle expected status %#x, actual %#x.\n", expect_status, status ); if (SUCCEEDED( status )) NtClose( handle ); diff --git a/server/process.c b/server/process.c index 6d794ba5ead..81c94a3c81d 100644 --- a/server/process.c +++ b/server/process.c @@ -1528,7 +1528,7 @@ DECL_HANDLER(get_process_debug_info) reply->debug_children = process->debug_children; if (!process->debug_obj) set_error( STATUS_PORT_NOT_SET ); - else reply->debug = alloc_handle( current->process, process->debug_obj, DEBUG_ALL_ACCESS, 0 ); + else reply->debug = alloc_handle( current->process, process->debug_obj, MAXIMUM_ALLOWED, 0 ); release_object( process ); } -- 2.31.1
Make retrieval of debug port object handle optional. Also, skip debug port object handle retrieval if serving requests that don't need it (i.e. ProcessDebugPort and ProcessDebugFlags). This also eliminates the extra round trip to the server for closing the unneeded debug port object handle. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntdll/unix/process.c | 13 ++++--------- server/process.c | 2 +- server/protocol.def | 1 + 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/dlls/ntdll/unix/process.c b/dlls/ntdll/unix/process.c index c834ef85c79..dfe040eeb25 100644 --- a/dlls/ntdll/unix/process.c +++ b/dlls/ntdll/unix/process.c @@ -1259,19 +1259,16 @@ NTSTATUS WINAPI NtQueryInformationProcess( HANDLE handle, PROCESSINFOCLASS class if (!info) ret = STATUS_ACCESS_VIOLATION; else { - HANDLE debug; - SERVER_START_REQ(get_process_debug_info) { req->handle = wine_server_obj_handle( handle ); + req->want_debug_obj = 0; ret = wine_server_call( req ); - debug = wine_server_ptr_handle( reply->debug ); } SERVER_END_REQ; if (ret == STATUS_SUCCESS) { *(DWORD_PTR *)info = ~0ul; - NtClose( debug ); } else if (ret == STATUS_PORT_NOT_SET) { @@ -1290,18 +1287,15 @@ NTSTATUS WINAPI NtQueryInformationProcess( HANDLE handle, PROCESSINFOCLASS class if (!info) ret = STATUS_ACCESS_VIOLATION; else { - HANDLE debug; - SERVER_START_REQ(get_process_debug_info) { req->handle = wine_server_obj_handle( handle ); + req->want_debug_obj = 0; ret = wine_server_call( req ); - debug = wine_server_ptr_handle( reply->debug ); *(DWORD *)info = reply->debug_children; } SERVER_END_REQ; - if (ret == STATUS_SUCCESS) NtClose( debug ); - else if (ret == STATUS_PORT_NOT_SET) ret = STATUS_SUCCESS; + if (ret == STATUS_PORT_NOT_SET) ret = STATUS_SUCCESS; } } else ret = STATUS_INFO_LENGTH_MISMATCH; @@ -1323,6 +1317,7 @@ NTSTATUS WINAPI NtQueryInformationProcess( HANDLE handle, PROCESSINFOCLASS class SERVER_START_REQ(get_process_debug_info) { req->handle = wine_server_obj_handle( handle ); + req->want_debug_obj = 1; ret = wine_server_call( req ); *(HANDLE *)info = wine_server_ptr_handle( reply->debug ); } diff --git a/server/process.c b/server/process.c index 81c94a3c81d..0f33dcfffa2 100644 --- a/server/process.c +++ b/server/process.c @@ -1528,7 +1528,7 @@ DECL_HANDLER(get_process_debug_info) reply->debug_children = process->debug_children; if (!process->debug_obj) set_error( STATUS_PORT_NOT_SET ); - else reply->debug = alloc_handle( current->process, process->debug_obj, MAXIMUM_ALLOWED, 0 ); + else if (req->want_debug_obj) reply->debug = alloc_handle( current->process, process->debug_obj, MAXIMUM_ALLOWED, 0 ); release_object( process ); } diff --git a/server/protocol.def b/server/protocol.def index c83e6a2ef7c..1e73e2783f8 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -968,6 +968,7 @@ typedef struct /* Retrieve debug information about a process */ @REQ(get_process_debug_info) obj_handle_t handle; /* process handle */ + int want_debug_obj; /* want debug port object? */ @REPLY obj_handle_t debug; /* handle to debug port */ int debug_children; /* inherit debugger to child processes */ -- 2.31.1
participants (1)
-
Jinoh Kang