Use NULL dey ACE rather than special Cygwin ACE

	* sec_acl.cc: Change preceeding comment explaining new-style ACLs.
	Describe how to generate deny ACEs in more detail.  Accommodate the
	fact that a NULL deny ACE is used for {DEF_}CLASS_OBJ, rather than
	a special Cygwin ACE.  Improve further comments.
	(CYG_ACE_NEW_STYLE): Define.
	(get_posix_access): Change from Cygwin ACE to NULL deny ACE.  Fix
	CLASS_OBJ handling to generate CLASS_OBJ and DEF_CLASS_OBJ from a single
	NULL deny ACE if the inheritance flags say so.
	* sec_helper.cc (well_known_cygwin_sid): Remove.
	* security.h (well_known_cygwin_sid): Drop declaration.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>

4 files changed, 76 insertions, 49 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index efe62621d..6210391f6 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,5 +1,18 @@
 2015-04-08  Corinna Vinschen  <corinna@vinschen.de>
 
+	* sec_acl.cc: Change preceeding comment explaining new-style ACLs.
+	Describe how to generate deny ACEs in more detail.  Accommodate the
+	fact that a NULL deny ACE is used for {DEF_}CLASS_OBJ, rather than
+	a special Cygwin ACE.  Improve further comments.
+	(CYG_ACE_NEW_STYLE): Define.
+	(get_posix_access): Change from Cygwin ACE to NULL deny ACE.  Fix
+	CLASS_OBJ handling to generate CLASS_OBJ and DEF_CLASS_OBJ from a single
+	NULL deny ACE if the inheritance flags say so.
+	* sec_helper.cc (well_known_cygwin_sid): Remove.
+	* security.h (well_known_cygwin_sid): Drop declaration.
+
+2015-04-08  Corinna Vinschen  <corinna@vinschen.de>
+
 	* include/cyggwin/acl.h (struct __acl16): Move from here...
 	* sec_acl.cc: ...to here.
 
diff --git a/winsup/cygwin/sec_acl.cc b/winsup/cygwin/sec_acl.cc
index f1f474eb2..55ff1bea7 100644
--- a/winsup/cygwin/sec_acl.cc
+++ b/winsup/cygwin/sec_acl.cc
@@ -27,23 +27,30 @@ details. */
 /* How does a correctly constructed new-style Windows ACL claiming to be a
    POSIX ACL look like?
 
-   - Cygwin ACE (special bits, CLASS_OBJ).
+   - NULL ACE (special bits, CLASS_OBJ).
 
-   - If a USER entry has more permissions than any group, Everyone, or if it
-     has more permissions than allowed by the CLASS_OBJ entry:
+   - USER_OBJ deny.  If the user has less permissions than the sum of CLASS_OBJ
+     (or GROUP_OBJ if CLASS_OBJ doesn't exist) and OTHER_OBJ, deny the excess
+     permissions so that group and other perms don't spill into the owner perms.
 
-     USER      deny ACEs   == POSIX allow
-			      & ^(mask | all group allows | Everyone allow)
+       USER_OBJ deny ACE   == ~USER_OBJ & (CLASS_OBJ | OTHER_OBJ)
+     or
+       USER_OBJ deny ACE   == ~USER_OBJ & (GROUP_OBJ | OTHER_OBJ)
+
+   - USER deny.  If a user has different permissions from CLASS_OBJ, or if the
+     user has less permissions than OTHER_OBJ, deny the excess permissions.
+
+       USER deny ACE       == (USER ^ CLASS_OBJ) | (~USER & OTHER_OBJ)
 
    - USER_OBJ  allow ACE
    - USER      allow ACEs
 
      The POSIX permissions returned for a USER entry are the allow bits alone!
 
-   - If a GROUP entry has more permissions than Everyone, or if it has more
-     permissions than allowed by the CLASS_OBJ entry:
+   - GROUP{_OBJ} deny.  If a group has more permissions than CLASS_OBJ,
+     or less permissions than OTHER_OBJ, deny the excess permissions.
 
-     GROUP	deny ACEs  == POSIX allow & ^(mask | Everyone allow)
+       GROUP{_OBJ} deny ACEs  == (GROUP & ~CLASS_OBJ) | (~GROUP & OTHER_OBJ)
 
    - GROUP_OBJ	allow ACE
    - GROUP	allow ACEs
@@ -54,15 +61,16 @@ details. */
 
    Rinse and repeat for default ACEs with INHERIT flags set.
 
