[RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Eric W. Biederman]

I have just merged a security fix into the linux kernel that corrects an
oversight in the permission checks of /proc/self/gid_map.

The root of the issue is that unix allows anyone to specify permissions
such like: --rwx---rwx on a file, and setgroups call at login time
allows seting groups that even setgid exectuables don't drop.  Which
results in the ability to assign a process fewer privileges just because
it is in a specified group, and this makes dropping groups an unsafe
operation.

Therefore unprivileged writing of /proc/self/gid_map has been disabled
unless /proc/self/setgroups is written first to permanently disable the
ability to call setgroups in that user namespace.

In part this design was chosen so that applications that are affected
will break early instead of late, and in part to make it clear to
everyone what is going on.

I think for the experimental tool that is unshare --make-root-user we
just want to flip the bit and be done with it (patch below).

However we may want to require an additional option to clear setgroups,
if there loging type applications running that call setgroups and having
explicit breakage up front instead of more silent stealthy breakage
when the application runs is desired.

If we don't want any extra options working tested code is below.

Eric

From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Thu, 11 Dec 2014 20:05:25 -0600
Subject: [PATCH] unshare: Fix --map-root-user to work on new kernels

In rare cases droping groups with setgroups(0, NULL) is an operation
that can grant a user additional privileges.  User namespaces were
allwoing that operation to unprivileged users and that had to be
fixed.

Update unshare --map-root-user to disable the setgroups operation
before setting the gid_map.

This is needed as after the security fix gid_map is restricted to
privileged users unless setgroups has been disabled.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/pathnames.h |  1 +
 sys-utils/unshare.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/pathnames.h b/include/pathnames.h
index 1cc4e15e6e4f..1c53e4554268 100644
--- a/include/pathnames.h
+++ b/include/pathnames.h
@@ -92,6 +92,7 @@
 
 #define _PATH_PROC_UIDMAP	"/proc/self/uid_map"
 #define _PATH_PROC_GIDMAP	"/proc/self/gid_map"
+#define _PATH_PROC_SETGROUPS	"/proc/self/setgroups"
 
 #define _PATH_PROC_ATTR_CURRENT	"/proc/self/attr/current"
 #define _PATH_PROC_ATTR_EXEC	"/proc/self/attr/exec"
diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 95e4afbd055e..d409a7c936b6 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -39,6 +39,24 @@
 #include "pathnames.h"
 #include "all-io.h"
 
+static void disable_setgroups(void)
+{
+	const char *file = _PATH_PROC_SETGROUPS;
+	const char *deny = "deny";
+	int fd;
+
+	fd = open(file, O_WRONLY);
+	if (fd < 0) {
+		if (errno == ENOENT)
+			return;
+		 err(EXIT_FAILURE, _("cannot open %s"), file);
+	}
+
+	if (write_all(fd, deny, strlen(deny)))
+		err(EXIT_FAILURE, _("write failed %s"), file);
+	close(fd);
+}
+
 static void map_id(const char *file, uint32_t from, uint32_t to)
 {
 	char *buf;
@@ -178,6 +196,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (maproot) {
+		disable_setgroups();
 		map_id(_PATH_PROC_UIDMAP, 0, real_euid);
 		map_id(_PATH_PROC_GIDMAP, 0, real_egid);
 	}
-- 
2.1.3

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> I have just merged a security fix into the linux kernel that corrects an
> oversight in the permission checks of /proc/self/gid_map.
>
> The root of the issue is that unix allows anyone to specify permissions
> such like: --rwx---rwx on a file, and setgroups call at login time
> allows seting groups that even setgid exectuables don't drop.  Which
> results in the ability to assign a process fewer privileges just because
> it is in a specified group, and this makes dropping groups an unsafe
> operation.
>
> Therefore unprivileged writing of /proc/self/gid_map has been disabled
> unless /proc/self/setgroups is written first to permanently disable the
> ability to call setgroups in that user namespace.
>
> In part this design was chosen so that applications that are affected
> will break early instead of late, and in part to make it clear to
> everyone what is going on.
>
> I think for the experimental tool that is unshare --make-root-user we
> just want to flip the bit and be done with it (patch below).
>
> However we may want to require an additional option to clear setgroups,
> if there loging type applications running that call setgroups and having
> explicit breakage up front instead of more silent stealthy breakage
> when the application runs is desired.
>
> If we don't want any extra options working tested code is below.

This may also have some affect on the setgroups(0, NULL) case of
nsenter as well.

Eric

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Karel Zak]

On Wed, Dec 17, 2014 at 05:21:31PM -0600, Eric W. Biederman wrote:
> ebiederm@xmission.com (Eric W. Biederman) writes:
> 
> > I have just merged a security fix into the linux kernel that corrects an
> > oversight in the permission checks of /proc/self/gid_map.
> >
> > The root of the issue is that unix allows anyone to specify permissions
> > such like: --rwx---rwx on a file, and setgroups call at login time
> > allows seting groups that even setgid exectuables don't drop.  Which
> > results in the ability to assign a process fewer privileges just because
> > it is in a specified group, and this makes dropping groups an unsafe
> > operation.
> >
> > Therefore unprivileged writing of /proc/self/gid_map has been disabled
> > unless /proc/self/setgroups is written first to permanently disable the
> > ability to call setgroups in that user namespace.

 What does it mean "allow" in /proc/self/setgroups? 
 
 If I good understand than /proc/self/gid_map is unwritable until the
 setgroups file is set to "deny", and "allow" means that gid_map is
 disabled at all, but setgroup() syscall is possible to use in the
 user namespace. Right?

