[PATCH 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 --- 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 d18a8943ed2..31b56fa1bc6 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -8911,6 +8911,74 @@ static void test_shell_token(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(); @@ -8983,6 +9051,7 @@ START_TEST(security) test_elevation(); test_mandatory_integrity(); test_shell_token(); + 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 31b56fa1bc6..701db2c2e8c 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -8970,10 +8970,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;
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=89085
Your paranoid android.
=== build (build log) ===
error: patch failed: dlls/advapi32/tests/security.c:8983 error: patch failed: dlls/advapi32/tests/security.c:8970 Task: Patch failed to apply
=== debiant2 (build log) ===
error: patch failed: dlls/advapi32/tests/security.c:8983 error: patch failed: dlls/advapi32/tests/security.c:8970 Task: Patch failed to apply
=== debiant2 (build log) ===
error: patch failed: dlls/advapi32/tests/security.c:8983 error: patch failed: dlls/advapi32/tests/security.c:8970 Task: Patch failed to apply
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=89084
Your paranoid android.
=== build (build log) ===
error: patch failed: dlls/advapi32/tests/security.c:8983 Task: Patch failed to apply
=== debiant2 (build log) ===
error: patch failed: dlls/advapi32/tests/security.c:8983 Task: Patch failed to apply
=== debiant2 (build log) ===
error: patch failed: dlls/advapi32/tests/security.c:8983 Task: Patch failed to apply
-
Marvin -
Zebediah Figura