userns: Disallow setgroups unless the gid_map writer is privileged

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 [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---

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(+)

include/linux/user_namespace.h +3 -0

index e95372654f09..0ae4a8c97165 100644
@@ -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

kernel/groups.c +3 -0

index 451698f86cfa..b5ec42423202 100644
@@ -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)

kernel/user.c +1 -0

index 4efa39350e44..58fba8ea0845 100644
@@ -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),

kernel/user_namespace.c +42 -0

index aa312b0dc3ec..1f63935483e9 100644
@@ -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]));

Quoted 32 lines · Andy Lutomirski

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.

Quoted 6 lines

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.

I think we need to do this but I also think setgroups allow/deny should be a separate knob than the uid/gid mapping.

If for no other reason than you missed at least two implementations of setgroups, in your implementation.

Quoted 12 lines

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.

Quoted 7 lines

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---

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.

I know what you mean. One of the pieces of software broken by all of
this is my test to verify the remount semantics. Which makes all of
this very unfortunate.

Quoted 4 lines

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.

Nope silently refusing to remove unmapped groups is not enough. I can make any gid in my supplemental groups my egid, it takes a sgid helper application but I don't need any special privileges to create that. Once that group is my egid I can map it. Which means I could drop any one group of my choosing without privielges. Which out and out breaks negative groups :(

I got to looking and I have a significant piece of code that all of this breaks.

tools/testing/selftests/mount/unprivileged-remount-test.c

So I am extra motivated to figure out at find a way to preserve most of the existing functionality. My regression tests won't pass until I can find something pallateable.

It is very annoying that every option I have considered so far breaks something useful.

Having a write once setgroups disable, and the allowing unprivileged mappings after that seems the most palatable option I have seen, semantically. Which means existing software that doesn't care about setgroups can just add the disable code and then work otherwise unmodified.

The other option that I have played with is forcing a set of groups in setgroups if your user namespace was created without privilege, that winds up requiring that verify you don't have any other supplementary groups, and is generally messy whichever way I look at it.

*Pounds head on desk*

What a mess.

Eric

Quoted 139 lines

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(+)

include/linux/user_namespace.h +3 -0

index e95372654f09..0ae4a8c97165 100644
@@ -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

kernel/groups.c +3 -0

index 451698f86cfa..b5ec42423202 100644
@@ -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)

kernel/user.c +1 -0

index 4efa39350e44..58fba8ea0845 100644
@@ -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),

kernel/user_namespace.c +42 -0

index aa312b0dc3ec..1f63935483e9 100644
@@ -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]));

Quoted 41 lines · Eric W. Biederman

Quoted 32 lines · Andy Lutomirski

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.

Quoted 6 lines

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.

Quoted 2 lines

I think we need to do this but I also think setgroups allow/deny should be a separate knob than the uid/gid mapping.

Yeah. It should be readable, too.

Quoted 3 lines

If for no other reason than you missed at least two implementations of setgroups, in your implementation.

I clearly didn't grep hard enough. Grr.

Quoted 29 lines

Quoted 12 lines

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.

Quoted 7 lines

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---

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.

I know what you mean. One of the pieces of software broken by all of
this is my test to verify the remount semantics. Which makes all of
this very unfortunate.

Quoted 4 lines

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.

Quoted 6 lines

Nope silently refusing to remove unmapped groups is not enough. I can make any gid in my supplemental groups my egid, it takes a sgid helper application but I don't need any special privileges to create that. Once that group is my egid I can map it. Which means I could drop any one group of my choosing without privielges. Which out and out breaks negative groups :(

Whoops, right. And you can, indeed, have egid match one of your supplementary groups.

Quoted 19 lines

I got to looking and I have a significant piece of code that all of this breaks.

tools/testing/selftests/mount/unprivileged-remount-test.c

So I am extra motivated to figure out at find a way to preserve most of the existing functionality. My regression tests won't pass until I can find something pallateable.

It is very annoying that every option I have considered so far breaks something useful.

Having a write once setgroups disable, and the allowing unprivileged mappings after that seems the most palatable option I have seen, semantically. Which means existing software that doesn't care about setgroups can just add the disable code and then work otherwise unmodified.

Quoted 4 lines

The other option that I have played with is forcing a set of groups in setgroups if your user namespace was created without privilege, that winds up requiring that verify you don't have any other supplementary groups, and is generally messy whichever way I look at it.

How bad would the automatic selection of setgroups behavior really be?

Suppose we have /proc/self/userns_setgroups_mode that can be "allow", "deny", or "auto". It starts out as "auto" (or "deny" if it's set to "deny" in the parent). Once any of the maps have been set, userns_options becomes readonly. If you try to write to gid_map when setgroups == auto, then it switches to "allow" or "deny" depending on whether the writer has privilege.

This is nasty magical behavior, but it should DTRT for existing users, and everyone can be updated to set the value explicitly.

FWIW, it might also make sense to move all of this stuff into /proc/PID/userns. There may be races in which a setuid or otherwise privileged helper pokes at more than one userns file but actually modifies different namespaces each time. I don't know whether these races matter. uid_map, gid_map, and projid_map could be symlinks.

Quoted 125 lines · Andy Lutomirski

Quoted 41 lines · Eric W. Biederman

Quoted 32 lines · Andy Lutomirski

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.

Quoted 6 lines

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.

Quoted 2 lines

I think we need to do this but I also think setgroups allow/deny should be a separate knob than the uid/gid mapping.

Yeah. It should be readable, too.

Quoted 3 lines

If for no other reason than you missed at least two implementations of setgroups, in your implementation.

I clearly didn't grep hard enough. Grr.

Quoted 29 lines

Quoted 12 lines

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.

Quoted 7 lines

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---

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.

I know what you mean. One of the pieces of software broken by all of
this is my test to verify the remount semantics. Which makes all of
this very unfortunate.

Quoted 4 lines

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.

Quoted 6 lines

Nope silently refusing to remove unmapped groups is not enough. I can make any gid in my supplemental groups my egid, it takes a sgid helper application but I don't need any special privileges to create that. Once that group is my egid I can map it. Which means I could drop any one group of my choosing without privielges. Which out and out breaks negative groups :(

Whoops, right. And you can, indeed, have egid match one of your supplementary groups.

Quoted 19 lines

I got to looking and I have a significant piece of code that all of this breaks.

tools/testing/selftests/mount/unprivileged-remount-test.c

So I am extra motivated to figure out at find a way to preserve most of the existing functionality. My regression tests won't pass until I can find something pallateable.

It is very annoying that every option I have considered so far breaks something useful.

Having a write once setgroups disable, and the allowing unprivileged mappings after that seems the most palatable option I have seen, semantically. Which means existing software that doesn't care about setgroups can just add the disable code and then work otherwise unmodified.

Quoted 4 lines

The other option that I have played with is forcing a set of groups in setgroups if your user namespace was created without privilege, that winds up requiring that verify you don't have any other supplementary groups, and is generally messy whichever way I look at it.

How bad would the automatic selection of setgroups behavior really be?

Suppose we have /proc/self/userns_setgroups_mode that can be "allow", "deny", or "auto". It starts out as "auto" (or "deny" if it's set to "deny" in the parent). Once any of the maps have been set, userns_options becomes readonly. If you try to write to gid_map when setgroups == auto, then it switches to "allow" or "deny" depending on whether the writer has privilege.

Quoted 2 lines

This is nasty magical behavior, but it should DTRT for existing users, and everyone can be updated to set the value explicitly.

Rarely is everything updated unless there is a requirement for an update.

For my code that cares an update is necessary anyway as it contains a gratuitous setgroups(0, NULL).

Since we have to break applications breaking them loud and clear and letting them set the flat to recover (if possible) seems the best we can do. That at least allows someone to ask if they depend on setgroups or init_groups.

Quoted 6 lines

FWIW, it might also make sense to move all of this stuff into /proc/PID/userns. There may be races in which a setuid or otherwise privileged helper pokes at more than one userns file but actually modifies different namespaces each time. I don't know whether these races matter. uid_map, gid_map, and projid_map could be symlinks.

I don't see how moving these files as removing any races.

Eric

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

A value of 0 means the setgroups system call is disabled in the current processes user namespace and can not be enabled in the future in this user namespace.

A value of 1 means the segtoups system call is enabled.

arch/s390/kernel/compat_linux.c +34 -0

                }
        }
 