> > In part this design was chosen so that applications that are affected
> > will break early instead of late, and in part to make it clear to
> > everyone what is going on.
> >
> > I think for the experimental tool that is unshare --make-root-user we
> > just want to flip the bit and be done with it (patch below).
> >
> > However we may want to require an additional option to clear setgroups,
> > if there loging type applications running that call setgroups and having
> > explicit breakage up front instead of more silent stealthy breakage
> > when the application runs is desired.
> >
> > If we don't want any extra options working tested code is below.

 Do you mean "unshare --setgroups-allow"?

 (And it has to be mutually exclusive to --map-root-user.)

 IMHO it's good idea to make it possible to control this feature by
 unshare util.

> This may also have some affect on the setgroups(0, NULL) case of
> nsenter as well.

 Definitely yes, if I good understand then the best way is to read
 /proc/self/setgroups to check for "allow" before we call setgroups().
 Now we call it all time (for --setguid).

 I can write the patches.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Eric W. Biederman]

Karel Zak <kzak@redhat.com> writes:

> On Wed, Dec 17, 2014 at 05:21:31PM -0600, Eric W. Biederman wrote:
>> ebiederm@xmission.com (Eric W. Biederman) writes:
>> 
>> > I have just merged a security fix into the linux kernel that corrects an
>> > oversight in the permission checks of /proc/self/gid_map.
>> >
>> > The root of the issue is that unix allows anyone to specify permissions
>> > such like: --rwx---rwx on a file, and setgroups call at login time
>> > allows seting groups that even setgid exectuables don't drop.  Which
>> > results in the ability to assign a process fewer privileges just because
>> > it is in a specified group, and this makes dropping groups an unsafe
>> > operation.
>> >
>> > Therefore unprivileged writing of /proc/self/gid_map has been disabled
>> > unless /proc/self/setgroups is written first to permanently disable the
>> > ability to call setgroups in that user namespace.
>
>  What does it mean "allow" in /proc/self/setgroups? 
>  
>  If I good understand than /proc/self/gid_map is unwritable until the
>  setgroups file is set to "deny", and "allow" means that gid_map is
>  disabled at all, but setgroup() syscall is possible to use in the
>  user namespace. Right?

No.

The current state is backwards compatible for root, and is a little
weird but not that weird.

setgroups(2) is only callable with CAP_SETGID.
CAP_SETGID in a user namespace (now) does not give you permission to
call setgroups(2) (or any other system call) until after gid_map has
been set.

/proc/self/setgroups controls the setgroups system call.
"allow" means setgroups(2) is callable (permission checks permitting).
"allow" is the default state of /proc/self/setgroups.
"deny" means setgroups(2) is permanently disabled in the user namespace.
"deny" is only settable while setgroups(2) is disabled (aka "deny" is
       only settable before the gid_map is programmed)

gid_map is writable by root when setgroups(2) is enabled.
gid_map becomes writable by "unprivileged" processes when setgroups(2)
is permamently disabled.


In short /proc/self/setgroups controlls setgroups(2), and /proc/self/gid_map
controlls gids and a processes ability to use system calls that take
gids.

Compared to a clean sheet design everything things are a little wonky
in the interests of backwards compatibility.  To allow existing
applications run as root to ignore /proc/self/setgroups.

>> > In part this design was chosen so that applications that are affected
>> > will break early instead of late, and in part to make it clear to
>> > everyone what is going on.
>> >
>> > I think for the experimental tool that is unshare --make-root-user we
>> > just want to flip the bit and be done with it (patch below).
>> >
>> > However we may want to require an additional option to clear setgroups,
>> > if there loging type applications running that call setgroups and having
              ^^^^^^ login
>> > explicit breakage up front instead of more silent stealthy breakage
>> > when the application runs is desired.
>> >
>> > If we don't want any extra options working tested code is below.
>
>  Do you mean "unshare --setgroups-allow"?

I was thinking --setgroups=deny.

>  (And it has to be mutually exclusive to --map-root-user.)

Agreed --setgroups=allow would need to be mutually exclusive with
--map-root-user.

>  IMHO it's good idea to make it possible to control this feature by
>  unshare util.

Fair enough.  A general control is reasonable, and not hard to support.
Call it --setgroups=[allow|deny].

I was wondering if we should have such a control and require it with
--map-root-user to tell users their shell scripts fork login will break.
For the prupose of breaking setups that will break a little later when
setgroups(2) is called I don't think the option is worth it.

Just as a general knob I can see value in having a
--setgroups=[allow|deny] knob.



Stepping back a minute to the big picture and how this functionality is
used in other programs.

Typically a process will create a user namespace and then another
process with privileges will write to uid_map and gid_map.  For
processes that don't run as root by the helpers newuidmap and newgidmap
from the shadow package provide this functionality.

Note: Even if you start as root with all privileges once you create
the user namespace and enter into it you don't have any privileges
so you must arrange for an outside process to set uid_map and gid_map.

There is a special case for processes without privilege.  They can map
their own uids and gids without privilege.  This facilitates testing
and using user namespaces without coordination with any higher power.

--map-root-user as currently designed only uses that narrow special case
for processes without privilege that allows  mapping your own euid and
egid.  It writes uid_map and gid_map from inside the user namespace
(which is without privilege for purposes of the uid_map and gid_map
permission checks).



