[PATCH v2 0/5] MR481: server: Use the token owner instead of the token user for default object owner.
This fixes Amazon Games not launching due to pipe client identity check in https://github.com/dotnet/runtime/blob/371905898f2c036cc30d662e8a6df162a2196.... -- v2: server: Use the token owner instead of the token user for default object owner. advapi32/tests: Test that the token default DACL uses token owner instead of token user. advapi32/tests: Test that default object group match the token primary group. advapi32/tests: Test that default object owner match the token owner. advapi32/tests: Check that each ACE is ACCESS_ALLOWED_ACE in the default DACL. https://gitlab.winehq.org/wine/wine/-/merge_requests/481
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> To ensure type safety, without complicating the test. --- dlls/advapi32/tests/security.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index aeee279a86d..7da9f3b3157 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -6353,6 +6353,8 @@ static void test_default_dacl_owner_sid(void) found = FALSE; while (GetAce( dacl, index++, (void **)&ace )) { + ok( ace->Header.AceType == ACCESS_ALLOWED_ACE_TYPE, + "expected ACCESS_ALLOWED_ACE_TYPE, got %d\n", ace->Header.AceType ); if (EqualSid( &ace->SidStart, owner )) found = TRUE; } ok( found, "owner sid not found in dacl\n" ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/481
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/advapi32/tests/security.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 7da9f3b3157..eab1bd2be5f 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -1765,6 +1765,24 @@ static void test_AccessCheck(void) HeapFree(GetProcessHeap(), 0, PrivSet); } +TOKEN_OWNER *get_alloc_token_owner( HANDLE token ) +{ + TOKEN_OWNER *token_owner; + DWORD size; + BOOL ret; + + ret = GetTokenInformation( token, TokenOwner, NULL, 0, &size ); + ok(!ret, "Expected failure, got %d\n", ret); + ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, + "Expected ERROR_INSUFFICIENT_BUFFER, got %ld\n", GetLastError()); + + token_owner = HeapAlloc( GetProcessHeap(), 0, size ); + ret = GetTokenInformation( token, TokenOwner, token_owner, size, &size ); + ok(ret, "GetTokenInformation failed with error %ld\n", GetLastError()); + + return token_owner; +} + /* test GetTokenInformation for the various attributes */ static void test_token_attr(void) { @@ -6306,7 +6324,8 @@ static void test_TokenIntegrityLevel(void) static void test_default_dacl_owner_sid(void) { - HANDLE handle; + TOKEN_OWNER *token_owner; + HANDLE handle, token; BOOL ret, defaulted, present, found; DWORD size, index; SECURITY_DESCRIPTOR *sd; @@ -6315,6 +6334,13 @@ static void test_default_dacl_owner_sid(void) ACL *dacl; ACCESS_ALLOWED_ACE *ace; + ret = OpenProcessToken( GetCurrentProcess(), TOKEN_QUERY, &token ); + ok(ret, "OpenProcessToken failed with error %ld\n", GetLastError()); + + token_owner = get_alloc_token_owner( token ); + + CloseHandle( token ); + sd = HeapAlloc( GetProcessHeap(), 0, SECURITY_DESCRIPTOR_MIN_LENGTH ); ret = InitializeSecurityDescriptor( sd, SECURITY_DESCRIPTOR_REVISION ); ok( ret, "error %lu\n", GetLastError() ); @@ -6339,6 +6365,8 @@ static void test_default_dacl_owner_sid(void) ok( ret, "error %lu\n", GetLastError() ); ok( owner != (void *)0xdeadbeef, "owner not set\n" ); ok( !defaulted, "owner defaulted\n" ); + todo_wine + ok( EqualSid( owner, token_owner->Owner ), "owner shall equal token owner\n" ); dacl = (void *)0xdeadbeef; present = FALSE; @@ -6362,6 +6390,8 @@ static void test_default_dacl_owner_sid(void) HeapFree( GetProcessHeap(), 0, sa.lpSecurityDescriptor ); HeapFree( GetProcessHeap(), 0, sd ); CloseHandle( handle ); + + HeapFree( GetProcessHeap(), 0, token_owner ); } static void test_AdjustTokenPrivileges(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/481
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/advapi32/tests/security.c | 39 +++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index eab1bd2be5f..12518722969 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -1783,6 +1783,24 @@ TOKEN_OWNER *get_alloc_token_owner( HANDLE token ) return token_owner; } +TOKEN_PRIMARY_GROUP *get_alloc_token_primary_group( HANDLE token ) +{ + TOKEN_PRIMARY_GROUP *token_primary_group; + DWORD size; + BOOL ret; + + ret = GetTokenInformation( token, TokenPrimaryGroup, NULL, 0, &size ); + ok(!ret, "Expected failure, got %d\n", ret); + ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, + "Expected ERROR_INSUFFICIENT_BUFFER, got %ld\n", GetLastError()); + + token_primary_group = HeapAlloc( GetProcessHeap(), 0, size ); + ret = GetTokenInformation( token, TokenPrimaryGroup, token_primary_group, size, &size ); + ok(ret, "GetTokenInformation failed with error %ld\n", GetLastError()); + + return token_primary_group; +} + /* test GetTokenInformation for the various attributes */ static void test_token_attr(void) { @@ -6322,15 +6340,16 @@ static void test_TokenIntegrityLevel(void) CloseHandle(token); } -static void test_default_dacl_owner_sid(void) +static void test_default_dacl_owner_group_sid(void) { TOKEN_OWNER *token_owner; + TOKEN_PRIMARY_GROUP *token_primary_group; HANDLE handle, token; BOOL ret, defaulted, present, found; DWORD size, index; SECURITY_DESCRIPTOR *sd; SECURITY_ATTRIBUTES sa; - PSID owner; + PSID owner, group; ACL *dacl; ACCESS_ALLOWED_ACE *ace; @@ -6338,6 +6357,7 @@ static void test_default_dacl_owner_sid(void) ok(ret, "OpenProcessToken failed with error %ld\n", GetLastError()); token_owner = get_alloc_token_owner( token ); + token_primary_group = get_alloc_token_primary_group( token ); CloseHandle( token ); @@ -6352,11 +6372,11 @@ static void test_default_dacl_owner_sid(void) ok( handle != NULL, "error %lu\n", GetLastError() ); size = 0; - ret = GetKernelObjectSecurity( handle, OWNER_SECURITY_INFORMATION|DACL_SECURITY_INFORMATION, NULL, 0, &size ); + ret = GetKernelObjectSecurity( handle, OWNER_SECURITY_INFORMATION|GROUP_SECURITY_INFORMATION|DACL_SECURITY_INFORMATION, NULL, 0, &size ); ok( !ret && GetLastError() == ERROR_INSUFFICIENT_BUFFER, "error %lu\n", GetLastError() ); sd = HeapAlloc( GetProcessHeap(), 0, size ); - ret = GetKernelObjectSecurity( handle, OWNER_SECURITY_INFORMATION|DACL_SECURITY_INFORMATION, sd, size, &size ); + ret = GetKernelObjectSecurity( handle, OWNER_SECURITY_INFORMATION|GROUP_SECURITY_INFORMATION|DACL_SECURITY_INFORMATION, sd, size, &size ); ok( ret, "error %lu\n", GetLastError() ); owner = (void *)0xdeadbeef; @@ -6368,6 +6388,14 @@ static void test_default_dacl_owner_sid(void) todo_wine ok( EqualSid( owner, token_owner->Owner ), "owner shall equal token owner\n" ); + group = (void *)0xdeadbeef; + defaulted = TRUE; + ret = GetSecurityDescriptorGroup( sd, &group, &defaulted ); + ok( ret, "error %lu\n", GetLastError() ); + ok( group != (void *)0xdeadbeef, "group not set\n" ); + ok( !defaulted, "group defaulted\n" ); + ok( EqualSid( group, token_primary_group->PrimaryGroup ), "group shall equal token primary group\n" ); + dacl = (void *)0xdeadbeef; present = FALSE; defaulted = TRUE; @@ -6391,6 +6419,7 @@ static void test_default_dacl_owner_sid(void) HeapFree( GetProcessHeap(), 0, sd ); CloseHandle( handle ); + HeapFree( GetProcessHeap(), 0, token_primary_group ); HeapFree( GetProcessHeap(), 0, token_owner ); } @@ -8534,7 +8563,7 @@ START_TEST(security) test_GetUserNameW(); test_CreateRestrictedToken(); test_TokenIntegrityLevel(); - test_default_dacl_owner_sid(); + test_default_dacl_owner_group_sid(); test_AdjustTokenPrivileges(); test_AddAce(); test_AddMandatoryAce(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/481
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/advapi32/tests/security.c | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 12518722969..ac853eb57f2 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -1765,6 +1765,24 @@ static void test_AccessCheck(void) HeapFree(GetProcessHeap(), 0, PrivSet); } +TOKEN_USER *get_alloc_token_user( HANDLE token ) +{ + TOKEN_USER *token_user; + DWORD size; + BOOL ret; + + ret = GetTokenInformation( token, TokenUser, NULL, 0, &size ); + ok(!ret, "Expected failure, got %d\n", ret); + ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, + "Expected ERROR_INSUFFICIENT_BUFFER, got %ld\n", GetLastError()); + + token_user = HeapAlloc( GetProcessHeap(), 0, size ); + ret = GetTokenInformation( token, TokenUser, token_user, size, &size ); + ok(ret, "GetTokenInformation failed with error %ld\n", GetLastError()); + + return token_user; +} + TOKEN_OWNER *get_alloc_token_owner( HANDLE token ) { TOKEN_OWNER *token_owner; @@ -6342,6 +6360,7 @@ static void test_TokenIntegrityLevel(void) static void test_default_dacl_owner_group_sid(void) { + TOKEN_USER *token_user; TOKEN_OWNER *token_owner; TOKEN_PRIMARY_GROUP *token_primary_group; HANDLE handle, token; @@ -6356,6 +6375,7 @@ static void test_default_dacl_owner_group_sid(void) ret = OpenProcessToken( GetCurrentProcess(), TOKEN_QUERY, &token ); ok(ret, "OpenProcessToken failed with error %ld\n", GetLastError()); + token_user = get_alloc_token_user( token ); token_owner = get_alloc_token_owner( token ); token_primary_group = get_alloc_token_primary_group( token ); @@ -6415,12 +6435,27 @@ static void test_default_dacl_owner_group_sid(void) } ok( found, "owner sid not found in dacl\n" ); + if (!EqualSid( token_user->User.Sid, token_owner->Owner )) + { + index = 0; + found = FALSE; + while (GetAce( dacl, index++, (void **)&ace )) + { + ok( ace->Header.AceType == ACCESS_ALLOWED_ACE_TYPE, + "expected ACCESS_ALLOWED_ACE_TYPE, got %d\n", ace->Header.AceType ); + if (EqualSid( &ace->SidStart, token_user->User.Sid )) found = TRUE; + } + todo_wine + ok( !found, "DACL shall not reference token user if it is different from token owner\n" ); + } + HeapFree( GetProcessHeap(), 0, sa.lpSecurityDescriptor ); HeapFree( GetProcessHeap(), 0, sd ); CloseHandle( handle ); HeapFree( GetProcessHeap(), 0, token_primary_group ); HeapFree( GetProcessHeap(), 0, token_owner ); + HeapFree( GetProcessHeap(), 0, token_user ); } static void test_AdjustTokenPrivileges(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/481
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Also, replace the token user with the token owner for the default DACL as well. Wine currently selects domain_users_sid as the token owner, so use that. This is required to pass the advapi32:security test which expects the security descriptor owner SID to be referenced in the DACL as well. --- dlls/advapi32/tests/security.c | 2 -- server/change.c | 2 +- server/file.c | 4 ++-- server/object.c | 2 +- server/security.h | 2 +- server/token.c | 6 +++--- 6 files changed, 8 insertions(+), 10 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index ac853eb57f2..a40a8beadb4 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -6405,7 +6405,6 @@ static void test_default_dacl_owner_group_sid(void) ok( ret, "error %lu\n", GetLastError() ); ok( owner != (void *)0xdeadbeef, "owner not set\n" ); ok( !defaulted, "owner defaulted\n" ); - todo_wine ok( EqualSid( owner, token_owner->Owner ), "owner shall equal token owner\n" ); group = (void *)0xdeadbeef; @@ -6445,7 +6444,6 @@ static void test_default_dacl_owner_group_sid(void) "expected ACCESS_ALLOWED_ACE_TYPE, got %d\n", ace->Header.AceType ); if (EqualSid( &ace->SidStart, token_user->User.Sid )) found = TRUE; } - todo_wine ok( !found, "DACL shall not reference token user if it is different from token owner\n" ); } diff --git a/server/change.c b/server/change.c index 6477b457f74..7a806abc017 100644 --- a/server/change.c +++ b/server/change.c @@ -391,7 +391,7 @@ static int dir_set_sd( struct object *obj, const struct security_descriptor *sd, else if (obj->sd) owner = sd_get_owner( obj->sd ); else - owner = token_get_user( current->process->token ); + owner = token_get_owner( current->process->token ); if (set_info & DACL_SECURITY_INFORMATION) { diff --git a/server/file.c b/server/file.c index eb2dc5696ed..76c687833c9 100644 --- a/server/file.c +++ b/server/file.c @@ -245,7 +245,7 @@ static struct object *create_file( struct fd *root, const char *nameptr, data_si { const struct sid *owner = sd_get_owner( sd ); if (!owner) - owner = token_get_user( current->process->token ); + owner = token_get_owner( current->process->token ); mode = sd_to_mode( sd, owner ); } else if (options & FILE_DIRECTORY_FILE) @@ -528,7 +528,7 @@ static int file_set_sd( struct object *obj, const struct security_descriptor *sd else if (obj->sd) owner = sd_get_owner( obj->sd ); else - owner = token_get_user( current->process->token ); + owner = token_get_owner( current->process->token ); /* group and sacl not supported */ diff --git a/server/object.c b/server/object.c index 333f9e7b5d6..89e541ffb6b 100644 --- a/server/object.c +++ b/server/object.c @@ -574,7 +574,7 @@ int set_sd_defaults_from_token( struct object *obj, const struct security_descri } else if (token) { - owner = token_get_user( token ); + owner = token_get_owner( token ); new_sd.owner_len = sid_len( owner ); } else new_sd.owner_len = 0; diff --git a/server/security.h b/server/security.h index fa91b81b77c..58ab1594eae 100644 --- a/server/security.h +++ b/server/security.h @@ -73,7 +73,7 @@ extern int token_check_privileges( struct token *token, int all_required, const struct luid_attr *reqprivs, unsigned int count, struct luid_attr *usedprivs ); extern const struct acl *token_get_default_dacl( struct token *token ); -extern const struct sid *token_get_user( struct token *token ); +extern const struct sid *token_get_owner( struct token *token ); extern const struct sid *token_get_primary_group( struct token *token ); extern unsigned int token_get_session_id( struct token *token ); extern int token_sid_present( struct token *token, const struct sid *sid, int deny ); diff --git a/server/token.c b/server/token.c index f817c1114f8..99f5e36e279 100644 --- a/server/token.c +++ b/server/token.c @@ -732,7 +732,7 @@ struct token *token_create_admin( unsigned primary, int impersonation_level, int /* on Windows, this value changes every time the user logs on */ struct sid logon_sid = { SID_REVISION, 3, SECURITY_NT_AUTHORITY, { SECURITY_LOGON_IDS_RID, 0, 0 /* FIXME: should be randomly generated when tokens are inherited by new processes */ }}; const struct sid *user_sid = security_unix_uid_to_sid( getuid() ); - struct acl *default_dacl = create_default_dacl( user_sid ); + struct acl *default_dacl = create_default_dacl( &domain_users_sid ); const struct luid_attr admin_privs[] = { { SeChangeNotifyPrivilege, SE_PRIVILEGE_ENABLED }, @@ -1044,9 +1044,9 @@ const struct acl *token_get_default_dacl( struct token *token ) return token->default_dacl; } -const struct sid *token_get_user( struct token *token ) +const struct sid *token_get_owner( struct token *token ) { - return token->user; + return token->owner; } const struct sid *token_get_primary_group( struct token *token ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/481
v2: add more tests to justify changing argument of `create_default_dacl()`; split commits -- https://gitlab.winehq.org/wine/wine/-/merge_requests/481#note_4641
participants (2)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)