[RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

[RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

[Al Viro]

	Bringing back an old conversation - what do you think about the
potential usefulness of the following ...at() option:
	* no mountpoint crossings allowed (mount --bind included)
	* only relative symlinks traversals are allowed
	* starting point acts as a chroot boundary (IOW, .. does not lead
out of it)

IIRC, you mentioned that something of that kind might be interesting for
git et.al. and it turns out to be surprisingly easy to implement -
 fs/namei.c                 | 31 +++++++++++++++++++++++++++----
 fs/stat.c                  |  4 +++-
 include/linux/namei.h      |  1 +
 include/uapi/linux/fcntl.h |  1 +
 4 files changed, 32 insertions(+), 5 deletions(-)
for implementation of LOOKUP_NO_JUMPS and its hookup for fstatat() and
friends.  And AFAICS it doesn't mess the fast paths either.  I've dropped
it into vfs.git#experimental-AT_NO_JUMPS; only built-tested at the moment
(and in any case, it would need manpage patches, xfstests and LTP ones,
etc.)

The points I'm not sure about:
	1) what would be a good error value (went with -ELOOP at the moment)
	2) handling of .. is quiet in that sucker; trying to walk out of
subtree does not fail with -ELOOP (as absolute symlinks or mountpoint
crossing would have), it simply stays at the root of subtree, same as it
would for chroot jail.  Wouldn't be too hard to make it fail (a couple of
if (...) break; in follow_dotdot,{_rcu}() turned into if (unlikely(...)) {
	if (unlikely(nd->flags & LOOKUP_NO_JUMPS)) return -ELOOP;
	break;
}); not sure if it's worth bothering with.
	3) absolute pathname with LOOKUP_NO_JUMPS => -ELOOP at the moment.
Wouldn't be hard to change, if somebody proposes a sane semantics.

Any comments would be welcome; is that a sane behaviour to implement, what 
changes (if any) would be needed to make it useful, etc.

Current variant of patch follows:

commit 8020273120c30b1444f67f7bbee2f20cee314d02
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Sat Mar 18 16:27:55 2017 -0400

    namei: new flag (LOOKUP_NO_JUMPS)
    
    semantics:
            * no mountpoint crossing allowed
            * only relative symlinks allowed
            * starting point acts as chroot boundary (i.e.
    .. does not lead out of it)
    
    Matching AT_... flag: AT_NO_JUMPS introduced, fstatat(2) (and
    corresponding statx/stat64 variants) taught about it.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/namei.c b/fs/namei.c
index d41fab78798b..57264a82878b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1054,10 +1054,15 @@ const char *get_link(struct nameidata *nd)
 		} else {
 			res = get(dentry, inode, &last->done);
 		}
+		if (unlikely(nd->flags & LOOKUP_NO_JUMPS) &&
+		    unlikely(nd->flags & LOOKUP_JUMPED))
+			return ERR_PTR(-ELOOP);
 		if (IS_ERR_OR_NULL(res))
 			return res;
 	}
 	if (*res == '/') {
+		if (unlikely(nd->flags & LOOKUP_NO_JUMPS))
+			return ERR_PTR(-ELOOP);
 		if (!nd->root.mnt)
 			set_root(nd);
 		if (unlikely(nd_jump_root(nd)))
@@ -1245,12 +1250,16 @@ static int follow_managed(struct path *path, struct nameidata *nd)
 		break;
 	}
 
-	if (need_mntput && path->mnt == mnt)
-		mntput(path->mnt);
+	if (need_mntput) {
+		if (path->mnt == mnt)
+			mntput(path->mnt);
+		if (unlikely(nd->flags & LOOKUP_NO_JUMPS))
+			ret = -ELOOP;
+		else
+			nd->flags |= LOOKUP_JUMPED;
+	}
 	if (ret == -EISDIR || !ret)
 		ret = 1;
-	if (need_mntput)
-		nd->flags |= LOOKUP_JUMPED;
 	if (unlikely(ret < 0))
 		path_put_conditional(path, nd);
 	return ret;
