The branch stable/14 has been updated by kevans:
URL:
https://cgit.FreeBSD.org/src/commit/?id=3eafc9f76fd54fbd222d587b11752290766a0af5
commit 3eafc9f76fd54fbd222d587b11752290766a0af5
Author: Kyle Evans <kev...@freebsd.org>
AuthorDate: 2025-07-26 06:11:58 +0000
Commit: Kyle Evans <kev...@freebsd.org>
CommitDate: 2025-08-15 05:04:12 +0000
chroot: don't clobber the egid with the first supplemental group
There are two problems here, really:
1.) If -G is specified, the egid of the runner will get clobbered by
the first supplemental group
2.) If both -G and -g are specified, the first supplemental group will
get clobbered by the -g group
Ideally our users shouldn't have to understand the quirks of our
setgroups(2) and the manpage doesn't describe the group list as needing
to contain the egid, so populate the egid slot as necessary.
I note that this code seems to have already been marginally aware of the
historical behavior because it was allocating NGROUPS_MAX + 1, but this
is an artifact of a later conversion to doing dynamic allocations
instead of pushing NGROUPS_MAX arrays on the stack -- the original code
did in-fact only have an NGROUPS_MAX-sized array, and the layout was
still incorrect.
Reviewed by: olce
(cherry picked from commit 48fd05999b0f8e822fbf7069779378d103a35f5c)
(cherry picked from commit babab49eee9472f628d774996de13d13d296c8c0)
---
usr.sbin/chroot/chroot.c | 57 ++++++++++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 21 deletions(-)
diff --git a/usr.sbin/chroot/chroot.c b/usr.sbin/chroot/chroot.c
index 9fc918e38eac..b73ac65f5cad 100644
--- a/usr.sbin/chroot/chroot.c
+++ b/usr.sbin/chroot/chroot.c
@@ -73,7 +73,9 @@ main(int argc, char *argv[])
gid = 0;
uid = 0;
+ gids = 0;
user = group = grouplist = NULL;
+ gidlist = NULL;
nonprivileged = false;
while ((ch = getopt(argc, argv, "G:g:u:n")) != -1) {
switch(ch) {
@@ -89,6 +91,11 @@ main(int argc, char *argv[])
break;
case 'G':
grouplist = optarg;
+
+ /*
+ * XXX Why not allow us to drop all of our supplementary
+ * groups?
+ */
if (*grouplist == '\0')
usage();
break;
@@ -120,29 +127,37 @@ main(int argc, char *argv[])
}
}
- ngroups_max = sysconf(_SC_NGROUPS_MAX) + 1;
- if ((gidlist = malloc(sizeof(gid_t) * ngroups_max)) == NULL)
- err(1, "malloc");
- for (gids = 0;
- (p = strsep(&grouplist, ",")) != NULL && gids < ngroups_max; ) {
- if (*p == '\0')
- continue;
-
- if (isdigit((unsigned char)*p)) {
- gidlist[gids] = (gid_t)strtoul(p, &endp, 0);
- if (*endp != '\0')
- goto getglist;
- } else {
+ if (grouplist != NULL) {
+ ngroups_max = sysconf(_SC_NGROUPS_MAX) + 1;
+ 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 &&
+ gids < ngroups_max; ) {
+ if (*p == '\0')
+ continue;
+
+ if (isdigit((unsigned char)*p)) {
+ gidlist[gids] = (gid_t)strtoul(p, &endp, 0);
+ if (*endp != '\0')
+ goto getglist;
+ } else {
getglist:
- if ((gp = getgrnam(p)) != NULL)
- gidlist[gids] = gp->gr_gid;
- else
- errx(1, "no such group `%s'", p);
+ if ((gp = getgrnam(p)) != NULL)
+ gidlist[gids] = gp->gr_gid;
+ else
+ errx(1, "no such group `%s'", p);
+ }
+ gids++;
}
- gids++;
+
+ if (p != NULL && gids == ngroups_max)
+ errx(1, "too many supplementary groups provided");
}
- if (p != NULL && gids == ngroups_max)
- errx(1, "too many supplementary groups provided");
if (user != NULL) {
if (isdigit((unsigned char)*user)) {
@@ -168,7 +183,7 @@ main(int argc, char *argv[])
if (chdir(argv[0]) == -1 || chroot(".") == -1)
err(1, "%s", argv[0]);
- if (gids && setgroups(gids, gidlist) == -1)
+ if (gidlist != NULL && setgroups(gids, gidlist) == -1)
err(1, "setgroups");
if (group && setgid(gid) == -1)
err(1, "setgid");
