linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] userns: Disallow setgroups unless the gid_map writer is privileged
@ 2014-11-29 17:26 Andy Lutomirski
  2014-12-02 12:09 ` Eric W. Biederman
  0 siblings, 1 reply; 79+ messages in thread
From: Andy Lutomirski @ 2014-11-29 17:26 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook,
	Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel,
	LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger,
	kenton, Andy Lutomirski, stable

Classic unix permission checks have an interesting feature.  The
group permissions for a file can be set to less than the other
permissions on a file.  Occasionally this is used deliberately to
give a certain group of users fewer permissions than the default.

User namespaces break this usage.  Groups set in rgid or egid are
unaffected because an unprivileged user namespace creator can only
map a single group, so setresgid inside and outside the namespace
have the same effect.  However, an unprivileged user namespace
creator can currently use setgroups(2) to drop all supplementary
groups, so, if a supplementary group denies access to some resource,
user namespaces can be used to bypass that restriction.

To fix this issue, this introduces a new user namespace flag
USERNS_SETGROUPS_ALLOWED.  If that flag is not set, then
setgroups(2) will fail regardless of the caller's capabilities.

USERNS_SETGROUPS_ALLOWED is cleared in a new user namespace.  By
default, if the writer of gid_map has CAP_SETGID in the parent
userns and the parent userns has USERNS_SETGROUPS_ALLOWED, then the
USERNS_SETGROUPS_ALLOWED will be set in the child.  If the writer is
not so privileged, then writing to gid_map will fail unless the
writer adds "setgroups deny" to gid_map, in which case the check is
skipped but USERNS_SETGROUPS_ALLOWED will remain cleared.

The full semantics are:

If "setgroups allow" is present or no explicit "setgroups" setting
is written to gid_map, then writing to gid_map will fail with -EPERM
unless the opener and writer have CAP_SETGID in the parent namespace
and the parent namespace has USERNS_SETGROUPS_ALLOWED.

If "setgroups deny" is present, then writing gid_map will work as
before, but USERNS_SETGROUPS_ALLOWED will remain cleared.  This will
result in processes in the userns that have CAP_SETGID to be
nontheless unable to use setgroups(2).  If this breaks something
inside the userns, then this is okay -- the userns creator
specifically requested this behavior.

While it could be safe to set USERNS_SETGROUPS_ALLOWED if the user
namespace creator has no supplementary groups, doing so could be
surprising and could have unpleasant interactions with setns(2).

Any application that uses newgidmap(1) should be unaffected by this
fix, but unprivileged users that create user namespaces to
manipulate mounts or sandbox themselves will break until they start
using "setgroups deny".

This should fix CVE-2014-8989.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

Unlike v1, this *will* break things like Sandstorm.  Fixing them will be
easy.  I agree that this will result in better long-term semantics, but
I'm not so happy about breaking working software.

If this is unpalatable, here's a different option: get rid of all these
permission checks and just change setgroups.  Specifically, make it so
that setgroups(2) in a userns will succeed but will silently refuse to
remove unmapped groups.

Changes from v1:
 - Userns flags are now properly atomic.
 - "setgroups allow" is now the default, so legacy unprivileged gid_map
   writers will start to fail.

 include/linux/user_namespace.h |  3 +++
 kernel/groups.c                |  3 +++
 kernel/user.c                  |  1 +
 kernel/user_namespace.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index e95372654f09..0ae4a8c97165 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -17,6 +17,8 @@ struct uid_gid_map {	/* 64 bytes -- 1 cache line */
 	} extent[UID_GID_MAP_MAX_EXTENTS];
 };
 
+#define USERNS_SETGROUPS_ALLOWED 0
+
 struct user_namespace {
 	struct uid_gid_map	uid_map;
 	struct uid_gid_map	gid_map;
@@ -27,6 +29,7 @@ struct user_namespace {
 	kuid_t			owner;
 	kgid_t			group;
 	unsigned int		proc_inum;
+	unsigned long		flags;
 
 	/* Register of per-UID persistent keyrings for this namespace */
 #ifdef CONFIG_PERSISTENT_KEYRINGS
diff --git a/kernel/groups.c b/kernel/groups.c
index 451698f86cfa..b5ec42423202 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -6,6 +6,7 @@
 #include <linux/slab.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
+#include <linux/user_namespace.h>
 #include <asm/uaccess.h>
 
 /* init to 2 - one for init_task, one to ensure it is never freed */
@@ -223,6 +224,8 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
 	struct group_info *group_info;
 	int retval;
 
+	if (!test_bit(USERNS_SETGROUPS_ALLOWED, &current_user_ns()->flags))
+		return -EPERM;
 	if (!ns_capable(current_user_ns(), CAP_SETGID))
 		return -EPERM;
 	if ((unsigned)gidsetsize > NGROUPS_MAX)
diff --git a/kernel/user.c b/kernel/user.c
index 4efa39350e44..58fba8ea0845 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -51,6 +51,7 @@ struct user_namespace init_user_ns = {
 	.owner = GLOBAL_ROOT_UID,
 	.group = GLOBAL_ROOT_GID,
 	.proc_inum = PROC_USER_INIT_INO,
+	.flags = (1 << USERNS_SETGROUPS_ALLOWED),
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 	.persistent_keyring_register_sem =
 	__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index aa312b0dc3ec..1f63935483e9 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -601,6 +601,10 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 	char *kbuf, *pos, *next_line;
 	ssize_t ret = -EINVAL;
 
+	bool may_setgroups = false;
+	bool setgroups_requested = true;
+	bool seen_explicit_setgroups = false;
+
 	/*
 	 * The id_map_mutex serializes all writes to any given map.
 	 *
@@ -633,6 +637,18 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 	if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN))
 		goto out;
 
+	if (map == &ns->gid_map) {
+		/*
+		 * Setgroups is permitted if the writer and the
+		 * parent ns are privileged.
+		 */
+		may_setgroups =
+			test_bit(USERNS_SETGROUPS_ALLOWED,
+				 &ns->parent->flags) &&
+			file_ns_capable(file, ns->parent, CAP_SETGID) &&
+			ns_capable(ns->parent, CAP_SETGID);
+	}
+
 	/* Get a buffer */
 	ret = -ENOMEM;
 	page = __get_free_page(GFP_TEMPORARY);
@@ -667,6 +683,23 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 				next_line = NULL;
 		}
 
+		/* Is this line a gid_map option? */
+		if (map == &ns->gid_map) {
+			if (!strcmp(pos, "setgroups deny")) {
+				if (seen_explicit_setgroups)
+					goto out;
+				seen_explicit_setgroups = true;
+				setgroups_requested = false;
+				continue;
+			} else if (!strcmp(pos, "setgroups allow")) {
+				if (seen_explicit_setgroups)
+					goto out;
+				seen_explicit_setgroups = true;
+				setgroups_requested = true;
+				continue;
+			}
+		}
+
 		pos = skip_spaces(pos);
 		extent->first = simple_strtoul(pos, &pos, 10);
 		if (!isspace(*pos))
@@ -741,6 +774,15 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 		extent->lower_first = lower_first;
 	}
 
+	/* Validate and install setgroups permission. */
+	if (map == &ns->gid_map && setgroups_requested) {
+		if (!may_setgroups) {
+			ret = -EPERM;
+			goto out;
+		}
+		set_bit(USERNS_SETGROUPS_ALLOWED, &ns->flags);
+	}
+
 	/* Install the map */
 	memcpy(map->extent, new_map.extent,
 		new_map.nr_extents*sizeof(new_map.extent[0]));
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 79+ messages in thread

end of thread, other threads:[~2015-03-04 14:00 UTC | newest]

Thread overview: 79+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-29 17:26 [PATCH v2] userns: Disallow setgroups unless the gid_map writer is privileged Andy Lutomirski
2014-12-02 12:09 ` Eric W. Biederman
2014-12-02 18:53   ` Andy Lutomirski
2014-12-02 19:45     ` Eric W. Biederman
2014-12-02 20:13       ` Andy Lutomirski
2014-12-02 20:25         ` [CFT][PATCH 1/3] userns: Avoid problems with negative groups Eric W. Biederman
2014-12-02 20:28           ` [CFT][PATCH 2/3] userns: Add a knob to disable setgroups on a per user namespace basis Eric W. Biederman
2014-12-02 20:30             ` [CFT][PATCH 3/3] userns: Unbreak the unprivileged remount tests Eric W. Biederman
2014-12-02 21:05             ` [CFT][PATCH 2/3] userns: Add a knob to disable setgroups on a per user namespace basis Andy Lutomirski
2014-12-02 21:45               ` Eric W. Biederman
2014-12-02 22:17                 ` Andy Lutomirski
2014-12-02 23:07                   ` Eric W. Biederman
2014-12-02 23:17                     ` Andy Lutomirski
2014-12-08 22:06                       ` [CFT][PATCH 1/7] userns: Document what the invariant required for safe unprivileged mappings Eric W. Biederman
2014-12-08 22:07                         ` [CFT][PATCH 2/7] userns: Don't allow setgroups until a gid mapping has been setablished Eric W. Biederman
2014-12-08 22:11                           ` Andy Lutomirski
     [not found]                             ` <87h9x5ok0h.fsf@x220.int.ebiederm.org>
2014-12-08 22:33                               ` Andy Lutomirski
2014-12-08 22:17                           ` Richard Weinberger
2014-12-08 22:25                             ` Andy Lutomirski
2014-12-08 22:27                               ` Richard Weinberger
     [not found]                                 ` <874mt5ojfh.fsf@x220.int.ebiederm.org>
