[2/2] server: Don't add Local System to file dacl's.
Vincent Povirk
madewokherd at gmail.com
Tue Apr 15 13:49:31 CDT 2014
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.
-------------- next part --------------
From 9668dbbd856c75ae700a32c4dd74fb4f50c87dba Mon Sep 17 00:00:00 2001
From: Vincent Povirk <vincent at codeweavers.com>
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
More information about the wine-patches
mailing list