@@ -844,6 +850,93 @@ static bool new_idmap_permitted(const struct file *file,
        return false;
 }
 
+static void *setgroups_m_start(struct seq_file *seq, loff_t *ppos)
+{
+       struct user_namespace *ns = seq->private;
+
+       return (*ppos == 0) ?  ns : NULL;
+}
+
+static void *setgroups_m_next(struct seq_file *seq, void *v, loff_t *ppos)
+{
+       ++*ppos;
+       return NULL;
+}
+
+static void setgroups_m_stop(struct seq_file *seq, void *v)
+{
+}
+
+static int setgroups_m_show(struct seq_file *seq, void *v)
+{
+       struct user_namespace *ns = seq->private;
+
+       seq_printf(seq, "%u\n", atomic_read(&ns->setgroups_allowed));
+       return 0;
+}
+
+const struct seq_operations proc_setgroups_seq_operations = {
+       .start  = setgroups_m_start,
+       .stop = setgroups_m_stop,
+       .next = setgroups_m_next,
+       .show = setgroups_m_show,
+};
+
+ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
+                            size_t count, loff_t *ppos)

arch/s390/kernel/compat_linux.c +22 -0

+
+       /* Only allow a very narrow range of strings to be written */
+       ret = -EINVAL;
+       if ((*ppos != 0) || (count >= sizeof(kbuf)) || (count < 1))
+               goto out;
+
+       /* What was written? */
+       ret = -EFAULT;
+       if (copy_from_user(kbuf, buf, count))
+               goto out;
+       kbuf[count] = '\0';
+
+       /* What is being requested? */
+       ret = -EINVAL;
+       if (kbuf[0] == '0')
+               setgroups_allowed = 0;
+       else if (kbuf[0] == '1')
+               setgroups_allowed = 1;
+       else
+               goto out;
+
+       /* Allow a trailing newline */

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

A value of 0 means the setgroups system call is disabled in the current processes user namespace and can not be enabled in the future in this user namespace.

A value of 1 means the segtoups system call is enabled.

arch/s390/kernel/compat_linux.c +34 -0

                }
        }
 

@@ -844,6 +850,93 @@ static bool new_idmap_permitted(const struct file *file,
        return false;
 }
 
+static void *setgroups_m_start(struct seq_file *seq, loff_t *ppos)
+{
+       struct user_namespace *ns = seq->private;
+
+       return (*ppos == 0) ?  ns : NULL;
+}
+
+static void *setgroups_m_next(struct seq_file *seq, void *v, loff_t *ppos)
+{
+       ++*ppos;
+       return NULL;
+}
+
+static void setgroups_m_stop(struct seq_file *seq, void *v)
+{
+}
+
+static int setgroups_m_show(struct seq_file *seq, void *v)
+{
+       struct user_namespace *ns = seq->private;
+
+       seq_printf(seq, "%u\n", atomic_read(&ns->setgroups_allowed));
+       return 0;
+}
+
+const struct seq_operations proc_setgroups_seq_operations = {
+       .start  = setgroups_m_start,
+       .stop = setgroups_m_stop,
+       .next = setgroups_m_next,
+       .show = setgroups_m_show,
+};
+
+ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
+                            size_t count, loff_t *ppos)

arch/s390/kernel/compat_linux.c +22 -0

+
+       /* Only allow a very narrow range of strings to be written */
+       ret = -EINVAL;
+       if ((*ppos != 0) || (count >= sizeof(kbuf)) || (count < 1))
+               goto out;
+
+       /* What was written? */
+       ret = -EFAULT;
+       if (copy_from_user(kbuf, buf, count))
+               goto out;
+       kbuf[count] = '\0';
+
+       /* What is being requested? */
+       ret = -EINVAL;
+       if (kbuf[0] == '0')
+               setgroups_allowed = 0;
+       else if (kbuf[0] == '1')
+               setgroups_allowed = 1;
+       else
+               goto out;
+
+       /* Allow a trailing newline */

Quoted 2 lines · Eric W. Biederman

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

Can you rename this to something clearer, e.g. userns_setgroups_mode?

Quoted 5 lines

A value of 0 means the setgroups system call is disabled in the current processes user namespace and can not be enabled in the future in this user namespace.

A value of 1 means the segtoups system call is enabled.

Would it make more sense to put strings like "allow" and "deny" in here? That way, future extensions could add additional values.

arch/s390/kernel/compat_linux.c COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis · line 257

Quoted 11 lines

arch/s390/kernel/compat_linux.c +1 -0

index 21c91feeca2d..6d0ee1b089fb 100644
@@ -252,6 +252,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
        int retval;
 
        if (!gid_mapping_possible(user_ns) ||
+           !atomic_read(&user_ns->setgroups_allowed) ||
            !capable(CAP_SETGID))
                return -EPERM;

This is now incomprehensible because of the gid_mapping_possible thing. If you renamed gid_mapping_possible to userns_setgroup_allowed, then this could be added to the implementation, and this would all make sense (not to mention avoiding duplicating this thing).

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 834

Quoted 9 lines

arch/s390/kernel/compat_linux.c +5 -0

@@ -826,6 +827,11 @@ static bool new_idmap_permitted(const struct file *file,
                        kuid_t uid = make_kuid(ns->parent, id);
                        if (uid_eq(uid, cred->euid))
                                return true;
+               } else if (cap_setid == CAP_SETGID) {
+                       kgid_t gid = make_kgid(ns->parent, id);
+                       if (!atomic_read(&ns->setgroups_allowed) &&
+                           gid_eq(gid, cred->egid))
+                               return true;

I still don't see why egid is any better than fsgid here.

Quoted 46 lines

arch/s390/kernel/compat_linux.c +39 -0

                }
        }
 

