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

[Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>]

On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without 
> setresuid()"):
> > On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> > > 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
> > 
> > What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
> > so there isn't much to protect.
> 
> 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.

> 
> > > 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.
> > 
> > I don't understand. We're talking about a simple kill(2) syscall here.
> 
> If we're not trying to restrict qemu's privilege at all, then I think
> the setuid is fine.
> There are then only two remaining concerns I have
> with this patch:
> 
> Firstly, we try to avoid #ifdefs like this.  It tends to make the code
> rather tangled, especially over time.  Instead we prefer to move the
> non-portable code into its own file, eg *_linux.c.
> 
> Secondly, I think we should check that dm_restrict is not enabled.
> I think an assert would do since I think we believe this is already
> prevented elsewhere ?
> 
> (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.

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

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

-- 
Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
     NetBSD: 26 ans d'experience feront toujours la difference
--