2014-12-08 22:47                                   ` Andy Lutomirski
2014-12-08 22:07                         ` [CFT][PATCH 3/7] userns: Don't allow unprivileged creation of gid mappings Eric W. Biederman
2014-12-08 22:08                         ` [CFT][PATCH 4/7] userns: Check euid no fsuid when establishing an unprivileged uid mapping Eric W. Biederman
2014-12-08 22:12                           ` Andy Lutomirski
2014-12-08 22:10                         ` [CFT][PATCH 5/7] userns: Only allow the creator of the userns unprivileged mappings Eric W. Biederman
2014-12-08 22:15                           ` Andy Lutomirski
2014-12-08 22:11                         ` [CFT][PATCH 6/7] userns: Add a knob to disable setgroups on a per user namespace basis Eric W. Biederman
2014-12-08 22:21                           ` Andy Lutomirski
2014-12-08 22:44                             ` Eric W. Biederman
2014-12-08 22:48                               ` Andy Lutomirski
2014-12-08 23:30                                 ` Eric W. Biederman
2014-12-09 19:31                                   ` Eric W. Biederman
2014-12-09 20:36                                     ` [CFT][PATCH 1/8] userns: Document what the invariant required for safe unprivileged mappings Eric W. Biederman
2014-12-09 20:38                                       ` [CFT][PATCH 2/8] userns: Don't allow setgroups until a gid mapping has been setablished Eric W. Biederman
2014-12-09 22:49                                         ` Andy Lutomirski
2014-12-09 20:39                                       ` [CFT][PATCH 3/8] userns: Don't allow unprivileged creation of gid mappings Eric W. Biederman
2014-12-09 23:00                                         ` Andy Lutomirski
2014-12-09 20:39                                       ` [CFT][PATCH 4/8] userns: Check euid no fsuid when establishing an unprivileged uid mapping Eric W. Biederman
2014-12-09 20:41                                       ` [CFT][PATCH 5/8] userns: Only allow the creator of the userns unprivileged mappings Eric W. Biederman
2014-12-09 20:41                                       ` [CFT][PATCH 6/8] userns: Rename id_map_mutex to userns_state_mutex Eric W. Biederman
2014-12-09 22:49                                         ` Andy Lutomirski
2014-12-09 20:42                                       ` [CFT][PATCH 7/8] userns: Add a knob to disable setgroups on a per user namespace basis Eric W. Biederman
2014-12-09 22:28                                         ` Andy Lutomirski
     [not found]                                           ` <971ad3f6-90fd-4e3f-916c-8988af3c826d@email.android.com>
