linux-kernel - Re: [RFC][PATCHSET] sanitized pathwalk machinery (v2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Feb 2020 12:24:51 +1100
From:   Aleksa Sarai <cyphar@...har.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [RFC][PATCHSET] sanitized pathwalk machinery (v2)

On 2020-02-25, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Sun, Feb 23, 2020 at 01:12:21AM +0000, Al Viro wrote:
> > 	This is a slightly extended repost of the patchset posted on
> > Jan 19.  Current branch is in vfs.git#work.do_last, the main
> > difference from the last time around being a bit of do_last()
> > untangling added in the end of series.  #work.openat2 is already
> > in mainline, which simplifies the series - now it's a straight
> > branch with no merges.
> 
> Whee...  While trying to massage ".." handling towards the use of
> regular mount crossing semantics, I've found something interesting.
> Namely, if you start in a directory with overmounted parent,
> LOOKUP_NO_XDEV resolution of ../something will bloody well cross
> into the overmount.

Oh boy...

> Reason: follow_dotdot() (and its RCU counterpart) check for LOOKUP_NO_XDEV
> when crossing into underlying fs, but not when crossing into overmount
> of the parent.
> 
> Interpretation of .. is basically
> 
> loop:	if we are in root					// uncommon
> 		next = current position
> 	else if we are in root of a mounted filesystem		// more rare
> 		move to underlying mountpoint
> 		goto loop
> 	else
> 		next = parent directory of current position	// most common
> 
> 	while next is overmounted				// _VERY_ uncommon
> 		next = whatever's mounted on next
> 
> 	move to next
> 
> The second loop should've been sharing code with the normal mountpoint
> crossing.  It doesn't, which has already lead to interesting inconsistencies
> (e.g. autofs generally expects ->d_manage() to be called before crossing
> into it; here it's not done).  LOOKUP_NO_XDEV has just added one more...

You're quite right -- LOOKUP_NO_XDEV should block that and I missed it.

> Incidentally, another inconsistency is LOOKUP_BENEATH treatment in case
> when we have walked out of the subtree by way of e.g. procfs symlink and
> then ran into .. in the absolute root (that's
>                 if (!follow_up(&nd->path))
>                         break;
> in follow_dotdot()).  Shouldn't that give the same reaction as ..
> in root (EXDEV on LOOKUP_BENEATH, that is)?  It doesn't...

You can't go through procfs symlinks with LOOKUP_BENEATH, but if it's
possible to do that kind of jump then it should also be blocked (but I
would say that I'd prefer "block any kind of weird jump").

> Another one is about LOOKUP_NO_XDEV again: suppose you have process'
> root directly overmounted and cwd in the root of whatever's overmounting
> it.  Resolution of .. will stay in cwd - we have no parent within the
> chroot jail we are in, so we move to whatever's overmounting that root.
> Which is the original location.  Should we fail on LOOKUP_NO_XDEV here?
> Plain .. in the root of chroot jail (not overmounted by anything) does
> *not*...

I think LOOKUP_NO_XDEV should block that since you end up crossing a
mountpoint.

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)