isolation: Don't create our userns as nobody

In isolate_user(), we set[ug]id() to our final UID/GID, then join or
create our userns.  This makes sense when we're going to use an already
existing userns, that is, with the --userns option, a PID passed to pasta
or with --netns-only (we use our current userns).  However, it causes a
problem when we create our own new userns.

The set[ug]id() is most often a no-op, but when it isn't it's generally a
switch from root to nobody.  If we create our userns after this, we will
create it owned by nobody.  That means that anything which can operate as
nobody can enter our userns with full capabilities, potentially allowing
it to attack a running passt or pasta.  This sort of attach is unlikely
in practice: something else on the system must have a security defect to
allow arbitrary actions as nobody, and even then it will probably be caught
by LSMs.

Still, it's better to avoid this.  So, rearrange the creation of our
userns so that it is owned by the original user (e.g. root).  This requires
us to always create a UID & GID mapping within the userns so that we can
switch to the correct final user after we've entered the namespace.

Since we now always create that user mapping when we create the ns, we no
longer need to do so as we spawn the pasta shell or command.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

4 files changed, 89 insertions, 20 deletions

diff --git a/conf.c b/conf.c
index faf2681..0282f68 100644
--- a/conf.c
+++ b/conf.c
@@ -1962,8 +1962,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		if (*netns) {
 			pasta_open_ns(c, netns);
 		} else {
-			pasta_start_ns(c, uid, gid, !netns_only,
-				       argc - optind, argv + optind);
+			pasta_start_ns(c, argc - optind, argv + optind);
 		}
 	}
 
