From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson To: passt-dev@passt.top Subject: Re: [PATCH v2 03/10] Consolidate determination of UID/GID to run as Date: Mon, 12 Sep 2022 19:53:38 +1000 Message-ID: In-Reply-To: <20220910224332.0ba32592@elisabeth> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7813795852383432812==" --===============7813795852383432812== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Sat, Sep 10, 2022 at 10:43:31PM +0200, Stefano Brivio wrote: > On Sat, 10 Sep 2022 17:15:41 +1000 > David Gibson wrote: >=20 > > On Fri, Sep 09, 2022 at 04:33:47PM +0200, Stefano Brivio wrote: > > > Also mere nitpicking on this one: > > >=20 > > > On Thu, 8 Sep 2022 13:59:00 +1000 > > > David Gibson wrote: > > > =20 > > > > Currently the logic to work out what UID and GID we will run as is sp= read > > > > across conf(). If --runas is specified it's handled in conf_runas(), > > > > otherwise it's handled by check_root(), which depends on initializati= on of > > > > the uid and gid variables by either conf() itself or conf_runas(). > > > >=20 > > > > Make this clearer by putting all the UID and GID logic into a single > > > > conf_ugid() function. > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > conf.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++----= -- > > > > util.c | 50 ------------------------------------ > > > > util.h | 1 - > > > > 3 files changed, 73 insertions(+), 59 deletions(-) > > > >=20 > > > > 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, unsigne= d int *uid, unsigned int *gid) > > > > #endif /* !GLIBC_NO_STATIC_NSS */ > > > > } > > > > =20 > > > > +/** > > > > + * 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[] =3D " 0 0 4294967295"; > > > > + struct passwd *pw; > > > > + char buf[BUFSIZ]; > > > > + int ret; > > > > + int fd; > > > > + > > > > + /* If user has specified --runas, that takes precedence */ > > > > + if (runas) { > > > > + ret =3D conf_runas(runas, uid, gid); > > > > + if (ret) > > > > + err("Invalid --runas option: %s", runas); > > > > + return ret; > > > > + } > > > > + > > > > + /* Otherwise default to current user and group.. */ > > > > + *uid =3D geteuid(); > > > > + *gid =3D getegid(); > > > > + > > > > + /* ..as long as it's not root.. */ > > > > + if (*uid) > > > > + return 0; > > > > + > > > > + /* ..or at least not root in the init namespace.. */ > > > > + if ((fd =3D open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) > > > > + return 0; > > > > + > > > > + if (read(fd, buf, BUFSIZ) !=3D sizeof(root_uid_map) || > > > > + strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) { > > > > + close(fd); > > > > + return 0; > > > > + } > > > > + > > > > + close(fd); > > > > + > > > > + /* ..otherwise use nobody:nobody */ =20 > > >=20 > > > I'd change all those comment to use ellipses (...) instead of "..". =20 > >=20 > > Ok, nit picked. > >=20 > > > 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). =20 > >=20 > > 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. >=20 > I think it's desirable for two cases (rather important in my opinion): >=20 > - 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 >=20 > - ...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. --=20 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 --===============7813795852383432812== Content-Type: application/pgp-signature Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="signature.asc" MIME-Version: 1.0 LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz MGRCK1h0Z3lwWTRnRXdZU0lGQW1NZkFac0FDZ2tRZ3lwWTRnRXcKWVNLRS9oQUF6Vi9WMTljREFo Zkwyc2RONWJGOEREK1l1RlE2MWNoZjdzT3NhZTNiZnZzMTFqS2daSHRjaGt3YgpYb3I2TUo4MFow SnMwYW9sMVp5NXUvcnNkdE1hTDB2dUpDNVljQkREVS9TaGhzazZyRnRLY2VIN3F0Skx1a1BnCjl1 RnNSNGlqaHJDb2krVzZwbzcwc2VBakwzeVZ0ajhWN0hLWHlqRnh5T3RoUUlBMGkvRnVjcVJwQWZD T1kvWnUKeUdsc3lIR3drYzUvTjQ3SVpBS0grUU9HYzFNbUUvWkF1SDV5d0pMM24vMjlJeEtzdzRQ ZDlzSmlRMG1qMFN4bgpEY2xpZHpCQzc3TjArRmgxRWZKdWVJVm0zdDlEWjQ3bFQ5U2ljM3kzdVdN ZVFadjNlMUVueTVwbHdrcUFIZFBKCjRIemhUb0kwRnJUNEU1WC9hQVRySlJuY1k2c1p5RVZQK0kv S0NGQ0hERWcwMmthOEhFTGdhQ3pJZkVldDVqVHkKYlNrQ01ObzV4UW9QT0k0WDVuUWY5R3hpNmty Vi9PVXZ2eEJ4M2FtSjJwT3Yxbm1DSG1iRmpGTHR6c1pvNDdrcApUc2ZUWURhdS9PQXI5eThEZmZs ZnFxNitaL0hjOWViK3NlSmRUcmI5T1l4eitNcllPMmhpQ1pHT2hwVlozdndmCjJzL0hpR1VlT3Vx YURySVo3VUN1S29MblBnd1BRWmFockUxVU9WaHJvNndJck5CUFRCWExUOGJyVGhaaXZLbTkKVFRn WE5NVnhvSXRnVVhKVXIvbTdkcjd1b2k0MzJPKzllZm0rYTlmdDlicVdJcWtwVml2Vm55c2hWSkJH RDBJNwp0dEQvYU9mTHlTdlFVdnhZay83Y2pWTk85TmpTNkgzbWtBdGJXUEQ4b1lxQXpHd1FMNGM9 Cj1IL2YvCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo= --===============7813795852383432812==--