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

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

-- 
Stefano