@@ -1307,6 +1316,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
 		mounted = __lookup_mnt(path->mnt, path->dentry);
 		if (!mounted)
 			break;
+		if (unlikely(nd->flags & LOOKUP_NO_JUMPS))
+			return false;
 		path->mnt = &mounted->mnt;
 		path->dentry = mounted->mnt.mnt_root;
 		nd->flags |= LOOKUP_JUMPED;
@@ -2177,6 +2188,8 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 
 	nd->m_seq = read_seqbegin(&mount_lock);
 	if (*s == '/') {
+		if (unlikely(flags & LOOKUP_NO_JUMPS))
+			return ERR_PTR(-ELOOP);
 		if (flags & LOOKUP_RCU)
 			rcu_read_lock();
 		set_root(nd);
@@ -2202,6 +2215,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 			get_fs_pwd(current->fs, &nd->path);
 			nd->inode = nd->path.dentry->d_inode;
 		}
+		if (unlikely(flags & LOOKUP_NO_JUMPS)) {
+			nd->root = nd->path;
+			if (!(flags & LOOKUP_RCU))
+				path_get(&nd->root);
+		}
 		return s;
 	} else {
 		/* Caller must check execute permissions on the starting path component */
@@ -2229,6 +2247,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 			path_get(&nd->path);
 			nd->inode = nd->path.dentry->d_inode;
 		}
+		if (unlikely(flags & LOOKUP_NO_JUMPS)) {
+			nd->root = nd->path;
+			if (!(flags & LOOKUP_RCU))
+				path_get(&nd->root);
+		}
 		fdput(f);
 		return s;
 	}
diff --git a/fs/stat.c b/fs/stat.c
index fa0be59340cc..1999ce5f77c9 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -168,7 +168,7 @@ int vfs_statx(int dfd, const char __user *filename, int flags,
 	unsigned int lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT;
 
 	if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
-		       AT_EMPTY_PATH | KSTAT_QUERY_FLAGS)) != 0)
+		       AT_EMPTY_PATH | KSTAT_QUERY_FLAGS | AT_NO_JUMPS)) != 0)
 		return -EINVAL;
 
 	if (flags & AT_SYMLINK_NOFOLLOW)
@@ -177,6 +177,8 @@ int vfs_statx(int dfd, const char __user *filename, int flags,
 		lookup_flags &= ~LOOKUP_AUTOMOUNT;
 	if (flags & AT_EMPTY_PATH)
 		lookup_flags |= LOOKUP_EMPTY;
+	if (flags & AT_NO_JUMPS)
+		lookup_flags |= LOOKUP_NO_JUMPS;
 
 retry:
 	error = user_path_at(dfd, filename, lookup_flags, &path);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index f29abda31e6d..593a998da846 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -44,6 +44,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_JUMPED		0x1000
 #define LOOKUP_ROOT		0x2000
 #define LOOKUP_EMPTY		0x4000
+#define LOOKUP_NO_JUMPS		0x8000
 
 extern int path_pts(struct path *path);
 
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 813afd6eee71..ca35ef523e40 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -68,5 +68,6 @@
 #define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
 #define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
 
+#define AT_NO_JUMPS		0x8000	/* No mountpoint crossing, no abs symlinks */
 
 #endif /* _UAPI_LINUX_FCNTL_H */

Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

[Linus Torvalds]

On Sun, Mar 19, 2017 at 10:24 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>         Bringing back an old conversation - what do you think about the
> potential usefulness of the following ...at() option:
>         * no mountpoint crossings allowed (mount --bind included)
>         * only relative symlinks traversals are allowed
>         * starting point acts as a chroot boundary (IOW, .. does not lead
> out of it)

Hmm. The original use of this was iirc apache (or maybe samba), that
simply wanted to be sure that when you did a pathname lookup, it
didn't escape out of the subtree that the lookup was done from (eg
some public_html directory or whatever).

But I had that discussions *long* ago, and I don't even remember who
it might have been with. My reaction at the time was simply that it
would have been easy for us to have a "no symlink traversal at all"
mode, because their solution was to just walk each path component
carefully by hand.