Given that unshare is a simple low level tool I can see real value in
my patch to fix --map-root-user.  I can see a little value in a general
--setgroups=[allow|deny] option.  Going any more general seems to
take unshare past the pont of being a simple general purpose utility
and well past the point of diminishing returns.

>> This may also have some affect on the setgroups(0, NULL) case of
>> nsenter as well.
>
>  Definitely yes, if I good understand then the best way is to read
>  /proc/self/setgroups to check for "allow" before we call setgroups().
>  Now we call it all time (for --setguid).
>
>  I can write the patches.

The best is not to check for "allow" before we call setgroups(0, NULL).
Reading /proc/self/setgroups won't tell you anything checking the return
of code of setgroups(0, NULL) won't tell you.

The best would be to call setgroups(0, NULL) before entering the user
namespace (so root can always clear their groups), and call setgroups(0,
NULL) after entering the user namespace (as currently happens).  If both
setgroups(0, NULL) calls fail then complain.

nsenter as currently constructed can not enter a user namespaces that
does not map uid 0 and gid 0.   So not handling setgroups=deny for
non-root users in seems reasonable.

What looks compelling to me is a --preserve-credentials option to
nsenter that would not touch uids or gids.  A --preserve-credentials
option will allow nsenter to enter all manner of user namespaces
irrespective of they are configured.

Does that clear up the confusion?

Eric

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Karel Zak]

On Fri, Dec 19, 2014 at 06:28:45AM -0600, Eric W. Biederman wrote:
> Karel Zak <kzak@redhat.com> writes:
> >  What does it mean "allow" in /proc/self/setgroups? 
> >  
> >  If I good understand than /proc/self/gid_map is unwritable until the
> >  setgroups file is set to "deny", and "allow" means that gid_map is
> >  disabled at all, but setgroup() syscall is possible to use in the
> >  user namespace. Right?
> 
> No.
> 
> The current state is backwards compatible for root, and is a little
> weird but not that weird.
> 
> setgroups(2) is only callable with CAP_SETGID.
> CAP_SETGID in a user namespace (now) does not give you permission to
> call setgroups(2) (or any other system call) until after gid_map has
> been set.
> 
> /proc/self/setgroups controls the setgroups system call.
> "allow" means setgroups(2) is callable (permission checks permitting).
> "allow" is the default state of /proc/self/setgroups.
> "deny" means setgroups(2) is permanently disabled in the user namespace.
> "deny" is only settable while setgroups(2) is disabled (aka "deny" is
>        only settable before the gid_map is programmed)
> 
> gid_map is writable by root when setgroups(2) is enabled.
> gid_map becomes writable by "unprivileged" processes when setgroups(2)
> is permamently disabled.

Thanks, this is more obvious description.

[...]

> Fair enough.  A general control is reasonable, and not hard to support.
> Call it --setgroups=[allow|deny].

OK.

> I was wondering if we should have such a control and require it with
> --map-root-user to tell users their shell scripts fork login will break.

I think it's better to make --map-root-user usable without any another
command line option (as suggested by our unshare patch). In man page
we can describe all the relation between --map-root-user and new
--setgroups.

> For the prupose of breaking setups that will break a little later when
> setgroups(2) is called I don't think the option is worth it.
> 
> Just as a general knob I can see value in having a
> --setgroups=[allow|deny] knob.

Yes.

[...]

> The best would be to call setgroups(0, NULL) before entering the user
> namespace (so root can always clear their groups), and call setgroups(0,
> NULL) after entering the user namespace (as currently happens).  If both
> setgroups(0, NULL) calls fail then complain.
> 
> nsenter as currently constructed can not enter a user namespaces that
> does not map uid 0 and gid 0.   So not handling setgroups=deny for
> non-root users in seems reasonable.
> 
> What looks compelling to me is a --preserve-credentials option to
> nsenter that would not touch uids or gids.  A --preserve-credentials
> option will allow nsenter to enter all manner of user namespaces
> irrespective of they are configured.
> 
> Does that clear up the confusion?

Good question, we will see :-) ... I'm going to prepare some unshare
and nsenter patches next week.

Thanks Eric.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Karel Zak]

On Fri, Dec 19, 2014 at 06:28:45AM -0600, Eric W. Biederman wrote:
> >  IMHO it's good idea to make it possible to control this feature by
> >  unshare util.
> 
> Fair enough.  A general control is reasonable, and not hard to support.
> Call it --setgroups=[allow|deny].

Implemented. Please, review. 

Note that I'm not sure if the description in the man page is good enough for 
end users (it's mostly copy & past from previous Eric's email:-).

    Karel


>From 0226ca0734065fe29cb9dbc4a48ed6a9b5640995 Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak@redhat.com>
Date: Thu, 8 Jan 2015 11:51:58 +0100
Subject: [PATCH] unshare: add --setgroups=deny|allow

Since Linux 3.19 the file /proc/self/setgroups controls setgroups(2)
syscall usage in user namespaces. This patch provides command line knob
for this feature.

The new --setgroups does not automatically implies --user to avoid
complexity, it's user's responsibility to use it in right context. The
exception is --map-root-user which is mutually exclusive to
--setgroups=allow.

CC: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Karel Zak <kzak@redhat.com>
---
 sys-utils/unshare.1 | 17 ++++++++++++++++-
 sys-utils/unshare.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/sys-utils/unshare.1 b/sys-utils/unshare.1
