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

[Ian Jackson <iwj@xenproject.org>]

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> > Yes, the dm is qemu.  If qemu restriction is not supported, that makes
> > a big difference.  The complex situation here is to do with trying to
> > kill a possibly hostile qemu.
> 
> Hum, I'll have to check this (how to check, BTW ?).
> I assumed qemu was running as root but it may not be completely true.
> Especially as I notice, now that I'm re-reading the patch, that
> we're doing a kill to -1. If we were doing so as root, user processes
> would be killed.

It may well be that this whole piece of code won't be executed on
NetBSD becauwe dm restriction will be off.

The background: dm restriction is a set of arrangements for trying to
run qemu without given it any more privilege than it needs, and
certainly not ultimate privilege over the host.  This is quite
complicated and includes running it as a non-root user, chroot, and so
on.

On Linux it's run in its own network namespace, so that a qemu
compromised by the guest cannot access host daemons.  IDK what
facilities one might want to use on NetBSD to try to contain qemu.

This seems to me all a matter for future work.  I'm sorry that code
for a feature you're not going to be benefiting from is getting in
your way.

> > (One option for making this work would be to simply disable the
> > killing by uid on NetBSD.  But I don't think that's a good answer
> > because killing by uid after eg setuid is more reliable even if it is
> > not 100% bulletproof.  So switching to setuid or maybe setreuid is the
> > right answer.)
> 
> This would have to be checked, but I don't think a non-root process
> can ptrace a process whose saved-user-id is root.

If I remember rightly the saved-set-id is reset by setuid.  But I
could be wrong.  This stuff is all quite complex :-/.

> Actually I think I could mimic the setresuid() with setreuid() and seteuid().

My last mail had in it a thing that claims to be a proof that this is
not possible.

But I'm hoping we can avoid this.

> > > OK so if I understand properly, you say Xen should not be used on NetBSD ?
> > 
> > I'm sorry to have offended and discouraged you.  That was not my
> > intention.  My apologies for sending an off-putting message.  For the
> > avoidance of any doubt, definitely don't think that.  We should make
> > this work properly.
> > 
> > Would you be willing to look into the two points I mention above and
> > send a revised version of the patch ?  If you find the refactoring
> > awkward I or Roger can help.
> 
> Actually I don't see how I could split this in a different file, without
> lot of duplicate code (even in just kill_device_model_uid_child(),
> we're talking of about 7 lines of code out of 75). So some guidance here
> would be welcome.

I think splitting it out at precisely the function needed is probably
better.

Can you try this experiment: what happens if you replace the call to
setresuid with abort() ?  I think you may find it all works, because
you're not using that code path.

If so then I suggest introducing

  int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);

which would call setresuid on Linux and on NetBSD would do this

  assert(!"setresuid is not available on NetBSD, and dm restrction is not supported, so this code path should not have been reached")

What do you think ?

Ian.