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:Ok, nit picked.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 "..".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. -- 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