index 1aa9bcb..f9626fc 100644
--- a/sys-utils/unshare.1
+++ b/sys-utils/unshare.1
@@ -83,8 +83,23 @@ Run the program only after the current effective user and group IDs have been ma
 the superuser UID and GID in the newly created user namespace.  This makes it possible to
 conveniently gain capabilities needed to manage various aspects of the newly created
 namespaces (such as configuring interfaces in the network namespace or mounting filesystems in
-the mount namespace) even when run unprivileged.  As a mere convenience feature, it does not support
+the mount namespace) even when run unprivileged.  As a more convenience feature, it does not support
 more sophisticated use cases, such as mapping multiple ranges of UIDs and GIDs.
+This option implies --setgroups=deny.
+.TP
+.BR \-s , " \-\-setgroups \fIallow|deny\fP"
+Allow or deny
+.BR setgroups (2)
+syscall in user namespaces.
+
+.BR setgroups(2)
+is only callable with CAP_SETGID and CAP_SETGID in a user
+namespace (since Linux 3.19) does not give you permission to call setgroups(2)
+until after GID map has been set. The GID map is writable by root when
+.BR setgroups(2)
+is enabled and GID map becomes writable by unprivileged processes when
+.BR setgroups(2)
+is permamently disabled.
 .TP
 .BR \-V , " \-\-version"
 Display version information and exit.
diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 9fdce93..11e2e6b 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -39,12 +39,39 @@
 #include "pathnames.h"
 #include "all-io.h"
 
-static void disable_setgroups(void)
+enum {
+	SETGROUPS_NONE = -1,
+	SETGROUPS_DENY = 0,
+	SETGROUPS_ALLOW = 1,
+};
+
+static const char *setgroups_strings[] =
+{
+	[SETGROUPS_DENY] = "deny",
+	[SETGROUPS_ALLOW] = "allow"
+};
+
+static int setgroups_str2id(const char *str)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(setgroups_strings); i++)
+		if (strcmp(str, setgroups_strings[i]) == 0)
+			return i;
+
+	errx(EXIT_FAILURE, _("unsupported --setgroups argument '%s'"), str);
+}
+
+static void setgroups_control(int action)
 {
 	const char *file = _PATH_PROC_SETGROUPS;
-	const char *deny = "deny";
+	const char *cmd;
 	int fd;
 
+	if (action < 0 || (size_t) action >= ARRAY_SIZE(setgroups_strings))
+		return;
+	cmd = setgroups_strings[action];
+
 	fd = open(file, O_WRONLY);
 	if (fd < 0) {
 		if (errno == ENOENT)
@@ -52,7 +79,7 @@ static void disable_setgroups(void)
 		 err(EXIT_FAILURE, _("cannot open %s"), file);
 	}
 
-	if (write_all(fd, deny, strlen(deny)))
+	if (write_all(fd, cmd, strlen(cmd)))
 		err(EXIT_FAILURE, _("write failed %s"), file);
 	close(fd);
 }
@@ -94,6 +121,7 @@ static void usage(int status)
 	fputs(_(" -f, --fork                fork before launching <program>\n"), out);
 	fputs(_("     --mount-proc[=<dir>]  mount proc filesystem first (implies --mount)\n"), out);
 	fputs(_(" -r, --map-root-user       map current user to root (implies --user)\n"), out);
+	fputs(_(" -s, --setgroups <allow|deny>  control setgroups syscall in user namespaces\n"), out);
 
 	fputs(USAGE_SEPARATOR, out);
 	fputs(USAGE_HELP, out);
@@ -120,9 +148,11 @@ int main(int argc, char *argv[])
 		{ "fork", no_argument, 0, 'f' },
 		{ "mount-proc", optional_argument, 0, OPT_MOUNTPROC },
 		{ "map-root-user", no_argument, 0, 'r' },
+		{ "setgroups", required_argument, 0, 's' },
 		{ NULL, 0, 0, 0 }
 	};
 
+	int setgrpcmd = SETGROUPS_NONE;
 	int unshare_flags = 0;
 	int c, forkit = 0, maproot = 0;
 	const char *procmnt = NULL;
