[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: two new chroot bugs
From: |
Jim Meyering |
Subject: |
Re: two new chroot bugs |
Date: |
Fri, 29 May 2009 17:34:35 +0200 |
Jim Meyering wrote:
> Jim Meyering wrote:
> ...
>> Here are patches, for review.
>> I'll add tests tomorrow.
>
> That latter log wasn't complete.
>
>> Subject: [PATCH] chroot: don't set bogus user-ID or group-ID for --u=U or
>> --u=:G
>>
>> * src/chroot.c (main): Initialize both "uid" and "gid". To -1.
>> This also allows one to set the primary group-ID to 0, in case
>> it's not that already.
>
> Here's what I have, now:
>
> chroot: don't set bogus user-ID or group-ID for --u=U: or --u=:G
>
> * src/chroot.c (main): Initialize both "uid" and "gid". To -1.
> This also allows one to set the user-ID or primary group-ID to 0,
> in case it's not that already.
I've added a test to exercise --u=U and --u=:G,
but didn't find a way to provoke setgid or setuid failure.
I was unable to exercise the --groups=G1,G2,G3 code when
not also specifying --user=... so I've fixed that, too.
Also, an empty group list was not diagnosed, and only the
first invalid group name was diagnosed. Now all are.
>From cfe8e7a0001a10d4deceb6065134fe8c402d0368 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Thu, 28 May 2009 22:36:05 +0200
Subject: [PATCH 1/4] tests: use "nobody" as the default group name in chroot
test
* tests/test-lib.sh (NON_ROOT_GROUP): Use "nobody", not "nogroup".
---
tests/test-lib.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index a765bd6..e5eed8d 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -204,7 +204,7 @@ require_root_()
{
uid_is_privileged_ || skip_test_ "must be run as root"
NON_ROOT_USERNAME=${NON_ROOT_USERNAME=nobody}
- NON_ROOT_GROUP=${NON_ROOT_GROUP=nogroup}
+ NON_ROOT_GROUP=${NON_ROOT_GROUP=nobody}
}
skip_if_root_() { uid_is_privileged_ && skip_test_ "must be run as non-root"; }
--
1.6.3.1.279.gd4bf4
>From 9e17c951e9e55c2769e99412110d9f0b2c0a835b Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 27 May 2009 22:06:04 +0200
Subject: [PATCH 2/4] chroot: set-*-ID failure must provoke nonzero exit before
execvp
* src/chroot.c (main): Exit upon set-group-ID or set-user-ID failure.
---
src/chroot.c | 21 ++++++++++++++++++---
1 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/chroot.c b/src/chroot.c
index 788a1fc..dccddd7 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -207,6 +207,7 @@ main (int argc, char **argv)
char *user;
char *group;
char const *err = parse_user_spec (userspec, &uid, &gid, &user, &group);
+ bool fail = false;
if (err)
error (EXIT_FAILURE, errno, "%s", err);
@@ -214,14 +215,28 @@ main (int argc, char **argv)
free (user);
free (group);
+ /* Attempt to set all three: supplementary groups, group ID, user ID.
+ Diagnose any failures. If any have failed, exit before execvp. */
if (groups && set_additional_groups (groups))
- error (0, errno, _("failed to set additional groups"));
+ {
+ error (0, errno, _("failed to set additional groups"));
+ fail = true;
+ }
if (gid && setgid (gid))
- error (0, errno, _("failed to set group-ID"));
+ {
+ error (0, errno, _("failed to set group-ID"));
+ fail = true;
+ }
if (uid && setuid (uid))
- error (0, errno, _("failed to set user-ID"));
+ {
+ error (0, errno, _("failed to set user-ID"));
+ fail = true;
+ }
+
+ if (fail)
+ exit (EXIT_FAILURE);
}
/* Execute the given command. */
--
1.6.3.1.279.gd4bf4
>From 18baffc2b5c6bbe538ee92c658697c026c697786 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 27 May 2009 23:06:15 +0200
Subject: [PATCH 3/4] chroot: don't set bogus user-ID or group-ID for --u=U: or
--u=:G
* src/chroot.c (main): Initialize both "uid" and "gid". To -1.
This also allows one to set the user-ID or primary group-ID to 0,
in case it's not that already.
* tests/chroot/credentials: Test for the above.
---
src/chroot.c | 8 ++++----
tests/chroot/credentials | 9 +++++++++
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/src/chroot.c b/src/chroot.c
index dccddd7..39b3acf 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -202,8 +202,8 @@ main (int argc, char **argv)
if (userspec)
{
- uid_t uid;
- gid_t gid;
+ uid_t uid = -1;
+ gid_t gid = -1;
char *user;
char *group;
char const *err = parse_user_spec (userspec, &uid, &gid, &user, &group);
@@ -223,13 +223,13 @@ main (int argc, char **argv)
fail = true;
}
- if (gid && setgid (gid))
+ if (gid != (gid_t) -1 && setgid (gid))
{
error (0, errno, _("failed to set group-ID"));
fail = true;
}
- if (uid && setuid (uid))
+ if (uid != (uid_t) -1 && setuid (uid))
{
error (0, errno, _("failed to set user-ID"));
fail = true;
diff --git a/tests/chroot/credentials b/tests/chroot/credentials
index 23d66bd..b76edea 100755
--- a/tests/chroot/credentials
+++ b/tests/chroot/credentials
@@ -40,4 +40,13 @@ test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP
/ whoami)" != root
test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP --groups= / id
-nG)"\
= $NON_ROOT_GROUP || fail=1
+# Verify that when specifying only the user name we get the current
+# primary group ID.
+test "$(chroot --userspec=$NON_ROOT_USERNAME / id -g)" = "$(id -g)" \
+ || fail=1
+
+# Verify that when specifying only a group we get the current user ID
+test "$(chroot --userspec=:$NON_ROOT_GROUP / id -u)" = "$(id -u)" \
+ || fail=1
+
Exit $fail
--
1.6.3.1.279.gd4bf4
>From ba0d82933a34f0a076c3503073b022d1365c8601 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 29 May 2009 08:44:56 +0200
Subject: [PATCH 4/4] chroot: make --groups= work without --userspec=; be more
robust
* src/chroot.c (set_additional_groups): Add comments.
Given an empty or all-comma group list, diagnose it and return nonzero.
When more than one group is invalid, diagnose all of them,
not just the first.
(main): Honor --groups= also when --userspec= is not specified.
Now that set_additional_groups consistently diagnoses its failures,
don't diagnose it separately here.
* tests/chroot/credentials: Do not invoke with an empty group list.
---
src/chroot.c | 61 +++++++++++++++++++++++++++++----------------
tests/chroot/credentials | 2 +-
2 files changed, 40 insertions(+), 23 deletions(-)
diff --git a/src/chroot.c b/src/chroot.c
index 39b3acf..12d282b 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -54,7 +54,11 @@ static struct option const long_opts[] =
{NULL, 0, NULL, 0}
};
-/* Groups is a comma separated list of additional groups. */
+/* Call setgroups to set the supplementary groups to those listed in GROUPS.
+ GROUPS is a comma separated list of supplementary groups (names or numbers).
+ Parse that list, converting any names to numbers, and call setgroups on the
+ resulting numbers. Upon any failure give a diagnostic and return nonzero.
+ Otherwise return zero. */
static int
set_additional_groups (char const *groups)
{
@@ -63,7 +67,7 @@ set_additional_groups (char const *groups)
size_t n_gids = 0;
char *buffer = xstrdup (groups);
char const *tmp;
- int ret;
+ int ret = 0;
for (tmp = strtok (buffer, ","); tmp; tmp = strtok (NULL, ","))
{
@@ -71,9 +75,7 @@ set_additional_groups (char const *groups)
unsigned long int value;
if (xstrtoul (tmp, NULL, 10, &value, "") == LONGINT_OK && value <=
MAXGID)
- {
- g = getgrgid (value);
- }
+ g = getgrgid (value);
else
{
g = getgrnam (tmp);
@@ -83,9 +85,9 @@ set_additional_groups (char const *groups)
if (g == NULL)
{
- error (0, errno, _("cannot find group %s"), tmp);
- free (buffer);
- return 1;
+ error (0, errno, _("invalid group %s"), quote (tmp));
+ ret = -1;
+ continue;
}
if (n_gids == n_gids_allocated)
@@ -93,16 +95,24 @@ set_additional_groups (char const *groups)
gids[n_gids++] = value;
}
- free (buffer);
+ if (ret == 0 && n_gids == 0)
+ {
+ error (0, 0, _("invalid group list %s"), quote (groups));
+ ret = -1;
+ }
- ret = setgroups (n_gids, gids);
+ if (ret == 0)
+ {
+ ret = setgroups (n_gids, gids);
+ if (ret)
+ error (0, errno, _("failed to set additional groups"));
+ }
+ free (buffer);
free (gids);
-
return ret;
}
-
void
usage (int status)
{
@@ -200,6 +210,10 @@ main (int argc, char **argv)
argv += optind + 1;
}
+ bool fail = false;
+
+ /* Attempt to set all three: supplementary groups, group ID, user ID.
+ Diagnose any failures. If any have failed, exit before execvp. */
if (userspec)
{
uid_t uid = -1;
@@ -207,7 +221,6 @@ main (int argc, char **argv)
char *user;
char *group;
char const *err = parse_user_spec (userspec, &uid, &gid, &user, &group);
- bool fail = false;
if (err)
error (EXIT_FAILURE, errno, "%s", err);
@@ -215,13 +228,8 @@ main (int argc, char **argv)
free (user);
free (group);
- /* Attempt to set all three: supplementary groups, group ID, user ID.
- Diagnose any failures. If any have failed, exit before execvp. */
if (groups && set_additional_groups (groups))
- {
- error (0, errno, _("failed to set additional groups"));
- fail = true;
- }
+ fail = true;
if (gid != (gid_t) -1 && setgid (gid))
{
@@ -234,10 +242,19 @@ main (int argc, char **argv)
error (0, errno, _("failed to set user-ID"));
fail = true;
}
-
- if (fail)
- exit (EXIT_FAILURE);
}
+ else
+ {
+ /* Yes, this call is identical to the one above.
+ However, when --userspec and --groups groups are used together,
+ we don't want to call this function until after parsing USER:GROUP,
+ and it must be called before setuid. */
+ if (groups && set_additional_groups (groups))
+ fail = true;
+ }
+
+ if (fail)
+ exit (EXIT_FAILURE);
/* Execute the given command. */
execvp (argv[0], argv);
diff --git a/tests/chroot/credentials b/tests/chroot/credentials
index b76edea..58c098f 100755
--- a/tests/chroot/credentials
+++ b/tests/chroot/credentials
@@ -37,7 +37,7 @@ test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP
/ whoami)" != root
|| fail=1
# Verify that there are no additional groups.
-test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP --groups= / id
-nG)"\
+test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP
--groups=$NON_ROOT_GROUP / id -nG)"\
= $NON_ROOT_GROUP || fail=1
# Verify that when specifying only the user name we get the current
--
1.6.3.1.279.gd4bf4
- Re: [PATCH] chroot specify user/group feature, (continued)
- Re: [PATCH] chroot specify user/group feature, Jim Meyering, 2009/05/25
- Re: [PATCH] chroot specify user/group feature, Jim Meyering, 2009/05/26
- Re: [PATCH] chroot specify user/group feature, Eric Blake, 2009/05/27
- Re: [PATCH] chroot specify user/group feature, Giuseppe Scrivano, 2009/05/27
- Re: [PATCH] chroot specify user/group feature, Jim Meyering, 2009/05/27
- Re: [PATCH] chroot specify user/group feature, Giuseppe Scrivano, 2009/05/28
- Re: [PATCH] chroot specify user/group feature, Eric Blake, 2009/05/28
- Re: [PATCH] chroot specify user/group feature, Giuseppe Scrivano, 2009/05/28
- two new chroot bugs, Jim Meyering, 2009/05/27
- Re: two new chroot bugs, Jim Meyering, 2009/05/27
- Re: two new chroot bugs,
Jim Meyering <=
- Re: [PATCH] chroot specify user/group feature, Pádraig Brady, 2009/05/20
- Re: [PATCH] chroot specify user/group feature, Jim Meyering, 2009/05/20
- Re: [PATCH] chroot specify user/group feature, Giuseppe Scrivano, 2009/05/20