-   - Default Cygwin ACE (S_ISGID, CLASS_OBJ). */
+   - Default NULL ACE (S_ISGID, CLASS_OBJ). */
 
 						/* POSIX <-> Win32 */
 
-/* Historically, these bits are stored in a NULL SID ACE.  To distinguish
-   the new ACL style from the old one, we're now using an invented SID, the
-   Cygwin SID (S-1-0-1132029815).  The new ACEs can exist twice in an ACL,
-   the "normal one" containg CLASS_OBJ and special bits, and the one with
-   INHERIT bit set to pass the DEF_CLASS_OBJ bits and the S_ISGID bit on. */
+/* Historically, these bits are stored in a NULL SID ACE.  To distinguish the
+   new ACL style from the old one, we're using an access denied ACE, plus
+   setting an as yet unused bit in the access mask.  The new ACEs can exist
+   twice in an ACL, the "normal one" containing CLASS_OBJ and special bits
+   and the one with INHERIT bit set to pass the DEF_CLASS_OBJ bits and the
+   S_ISGID bit on. */
 #define CYG_ACE_ISVTX		0x001		/* 0x200 <-> 0x001 */
 #define CYG_ACE_ISGID		0x002		/* 0x400 <-> 0x002 */
 #define CYG_ACE_ISUID		0x004		/* 0x800 <-> 0x004 */
@@ -81,6 +89,7 @@ details. */
 #define CYG_ACE_MASK_TO_WIN(val)	\
 				((((val) & S_IRWXO) << 3) \
 				 | CYG_ACE_MASK_VALID)
+#define CYG_ACE_NEW_STYLE	READ_CONTROL	/* New style if set. */
 
 static int
 searchace (aclent_t *aclp, int nentries, int type, uid_t id = ILLEGAL_UID)
@@ -386,7 +395,9 @@ setacl (HANDLE handle, path_conv &pc, int nentries, aclent_t *aclbufp,
   return set_file_sd (handle, pc, sd_ret, false);
 }
 
-/* Temporary access denied bits */
+/* Temporary access denied bits used by getace and get_posix_access during
+   Windows ACL processing.  These bits get removed before the created POSIX
+   ACL gets published. */
 #define DENY_R 040000
 #define DENY_W 020000
 #define DENY_X 010000