@@ -134,7 +164,7 @@ int main(int argc, char *argv[])
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while ((c = getopt_long(argc, argv, "+fhVmuinpUr", longopts, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, "+fhVmuinpUrs:", longopts, NULL)) != -1) {
 		switch (c) {
 		case 'f':
 			forkit = 1;
@@ -170,6 +200,9 @@ int main(int argc, char *argv[])
 			unshare_flags |= CLONE_NEWUSER;
 			maproot = 1;
 			break;
+		case 's':
+			setgrpcmd = setgroups_str2id(optarg);
+			break;
 		default:
 			usage(EXIT_FAILURE);
 		}
@@ -199,10 +232,20 @@ int main(int argc, char *argv[])
 	}
 
 	if (maproot) {
-		disable_setgroups();
+		if (setgrpcmd == SETGROUPS_ALLOW)
+			errx(EXIT_FAILURE, _("options --setgroups=allow and "
+					"--map-root-user are mutually exclusive."));
+
+		/* since Linux 3.19 unprivileged writing of /proc/self/gid_map
+		 * has s been disabled unless /proc/self/setgroups is written
+		 * first to permanently disable the ability to call setgroups
+		 * in that user namespace. */
+		setgroups_control(SETGROUPS_DENY);
 		map_id(_PATH_PROC_UIDMAP, 0, real_euid);
 		map_id(_PATH_PROC_GIDMAP, 0, real_egid);
-	}
+
+	} else if (setgrpcmd != SETGROUPS_NONE)
+		setgroups_control(setgrpcmd);
 
 	if (procmnt &&
 	    (mount("none", procmnt, NULL, MS_PRIVATE|MS_REC, NULL) != 0 ||
-- 
1.9.3

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Eric W. Biederman]

Karel Zak <kzak@redhat.com> writes:

> On Fri, Dec 19, 2014 at 06:28:45AM -0600, Eric W. Biederman wrote:
>> >  IMHO it's good idea to make it possible to control this feature by
>> >  unshare util.
>> 
>> Fair enough.  A general control is reasonable, and not hard to support.
>> Call it --setgroups=[allow|deny].
>
> Implemented. Please, review. 
>
> Note that I'm not sure if the description in the man page is good enough for 
> end users (it's mostly copy & past from previous Eric's email:-).

A few caveats below.

The basic logic looks solid.

Eric



>     Karel
>
>
> From 0226ca0734065fe29cb9dbc4a48ed6a9b5640995 Mon Sep 17 00:00:00 2001
> From: Karel Zak <kzak@redhat.com>
> Date: Thu, 8 Jan 2015 11:51:58 +0100
> Subject: [PATCH] unshare: add --setgroups=deny|allow
>
> Since Linux 3.19 the file /proc/self/setgroups controls setgroups(2)
> syscall usage in user namespaces. This patch provides command line knob
> for this feature.
>
> The new --setgroups does not automatically implies --user to avoid
> complexity, it's user's responsibility to use it in right context. The
> exception is --map-root-user which is mutually exclusive to
> --setgroups=allow.
>
> CC: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Karel Zak <kzak@redhat.com>
> ---
>  sys-utils/unshare.1 | 17 ++++++++++++++++-
>  sys-utils/unshare.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 65 insertions(+), 7 deletions(-)
>
> diff --git a/sys-utils/unshare.1 b/sys-utils/unshare.1
> index 1aa9bcb..f9626fc 100644
> --- a/sys-utils/unshare.1
> +++ b/sys-utils/unshare.1
> @@ -83,8 +83,23 @@ Run the program only after the current effective user and group IDs have been ma
>  the superuser UID and GID in the newly created user namespace.  This makes it possible to
>  conveniently gain capabilities needed to manage various aspects of the newly created
>  namespaces (such as configuring interfaces in the network namespace or mounting filesystems in
> -the mount namespace) even when run unprivileged.  As a mere convenience feature, it does not support
> +the mount namespace) even when run unprivileged.  As a more convenience feature, it does not support
                                                          ^^^^
mere is the correct word.  The point is that --map-root is there merely for
convinience and does not handle the general case.
I can see rewording that.

>  more sophisticated use cases, such as mapping multiple ranges of UIDs and GIDs.
> +This option implies --setgroups=deny.
> +.TP
> +.BR \-s , " \-\-setgroups \fIallow|deny\fP"
> +Allow or deny
> +.BR setgroups (2)
> +syscall in user namespaces.
> +
> +.BR setgroups(2)
> +is only callable with CAP_SETGID and CAP_SETGID in a user
> +namespace (since Linux 3.19) does not give you permission to call setgroups(2)
> +until after GID map has been set. The GID map is writable by root when
> +.BR setgroups(2)
> +is enabled and GID map becomes writable by unprivileged processes when
> +.BR setgroups(2)
> +is permamently disabled.
>  .TP
>  .BR \-V , " \-\-version"
>  Display version information and exit.
> diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
> index 9fdce93..11e2e6b 100644
> --- a/sys-utils/unshare.c
> +++ b/sys-utils/unshare.c
> @@ -39,12 +39,39 @@
>  #include "pathnames.h"
>  #include "all-io.h"
>  
> -static void disable_setgroups(void)
> +enum {
> +	SETGROUPS_NONE = -1,
> +	SETGROUPS_DENY = 0,
> +	SETGROUPS_ALLOW = 1,
> +};
> +
> +static const char *setgroups_strings[] =
> +{
> +	[SETGROUPS_DENY] = "deny",
> +	[SETGROUPS_ALLOW] = "allow"
> +};
> +
> +static int setgroups_str2id(const char *str)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(setgroups_strings); i++)
> +		if (strcmp(str, setgroups_strings[i]) == 0)
> +			return i;
> +
> +	errx(EXIT_FAILURE, _("unsupported --setgroups argument '%s'"), str);
> +}
> +
> +static void setgroups_control(int action)
>  {
>  	const char *file = _PATH_PROC_SETGROUPS;
> -	const char *deny = "deny";
> +	const char *cmd;
>  	int fd;
>  
> +	if (action < 0 || (size_t) action >= ARRAY_SIZE(setgroups_strings))
> +		return;
> +	cmd = setgroups_strings[action];
> +
>  	fd = open(file, O_WRONLY);
>  	if (fd < 0) {
>  		if (errno == ENOENT)
> @@ -52,7 +79,7 @@ static void disable_setgroups(void)
>  		 err(EXIT_FAILURE, _("cannot open %s"), file);
>  	}
>  
> -	if (write_all(fd, deny, strlen(deny)))
> +	if (write_all(fd, cmd, strlen(cmd)))
>  		err(EXIT_FAILURE, _("write failed %s"), file);
>  	close(fd);
>  }
> @@ -94,6 +121,7 @@ static void usage(int status)
>  	fputs(_(" -f, --fork                fork before launching <program>\n"), out);
>  	fputs(_("     --mount-proc[=<dir>]  mount proc filesystem first (implies --mount)\n"), out);
>  	fputs(_(" -r, --map-root-user       map current user to root (implies --user)\n"), out);
> +	fputs(_(" -s, --setgroups <allow|deny>  control setgroups syscall in user namespaces\n"), out);
>  
>  	fputs(USAGE_SEPARATOR, out);
>  	fputs(USAGE_HELP, out);
> @@ -120,9 +148,11 @@ int main(int argc, char *argv[])
>  		{ "fork", no_argument, 0, 'f' },
>  		{ "mount-proc", optional_argument, 0, OPT_MOUNTPROC },
>  		{ "map-root-user", no_argument, 0, 'r' },
> +		{ "setgroups", required_argument, 0, 's' },

