Date: Tue, 23 Sep 2025 10:24:18 -0500 From: Kyle Evans <kevans@FreeBSD.org> To: Olivier Certner <olce@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: Re: git: 8878569103a3 - stable/15 - initgroups(3): Add a pre-FreeBSD-15-compatible version Message-ID: <8f835667-5d3c-4972-a6ee-0a2d4f20269e@FreeBSD.org> In-Reply-To: <202509231203.58NC3lOg008905@gitrepo.freebsd.org> References: <202509231203.58NC3lOg008905@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 9/23/25 07:03, Olivier Certner wrote: > The branch stable/15 has been updated by olce: > > URL: https://cgit.FreeBSD.org/src/commit/?id=8878569103a382aa2c7142c203285a50484fe298 > > commit 8878569103a382aa2c7142c203285a50484fe298 > Author: Olivier Certner <olce@FreeBSD.org> > AuthorDate: 2025-08-29 14:19:33 +0000 > Commit: Olivier Certner <olce@FreeBSD.org> > CommitDate: 2025-09-23 12:02:46 +0000 > > initgroups(3): Add a pre-FreeBSD-15-compatible version > > After commit 9da2fe96ff2e ("kern: fix setgroups(2) and getgroups(2) to > match other platforms"), initgroups() does not set the effective GID > anymore and uses all passed groups as the supplementary group list. > This effectively breaks backwards compatibility with programs/libraries > compiled on a FreeBSD 14 or earlier system. > > Restore compatibility by creating a new version of the 'initgroups' > symbol that designates the current implementation and providing > a pre-FreeBSD-15-compatible version under the symbol's previously > exported version. The new version calls the new setgroups(2) system > call, while the compatible one calls the original one (called > freebsd14_setgroups()). > > Update the manual page with some history and comparison with other > current open-source systems. Add a "SECURITY CONSIDERATIONS" section > highlighting some security properties of this approach and the reasons > we adopt it. While here, revamp the manual page, in particular to use > the exact POSIX terminology where possible. > > Note for MFC to stable/14: Only the manual page update is to be MFCed, > and the text changed to reflect the old behavior and inform readers of > the new upcoming behavior in 15. > > Reviewed by: kib > Fixes: 9da2fe96ff2e ("kern: fix setgroups(2) and getgroups(2) to match other platforms") > MFC after: 5 days > Sponsored by: The FreeBSD Foundation > Differential Revision: https://reviews.freebsd.org/D52282 > > (cherry picked from commit 9dc1ac8691966480ff8bd9c37dd405b981b41dd5) This one should not have been MFC'd without the follow-up that fixes initgroups@FBSD_1.0, please back it out or MFC the fix ASAP- it doesn't make any sense to leave stable/15 broken like this when we knew there was a problem. Thanks, Kyle Evans > --- > lib/libc/gen/Symbol.map | 2 +- > lib/libc/gen/gen-compat.h | 2 + > lib/libc/gen/initgroups.3 | 101 +++++++++++++++++++++++++++++++++++++++------- > lib/libc/gen/initgroups.c | 44 +++++++++++++++----- > 4 files changed, 124 insertions(+), 25 deletions(-) > > diff --git a/lib/libc/gen/Symbol.map b/lib/libc/gen/Symbol.map > index 26f638568efc..494b65bc5cc1 100644 > --- a/lib/libc/gen/Symbol.map > +++ b/lib/libc/gen/Symbol.map > @@ -193,7 +193,6 @@ FBSD_1.0 { > __isinff; > __isinfl; > isatty; > - initgroups; > jrand48; > lcong48; > ldexp; > @@ -462,6 +461,7 @@ FBSD_1.8 { > fdscandir_b; > fts_open_b; > glob_b; > + initgroups; > inotify_add_watch; > inotify_init; > inotify_init1; > diff --git a/lib/libc/gen/gen-compat.h b/lib/libc/gen/gen-compat.h > index 08e80ede6b6e..dac8f54b45a2 100644 > --- a/lib/libc/gen/gen-compat.h > +++ b/lib/libc/gen/gen-compat.h > @@ -52,4 +52,6 @@ int freebsd11_getmntinfo(struct freebsd11_statfs **, int); > char *freebsd11_devname(__uint32_t dev, __mode_t type); > char *freebsd11_devname_r(__uint32_t dev, __mode_t type, char *buf, int len); > > +int freebsd14_setgroups(int gidsize, const __gid_t *gidset); > + > #endif /* _GEN_COMPAT_H_ */ > diff --git a/lib/libc/gen/initgroups.3 b/lib/libc/gen/initgroups.3 > index 03bd07494fc9..4f538fb180ec 100644 > --- a/lib/libc/gen/initgroups.3 > +++ b/lib/libc/gen/initgroups.3 > @@ -1,5 +1,13 @@ > +.\"- > +.\" SPDX-License-Identifier: BSD-3-Clause > +.\" > .\" Copyright (c) 1983, 1991, 1993 > .\" The Regents of the University of California. All rights reserved. > +.\" Copyright (c) 2025 The FreeBSD Foundation > +.\" > +.\" Portions of this documentation were written by Olivier Certner > +.\" <olce@FreeBSD.org> at Kumacom SARL under sponsorship from the FreeBSD > +.\" Foundation. > .\" > .\" Redistribution and use in source and binary forms, with or without > .\" modification, are permitted provided that the following conditions > @@ -25,12 +33,12 @@ > .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > .\" SUCH DAMAGE. > .\" > -.Dd October 26, 2014 > +.Dd September 17, 2025 > .Dt INITGROUPS 3 > .Os > .Sh NAME > .Nm initgroups > -.Nd initialize group access list > +.Nd initialize supplementary groups as per the group database > .Sh LIBRARY > .Lb libc > .Sh SYNOPSIS > @@ -40,19 +48,18 @@ > .Sh DESCRIPTION > The > .Fn initgroups > -function > -uses the > -.Xr getgrouplist 3 > -function to calculate the group access list for the user > -specified in > +function initializes the current process' supplementary groups as prescribed by > +its arguments and the system's group database. > +.Pp > +It first uses the > +.Fn getgrouplist > +function to compute a list of groups containing the passed > +.Fa basegid , > +which typically is the user's initial numerical group ID from the password > +database, and the supplementary groups in the group database for the user named > .Fa name . > -This group list is then setup for the current process using > -.Xr setgroups 2 . > -The > -.Fa basegid > -is automatically included in the groups list. > -Typically this value is given as > -the group number from the password file. > +It then installs this list as the current process' supplementary groups using > +.Fn setgroups . > .Sh RETURN VALUES > .Rv -std initgroups > .Sh ERRORS > @@ -60,7 +67,7 @@ The > .Fn initgroups > function may fail and set > .Va errno > -for any of the errors specified for the library function > +to any of the errors specified for the library function > .Xr setgroups 2 . > It may also return: > .Bl -tag -width Er > @@ -77,3 +84,67 @@ The > .Fn initgroups > function appeared in > .Bx 4.2 . > +.Pp > +The > +.Fn initgroups > +function changed semantics in > +.Fx 15 , > +following that of > +.Xr setgroups 2 > +in the same release. > +Before that, it would also set the effective group ID to > +.Fa basegid , > +and would not include the latter in the supplementary groups except before > +.Fx 8 . > +Its current behavior in these respects is known to be compatible with that of > +the following systems up to the specified versions that are current at time of > +this writing: > +.Bl -dash -width "-" -compact > +.It > +Linux (up to 6.6) with the GNU libc (up to 2.42) > +.It > +.Nx 1.1 and greater (up to 10) > +.It > +.Ox (up to 7.7) > +.It > +Systems based on illumos (up to August 2025 sources) > +.El > +.Sh SECURITY CONSIDERATIONS > +As > +.Fa basegid > +is typically the user's initial numerical group ID, to which the current > +process' effective group ID is generally initialized, processes using functions > +to change their effective group ID > +.Pq via Xr setgid 2 or similar > +or that are spawned from executables with the set-group-ID mode bit set will not > +be able to relinquish the access rights deriving from being a member of > +.Fa basegid , > +as these functions do not change the supplementary groups. > +.Pp > +This behavior is generally desirable in order to paper over the difference of > +treatment between the effective group and supplementary ones in this situation, > +as they are all in the end indiscriminately used in traditional UNIX > +discretionary access checks. > +It blends well with the practice of allocating each user its own private group, > +as processes launched from a set-group-ID executable keep the same user and > +consistently stay also in the same user's group. > +Finally, it was also chosen for compatibility with other systems > +.Po > +see the > +.Sx HISTORY > +section > +.Pc . > +.Pp > +This convention of including > +.Fa basegid > +in the supplementary groups is however only enforced by the > +.Fn initgroups > +function, and not by the > +.Xr setgroups 2 > +system call, so applications expressly wanting to include in the supplementary > +groups only those specified by the group database can themselves call > +.Fn getgrouplist > +and then > +.Fn setgroups > +on the result with the first element skipped > +.Pq see Xr getgrouplist 3 . > diff --git a/lib/libc/gen/initgroups.c b/lib/libc/gen/initgroups.c > index 55f17a94fa8e..a1a7d92250e2 100644 > --- a/lib/libc/gen/initgroups.c > +++ b/lib/libc/gen/initgroups.c > @@ -3,6 +3,11 @@ > * > * Copyright (c) 1983, 1993 > * The Regents of the University of California. All rights reserved. > + * Copyright (c) 2025 The FreeBSD Foundation > + * > + * Portions of this software were developed by Olivier Certner > + * <olce@FreeBSD.org> at Kumacom SARL under sponsorship from the FreeBSD > + * Foundation. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -29,22 +34,28 @@ > * SUCH DAMAGE. > */ > > -#include <sys/param.h> > +/* For __sym_compat(). */ > +#include <sys/cdefs.h> > > #include <errno.h> > #include <stdlib.h> > #include <unistd.h> > > -int > -initgroups(const char *uname, gid_t agroup) > +/* For freebsd14_setgroups(). */ > +#include "gen-compat.h" > + > +static int > +initgroups_impl(const char *uname, gid_t agroup, > + int (*setgroups)(int, const gid_t *)) > { > - int ngroups, ret; > - long ngroups_max; > gid_t *groups; > + long ngroups_max; > + int ngroups, ret; > > /* > - * Provide space for one group more than possible to allow > - * setgroups to fail and set errno. > + * Provide space for one group more than possible to allow setgroups() > + * to fail and set 'errno' in case we get back more than {NGROUPS_MAX} + > + * 1 groups. > */ > ngroups_max = sysconf(_SC_NGROUPS_MAX) + 2; > groups = malloc(sizeof(*groups) * ngroups_max); > @@ -52,8 +63,23 @@ initgroups(const char *uname, gid_t agroup) > return (-1); /* malloc() set 'errno'. */ > > ngroups = (int)ngroups_max; > - getgrouplist(uname, agroup, groups, &ngroups); > - ret = setgroups(ngroups, groups); > + (void)getgrouplist(uname, agroup, groups, &ngroups); > + ret = (*setgroups)(ngroups, groups); > + > free(groups); > return (ret); /* setgroups() set 'errno'. */ > } > + > +int > +initgroups(const char *uname, gid_t agroup) > +{ > + return (initgroups_impl(uname, agroup, setgroups)); > +} > + > +int > +freebsd14_initgroups(const char *uname, gid_t agroup) > +{ > + return (initgroups_impl(uname, agroup, freebsd14_setgroups)); > +} > + > +__sym_compat(initgroups, freebsd14_initgroups, FBSD_1.0);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8f835667-5d3c-4972-a6ee-0a2d4f20269e>