* sec_helper.cc (security_descriptor::free): If sd_size is 0, call

	LocalFree instead of ::free.

	* sec_acl.cc: Throughout replace old ACE flag definitions with current
	definitions as used in MSDN man pages.
	* security.cc: Ditto.

	* fhandler.cc (fhandler_base::open): Make sure file has really been
	just created before fixing file permissions.  Add S_JUSTCREATED
	attribute to set_file_attribute call.
	* fhandler_disk_file.cc (fhandler_disk_file::mkdir): Always create dir
	with default security descriptor and fix descriptor afterwards.
	Add S_JUSTCREATED flag to set_file_attribute call.
	* fhandler_socket.cc (fhandler_socket::bind): Ditto for AF_LOCAL
	socket files.
	* path.cc (symlink_worker): Ditto for symlinks.
	* security.cc (get_file_sd): Call GetSecurityInfo rather than
	NtQuerySecurityObject.  Explain why.  Change error handling accordingly.
	(alloc_sd): Skip non-inherited, non-standard entries in ACL if
	S_JUSTCREATED attribute is set.  Explain why.  Minor format fixes.
	* security.h (S_JUSTCREATED): New define.
	(security_descriptor::operator=): New operator.

9 files changed, 93 insertions, 57 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index f89702404..5cf39820d 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,5 +1,30 @@
 2009-10-30  Corinna Vinschen  <corinna@vinschen.de>
 
+	* sec_helper.cc (security_descriptor::free): If sd_size is 0, call
+	LocalFree instead of ::free.
+
+	* sec_acl.cc: Throughout replace old ACE flag definitions with current
+	definitions as used in MSDN man pages.
+	* security.cc: Ditto.
+
+	* fhandler.cc (fhandler_base::open): Make sure file has really been
+	just created before fixing file permissions.  Add S_JUSTCREATED
+	attribute to set_file_attribute call.
+	* fhandler_disk_file.cc (fhandler_disk_file::mkdir): Always create dir
+	with default security descriptor and fix descriptor afterwards.
+	Add S_JUSTCREATED flag to set_file_attribute call.
+	* fhandler_socket.cc (fhandler_socket::bind): Ditto for AF_LOCAL
+	socket files.
+	* path.cc (symlink_worker): Ditto for symlinks.
+	* security.cc (get_file_sd): Call GetSecurityInfo rather than
+	NtQuerySecurityObject.  Explain why.  Change error handling accordingly.
+	(alloc_sd): Skip non-inherited, non-standard entries in ACL if
+	S_JUSTCREATED attribute is set.  Explain why.  Minor format fixes.
+	* security.h (S_JUSTCREATED): New define.
+	(security_descriptor::operator=): New operator.
+
+2009-10-30  Corinna Vinschen  <corinna@vinschen.de>
+
 	* fhandler_random.cc (fhandler_dev_random::lseek): Revert change from
 	2009-10-23.
 
diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index c978d005f..fa93110f7 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -615,8 +615,8 @@ fhandler_base::open (int flags, mode_t mode)
      in a domain as well as on standalone servers.
      This is the result of a discussion on the samba-technical list, starting at
      http://lists.samba.org/archive/samba-technical/2008-July/060247.html */
-  if ((flags & O_CREAT) && has_acls ())
-    set_file_attribute (fh, pc, ILLEGAL_UID, ILLEGAL_GID, mode);
+  if (io.Information == FILE_CREATED && has_acls ())
+    set_file_attribute (fh, pc, ILLEGAL_UID, ILLEGAL_GID, S_JUSTCREATED | mode);
 
   set_io_handle (fh);
   set_flags (flags, pc.binmode ());
diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index 3bf24b037..44e03d31a 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1463,14 +1463,6 @@ fhandler_disk_file::mkdir (mode_t mode)
 {
   int res = -1;
   SECURITY_ATTRIBUTES sa = sec_none_nih;
-  security_descriptor sd;
-
-  /* See comments in fhander_base::open () for an explanation why we defer
-     setting security attributes on remote files. */
-  if (has_acls () && !pc.isremote ())
-    set_security_attribute (pc, S_IFDIR | ((mode & 07777) & ~cygheap->umask),
-			    &sa, sd);
-
   NTSTATUS status;
   HANDLE dir;
   OBJECT_ATTRIBUTES attr;
@@ -1505,9 +1497,10 @@ fhandler_disk_file::mkdir (mode_t mode)
 			 p, plen);
   if (NT_SUCCESS (status))
     {
-      if (has_acls () && pc.isremote ())
+      if (has_acls ())
 	set_file_attribute (dir, pc, ILLEGAL_UID, ILLEGAL_GID,
-			    S_IFDIR | ((mode & 07777) & ~cygheap->umask));
+			    S_JUSTCREATED | S_IFDIR
+			    | ((mode & 07777) & ~cygheap->umask));
       NtClose (dir);
       res = 0;
     }
diff --git a/winsup/cygwin/fhandler_socket.cc b/winsup/cygwin/fhandler_socket.cc
index 5f1ab2776..a1cfce30f 100644
--- a/winsup/cygwin/fhandler_socket.cc
+++ b/winsup/cygwin/fhandler_socket.cc
@@ -890,15 +890,11 @@ fhandler_socket::bind (const struct sockaddr *name, int namelen)
       if (!(mode & (S_IWUSR | S_IWGRP | S_IWOTH)))
 	fattr |= FILE_ATTRIBUTE_READONLY;
       SECURITY_ATTRIBUTES sa = sec_none_nih;
-      security_descriptor sd;
-      /* See comments in fhander_base::open () for an explanation why we defer
-	 setting security attributes on remote files. */
-      if (pc.has_acls () && !pc.isremote ())
-	set_security_attribute (pc, mode, &sa, sd);
       NTSTATUS status;
       HANDLE fh;
       OBJECT_ATTRIBUTES attr;
       IO_STATUS_BLOCK io;