Do we really want a short option?  I can't imagine this being common
enough to want a short option, and and I expect a short option would tend to be
confusing.  

>  		{ NULL, 0, 0, 0 }
>  	};
>  
> +	int setgrpcmd = SETGROUPS_NONE;
>  	int unshare_flags = 0;
>  	int c, forkit = 0, maproot = 0;
>  	const char *procmnt = NULL;
> @@ -134,7 +164,7 @@ int main(int argc, char *argv[])
>  	textdomain(PACKAGE);
>  	atexit(close_stdout);
>  
> -	while ((c = getopt_long(argc, argv, "+fhVmuinpUr", longopts, NULL)) != -1) {
> +	while ((c = getopt_long(argc, argv, "+fhVmuinpUrs:", longopts, NULL)) != -1) {
>  		switch (c) {
>  		case 'f':
>  			forkit = 1;
> @@ -170,6 +200,9 @@ int main(int argc, char *argv[])
>  			unshare_flags |= CLONE_NEWUSER;
>  			maproot = 1;
>  			break;
> +		case 's':
> +			setgrpcmd = setgroups_str2id(optarg);
> +			break;
>  		default:
>  			usage(EXIT_FAILURE);
>  		}
> @@ -199,10 +232,20 @@ int main(int argc, char *argv[])
>  	}
>  
>  	if (maproot) {
> -		disable_setgroups();
> +		if (setgrpcmd == SETGROUPS_ALLOW)
> +			errx(EXIT_FAILURE, _("options --setgroups=allow and "
> +					"--map-root-user are mutually exclusive."));
> +
> +		/* since Linux 3.19 unprivileged writing of /proc/self/gid_map
> +		 * has s been disabled unless /proc/self/setgroups is written
> +		 * first to permanently disable the ability to call setgroups
> +		 * in that user namespace. */
> +		setgroups_control(SETGROUPS_DENY);
>  		map_id(_PATH_PROC_UIDMAP, 0, real_euid);
>  		map_id(_PATH_PROC_GIDMAP, 0, real_egid);
> -	}
> +
> +	} else if (setgrpcmd != SETGROUPS_NONE)
> +		setgroups_control(setgrpcmd);
>  
>  	if (procmnt &&
>  	    (mount("none", procmnt, NULL, MS_PRIVATE|MS_REC, NULL) != 0 ||

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Karel Zak]

On Thu, Jan 08, 2015 at 10:12:47AM -0600, Eric W. Biederman wrote:
> A few caveats below.

 Fixed and both patches merged to master branch.

 Thanks Eric!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Karel Zak]

On Fri, Dec 19, 2014 at 06:28:45AM -0600, Eric W. Biederman wrote:
> >> This may also have some affect on the setgroups(0, NULL) case of
> >> nsenter as well.
... 
> The best would be to call setgroups(0, NULL) before entering the user
> namespace (so root can always clear their groups), and call setgroups(0,
> NULL) after entering the user namespace (as currently happens).  If both
> setgroups(0, NULL) calls fail then complain.
> 
> nsenter as currently constructed can not enter a user namespaces that
> does not map uid 0 and gid 0.   So not handling setgroups=deny for
> non-root users in seems reasonable.
> 
> What looks compelling to me is a --preserve-credentials option to
> nsenter that would not touch uids or gids.  A --preserve-credentials
> option will allow nsenter to enter all manner of user namespaces
> irrespective of they are configured.

 Implemented. Please, review.

    Karel


>From 6e38747286e958e5c0f6060e803ea916bb6be756 Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak@redhat.com>
Date: Thu, 8 Jan 2015 12:52:43 +0100
Subject: [PATCH] nsenter: add --preserve-credentials and cleanup setgroups()
 usage

The new option --preserve-credentials completely disables all
operations related to UIGs and GIDs.

The patch also calls setgroups() before we enter user namespace (so
root can always clear their groups) and after we enter user namespace
(to detect /proc/self/setgroups "deny"). If both fail then nsenter
complains.

CC: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Karel Zak <kzak@redhat.com>
---
 sys-utils/nsenter.1 |  4 ++++
 sys-utils/nsenter.c | 27 +++++++++++++++++++++------
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/sys-utils/nsenter.1 b/sys-utils/nsenter.1
index 487f731..0b1fceb 100644
--- a/sys-utils/nsenter.1
+++ b/sys-utils/nsenter.1
@@ -136,6 +136,10 @@ Set the user ID which will be used in the entered namespace.
 .BR nsenter (1)
 always sets UID for user namespaces, the default is 0.
 .TP
