From 06371539bd405ce4ba288d44efaaeb645ed37299 Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Fri, 27 Feb 2015 12:59:09 +0000 Subject: [PATCH] * sec_acl.cc (setacl): Fix bug which leads to ACE duplication in case owner SID == group SID. (getacl): Reverse order of SID test against group or owner sid to prefer owner attributes over group attributes. Disable setting group permissions equivalent to owner permissions if owner == group. Add comment to explain why. Fix indentation. * security.cc (get_attribute_from_acl): Change type of local variables containing permission to mode_t. Apply deny mask to group if group SID == owner SID to avoid Everyone permissions to spill over into group permissions. Disable setting group permissions equivalent to owner permissions if owner == group. Add comment to explain why. * uinfo.cc (pwdgrp::fetch_account_from_windows): Allow user SID as group account if user is a "Microsoft Account". Explain why. Drop workaround enforcing primary group "Users" for "Microsoft Accounts". --- winsup/cygwin/ChangeLog | 17 ++++++++++++ winsup/cygwin/sec_acl.cc | 55 ++++++++++++++++++++++----------------- winsup/cygwin/security.cc | 23 +++++++++++++--- winsup/cygwin/uinfo.cc | 50 +++++++++++++++++------------------ 4 files changed, 93 insertions(+), 52 deletions(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 12dd7734d..156680638 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,20 @@ +2015-02-27 Corinna Vinschen + + * sec_acl.cc (setacl): Fix bug which leads to ACE duplication in + case owner SID == group SID. + (getacl): Reverse order of SID test against group or owner sid to + prefer owner attributes over group attributes. Disable setting group + permissions equivalent to owner permissions if owner == group. Add + comment to explain why. Fix indentation. + * security.cc (get_attribute_from_acl): Change type of local variables + containing permission to mode_t. Apply deny mask to group if group SID + == owner SID to avoid Everyone permissions to spill over into group + permissions. Disable setting group permissions equivalent to owner + permissions if owner == group. Add comment to explain why. + * uinfo.cc (pwdgrp::fetch_account_from_windows): Allow user SID as + group account if user is a "Microsoft Account". Explain why. Drop + workaround enforcing primary group "Users" for "Microsoft Accounts". + 2015-02-26 Corinna Vinschen * ldap.cc (cyg_ldap::wait): Call cygwait with cw_infinite timeout value diff --git a/winsup/cygwin/sec_acl.cc b/winsup/cygwin/sec_acl.cc index b2b0586f3..fb3f8c3c9 100644 --- a/winsup/cygwin/sec_acl.cc +++ b/winsup/cygwin/sec_acl.cc @@ -169,7 +169,7 @@ setacl (HANDLE handle, path_conv &pc, int nentries, aclent_t *aclbufp, *allow |= FILE_DELETE_CHILD; invalid[i] = true; } - bool isownergroup = (owner_sid == group_sid); + bool isownergroup = (owner == group); DWORD owner_deny = ~owner_allow & (group_allow | other_allow); owner_deny &= ~(STANDARD_RIGHTS_READ | FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES); @@ -179,27 +179,27 @@ setacl (HANDLE handle, path_conv &pc, int nentries, aclent_t *aclbufp, /* Set deny ACE for owner. */ if (owner_deny && !add_access_denied_ace (acl, ace_off++, owner_deny, - owner_sid, acl_len, NO_INHERITANCE)) + owner, acl_len, NO_INHERITANCE)) return -1; /* Set deny ACE for group here to respect the canonical order, if this does not impact owner */ if (group_deny && !(group_deny & owner_allow) && !isownergroup && !add_access_denied_ace (acl, ace_off++, group_deny, - group_sid, acl_len, NO_INHERITANCE)) + group, acl_len, NO_INHERITANCE)) return -1; /* Set allow ACE for owner. */ if (!add_access_allowed_ace (acl, ace_off++, owner_allow, - owner_sid, acl_len, NO_INHERITANCE)) + owner, acl_len, NO_INHERITANCE)) return -1; /* Set deny ACE for group, if still needed. */ if (group_deny & owner_allow && !isownergroup && !add_access_denied_ace (acl, ace_off++, group_deny, - group_sid, acl_len, NO_INHERITANCE)) + group, acl_len, NO_INHERITANCE)) return -1; /* Set allow ACE for group. */ if (!isownergroup && !add_access_allowed_ace (acl, ace_off++, group_allow, - group_sid, acl_len, NO_INHERITANCE)) + group, acl_len, NO_INHERITANCE)) return -1; /* Set allow ACE for everyone. */ if (!add_access_allowed_ace (acl, ace_off++, other_allow, @@ -451,16 +451,16 @@ getacl (HANDLE handle, path_conv &pc, int nentries, aclent_t *aclbufp) type = OTHER_OBJ; id = ILLEGAL_GID; } - else if (ace_sid == group_sid) - { - type = GROUP_OBJ; - id = gid; - } else if (ace_sid == owner_sid) { type = USER_OBJ; id = uid; } + else if (ace_sid == group_sid) + { + type = GROUP_OBJ; + id = gid; + } else if (ace_sid == well_known_creator_group_sid) { type = DEF_GROUP_OBJ; @@ -563,19 +563,26 @@ getacl (HANDLE handle, path_conv &pc, int nentries, aclent_t *aclbufp) } if ((pos = searchace (lacl, MAX_ACL_ENTRIES, 0)) < 0) pos = MAX_ACL_ENTRIES; - if (aclbufp) { - if (owner_sid == group_sid) - lacl[0].a_perm = lacl[1].a_perm; - if (pos > nentries) - { - set_errno (ENOSPC); - return -1; - } - memcpy (aclbufp, lacl, pos * sizeof (aclent_t)); - for (i = 0; i < pos; ++i) - aclbufp[i].a_perm &= ~(DENY_R | DENY_W | DENY_X); - aclsort32 (pos, 0, aclbufp); - } + if (aclbufp) + { +#if 0 + /* Disable owner/group permissions equivalence if owner SID == group SID. + It's technically not quite correct, but it helps in case a security + conscious application checks if a file has too open permissions. In + fact, since owner == group, there's no security issue here. */ + if (owner_sid == group_sid) + lacl[1].a_perm = lacl[0].a_perm; +#endif + if (pos > nentries) + { + set_errno (ENOSPC); + return -1; + } + memcpy (aclbufp, lacl, pos * sizeof (aclent_t)); + for (i = 0; i < pos; ++i) + aclbufp[i].a_perm &= ~(DENY_R | DENY_W | DENY_X); + aclsort32 (pos, 0, aclbufp); + } syscall_printf ("%R = getacl(%S)", pos, pc.get_nt_native_path ()); return pos; } diff --git a/winsup/cygwin/security.cc b/winsup/cygwin/security.cc index 9c94c7053..6dde7d3c8 100644 --- a/winsup/cygwin/security.cc +++ b/winsup/cygwin/security.cc @@ -239,9 +239,9 @@ get_attribute_from_acl (mode_t *attribute, PACL acl, PSID owner_sid, PSID group_sid, bool grp_member) { ACCESS_ALLOWED_ACE *ace; - int allow = 0; - int deny = 0; - int *flags, *anti; + mode_t allow = 0; + mode_t deny = 0; + mode_t *flags, *anti; for (DWORD i = 0; i < acl->AceCount; ++i) { @@ -301,6 +301,17 @@ get_attribute_from_acl (mode_t *attribute, PACL acl, PSID owner_sid, *flags |= ((!(*anti & S_IWUSR)) ? S_IWUSR : 0); if (ace->Mask & FILE_EXEC_BITS) *flags |= ((!(*anti & S_IXUSR)) ? S_IXUSR : 0); + /* Apply deny mask to group if group SID == owner SID. */ + if (group_sid && RtlEqualSid (owner_sid, group_sid) + && ace->Header.AceType == ACCESS_DENIED_ACE_TYPE) + { + if (ace->Mask & FILE_READ_BITS) + *flags |= ((!(*anti & S_IRUSR)) ? S_IRGRP : 0); + if (ace->Mask & FILE_WRITE_BITS) + *flags |= ((!(*anti & S_IWUSR)) ? S_IWGRP : 0); + if (ace->Mask & FILE_EXEC_BITS) + *flags |= ((!(*anti & S_IXUSR)) ? S_IXGRP : 0); + } } else if (ace_sid == group_sid) { @@ -331,6 +342,11 @@ get_attribute_from_acl (mode_t *attribute, PACL acl, PSID owner_sid, } } *attribute &= ~(S_IRWXU | S_IRWXG | S_IRWXO | S_ISVTX | S_ISGID | S_ISUID); +#if 0 + /* Disable owner/group permissions equivalence if owner SID == group SID. + It's technically not quite correct, but it helps in case a security + conscious application checks if a file has too open permissions. In + fact, since owner == group, there's no security issue here. */ if (owner_sid && group_sid && RtlEqualSid (owner_sid, group_sid) /* FIXME: temporary exception for /var/empty */ && well_known_system_sid != group_sid) @@ -340,6 +356,7 @@ get_attribute_from_acl (mode_t *attribute, PACL acl, PSID owner_sid, | ((allow & S_IWUSR) ? S_IWGRP : 0) | ((allow & S_IXUSR) ? S_IXGRP : 0)); } +#endif *attribute |= allow; } diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc index d72baaee0..12d89a941 100644 --- a/winsup/cygwin/uinfo.cc +++ b/winsup/cygwin/uinfo.cc @@ -2053,12 +2053,31 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap) switch (acc_type) { case SidTypeUser: - /* Don't allow users as group. While this is technically possible, - it doesn't make sense in a POSIX scenario. It *is* used for - Microsoft Accounts, but those are converted to well-known groups - above. */ - if (is_group ()) - return NULL; + if (is_group () && acc_type == SidTypeUser) + { + /* Don't allow users as group. While this is technically + possible, it doesn't make sense in a POSIX scenario. + + And then there are the so-called Microsoft Accounts. The + special SID with security authority 11 is converted to a + well known group above, but additionally, when logging in + with such an account, the user's primary group SID is the + user's SID. Those we let pass, but no others. */ + bool its_ok = false; + if (wincap.has_microsoft_accounts ()) + { + struct cyg_USER_INFO_24 *ui24; + if (NetUserGetInfo (NULL, name, 24, (PBYTE *) &ui24) + == NERR_Success) + { + if (ui24->usri24_internet_identity) + its_ok = true; + NetApiBufferFree (ui24); + } + } + if (!its_ok) + return NULL; + } /*FALLTHRU*/ case SidTypeGroup: case SidTypeAlias: @@ -2231,25 +2250,6 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap) debug_printf ("NetUserGetInfo(%W) %u", name, nas); break; } - /* Logging in with a Microsoft Account, the user's primary - group SID is the user's SID. Security sensitive tools - expecting tight file permissions choke on that. We need - an explicit primary group which is not identical to the - user account. Unfortunately, while the default primary - group of the account in SAM is still "None", "None" is not - in the user token group list. So, what we do here is to - use "Users" as a sane default primary group instead. */ - if (wincap.has_microsoft_accounts ()) - { - struct cyg_USER_INFO_24 *ui24; - nas = NetUserGetInfo (NULL, name, 24, (PBYTE *) &ui24); - if (nas == NERR_Success) - { - if (ui24->usri24_internet_identity) - gid = DOMAIN_ALIAS_RID_USERS; - NetApiBufferFree (ui24); - } - } /* Fetch user attributes. */ home = cygheap->pg.get_home (ui, sid, dom, name, fully_qualified_name);