kern: fix setgroups(2) and getgroups(2) to match other platforms

On most other platforms observed, including OpenBSD, NetBSD, and Linux,
these system calls have long since been converted to only touching the
supplementary groups of the process.  This poses both portability and
security concerns in porting software to and from FreeBSD, as this
subtle difference is a landmine waiting to happen.  Bugs have been
discovered even in FreeBSD-local sources, since this behavior is
somewhat unintuitive (see, e.g., fix 48fd05999b for chroot(8)).

Now that the egid is tracked outside of cr_groups in our ucred, convert
the syscalls to deal with only supplementary groups.  Some remaining
stragglers in base that had baked in assumptions about these syscalls
are fixed in the process to avoid heartburn in conversion.

For relnotes: application developers should audit their use of both
setgroups(2) and getgroups(2) for signs that they had assumed the
previous FreeBSD behavior of using the first element for the egid.  Any
calls to setgroups() to clear groups that used a single array of the
now or soon-to-be egid can be converted to setgroups(0, NULL) calls to
clear the supplementary groups entirely on all FreeBSD versions.

Co-authored-by:	olce (but bugs are likely mine)
Relnotes:	yes (see last paragraph)
Reviewed by:	kib
Differential Revision:	https://reviews.freebsd.org/D51648

lib/libc/include/compat.h

@@ -69,6 +69,9 @@ __sym_compat(kevent, freebsd11_kevent, FBSD_1.0);
 
 __sym_compat(swapoff, freebsd13_swapoff, FBSD_1.0);
 
+__sym_compat(getgroups, freebsd14_getgroups, FBSD_1.0);
+__sym_compat(setgroups, freebsd14_setgroups, FBSD_1.0);
+
 #undef __sym_compat
 
 #define	__weak_reference(sym,alias)	\

lib/libsys/Symbol.sys.map

