The branch main has been updated by kevans:
URL:
https://cgit.FreeBSD.org/src/commit/?id=9da2fe96ff2ea227e4d5f03ef92b55aabeabb7fc
commit 9da2fe96ff2ea227e4d5f03ef92b55aabeabb7fc
Author: Kyle Evans <kev...@freebsd.org>
AuthorDate: 2025-08-15 04:06:09 +0000
Commit: Kyle Evans <kev...@freebsd.org>
CommitDate: 2025-08-15 04:06:09 +0000
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 48fd05999b0f 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 | 3 ++
lib/libsys/Symbol.sys.map | 4 +-
lib/libsys/getgroups.2 | 15 ++++--
lib/libsys/setgroups.2 | 36 ++++++-------
sbin/hastd/subr.c | 9 ++--
sys/kern/kern_prot.c | 126 ++++++++++++++++++++++++++++------------------
sys/kern/syscalls.master | 16 +++++-
usr.bin/newgrp/newgrp.c | 12 +++--
usr.bin/quota/quota.c | 11 ++--
usr.sbin/chroot/chroot.c | 9 +---
usr.sbin/lpr/lpc/lpc.c | 2 +
11 files changed, 145 insertions(+), 98 deletions(-)
diff --git a/lib/libc/include/compat.h b/lib/libc/include/compat.h
index 70fb8dcd97f3..97f22607ddd7 100644
--- a/lib/libc/include/compat.h
+++ b/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) \
diff --git a/lib/libsys/Symbol.sys.map b/lib/libsys/Symbol.sys.map
index 45e0160100af..1a297f9df581 100644
--- a/lib/libsys/Symbol.sys.map
+++ b/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 {
diff --git a/lib/libsys/getgroups.2 b/lib/libsys/getgroups.2
index 91cca2748ec2..37c8fbad7215 100644
--- a/lib/libsys/getgroups.2
+++ b/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.
diff --git a/lib/libsys/setgroups.2 b/lib/libsys/setgroups.2
index a226aeafea96..451f63ba1266 100644
--- a/lib/libsys/setgroups.2
+++ b/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 .
-.Pp
-Only the super-user may set a new group list.
+.Dv {NGROUPS_MAX} .
+The
+.Fa ngroups
+argument may be set to 0 to clear the supplementary 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 .
diff --git a/sbin/hastd/subr.c b/sbin/hastd/subr.c
index 2a26482b3727..284fb0d07647 100644
--- a/sbin/hastd/subr.c
+++ b/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.",
diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 2cd5b7069023..beab30a9d157 100644
--- a/sys/kern/kern_prot.c
+++ b/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);
diff --git a/sys/kern/syscalls.master b/sys/kern/syscalls.master
index 53b5d3cbbba9..fa64597d14a5 100644
--- a/sys/kern/syscalls.master
+++ b/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
diff --git a/usr.bin/newgrp/newgrp.c b/usr.bin/newgrp/newgrp.c
index f1da1c8cb1f5..0971f4d13b49 100644
--- a/usr.bin/newgrp/newgrp.c
+++ b/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 {
diff --git a/usr.bin/quota/quota.c b/usr.bin/quota/quota.c
index b5d28fd7c184..9ad4076cec40 100644
--- a/usr.bin/quota/quota.c
+++ b/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]);
diff --git a/usr.sbin/chroot/chroot.c b/usr.sbin/chroot/chroot.c
index 7ec5a00b50f0..e1af0a4131d3 100644
--- a/usr.sbin/chroot/chroot.c
+++ b/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;
diff --git a/usr.sbin/lpr/lpc/lpc.c b/usr.sbin/lpr/lpc/lpc.c
index a3da852de46e..b4db5bb2e29f 100644
--- a/usr.sbin/lpr/lpc/lpc.c
+++ b/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);