diff --git a/isolation.c b/isolation.c
index 024c26a..a00475f 100644
--- a/isolation.c
+++ b/isolation.c
@@ -311,6 +311,61 @@ static void enter_userns(const char *userns)
 }
 
 /**
+ * userns_holder() - Hold a clone()ed namespace open until killed
+ * @arg:	Unused
+ *
+ * Return: this function never returns
+ */
+static int userns_holder(void *arg)
+{
+	sigset_t set;
+
+	(void)arg;
+	/* If the parent dies with an error, so should we */
+	if (prctl(PR_SET_PDEATHSIG, SIGKILL))
+		die_perror("Couldn't set PR_SET_PDEATHSIG");
+
+	/* Wait until the parent kills us */
+	sigemptyset(&set);
+	sigwaitinfo(&set, NULL);
+
+	die("userns holder process wasn't killed");
+}
+
+/**
+ * create_userns() - Create a new userns to isolate ourselves
+ * @uid:	Parent UID to map to 0 within the namespace
+ * @gid:	Parent GID to map to 0 within the namespace
+ *
+ * Return: PID of the process holding the new userns
+ *
+ * Several things combine to make this more complicated than you'd expect.
+ * - We want to make the userns before we setuid() to nobody (or the userns
+ *   would be owned by nobody)
+ * - We still want to setuid() _after_ we enter the userns, which means
+ *   (parent-)nobody must be mapped within the userns
+ * - It's only possible to map the current user from within a userns, so we must
+ *   create that mapping from the parent
+ * - Therefore we can't create the userns with unshare(2), but must create it
+ *   by clone(2)ing a temporary holder process.
+ */
+static pid_t create_userns(uid_t uid, gid_t gid)
+{
+	char ns_fn_stack[NS_FN_STACK_SIZE]
+	__attribute__ ((aligned(__alignof__(max_align_t))));
+	pid_t pid;
+
+	pid = do_clone(userns_holder, ns_fn_stack, sizeof(ns_fn_stack),
+		       CLONE_NEWUSER | SIGCHLD, NULL);
+	if (pid < 0)
+		die_perror("Unable to create user namespace");
+
+	make_ugid_map(pid, uid, gid);
+
+	return pid;
+}
+
+/**
  * isolate_user() - Switch to final UID/GID and move into userns
  * @c:		Execution context
  * @uid:	User ID to run as (in original userns)
@@ -336,17 +391,41 @@ void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns,
 			die_perror("Can't drop supplementary groups");
 	}
 
-	if (setgid(gid) != 0)
-		die_perror("Can't set GID to %u", gid);
+	/* If we're going to use a pre-existing userns (either named, or with
+	 * --netns-only the current one), we need to switch to drop root first.
+	 * However, if we're going to create our own userns, we need to delay
+	 * dropping root, because we don't our userns to be owned by nobody.
+	 */
+	if (*userns || !use_userns) {
+		if (setgid(gid) != 0)
+			die_perror("Can't set GID to %u", gid);
 
-	if (setuid(uid) != 0)
-		die_perror("Can't set UID to %u", uid);
+		if (setuid(uid) != 0)
+			die_perror("Can't set UID to %u", uid);
+	}
 
 	if (*userns) { /* If given a userns, join it */
 		enter_userns(userns);
-	} else if (use_userns) { /* Create and join a new userns */
-		if (unshare(CLONE_NEWUSER) != 0)
-			die_perror("Couldn't create user namespace");
+	} else if (use_userns) { /* Otherwise create our own */
+		pid_t holder_pid = create_userns(uid, gid);
+		char new_userns[PATH_MAX];
+
+		if (snprintf_check(new_userns, sizeof(new_userns),
+				   "/proc/%u/ns/user", holder_pid))
+			die_perror("Could not build userns path");
+
+		enter_userns(new_userns);
+
+		/* Now that we occupy the ns, we can kill the temporary holder */
+		if (kill(holder_pid, SIGKILL))
+			die_perror("Could not kill userns temporary holder");
+
+		/* Switch to our final uid/gid, which are mapped to 0 in the userns */
+		if (setgid(0) != 0)
+			die_perror("Can't set GID to 0 in userns");
+
+		if (setuid(0) != 0)
+			die_perror("Can't set UID to 0 in userns");
 	}
 
 	/* Joining a new userns gives us full capabilities; drop the
diff --git a/pasta.c b/pasta.c
index 3abb9ba..644e95f 100644
--- a/pasta.c
+++ b/pasta.c
@@ -235,14 +235,10 @@ static int pasta_spawn_cmd(void *arg)
 /**
  * pasta_start_ns() - Fork command in new namespace if target ns is not given
  * @c:		Execution context
- * @uid:	UID we're running as in the init namespace
- * @gid:	GID we're running as in the init namespace
- * @config_idmaps:	Whether to configure user mappings
  * @argc:	Number of arguments for spawned command
  * @argv:	Command to spawn and arguments
  */
-void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, bool config_idmaps,
-		    int argc, char *argv[])
+void pasta_start_ns(struct ctx *c, int argc, char *argv[])
 {
 	char ns_fn_stack[NS_FN_STACK_SIZE]
 	__attribute__ ((aligned(__alignof__(max_align_t))));
@@ -259,10 +255,6 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, bool config_idmaps,
 	if (!c->debug)
 		c->quiet = 1;
 
-	/* Configure user and group mappings */
-	if (config_idmaps)
-		make_ugid_map(0, uid, gid);
-
 	if (argc == 0) {
 		arg.exe = getenv("SHELL");
 		if (!arg.exe)
diff --git a/pasta.h b/pasta.h
index b6a4cd5..de39d5f 100644
--- a/pasta.h
+++ b/pasta.h
@@ -12,8 +12,7 @@
 extern int pasta_child_pid;
 
 void pasta_open_ns(struct ctx *c, const char *netns);
-void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, bool config_idmaps,
-		    int argc, char *argv[]);
+void pasta_start_ns(struct ctx *c, int argc, char *argv[]);
 void pasta_ns_conf(struct ctx *c);
 void pasta_child_handler(int signal);
 void pasta_netns_quit_init(const struct ctx *c);