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

[Stefano Brivio <sbrivio@redhat.com>]

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

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?

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

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

-- 
Stefano