Re: [PATCH] libs/light: make it build without setresuid()

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Roger Pau Monné writes ("Re: [PATCH] libs/light: make it build without 
setresuid()"):
> On Tue, Jan 12, 2021 at 07:12:36PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@xxxxxxxxxx>
> > 
> > NetBSD doesn't have setresuid(). Add a configure check for it,
> > and use plain setuid() if !HAVE_SETRESUID
...
> LGTM from a code PoV, but I think George/Ian should take a look, since
> they know exactly what this is supposed to do, and I would bet there
> are some reasons why setresuid is used instead of setuid, which should
> likely be taken into account in the commit message to justify why
> using setuid in it's place it's fine.

There is indeed a reason for using setresuid here.  See the comments
at the top of kill_device_model_uid_child and the commit messages for
87f9458e3400 and 0c653574d39c.  This is all quite complex:

https://xenproject.org/2018/08/01/killing-processes-that-dont-want-to-be-killed/

https://marc.info/?l=xen-devel&m=152215770803468
 (search in that message for "libxl UID cleanup")

I wrote a message to George in 2018 proving that the desired set of
IDs cannot be made without setresuid.  I'll c&p the relevant part below.

I don't think setuid is safe - at least, if we are trying to restrict
the dm.  Since I think after the libxl child is forked, and has called
setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
the dm.  The dm could puppet it into pretending it had succeeded, but
then hang around until the domid is reused.

At the very least, this patch needs an argument, in detail, why this
is OK.

Also, why oh why does NetBSD not have setresuid ??  It's at least 20
years old !

Sorry,
Ian.

PS there is a long discussion of the history of saved set-ids, real vs
effective uids, etc., here
  https://pubs.opengroup.org/onlinepubs/9699919799/functions/setuid.html
but sadly it does not discuss setresuid.


>From me to George etc. in 2018.

George emailed me a draft post:
> # No POSIX-compliant mousetraps?
> 
> Although `setresuid` is implemented by both Linux and FreeBSD, it is
> not in the [current POSIX
> specification](http://pubs.opengroup.org/onlinepubs/9699919799/).
> Looking at the official list of POSIX system interfaces, it's not
> clear how to get a process to have the required tuple using only POSIX
> interfaces (namely `setuid` and `setreuid`, without recourse to
> `setresuid` or Linux's `CAP_SETUID`); the assumption seems to be that
> `euid` must always be set to either `ruid` or `suid`.

Proof that this can't be simulated by proper use of setuid, seteuid
and setreuid:

                                        ruid    euid    suid

The desired state is:                   reaper  target  reaper

If the final call is seteuid:

   seteuid(target);                     reaper  target  reaper

For this to be permitted, and nontrivial, euid was 0:
   
Penultimate status                      reaper  0       reaper

This state cannot be generated by setuid either euid==0 previously and
setuid would have set all of the ids; or the old euid was not 0, in
which case setuid() would have set only the euid, and required that
one of the other ids was 0, which can see that it can't have been.

This penultimate state cannot be generated by seteuid from any
different state.

So it must have been generated by setreuid.  We must avoid setreuid
setting the suid to the same as the new euid (0), which means that our
setreuid call did not change the ruid either.  That form of setreuid
is just like euid for our purposes, and not useful.

So the desired state could not be made by seteuid.

Let's consider setreuid.  Well, either setreuid sets the suid to the
same as the new euid, or it only changes the euid.  Ie, it would only
do something we could have done with seteuid and the argument above
applies.

What abouit setuid ?  Well, either setuid sets all three uids to the
same thing, or it, again, sets only the euid.

Ian.