Re: [PATCH 09/10] isolation: Only configure UID/GID mappings in userns when spawning shell

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 11 Oct 2022 16:40:17 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> When in passt mode, or pasta mode spawning a command, we create a userns
> for ourselves.  This is used both to isolate the pasta/passt process itself
> and to run the spawned command, if any.
> 
> Since eed17a47 "Handle userns isolation and dropping root at the same time"
> we've handled both cases the same, configuring the UID and GID mappings in
> the new userns to map whichever UID we're running as to root within the
> userns.
> 
> This mapping is desirable when spawning a shell or other command, so that
> the user gets a root shell with reasonably clear abilities within the
> userns and netns.  It's not necessarily essential, though.  When not
> spawning a shell, it doesn't really have any purpose: passt itself doesn't
> need to be root and can operate fine with an unmapped user (using some of
> the capabilities we get when entering the userns instead).
> 
> Configuring the uid_map can cause problems if passt is running with any
> capabilities in the initial namespace, such as CAP_NET_BIND_SERVICE to
> allow it to forward low ports.  In this case the kernel makes files in
> /proc/pid owned by root rather than the starting user to prevent the user
> from interfering with the operation of the capability-enhanced process.
> This includes uid_map meaning we are not able to write to it.
> 
> Whether this behaviour is correct in the kernel is debatable, but in any
> case we might as well avoid problems by only initializing the user mappings
> when we really want them.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c      |  3 ++-
>  isolation.c | 13 -------------
>  pasta.c     | 16 ++++++++++++++--
>  pasta.h     |  3 ++-
>  4 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 1537dbf..b7661b6 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1478,7 +1478,8 @@ void conf(struct ctx *c, int argc, char **argv)
>  		if (*netns) {
>  			pasta_open_ns(c, netns);
>  		} else {
> -			pasta_start_ns(c, argc - optind, argv + optind);
> +			pasta_start_ns(c, uid, gid,
> +				       argc - optind, argv + optind);
>  		}
>  	}
>  
> diff --git a/isolation.c b/isolation.c
> index e1a024d..b94226d 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -207,9 +207,6 @@ void isolate_initial(void)
>   */
>  void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns)
>  {
> -	char uidmap[BUFSIZ];
> -	char gidmap[BUFSIZ];
> -
>  	/* First set our UID & GID in the original namespace */
>  	if (setgroups(0, NULL)) {
>  		/* If we don't have CAP_SETGID, this will EPERM */
> @@ -261,16 +258,6 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns)
>  		err("Couldn't create user namespace: %s", strerror(errno));
>  		exit(EXIT_FAILURE);
>  	}
> -
> -	/* Configure user and group mappings */
> -	snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
> -	snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
> -
> -	if (write_file("/proc/self/uid_map", uidmap) ||
> -	    write_file("/proc/self/setgroups", "deny") ||
> -	    write_file("/proc/self/gid_map", gidmap)) {
> -		warn("Couldn't configure user namespace");
> -	}
>  }
>  
>  /**
> diff --git a/pasta.c b/pasta.c
> index 0ab2fe4..9666fed 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -166,7 +166,6 @@ static int pasta_setup_ns(void *arg)
>  {
>  	const struct pasta_setup_ns_arg *a
>  		= (const struct pasta_setup_ns_arg *)arg;
> -

Unrelated.

>  	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
>  		warn("Cannot set ping_group_range, ICMP requests might fail");
>  
> @@ -179,16 +178,20 @@ static int pasta_setup_ns(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
>   * @argc:	Number of arguments for spawned command
>   * @argv:	Command to spawn and arguments
>   */
> -void pasta_start_ns(struct ctx *c, int argc, char *argv[])
> +void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> +		    int argc, char *argv[])
>  {
>  	struct pasta_setup_ns_arg arg = {
>  		.exe = argv[0],
>  		.argv = argv,
>  	};
>  	char *sh_argv[] = { NULL, NULL };
> +	char uidmap[BUFSIZ], gidmap[BUFSIZ];

Should go at the beginning for the usual semi-silly reason.

>  	char ns_fn_stack[NS_FN_STACK_SIZE];
>  	char sh_arg0[PATH_MAX + 1];
>  
> @@ -196,7 +199,16 @@ void pasta_start_ns(struct ctx *c, int argc, char *argv[])
>  	if (!c->debug)
>  		c->quiet = 1;
>  
> +	/* Configure user and group mappings */
> +	snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
> +	snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
>  
> +	if (write_file("/proc/self/uid_map", uidmap) ||
> +	    write_file("/proc/self/setgroups", "deny") ||
> +	    write_file("/proc/self/gid_map", gidmap)) {
> +		warn("Couldn't configure user mappings");
> +	}
> +	

Excess whitespace.

>  	if (argc == 0) {
>  		arg.exe = getenv("SHELL");
>  		if (!arg.exe)
> diff --git a/pasta.h b/pasta.h
> index 02df1f6..a8b9893 100644
> --- a/pasta.h
> +++ b/pasta.h
> @@ -7,7 +7,8 @@
>  #define PASTA_H
>  
>  void pasta_open_ns(struct ctx *c, const char *netns);
> -void pasta_start_ns(struct ctx *c, int argc, char *argv[]);
> +void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> +		    int argc, char *argv[]);
>  void pasta_ns_conf(struct ctx *c);
>  void pasta_child_handler(int signal);
>  int pasta_netns_quit_init(struct ctx *c);

-- 
Stefano