@@ -844,6 +850,93 @@ static bool new_idmap_permitted(const struct file *file,
        return false;
 }
 
+static void *setgroups_m_start(struct seq_file *seq, loff_t *ppos)
+{
+       struct user_namespace *ns = seq->private;
+
+       return (*ppos == 0) ?  ns : NULL;
+}
+
+static void *setgroups_m_next(struct seq_file *seq, void *v, loff_t *ppos)
+{
+       ++*ppos;
+       return NULL;
+}
+
+static void setgroups_m_stop(struct seq_file *seq, void *v)
+{
+}
+
+static int setgroups_m_show(struct seq_file *seq, void *v)
+{
+       struct user_namespace *ns = seq->private;
+
+       seq_printf(seq, "%u\n", atomic_read(&ns->setgroups_allowed));
+       return 0;
+}
+
+const struct seq_operations proc_setgroups_seq_operations = {
+       .start  = setgroups_m_start,
+       .stop = setgroups_m_stop,
+       .next = setgroups_m_next,
+       .show = setgroups_m_show,
+};
+
+ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
+                            size_t count, loff_t *ppos)
+{
+       struct seq_file *seq = file->private_data;
+       struct user_namespace *ns = seq->private;
+       char kbuf[3];
+       int setgroups_allowed;

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 896

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       ssize_t ret;
+
+       ret = -EPERM;
+       if (!file_ns_capable(file, ns, CAP_SETGID))
+               goto out;

CAP_SYS_ADMIN? This isn't setting a gid in the namespace; it's reconfiguring the namespace.

Quoted 26 lines

arch/s390/kernel/compat_linux.c +26 -0

+       /* Only allow a very narrow range of strings to be written */
+       ret = -EINVAL;
+       if ((*ppos != 0) || (count >= sizeof(kbuf)) || (count < 1))
+               goto out;
+
+       /* What was written? */
+       ret = -EFAULT;
+       if (copy_from_user(kbuf, buf, count))
+               goto out;
+       kbuf[count] = '\0';
+
+       /* What is being requested? */
+       ret = -EINVAL;
+       if (kbuf[0] == '0')
+               setgroups_allowed = 0;
+       else if (kbuf[0] == '1')
+               setgroups_allowed = 1;
+       else
+               goto out;
+
+       /* Allow a trailing newline */
+       ret = -EINVAL;
+       if ((count == 2) && (kbuf[1] != '\n'))
+               goto out;
+
+

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 927

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       if (setgroups_allowed) {
+               ret = -EINVAL;
+               if (atomic_read(&ns->setgroups_allowed) == 0)
+                       goto out;
+       } else {

I would disallow this if gid_map has been written in the interest of sanity.

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 929

Quoted 2 lines

arch/s390/kernel/compat_linux.c +2 -0

+               atomic_set(&ns->setgroups_allowed, 0);
+               /* sigh memory barriers! */

I don't think that any barriers are needed. If you ever observe setgroups_allowed == 0, it will stay that way forever.

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 941

Quoted 12 lines

arch/s390/kernel/compat_linux.c +9 -0

+       }
+
+       /* Report a successful write */
+       *ppos = count;
+       ret = count;
+out:
+       return ret;
+}
+
 static void *userns_get(struct task_struct *task)
 {
        struct user_namespace *user_ns;

--Andy

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Eric W. Biederman

Quoted 3 lines

Quoted 1 line

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

Can you rename this to something clearer, e.g. userns_setgroups_mode?

I am not certain that is any clearer. That just reads as wordier.

The userns bit is definitely confusing and wrong. Why should we need to spell out the scope?

Quoted 7 lines

Quoted 6 lines

A value of 0 means the setgroups system call is disabled in the current processes user namespace and can not be enabled in the future in this user namespace.

A value of 1 means the segtoups system call is enabled.

Quoted 2 lines

Would it make more sense to put strings like "allow" and "deny" in here? That way, future extensions could add additional values.

If the implementation of the write side isn't too bad. I would love
to see precedent elsewhere in the kernel. What I don't expect to do
is have any values except setgroups are enabled and setgroups are disabled.

I am fine with allowing for the possibility (that is just good engineering) but I don't intend to seriously considering or implementing other possibilities.

Quoted 19 lines

arch/s390/kernel/compat_linux.c COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis · line 257

Quoted 12 lines

arch/s390/kernel/compat_linux.c +1 -0

index 21c91feeca2d..6d0ee1b089fb 100644
@@ -252,6 +252,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
        int retval;
 
        if (!gid_mapping_possible(user_ns) ||
+           !atomic_read(&user_ns->setgroups_allowed) ||
            !capable(CAP_SETGID))
                return -EPERM;

This is now incomprehensible because of the gid_mapping_possible thing. If you renamed gid_mapping_possible to userns_setgroup_allowed, then this could be added to the implementation, and this would all make sense (not to mention avoiding duplicating this thing).

Quoted 11 lines

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 834

Quoted 9 lines

arch/s390/kernel/compat_linux.c +5 -0

@@ -826,6 +827,11 @@ static bool new_idmap_permitted(const struct file *file,
                        kuid_t uid = make_kuid(ns->parent, id);
                        if (uid_eq(uid, cred->euid))
                                return true;
+               } else if (cap_setid == CAP_SETGID) {
+                       kgid_t gid = make_kgid(ns->parent, id);
+                       if (!atomic_read(&ns->setgroups_allowed) &&
+                           gid_eq(gid, cred->egid))
+                               return true;

I still don't see why egid is any better than fsgid here.

Answered in my earlier response fsgid was a goof.
I can set any gid to my egid with my existing permissions. Show me how I can do that with fsgid or fsuid and I will be happy to use those.

Quoted 8 lines

Quoted 8 lines

arch/s390/kernel/compat_linux.c +8 -0

+
+ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
+                            size_t count, loff_t *ppos)
+{
+       struct seq_file *seq = file->private_data;
+       struct user_namespace *ns = seq->private;
+       char kbuf[3];
+       int setgroups_allowed;

Quoted 8 lines

arch/s390/kernel/compat_linux.c

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       ssize_t ret;
+
+       ret = -EPERM;
+       if (!file_ns_capable(file, ns, CAP_SETGID))
+               goto out;

CAP_SYS_ADMIN? This isn't setting a gid in the namespace; it's reconfiguring the namespace.

Hmm. Maybe. It is an activity that is normally controlled by CAP_SETGID.

Frankly I think the entire split up of the capability model is almost totally broken. But I think CAP_SETGID is a close approximation of the right thing here.

Quoted 27 lines

Quoted 27 lines

arch/s390/kernel/compat_linux.c +26 -0

+       /* Only allow a very narrow range of strings to be written */
+       ret = -EINVAL;
+       if ((*ppos != 0) || (count >= sizeof(kbuf)) || (count < 1))
+               goto out;
+
+       /* What was written? */
+       ret = -EFAULT;
+       if (copy_from_user(kbuf, buf, count))
+               goto out;
+       kbuf[count] = '\0';
+
+       /* What is being requested? */
+       ret = -EINVAL;
+       if (kbuf[0] == '0')
+               setgroups_allowed = 0;
+       else if (kbuf[0] == '1')
+               setgroups_allowed = 1;
+       else
+               goto out;
+
+       /* Allow a trailing newline */
+       ret = -EINVAL;
+       if ((count == 2) && (kbuf[1] != '\n'))
+               goto out;
+
+

Quoted 8 lines

arch/s390/kernel/compat_linux.c

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       if (setgroups_allowed) {
+               ret = -EINVAL;
+               if (atomic_read(&ns->setgroups_allowed) == 0)
+                       goto out;
+       } else {

I would disallow this if gid_map has been written in the interest of sanity.

Not a chance. That is part of making this an independent knob. If there is another reason for disabling setgroups you can flip this knob even after mappings are established.

Quoted 4 lines

Quoted 3 lines

arch/s390/kernel/compat_linux.c +2 -0

+               atomic_set(&ns->setgroups_allowed, 0);
+               /* sigh memory barriers! */

Quoted 2 lines

I don't think that any barriers are needed. If you ever observe setgroups_allowed == 0, it will stay that way forever.

Likely. In practice the code works today.

But I need to review things closely to understand if there are barriers needed. But especially since it is a write once knob we can get away with a lot.

Quoted 5 lines · Eric W. Biederman

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Eric W. Biederman

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

Can you rename this to something clearer, e.g. userns_setgroups_mode?

I am not certain that is any clearer. That just reads as wordier.

The userns bit is definitely confusing and wrong. Why should we need to spell out the scope?

Because it's a property of the process' userns, not a property of the process.

userns_setgroups would be okay. (Arguably it should've been userns_uid_map, too, but at least uid_map sounds obviously namespace-related.)

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

A value of 0 means the setgroups system call is disabled in the current processes user namespace and can not be enabled in the future in this user namespace.

A value of 1 means the segtoups system call is enabled.

Quoted 7 lines

Quoted 2 lines

Would it make more sense to put strings like "allow" and "deny" in here? That way, future extensions could add additional values.

If the implementation of the write side isn't too bad. I would love
to see precedent elsewhere in the kernel. What I don't expect to do
is have any values except setgroups are enabled and setgroups are disabled.

current_clocksource? I think there are lots of things like that.

Quoted 1 line

Quoted 3 lines

I am fine with allowing for the possibility (that is just good engineering) but I don't intend to seriously considering or implementing other possibilities.

I can imagine a mode where there's a fixed set of groups that are forced set but other groups can be added and dropped. It would be complicated to set up right, but someone might want it some day.

Quoted 30 lines

Quoted 29 lines

arch/s390/kernel/compat_linux.c COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis · line 257

Quoted 11 lines

arch/s390/kernel/compat_linux.c +1 -0

index 21c91feeca2d..6d0ee1b089fb 100644
@@ -252,6 +252,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
        int retval;
 
        if (!gid_mapping_possible(user_ns) ||
+           !atomic_read(&user_ns->setgroups_allowed) ||
            !capable(CAP_SETGID))
                return -EPERM;

This is now incomprehensible because of the gid_mapping_possible thing. If you renamed gid_mapping_possible to userns_setgroup_allowed, then this could be added to the implementation, and this would all make sense (not to mention avoiding duplicating this thing).

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 834

Quoted 9 lines

arch/s390/kernel/compat_linux.c +5 -0

@@ -826,6 +827,11 @@ static bool new_idmap_permitted(const struct file *file,
                        kuid_t uid = make_kuid(ns->parent, id);
                        if (uid_eq(uid, cred->euid))
                                return true;
+               } else if (cap_setid == CAP_SETGID) {
+                       kgid_t gid = make_kgid(ns->parent, id);
+                       if (!atomic_read(&ns->setgroups_allowed) &&
+                           gid_eq(gid, cred->egid))
+                               return true;

I still don't see why egid is any better than fsgid here.

Quoted 4 lines

Answered in my earlier response fsgid was a goof.
I can set any gid to my egid with my existing permissions. Show me how I can do that with fsgid or fsuid and I will be happy to use those.

You can use your fsgid to make a setgid file, I think. But yes, point taken, although as mentioned in the other thread, I think it would be a lot clearer if it were a separate patch.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

arch/s390/kernel/compat_linux.c +7 -0

+ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
+                            size_t count, loff_t *ppos)
+{
+       struct seq_file *seq = file->private_data;
+       struct user_namespace *ns = seq->private;
+       char kbuf[3];
+       int setgroups_allowed;

Quoted 8 lines

arch/s390/kernel/compat_linux.c

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       ssize_t ret;
+
+       ret = -EPERM;
+       if (!file_ns_capable(file, ns, CAP_SETGID))
+               goto out;

CAP_SYS_ADMIN? This isn't setting a gid in the namespace; it's reconfiguring the namespace.

Quoted 6 lines

Hmm. Maybe. It is an activity that is normally controlled by CAP_SETGID.

Frankly I think the entire split up of the capability model is almost totally broken. But I think CAP_SETGID is a close approximation of the right thing here.

I agree that the cap model is screwy. But we use CAP_SYS_ADMIN for everything else that changes the overall behavior of a namespace.

In any event, the only way it matters is for a non-ns owner in the parent ns with CAP_SETGID set but not CAP_SYS_ADMIN. I'd argue that CAP_SETGID should not be usable to make unrelated process' syscalls fail.

Quoted 35 lines

Quoted 26 lines

Quoted 26 lines

arch/s390/kernel/compat_linux.c +26 -0

+       /* Only allow a very narrow range of strings to be written */
+       ret = -EINVAL;
+       if ((*ppos != 0) || (count >= sizeof(kbuf)) || (count < 1))
+               goto out;
+
+       /* What was written? */
+       ret = -EFAULT;
+       if (copy_from_user(kbuf, buf, count))
+               goto out;
+       kbuf[count] = '\0';
+
+       /* What is being requested? */
+       ret = -EINVAL;
+       if (kbuf[0] == '0')
+               setgroups_allowed = 0;
+       else if (kbuf[0] == '1')
+               setgroups_allowed = 1;
+       else
+               goto out;
+
+       /* Allow a trailing newline */
+       ret = -EINVAL;
+       if ((count == 2) && (kbuf[1] != '\n'))
+               goto out;
+
+

Quoted 8 lines

arch/s390/kernel/compat_linux.c

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       if (setgroups_allowed) {
+               ret = -EINVAL;
+               if (atomic_read(&ns->setgroups_allowed) == 0)
+                       goto out;
+       } else {

I would disallow this if gid_map has been written in the interest of sanity.

Quoted 3 lines

Not a chance. That is part of making this an independent knob. If there is another reason for disabling setgroups you can flip this knob even after mappings are established.

Then you really want CAP_SYS_ADMIN, I think.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

arch/s390/kernel/compat_linux.c +2 -0

+               atomic_set(&ns->setgroups_allowed, 0);
+               /* sigh memory barriers! */

Quoted 9 lines

Quoted 2 lines

I don't think that any barriers are needed. If you ever observe setgroups_allowed == 0, it will stay that way forever.

Likely. In practice the code works today.

But I need to review things closely to understand if there are barriers needed. But especially since it is a write once knob we can get away with a lot.

Yeah.

For long-term use, I kind of like the flags approach I added in the other patch. It makes it easy to add more flags in the future.

In any event, I think the only barrier that's needed is when writing gid_map:

atomic_read / test_bit to determine whether unpriv mappings are okay; smp_mb() or whatever the current _after_atomic thing is these days; write mapping;

Although I'm not sure whether Linux supports any architectures that can violate causality in the way that barrier is there to prevent.

Quoted 39 lines · Andy Lutomirski

Quoted 5 lines · Eric W. Biederman

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Eric W. Biederman

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

Can you rename this to something clearer, e.g. userns_setgroups_mode?

I am not certain that is any clearer. That just reads as wordier.

The userns bit is definitely confusing and wrong. Why should we need to spell out the scope?

Because it's a property of the process' userns, not a property of the process.

userns_setgroups would be okay. (Arguably it should've been userns_uid_map, too, but at least uid_map sounds obviously namespace-related.)

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

A value of 0 means the setgroups system call is disabled in the current processes user namespace and can not be enabled in the future in this user namespace.

A value of 1 means the segtoups system call is enabled.

Quoted 7 lines

Quoted 2 lines

Would it make more sense to put strings like "allow" and "deny" in here? That way, future extensions could add additional values.

If the implementation of the write side isn't too bad. I would love
to see precedent elsewhere in the kernel. What I don't expect to do
is have any values except setgroups are enabled and setgroups are disabled.

current_clocksource? I think there are lots of things like that.

Quoted 1 line

Quoted 7 lines

Quoted 3 lines

I am fine with allowing for the possibility (that is just good engineering) but I don't intend to seriously considering or implementing other possibilities.

I can imagine a mode where there's a fixed set of groups that are forced set but other groups can be added and dropped. It would be complicated to set up right, but someone might want it some day.

Yeah. I am defintiely not designing for that.

Quoted 30 lines

Quoted 30 lines

Quoted 29 lines

arch/s390/kernel/compat_linux.c COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis · line 257

Quoted 11 lines

arch/s390/kernel/compat_linux.c +1 -0

index 21c91feeca2d..6d0ee1b089fb 100644
@@ -252,6 +252,7 @@ COMPAT_SYSCALL_DEFINE2(s390_setgroups16, int, gidsetsize, u16 __user *, grouplis
        int retval;
 
        if (!gid_mapping_possible(user_ns) ||
+           !atomic_read(&user_ns->setgroups_allowed) ||
            !capable(CAP_SETGID))
                return -EPERM;

This is now incomprehensible because of the gid_mapping_possible thing. If you renamed gid_mapping_possible to userns_setgroup_allowed, then this could be added to the implementation, and this would all make sense (not to mention avoiding duplicating this thing).

arch/s390/kernel/compat_linux.c static bool new_idmap_permitted(const struct file *file, · line 834

Quoted 9 lines

arch/s390/kernel/compat_linux.c +5 -0

@@ -826,6 +827,11 @@ static bool new_idmap_permitted(const struct file *file,
                        kuid_t uid = make_kuid(ns->parent, id);
                        if (uid_eq(uid, cred->euid))
                                return true;
+               } else if (cap_setid == CAP_SETGID) {
+                       kgid_t gid = make_kgid(ns->parent, id);
+                       if (!atomic_read(&ns->setgroups_allowed) &&
+                           gid_eq(gid, cred->egid))
+                               return true;

I still don't see why egid is any better than fsgid here.

Quoted 8 lines

Quoted 4 lines

Answered in my earlier response fsgid was a goof.
I can set any gid to my egid with my existing permissions. Show me how I can do that with fsgid or fsuid and I will be happy to use those.

You can use your fsgid to make a setgid file, I think. But yes, point taken, although as mentioned in the other thread, I think it would be a lot clearer if it were a separate patch.

Agreed.

Quoted 23 lines

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

arch/s390/kernel/compat_linux.c +7 -0

+ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
+                            size_t count, loff_t *ppos)
+{
+       struct seq_file *seq = file->private_data;
+       struct user_namespace *ns = seq->private;
+       char kbuf[3];
+       int setgroups_allowed;

Quoted 8 lines

arch/s390/kernel/compat_linux.c

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       ssize_t ret;
+
+       ret = -EPERM;
+       if (!file_ns_capable(file, ns, CAP_SETGID))
+               goto out;

CAP_SYS_ADMIN? This isn't setting a gid in the namespace; it's reconfiguring the namespace.

Quoted 6 lines

Hmm. Maybe. It is an activity that is normally controlled by CAP_SETGID.

Frankly I think the entire split up of the capability model is almost totally broken. But I think CAP_SETGID is a close approximation of the right thing here.

Quoted 7 lines

I agree that the cap model is screwy. But we use CAP_SYS_ADMIN for everything else that changes the overall behavior of a namespace.

In any event, the only way it matters is for a non-ns owner in the parent ns with CAP_SETGID set but not CAP_SYS_ADMIN. I'd argue that CAP_SETGID should not be usable to make unrelated process' syscalls fail.

That is a pretty decent argument. I will take a good stare at this
issue as I refresh these patches and see how close to perfection I can make them.

Quoted 55 lines

Quoted 35 lines

Quoted 26 lines

Quoted 26 lines

arch/s390/kernel/compat_linux.c +26 -0

+       /* Only allow a very narrow range of strings to be written */
+       ret = -EINVAL;
+       if ((*ppos != 0) || (count >= sizeof(kbuf)) || (count < 1))
+               goto out;
+
+       /* What was written? */
+       ret = -EFAULT;
+       if (copy_from_user(kbuf, buf, count))
+               goto out;
+       kbuf[count] = '\0';
+
+       /* What is being requested? */
+       ret = -EINVAL;
+       if (kbuf[0] == '0')
+               setgroups_allowed = 0;
+       else if (kbuf[0] == '1')
+               setgroups_allowed = 1;
+       else
+               goto out;
+
+       /* Allow a trailing newline */
+       ret = -EINVAL;
+       if ((count == 2) && (kbuf[1] != '\n'))
+               goto out;
+
+

Quoted 8 lines

arch/s390/kernel/compat_linux.c

Quoted 5 lines

arch/s390/kernel/compat_linux.c +5 -0

+       if (setgroups_allowed) {
+               ret = -EINVAL;
+               if (atomic_read(&ns->setgroups_allowed) == 0)
+                       goto out;
+       } else {

I would disallow this if gid_map has been written in the interest of sanity.

Quoted 3 lines

Not a chance. That is part of making this an independent knob. If there is another reason for disabling setgroups you can flip this knob even after mappings are established.

Then you really want CAP_SYS_ADMIN, I think.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

arch/s390/kernel/compat_linux.c +2 -0

+               atomic_set(&ns->setgroups_allowed, 0);
+               /* sigh memory barriers! */

Quoted 9 lines

Quoted 2 lines

I don't think that any barriers are needed. If you ever observe setgroups_allowed == 0, it will stay that way forever.

Likely. In practice the code works today.

But I need to review things closely to understand if there are barriers needed. But especially since it is a write once knob we can get away with a lot.

Quoted 4 lines

Yeah.

For long-term use, I kind of like the flags approach I added in the other patch. It makes it easy to add more flags in the future.

I thought a dedicated word might be easier. I don't know that it makes much of a difference at this point.

Quoted 8 lines

In any event, I think the only barrier that's needed is when writing gid_map:

atomic_read / test_bit to determine whether unpriv mappings are okay; smp_mb() or whatever the current _after_atomic thing is these days; write mapping;

Although I'm not sure whether Linux supports any architectures that can violate causality in the way that barrier is there to prevent.

The DEC Alpha at least used to be able to violate causality in ways weird enough to confuse anyone, and the alpha still seems to be in the tree. What keyed me in was the part in atomic_ops.txt that says these things don't have barriers and you will probably need them, and I remember spin locks tend to take care of all those issues for you.

Anyway thanks for your barrier pointer.

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 

include/linux/user_namespace.h +1 -0

+
 #ifdef CONFIG_USER_NS
 
 static inline struct user_namespace *get_user_ns(struct user_namespace *ns)

kernel/groups.c +6 -2

index 02d8a251c476..e0335e44f76a 100644
@@ -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 */

@@ -217,7 +218,11 @@ bool may_setgroups(void)
 {
        struct user_namespace *user_ns = current_user_ns();
 
-       return ns_capable(user_ns, CAP_SETGID);
+       /* It is not safe to use setgroups until a gid mapping in
+        * the user namespace has been established.
+        */
+       return userns_gid_mappings_established(user_ns) &&
+               ns_capable(user_ns, CAP_SETGID);
 }
 
 /*
--

1.9.1

Quoted 27 lines · Eric W. Biederman

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;
 

include/linux/user_namespace.h struct user_namespace { · line 47

Quoted 8 lines

include/linux/user_namespace.h +8 -0

+static inline bool userns_gid_mappings_established(const struct user_namespace *ns)
+{
+       bool established;
+       smp_mb__before_atomic();
+       established = ACCESS_ONCE(ns->gid_map.nr_extents) != 0;
+       smp_mb__after_atomic();
+       return established;
+}

I don't think this works on all platforms. ACCESS_ONCE is not atomic in the smp_mb__before_atomic sense.

Quoted 30 lines

include/linux/user_namespace.h +1 -0

+
 #ifdef CONFIG_USER_NS
 
 static inline struct user_namespace *get_user_ns(struct user_namespace *ns)

kernel/groups.c +6 -2

index 02d8a251c476..e0335e44f76a 100644
@@ -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 */

@@ -217,7 +218,11 @@ bool may_setgroups(void)
 {
        struct user_namespace *user_ns = current_user_ns();
 
-       return ns_capable(user_ns, CAP_SETGID);
+       /* It is not safe to use setgroups until a gid mapping in
+        * the user namespace has been established.
+        */
+       return userns_gid_mappings_established(user_ns) &&
+               ns_capable(user_ns, CAP_SETGID);
 }
 
 /*
--

Quoted 2 lines

1.9.1

--Andy

Quoted 28 lines · Andy Lutomirski

Quoted 27 lines · Eric W. Biederman

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;
 

Quoted 11 lines

include/linux/user_namespace.h struct user_namespace { · line 47

Quoted 8 lines

include/linux/user_namespace.h +8 -0

+static inline bool userns_gid_mappings_established(const struct user_namespace *ns)
+{
+       bool established;
+       smp_mb__before_atomic();
+       established = ACCESS_ONCE(ns->gid_map.nr_extents) != 0;
+       smp_mb__after_atomic();
+       return established;
+}

I don't think this works on all platforms. ACCESS_ONCE is not atomic in the smp_mb__before_atomic sense.

Documentation/atomic_ops.txt documents ACCESS_ONCE as being equivalent to atomic_read() and atomic_set(). smp_mb__before_atomic and smp_mb__after_atomic() are Documented as working with atomic_read and atomic_set. Maybe it is a stretch to use them but it doesn't seem like much of a stretch.

Further at this point I don't know that any barriers are strictly needed, beyond the ACCESS_ONCE. However since x86 does all of the ordering in hardware that I need I am not going to find any bugs that don't require a barrier.

All I really want is the same level of barriers I would get if I used a spin-lock protected data structure so I don't need to worry about crazy smp issues that happen when the hardware decides it is safe to reorder things.

Eric

Quoted 34 lines

Quoted 30 lines

include/linux/user_namespace.h +1 -0

+
 #ifdef CONFIG_USER_NS
 
 static inline struct user_namespace *get_user_ns(struct user_namespace *ns)

kernel/groups.c +6 -2

index 02d8a251c476..e0335e44f76a 100644
@@ -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 */

@@ -217,7 +218,11 @@ bool may_setgroups(void)
 {
        struct user_namespace *user_ns = current_user_ns();
 
-       return ns_capable(user_ns, CAP_SETGID);
+       /* It is not safe to use setgroups until a gid mapping in
+        * the user namespace has been established.
+        */
+       return userns_gid_mappings_established(user_ns) &&
+               ns_capable(user_ns, CAP_SETGID);
 }
 
 /*
--

Quoted 2 lines

1.9.1

--Andy

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;

Am 08.12.2014 um 23:07 schrieb Eric W. Biederman:

Quoted 27 lines

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;
 

include/linux/user_namespace.h struct user_namespace { · line 48

Quoted 9 lines

include/linux/user_namespace.h +9 -0

+static inline bool userns_gid_mappings_established(const struct user_namespace *ns)
+{
+     bool established;
+     smp_mb__before_atomic();
+     established = ACCESS_ONCE(ns->gid_map.nr_extents) != 0;
+     smp_mb__after_atomic();
+     return established;
+}
+

Maybe this is a stupid question, but why do we need all this magic around established = ... ?
The purpose of this code is to check whether ns->gid_map.nr_extents != 0 in a lock-free manner?

Thanks,
//richard

Quoted 28 lines

Am 08.12.2014 um 23:07 schrieb Eric W. Biederman:

Quoted 27 lines

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;
 

Quoted 15 lines

include/linux/user_namespace.h struct user_namespace { · line 48

Quoted 9 lines

include/linux/user_namespace.h +9 -0

+static inline bool userns_gid_mappings_established(const struct user_namespace *ns)
+{
+     bool established;
+     smp_mb__before_atomic();
+     established = ACCESS_ONCE(ns->gid_map.nr_extents) != 0;
+     smp_mb__after_atomic();
+     return established;
+}
+

Maybe this is a stupid question, but why do we need all this magic around established = ... ?
The purpose of this code is to check whether ns->gid_map.nr_extents != 0 in a lock-free manner?

See my other comment -- the ordering will matter at the end of the series.

Am 08.12.2014 um 23:25 schrieb Andy Lutomirski:

Quoted 45 lines · Richard Weinberger

Quoted 43 lines

Am 08.12.2014 um 23:07 schrieb Eric W. Biederman:

Quoted 26 lines

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;

include/linux/user_namespace.h struct user_namespace { · line 48

Quoted 10 lines

include/linux/user_namespace.h +9 -0

 
+static inline bool userns_gid_mappings_established(const struct user_namespace *ns)
+{
+     bool established;
+     smp_mb__before_atomic();
+     established = ACCESS_ONCE(ns->gid_map.nr_extents) != 0;
+     smp_mb__after_atomic();
+     return established;
+}
+

Maybe this is a stupid question, but why do we need all this magic around established = ... ?
The purpose of this code is to check whether ns->gid_map.nr_extents != 0 in a lock-free manner?

Quoted 1 line

See my other comment -- the ordering will matter at the end of the series.

But ns->gid_map.nr_extents is not atomic, it is a plain u32. This confuses me.

Quoted 46 lines · Richard Weinberger

Am 08.12.2014 um 23:25 schrieb Andy Lutomirski:

Quoted 45 lines · Richard Weinberger

Quoted 43 lines

Am 08.12.2014 um 23:07 schrieb Eric W. Biederman:

Quoted 26 lines

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;

include/linux/user_namespace.h struct user_namespace { · line 48

Quoted 10 lines

include/linux/user_namespace.h +9 -0

 
+static inline bool userns_gid_mappings_established(const struct user_namespace *ns)
+{
+     bool established;
+     smp_mb__before_atomic();
+     established = ACCESS_ONCE(ns->gid_map.nr_extents) != 0;
+     smp_mb__after_atomic();
+     return established;
+}
+

Maybe this is a stupid question, but why do we need all this magic around established = ... ?
The purpose of this code is to check whether ns->gid_map.nr_extents != 0 in a lock-free manner?

Quoted 4 lines

Quoted 1 line

See my other comment -- the ordering will matter at the end of the series.

But ns->gid_map.nr_extents is not atomic, it is a plain u32. This confuses me.

Read Documentation/atomic_ops.txt a plain u32 is atomic by definiton.

Which is a little bit convoluted. However that is part of the of the gid mapping path and I optimized that as far as I humanly could so that calls like stat don't take a noticable slow donw.

On this path we don't particularly care except that I am using an the existing data structure.

Eric

Quoted 28 lines

Am 08.12.2014 um 23:07 schrieb Eric W. Biederman:

Quoted 27 lines

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;
 

Quoted 15 lines

include/linux/user_namespace.h struct user_namespace { · line 48

Quoted 9 lines

include/linux/user_namespace.h +9 -0

+static inline bool userns_gid_mappings_established(const struct user_namespace *ns)
+{
+     bool established;
+     smp_mb__before_atomic();
+     established = ACCESS_ONCE(ns->gid_map.nr_extents) != 0;
+     smp_mb__after_atomic();
+     return established;
+}
+

Maybe this is a stupid question, but why do we need all this magic around established = ... ?
The purpose of this code is to check whether ns->gid_map.nr_extents != 0 in a lock-free manner?

See my other comment -- the ordering will matter at the end of the series.

setgroups is unique in not needing a valid mapping before it can be called, in the case of setgroups(0, NULL) which drops all supplemental groups.

The design of the user namespace assumes that CAP_SETGID can not actually be used until a gid mapping is established. Therefore add a helper function to see if the user namespace gid mapping has been established and call that function in the setgroups permission check.

This is part of the fix for CVE-2014-8989, being able to drop groups without privilege using user namespaces.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h | 9 +++++++++
 kernel/groups.c                | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

include/linux/user_namespace.h +0 -0

index e95372654f09..41cc26e5a350 100644
@@ -37,6 +37,15 @@ struct user_namespace {
 
 extern struct user_namespace init_user_ns;

setresuid allows the euid to be set to any of uid, euid, suid, and fsuid. Therefor it is safe to allow an unprivileged user to map their euid and use CAP_SETUID privileged with exactly that uid, as no new credentials can be obtained.

I can not find a combination of existing system calls that allows setting uid, euid, suid, and fsuid from the fsuid making the previous use of fsuid for allowing unprivileged mappings a bug.

This is part of a fix for CVE-2014-8989.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/user_namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/user_namespace.c +1 -2

index 8e7c87162171..da1eeb927b21 100644
@@ -819,7 +819,7 @@ static bool new_idmap_permitted(const struct file *file,
                u32 id = new_map->extent[0].lower_first;
                if (cap_setid == CAP_SETUID) {
                        kuid_t uid = make_kuid(ns->parent, id);
-                       if (uid_eq(uid, file->f_cred->fsuid))
+                       if (uid_eq(uid, file->f_cred->euid))
                                return true;
                }
        }
--

1.9.1

setresuid allows the euid to be set to any of uid, euid, suid, and fsuid. Therefor it is safe to allow an unprivileged user to map their euid and use CAP_SETUID privileged with exactly that uid, as no new credentials can be obtained.

I can not find a combination of existing system calls that allows setting uid, euid, suid, and fsuid from the fsuid making the previous use of fsuid for allowing unprivileged mappings a bug.

This is part of a fix for CVE-2014-8989.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 kernel/user_namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/user_namespace.c +1 -2

index 8e7c87162171..da1eeb927b21 100644
@@ -819,7 +819,7 @@ static bool new_idmap_permitted(const struct file *file,
                u32 id = new_map->extent[0].lower_first;
                if (cap_setid == CAP_SETUID) {
                        kuid_t uid = make_kuid(ns->parent, id);
-                       if (uid_eq(uid, file->f_cred->fsuid))
+                       if (uid_eq(uid, file->f_cred->euid))
                                return true;
                }
        }
--

1.9.1

If you did not create the user namespace and are allowed to write to uid_map or gid_map you should already have the necessary privilege in the parent user namespace to establish any mapping you want so this will not affect userspace in practice.

Limiting unprivileged uid mapping establishment to the creator of the user namespace reduces the set of credentials that must be verified can be obtained without privielge, making code verification simpler.

Limiting unprivileged gid mapping establishment (which is temporarily absent) to the creator of the user namespace also ensures that the combination of uid and gid can already be obtained without privilege.

This is part of the fix for CVE-2014-8989.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 kernel/user_namespace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/user_namespace.c +4 -3

index da1eeb927b21..413f60fd5983 100644
@@ -812,14 +812,16 @@ static bool new_idmap_permitted(const struct file *file,
                                struct user_namespace *ns, int cap_setid,
                                struct uid_gid_map *new_map)
 {
+       const struct cred *cred = file->f_cred;
        /* Don't allow mappings that would allow anything that wouldn't
         * be allowed without the establishment of unprivileged mappings.
         */
-       if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) {
+       if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
+           uid_eq(ns->owner, cred->euid)) {
                u32 id = new_map->extent[0].lower_first;
                if (cap_setid == CAP_SETUID) {
                        kuid_t uid = make_kuid(ns->parent, id);
-                       if (uid_eq(uid, file->f_cred->euid))
+                       if (uid_eq(uid, cred->euid))
                                return true;
                }
        }
--

1.9.1

If you did not create the user namespace and are allowed to write to uid_map or gid_map you should already have the necessary privilege in the parent user namespace to establish any mapping you want so this will not affect userspace in practice.

Limiting unprivileged uid mapping establishment to the creator of the user namespace reduces the set of credentials that must be verified can be obtained without privielge, making code verification simpler.

Limiting unprivileged gid mapping establishment (which is temporarily absent) to the creator of the user namespace also ensures that the combination of uid and gid can already be obtained without privilege.

This is part of the fix for CVE-2014-8989.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 kernel/user_namespace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/user_namespace.c +4 -3

index da1eeb927b21..413f60fd5983 100644
@@ -812,14 +812,16 @@ static bool new_idmap_permitted(const struct file *file,
                                struct user_namespace *ns, int cap_setid,
                                struct uid_gid_map *new_map)
 {
+       const struct cred *cred = file->f_cred;
        /* Don't allow mappings that would allow anything that wouldn't
         * be allowed without the establishment of unprivileged mappings.
         */
-       if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) {
+       if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
+           uid_eq(ns->owner, cred->euid)) {
                u32 id = new_map->extent[0].lower_first;
                if (cap_setid == CAP_SETUID) {
                        kuid_t uid = make_kuid(ns->parent, id);
-                       if (uid_eq(uid, file->f_cred->euid))
+                       if (uid_eq(uid, cred->euid))
                                return true;
                }
        }
--

1.9.1

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

A value of 0 means the setgroups system call is disabled in the

current processes user namespace and can not be enabled in the future in this user namespace.

A value of 1 means the segtoups system call is enabled.

- Descedent user namespaces inherit the value of setgroups from

Quoted 4 lines · Eric W. Biederman

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

A value of 0 means the setgroups system call is disabled in the

"deny"

Quoted 3 lines

current processes user namespace and can not be enabled in the future in this user namespace.

Quoted 2 lines

A value of 1 means the segtoups system call is enabled.

"allow"

Quoted 1 line

- Descedent user namespaces inherit the value of setgroups from

s/Descedent/Descendent/

kernel/groups.c bool may_setgroups(void) · line 227

Quoted 9 lines

kernel/groups.c +1 -0

@@ -222,6 +222,7 @@ bool may_setgroups(void)
         * the user namespace has been established.
         */
        return userns_gid_mappings_established(user_ns) &&
+               userns_setgroups_allowed(user_ns) &&
                ns_capable(user_ns, CAP_SETGID);
 }

Can you add a comment explaining the ordering? For example:

We need to check for a gid mapping before checking setgroups_allowed because an unprivileged user can create a userns with setgroups allowed, then disallow setgroups and add a mapping. If we check in the opposite order, then we have a race: we could see that setgroups is allowed before the user clears the bit and then see that there is a gid mapping after the other thread is done.

Quoted 11 lines · Andy Lutomirski

Quoted 4 lines · Eric W. Biederman

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

A value of 0 means the setgroups system call is disabled in the

"deny"

Quoted 3 lines

current processes user namespace and can not be enabled in the future in this user namespace.

Quoted 8 lines

Quoted 2 lines

A value of 1 means the segtoups system call is enabled.

"allow"

Quoted 1 line

- Descedent user namespaces inherit the value of setgroups from

s/Descedent/Descendent/

Bah. I updated everything but the changelog comment.

Quoted 11 lines

kernel/groups.c bool may_setgroups(void) · line 227

Quoted 9 lines

kernel/groups.c +1 -0

@@ -222,6 +222,7 @@ bool may_setgroups(void)
         * the user namespace has been established.
         */
        return userns_gid_mappings_established(user_ns) &&
+               userns_setgroups_allowed(user_ns) &&
                ns_capable(user_ns, CAP_SETGID);
 }

Can you add a comment explaining the ordering? For example:

I need to think on what I can say to make it clear.
Perhaps: /* Careful the order of these checks is important. */

Quoted 6 lines

We need to check for a gid mapping before checking setgroups_allowed because an unprivileged user can create a userns with setgroups allowed, then disallow setgroups and add a mapping. If we check in the opposite order, then we have a race: we could see that setgroups is allowed before the user clears the bit and then see that there is a gid mapping after the other thread is done.

Since these are independent atomic variables yes that ordering issue seems to be the case.

For me it was the natural ordering of the checks so I didn't even bother to think about what happens when you reorder them.

Eric

Quoted 36 lines · Eric W. Biederman

Quoted 11 lines · Andy Lutomirski

Quoted 4 lines · Eric W. Biederman

- Expose the knob to user space through a proc file /proc/<pid>/setgroups

A value of 0 means the setgroups system call is disabled in the

"deny"

Quoted 3 lines

current processes user namespace and can not be enabled in the future in this user namespace.

Quoted 8 lines

Quoted 2 lines

A value of 1 means the segtoups system call is enabled.

"allow"

Quoted 1 line

- Descedent user namespaces inherit the value of setgroups from

s/Descedent/Descendent/

Bah. I updated everything but the changelog comment.

Quoted 11 lines

kernel/groups.c bool may_setgroups(void) · line 227

Quoted 9 lines

kernel/groups.c +1 -0

@@ -222,6 +222,7 @@ bool may_setgroups(void)
         * the user namespace has been established.
         */
        return userns_gid_mappings_established(user_ns) &&
+               userns_setgroups_allowed(user_ns) &&
                ns_capable(user_ns, CAP_SETGID);
 }

Can you add a comment explaining the ordering? For example:

Quoted 10 lines

I need to think on what I can say to make it clear.
Perhaps: /* Careful the order of these checks is important. */

Quoted 6 lines

We need to check for a gid mapping before checking setgroups_allowed because an unprivileged user can create a userns with setgroups allowed, then disallow setgroups and add a mapping. If we check in the opposite order, then we have a race: we could see that setgroups is allowed before the user clears the bit and then see that there is a gid mapping after the other thread is done.

This text was actually my suggested comment text.

If you put smp_rmb() in this function with a comment like that, then I think it will all make sense and be obviously correct (even with most of the other barriers removed).

Quoted 2 lines · Andy Lutomirski

This text was actually my suggested comment text.

Now I see.

Quoted 3 lines

If you put smp_rmb() in this function with a comment like that, then I think it will all make sense and be obviously correct (even with most of the other barriers removed).

Right.

Given that we have to be careful when using these things anyway what I was hoping to achieve with the barriers appears impossible, and confusing so I will see about just adding barriers where we need them for real. Sigh.