Re: [PATCH v2 02/10] Split checking for root from dropping root privilege

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3231 bytes --]

On Fri, Sep 09, 2022 at 04:33:27PM +0200, Stefano Brivio wrote:
> Just some ridiculous nitpicking on this one:
> 
> On Thu,  8 Sep 2022 13:58:59 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > [...]
> >
> > +++ b/passt.1
> > @@ -104,9 +104,10 @@ terminal, and to both system logger and standard error otherwise.
> >  
> >  .TP
> >  .BR \-\-runas " " \fIUID\fR|\fIUID:GID\fR|\fILOGIN\fR|\fILOGIN:GROUP\fR
> > -If started as root, change to given UID and corresponding group if UID is given,
> > +Attempt to change to given UID and corresponding group if UID is given,
> >  or to given UID and given GID if both are given. Alternatively, login name, or
> > -login name and group name can be passed.
> > +login name and group name can be passed.  This requires privilege (either
> 
> I'd change this to "privileges", right?

Hmm.. I think either would work, but I'll change it.


> Also, adding two spaces following a period, as you do, seems to have
> some merits:
> 
>   https://link.springer.com/article/10.3758/s13414-018-1527-6
> 
>   Johnson, R.L., Bui, B. & Schmitt, L.L. Are two spaces better than
>   one? The effect of spacing following periods and commas during reading.
>   Atten Percept Psychophys 80, 1504–1511 (2018)
> 
> ...but in man pages, nroff might turn that into three or more spaces,
> inconsistently, in a justified paragraph.
> 
> I'd stick to one. Or change all of them (I almost never use two, so
> here it's all single spaces).

Fair enough.  I believe the question of spaces after a full stop
(British/Australian English for "period") is a bit of a contentious
issue.  At least in the English speaking world - I gather it's rarely
used outside that.  It's mostly irrelevant for modern typesetting,
because the typesetter will adjust anyways, I think I developed the
habit from writing text documents, at least some in the age of
monospaced printing.

I've removed it, anyway.

> > +initial effective UID 0 or CAP_SETUID capability) to work.
> >  Default is to change to user \fInobody\fR if started as root.
> >  
> >  .TP
> > diff --git a/util.c b/util.c
> > index b2ccb3d..17595c1 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -492,7 +492,13 @@ void check_root(uid_t *uid, gid_t *gid)
> >  	char buf[BUFSIZ];
> >  	int fd;
> >  
> > -	if (getuid() && geteuid())
> > +	if (!*uid)
> > +		*uid = geteuid();
> > +
> > +	if (!*gid)
> > +		*gid = getegid();
> > +
> > +	if (*uid)
> >  		return;
> >  
> >  	if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
> > @@ -524,11 +530,26 @@ void check_root(uid_t *uid, gid_t *gid)
> >  		*uid = *gid = 65534;
> >  #endif
> >  	}
> > +}
> > +
> > +/**
> > + * drop_root() - Switch to given UID and GID
> 
> I would add the usual:
> 
>  * @uid:	User ID to switch to
>  * @gid:	Group ID to switch to
> 
> even though it's totally obvious here, in case somebody should ever need
> to parse this stuff to produce documentation.

Good point, added.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]