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

From: Manuel Bouyer <bouyer@antioche.eu.org>
To: Ian Jackson <iwj@xenproject.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org, "Wei Liu" <wl@xen.org>,
	"Anthony PERARD" <anthony.perard@citrix.com>
Subject: Re: [PATCH] libs/light: make it build without setresuid()
Date: Wed, 20 Jan 2021 16:13:21 +0100	[thread overview]
Message-ID: <20210120151321.GB4175@antioche.eu.org> (raw)
In-Reply-To: <24584.17302.958286.788145@mariner.uk.xensource.com>

On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> 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@netbsd.org>
> > > 
> > > 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

What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
so there isn't much to protect.

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

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

It's not because it's old that it's good.

> 
> Sorry,

OK so if I understand properly, you say Xen should not be used on NetBSD ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--