From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top
Subject: Re: [PATCH v2 03/10] Consolidate determination of UID/GID to run as
Date: Sat, 10 Sep 2022 17:15:41 +1000
Message-ID: <Yxw5neNDgPFY68tH@yekko>
In-Reply-To: <20220909163347.788ddcad@elisabeth>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2563776498642762026=="

--===============2563776498642762026==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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 <david(a)gibson.dropbear.id.au> wrote:
>=20
> > 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().
> >=20
> > Make this clearer by putting all the UID and GID logic into a single
> > conf_ugid() function.
> >=20
> > 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(-)
> >=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, unsigned in=
t *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
> 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.

--=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

--===============2563776498642762026==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz
MGRCK1h0Z3lwWTRnRXdZU0lGQW1NY09aWUFDZ2tRZ3lwWTRnRXcKWVNKdzV4QUF1OHlkQ2F6TWdT
UjN5OXlHbEJVVlFybTZRR3pXM2JtaGM4bkNJOXVPdDZXVEFxNG4rSUtuWXA2LworanZnS1A2THJ5
TnNDVmV3S1RGalZZemppYmRkTVltZzU0Q0RKc0dic256VDNFZENEREYxTkVzenBDMDc5YTlRCmhC
RGhxbUhjTU8wQWZVb3hDSTZmb1dWcEcxeThuRS9ZdGZoWmdrQ2F6b3Z0Y3J2cktKdjMxb3NGdVpS
Y29pUDMKa2duVkFuc3lzZFpuU1JNMFpuTnZ5c2tKbXMyZnIvbVNFM3laQmJJUHYrVk5oNXZsdUNx
VW9VVG90MWtiWGtGbApQaXJnZnRacXlENDE1dXlUaEFuQlZKQ0kyTjRCNHJsZ3FMd0t4ZTVhQ2RW
NlQzZ0ZLSkJJQU91c3dEVkkzSkRUCm1aSmxjdkdoV2t0ZHhhdnVjOXRwWmUwNmFrZnhzbUtjTHp4
MzhJbWFVcmFwZmMvcVdPRHdseEp6d3hrOHpqWWEKdkpGQTRSdlh0RVV0Q0UvVmtMUUhHMGhHQlVZ
N0RTcVpXV00wQnhaZXZWSCtHL093cWFhQ0RERWc2NUFLQnp5TQorbXJHbFQzY1Jyd2t6R2dKdWZz
bGVmTXhwU1hKdGh3eVhSTnF2T2tzZlI0RVM4K2hqUmIyTEVUTUdlNDJuYlBuCmZuSDU3WXQzUnkw
YkFBTFEyU3dnYlZ5WGhaSFNxTFVoaWthNHJDZTNXVjlTcHE4bGh0RnpkRkhnL0syOGdyZWQKQnRU
Q1VYMTNQbkhQODRFYmpreDhoVWpjekx4ZWZZdnlDdE5mUWc4QzJLQzlLU21GUTNtdmpQcEtjdVcy
SkEyQgp1SnZrbW9PRm43WVVYZ0gvU1djdWxJb3d2QlNRaXFPOHJMUU1pSnpaLzNxdlFkNlp0WHM9
Cj1PZ3IyCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============2563776498642762026==--