[PATCH v2 0/4] MR6602: win32u: Always launch explorer for the desktop with a default token.
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 get 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. -- v2: services: Apply a default admin token to the process. ntdll: Count default-elevation admin tokens as elevated from NtQueryInformationToken(TokenElevation). 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 | 13 +++++++++++++ dlls/wow64/process.c | 1 + include/winternl.h | 1 + programs/explorer/desktop.c | 6 ++++++ server/protocol.def | 7 +++++++ server/token.c | 16 ++++++++++++++++ 6 files changed, 44 insertions(+) diff --git a/dlls/ntdll/unix/process.c b/dlls/ntdll/unix/process.c index f3551c2e7d7..674d0d0068d 100644 --- a/dlls/ntdll/unix/process.c +++ b/dlls/ntdll/unix/process.c @@ -1775,6 +1775,19 @@ NTSTATUS WINAPI NtSetInformationProcess( HANDLE handle, PROCESSINFOCLASS class, SERVER_END_REQ; return ret; + case ProcessWineSetAdminToken: + SERVER_START_REQ( create_primary_admin_token ) + { + if (!(ret = wine_server_call( req ))) + { + HANDLE token = wine_server_ptr_handle( reply->token ); + PROCESS_ACCESS_TOKEN info = { .Token = token, .Thread = NULL }; + ret = NtSetInformationProcess( handle, ProcessAccessToken, &info, sizeof(info) ); + } + } + SERVER_END_REQ; + return ret; + case ProcessPowerThrottlingState: FIXME( "ProcessPowerThrottlingState - stub\n" ); return STATUS_SUCCESS; diff --git a/dlls/wow64/process.c b/dlls/wow64/process.c index 4f6233002bc..1c0057eff9a 100644 --- a/dlls/wow64/process.c +++ b/dlls/wow64/process.c @@ -871,6 +871,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 ProcessWineSetAdminToken: /* NULL */ return NtSetInformationProcess( handle, class, ptr, len ); case ProcessAccessToken: /* PROCESS_ACCESS_TOKEN */ diff --git a/include/winternl.h b/include/winternl.h index 4f24a921bb1..1f4fdc172bd 100644 --- a/include/winternl.h +++ b/include/winternl.h @@ -1897,6 +1897,7 @@ typedef enum _PROCESSINFOCLASS { #ifdef __WINESRC__ ProcessWineMakeProcessSystem = 1000, ProcessWineLdtCopy, + ProcessWineSetAdminToken, #endif } PROCESSINFOCLASS; diff --git a/programs/explorer/desktop.c b/programs/explorer/desktop.c index 800b03f48bf..1e70a5b74da 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(), ProcessWineSetAdminToken, 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/protocol.def b/server/protocol.def index c6af7379f17..a7a89724d31 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3279,6 +3279,13 @@ enum caret_state @END +/* Create a new primary admin token with Default elevation. */ +(a)REQ(create_primary_admin_token) +(a)REPLY + obj_handle_t token; /* handle to the token */ +(a)END + + /* Open a security token */ @REQ(open_token) obj_handle_t handle; /* handle to the thread or process */ diff --git a/server/token.c b/server/token.c index 48ee1eca8fe..698b2627ccb 100644 --- a/server/token.c +++ b/server/token.c @@ -1209,6 +1209,22 @@ DECL_HANDLER(create_token) } +/* create a new primary admin token with Default elevation */ +DECL_HANDLER(create_primary_admin_token) +{ + struct token *token = token_create_admin( TRUE, SecurityIdentification, + TokenElevationTypeDefault, default_session_id ); + if (!token) + { + set_error( STATUS_UNSUCCESSFUL ); + return; + } + + reply->token = alloc_handle( current->process, token, TOKEN_ALL_ACCESS, 0 ); + release_object( token ); +} + + /* open a security token */ DECL_HANDLER(open_token) { -- 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 d2386fd5af5..386f6924c2f 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -765,8 +765,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; @@ -799,30 +798,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/advapi32/tests/security.c | 37 ++++++++++++++++++++++++++++++++++ dlls/kernelbase/security.c | 6 ++++-- dlls/ntdll/unix/security.c | 4 ++-- server/protocol.def | 3 ++- server/token.c | 9 ++++++++- 5 files changed, 53 insertions(+), 6 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 0917b144648..6d0be6cd987 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -28,6 +28,7 @@ #include "winbase.h" #include "winerror.h" #include "winternl.h" +#include "winuser.h" #include "aclapi.h" #include "winnt.h" #include "sddl.h" @@ -8537,6 +8538,42 @@ static void test_elevation(void) CloseHandle(token); } +static void test_admin_elevation(void) +{ + /* Tokens with elevation type TokenElevationTypeDefault should still come + back as elevated from a TokenElevation query if they belong to the admin + group. The owner of the desktop window should have such a token. */ + DWORD tid, pid; + HANDLE hproc, htok; + TOKEN_ELEVATION_TYPE elevation_type; + TOKEN_ELEVATION elevation; + DWORD size; + BOOL ret; + + tid = GetWindowThreadProcessId(GetDesktopWindow(), &pid); + ok(tid, "got error %lu\n", GetLastError()); + + hproc = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE, pid); + ok(hproc != NULL, "got error %lu\n", GetLastError()); + + ret = OpenProcessToken(hproc, TOKEN_READ, &htok); + ok(ret, "got error %lu\n", GetLastError()); + + CloseHandle(hproc); + + size = sizeof(elevation_type); + ret = GetTokenInformation(htok, TokenElevationType, &elevation_type, size, &size); + ok(ret, "got error %lu\n", GetLastError()); + ok(elevation_type == TokenElevationTypeDefault, "unexpected elevation type %d\n", elevation_type); + + size = sizeof(elevation); + ret = GetTokenInformation(htok, TokenElevation, &elevation, size, &size); + ok(ret, "got error %lu\n", GetLastError()); + ok(elevation.TokenIsElevated, "expected token to be elevated\n"); + + CloseHandle(htok); +} + static void test_group_as_file_owner(void) { char sd_buffer[200], sid_buffer[100]; diff --git a/dlls/kernelbase/security.c b/dlls/kernelbase/security.c index 1dd975a7aad..dcf361882df 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,8 @@ BOOL WINAPI GetTokenInformation( HANDLE token, TOKEN_INFORMATION_CLASS class, (class == TokenGroupsAndPrivileges) ? "TokenGroupsAndPrivileges" : (class == TokenSessionReference) ? "TokenSessionReference" : (class == TokenSandBoxInert) ? "TokenSandBoxInert" : + (class == TokenElevation) ? "TokenElevation" : + (class == TokenElevationType) ? "TokenElevationType" : "Unknown", info, len, retlen); diff --git a/dlls/ntdll/unix/security.c b/dlls/ntdll/unix/security.c index 03cf26c1555..956c0ec4723 100644 --- a/dlls/ntdll/unix/security.c +++ b/dlls/ntdll/unix/security.c @@ -516,7 +516,7 @@ NTSTATUS WINAPI NtQueryInformationToken( HANDLE token, TOKEN_INFORMATION_CLASS c req->handle = wine_server_obj_handle( token ); status = wine_server_call( req ); - if (!status) *type = reply->elevation; + if (!status) *type = reply->elevation_type; } SERVER_END_REQ; break; @@ -528,7 +528,7 @@ NTSTATUS WINAPI NtQueryInformationToken( HANDLE token, TOKEN_INFORMATION_CLASS c req->handle = wine_server_obj_handle( token ); status = wine_server_call( req ); - if (!status) elevation->TokenIsElevated = (reply->elevation == TokenElevationTypeFull); + if (!status) elevation->TokenIsElevated = reply->is_elevated; } SERVER_END_REQ; break; diff --git a/server/protocol.def b/server/protocol.def index a7a89724d31..a4de7cf5ed9 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3743,7 +3743,8 @@ typedef union unsigned int session_id; /* token session id */ int primary; /* is the token primary or impersonation? */ int impersonation_level; /* level of impersonation */ - int elevation; /* elevation type */ + int elevation_type; /* elevation type (TokenElevation*) */ + int is_elevated; /* is the token elevated as per GetTokenInformation(TokenElevation)? */ int group_count; /* the number of groups the token is a member of */ int privilege_count; /* the number of privileges the token has */ @END diff --git a/server/token.c b/server/token.c index 698b2627ccb..ab3f4b1b5f7 100644 --- a/server/token.c +++ b/server/token.c @@ -1554,7 +1554,14 @@ DECL_HANDLER(get_token_info) reply->session_id = token->session_id; reply->primary = token->primary; reply->impersonation_level = token->impersonation_level; - reply->elevation = token->elevation; + reply->elevation_type = token->elevation; + /* Tokens with TokenElevationTypeDefault are considered elevated for the + purposes of GetTokenInformation(TokenElevation) if they belong to the + admins group. */ + if (token->elevation == TokenElevationTypeDefault) + reply->is_elevated = token_sid_present( token, &builtin_admins_sid, FALSE ); + else + reply->is_elevated = token->elevation == TokenElevationTypeFull; reply->group_count = list_count( &token->groups ); reply->privilege_count = list_count( &token->privileges ); release_object( token ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6602
From: Tim Clem <tclem(a)codeweavers.com> --- programs/services/services.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/programs/services/services.c b/programs/services/services.c index 9636752438e..097e052b74c 100644 --- a/programs/services/services.c +++ b/programs/services/services.c @@ -1343,6 +1343,13 @@ int __cdecl main(int argc, char *argv[]) JOBOBJECT_ASSOCIATE_COMPLETION_PORT port_info; HANDLE started_event, process_monitor_thread; DWORD err; + NTSTATUS status; + + /* FIXME: Until we start honoring each service's lpServiceStartName, we want + services to inherit a default admin token. */ + status = NtSetInformationProcess( GetCurrentProcess(), ProcessWineSetAdminToken, NULL, 0 ); + if (status) + WARN( "couldn't set admin token, error %08lx\n", status ); job_object = CreateJobObjectW(NULL, NULL); job_limit.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_BREAKAWAY_OK | JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK; -- 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=149039 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: comm.c:1574: Test failed: AbortWaitCts hComPortEvent failed comm.c:1586: Test failed: Unexpected time 1001, expected around 500
v2 switches the approach to an extension to NtSetInformationProcess. I also added related patches around GetTokenInformation(TokenElevation) and have services.exe elevate itself. Let me know if you'd prefer things split up a bit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6602#note_85183
participants (3)
-
Marvin -
Tim Clem -
Tim Clem (＠tclem)