+
       status = NtCreateFile (&fh, DELETE | FILE_GENERIC_WRITE,
 			     pc.get_object_attr (attr, sa), &io, NULL, fattr,
 			     FILE_SHARE_VALID_FLAGS, FILE_CREATE,
@@ -915,8 +911,9 @@ fhandler_socket::bind (const struct sockaddr *name, int namelen)
 	}
       else
 	{
-	  if (pc.has_acls () && pc.isremote ())
-	    set_file_attribute (fh, pc, ILLEGAL_UID, ILLEGAL_GID, mode);
+	  if (pc.has_acls ())
+	    set_file_attribute (fh, pc, ILLEGAL_UID, ILLEGAL_GID,
+				S_JUSTCREATED | mode);
 	  char buf[sizeof (SOCKET_COOKIE) + 80];
 	  __small_sprintf (buf, "%s%u %c ", SOCKET_COOKIE, sin.sin_port,
 			   get_socket_type () == SOCK_STREAM ? 's'
diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 2020eda8f..b51a687c6 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -1399,7 +1399,6 @@ symlink_worker (const char *oldpath, const char *newpath, bool use_winsym,
   path_conv win32_newpath, win32_oldpath;
   char *buf, *cp;
   SECURITY_ATTRIBUTES sa = sec_none_nih;
-  security_descriptor sd;
   OBJECT_ATTRIBUTES attr;
   IO_STATUS_BLOCK io;
   NTSTATUS status;
@@ -1660,11 +1659,6 @@ symlink_worker (const char *oldpath, const char *newpath, bool use_winsym,
 	  goto done;
 	}
     }
-  /* See comments in fhander_base::open () for an explanation why we defer
-     setting security attributes on remote files. */
-  if (win32_newpath.has_acls () && !win32_newpath.isremote ())
-    set_security_attribute (win32_newpath, S_IFLNK | STD_RBITS | STD_WBITS,
-			    &sa, sd);
   status = NtCreateFile (&fh, DELETE | FILE_GENERIC_WRITE,
 			 win32_newpath.get_object_attr (attr, sa),
 			 &io, NULL, FILE_ATTRIBUTE_NORMAL,
@@ -1679,9 +1673,10 @@ symlink_worker (const char *oldpath, const char *newpath, bool use_winsym,
       __seterrno_from_nt_status (status);
       goto done;
     }
-  if (win32_newpath.has_acls () && win32_newpath.isremote ())
+  if (win32_newpath.has_acls ())
     set_file_attribute (fh, win32_newpath, ILLEGAL_UID, ILLEGAL_GID,
-			S_IFLNK | STD_RBITS | STD_WBITS);
+			(io.Information == FILE_CREATED ? S_JUSTCREATED : 0)
+			| S_IFLNK | STD_RBITS | STD_WBITS);
   status = NtWriteFile (fh, NULL, NULL, NULL, &io, buf, cp - buf, NULL, NULL);
   if (NT_SUCCESS (status) && io.Information == (ULONG) (cp - buf))
     {
diff --git a/winsup/cygwin/sec_acl.cc b/winsup/cygwin/sec_acl.cc
index 2a9b4fc71..3a5497574 100644
--- a/winsup/cygwin/sec_acl.cc
+++ b/winsup/cygwin/sec_acl.cc
@@ -123,7 +123,8 @@ setacl (HANDLE handle, path_conv &pc, int nentries, __aclent32_t *aclbufp,
 	allow |= FILE_DELETE_CHILD;
       /* Set inherit property. */
       DWORD inheritance = (aclbufp[i].a_type & ACL_DEFAULT)
-			  ? (SUB_CONTAINERS_AND_OBJECTS_INHERIT | INHERIT_ONLY)
+			  ? (CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE
+			     | INHERIT_ONLY_ACE)
 			  : NO_INHERITANCE;
       /*
        * If a specific acl contains a corresponding default entry with
@@ -138,7 +139,7 @@ setacl (HANDLE handle, path_conv &pc, int nentries, __aclent32_t *aclbufp,
 			       ? aclbufp[i].a_id : ILLEGAL_UID)) >= 0
 	  && aclbufp[i].a_perm == aclbufp[i + 1 + pos].a_perm)
 	{
-	  inheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
+	  inheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE;
 	  /* This invalidates the corresponding default entry. */
 	  aclbufp[i + 1 + pos].a_type = USER|GROUP|ACL_DEFAULT;
 	}
@@ -365,12 +366,13 @@ getacl (HANDLE handle, path_conv &pc, int nentries, __aclent32_t *aclbufp)
 
 	  if (!type)
 	    continue;
-	  if (!(ace->Header.AceFlags & INHERIT_ONLY || type & ACL_DEFAULT))
+	  if (!(ace->Header.AceFlags & INHERIT_ONLY_ACE || type & ACL_DEFAULT))
 	    {
 	      if ((pos = searchace (lacl, MAX_ACL_ENTRIES, type, id)) >= 0)
 		getace (lacl[pos], type, id, ace->Mask, ace->Header.AceType);
 	    }
-	  if ((ace->Header.AceFlags & SUB_CONTAINERS_AND_OBJECTS_INHERIT)
+	  if ((ace->Header.AceFlags
+	      & (CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE))
 	      && pc.isdir ())
 	    {
 	      if (type == USER_OBJ)
diff --git a/winsup/cygwin/sec_helper.cc b/winsup/cygwin/sec_helper.cc
index 63be25c5d..f8e01452f 100644
--- a/winsup/cygwin/sec_helper.cc
+++ b/winsup/cygwin/sec_helper.cc
@@ -298,7 +298,12 @@ void
 security_descriptor::free ()
 {
   if (psd)
-    ::free (psd);
+    {
+      if (!sd_size)
+	LocalFree (psd);
+      else
+	::free (psd);
+    }
   psd = NULL;
   sd_size = 0;
 }
diff --git a/winsup/cygwin/security.cc b/winsup/cygwin/security.cc
index b8750eb8a..5d83060e6 100644
--- a/winsup/cygwin/security.cc
+++ b/winsup/cygwin/security.cc
@@ -24,6 +24,7 @@ details. */
 #include "cygheap.h"
 #include "ntdll.h"
 #include "pwdgrp.h"
+#include <aclapi.h>
 
 #define ALL_SECURITY_INFORMATION (DACL_SECURITY_INFORMATION \
 				  | GROUP_SECURITY_INFORMATION \
@@ -32,8 +33,7 @@ details. */
 LONG
 get_file_sd (HANDLE fh, path_conv &pc, security_descriptor &sd)
 {
-  NTSTATUS status = STATUS_SUCCESS;
-  ULONG len = 0;
+  DWORD error = ERROR_SUCCESS;
   int retry = 0;
   int res = -1;
 
@@ -41,20 +41,17 @@ get_file_sd (HANDLE fh, path_conv &pc, security_descriptor &sd)
     {
       if (fh)
 	{
-	  status = NtQuerySecurityObject (fh, ALL_SECURITY_INFORMATION,
-					  sd, len, &len);
-	  if (status == STATUS_BUFFER_TOO_SMALL)
-	    {
-	      if (!sd.malloc (len))
-		{
-		  set_errno (ENOMEM);
-		  break;
-		}
-	      status = NtQuerySecurityObject (fh, ALL_SECURITY_INFORMATION,
-					      sd, len, &len);
-	    }
-	  if (NT_SUCCESS (status))
+	  /* Amazing but true.  If you want to know if an ACE is inherited
+	     from the parent object, you can't use the NtQuerySecurityObject
+	     function.  In the DACL returned by this functions, the
+	     INHERITED_ACE flag is never set.  Only by calling GetSecurityInfo
+	     you get this information.  Oh well. */
+	  PSECURITY_DESCRIPTOR psd;
+	  error = GetSecurityInfo (fh, SE_FILE_OBJECT, ALL_SECURITY_INFORMATION,
+				   NULL, NULL, NULL, NULL, &psd);
+	  if (error == ERROR_SUCCESS)
 	    {
+	      sd = psd;
 	      res = 0;
 	      break;
 	    }
@@ -63,6 +60,7 @@ get_file_sd (HANDLE fh, path_conv &pc, security_descriptor &sd)
 	{
 	  OBJECT_ATTRIBUTES attr;
 	  IO_STATUS_BLOCK io;
+	  NTSTATUS status;
 
 	  status = NtOpenFile (&fh, READ_CONTROL,
 			       pc.get_object_attr (attr, sec_none_nih),
@@ -71,14 +69,15 @@ get_file_sd (HANDLE fh, path_conv &pc, security_descriptor &sd)
 	  if (!NT_SUCCESS (status))
 	    {
 	      fh = NULL;
+	      error = RtlNtStatusToDosError (status);
 	      break;
 	    }
 	}
     }
   if (retry && fh)
     NtClose (fh);
-  if (!NT_SUCCESS (status))
-    __seterrno_from_nt_status (status);
+  if (error != ERROR_SUCCESS)
+    __seterrno_from_win_error (error);
   return res;
 }
 
@@ -138,7 +137,7 @@ get_attribute_from_acl (mode_t *attribute, PACL acl, PSID owner_sid,
     {
       if (!GetAce (acl, i, (PVOID *) &ace))
 	continue;
-      if (ace->Header.AceFlags & INHERIT_ONLY)
+      if (ace->Header.AceFlags & INHERIT_ONLY_ACE)
 	continue;
       switch (ace->Header.AceType)
 	{
@@ -386,6 +385,9 @@ alloc_sd (path_conv &pc, __uid32_t uid, __gid32_t gid, int attribute,
 {
   BOOL dummy;
 
+  /* NOTE: If the high bit of attribute is set, we have just created
+     a file or directory.  See below for an explanation. */
+
   debug_printf("uid %d, gid %d, attribute %x", uid, gid, attribute);
 
   /* Get owner and group from current security descriptor. */
@@ -583,15 +585,24 @@ alloc_sd (path_conv &pc, __uid32_t uid, __gid32_t gid, int attribute,
 	      || (ace_sid == group_sid)
 	      || (ace_sid == well_known_world_sid))
 	    {
-	      if (ace->Header.AceFlags & SUB_CONTAINERS_AND_OBJECTS_INHERIT)
-		ace->Header.AceFlags |= INHERIT_ONLY;
+	      if (ace->Header.AceFlags
+		  & (CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE))
+		ace->Header.AceFlags |= INHERIT_ONLY_ACE;
 	      else
 		continue;
 	    }
+	  else if ((attribute & S_JUSTCREATED)
+		   && !(ace->Header.AceFlags & INHERITED_ACE))
+	    /* Since files and dirs are created with a NULL descriptor,
+	       inheritence rules kick in.  However, if no inheritable entries
+	       exist in the parent object, Windows will create entries from the
+	       user token's default DACL in the file DACL.  These entries are
+	       not desired and we drop them silently here. */
+	    continue;
 	  /*
 	   * Add unrelated ACCESS_DENIED_ACE to the beginning but
 	   * behind the owner_deny, ACCESS_ALLOWED_ACE to the end.
-	   * FIXME: this would break the order of the inherit_only ACEs
+	   * FIXME: this would break the order of the inherit-only ACEs
 	   */
 	  if (!AddAce (acl, ACL_REVISION,
 		       ace->Header.AceType == ACCESS_DENIED_ACE_TYPE?
@@ -611,9 +622,10 @@ alloc_sd (path_conv &pc, __uid32_t uid, __gid32_t gid, int attribute,
      impact. */
 
   /* Construct appropriate inherit attribute for new directories */
-  if (S_ISDIR (attribute) && !acl_exists)
+  if (S_ISDIR (attribute) && (attribute & S_JUSTCREATED))
     {
-      const DWORD inherit = SUB_CONTAINERS_AND_OBJECTS_INHERIT | INHERIT_ONLY;
+      const DWORD inherit = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE
+			    | INHERIT_ONLY_ACE;
 
 #if 0 /* FIXME: Not done currently as this breaks the canonical order */
       /* Set deny ACE for owner. */
@@ -630,7 +642,8 @@ alloc_sd (path_conv &pc, __uid32_t uid, __gid32_t gid, int attribute,
 #endif
       /* Set allow ACE for owner. */
       if (!add_access_allowed_ace (acl, ace_off++, owner_allow,
-				   well_known_creator_owner_sid, acl_len, inherit))
+				   well_known_creator_owner_sid, acl_len,
+				   inherit))
 	return NULL;
 #if 0 /* FIXME: Not done currently as this breaks the canonical order and
 	 won't be preserved on chown and chmod */
@@ -642,7 +655,8 @@ alloc_sd (path_conv &pc, __uid32_t uid, __gid32_t gid, int attribute,
 #endif
       /* Set allow ACE for group. */
       if (!add_access_allowed_ace (acl, ace_off++, group_allow,
-				   well_known_creator_group_sid, acl_len, inherit))
+				   well_known_creator_group_sid, acl_len,
+				   inherit))
 	return NULL;
       /* Set allow ACE for everyone. */
       if (!add_access_allowed_ace (acl, ace_off++, other_allow,
diff --git a/winsup/cygwin/security.h b/winsup/cygwin/security.h
index d0cb226a9..4ef1a528b 100644
--- a/winsup/cygwin/security.h
+++ b/winsup/cygwin/security.h
@@ -14,6 +14,10 @@ details. */
 
 #include <accctrl.h>
 
+/* Special file attribute set, for instance, in open() and mkdir() to
+   flag that a file has just been created.  Used in alloc_sd, see there. */
+#define S_JUSTCREATED 0x80000000
+
 #define DEFAULT_UID DOMAIN_USER_RID_ADMIN
 #define UNKNOWN_UID 400 /* Non conflicting number */
 #define UNKNOWN_GID 401
@@ -279,6 +283,7 @@ public:
   }
   inline operator const PSECURITY_DESCRIPTOR () { return psd; }
   inline operator PSECURITY_DESCRIPTOR *() { return &psd; }
+  inline void operator =(PSECURITY_DESCRIPTOR nsd) { psd = nsd; }
 };
 
 class user_groups {