* 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".

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  <corinna@vinschen.de>
+
+	* 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  <corinna@vinschen.de>
 
 	* 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);