Maybe somebody who knows apache (or samba) can pipe up and say "yeah,
we still do that", or "we solved it with our own filename cache, so we
don't care".

> IIRC, you mentioned that something of that kind might be interesting for
> git et.al. and it turns out to be surprisingly easy to implement -

So for git, we have a notion of "real path", and your thing doesn't
solve that, because it really is "return the actual canonical path to
the filename given by xyz".

And _usually_ the real path of xyz is trivially xyz. But if xyz
contains a symlink, or if xyz contains a "..", that gets resolved. But
that needs to happen even *if* the path stays inside the project.

So git *could* make use of something that errored out for anything
that wasn't canonical: that includes using "." and "" in the pathname,
btw, but also symlinks and "..".

That would meke the "realpath()" logic be:

 - try to look it up using the "strict canonical case only", and if
that succeeds, we just return the original pathname

 - otherwise do the per-component lookup and fix it up manually.

So for git, escaping the subdirectory is only one small issue, and
your patch wouldn't be sufficient because it still allows symlinks
that don't escape the tree.

(Also, honestly, I don't think it's ever a real performance issue, and
since git would need to have the fallback code to do the resolution
anyway, it doesn't help from a complexity standpoint either).

So I don't think the patch is interesting for git at least as-is, and
because of complexity and portability, possibly not even if it did add
a flag to always error out on the special path components of "", ".",
"..", and symlinks.

I think the concept makes sense, and we might well want to encourage
it as a way for people to efficiently and easily verify that the end
result of a lookup stays inside a certain directory structure, but I
think we'd want to have a champion of this feature that would actually
use it.

Maybe somebody on fsdevel knows of a particular load that is
performance-critical and important for some group of people, and
currently looks up pathnames component by component because of issues
like this..

Anybody?

                        Linus

Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

[Omar Sandoval]

On Sun, Mar 19, 2017 at 06:46:01PM -0700, Linus Torvalds wrote:
> On Sun, Mar 19, 2017 at 10:24 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >         Bringing back an old conversation - what do you think about the
> > potential usefulness of the following ...at() option:
> >         * no mountpoint crossings allowed (mount --bind included)
> >         * only relative symlinks traversals are allowed
> >         * starting point acts as a chroot boundary (IOW, .. does not lead
> > out of it)
> 
> Hmm. The original use of this was iirc apache (or maybe samba), that
> simply wanted to be sure that when you did a pathname lookup, it
> didn't escape out of the subtree that the lookup was done from (eg
> some public_html directory or whatever).
> 
> But I had that discussions *long* ago, and I don't even remember who
> it might have been with. My reaction at the time was simply that it
> would have been easy for us to have a "no symlink traversal at all"
> mode, because their solution was to just walk each path component
> carefully by hand.
> 
> Maybe somebody who knows apache (or samba) can pipe up and say "yeah,
> we still do that", or "we solved it with our own filename cache, so we
> don't care".

The last posting of the O_BENEATH flag with very similar semantics
indicated that there were sandboxing usecases that still want this [1].
I think it's a good idea; the kernel is in the best place to do this
correctly instead of having a bunch of half-baked implementations in
userspace everywhere.

1: http://marc.info/?l=linux-fsdevel&m=143945842819081&w=2

Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On Sun 2017-03-19 17:24:15, Al Viro wrote:
> 	Bringing back an old conversation - what do you think about the
> potential usefulness of the following ...at() option:
> 	* no mountpoint crossings allowed (mount --bind included)

Returning error or returning the object that should be hidden by the
mount?

I believe the second option would be a bit dangerous...
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

[Al Viro]

On Tue, May 02, 2017 at 09:57:40PM +0200, Pavel Machek wrote:
> On Sun 2017-03-19 17:24:15, Al Viro wrote:
> > 	Bringing back an old conversation - what do you think about the
> > potential usefulness of the following ...at() option:
> > 	* no mountpoint crossings allowed (mount --bind included)
> 
> Returning error or returning the object that should be hidden by the
> mount?

Error, obviously - as clearly said a few lines below...