[PATCH v4 0/3] MR6602: explorer, services: Use admin tokens when appropriate.
Let me know if you'd like me to split this up. As per feedback, I'm now doing the admin token-setting via an extension to NtSetInformationProcess. Testing on Windows shows that the owner of GetDesktopWindow() always has a default admin token with `TokenElevationTypeDefault`, no matter the token of the process/thread that is responsible for creating it. We've had issues in this area in the past - 99e2fad1 was a case where it was important that explorer not inherit the token of the *process* spawning it, but instead the token of the *thread*. This patch keeps that app working, since now explorer will set a default token regardless. In addition to the privilege issues from 99e2fad1, it is a relatively common pattern to duplicate the token of the owner of GetDesktopWindow to acquire a default token. Services are also currently launched with limited tokens, so this series also has services.exe elevate itself. -- v4: kernelbase: Improve logging of information classes in GetTokenInformation. Revert "win32u: Create explorer with the thread effective access token." explorer: Apply a default admin token when running for the desktop. https://gitlab.winehq.org/wine/wine/-/merge_requests/6602
From: Tim Clem <tclem(a)codeweavers.com> --- dlls/ntdll/unix/process.c | 9 +++++++++ dlls/wow64/process.c | 1 + include/winternl.h | 1 + programs/explorer/desktop.c | 6 ++++++ server/process.c | 27 +++++++++++++++++++++++++++ server/protocol.def | 6 ++++++ 6 files changed, 50 insertions(+) diff --git a/dlls/ntdll/unix/process.c b/dlls/ntdll/unix/process.c index 6872a2460b2..c3a730379a4 100644 --- a/dlls/ntdll/unix/process.c +++ b/dlls/ntdll/unix/process.c @@ -1837,6 +1837,15 @@ NTSTATUS WINAPI NtSetInformationProcess( HANDLE handle, PROCESSINFOCLASS class, SERVER_END_REQ; return ret; + case ProcessWineGrantAdminToken: + SERVER_START_REQ( grant_process_admin_token ) + { + req->handle = wine_server_obj_handle( handle ); + ret = wine_server_call( req ); + } + SERVER_END_REQ; + break; + case ProcessPowerThrottlingState: FIXME( "ProcessPowerThrottlingState - stub\n" ); return STATUS_SUCCESS; diff --git a/dlls/wow64/process.c b/dlls/wow64/process.c index 0153dba3433..4d067cea06f 100644 --- a/dlls/wow64/process.c +++ b/dlls/wow64/process.c @@ -872,6 +872,7 @@ NTSTATUS WINAPI wow64_NtSetInformationProcess( UINT *args ) case ProcessPagePriority: /* MEMORY_PRIORITY_INFORMATION */ case ProcessPowerThrottlingState: /* PROCESS_POWER_THROTTLING_STATE */ case ProcessLeapSecondInformation: /* PROCESS_LEAP_SECOND_INFO */ + case ProcessWineGrantAdminToken: /* NULL */ return NtSetInformationProcess( handle, class, ptr, len ); case ProcessAccessToken: /* PROCESS_ACCESS_TOKEN */ diff --git a/include/winternl.h b/include/winternl.h index a1e6cca2251..0bae9fbf385 100644 --- a/include/winternl.h +++ b/include/winternl.h @@ -1897,6 +1897,7 @@ typedef enum _PROCESSINFOCLASS { #ifdef __WINESRC__ ProcessWineMakeProcessSystem = 1000, ProcessWineLdtCopy, + ProcessWineGrantAdminToken, #endif } PROCESSINFOCLASS; diff --git a/programs/explorer/desktop.c b/programs/explorer/desktop.c index e526b468b7d..e0b57a4b37f 100644 --- a/programs/explorer/desktop.c +++ b/programs/explorer/desktop.c @@ -24,6 +24,7 @@ #define COBJMACROS #define OEMRESOURCE #include <windows.h> +#include <winternl.h> #include <rpc.h> #include <shlobj.h> #include <shellapi.h> @@ -1208,6 +1209,7 @@ void manage_desktop( WCHAR *arg ) HMODULE shell32; HANDLE thread; DWORD id; + NTSTATUS status; /* get the rest of the command line (if any) */ while (*p && !is_whitespace(*p)) p++; @@ -1260,6 +1262,10 @@ void manage_desktop( WCHAR *arg ) SetThreadDesktop( desktop ); } + /* the desktop process should always have an admin token */ + status = NtSetInformationProcess( GetCurrentProcess(), ProcessWineGrantAdminToken, NULL, 0 ); + if (status) WARN( "couldn't set admin token for desktop, error %08lx\n", status ); + /* create the desktop window */ hwnd = CreateWindowExW( 0, DESKTOP_CLASS_ATOM, NULL, WS_POPUP | WS_CLIPSIBLINGS | WS_CLIPCHILDREN, 0, 0, 0, 0, 0, 0, 0, &guid ); diff --git a/server/process.c b/server/process.c index 737b37d9441..00aa6a18f1f 100644 --- a/server/process.c +++ b/server/process.c @@ -1721,6 +1721,33 @@ DECL_HANDLER(make_process_system) release_object( process ); } +/* grant a process a primary admin token with Default elevation */ +DECL_HANDLER(grant_process_admin_token) +{ + struct process *process; + struct token *token; + + if (!(process = get_process_from_handle( req->handle, PROCESS_SET_INFORMATION ))) + { + set_error( STATUS_INVALID_HANDLE ); + return; + } + + token = token_create_admin( TRUE, SecurityIdentification, + TokenElevationTypeDefault, default_session_id ); + if (!token) + { + release_object( process ); + set_error( STATUS_UNSUCCESSFUL ); + return; + } + + release_object( process->token ); + process->token = token; + + release_object( process ); +} + /* create a new job object */ DECL_HANDLER(create_job) { diff --git a/server/protocol.def b/server/protocol.def index bd0307c5d79..4b9c4dc8e5c 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3755,6 +3755,12 @@ struct handle_info @END +/* Grant the given process an admin token with Default elevation */ +(a)REQ(grant_process_admin_token) + obj_handle_t handle; /* handle to the process */ +(a)END + + /* Get detailed fixed-size information about a token */ @REQ(get_token_info) obj_handle_t handle; /* handle to the object */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6602
From: Tim Clem <tclem(a)codeweavers.com> This reverts commit 99e2fad1bf4433d82c3f77c9bdeac1872e6d6ee9. --- dlls/win32u/winstation.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index d8aff54009b..0d61f645c46 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -808,8 +808,7 @@ HWND get_desktop_window(void) static const WCHAR system_dir[] = {'C',':','\\','w','i','n','d','o','w','s','\\', 's','y','s','t','e','m','3','2','\\',0}; RTL_USER_PROCESS_PARAMETERS params = { sizeof(params), sizeof(params) }; - ULONG_PTR buffer[offsetof( PS_ATTRIBUTE_LIST, Attributes[2] ) / sizeof(ULONG_PTR)]; - PS_ATTRIBUTE_LIST *ps_attr = (PS_ATTRIBUTE_LIST *)buffer; + PS_ATTRIBUTE_LIST ps_attr; PS_CREATE_INFO create_info; WCHAR desktop[MAX_PATH]; PEB *peb = NtCurrentTeb()->Peb; @@ -842,30 +841,24 @@ HWND get_desktop_window(void) RtlInitUnicodeString( ¶ms.WindowTitle, appnameW + 4 ); RtlInitUnicodeString( ¶ms.Desktop, desktop ); - ps_attr->Attributes[0].Attribute = PS_ATTRIBUTE_IMAGE_NAME; - ps_attr->Attributes[0].Size = sizeof(appnameW) - sizeof(WCHAR); - ps_attr->Attributes[0].ValuePtr = (WCHAR *)appnameW; - ps_attr->Attributes[0].ReturnLength = NULL; - - ps_attr->Attributes[1].Attribute = PS_ATTRIBUTE_TOKEN; - ps_attr->Attributes[1].Size = sizeof(HANDLE); - ps_attr->Attributes[1].ValuePtr = GetCurrentThreadEffectiveToken(); - ps_attr->Attributes[1].ReturnLength = NULL; - - ps_attr->TotalLength = offsetof( PS_ATTRIBUTE_LIST, Attributes[2] ); + ps_attr.TotalLength = sizeof(ps_attr); + ps_attr.Attributes[0].Attribute = PS_ATTRIBUTE_IMAGE_NAME; + ps_attr.Attributes[0].Size = sizeof(appnameW) - sizeof(WCHAR); + ps_attr.Attributes[0].ValuePtr = (WCHAR *)appnameW; + ps_attr.Attributes[0].ReturnLength = NULL; if (NtCurrentTeb64() && !NtCurrentTeb64()->TlsSlots[WOW64_TLS_FILESYSREDIR]) { NtCurrentTeb64()->TlsSlots[WOW64_TLS_FILESYSREDIR] = TRUE; status = NtCreateUserProcess( &process, &thread, PROCESS_ALL_ACCESS, THREAD_ALL_ACCESS, NULL, NULL, 0, THREAD_CREATE_FLAGS_CREATE_SUSPENDED, ¶ms, - &create_info, ps_attr ); + &create_info, &ps_attr ); NtCurrentTeb64()->TlsSlots[WOW64_TLS_FILESYSREDIR] = FALSE; } else status = NtCreateUserProcess( &process, &thread, PROCESS_ALL_ACCESS, THREAD_ALL_ACCESS, NULL, NULL, 0, THREAD_CREATE_FLAGS_CREATE_SUSPENDED, ¶ms, - &create_info, ps_attr ); + &create_info, &ps_attr ); if (!status) { NtResumeThread( thread, NULL ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6602
From: Tim Clem <tclem(a)codeweavers.com> --- dlls/kernelbase/security.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dlls/kernelbase/security.c b/dlls/kernelbase/security.c index 1dd975a7aad..fd0eb4e561d 100644 --- a/dlls/kernelbase/security.c +++ b/dlls/kernelbase/security.c @@ -704,8 +704,8 @@ BOOL WINAPI DuplicateTokenEx( HANDLE token, DWORD access, LPSECURITY_ATTRIBUTES BOOL WINAPI GetTokenInformation( HANDLE token, TOKEN_INFORMATION_CLASS class, LPVOID info, DWORD len, LPDWORD retlen ) { - TRACE("(%p, %s, %p, %ld, %p):\n", - token, + TRACE("(%p, %d [%s], %p, %ld, %p):\n", + token, class, (class == TokenUser) ? "TokenUser" : (class == TokenGroups) ? "TokenGroups" : (class == TokenPrivileges) ? "TokenPrivileges" : @@ -721,6 +721,9 @@ BOOL WINAPI GetTokenInformation( HANDLE token, TOKEN_INFORMATION_CLASS class, (class == TokenGroupsAndPrivileges) ? "TokenGroupsAndPrivileges" : (class == TokenSessionReference) ? "TokenSessionReference" : (class == TokenSandBoxInert) ? "TokenSandBoxInert" : + (class == TokenElevation) ? "TokenElevation" : + (class == TokenElevationType) ? "TokenElevationType" : + (class == TokenLinkedToken) ? "TokenLinkedToken" : "Unknown", info, len, retlen); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6602
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=150175 Your paranoid android. === debian11b (64 bit WoW report) === user32: win.c:4070: Test failed: Expected active window 0000000002AB0160, got 0000000000000000. win.c:4071: Test failed: Expected focus window 0000000002AB0160, got 0000000000000000.
On Fri Oct 18 15:01:39 2024 +0000, Alexandre Julliard wrote:
Please avoid nesting server requests like that. It looks like this could be done just as well as a single request instead of 2 (actually 3 since you need to close the handle). v3 gave this its own server call. I also removed anything involving services.exe for now, since I know it leads to at least one regression (ultimately involving our stub for WTSQueryUserToken).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6602#note_89612
participants (3)
-
Marvin -
Tim Clem -
Tim Clem (＠tclem)