Date: Wed, 27 Jun 2001 13:40:44 +0300 From: Ruslan Ermilov <ru@FreeBSD.ORG> To: Bruce Evans <bde@zeta.org.au> Cc: Robert Watson <rwatson@FreeBSD.ORG>, arch@FreeBSD.ORG, current@FreeBSD.ORG Subject: Re: [CFR] ucred.cr_gid Message-ID: <20010627134044.A23159@sunbay.com> In-Reply-To: <Pine.BSF.4.21.0106271733290.21476-100000@besplex.bde.org>; from bde@zeta.org.au on Wed, Jun 27, 2001 at 06:40:53PM %2B1000 References: <20010627100905.A2097@sunbay.com> <Pine.BSF.4.21.0106271733290.21476-100000@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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,
ru@sunbay.com Sunbay Software AG,
ru@FreeBSD.org FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010627134044.A23159>