@@ -89,7 +89,6 @@ FBSD_1.0 {
 	geteuid;
 	getfh;
 	getgid;
-	getgroups;
 	getitimer;
 	getpagesize;
 	getpeername;
@@ -204,7 +203,6 @@ FBSD_1.0 {
 	setegid;
 	seteuid;
 	setgid;
-	setgroups;
 	setitimer;
 	setlogin;
 	setpgid;
@@ -380,11 +378,13 @@ FBSD_1.7 {
 FBSD_1.8 {
 	exterrctl;
 	fchroot;
+	getgroups;
 	getrlimitusage;
 	inotify_add_watch_at;
 	inotify_rm_watch;
 	kcmp;
 	setcred;
+	setgroups;
 };
 
 FBSDprivate_1.0 {

lib/libsys/getgroups.2

@@ -25,7 +25,7 @@
 .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 .\" SUCH DAMAGE.
 .\"
-.Dd January 21, 2011
+.Dd August 1, 2025
 .Dt GETGROUPS 2
 .Os
 .Sh NAME
@@ -41,8 +41,8 @@
 The
 .Fn getgroups
 system call
-gets the current group access list of the user process
-and stores it in the array
+gets the current supplementary groups of the user process and stores it in the
+array
 .Fa gidset .
 The
 .Fa gidsetlen
@@ -54,7 +54,7 @@ The
 system call
 returns the actual number of groups returned in
 .Fa gidset .
-At least one and as many as {NGROUPS_MAX}+1 values may be returned.
+As many as {NGROUPS_MAX} values may be returned.
 If
 .Fa gidsetlen
 is zero,
@@ -102,3 +102,10 @@ The
 .Fn getgroups
 system call appeared in
 .Bx 4.2 .
+.Pp
+Before
+.Fx 15.0 ,
+the
+.Fn getgroups
+system call always returned the effective group ID for the process as the first
+element of the array, before the supplementary groups.

lib/libsys/setgroups.2

@@ -25,7 +25,7 @@
 .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 .\" SUCH DAMAGE.
 .\"
-.Dd January 19, 2018
+.Dd August 1, 2025
 .Dt SETGROUPS 2
 .Os
 .Sh NAME
@@ -42,7 +42,7 @@
 The
 .Fn setgroups
 system call
-sets the group access list of the current user process
+sets the supplementary group list of the current user process
 according to the array
 .Fa gidset .
 The
@@ -50,26 +50,12 @@ The
 argument
 indicates the number of entries in the array and must be no
 more than
-.Dv {NGROUPS_MAX}+1 .
+.Dv {NGROUPS_MAX} .
+The
+.Fa ngroups
+argument may be set to 0 to clear the supplementary group list.
 .Pp
-Only the super-user may set a new group list.
-.Pp
-The first entry of the group array
-.Pq Va gidset[0]
-is used as the effective group-ID for the process.
-This entry is over-written when a setgid program is run.
-To avoid losing access to the privileges of the
-.Va gidset[0]
-entry, it should be duplicated later in the group array.
-By convention,
-this happens because the group value indicated
-in the password file also appears in
-.Pa /etc/group .
-The group value in the password file is placed in
-.Va gidset[0]
-and that value then gets added a second time when the
-.Pa /etc/group
-file is scanned to create the group set.
+Only the super-user may set a new supplementary group list.
 .Sh RETURN VALUES
 .Rv -std setgroups
 .Sh ERRORS
@@ -99,3 +85,11 @@ The
 .Fn setgroups
 system call appeared in
 .Bx 4.2 .
+.Pp
+Before
+.Fx 15.0 ,
+the
+.Fn setgroups
+system call would set the effective group ID for the process to the first
+element of
+.Fa gidset .

sbin/hastd/subr.c

@@ -207,10 +207,8 @@ drop_privs(const struct hast_resource *res)
 		}
 	}
 	PJDLOG_VERIFY(chdir("/") == 0);
-	gidset[0] = pw->pw_gid;
-	if (setgroups(1, gidset) == -1) {
-		pjdlog_errno(LOG_ERR, "Unable to set groups to gid %u",
-		    (unsigned int)pw->pw_gid);
+	if (setgroups(0, NULL) == -1) {
+		pjdlog_errno(LOG_ERR, "Unable to drop supplementary groups");
 		return (-1);
 	}
 	if (setgid(pw->pw_gid) == -1) {
@@ -287,8 +285,7 @@ drop_privs(const struct hast_resource *res)
 	PJDLOG_VERIFY(egid == pw->pw_gid);
 	PJDLOG_VERIFY(sgid == pw->pw_gid);
 	PJDLOG_VERIFY(getgroups(0, NULL) == 1);
-	PJDLOG_VERIFY(getgroups(1, gidset) == 1);
-	PJDLOG_VERIFY(gidset[0] == pw->pw_gid);
+	PJDLOG_VERIFY(getgroups(1, gidset) == 0);
 
 	pjdlog_debug(1,
 	    "Privileges successfully dropped using %s%s+setgid+setuid.",

sys/kern/kern_prot.c

@@ -310,6 +310,39 @@ sys_getegid(struct thread *td, struct getegid_args *uap)
 	return (0);
 }
 
+#ifdef COMPAT_FREEBSD14
+int
+freebsd14_getgroups(struct thread *td, struct freebsd14_getgroups_args *uap)
+{
+	struct ucred *cred;
+	int ngrp, error;
+
+	cred = td->td_ucred;
+
+	/*
+	 * For FreeBSD < 15.0, we account for the egid being placed at the
+	 * beginning of the group list prior to all supplementary groups.
+	 */
+	ngrp = cred->cr_ngroups + 1;
+	if (uap->gidsetsize == 0) {
+		error = 0;
+		goto out;
+	} else if (uap->gidsetsize < ngrp) {
+		return (EINVAL);
+	}
+
+	error = copyout(&cred->cr_gid, uap->gidset, sizeof(gid_t));
+	if (error != 0)
+		error = copyout(cred->cr_groups, uap->gidset + 1,
+		    (ngrp - 1) * sizeof(gid_t));
+
+out:
+	td->td_retval[0] = ngrp;
+	return (error);
+
+}
+#endif	/* COMPAT_FREEBSD14 */
+
 #ifndef _SYS_SYSPROTO_H_
 struct getgroups_args {
 	int	gidsetsize;
@@ -320,18 +353,11 @@ int
 sys_getgroups(struct thread *td, struct getgroups_args *uap)
 {
 	struct ucred *cred;
-	gid_t *ugidset;
 	int ngrp, error;
 
 	cred = td->td_ucred;
 
-	/*
-	 * cr_gid has been moved out of cr_groups, but we'll continue exporting
-	 * the egid as groups[0] for the time being until we audit userland for
-	 * any surprises.
-	 */
-	ngrp = cred->cr_ngroups + 1;
-
+	ngrp = cred->cr_ngroups;
 	if (uap->gidsetsize == 0) {
 		error = 0;
 		goto out;
@@ -339,14 +365,7 @@ sys_getgroups(struct thread *td, struct getgroups_args *uap)
 	if (uap->gidsetsize < ngrp)
 		return (EINVAL);
 
-	ugidset = uap->gidset;
-	error = copyout(&cred->cr_gid, ugidset, sizeof(*ugidset));
-	if (error != 0)
-		goto out;
-
-	if (ngrp > 1)
-		error = copyout(cred->cr_groups, ugidset + 1,
-		    (ngrp - 1) * sizeof(*ugidset));
+	error = copyout(cred->cr_groups, uap->gidset, ngrp * sizeof(gid_t));
 out:
 	td->td_retval[0] = ngrp;
 	return (error);
@@ -1186,6 +1205,44 @@ fail:
 	return (error);
 }
 
+#ifdef COMPAT_FREEBSD14
+int
+freebsd14_setgroups(struct thread *td, struct freebsd14_setgroups_args *uap)
+{
+	gid_t smallgroups[CRED_SMALLGROUPS_NB];
+	gid_t *groups;
+	int gidsetsize, error;
+
+	/*
+	 * Before FreeBSD 15.0, we allow one more group to be supplied to
+	 * account for the egid appearing before the supplementary groups.  This
+	 * may technically allow one more supplementary group for systems that
+	 * did use the default NGROUPS_MAX if we round it back up to 1024.
+	 */
+	gidsetsize = uap->gidsetsize;
+	if (gidsetsize > ngroups_max + 1 || gidsetsize < 0)
+		return (EINVAL);
+
+	if (gidsetsize > CRED_SMALLGROUPS_NB)
+		groups = malloc(gidsetsize * sizeof(gid_t), M_TEMP, M_WAITOK);
+	else
+		groups = smallgroups;
+
+	error = copyin(uap->gidset, groups, gidsetsize * sizeof(gid_t));
+	if (error == 0) {
+		int ngroups = gidsetsize > 0 ? gidsetsize - 1 /* egid */ : 0;
+
+		error = kern_setgroups(td, &ngroups, groups + 1);
+		if (error == 0 && gidsetsize > 0)
+			td->td_proc->p_ucred->cr_gid = groups[0];
+	}
+
+	if (groups != smallgroups)
+		free(groups, M_TEMP);
+	return (error);
+}
+#endif	/* COMPAT_FREEBSD14 */
+
 #ifndef _SYS_SYSPROTO_H_
 struct setgroups_args {
 	int	gidsetsize;
@@ -1210,8 +1267,7 @@ sys_setgroups(struct thread *td, struct setgroups_args *uap)
 	 * setgroups() differ.
 	 */
 	gidsetsize = uap->gidsetsize;
-	/* XXXKE Limit to ngroups_max when we change the userland interface. */
-	if (gidsetsize > ngroups_max + 1 || gidsetsize < 0)
+	if (gidsetsize > ngroups_max || gidsetsize < 0)
 		return (EINVAL);
 
 	if (gidsetsize > CRED_SMALLGROUPS_NB)
@@ -1238,35 +1294,17 @@ kern_setgroups(struct thread *td, int *ngrpp, gid_t *groups)
 	struct proc *p = td->td_proc;
 	struct ucred *newcred, *oldcred;
 	int ngrp, error;
-	gid_t egid;
 
 	ngrp = *ngrpp;
 	/* Sanity check size. */
-	/* XXXKE Limit to ngroups_max when we change the userland interface. */
-	if (ngrp < 0 || ngrp > ngroups_max + 1)
+	if (ngrp < 0 || ngrp > ngroups_max)
 		return (EINVAL);
 
 	AUDIT_ARG_GROUPSET(groups, ngrp);
-	/*
-	 * setgroups(0, NULL) is a legitimate way of clearing the groups vector
-	 * on non-BSD systems (which generally do not have the egid in the
-	 * groups[0]).  We risk security holes when running non-BSD software if
-	 * we do not do the same.  So we allow and treat 0 for 'ngrp' specially
-	 * below (twice).
-	 */
-	if (ngrp != 0) {
-		/*
-		 * To maintain userland compat for now, we use the first group
-		 * as our egid and we'll use the rest as our supplemental
-		 * groups.
-		 */
-		egid = groups[0];
-		ngrp--;
-		groups++;
 
-		groups_normalize(&ngrp, groups);
-		*ngrpp = ngrp;
-	}
+	groups_normalize(&ngrp, groups);
+	*ngrpp = ngrp;
+
 	newcred = crget();
 	crextend(newcred, ngrp);
 	PROC_LOCK(p);
@@ -1289,15 +1327,7 @@ kern_setgroups(struct thread *td, int *ngrpp, gid_t *groups)
 	if (error)
 		goto fail;
 
-	/*
-	 * If some groups were passed, the first one is currently the desired
-	 * egid.  This code is to be removed (along with some commented block
-	 * above) when setgroups() is changed to take only supplementary groups.
-	 */
-	if (ngrp != 0)
-		newcred->cr_gid = egid;
 	crsetgroups_internal(newcred, ngrp, groups);
-
 	setsugid(p);
 	proc_set_cred(p, newcred);
 	PROC_UNLOCK(p);

sys/kern/syscalls.master

@@ -552,13 +552,13 @@
 		    _Out_writes_bytes_(len/PAGE_SIZE) char *vec
 		);
 	}
-79	AUE_GETGROUPS	STD|CAPENABLED {
+79	AUE_GETGROUPS	STD|CAPENABLED|COMPAT14 {
 		int getgroups(
 		    int gidsetsize,
 		    _Out_writes_opt_(gidsetsize) gid_t *gidset
 		);
 	}
-80	AUE_SETGROUPS	STD {
+80	AUE_SETGROUPS	STD|COMPAT14 {
 		int setgroups(
 		    int gidsetsize,
 		    _In_reads_(gidsetsize) const gid_t *gidset
@@ -3371,5 +3371,17 @@
 		    int wd
 		);
 	}
+595	AUE_GETGROUPS	STD|CAPENABLED {
+		int getgroups(
+		    int gidsetsize,
+		    _Out_writes_opt_(gidsetsize) gid_t *gidset
+		);
+	}
+596	AUE_SETGROUPS	STD {
+		int setgroups(
+		    int gidsetsize,
+		    _In_reads_(gidsetsize) const gid_t *gidset
+		);
+	}
 
 ; vim: syntax=off

usr.bin/newgrp/newgrp.c

@@ -186,7 +186,7 @@ addgroup(const char *grpname)
 		}
 	}
 
-	ngrps_max = sysconf(_SC_NGROUPS_MAX) + 1;
+	ngrps_max = sysconf(_SC_NGROUPS_MAX);
 	if ((grps = malloc(sizeof(gid_t) * ngrps_max)) == NULL)
 		err(1, "malloc");
 	if ((ngrps = getgroups(ngrps_max, (gid_t *)grps)) < 0) {
@@ -194,7 +194,12 @@ addgroup(const char *grpname)
 		goto end;
 	}
 
-	/* Remove requested gid from supp. list if it exists. */
+	/*
+	 * Remove requested gid from supp. list if it exists and doesn't match
+	 * our prior egid -- this exception is to avoid providing the user a
+	 * means to get rid of a group that could be used for, e.g., negative
+	 * permissions.
+	 */
 	if (grp->gr_gid != egid && inarray(grp->gr_gid, grps, ngrps)) {
 		for (i = 0; i < ngrps; i++)
 			if (grps[i] == grp->gr_gid)
@@ -217,10 +222,9 @@ addgroup(const char *grpname)
 		goto end;
 	}
 	PRIV_END;
-	grps[0] = grp->gr_gid;
 
 	/* Add old effective gid to supp. list if it does not exist. */
-	if (egid != grp->gr_gid && !inarray(egid, grps, ngrps)) {
+	if (!inarray(egid, grps, ngrps)) {
 		if (ngrps == ngrps_max)
 			warnx("too many groups");
 		else {

usr.bin/quota/quota.c

@@ -100,8 +100,7 @@ static char	*filename = NULL;
 int
 main(int argc, char *argv[])
 {
-	int ngroups; 
-	gid_t mygid, gidset[NGROUPS];
+	int ngroups;
 	int i, ch, gflag = 0, uflag = 0, errflag = 0;
 
 	while ((ch = getopt(argc, argv, "f:ghlrquv")) != -1) {
@@ -142,11 +141,15 @@ main(int argc, char *argv[])
 		if (uflag)
 			errflag += showuid(getuid());
 		if (gflag) {
+			gid_t mygid, myegid, gidset[NGROUPS_MAX];
+
 			mygid = getgid();
-			ngroups = getgroups(NGROUPS, gidset);
+			errflag += showgid(mygid);
+			myegid = getegid();
+			errflag += showgid(myegid);
+			ngroups = getgroups(NGROUPS_MAX, gidset);
 			if (ngroups < 0)
 				err(1, "getgroups");
-			errflag += showgid(mygid);
 			for (i = 0; i < ngroups; i++)
 				if (gidset[i] != mygid)
 					errflag += showgid(gidset[i]);

usr.sbin/chroot/chroot.c

@@ -147,15 +147,10 @@ main(int argc, char *argv[])
 		gid = resolve_group(group);
 
 	if (grouplist != NULL) {
-		ngroups_max = sysconf(_SC_NGROUPS_MAX) + 1;
+		ngroups_max = sysconf(_SC_NGROUPS_MAX);
 		if ((gidlist = malloc(sizeof(gid_t) * ngroups_max)) == NULL)
 			err(1, "malloc");
-		/* Populate the egid slot in our groups to avoid accidents. */
-		if (gid == 0)
-			gidlist[0] = getegid();
-		else
-			gidlist[0] = gid;
-		for (gids = 1; (p = strsep(&grouplist, ",")) != NULL &&
+		for (gids = 0; (p = strsep(&grouplist, ",")) != NULL &&
 		    gids < ngroups_max; ) {
 			if (*p == '\0')
 				continue;

usr.sbin/lpr/lpc/lpc.c

@@ -358,6 +358,8 @@ ingroup(const char *grname)
 			err(1, "getgroups");
 	}
 	gid = gptr->gr_gid;
+	if (gid == getegid())
+		return(1);
 	for (i = 0; i < ngroups; i++)
 		if (gid == groups[i])
 			return(1);