Re: [CFR] ucred.cr_gid
On Tue, Jun 26, 2001 at 11:18:56AM -0400, Robert Watson wrote: On Tue, 26 Jun 2001, Ruslan Ermilov wrote: Could someone please take a look at it before I commit this? I won't get a chance to properly review this until I'm at USENIX tomorrow. If you're willing to hold off for about a week, I'd be happy to give it a fairly detailed review: I had some thoughts of doing this when I originally merged ucred and pcred a few weeks ago, but decided to hold off. I'm generally fairly positive about this change, but would be interested in hearing Bruce's thoughts on any compatibility issues, in particular, with respects to the behavior of userland processes with expectations about the old behavior. Obviously, this is a change that is very sensitive to subtle semantic changes on calls--on the other hand, I think moving towards making the supplementary groups being independent from the effect gid is a good goal, as it simplifies our credential code, and improves compatibility. At least one compatibility issue here is that it's no longer possible to use initgroups(3) to set the effective group ID. Date: Fri, 22 Jun 2001 18:05:09 +0300 From: Ruslan Ermilov [EMAIL PROTECTED] To: [EMAIL PROTECTED], [EMAIL PROTECTED] Subject: ucred.cr_gid Message-ID: [EMAIL PROTECTED] Mail-Followup-To: [EMAIL PROTECTED], [EMAIL PROTECTED] Hi! The attached patch replaces ucred.cr_groups[0] with ucred.cr_gid. This is mostly needed for POSIX alignment. setegid(2) etc. should not change supplementary groups set. Also, type of grp.h's group.gr_gid changed to a more natural gid_t (also as in POSIX). Sounds good, I think this change was bandied about once before and perhaps simply didn't get committed. Some of the assorted changes were committed as part of Hesiod import from NetBSD. getgrouplist(3)'s and initgroups(3)'s prototypes fixed. getgrouplist(3) has been also fixed to not duplicate the primary group, and always return number of suplementary groups, even if ngroups is zero (similar to sysctl(3)). Having not looked at the patch yet, just need to make sure I point out the following areas that are sensitive to this type of change: linux and other ABI emulation, where semantic mapping of this sort is already performed, as well as userland applications managing groups. I think my patch handles these. Assorted changes: cmsgcred.cmcred_egidNew This is an ABI change that will break applications compiled for older versions of FreeBSD. Is this a change that applications can detect via some sort of sizeof/sanity check on cmsg results? I can't see how this would break old applications. kproc_info.ki_gid New portal_cred.pcr_gid New xucred.cr_gid New I'm not sure what to do with xucred. Probably reflect changes made in ucred fairly closely. I mean, I'm not sure if we should preserve the 4.2's size of this structure or no, and if so, how to actually do it. Theoretically, this could be done by placing cr_gid in a union with _cr_unused1 and #define that untangles the fact that cr_gid is in a union, but that define would have to be ``#define cr_gid ...'' which is too bad. I'll try to give you a detailed code review in a couple of days. Thanks! Cheers, -- Ruslan Ermilov Oracle Developer/DBA, [EMAIL PROTECTED] Sunbay Software AG, [EMAIL PROTECTED] FreeBSD committer, +380.652.512.251Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: [CFR] ucred.cr_gid
On Wed, 27 Jun 2001, Ruslan Ermilov wrote:
On Tue, Jun 26, 2001 at 11:18:56AM -0400, Robert Watson wrote:
...
off. I'm generally fairly positive about this change, but would be
interested in hearing Bruce's thoughts on any compatibility issues, in
particular, with respects to the behavior of userland processes with
expectations about the old behavior. Obviously, this is a change that is
Me too :-). I don't know much about this except that it is related to
longstanding bugs in gid management.
At least one compatibility issue here is that it's no longer possible
to use initgroups(3) to set the effective group ID.
I think this shows that keeping the egid in group lists is intentional.
The only bug in the current implementation seems to be that NGROUPS_MAX
is 1 too small. The first gid in group lists is conventionally always
the egid, but there must be space for NGROUPS_MAX supplementary groups,
so statically allocated group lists must have size NGROUPS_MAX+1, but
they currently (all?) have size NGROUPS_MAX. POSIX.1-200x documents this
for getgroups(2) -- returning the egid is optional, and getgroups() may
return {NGROUPS_MAX}+1 entries.
I think the semantics of getgroups(), setgroups() and initgroups()
shouldn't be changed. To set a really supplemental gid (one not
affected by setegid(), setgroups() must put the gid in the list after
the first entry even if it is is the egid).
In the kernel, the problem is not really changed by keeping the egid
in a separate variable. I currently slightly prefer keeping it in
group lists.
Binary compatibility could be preserve by hacking NGROUPS_MAX to
NGROUPS_MAX - 1 (ugh). I don't see how to preserve source level
compatibility. You have to change either the semantics by not
putting the egid in group lists, or NGROUPS_MAX to NGROUPS_MAX+1
in many places. Portable applications need the latter change anyway.
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: [CFR] ucred.cr_gid
On Wed, Jun 27, 2001 at 06:40:53PM +1000, Bruce Evans wrote:
On Wed, 27 Jun 2001, Ruslan Ermilov wrote:
On Tue, Jun 26, 2001 at 11:18:56AM -0400, Robert Watson wrote:
...
off. I'm generally fairly positive about this change, but would be
interested in hearing Bruce's thoughts on any compatibility issues, in
particular, with respects to the behavior of userland processes with
expectations about the old behavior. Obviously, this is a change that is
Me too :-). I don't know much about this except that it is related to
longstanding bugs in gid management.
At least one compatibility issue here is that it's no longer possible
to use initgroups(3) to set the effective group ID.
I think this shows that keeping the egid in group lists is intentional.
It's hard to say actually. 4.3BSD up to Tahoe and Net/1 had (in user.h)
explicit holder for egid and NGROUPS supplementary group IDs.
The only bug in the current implementation seems to be that NGROUPS_MAX
is 1 too small. The first gid in group lists is conventionally always
the egid, but there must be space for NGROUPS_MAX supplementary groups,
so statically allocated group lists must have size NGROUPS_MAX+1, but
they currently (all?) have size NGROUPS_MAX. POSIX.1-200x documents this
for getgroups(2) -- returning the egid is optional, and getgroups() may
return {NGROUPS_MAX}+1 entries.
What's wrong with keeping cr_gid in a separate structure member?
Continuing to keep it inside the cr_groups[] would cause us to
deal with NGROUPS_MAX vs. NGROUPS_MAX + 1 calculations all over
the place inside the kernel. This IMHO only unnecessary complicates
the things.
We could still preserve the old behavior of getgroups(2) returning
the effective GID, but this only makes sense if we also preserve
the semantics of setgroups(2) setting the effective GID, which is
bogus; setgroups(2) should only be allowed to set the supplementary
group IDs, like most other OSes do, including NetBSD since 1995.
I think the semantics of getgroups(), setgroups() and initgroups()
shouldn't be changed.
This isn't possible, as if we continue to return egid with getgroups(),
it will now return maximum {NGROUPS_MAX} + 1 gids, as opposed to the
currently documented no more than NGROUPS_MAX will ever be returned,
thus breaking backwards compatibility anyway.
To set a really supplemental gid (one not affected by setegid(),
setgroups() must put the gid in the list after the first entry
even if it is is the egid).
In the kernel, the problem is not really changed by keeping the egid
in a separate variable. I currently slightly prefer keeping it in
group lists.
Again, why?
Binary compatibility could be preserve by hacking NGROUPS_MAX to
NGROUPS_MAX - 1 (ugh). I don't see how to preserve source level
compatibility. You have to change either the semantics by not
putting the egid in group lists, or NGROUPS_MAX to NGROUPS_MAX+1
in many places. Portable applications need the latter change anyway.
BTW, in a second pass, I've found one place where I missed the
obvious change.
Index: kern_prot.c
===
RCS file: /home/ncvs/src/sys/kern/kern_prot.c,v
retrieving revision 1.93
diff -u -p -r1.93 kern_prot.c
--- kern_prot.c 2001/06/06 13:58:03 1.93
+++ kern_prot.c 2001/06/27 10:11:17
@@ -689,7 +687,7 @@ setgroups(p, uap)
* have the egid in the groups[0]). We risk security holes
* when running non-BSD software if we do not do the same.
*/
- newcred-cr_ngroups = 1;
+ newcred-cr_ngroups = 0;
} else {
if ((error = copyin((caddr_t)uap-gidset,
(caddr_t)newcred-cr_groups, ngrp * sizeof(gid_t {
--
Ruslan Ermilov Oracle Developer/DBA,
[EMAIL PROTECTED] Sunbay Software AG,
[EMAIL PROTECTED] FreeBSD committer,
+380.652.512.251Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
[CFR] ucred.cr_gid
Could someone please take a look at it before I commit this? - Forwarded message from Ruslan Ermilov [EMAIL PROTECTED] - Date: Fri, 22 Jun 2001 18:05:09 +0300 From: Ruslan Ermilov [EMAIL PROTECTED] To: [EMAIL PROTECTED], [EMAIL PROTECTED] Subject: ucred.cr_gid Message-ID: [EMAIL PROTECTED] Mail-Followup-To: [EMAIL PROTECTED], [EMAIL PROTECTED] Hi! The attached patch replaces ucred.cr_groups[0] with ucred.cr_gid. This is mostly needed for POSIX alignment. setegid(2) etc. should not change supplementary groups set. Also, type of grp.h's group.gr_gid changed to a more natural gid_t (also as in POSIX). getgrouplist(3)'s and initgroups(3)'s prototypes fixed. getgrouplist(3) has been also fixed to not duplicate the primary group, and always return number of suplementary groups, even if ngroups is zero (similar to sysctl(3)). Assorted changes: cmsgcred.cmcred_egidNew kproc_info.ki_gid New portal_cred.pcr_gid New xucred.cr_gid New I'm not sure what to do with xucred. Also, I'm not sure about KINFO_PROC_SIZE on ia64 and PowerPC. Please review. See also ChangeLog. Cheers, -- Ruslan Ermilov Oracle Developer/DBA, [EMAIL PROTECTED] Sunbay Software AG, [EMAIL PROTECTED] FreeBSD committer, +380.652.512.251Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age - End forwarded message - To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: [CFR] ucred.cr_gid
On Tue, 26 Jun 2001, Ruslan Ermilov wrote: Could someone please take a look at it before I commit this? I won't get a chance to properly review this until I'm at USENIX tomorrow. If you're willing to hold off for about a week, I'd be happy to give it a fairly detailed review: I had some thoughts of doing this when I originally merged ucred and pcred a few weeks ago, but decided to hold off. I'm generally fairly positive about this change, but would be interested in hearing Bruce's thoughts on any compatibility issues, in particular, with respects to the behavior of userland processes with expectations about the old behavior. Obviously, this is a change that is very sensitive to subtle semantic changes on calls--on the other hand, I think moving towards making the supplementary groups being independent from the effect gid is a good goal, as it simplifies our credential code, and improves compatibility. Date: Fri, 22 Jun 2001 18:05:09 +0300 From: Ruslan Ermilov [EMAIL PROTECTED] To: [EMAIL PROTECTED], [EMAIL PROTECTED] Subject: ucred.cr_gid Message-ID: [EMAIL PROTECTED] Mail-Followup-To: [EMAIL PROTECTED], [EMAIL PROTECTED] Hi! The attached patch replaces ucred.cr_groups[0] with ucred.cr_gid. This is mostly needed for POSIX alignment. setegid(2) etc. should not change supplementary groups set. Also, type of grp.h's group.gr_gid changed to a more natural gid_t (also as in POSIX). Sounds good, I think this change was bandied about once before and perhaps simply didn't get committed. getgrouplist(3)'s and initgroups(3)'s prototypes fixed. getgrouplist(3) has been also fixed to not duplicate the primary group, and always return number of suplementary groups, even if ngroups is zero (similar to sysctl(3)). Having not looked at the patch yet, just need to make sure I point out the following areas that are sensitive to this type of change: linux and other ABI emulation, where semantic mapping of this sort is already performed, as well as userland applications managing groups. Assorted changes: cmsgcred.cmcred_egid New This is an ABI change that will break applications compiled for older versions of FreeBSD. Is this a change that applications can detect via some sort of sizeof/sanity check on cmsg results? kproc_info.ki_gid New portal_cred.pcr_gid New xucred.cr_gid New I'm not sure what to do with xucred. Probably reflect changes made in ucred fairly closely. I'll try to give you a detailed code review in a couple of days. Robert N M Watson FreeBSD Core Team, TrustedBSD Project [EMAIL PROTECTED] NAI Labs, Safeport Network Services To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