+\fB\-\-preserve-credentials\fR
+Don't modify UID and GID when enter user namespace. The default is to
+drops supplementary groups and sets GID and UID to 0.
+.TP
 \fB\-r\fR, \fB\-\-root\fR[=\fIdirectory\fR]
 Set the root directory.  If no directory is specified, set the root directory to
 the root directory of the target process.  If directory is specified, set the
diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
index 50f77f3..b029f80 100644
--- a/sys-utils/nsenter.c
+++ b/sys-utils/nsenter.c
@@ -78,6 +78,7 @@ static void usage(int status)
 	fputs(_(" -U, --user[=<file>]    enter user namespace\n"), out);
 	fputs(_(" -S, --setuid <uid>     set uid in entered namespace\n"), out);
 	fputs(_(" -G, --setgid <gid>     set gid in entered namespace\n"), out);
+	fputs(_("     --preserve-credentials do not touch uids or gids\n"), out);
 	fputs(_(" -r, --root[=<dir>]     set the root directory\n"), out);
 	fputs(_(" -w, --wd[=<dir>]       set the working directory\n"), out);
 	fputs(_(" -F, --no-fork          do not fork before exec'ing <program>\n"), out);
@@ -165,6 +166,9 @@ static void continue_as_child(void)
 
 int main(int argc, char *argv[])
 {
+	enum {
+		OPT_PRESERVE_CRED = CHAR_MAX + 1
+	};
 	static const struct option longopts[] = {
 		{ "help", no_argument, NULL, 'h' },
 		{ "version", no_argument, NULL, 'V'},
@@ -180,11 +184,12 @@ int main(int argc, char *argv[])
 		{ "root", optional_argument, NULL, 'r' },
 		{ "wd", optional_argument, NULL, 'w' },
 		{ "no-fork", no_argument, NULL, 'F' },
+		{ "preserve-credentials", no_argument, NULL, OPT_PRESERVE_CRED },
 		{ NULL, 0, NULL, 0 }
 	};
 
 	struct namespace_file *nsfile;
-	int c, namespaces = 0;
+	int c, namespaces = 0, setgroups_nerrs = 0, preserve_cred = 0;
 	bool do_rd = false, do_wd = false, force_uid = false, force_gid = false;
 	int do_fork = -1; /* unknown yet */
 	uid_t uid = 0;
@@ -267,6 +272,9 @@ int main(int argc, char *argv[])
 			else
 				do_wd = true;
 			break;
+		case OPT_PRESERVE_CRED:
+			preserve_cred = 1;
+			break;
 		default:
 			usage(EXIT_FAILURE);
 		}
@@ -292,6 +300,17 @@ int main(int argc, char *argv[])
 		namespaces |= nsfile->nstype;
 	}
 
+	/* for user namespaces we always set UID and GID (default is 0)
+	 * and clear root's groups if --preserve-credentials is no specified */
+	if ((namespaces & CLONE_NEWUSER) && !preserve_cred) {
+		force_uid = true, force_gid = true;
+
+		/* We call setgroups() before and after we enter user namespace,
+		 * let's complain only if both fail */
+		if (setgroups(0, NULL) != 0)
+			setgroups_nerrs++;
+	}
+
 	/*
 	 * Now that we know which namespaces we want to enter, enter them.
 	 */
@@ -342,12 +361,8 @@ int main(int argc, char *argv[])
 	if (do_fork == 1)
 		continue_as_child();
 
