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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6289 bytes --]

On Thu, Oct 13, 2022 at 04:18:29AM +0200, Stefano Brivio wrote:
> 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.

Removed.

> >  	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.

Done.

> >  	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.

Fixed.

> >  	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);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]