From b42441d32b5a08a8d0e192535aaf7230236b2865 Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Fri, 30 Oct 2009 19:58:53 +0000 Subject: [PATCH] * 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. --- winsup/cygwin/ChangeLog | 25 +++++++++++ winsup/cygwin/fhandler.cc | 4 +- winsup/cygwin/fhandler_disk_file.cc | 13 ++---- winsup/cygwin/fhandler_socket.cc | 11 ++--- winsup/cygwin/path.cc | 11 ++--- winsup/cygwin/sec_acl.cc | 10 +++-- winsup/cygwin/sec_helper.cc | 7 +++- winsup/cygwin/security.cc | 64 ++++++++++++++++++----------- winsup/cygwin/security.h | 5 +++ 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,3 +1,28 @@ +2009-10-30 Corinna Vinschen + + * 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 * fhandler_random.cc (fhandler_dev_random::lseek): Revert change from 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 #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 +/* 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 {