@@ -545,44 +556,50 @@ get_posix_access (PSECURITY_DESCRIPTOR psd,
 
       if (ace_sid == well_known_null_sid)
 	{
-	  /* Old-style or non-Cygwin ACL.  Fetch only the special bits. */
-	  attr |= CYG_ACE_ISBITS_TO_POSIX (ace->Mask);
-	  continue;
-	}
-      if (ace_sid == well_known_cygwin_sid)
-	{
-	  /* New-style ACL.  Note the fact that a mask value is present since
-	     that changes how getace fetches the information.  That's fine,
-	     because the Cygwin SID ACE is supposed to precede all USER, GROUP
-	     and GROUP_OBJ entries.  Any ACL not created that way has been
-	     rearranged by the Windows functionality to create the brain-dead
-	     "canonical" ACL order and is broken anyway. */
+	  /* Fetch special bits. */
 	  attr |= CYG_ACE_ISBITS_TO_POSIX (ace->Mask);
-	  if (ace->Mask & CYG_ACE_MASK_VALID)
+	  if (ace->Header.AceType == ACCESS_DENIED_ACE_TYPE
+	      && ace->Mask & CYG_ACE_NEW_STYLE)
 	    {
-	      new_style = true;
-	      type = (ace->Header.AceFlags & SUB_CONTAINERS_AND_OBJECTS_INHERIT)
-		     ? DEF_CLASS_OBJ : CLASS_OBJ;
-	      if ((pos = searchace (lacl, MAX_ACL_ENTRIES, type)) >= 0)
-		{
-		  lacl[pos].a_type = type;
-		  lacl[pos].a_id = ILLEGAL_GID;
-		  lacl[pos].a_perm = CYG_ACE_MASK_TO_POSIX (ace->Mask);
-		}
-	      if (type == CLASS_OBJ)	/* Needed for POSIX permissions. */
+	      /* New-style ACL.  Note the fact that a mask value is present
+		 since that changes how getace fetches the information.  That's
+		 fine, because the Cygwin SID ACE is supposed to precede all
+		 USER, GROUP and GROUP_OBJ entries.  Any ACL not created that
+		 way has been rearranged by the Windows functionality to create
+		 the brain-dead "canonical" ACL order and is broken anyway. */
+	      attr |= CYG_ACE_ISBITS_TO_POSIX (ace->Mask);
+	      if (ace->Mask & CYG_ACE_MASK_VALID)
 		{
-		  has_class_perm = true;
-		  class_perm = lacl[pos].a_perm;
-		}
-	      else
-		{
-		  has_def_class_perm = true;
-		  def_class_perm = lacl[pos].a_perm;
+		  new_style = true;
+		  if (!(ace->Header.AceFlags & INHERIT_ONLY))
+		    {
+		      if ((pos = searchace (lacl, MAX_ACL_ENTRIES, CLASS_OBJ))
+			  >= 0)
+			{
+			  lacl[pos].a_type = CLASS_OBJ;
+			  lacl[pos].a_id = ILLEGAL_GID;
+			  lacl[pos].a_perm = CYG_ACE_MASK_TO_POSIX (ace->Mask);
+			}
+		      has_class_perm = true;
+		      class_perm = lacl[pos].a_perm;
+		    }
+		  if (ace->Header.AceFlags & SUB_CONTAINERS_AND_OBJECTS_INHERIT)
+		    {
+		      if ((pos = searchace (lacl, MAX_ACL_ENTRIES,
+					    DEF_CLASS_OBJ)) >= 0)
+			{
+			  lacl[pos].a_type = DEF_CLASS_OBJ;
+			  lacl[pos].a_id = ILLEGAL_GID;
+			  lacl[pos].a_perm = CYG_ACE_MASK_TO_POSIX (ace->Mask);
+			}
+		      has_def_class_perm = true;
+		      def_class_perm = lacl[pos].a_perm;
+		    }
 		}
 	    }
 	  continue;
 	}
-      else if (ace_sid == owner_sid)
+      if (ace_sid == owner_sid)
 	{
 	  type = USER_OBJ;
 	  id = uid;
diff --git a/winsup/cygwin/sec_helper.cc b/winsup/cygwin/sec_helper.cc
index 1ea69c372..753f15673 100644
--- a/winsup/cygwin/sec_helper.cc
+++ b/winsup/cygwin/sec_helper.cc
@@ -40,8 +40,6 @@ SECURITY_ATTRIBUTES NO_COPY_RO sec_all_nih =
 
 MKSID (well_known_null_sid, "S-1-0-0",
        SECURITY_NULL_SID_AUTHORITY, 1, SECURITY_NULL_RID);
-MKSID (well_known_cygwin_sid, "S-1-0-1132029815",
-       SECURITY_NULL_SID_AUTHORITY, 1, 0x43796777); /* "Cygw" */
 MKSID (well_known_world_sid, "S-1-1-0",
        SECURITY_WORLD_SID_AUTHORITY, 1, SECURITY_WORLD_RID);
 MKSID (well_known_local_sid, "S-1-2-0",
diff --git a/winsup/cygwin/security.h b/winsup/cygwin/security.h
index 7bf0270ef..0378814ca 100644
--- a/winsup/cygwin/security.h
+++ b/winsup/cygwin/security.h
@@ -393,7 +393,6 @@ public:
 };
 
 extern cygpsid well_known_null_sid;
-extern cygpsid well_known_cygwin_sid;
 extern cygpsid well_known_world_sid;
 extern cygpsid well_known_local_sid;
 extern cygpsid well_known_console_logon_sid;