Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Sun, Nov 17, 2002 at 10:42:55PM -0500, Pierre A. Humblet wrote: At 06:56 PM 11/15/2002 +0100, Corinna Vinschen wrote: On Fri, Nov 15, 2002 at 12:29:44PM -0500, Pierre A. Humblet wrote: Alternatively I could add it, but add a check for group sid is SYSTEM, and then skip the step. That would be very easy to do, and to remove later when ssh is ready. I like this best actually. Good idea! Me too. But that must go into both functions, get_attribute_from_acl() and alloc_sd(). OK for get_attribute_from_acl (which is where the /var/empty problem originates), but why for alloc_sd () ? The patch will do the right thing, whereas the current code can give rise to unexpected results. See below. Ok. Applied. Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:[EMAIL PROTECTED] Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
At 06:56 PM 11/15/2002 +0100, Corinna Vinschen wrote:
On Fri, Nov 15, 2002 at 12:29:44PM -0500, Pierre A. Humblet wrote:
Alternatively I could add it, but add a check for group
sid is SYSTEM, and then skip the step. That would be very easy
to do, and to remove later when ssh is ready.
I like this best actually.
Good idea! Me too. But that must go into both functions,
get_attribute_from_acl() and alloc_sd().
OK for get_attribute_from_acl (which is where the /var/empty
problem originates), but why for alloc_sd () ?
The patch will do the right thing, whereas the current code
can give rise to unexpected results. See below.
chmod 755 /var/empty (which has owner == group)
With patch
**
owner: system
group: system
acl: system7
everybody 5 == Effective result 775
Currently
*
owner: system
group: system
acl: system7
system5 == Note duplicate entry
everybody 5 == Effective result 775
A more interesting case is
chmod 575 /var/empty (just as an illustration)
With patch
**
owner: system
group: system
acl: system7 == Single entry, 5 | 7
everybody 5 == Effective result 775
Currently (if user system is NOT listed as a member of group system in
/etc/group)
*
owner: system
group: system
acl: system5
system7
everybody 5 == Effective result 775
Currently (change mkgroup -u to list user system in group system)
*
owner: system
group: system
acl:system deny 2
system5
system7
everybody 5 == Effective result 555 !
B.t.w. let's not change mkgroup, it's not necessary with the patch.
Greetings from him (Michael Hirmke), btw.!
Hi Michael! I now put 2 and 2 together. Michael wrote to me
he was coming back from 5 a week vacation. For a while I was wondering
if October was to Germany as August is to France.
B.t.w. I don't have access to NT on weekends, so it's untested. I don't
expect any trouble but...
Pierre
2002-11-18 Pierre Humblet [EMAIL PROTECTED]
* security.cc (get_attribute_from_acl): Always test anti,
just in case an access_denied ACE follows an access_allowed.
Handle the case owner_sid == group_sid, with a FIXME.
Remove unnecessary tests for non-NULL PSIDs.
(alloc_sd): Use existing owner and group sids if {ug}id == -1.
Handle case where owner_sid == group_sid.
Do not call is_grp_member. Try to preserve canonical ACE order.
Remove unnecessary tests for non-NULL PSIDs. Reorganize
debug_printf's.
(get_initgroups_sidlist): Put well_known_system_sid on left
side of ==.
(add_access_denied_ace): Only call GetAce if inherit != 0.
(add_access_allowed_ace): Ditto. Use appropriate sizeof.
* syscalls.cc (chown_worker): Pass {ug}id equal to -1 to
alloc_sd, which removes the need to obtain old_{ug}id.
(chmod): Remove call to get_file_attribute (), simply pass
{ug}id equal to -1 to alloc_sd.
--- security.cc.orig2002-10-22 23:18:40.0 -0400
+++ security.cc 2002-11-15 19:26:22.0 -0500
@@ -449,7 +449,7 @@ get_user_primary_group (WCHAR *wlogonser
BOOL retval = FALSE;
UCHAR count = 0;
- if (pusersid == well_known_system_sid)
+ if (well_known_system_sid == pusersid)
{
pgrpsid = well_known_system_sid;
return TRUE;
@@ -540,7 +540,7 @@ get_initgroups_sidlist (cygsidlist grp_
{
grp_list += well_known_world_sid;
grp_list += well_known_authenticated_users_sid;
- if (usersid == well_known_system_sid)
+ if (well_known_system_sid == usersid)
{
auth_pos = -1;
grp_list += well_known_admins_sid;
@@ -1238,19 +1238,17 @@ get_attribute_from_acl (int * attribute,
if (ace_sid == well_known_world_sid)
{
if (ace-Mask FILE_READ_DATA)
- *flags |= S_IROTH
+ *flags |= ((!(*anti S_IROTH)) ? S_IROTH : 0)
| ((!(*anti S_IRGRP)) ? S_IRGRP : 0)
| ((!(*anti S_IRUSR)) ? S_IRUSR : 0);
if (ace-Mask FILE_WRITE_DATA)
- *flags |= S_IWOTH
+ *flags |= ((!(*anti S_IWOTH)) ? S_IWOTH : 0)
| ((!(*anti S_IWGRP)) ? S_IWGRP : 0)
| ((!(*anti S_IWUSR)) ? S_IWUSR : 0);
if (ace-Mask FILE_EXECUTE)
- {
- *flags |= S_IXOTH
- | ((!(*anti S_IXGRP)) ? S_IXGRP : 0)
- | ((!(*anti S_IXUSR)) ? S_IXUSR : 0);
- }
+ *flags |= ((!(*anti S_IXOTH)) ? S_IXOTH : 0)
+ | ((!(*anti S_IXGRP)) ? S_IXGRP : 0)
+ | ((!(*anti S_IXUSR)) ? S_IXUSR : 0);
if ((*attribute S_IFDIR)
(ace-Mask (FILE_WRITE_DATA | FILE_EXECUTE | FILE_DELETE_CHILD))
== (FILE_WRITE_DATA | FILE_EXECUTE))
@@ -1266,31 +1264,39 @@ get_attribute_from_acl (int * attribute,
if (ace-Mask FILE_APPEND_DATA)
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Thu, Nov 14, 2002 at 10:04:54PM -0500, Pierre A. Humblet wrote:
Great! Here are my patches. I think they are as we agreed on.
Pierre
Sorry, I still have some problems:
}
}
*attribute = ~(S_IRWXU | S_IRWXG | S_IRWXO | S_ISVTX | S_ISGID | S_ISUID);
+ if (owner_sid group_sid EqualSid (owner_sid, group_sid))
+{
+ allow = ~(S_IRGRP | S_IWGRP | S_IXGRP);
+ allow |= (((allow S_IRUSR) ? S_IRGRP : 0)
+ | ((allow S_IWUSR) ? S_IWGRP : 0)
+ | ((allow S_IXUSR) ? S_IXGRP : 0));
+}
*attribute |= allow;
- *attribute = ~deny;
return;
}
I found that this change had an unpredictable result. sshd refused to
start:
Bad owner or mode for /var/empty
Looking for the modes. Before that change:
drwxr-xr-x2 SYSTEM SYSTEM 0 Jul 7 11:39 /var/empty
With that patch:
drwxrwxr-x2 SYSTEM SYSTEM 0 Jul 7 11:39 /var/empty
I'm not against the patch since it reflects the permissions obviously
better than the old version. However, how many people are running
sshd now with the above /var/empty settings. Urgh.
I only see two choices:
- Let it as it is now.
- Force all people with a running sshd installation to chmod 544
/var/empty.
-1664,18 +1666,25 alloc_sd (__uid32_t uid, __gid32_t gid,
if (attribute S_ISVTX)
null_allow |= FILE_READ_DATA;
}
-
- /* Construct deny attributes for owner and group. */
- DWORD owner_deny = 0;
- if (is_grp_member (uid, gid))
-owner_deny = ~owner_allow (group_allow | other_allow);
Erm... that's not exactly what we agreed upon...
+
+ /* Add owner and group permissions if SIDs are equal
+ and construct deny attributes for group and owner. */
+ DWORD group_deny;
+ if (owner_sid == group_sid)
+{
+ owner_allow |= group_allow;
+ group_allow = group_deny = 0L;
+}
And this change will produce the above sshd problem for any new user
calling ssh-host-config.
if (!AddAce (acl, ACL_REVISION,
ace-Header.AceType == ACCESS_DENIED_ACE_TYPE ?
-(owner_deny ? 1 : 0) : MAXDWORD,
+ace-Mask owner_allow ? owner_off + 1 : owner_off++
+: MAXDWORD,
(LPVOID) ace, ace-Header.AceSize))
After applying the patch to my local sandbox I found that I'm still
having problems here. While I see the advantage for emulating POSIX
permissions closer, I also see that the probability is pretty high
that all unrelated deny ACEs will be placed after the owner_allow
(which probably has most bits set). This doesn't really support the
wish to produce ACLs in canonical order. So far, only the group_deny
could possibly but unlikely be placed after the owner_allow...
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Developermailto:cygwin;cygwin.com
Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
Corinna Vinschen wrote: Sorry, I still have some problems: Me too! Looking for the modes. Before that change: drwxr-xr-x2 SYSTEM SYSTEM 0 Jul 7 11:39 /var/empty With that patch: drwxrwxr-x2 SYSTEM SYSTEM 0 Jul 7 11:39 /var/empty I'm not against the patch since it reflects the permissions obviously better than the old version. However, how many people are running sshd now with the above /var/empty settings. Urgh. I only see two choices: - Let it as it is now. - Force all people with a running sshd installation to chmod 544 /var/empty. I am in favor of showing the truth. The basic problem is that owner == group but they have different permissions, which is non-sensical in Windows. In this specific case it helps to lie, but if we lie and the intention of the user was to chmod to 775 or 770, the user would have every right to complain bitterly and we would have a very lame excuse. I am not familar with /var/empty. Is there a reason why owner == group and the modes are not equal? This is an exceptional case where a postinstall script might help. -1664,18 +1666,25 alloc_sd (__uid32_t uid, __gid32_t gid, if (attribute S_ISVTX) null_allow |= FILE_READ_DATA; } - - /* Construct deny attributes for owner and group. */ - DWORD owner_deny = 0; - if (is_grp_member (uid, gid)) -owner_deny = ~owner_allow (group_allow | other_allow); Erm... that's not exactly what we agreed upon... We discussed is_grp_member in get_nt_attribute and get_nt_object_attribute but this is VERY different. It's in alloc_sd. There is no good reason to call is_grp_member here. The old code was omitting the insertion of an owner_deny ace in that case. That's undesirable because it makes the acl ambiguous. Another bad side effect of omitting the deny ace is that if the groups of the owner of the file are changed, then the modes of the file might also change. if (!AddAce (acl, ACL_REVISION, ace-Header.AceType == ACCESS_DENIED_ACE_TYPE ? -(owner_deny ? 1 : 0) : MAXDWORD, +ace-Mask owner_allow ? owner_off + 1 : owner_off++ +: MAXDWORD, (LPVOID) ace, ace-Header.AceSize)) After applying the patch to my local sandbox I found that I'm still having problems here. What problems? While I see the advantage for emulating POSIX permissions closer, I also see that the probability is pretty high that all unrelated deny ACEs will be placed after the owner_allow (which probably has most bits set). This doesn't really support the wish to produce ACLs in canonical order. So far, only the group_deny could possibly but unlikely be placed after the owner_allow... This is really an issue I don't care about, there are very few unrelated deny ACEs out there. I thought that Cygwin approach was to try to conform to Posix, at the cost of possibly not respecting the canonical order. Once the canonical order is broken (by group_deny), I don't see what extra harm is done to break it a little more with the unrelated ACEs. In both cases the Windows security GUI will complain equally. Pierre
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Fri, Nov 15, 2002 at 10:24:36AM -0500, Pierre A. Humblet wrote: Corinna Vinschen wrote: chgrp 544 or 513 /var/empty but that only works for default /etc/group files. 544 is still the best solution, IMHO. Let's take the long term view. Yep. But as far as I'm concerned we should drop that part of your patch until I could update ssh. It's not a group_deny, it's an owner deny (which would go on top, so canonical order is OK here). Oops, thick fingers... Also if the owner is not in the group when alloc_sd is called, and is placed in the group later, then the owner access mode of the file would change, which isn't POSIX. Let's look at it from another angle: what is gained by going through the trouble of calling is_grp_member and possibly omitting the owner_deny? Since is_grp_member() isn't that slow anymore, what does it hurt to get the situation right in the first place? I'm somehow more and more convinced that this is just a matter of taste. The non canonical order is produced when the group has less permission than everyone, which I agree is unlikely. Yeah, my mind was on another issue. Time for weekend. It's 100% OK with me to give preference to being nice! Ok. I'm really sorry that I'm making your live that hard but I assume you know that I'm just trying to find something as a best solution (if that's at all possible). Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:cygwin;cygwin.com Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Fri, Nov 15, 2002 at 12:29:44PM -0500, Pierre A. Humblet wrote: Corinna Vinschen wrote: Ok. I'm really sorry that I'm making your live that hard but I assume you know that I'm just trying to find something as a best solution (if that's at all possible). Sure, and it's reciprocal. Like I said before, painful as this can be, it is *much* appreciated. cgf
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Fri, Nov 15, 2002 at 12:29:44PM -0500, Pierre A. Humblet wrote: Corinna Vinschen wrote: Yep. But as far as I'm concerned we should drop that part of your patch until I could update ssh. What about putting it in with #if 0 ? It will then be easier to turn it on when ssh is ready. Alternatively I could add it, but add a check for group sid is SYSTEM, and then skip the step. That would be very easy to do, and to remove later when ssh is ready. I like this best actually. Good idea! Me too. But that must go into both functions, get_attribute_from_acl() and alloc_sd(). Since is_grp_member() isn't that slow anymore, what does it hurt to get the situation right in the first place? I'm somehow more and more convinced that this is just a matter of taste. As far as I can see there is absolutely no advantage to calling is_grp_member() in alloc_sd() and by potentially omitting the owner_deny we are making the situation worse! So here I am insistent! Hmm. By the way could you ask your friend if large organizations really use deny ACEs? Are there tools that insert them in ACLs? Historically they are currently not using deny ACEs since they were more or less unknown under NT4. In the next months, they will upgrade to 2K and the usage of deny ACEs is officially projected. Greetings from him (Michael Hirmke), btw.! Have a relaxing weekend! Thanks, you too, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:cygwin;cygwin.com Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Wed, Nov 13, 2002 at 12:32:31PM -0500, Pierre A. Humblet wrote: Corinna Vinschen wrote: It doesn't add any overhead which isn't already there. If already is before the patch, it scans the group file instead of scanning the token groups. If already is after the patch, it scans the group file instead of scanning the token groups or doing nothing, depending if the uid of the file owner differs from the uid of the process. So what? It just uses /etc/group to determine the group membership of user username. What's wrong with that? username is != current user so it reflects the default circumstances for that user. I don't think we can get it better due to Win/POSIX divergence. The fundamental problem is that there is not enough information to know the real permissions of the owner. Is User_foo a member of Admins or not, at the time she accesses the file ? Sure. We can't know that. We're reflecting the default. You make a lot of assumptions in your example. A more detailed description of the way the code works today (before patch) is this: If the process running ls -l is a member of Admins: rwxrwxr-- If the process running ls -l in not a member of Admins: ---rwxr-- and that's the case *whether or not* User_foo is *nominally* a member of Admins. Wait, I'm assuming that we have a corrected version of is_grp_member(). We already know that is_grp_member() isn't quite right, currently. Let's assume is_grp_member() works as expected which means, including my small patch plus a patch to take all groups in the ACL into account. Then the most ugly problem - using the access token of another user - is dropped from our analyzis. Back to the example. Assume that user_foo is a member of Admins in the SAM. The default case is that access tokens are created with Admin being one of the token groups. With the current patch, the output of ls -l would be ---rwxr-- if ls -l is run by somebody else than User_foo It would be rwxrwxr-- if ls -l is run by User_foo if User_foo is *currently* a member of Admins, and ---rwxr-- if ls -l is run by User_foo if User_foo is NOT *currently* a member of Admins To me, that's slightly better than currently. I'm sorry if I miss something here but with my patch it would be rwxrwxr-- if ls -l is run by somebody else than User_foo. Note also that your example assumes implicitly that the ACL was not created by Cygwin. Sure. That's the whole point in this discussion, isn't it? Pure Cygwin ACLs are created according to POSIX rules so that's a non-issue. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:cygwin;cygwin.com Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Thu, Nov 14, 2002 at 09:30:01AM -0500, Pierre A. Humblet wrote: Corinna Vinschen wrote: Isn't the impersonation token automatically read by OpenProcessToken() when an impersonation took place? I don't think so. I just had another look into MSDN and AFAICS, we would have to call OpenThreadToken() with parameter OpenAsSelf set to FALSE instead of OpenProcessToken() to get always the current active access token. Or how do you understand that? - Has anybody reported problems with incorrect owner modes for a file owner different from the current process user? If not, I wouldn't even start writing code for it. We know it can't work all the time, and that to work assuming the default group membership, it needs mkgroup -u (or PDC lookup). Yesterday I checked that at a medium size company (~ 150 persons). There where a total of 1047 names in the gr_mem fields. getgroups32 scans them all, every time. That would be for every file stat. You're trying to frustrate me by reality, don't you? - I am a little bit confused by your patch. Your intention is to skip the token lookup if the file uid isn't that of the current user. You then fall to the bottom of getgroups32. But there the gid is ALWAYS included in the group list (because in the context of getgroups the gid is that of the user, not that of a file). Thus your small patch will always report that the uid is a member of the gid. Look into is_grp_member() again. getgroups32() isn't called with the incoming gid but with pw-pw_gid. Then the incoming gid is compared against the whole list. Could you then please resend the parts of your #1 patch we agreed upon? OK. Do you want to call is_grp_member all the time or only if the current user is the file owner? I still think it's correct to call is_grp_member() always. The actual solution is not to ignore is_grp_member() but to get it to work reasonably well in terms of correctness and (sic) time. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:cygwin;cygwin.com Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Thu, Nov 14, 2002 at 06:23:23PM +0100, Corinna Vinschen wrote: On Thu, Nov 14, 2002 at 12:03:24PM -0500, Pierre A. Humblet wrote: If you are emulated, you already have the token in the cygheap-user. There is no need to open the thread, see how it's done e.g.in setegid. Good point. You want to know if the file owner uid is in the group of the file gid. Write a new routine scanning the /etc/group file until you find the gid. Then scan the members of that group to see if the uid is in it. That's it (well, there will be mutex too, against threads rereading /etc/group). Also a good point. I'm going to rewrite is_grp_member(). I've checked in a patch to getgroups32() and is_grp_member(). getgroups32() now only makes sense for the current user again. It uses the impersonation token if impersonated. is_grp_member() calls getgroups32() only for the current user and scans passwd and group otherwise, trying to be more efficient. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developermailto:cygwin;cygwin.com Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Tue, Nov 12, 2002 at 02:43:51PM -0500, Pierre A. Humblet wrote:
It's just a flaw in is_grp_member() but it's still needed to get the
information about the group membership. is_grp_member() shouldn't check
the current token if the uid isn't myself-uid but otherwise it's ok.
That's the heart of the matter. How do you propose to fix is_grp_member()?
I believe it would involve contacting the PDC for domain users. IMHO it's
unacceptable for performance reasons.
That's why I would only insist on B1), not B3).
I think I found an easy (not necessaily fast) solution which doesn't
involve calling the PDC. Basically we do already depend on /etc/group
heavily so we can do this here, too, IMHO:
Index: grp.cc
===
RCS file: /cvs/src/src/winsup/cygwin/grp.cc,v
retrieving revision 1.56
diff -u -p -r1.56 grp.cc
--- grp.cc 30 Sep 2002 15:17:44 - 1.56
+++ grp.cc 13 Nov 2002 12:34:45 -
-342,6 +342,7 getgroups32 (int gidsetsize, __gid32_t *
read_etc_group ();
if (allow_ntsec
+ strcasematch (username, cygheap-user.name ())
OpenProcessToken (hMainProc, TOKEN_QUERY, hToken))
{
if (GetTokenInformation (hToken, TokenGroups, NULL, 0, size)
This skips the GetTokenInformation and just uses the group info
found in /etc/group. What do you think (besides trying to speed
up getgroups32)?
Why are you turning around the order of the conditionals? I don't see
a reason.
One side is a cygsid, the other is a PSID. I would like the cygsid ==
operator to apply. Apparently the order matters (I didn't know it).
Ah, ok.
to test. I should really replace the last line above by owner_sid == group_sid,
although that returns TRUE if both are NULL, so there is a small difference
(but that case should never arise).
Yep.
+{
+ allow = ~(S_IRGRP | S_IWGRP | S_IXGRP);
This line is essentially superfluous. It's job has been done two lines
before.
Uh?
I'm sorry, I misread the situation.
Why do you remove this check? It's still needed when called by
set_security_attribute(). Or set_security_attribute() needs that check.
set_security_attribute() is only called when a file has acls, which AFAIK
can only happen on NT. So it's OK.
Hmm, you're right.
Why do you remove this check? It's still an interesting info, isn't it?
Yes, it's just for uniformity of interface. None of the other security
routines go into that level of detail when they read passwd.
The information is available from the remaining debug_printf.
Ok.
- owner_sid.debug_print (alloc_sd: owner SID =);
And this one?
I thought that was a debug leftover from the time you introduced the
caching. It's not done anywhere else.
No, I was already often happy to see the SID at this point. It helped
debugging. I'd like to keep it.
if (!AddAce (acl, ACL_REVISION,
ace-Header.AceType == ACCESS_DENIED_ACE_TYPE ?
-(owner_deny ? 1 : 0) : MAXDWORD,
+ace-Mask owner_allow ? owner_off + 1 : owner_off++
Could you explain how that should work? I'm not sure about that.
owner_off is the position of the owner_allow ACE. Since the above
test already filters all related ACEs, the incoming ACE is unrelated
to the owner entry. So why do you check the content of the ACE
against the bits set in the owner_allow mask?!?
Good question. I was puzzled by the original comment and code:
* Add unrelated ACCESS_DENIED_ACE to the beginning but
* behind the owner_deny, ACCESS_ALLOWED_ACE to the end.
The ACCESS_DENIED_ACE part made no sense to me because if there are a
bunch of ACCESS_DENIED following each other, their order doesn't matter.
So I thought that the intention of the original code was that unrelated
ACE should not affect the owner and should thus be positioned after the
ACCESS_ALLOWED_ACE of the owner (i.e. the original author meant
owner_allow, not owner_deny, in the comment).
No, the original code actually knows that the current situation is
either
owner_allow
group_ace
...
or
owner_deny
owner_allow
group_ace
...
The code just differs between having or having not an owner_deny.
All denies are going to the beginning of the ACL, maintaining the
canonical order. The canonical order places user ACEs in from of
group ACEs. To keep this order, the remaining unrelated deny
ACEs are positioned after the owner ACE. That's all and that's
correct, AFAICS.
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Developermailto:cygwin;cygwin.com
Red Hat, Inc.
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
Corinna Vinschen wrote: On Tue, Nov 12, 2002 at 02:43:51PM -0500, Pierre A. Humblet wrote: It's just a flaw in is_grp_member() but it's still needed to get the information about the group membership. is_grp_member() shouldn't check the current token if the uid isn't myself-uid but otherwise it's ok. That's the heart of the matter. How do you propose to fix is_grp_member()? I believe it would involve contacting the PDC for domain users. IMHO it's unacceptable for performance reasons. That's why I would only insist on B1), not B3). I think I found an easy (not necessaily fast) solution which doesn't involve calling the PDC. Basically we do already depend on /etc/group heavily so we can do this here, too, IMHO: Yes, that works (most of the time) assuming that /etc/group was build with mkgroup -u. But still, I wouldn't do it, for many reasons: - it adds overhead to stat (), which is already slow - it only considers the gid of the file, but not the unrelated groups that may be present in the file acl, so it doesn't achieve B3 nor B2. - it doesn't help at all if mkgroup wasn't run with -u - it doesn't help at all when the acl was built by cygwin - it doesn't help at all in the usual case where the owner permissions are wider than those of groups - it assumes that all processes with a given uid will always run with the groups derived from /etc/passwd and /etc/group. Although this is typical, it is not necessarily true. setgid () or setgroups () might have been called to change that. - the permission for the owner should reflect the permission given to the uid itself, independently of the groups that the process may or may not be in at the time it accesses a file. - We rely on /etc/group mostly to map gid = sid. Relying on it even more exposes us to user screw up and unexpected behavior. Again, the permission given to a uid in Unix are independent of what's in /etc/group. Unfortunately the Windows and Unix models of security diverge here, and Cygwin is caught in between. I would go for simplicity. - owner_sid.debug_print (alloc_sd: owner SID =); And this one? I thought that was a debug leftover from the time you introduced the caching. It's not done anywhere else. No, I was already often happy to see the SID at this point. It helped debugging. I'd like to keep it. OK, then for consistency let's also debug_print the group SID. Good question. I was puzzled by the original comment and code: * Add unrelated ACCESS_DENIED_ACE to the beginning but * behind the owner_deny, ACCESS_ALLOWED_ACE to the end. The ACCESS_DENIED_ACE part made no sense to me because if there are a bunch of ACCESS_DENIED following each other, their order doesn't matter. So I thought that the intention of the original code was that unrelated ACE should not affect the owner and should thus be positioned after the ACCESS_ALLOWED_ACE of the owner (i.e. the original author meant owner_allow, not owner_deny, in the comment). No, the original code actually knows that the current situation is either owner_allow group_ace ... or owner_deny owner_allow group_ace ... The code just differs between having or having not an owner_deny. All denies are going to the beginning of the ACL, maintaining the canonical order. The canonical order places user ACEs in from of group ACEs. To keep this order, the remaining unrelated deny ACEs are positioned after the owner ACE. That's all and that's correct, AFAICS. OK, what you are proposing is actually close to what the patch I sent at the beginning of October was doing. The existing code behaves as follows: in the first case above it inserts the unrelated deny acl at the top, in the second case it inserts it after the owner_deny. There is no reason for this dual behavior, my October patch was always inserting the unrelated deny acl at the top. The reason why I changed the behavior between October and November is very much related to the discussion on is_grp_member(). The new method insures that the group membership of the owner does not impede a right that was given explicitly given to the owner. In other words, if I chmod a file 700, it shouldn't be the case that the owner only has 600 because she happens to be in some unrelated group that was in the old acl. Doing so (changing the right) is actually closer to the Windows security model than to the Unix one. Pierre
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Wed, Nov 13, 2002 at 11:18:33AM -0500, Pierre A. Humblet wrote: Corinna Vinschen wrote: I think I found an easy (not necessaily fast) solution which doesn't involve calling the PDC. Basically we do already depend on /etc/group heavily so we can do this here, too, IMHO: Yes, that works (most of the time) assuming that /etc/group was build with mkgroup -u. But still, I wouldn't do it, for many reasons: - it adds overhead to stat (), which is already slow It doesn't add any overhead which isn't already there. - it only considers the gid of the file, but not the unrelated groups that may be present in the file acl, so it doesn't achieve B3 nor B2. Agree. - it doesn't help at all if mkgroup wasn't run with -u - it doesn't help at all when the acl was built by cygwin - it doesn't help at all in the usual case where the owner permissions are wider than those of groups The whole thing is a result of the divergence between Win and Posix. Unfortunately it's not always the usual case to have owner permissions wider that group permissions. Often there aren't even owner permissions in the ACL since the permissions are given by only group permissions. And in contrast to Posix the permissions add up to each other. - it assumes that all processes with a given uid will always run with the groups derived from /etc/passwd and /etc/group. Although this is typical, it is not necessarily true. setgid () or setgroups () might have been called to change that. - the permission for the owner should reflect the permission given to the uid itself, independently of the groups that the process may or may not be in at the time it accesses a file. The problem we're trying to circumvent is the following: Owner: User_foo Group: Admins ACL:Admins rwx Everyone r-- Following your suggestion we get: $ ls -l ---rwxr-- 1 User_foo Admins ... bar This doesn't reflect the real permissions of the user at all, not even approximately. The current code creates: $ ls -l rwxrwxr-- 1 User_foo Admins ... bar which reflects the truth somewhat better *and* solves a lot of problems we had in earlier implementations. Granted that the implementation isn't complete. - We rely on /etc/group mostly to map gid = sid. Relying on it even more exposes us to user screw up and unexpected behavior. But that invalidates your attempts to rely more on /etc/group instead of calling LookupAccountSid/Name. That's somewhat chicken-egg, isn't it? I try to avoid time lags by not calling LookupAccountSid means to rely more on the correctness of /etc/group and vice versa... Again, the permission given to a uid in Unix are independent of what's in /etc/group. But not in Windows. Unfortunately the Windows and Unix models of security diverge here, and Cygwin is caught in between. I would go for simplicity. The above ls -l example shows the result if we don't use is_grp_member(). We already had a lot of problems due to this some time ago. I won't return to the old state. I, for one, would better like to improve is_grp_member(). OK, then for consistency let's also debug_print the group SID. Ok. The code just differs between having or having not an owner_deny. All denies are going to the beginning of the ACL, maintaining the canonical order. The canonical order places user ACEs in from of group ACEs. To keep this order, the remaining unrelated deny ACEs are positioned after the owner ACE. That's all and that's correct, AFAICS. OK, what you are proposing is actually close to what the patch I sent at the beginning of October was doing. The existing code behaves as follows: in the first case above it inserts the unrelated deny acl at the top, in the second case it inserts it after the owner_deny. There is no reason for this dual behavior, my October patch was always inserting the unrelated deny acl at the top. The canonical order places user ACEs in from of group ACEs. The reason why I changed the behavior between October and November is very much related to the discussion on is_grp_member(). The new method insures that the group membership of the owner does not impede a right that was given explicitly given to the owner. In other words, if I chmod a file 700, it shouldn't be the case that the owner only has 600 because she happens to be in some unrelated group that was in the old acl. Doing so (changing the right) is actually closer to the Windows security model than to the Unix one. Sigh, yes, the Windows security model. It's lovely, isn't it? If there's any order which would make sense, then it's user_denies user_allows group_denies group_allows everyone but that's probably too sophisticated... I agree that moving the group_denies after the user_allow is closer to what *we* want. I recall a discussion on the cygwin ML (was it in 2000? I don't know) about Cygwin being incompatible to NT4
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
Oops, there is an error in our examples. The Everyone permission should propagate. Here is the correct output. Pierre A. Humblet wrote: Corinna Vinschen wrote: On Wed, Nov 13, 2002 at 11:18:33AM -0500, Pierre A. Humblet wrote: Corinna Vinschen wrote: I think I found an easy (not necessaily fast) solution which doesn't involve calling the PDC. Basically we do already depend on /etc/group heavily so we can do this here, too, IMHO: Yes, that works (most of the time) assuming that /etc/group was build with mkgroup -u. But still, I wouldn't do it, for many reasons: - it adds overhead to stat (), which is already slow It doesn't add any overhead which isn't already there. If already is before the patch, it scans the group file instead of scanning the token groups. If already is after the patch, it scans the group file instead of scanning the token groups or doing nothing, depending if the uid of the file owner differs from the uid of the process. The problem we're trying to circumvent is the following: Owner: User_foo Group: Admins ACL:Admins rwx Everyone r-- Following your suggestion we get: $ ls -l ---rwxr-- 1 User_foo Admins ... bar This doesn't reflect the real permissions of the user at all, not even approximately. The current code creates: $ ls -l rwxrwxr-- 1 User_foo Admins ... bar which reflects the truth somewhat better *and* solves a lot of problems we had in earlier implementations. The fundamental problem is that there is not enough information to know the real permissions of the owner. Is User_foo a member of Admins or not, at the time she accesses the file ? You make a lot of assumptions in your example. A more detailed description of the way the code works today (before patch) is this: If the process running ls -l is a member of Admins: rwxrwxr-- If the process running ls -l in not a member of Admins: ---rwxr-- and that's the case *whether or not* User_foo is *nominally* a member of Admins. Note the dependency on the process running the command and the absence of dependency on what User_foo actually belongs to !!! Your example implicitly assumes that User_foo is a member of Admins and the ls -l is run by a member of Admins. With the current patch, the output of ls -l would be r--rwxr-- if ls -l is run by somebody else than User_foo It would be rwxrwxr-- if ls -l is run by User_foo if User_foo is *currently* a member of Admins, and r--rwxr-- if ls -l is run by User_foo if User_foo is NOT *currently* a member of Admins To me, that's slightly better than currently. Note also that your example assumes implicitly that the ACL was not created by Cygwin. With the current patch, if the chmod is 774, the acl would be User_foo rwx Adminsrwx Everyone r-- if the chmod is 074, the acl would be User_foo deny rwx Admins rwx Everyone r-- if the chmod is 474 the acl would be User_foo deny -wx User_foo r__ Admins rwx Everyone r-- So there is absolutely no dependency on whether User_foo is or isn't a member of Admins at the time she accesses the file. Everything is completely determined by the chmod and it is not necessary to scan the token groups. Pierre
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
On Wed, Nov 13, 2002 at 10:35:09PM -0500, Pierre A. Humblet wrote: At 05:50 PM 11/13/2002 +0100, Corinna Vinschen wrote: The above ls -l example shows the result if we don't use is_grp_member(). We already had a lot of problems due to this some time ago. I won't return to the old state. I, for one, would better like to improve is_grp_member(). Hello Corinna, Sorry, didn't respond to that paragraph in my previous e-mail. I agree that is_grp_member () is useful and withdraw my suggestion to eliminate it. I have nothing useful to add here other than the fact that I am really loving this dialog. I like to see things being hashed out like this. Pierre, I really appreciate your delving into issues here. And, of course I appreciate all that Corinna has done with ntsec. I am sure that she regrets ever listening to me when I suggested that she do something with Windows permissions years ago. Not that I am trying to insert any claim to helping with ntsec. This is all a black box to me. Anyway, I think between the two of you, we'll eventually have things working as well as humanly possible on Windows. I'm happy to see you working on these issues. My 2c. cgf
Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member
Corinna Vinschen wrote:
Hi Pierre,
On Wed, 06 Nov 2002 11:28:30 -0500, Pierre A. Humblet wrote:
Note that is_grp_member is expensive: a passwd scan + getting the token
groups in a malloc'ed structure. I am wondering if the effort is
justified, considering that it is useless when the ACL is built by
Cygwin (because Cygwin will put an access denied ACE if needed).
basically the function has been designed to help acl_access (the
implementation of access(2) when ntsec is on, see sec_acl.cc).
I looked into that implementation and there it's called always with
the correct (own) uid.
No problem there.
This raises a basic issue: What is get_attribute_from acl trying to
accomplish? I can see several answers:
A) For other and group, have modes report the true access rights
A1) if the ACL was built by Cygwin
A2) all the time.
I believe we can easily do A2.
B) For user, have modes report the true access rights
B1) if the ACL was built by Cygwin
B2) if the file uid is the current euid
B3) all the time
The patch stands somewhere between B1 and B2 (we don't take all the
group ACE in consideration, only the gid). Should we reduce to B1
(by removing is_grp_member completely)
or extend to B2 (perhaps by using AccessCheck)?
Doing B3 would require looking up the PDC etc.., not recommended.
I don't understand. get_attribute_from_acl() creates a UNIX mode from
the ACL. It gets the owner and group SID as input so it just creates
the UNIX permission bits from the ACL according to the way they are
interpreted by NT. What is the exact problem? Somehow I'm missing
that.
The problem is the flaw in is_grp_member, see below.
Although I didn't do it, I would remove is_grp_member completely.
If it is kept, the output of stat and ls -l can depend on the sid
of the user running the command. That's undesirable.
It's just a flaw in is_grp_member() but it's still needed to get the
information about the group membership. is_grp_member() shouldn't check
the current token if the uid isn't myself-uid but otherwise it's ok.
That's the heart of the matter. How do you propose to fix is_grp_member()?
I believe it would involve contacting the PDC for domain users. IMHO it's
unacceptable for performance reasons.
That's why I would only insist on B1), not B3).
Why are you turning around the order of the conditionals? I don't see
a reason.
One side is a cygsid, the other is a PSID. I would like the cygsid ==
operator to apply. Apparently the order matters (I didn't know it).
-1266,31 +1264,37 get_attribute_from_acl (int * attribute,
if (ace-Mask FILE_APPEND_DATA)
*flags |= S_ISUID;
}
- else if (owner_sid ace_sid == owner_sid)
+ else if (ace_sid == owner_sid)
[...]
- else if (group_sid ace_sid == group_sid)
+ else if (ace_sid == group_sid)
[...]
*attribute = ~(S_IRWXU | S_IRWXG | S_IRWXO | S_ISVTX | S_ISGID | S_ISUID);
+ if (owner_sid group_sid EqualSid (owner_sid, group_sid))
Why are you checking owner_sid group_sid for non-NULL here while
removing these checks in the lines before?
Because in the lines before the cygsid == operator already checks for non-null,
so it's useless to test twice. However EqualSid doesn't test, so it's necessary
to test. I should really replace the last line above by owner_sid == group_sid,
although that returns TRUE if both are NULL, so there is a small difference
(but that case should never arise).
+{
+ allow = ~(S_IRGRP | S_IWGRP | S_IXGRP);
This line is essentially superfluous. It's job has been done two lines
before.
Uh?
Why do you remove this check? It's still needed when called by
set_security_attribute(). Or set_security_attribute() needs that check.
set_security_attribute() is only called when a file has acls, which AFAIK
can only happen on NT. So it's OK.
If it's not OK, the test should be moved to set_security_attribute()
as you suggest.
- if (!pw)
- {
- debug_printf (no /etc/passwd entry for %d, uid);
- set_errno (EINVAL);
- return NULL;
- }
Why do you remove this check? It's still an interesting info, isn't it?
Yes, it's just for uniformity of interface. None of the other security
routines go into that level of detail when they read passwd.
The information is available from the remaining debug_printf.
- owner_sid.debug_print (alloc_sd: owner SID =);
And this one?
I thought that was a debug leftover from the time you introduced the
caching. It's not done anywhere else.
if (gid == myself-gid)
group_sid = cygheap-user.groups.pgsid;
+ else if (uid == ILLEGAL_GID)
^^^
oops, thanks.
-* Add unrelated ACCESS_DENIED_ACE to the beginning but
-* behind the owner_deny, ACCESS_ALLOWED_ACE to the end.
+* Add unrelated ACCESS_DENIED_ACE to the beginning,
+* preferrably before the