-	/* for user namespaces we always set UID and GID (default is 0) */
-	if (namespaces & CLONE_NEWUSER)
-		force_uid = true, force_gid = true;
-
 	if (force_uid || force_gid) {
-		if (force_gid && setgroups(0, NULL))		/* drop supplementary groups */
+		if (force_gid && setgroups(0, NULL) != 0 && setgroups_nerrs)	/* drop supplementary groups */
 			err(EXIT_FAILURE, _("setgroups failed"));
 		if (force_gid && setgid(gid) < 0)		/* change GID */
 			err(EXIT_FAILURE, _("setgid failed"));
-- 
1.9.3


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [RFC][PATCH] unshare: Fix --map-root-user to work on new kernels

[Eric W. Biederman]

Karel Zak <kzak@redhat.com> writes:

> On Fri, Dec 19, 2014 at 06:28:45AM -0600, Eric W. Biederman wrote:
>> >> This may also have some affect on the setgroups(0, NULL) case of
>> >> nsenter as well.
> ... 
>> The best would be to call setgroups(0, NULL) before entering the user
>> namespace (so root can always clear their groups), and call setgroups(0,
>> NULL) after entering the user namespace (as currently happens).  If both
>> setgroups(0, NULL) calls fail then complain.
>> 
>> nsenter as currently constructed can not enter a user namespaces that
>> does not map uid 0 and gid 0.   So not handling setgroups=deny for
>> non-root users in seems reasonable.
>> 
>> What looks compelling to me is a --preserve-credentials option to
>> nsenter that would not touch uids or gids.  A --preserve-credentials
>> option will allow nsenter to enter all manner of user namespaces
>> irrespective of they are configured.
>
>  Implemented. Please, review.


Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reading it through that looks reasonable.

>     Karel
>
>
> From 6e38747286e958e5c0f6060e803ea916bb6be756 Mon Sep 17 00:00:00 2001
> From: Karel Zak <kzak@redhat.com>
> Date: Thu, 8 Jan 2015 12:52:43 +0100
> Subject: [PATCH] nsenter: add --preserve-credentials and cleanup setgroups()
>  usage
>
> The new option --preserve-credentials completely disables all
> operations related to UIGs and GIDs.
>
> The patch also calls setgroups() before we enter user namespace (so
> root can always clear their groups) and after we enter user namespace
> (to detect /proc/self/setgroups "deny"). If both fail then nsenter
> complains.
>
> CC: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Karel Zak <kzak@redhat.com>
> ---
>  sys-utils/nsenter.1 |  4 ++++
>  sys-utils/nsenter.c | 27 +++++++++++++++++++++------
>  2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/sys-utils/nsenter.1 b/sys-utils/nsenter.1
> index 487f731..0b1fceb 100644
> --- a/sys-utils/nsenter.1
> +++ b/sys-utils/nsenter.1
> @@ -136,6 +136,10 @@ Set the user ID which will be used in the entered namespace.
>  .BR nsenter (1)
>  always sets UID for user namespaces, the default is 0.
>  .TP
> +\fB\-\-preserve-credentials\fR
> +Don't modify UID and GID when enter user namespace. The default is to
> +drops supplementary groups and sets GID and UID to 0.
> +.TP
>  \fB\-r\fR, \fB\-\-root\fR[=\fIdirectory\fR]
>  Set the root directory.  If no directory is specified, set the root directory to
>  the root directory of the target process.  If directory is specified, set the
> diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
> index 50f77f3..b029f80 100644
> --- a/sys-utils/nsenter.c
> +++ b/sys-utils/nsenter.c
> @@ -78,6 +78,7 @@ static void usage(int status)
>  	fputs(_(" -U, --user[=<file>]    enter user namespace\n"), out);
>  	fputs(_(" -S, --setuid <uid>     set uid in entered namespace\n"), out);
>  	fputs(_(" -G, --setgid <gid>     set gid in entered namespace\n"), out);
> +	fputs(_("     --preserve-credentials do not touch uids or gids\n"), out);
>  	fputs(_(" -r, --root[=<dir>]     set the root directory\n"), out);
>  	fputs(_(" -w, --wd[=<dir>]       set the working directory\n"), out);
>  	fputs(_(" -F, --no-fork          do not fork before exec'ing <program>\n"), out);
> @@ -165,6 +166,9 @@ static void continue_as_child(void)
>  
>  int main(int argc, char *argv[])
>  {
> +	enum {
> +		OPT_PRESERVE_CRED = CHAR_MAX + 1
> +	};
>  	static const struct option longopts[] = {
>  		{ "help", no_argument, NULL, 'h' },
>  		{ "version", no_argument, NULL, 'V'},
> @@ -180,11 +184,12 @@ int main(int argc, char *argv[])
>  		{ "root", optional_argument, NULL, 'r' },
>  		{ "wd", optional_argument, NULL, 'w' },
>  		{ "no-fork", no_argument, NULL, 'F' },
> +		{ "preserve-credentials", no_argument, NULL, OPT_PRESERVE_CRED },
>  		{ NULL, 0, NULL, 0 }
>  	};
>  
>  	struct namespace_file *nsfile;
> -	int c, namespaces = 0;
> +	int c, namespaces = 0, setgroups_nerrs = 0, preserve_cred = 0;
>  	bool do_rd = false, do_wd = false, force_uid = false, force_gid = false;
>  	int do_fork = -1; /* unknown yet */
>  	uid_t uid = 0;
> @@ -267,6 +272,9 @@ int main(int argc, char *argv[])
>  			else
>  				do_wd = true;
>  			break;
> +		case OPT_PRESERVE_CRED:
> +			preserve_cred = 1;
> +			break;
>  		default:
>  			usage(EXIT_FAILURE);
>  		}
> @@ -292,6 +300,17 @@ int main(int argc, char *argv[])
>  		namespaces |= nsfile->nstype;
>  	}
>  
> +	/* for user namespaces we always set UID and GID (default is 0)
> +	 * and clear root's groups if --preserve-credentials is no specified */
> +	if ((namespaces & CLONE_NEWUSER) && !preserve_cred) {
> +		force_uid = true, force_gid = true;
> +
> +		/* We call setgroups() before and after we enter user namespace,
> +		 * let's complain only if both fail */
> +		if (setgroups(0, NULL) != 0)
> +			setgroups_nerrs++;
> +	}
> +
>  	/*
>  	 * Now that we know which namespaces we want to enter, enter them.
>  	 */
> @@ -342,12 +361,8 @@ int main(int argc, char *argv[])
>  	if (do_fork == 1)
>  		continue_as_child();
>  
> -	/* for user namespaces we always set UID and GID (default is 0) */
> -	if (namespaces & CLONE_NEWUSER)
> -		force_uid = true, force_gid = true;
> -
>  	if (force_uid || force_gid) {
> -		if (force_gid && setgroups(0, NULL))		/* drop supplementary groups */
> +		if (force_gid && setgroups(0, NULL) != 0 && setgroups_nerrs)	/* drop supplementary groups */
>  			err(EXIT_FAILURE, _("setgroups failed"));
>  		if (force_gid && setgid(gid) < 0)		/* change GID */
>  			err(EXIT_FAILURE, _("setgid failed"));
> -- 
> 1.9.3