From: Vincent Povirk Subject: [2/2] server: Don't add Local System to file dacl's. Message-Id: Date: Tue, 15 Apr 2014 13:49:31 -0500 I'm not sure about this one, but I figured the easiest way to ask about it was to send the patch and see what happens. We have a bunch of security tests that write a dacl with entries for the current user and Administrators group. They expect to read the entries back, in that order, but actually they get Local System first. Now they get the entries in the order expected. Arguably, though, this isn't generally better, just better for what the tests happen to do. If the tests wrote Local System in their acl, then the current behavior would be better. It's nice to have more tests passing, so in theory we're better protected against regressions, but we could also accomplish that by changing the tests or reporting Local System last. What it comes down to is that I don't know the purpose of the Local System entry. I think it's better to remove code we don't need, but I also risk introducing a regression where the only way to fix it is to revert the patch or find a real way to preserve the acl. From 9668dbbd856c75ae700a32c4dd74fb4f50c87dba Mon Sep 17 00:00:00 2001 From: Vincent Povirk Date: Tue, 15 Apr 2014 13:05:55 -0500 Subject: [PATCH 2/2] server: Don't add Local System to file dacl's. --- dlls/advapi32/tests/security.c | 20 ++++++++++---------- server/file.c | 32 +++++++++++--------------------- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 9960109..6e280f0 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -3098,14 +3098,14 @@ static void test_CreateDirectoryA(void) bret = pGetAclInformation(pDacl, &acl_size, sizeof(acl_size), AclSizeInformation); ok(bret, "GetAclInformation failed\n"); - todo_wine ok(acl_size.AceCount == 2, "GetAclInformation returned unexpected entry count (%d != 2).\n", + ok(acl_size.AceCount == 2, "GetAclInformation returned unexpected entry count (%d != 2).\n", acl_size.AceCount); if (acl_size.AceCount > 0) { bret = pGetAce(pDacl, 0, (VOID **)&ace); ok(bret, "Failed to get Current User ACE.\n"); bret = EqualSid(&ace->SidStart, user_sid); - todo_wine ok(bret, "Current User ACE != Current User SID.\n"); + ok(bret, "Current User ACE != Current User SID.\n"); todo_wine ok(((ACE_HEADER *)ace)->AceFlags == (OBJECT_INHERIT_ACE|CONTAINER_INHERIT_ACE), "Current User ACE has unexpected flags (0x%x != 0x03)\n", ((ACE_HEADER *)ace)->AceFlags); @@ -3117,11 +3117,11 @@ static void test_CreateDirectoryA(void) bret = pGetAce(pDacl, 1, (VOID **)&ace); ok(bret, "Failed to get Administators Group ACE.\n"); bret = EqualSid(&ace->SidStart, admin_sid); - todo_wine ok(bret, "Administators Group ACE != Administators Group SID.\n"); + ok(bret, "Administators Group ACE != Administators Group SID.\n"); todo_wine ok(((ACE_HEADER *)ace)->AceFlags == (OBJECT_INHERIT_ACE|CONTAINER_INHERIT_ACE), "Administators Group ACE has unexpected flags (0x%x != 0x03)\n", ((ACE_HEADER *)ace)->AceFlags); - ok(ace->Mask == 0x1f01ff, "Administators Group ACE has unexpected mask (0x%x != 0x1f01ff)\n", + todo_wine ok(ace->Mask == 0x1f01ff, "Administators Group ACE has unexpected mask (0x%x != 0x1f01ff)\n", ace->Mask); } @@ -3294,7 +3294,7 @@ static void test_GetNamedSecurityInfoA(void) bret = pGetAce(pDacl, 0, (VOID **)&ace); ok(bret, "Failed to get Current User ACE.\n"); bret = EqualSid(&ace->SidStart, user_sid); - todo_wine ok(bret, "Current User ACE != Current User SID.\n"); + ok(bret, "Current User ACE != Current User SID.\n"); ok(((ACE_HEADER *)ace)->AceFlags == 0, "Current User ACE has unexpected flags (0x%x != 0x0)\n", ((ACE_HEADER *)ace)->AceFlags); ok(ace->Mask == 0x1f01ff, "Current User ACE has unexpected mask (0x%x != 0x1f01ff)\n", @@ -3305,11 +3305,11 @@ static void test_GetNamedSecurityInfoA(void) bret = pGetAce(pDacl, 1, (VOID **)&ace); ok(bret, "Failed to get Administators Group ACE.\n"); bret = EqualSid(&ace->SidStart, admin_sid); - todo_wine ok(bret || broken(!bret) /* win2k */, + ok(bret || broken(!bret) /* win2k */, "Administators Group ACE != Administators Group SID.\n"); ok(((ACE_HEADER *)ace)->AceFlags == 0, "Administators Group ACE has unexpected flags (0x%x != 0x0)\n", ((ACE_HEADER *)ace)->AceFlags); - ok(ace->Mask == 0x1f01ff || broken(ace->Mask == GENERIC_ALL) /* win2k */, + todo_wine ok(ace->Mask == 0x1f01ff || broken(ace->Mask == GENERIC_ALL) /* win2k */, "Administators Group ACE has unexpected mask (0x%x != 0x1f01ff)\n", ace->Mask); } LocalFree(pSD); @@ -3960,7 +3960,7 @@ static void test_GetSecurityInfo(void) bret = pGetAce(pDacl, 0, (VOID **)&ace); ok(bret, "Failed to get Current User ACE.\n"); bret = EqualSid(&ace->SidStart, user_sid); - todo_wine ok(bret, "Current User ACE != Current User SID.\n"); + ok(bret, "Current User ACE != Current User SID.\n"); ok(((ACE_HEADER *)ace)->AceFlags == 0, "Current User ACE has unexpected flags (0x%x != 0x0)\n", ((ACE_HEADER *)ace)->AceFlags); ok(ace->Mask == 0x1f01ff, "Current User ACE has unexpected mask (0x%x != 0x1f01ff)\n", @@ -3971,10 +3971,10 @@ static void test_GetSecurityInfo(void) bret = pGetAce(pDacl, 1, (VOID **)&ace); ok(bret, "Failed to get Administators Group ACE.\n"); bret = EqualSid(&ace->SidStart, admin_sid); - todo_wine ok(bret, "Administators Group ACE != Administators Group SID.\n"); + ok(bret, "Administators Group ACE != Administators Group SID.\n"); ok(((ACE_HEADER *)ace)->AceFlags == 0, "Administators Group ACE has unexpected flags (0x%x != 0x0)\n", ((ACE_HEADER *)ace)->AceFlags); - ok(ace->Mask == 0x1f01ff, "Administators Group ACE has unexpected mask (0x%x != 0x1f01ff)\n", + todo_wine ok(ace->Mask == 0x1f01ff, "Administators Group ACE has unexpected mask (0x%x != 0x1f01ff)\n", ace->Mask); } CloseHandle(obj); diff --git a/server/file.c b/server/file.c index 358adf0..bd0d20d 100644 --- a/server/file.c +++ b/server/file.c @@ -318,10 +318,8 @@ struct security_descriptor *mode_to_sd( mode_t mode, const SID *user, const SID SID *sid; char *ptr; const SID *world_sid = security_world_sid; - const SID *local_system_sid = security_local_system_sid; - dacl_size = sizeof(ACL) + FIELD_OFFSET(ACCESS_ALLOWED_ACE, SidStart) + - security_sid_len( local_system_sid ); + dacl_size = sizeof(ACL); if (mode & S_IRWXU) dacl_size += FIELD_OFFSET(ACCESS_ALLOWED_ACE, SidStart) + security_sid_len( user ); if (mode & S_IRWXG) @@ -354,28 +352,19 @@ struct security_descriptor *mode_to_sd( mode_t mode, const SID *user, const SID dacl->AclRevision = ACL_REVISION; dacl->Sbz1 = 0; dacl->AclSize = dacl_size; - dacl->AceCount = 1 + (mode & S_IRWXU ? 1 : 0) + (mode & S_IRWXG ? 1 : 0) + (mode & S_IRWXO ? 1 : 0); + dacl->AceCount = (mode & S_IRWXU ? 1 : 0) + (mode & S_IRWXG ? 1 : 0) + (mode & S_IRWXO ? 1 : 0); if ((!(mode & S_IRUSR) && (mode & (S_IRGRP|S_IROTH))) || (!(mode & S_IWUSR) && (mode & (S_IWGRP|S_IWOTH))) || (!(mode & S_IXUSR) && (mode & (S_IXGRP|S_IXOTH)))) dacl->AceCount++; dacl->Sbz2 = 0; - /* always give FILE_ALL_ACCESS for Local System */ - aaa = (ACCESS_ALLOWED_ACE *)(dacl + 1); - current_ace = &aaa->Header; - aaa->Header.AceType = ACCESS_ALLOWED_ACE_TYPE; - aaa->Header.AceFlags = 0; - aaa->Header.AceSize = FIELD_OFFSET(ACCESS_ALLOWED_ACE, SidStart) + security_sid_len( local_system_sid ); - aaa->Mask = FILE_ALL_ACCESS; - sid = (SID *)&aaa->SidStart; - memcpy( sid, local_system_sid, security_sid_len( local_system_sid )); + current_ace = (ACE_HEADER*)(dacl + 1); if (mode & S_IRWXU) { /* appropriate access rights for the user */ - aaa = (ACCESS_ALLOWED_ACE *)ace_next( current_ace ); - current_ace = &aaa->Header; + aaa = (ACCESS_ALLOWED_ACE *)current_ace; aaa->Header.AceType = ACCESS_ALLOWED_ACE_TYPE; aaa->Header.AceFlags = 0; aaa->Header.AceSize = FIELD_OFFSET(ACCESS_ALLOWED_ACE, SidStart) + security_sid_len( user ); @@ -386,12 +375,12 @@ struct security_descriptor *mode_to_sd( mode_t mode, const SID *user, const SID aaa->Mask |= FILE_GENERIC_WRITE | DELETE | FILE_DELETE_CHILD; sid = (SID *)&aaa->SidStart; memcpy( sid, user, security_sid_len( user )); + current_ace = (ACE_HEADER*)ace_next( current_ace ); } if (mode & S_IRWXG) { /* appropriate access rights for the group */ - aaa = (ACCESS_ALLOWED_ACE *)ace_next( current_ace ); - current_ace = &aaa->Header; + aaa = (ACCESS_ALLOWED_ACE *)current_ace; aaa->Header.AceType = ACCESS_ALLOWED_ACE_TYPE; aaa->Header.AceFlags = 0; aaa->Header.AceSize = FIELD_OFFSET(ACCESS_ALLOWED_ACE, SidStart) + security_sid_len( group ); @@ -402,14 +391,14 @@ struct security_descriptor *mode_to_sd( mode_t mode, const SID *user, const SID aaa->Mask |= FILE_GENERIC_WRITE | DELETE | FILE_DELETE_CHILD; sid = (SID *)&aaa->SidStart; memcpy( sid, group, security_sid_len( group )); + current_ace = (ACE_HEADER*)ace_next( current_ace ); } if ((!(mode & S_IRUSR) && (mode & (S_IRGRP|S_IROTH))) || (!(mode & S_IWUSR) && (mode & (S_IWGRP|S_IWOTH))) || (!(mode & S_IXUSR) && (mode & (S_IXGRP|S_IXOTH)))) { /* deny just in case the user is a member of the group */ - ACCESS_DENIED_ACE *ada = (ACCESS_DENIED_ACE *)ace_next( current_ace ); - current_ace = &ada->Header; + ACCESS_DENIED_ACE *ada = (ACCESS_DENIED_ACE *)current_ace; ada->Header.AceType = ACCESS_DENIED_ACE_TYPE; ada->Header.AceFlags = 0; ada->Header.AceSize = FIELD_OFFSET(ACCESS_DENIED_ACE, SidStart) + security_sid_len( user ); @@ -421,12 +410,12 @@ struct security_descriptor *mode_to_sd( mode_t mode, const SID *user, const SID ada->Mask &= ~STANDARD_RIGHTS_ALL; /* never deny standard rights */ sid = (SID *)&ada->SidStart; memcpy( sid, user, security_sid_len( user )); + current_ace = (ACE_HEADER*)ace_next( current_ace ); } if (mode & S_IRWXO) { /* appropriate access rights for Everyone */ - aaa = (ACCESS_ALLOWED_ACE *)ace_next( current_ace ); - current_ace = &aaa->Header; + aaa = (ACCESS_ALLOWED_ACE *)current_ace; aaa->Header.AceType = ACCESS_ALLOWED_ACE_TYPE; aaa->Header.AceFlags = 0; aaa->Header.AceSize = FIELD_OFFSET(ACCESS_ALLOWED_ACE, SidStart) + security_sid_len( world_sid ); @@ -437,6 +426,7 @@ struct security_descriptor *mode_to_sd( mode_t mode, const SID *user, const SID aaa->Mask |= FILE_GENERIC_WRITE | DELETE | FILE_DELETE_CHILD; sid = (SID *)&aaa->SidStart; memcpy( sid, world_sid, security_sid_len( world_sid )); + current_ace = (ACE_HEADER*)ace_next( current_ace ); } return sd; -- 1.8.3.2