[PATCH v2 1/2] advapi32/tests: Add a test for setting the SD owner of a directory to a group.
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- v2: rebase onto master...
dlls/advapi32/tests/security.c | 69 ++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index ce9c26d238e..08b73495aaa 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -8306,6 +8306,74 @@ static void test_elevation(void) CloseHandle(token); }
+static void test_group_as_file_owner(void) +{ + char sd_buffer[200], sid_buffer[100]; + SECURITY_DESCRIPTOR *sd = (SECURITY_DESCRIPTOR *)sd_buffer; + char temp_path[MAX_PATH], path[MAX_PATH]; + SID *admin_sid = (SID *)sid_buffer; + BOOL ret, present, defaulted; + SECURITY_DESCRIPTOR new_sd; + HANDLE file; + DWORD size; + ACL *dacl; + + /* The EA Origin client sets the SD owner of a directory to Administrators, + * while using the default DACL, and subsequently tries to create + * subdirectories. */ + + size = sizeof(sid_buffer); + CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, admin_sid, &size); + + ret = CheckTokenMembership(NULL, admin_sid, &present); + ok(ret, "got error %u\n", GetLastError()); + if (!present) + { + skip("user is not an administrator\n"); + return; + } + + GetTempPathA(ARRAY_SIZE(temp_path), temp_path); + sprintf(path, "%s\testdir", temp_path); + + ret = CreateDirectoryA(path, NULL); + ok(ret, "got error %u\n", GetLastError()); + + file = CreateFileA(path, FILE_ALL_ACCESS, 0, NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0); + ok(file != INVALID_HANDLE_VALUE, "got error %u\n", GetLastError()); + + ret = GetKernelObjectSecurity(file, DACL_SECURITY_INFORMATION, sd_buffer, sizeof(sd_buffer), &size); + ok(ret, "got error %u\n", GetLastError()); + ret = GetSecurityDescriptorDacl(sd, &present, &dacl, &defaulted); + ok(ret, "got error %u\n", GetLastError()); + + InitializeSecurityDescriptor(&new_sd, SECURITY_DESCRIPTOR_REVISION); + + ret = SetSecurityDescriptorOwner(&new_sd, admin_sid, FALSE); + ok(ret, "got error %u\n", GetLastError()); + + ret = GetSecurityDescriptorDacl(sd, &present, &dacl, &defaulted); + ok(ret, "got error %u\n", GetLastError()); + + ret = SetSecurityDescriptorDacl(&new_sd, present, dacl, defaulted); + ok(ret, "got error %u\n", GetLastError()); + + ret = SetKernelObjectSecurity(file, OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, &new_sd); + ok(ret, "got error %u\n", GetLastError()); + + CloseHandle(file); + + sprintf(path, "%s\testdir\subdir", temp_path); + ret = CreateDirectoryA(path, NULL); + todo_wine ok(ret, "got error %u\n", GetLastError()); + + ret = RemoveDirectoryA(path); + todo_wine ok(ret, "got error %u\n", GetLastError()); + sprintf(path, "%s\testdir", temp_path); + ret = RemoveDirectoryA(path); + ok(ret, "got error %u\n", GetLastError()); +} + START_TEST(security) { init(); @@ -8374,6 +8442,7 @@ START_TEST(security) test_duplicate_token(); test_GetKernelObjectSecurity(); test_elevation(); + test_group_as_file_owner();
/* Must be the last test, modifies process token */ test_token_security_descriptor();
Instead of requiring the SD owner to match the token user.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=44691 Signed-off-by: Zebediah Figura z.figura12@gmail.com --- Granted, it's not immediately clear to me that this is the best way to handle this case, but nothing else I considered seemed obviously any more faithful.
As the previous patch describes, the security descriptor that the Origin installer sets has the owner set to the Administrators SID, and the default DACL list of {allow FILE_ALL_ACCESS to LOCAL SYSTEM, allow FILE_ALL_ACCESS to the current user, allow FILE_READ_ACCESS to world}.
Admittedly it doesn't seem to make a lot of sense to me to handle user and group permissions differently. The concept of "apply this permission only if the SID is the token user" just isn't present in the Windows DACL; the "token user" only exists to set the default user and DACL for new objects. I'd be inclined to argue that we should do is map a permission to both user and group if it applies at all to the current token—i.e. what this patch does—and get rid of the "user only" case.
dlls/advapi32/tests/security.c | 4 ++-- server/file.c | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 08b73495aaa..299a340dcf3 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -8365,10 +8365,10 @@ static void test_group_as_file_owner(void)
sprintf(path, "%s\testdir\subdir", temp_path); ret = CreateDirectoryA(path, NULL); - todo_wine ok(ret, "got error %u\n", GetLastError()); + ok(ret, "got error %u\n", GetLastError());
ret = RemoveDirectoryA(path); - todo_wine ok(ret, "got error %u\n", GetLastError()); + ok(ret, "got error %u\n", GetLastError()); sprintf(path, "%s\testdir", temp_path); ret = RemoveDirectoryA(path); ok(ret, "got error %u\n", GetLastError()); diff --git a/server/file.c b/server/file.c index 9a072e6c64e..aff4d9e09e1 100644 --- a/server/file.c +++ b/server/file.c @@ -473,7 +473,6 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) mode_t mode; int present; const ACL *dacl = sd_get_dacl( sd, &present ); - const SID *user = token_get_user( current->process->token ); if (present && dacl) { const ACE_HEADER *ace = (const ACE_HEADER *)(dacl + 1); @@ -496,8 +495,8 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) { bits_to_set &= ~((mode << 6) | (mode << 3) | mode); /* all */ } - else if ((security_equal_sid( user, owner ) && - token_sid_present( current->process->token, sid, TRUE ))) + else if (token_sid_present( current->process->token, owner, TRUE ) && + token_sid_present( current->process->token, sid, TRUE )) { bits_to_set &= ~((mode << 6) | (mode << 3)); /* user + group */ } @@ -516,8 +515,8 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) new_mode |= mode & bits_to_set; bits_to_set &= ~mode; } - else if ((security_equal_sid( user, owner ) && - token_sid_present( current->process->token, sid, FALSE ))) + else if (token_sid_present( current->process->token, owner, FALSE ) && + token_sid_present( current->process->token, sid, FALSE )) { mode = (mode << 6) | (mode << 3); /* user + group */ new_mode |= mode & bits_to_set;
-
Zebediah Figura