Re: [PATCH v17 08/13] namei: LOOKUP_BENEATH: O_BENEATH-like scoped resolution

[Aleksa Sarai <cyphar at cyphar dot com>]

On 2019-11-25, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Nov 17, 2019 at 12:17:08PM +1100, Aleksa Sarai wrote:
> 
> > +	if (unlikely(nd->flags & LOOKUP_IS_SCOPED)) {
> > +		/*
> > +		 * Do a final check to ensure that the path didn't escape. Note
> > +		 * that this should already be guaranteed by all of the other
> > +		 * LOOKUP_IS_SCOPED checks (and delaying this check this late
> > +		 * does open the door to some possible timing-based attacks).
> > +		 */
> > +		if (WARN_ON(!path_is_under(&nd->path, &nd->root)))
> > +			return -EXDEV;
> 
> I don't like that.  What it gives is an ability to race that with
> rename(), with user-triggered WARN_ON.  You *can't* promise that result of
> lookup is in a subtree, simply because it can get moved just as you've
> declared it to be in the clear.
> 
> 	Anyone who relies upon that is delusional; it really can't be done.
> What warranties LOOKUP_IS_SCOPED is really supposed to provide?  That we do not
> attempt to walk out of the subtree rooted at the start point?  Fine, but this
> is not what this test does.  What are you trying to achieve there?  If it's
> "what we'd got was at one point in our subtree", the test is more or less
> right, but WARN_ON isn't.

You're right that (looking at this again) this chunk doesn't make too
much sense -- though I still think it wouldn't be a bad idea to include
it without the WARN_ON.

The reason I added it was as an attempt to have a last-chance check to
make sure we don't hand out a file descriptor to userspace that is
outside nd->root as a result of some yet-unknown namei bug (hence the
WARN_ON). But you're quite right that I overlooked that user-space could
trigger this maliciously.

Regarding the warranties LOOKUP_IS_SCOPED is supposed to provide --
arguably the guarantee is meant to be "you never step outside the root
during lookup" but that should already be implemented with the
handle_dots() checks -- and it's not something you could easily check at
the end of a lookup anyway. The idea was that (if for some reason those
checks were bypassed), at the very least you wouldn't silently get a
file descriptor which is completely outside the root.

Looking at this again, I can see the argument that check this shouldn't
be done at all -- but I will admit that I feel more comfortable with the
guarantees of LOOKUP_IS_SCOPED if we had some kind of last-chance check
to avoid a privileged process opening /etc/shadow on the host. Then
again, libpathrs (which I assume will be the primary consumer of
LOOKUP_IN_ROOT) already does a double-check in userspace after getting
the file descriptor from openat2().

All of that being said, I'd be happy to drop it entirely if you feel
it's unnecessary.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachment: signature.asc
Description: PGP signature