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: Tue, 13 Sep 2022 15:20:49 +1000 Message-ID: In-Reply-To: <20220913054952.02818197@elisabeth> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0465205525643610797==" --===============0465205525643610797== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Tue, Sep 13, 2022 at 05:49:52AM +0200, Stefano Brivio wrote: > On Mon, 12 Sep 2022 19:53:38 +1000 > David Gibson wrote: >=20 > > 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: =20 > > > > > 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 i= s spread > > > > > > across conf(). If --runas is specified it's handled in conf_runa= s(), > > > > > > otherwise it's handled by check_root(), which depends on initiali= zation 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 sin= gle > > > > > > 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, uns= igned 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 f= ind > > > > > 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 > > >=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 =20 > >=20 > > 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 > Ah, I see now. >=20 > 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: >=20 > - user mistakes happen, and it's also arguably our job to make usage > less error-prone >=20 > - 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 >=20 > - 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 >=20 > I also guess that forcing users to rebuild from source if they want to > do is reasonable for those edge cases. Yeah, I guess that's fair enough. --=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 --===============0465205525643610797== Content-Type: application/pgp-signature Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="signature.asc" MIME-Version: 1.0 LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz MGRCK1h0Z3lwWTRnRXdZU0lGQW1NZ0V5a0FDZ2tRZ3lwWTRnRXcKWVNMdHJ3LytJUkthd3F6QlRH UE1kWXhVcUQ3T3FTemFkSnFSbWFuWVU1a0pOdEpFZUh6WWZwcWxsbUtFL2FVNApQeXdZa095Zk1p S21GN2k0Y09lOU5FZ0FtUk9qeVBWdjNTck01ZkVkSDBEYmZ6dkdWZ2JXOHZhZUpPbUFjbFBMCkpw OTZybW80dlZMZG8xYW9FaEpKclJXejgvUlprM3pHN3dMazBWenEycUlzYjZsWWdLVk0xU2s2MDZp c1VWSDgKOWtWYzlZeElzL0RjM2RhbGRGNEtrNGFGWVFIYlJtUm93dU8zRTBvalQzaGpwc1dhK1Zh anVQdzVscE9VSkR5UQpwSmpET2JDWHE0YkUxYUk1WThZdDduQjVaY2FiV1VnRDRmbWw0b25wWG1s STdzeVJLcXJyeE9OYjN6SmtlK0ZXCnc1QmdsL2FHZTVhOFdaSFZVK2pBdlNLVUNzSHNXM1Q2UUhY YS9lOUkrK2ZSalN4Z3A4ZGhRcjNKRkkxMFd6OS8KQ1JWbkR2NmdLUGNvdjlNQXN5Tkp3ZjlyUGc4 eEk2SUdPV1dSWjJSTU8xUWpaMEhqTndlNzNlZE5IQnNCc1Z6UApBRTBNUEJ4MGMrUklBSmpkTHp3 aFZndFg0Zkl4MjNnNTgvUHZvTHBNcW1lSUVBdm5yNjlWNlEwbDRFdmdTcktGCndwZGk0MXNNQnJu ZzJGQmtLN3ZhaHBHcEdnY2pUOTBoOFZqZ1VpdFdPaFFUU2dTMU1RY3AyZFRNQmlIaUZxS1AKWWhB c21JZzkwekRaTnlXMDFhLzNyNFU2U1UvaVhCSnFnQ1k2ZzVqK3lnVnNpWnY1ZXBsMVhrMlhpanMv VnMvcwpwcjRXNm5GUlhyWW1DTGQ0OWY4WjU3SzhmU2JSeGZqd001NUphUTk0elRaQjYwWEY2UG89 Cj1OK3pICi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo= --===============0465205525643610797==--