2014-12-10  0:21                                             ` Andy Lutomirski
     [not found]                                               ` <87wq5zf83t.fsf@x220.int.ebiederm.org>
     [not found]                                                 ` <87iohh3c9c.fsf@x220.int.ebiederm.org>
2014-12-12  1:30                                                   ` Andy Lutomirski
     [not found]                                                   ` <8761dh3b7k.fsf_-_@x220.int.ebiederm.org>
     [not found]                                                     ` <878uicy1r9.fsf_-_@x220.int.ebiederm.org>
2014-12-12 21:54                                                       ` [PATCH 1/2] proc.5: Document /proc/[pid]/setgroups Eric W. Biederman
2015-02-02 15:36                                                         ` Michael Kerrisk (man-pages)
2015-02-11  8:01                                                           ` Michael Kerrisk (man-pages)
2015-02-11 13:51                                                             ` Eric W. Biederman
2015-02-12 13:53                                                               ` Michael Kerrisk (man-pages)
2015-02-21  7:57                                                                 ` Michael Kerrisk (man-pages)
2015-03-03 11:39                                                                 ` Michael Kerrisk (man-pages)
2014-12-12 21:54                                                       ` [PATCH 2/2] user_namespaces.7: Update the documention to reflect the fixes for negative groups Eric W. Biederman
2015-02-02 15:37                                                         ` Michael Kerrisk (man-pages)
2015-02-11  8:02                                                           ` Michael Kerrisk (man-pages)
2015-02-11 14:01                                                             ` Eric W. Biederman
2015-02-12 10:11                                                               ` Michael Kerrisk (man-pages)
2015-02-02 21:31                                                         ` Alban Crequy
2015-03-04 14:00                                                           ` Michael Kerrisk (man-pages)
2014-12-09 20:43                                       ` [CFT][PATCH 8/8] userns: Allow setting gid_maps without privilege when setgroups is disabled Eric W. Biederman
2014-12-10 16:39                                       ` [CFT] Can I get some Tested-By's on this series? Eric W. Biederman
2014-12-10 22:48                                         ` Serge Hallyn
2014-12-10 22:50                                           ` Richard Weinberger
2014-12-10 23:19                                             ` Eric W. Biederman
2014-12-11 19:27                                               ` Richard Weinberger
2014-12-12  6:56                                               ` Chen, Hanxiao
2014-12-13 22:31                                           ` serge
     [not found]                                           ` <87lhmcy2et.fsf@x220.int.ebiederm.org>
     [not found]                                             ` <20141212220840.GF22091@castiana.ipv6.teksavvy.com>
     [not found]                                               ` <8761dgze56.fsf@x220.int.ebiederm.org>
2014-12-15 19:38                                                 ` Serge Hallyn
2014-12-15 20:11                                                   ` Eric W. Biederman
2014-12-15 20:49                                                     ` Serge Hallyn
2014-12-16  2:05                                         ` Andy Lutomirski
2014-12-16  9:23                                           ` Richard Weinberger
2014-12-08 22:14                         ` [CFT][PATCH 7/7] userns: Allow setting gid_maps without privilege when setgroups is disabled Eric W. Biederman
2014-12-08 22:26                           ` Andy Lutomirski
2014-12-02 20:58           ` [CFT][PATCH 1/3] userns: Avoid problems with negative groups Andy Lutomirski
2014-12-02 21:26             ` Eric W. Biederman
2014-12-02 22:09               ` Andy Lutomirski
2014-12-02 22:48                 ` Eric W. Biederman
2014-12-02 22:56                   ` Andy Lutomirski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).