Re: [PATCH v2 03/10] Consolidate determination of UID/GID to run as

[Stefano Brivio <sbrivio@redhat.com>]

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

On Mon, 12 Sep 2022 19:53:38 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Sat, Sep 10, 2022 at 10:43:31PM +0200, Stefano Brivio wrote:
> > On Sat, 10 Sep 2022 17:15:41 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Sep 09, 2022 at 04:33:47PM +0200, Stefano Brivio wrote:  
> > > > Also mere nitpicking on this one:
> > > > 
> > > > On Thu,  8 Sep 2022 13:59:00 +1000
> > > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > > >     
> > > > > Currently the logic to work out what UID and GID we will run as is spread
> > > > > across conf().  If --runas is specified it's handled in conf_runas(),
> > > > > otherwise it's handled by check_root(), which depends on initialization of
> > > > > the uid and gid variables by either conf() itself or conf_runas().
> > > > > 
> > > > > Make this clearer by putting all the UID and GID logic into a single
> > > > > conf_ugid() function.
> > > > > 
> > > > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > > > > ---
> > > > >  conf.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
> > > > >  util.c | 50 ------------------------------------
> > > > >  util.h |  1 -
> > > > >  3 files changed, 73 insertions(+), 59 deletions(-)
> > > > > 
> > > > > diff --git a/conf.c b/conf.c
> > > > > index 545f61d..5c293b5 100644
> > > > > --- a/conf.c
> > > > > +++ b/conf.c
> > > > > @@ -1021,6 +1021,70 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
> > > > >  #endif /* !GLIBC_NO_STATIC_NSS */
> > > > >  }
> > > > >  
> > > > > +/**
> > > > > + * conf_ugid() - Determine UID and GID to run as
> > > > > + * @runas:	--runas option, may be NULL
> > > > > + * @uid:	User ID, set on success
> > > > > + * @gid:	Group ID, set on success
> > > > > + *
> > > > > + * Return: 0 on success, negative error code on failure
> > > > > + */
> > > > > +static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid)
> > > > > +{
> > > > > +	const char root_uid_map[] = "         0          0 4294967295";
> > > > > +	struct passwd *pw;
> > > > > +	char buf[BUFSIZ];
> > > > > +	int ret;
> > > > > +	int fd;
> > > > > +
> > > > > +	/* If user has specified --runas, that takes precedence */
> > > > > +	if (runas) {
> > > > > +		ret = conf_runas(runas, uid, gid);
> > > > > +		if (ret)
> > > > > +			err("Invalid --runas option: %s", runas);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	/* Otherwise default to current user and group.. */
> > > > > +	*uid = geteuid();
> > > > > +	*gid = getegid();
> > > > > +
> > > > > +	/* ..as long as it's not root.. */
> > > > > +	if (*uid)
> > > > > +		return 0;
> > > > > +
> > > > > +	/* ..or at least not root in the init namespace.. */
> > > > > +	if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
> > > > > +	    strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
> > > > > +		close(fd);
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	close(fd);
> > > > > +
> > > > > +	/* ..otherwise use nobody:nobody */    
> > > > 
> > > > I'd change all those comment to use ellipses (...) instead of "..".    
> > > 
> > > Ok, nit picked.
> > >   
> > > > I think your comments here help a lot, by the way (and I couldn't find
> > > > a better way to check for UID 0 in non-init other than that hack).    
> > > 
> > > Right.  The extra complexity - both of code and of mental model - in
> > > going from "not UID 0" to "not UID 0 in the init namespace" is what
> > > makes me really wonder if this check is worth having.  
> > 
> > I think it's desirable for two cases (rather important in my opinion):
> > 
> > - running passt with a further isolation implemented by a user
> >   namespace (e.g. with pasta). There it's not really practical to use a
> >   non-zero UID, and allowing to do this easily is a relevant security
> >   feature
> > 
> > - ...if I recall correctly (but I can't check right now) Podman does
> >   the same  
> 
> Sorry, I realize I wasn't clear.  We absolutely need the ability to
> run as "root" (UID 0) within a user namespace.  What I'm questioning
> is given that whether it's worth preventing running when UID 0 outside
> a user namespace (as far as we can tell).  There's arguably some edge
> cases where it might be useful, and it's debatable whether it's
> passt's job to prevent the user from shooting themselves in the foot
> in this way.

Ah, I see now.

Well, I also think it's debatable, and I'd even tend to say it's not
actually passt's job, but I still think there are three reasons to keep
doing this:

- user mistakes happen, and it's also arguably our job to make usage
  less error-prone

- if users run this as root, we won't actually run as root, so we
  obtain an essentially equivalent level of security while letting
  lazy/distracted users do whatever... compared to the alternative, it
  sounds appealing

- given that it's debatable (for instance, many other tools and daemons
  do the same), I'd keep erring on the side of caution, as this might
  significantly decide the perceived, or factual, severity of any
  vulnerability that might be found

I also guess that forcing users to rebuild from source if they want to
do is reasonable for those edge cases.

-- 
Stefano