Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1633356 > unrolled thread

new ...at() flag: AT_NO_JUMPS

Back to article view | Back to linux.kernel

Contents

  new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-30 00:10 +0200
    Re: new ...at() flag: AT_NO_JUMPS Andy Lutomirski <luto@kernel.org> - 2017-04-30 01:20 +0200
      Re: new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-30 01:30 +0200
        Re: new ...at() flag: AT_NO_JUMPS Andy Lutomirski <luto@kernel.org> - 2017-04-30 03:20 +0200
        Re: new ...at() flag: AT_NO_JUMPS Matthew Wilcox <willy@infradead.org> - 2017-04-30 06:40 +0200
          Re: new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-30 18:20 +0200
            Re: new ...at() flag: AT_NO_JUMPS Andy Lutomirski <luto@kernel.org> - 2017-05-01 07:00 +0200
              Re: new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-01 07:20 +0200
    Re: new ...at() flag: AT_NO_JUMPS Jann Horn <jannh@google.com> - 2017-05-01 19:40 +0200
      Re: new ...at() flag: AT_NO_JUMPS Andy Lutomirski <luto@kernel.org> - 2017-05-01 21:40 +0200
      Re: new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-05 02:40 +0200
        Re: new ...at() flag: AT_NO_JUMPS Andy Lutomirski <luto@kernel.org> - 2017-05-05 02:50 +0200
          Re: new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-05 03:10 +0200
        Re: new ...at() flag: AT_NO_JUMPS Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-05 03:30 +0200
          Re: new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-05 05:10 +0200
            Re: new ...at() flag: AT_NO_JUMPS Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-05 06:10 +0200
              Re: new ...at() flag: AT_NO_JUMPS Andy Lutomirski <luto@kernel.org> - 2017-05-05 06:40 +0200
        Re: new ...at() flag: AT_NO_JUMPS Jann Horn <jannh@google.com> - 2017-05-05 04:50 +0200
          Re: new ...at() flag: AT_NO_JUMPS Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-05 05:50 +0200
            Re: new ...at() flag: AT_NO_JUMPS Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-05 06:40 +0200
              Re: new ...at() flag: AT_NO_JUMPS Andy Lutomirski <luto@kernel.org> - 2017-05-05 06:50 +0200
                Re: new ...at() flag: AT_NO_JUMPS ebiederm@xmission.com (Eric W. Biederman) - 2017-05-05 22:20 +0200
              Re: new ...at() flag: AT_NO_JUMPS ebiederm@xmission.com (Eric W. Biederman) - 2017-05-05 22:40 +0200
                Re: new ...at() flag: AT_NO_JUMPS Mickaël Salaün <mic@digikod.net> - 2017-05-08 21:40 +0200

Page 1 of 2  [1] 2  Next page →

#1633356 — new ...at() flag: AT_NO_JUMPS

New AT_... flag - AT_NO_JUMPS

Semantics: pathname resolution must not involve
	* traversals of absolute symlinks
	* traversals of procfs-style symlinks
	* traversals of mountpoints (including bindings, referrals, etc.)
	* traversal of .. in the starting point of pathname resolution.

All of those lead to failure with -ELOOP.  Relative symlinks are fine,
as long as their resolution does not end up stepping into the conditions
above.

It guarantees that result of successful pathname resolution will be on the
same filesystem as its starting point and within the subtree rooted at
the starting point.

Right now I have it hooked only for fstatat() and friends; it could be
easily extended to any ...at() syscalls.  Objections?

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

    namei: new flag (LOOKUP_NO_JUMPS)
    
    semantics: fail with -ELOOP upon
            * attempt to cross mountpoint (including bindings)
            * attempt to traverse a non-relative symlink
            * attempt to cross the starting point by ".." traversal
    
    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..de1f07ec8ccd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -874,6 +874,8 @@ static int nd_jump_root(struct nameidata *nd)
 		path_get(&nd->path);
 		nd->inode = nd->path.dentry->d_inode;
 	}
+	if (unlikely(nd->flags & LOOKUP_NO_JUMPS))
+		return -ELOOP;
 	nd->flags |= LOOKUP_JUMPED;
 	return 0;
 }
@@ -1054,14 +1056,18 @@ 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 (!nd->root.mnt)
 			set_root(nd);
-		if (unlikely(nd_jump_root(nd)))
-			return ERR_PTR(-ECHILD);
+		error = nd_jump_root(nd);
+		if (unlikely(error))
+			return ERR_PTR(error);
 		while (unlikely(*++res == '/'))
 			;
 	}
@@ -1245,12 +1251,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 +1317,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;
@@ -1327,8 +1339,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 	struct inode *inode = nd->inode;
 
 	while (1) {
-		if (path_equal(&nd->path, &nd->root))
+		if (unlikely(path_equal(&nd->path, &nd->root))) {
+			if (nd->flags & LOOKUP_NO_JUMPS)
+				return -ELOOP;
 			break;
+		}
 		if (nd->path.dentry != nd->path.mnt->mnt_root) {
 			struct dentry *old = nd->path.dentry;
 			struct dentry *parent = old->d_parent;
@@ -1455,8 +1470,9 @@ static int path_parent_directory(struct path *path)
 static int follow_dotdot(struct nameidata *nd)
 {
 	while(1) {
-		if (nd->path.dentry == nd->root.dentry &&
-		    nd->path.mnt == nd->root.mnt) {
+		if (unlikely(path_equal(&nd->path, &nd->root))) {
+			if (nd->flags & LOOKUP_NO_JUMPS)
+				return -ELOOP;
 			break;
 		}
 		if (nd->path.dentry != nd->path.mnt->mnt_root) {
@@ -2177,14 +2193,16 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 
 	nd->m_seq = read_seqbegin(&mount_lock);
 	if (*s == '/') {
+		int error;
 		if (flags & LOOKUP_RCU)
 			rcu_read_lock();
 		set_root(nd);
-		if (likely(!nd_jump_root(nd)))
-			return s;
-		nd->root.mnt = NULL;
-		rcu_read_unlock();
-		return ERR_PTR(-ECHILD);
+		error = nd_jump_root(nd);
+		if (unlikely(error)) {
+			terminate_walk(nd);
+			s = ERR_PTR(error);
+		}
+		return s;
 	} else if (nd->dfd == AT_FDCWD) {
 		if (flags & LOOKUP_RCU) {
 			struct fs_struct *fs = current->fs;
@@ -2202,6 +2220,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 +2252,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..3cefb90f38ca 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -45,6 +45,8 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_ROOT		0x2000
 #define LOOKUP_EMPTY		0x4000
 
+#define LOOKUP_NO_JUMPS		0x10000
+
 extern int path_pts(struct path *path);
 
 extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
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 */

[toc] | [next] | [standalone]

#1633360

On Sat, Apr 29, 2017 at 3:04 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> New AT_... flag - AT_NO_JUMPS
>
> Semantics: pathname resolution must not involve
>         * traversals of absolute symlinks
>         * traversals of procfs-style symlinks
>         * traversals of mountpoints (including bindings, referrals, etc.)
>         * traversal of .. in the starting point of pathname resolution.

Can you clarify this last one?  I assume that ".." will be rejected,
but what about "a/../.."?  How about "b" if b is a symlink to ".."?
How about "a/b" if a is a directory and b is a symlink to "../.."?

> Right now I have it hooked only for fstatat() and friends; it could be
> easily extended to any ...at() syscalls.  Objections?

I like it, assuming the answers to all the questions above are that
they will be rejected.

[toc] | [prev] | [next] | [standalone]

#1633361

On Sat, Apr 29, 2017 at 04:17:18PM -0700, Andy Lutomirski wrote:
> On Sat, Apr 29, 2017 at 3:04 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > New AT_... flag - AT_NO_JUMPS
> >
> > Semantics: pathname resolution must not involve
> >         * traversals of absolute symlinks
> >         * traversals of procfs-style symlinks
> >         * traversals of mountpoints (including bindings, referrals, etc.)
> >         * traversal of .. in the starting point of pathname resolution.
> 
> Can you clarify this last one?  I assume that ".." will be rejected,
> but what about "a/../.."?  How about "b" if b is a symlink to ".."?
> How about "a/b" if a is a directory and b is a symlink to "../.."?

All of those will be rejected - in each of those cases pathname traversal
leads back into the starting point with .. being the next component to
handle.

[toc] | [prev] | [next] | [standalone]

#1633366

On Sat, Apr 29, 2017 at 4:25 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Apr 29, 2017 at 04:17:18PM -0700, Andy Lutomirski wrote:
>> On Sat, Apr 29, 2017 at 3:04 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> > New AT_... flag - AT_NO_JUMPS
>> >
>> > Semantics: pathname resolution must not involve
>> >         * traversals of absolute symlinks
>> >         * traversals of procfs-style symlinks
>> >         * traversals of mountpoints (including bindings, referrals, etc.)
>> >         * traversal of .. in the starting point of pathname resolution.
>>
>> Can you clarify this last one?  I assume that ".." will be rejected,
>> but what about "a/../.."?  How about "b" if b is a symlink to ".."?
>> How about "a/b" if a is a directory and b is a symlink to "../.."?
>
> All of those will be rejected - in each of those cases pathname traversal
> leads back into the starting point with .. being the next component to
> handle.

Sounds good.

Might it make sense to split it into two flags, one to prevent moving
between mounts and one for everything else?  I can imagine webservers
and such that are fine with traversing mount points but don't want to
escape their home directory.

--Andy

[toc] | [prev] | [next] | [standalone]

#1633377

On Sun, Apr 30, 2017 at 12:25:04AM +0100, Al Viro wrote:
> On Sat, Apr 29, 2017 at 04:17:18PM -0700, Andy Lutomirski wrote:
> > On Sat, Apr 29, 2017 at 3:04 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > New AT_... flag - AT_NO_JUMPS
> > >
> > > Semantics: pathname resolution must not involve
> > >         * traversals of absolute symlinks
> > >         * traversals of procfs-style symlinks
> > >         * traversals of mountpoints (including bindings, referrals, etc.)
> > >         * traversal of .. in the starting point of pathname resolution.
> > 
> > Can you clarify this last one?  I assume that ".." will be rejected,
> > but what about "a/../.."?  How about "b" if b is a symlink to ".."?
> > How about "a/b" if a is a directory and b is a symlink to "../.."?
> 
> All of those will be rejected - in each of those cases pathname traversal
> leads back into the starting point with .. being the next component to
> handle.

It sounds more like AT_NO_ESCAPE ... or AT_BELOW, or something.  Perhaps
some example usages in the changelog?

[toc] | [prev] | [next] | [standalone]

#1633450

On Sat, Apr 29, 2017 at 09:38:22PM -0700, Matthew Wilcox wrote:

> It sounds more like AT_NO_ESCAPE ... or AT_BELOW, or something.

I considered AT_ROACH_MOTEL at one point...  Another interesting
question is whether EXDEV would've been better than ELOOP.
Opinions?

[toc] | [prev] | [next] | [standalone]

#1633546

On Sun, Apr 30, 2017 at 9:10 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Apr 29, 2017 at 09:38:22PM -0700, Matthew Wilcox wrote:
>
>> It sounds more like AT_NO_ESCAPE ... or AT_BELOW, or something.
>
> I considered AT_ROACH_MOTEL at one point...  Another interesting
> question is whether EXDEV would've been better than ELOOP.
> Opinions?

In support of my homeland, I propose AT_HOTEL_CALIFORNIA.

How about EXDEV for crossing a mountpoint and ELOOP for absolute
symlinks or invalid ..?  (Is there a technical reason why the same AT_
flag should trigger both cases?)

--Andy

[toc] | [prev] | [next] | [standalone]

#1633548

On Sun, Apr 30, 2017 at 09:52:37PM -0700, Andy Lutomirski wrote:
> On Sun, Apr 30, 2017 at 9:10 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Sat, Apr 29, 2017 at 09:38:22PM -0700, Matthew Wilcox wrote:
> >
> >> It sounds more like AT_NO_ESCAPE ... or AT_BELOW, or something.
> >
> > I considered AT_ROACH_MOTEL at one point...  Another interesting
> > question is whether EXDEV would've been better than ELOOP.
> > Opinions?
> 
> In support of my homeland, I propose AT_HOTEL_CALIFORNIA.
> 
> How about EXDEV for crossing a mountpoint and ELOOP for absolute
> symlinks or invalid ..?  (Is there a technical reason why the same AT_
> flag should trigger both cases?)

You do realize that mount --bind can do everything absolute symlinks could,
right?  And absolute symlinks most likely do lead to (or at least through)
a different fs...

[toc] | [prev] | [next] | [standalone]

#1633764

On Sun, Apr 30, 2017 at 12:04 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> New AT_... flag - AT_NO_JUMPS
>
> Semantics: pathname resolution must not involve
>         * traversals of absolute symlinks
>         * traversals of procfs-style symlinks
>         * traversals of mountpoints (including bindings, referrals, etc.)
>         * traversal of .. in the starting point of pathname resolution.
>
> All of those lead to failure with -ELOOP.  Relative symlinks are fine,
> as long as their resolution does not end up stepping into the conditions
> above.
>
> It guarantees that result of successful pathname resolution will be on the
> same filesystem as its starting point and within the subtree rooted at
> the starting point.
>
> Right now I have it hooked only for fstatat() and friends; it could be
> easily extended to any ...at() syscalls.  Objections?

Oh, nice!

It looks like this is somewhat similar to the old O_BENEATH proposal,
but because the intentions behind the proposals are different
(application sandboxing versus permitting an application to restrict its
own filesystem accesses), the semantics differ: AT_NO_JUMPS
doesn't prevent starting the path with "/", but does prevent mountpoint
traversal. Is that correct?

I think that, as Andy mentioned, it might make sense to split out (or
even remove?) the prevention of mountpoint traversal. A user who
can create visible mountpoints needs to have capabilities over the
mount namespace the file descriptor refers to already.

I suspect that if this lands, it would be pretty straightforward to add
another flag AT_NO_ABSOLUTE or so that, combined with
AT_NO_JUMPS, has the same semantics as O_BENEATH?

[toc] | [prev] | [next] | [standalone]

#1633834

On Mon, May 1, 2017 at 10:36 AM, Jann Horn <jannh@google.com> wrote:
> On Sun, Apr 30, 2017 at 12:04 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> New AT_... flag - AT_NO_JUMPS
>>
>> Semantics: pathname resolution must not involve
>>         * traversals of absolute symlinks
>>         * traversals of procfs-style symlinks
>>         * traversals of mountpoints (including bindings, referrals, etc.)
>>         * traversal of .. in the starting point of pathname resolution.
>>
>> All of those lead to failure with -ELOOP.  Relative symlinks are fine,
>> as long as their resolution does not end up stepping into the conditions
>> above.
>>
>> It guarantees that result of successful pathname resolution will be on the
>> same filesystem as its starting point and within the subtree rooted at
>> the starting point.
>>
>> Right now I have it hooked only for fstatat() and friends; it could be
>> easily extended to any ...at() syscalls.  Objections?
>
> Oh, nice!
>
> It looks like this is somewhat similar to the old O_BENEATH proposal,
> but because the intentions behind the proposals are different
> (application sandboxing versus permitting an application to restrict its
> own filesystem accesses), the semantics differ: AT_NO_JUMPS
> doesn't prevent starting the path with "/", but does prevent mountpoint
> traversal. Is that correct?
>

I missed that.  I think that AT_HOTEL_CALIFORNIA or whatever we call
it should disallow even explicit absolute paths.  If I do:

openat([fd to /var/www], "possibly untrusted path here",
AT_HOTEL_CALIFORNIA, O_WHATEVER);

I should not have to separately verify that the path doesn't start
with "/" to make sure that I don't escape.  There's a big added
advantage of this approach, too: I could write a seccomp rule that
only lets me call openat() with this new flag set, and now I can't
escape.


> I think that, as Andy mentioned, it might make sense to split out (or
> even remove?) the prevention of mountpoint traversal. A user who
> can create visible mountpoints needs to have capabilities over the
> mount namespace the file descriptor refers to already.

Agreed.  There's a big difference between the admin bind-mounting /etc
into /var/www and some web app putting a symlink to /etc into
/var/www.

[toc] | [prev] | [next] | [standalone]

#1636106

On Mon, May 01, 2017 at 07:36:52PM +0200, Jann Horn wrote:

> Oh, nice!
> 
> It looks like this is somewhat similar to the old O_BENEATH proposal,
> but because the intentions behind the proposals are different
> (application sandboxing versus permitting an application to restrict its
> own filesystem accesses), the semantics differ: AT_NO_JUMPS
> doesn't prevent starting the path with "/", but does prevent mountpoint
> traversal. Is that correct?

It prevents both, actually - I missed that in description, but this
        if (unlikely(nd->flags & LOOKUP_NO_JUMPS))
                return -ELOOP;
in nd_jump_root() affects absolute pathnames same way as it affects
absolute symlinks.

It's not quite O_BENEATH, and IMO it's saner that way - a/b/c/../d is
bloody well allowed, and so are relative symlinks that do not lead out of
the subtree.  If somebody has a good argument in favour of flat-out
ban on .. (_other_ than "other guys do it that way, and it doesn't need
to make sense 'cuz security!!1!!!", please), I'd be glad to hear it.

As for mountpoint crossing...  it might make sense to split those.
O_BENEATH allowed it, and if we want AT_BENEATH to match that - let's
do it.  Then this one would become AT_BENEATH | AT_XDEV (the latter named
after find(1) option, obviously).

So how about this:

AT_BENEATH:
	* no absolute pathnames
	* no absolute symlinks
	* no procfs-style symlinks
	* no traversal of .. when we are at the same place where we'd started
(dir/../file is allowed, dir/../.. isn't)

AT_XDEV:
	* no mountpoint crossing allowed

For the latter I would prefer -EXDEV, for obvious reasons.  For the former...
not sure.  I'm not too happy about -ELOOP, but -EPERM (as with O_BENEATH)
is an atrocity - it's even more overloaded.

Suggestions?

[toc] | [prev] | [next] | [standalone]

#1636107

On Thu, May 4, 2017 at 5:30 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, May 01, 2017 at 07:36:52PM +0200, Jann Horn wrote:
>
>> Oh, nice!
>>
>> It looks like this is somewhat similar to the old O_BENEATH proposal,
>> but because the intentions behind the proposals are different
>> (application sandboxing versus permitting an application to restrict its
>> own filesystem accesses), the semantics differ: AT_NO_JUMPS
>> doesn't prevent starting the path with "/", but does prevent mountpoint
>> traversal. Is that correct?
>
> It prevents both, actually - I missed that in description, but this
>         if (unlikely(nd->flags & LOOKUP_NO_JUMPS))
>                 return -ELOOP;
> in nd_jump_root() affects absolute pathnames same way as it affects
> absolute symlinks.
>
> It's not quite O_BENEATH, and IMO it's saner that way - a/b/c/../d is
> bloody well allowed, and so are relative symlinks that do not lead out of
> the subtree.  If somebody has a good argument in favour of flat-out
> ban on .. (_other_ than "other guys do it that way, and it doesn't need
> to make sense 'cuz security!!1!!!", please), I'd be glad to hear it.

I don't have an argument for allowing '..'.  I think it would be okay
to disallow it, but I don't think it matters all that much either way.

>
> As for mountpoint crossing...  it might make sense to split those.
> O_BENEATH allowed it, and if we want AT_BENEATH to match that - let's
> do it.  Then this one would become AT_BENEATH | AT_XDEV (the latter named
> after find(1) option, obviously).
>
> So how about this:
>
> AT_BENEATH:
>         * no absolute pathnames
>         * no absolute symlinks
>         * no procfs-style symlinks
>         * no traversal of .. when we are at the same place where we'd started
> (dir/../file is allowed, dir/../.. isn't)
>
> AT_XDEV:
>         * no mountpoint crossing allowed
>
> For the latter I would prefer -EXDEV, for obvious reasons.  For the former...
> not sure.  I'm not too happy about -ELOOP, but -EPERM (as with O_BENEATH)
> is an atrocity - it's even more overloaded.
>
> Suggestions?

-EDOTDOT would be amusing.

[toc] | [prev] | [next] | [standalone]

#1636110

On Thu, May 04, 2017 at 05:44:19PM -0700, Andy Lutomirski wrote:
> > It's not quite O_BENEATH, and IMO it's saner that way - a/b/c/../d is
> > bloody well allowed, and so are relative symlinks that do not lead out of
> > the subtree.  If somebody has a good argument in favour of flat-out
> > ban on .. (_other_ than "other guys do it that way, and it doesn't need
> > to make sense 'cuz security!!1!!!", please), I'd be glad to hear it.
> 
> I don't have an argument for allowing '..'.  I think it would be okay
> to disallow it, but I don't think it matters all that much either way.

Relative symlinks as argument in favour of allowing .. _when_ _it_ _stays_
_in_ _subtree_.

> > For the latter I would prefer -EXDEV, for obvious reasons.  For the former...
> > not sure.  I'm not too happy about -ELOOP, but -EPERM (as with O_BENEATH)
> > is an atrocity - it's even more overloaded.
> >
> > Suggestions?
> 
> -EDOTDOT would be amusing.

For ln -s /tmp foo/bar, lookup for foo/bar/baz?  Seriously?  Hell, even
-EXDEV would make more sense...

[toc] | [prev] | [next] | [standalone]

#1636113

On Thu, May 4, 2017 at 5:30 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> As for mountpoint crossing...  it might make sense to split those.
> O_BENEATH allowed it, and if we want AT_BENEATH to match that - let's
> do it.  Then this one would become AT_BENEATH | AT_XDEV (the latter named
> after find(1) option, obviously).

So I would still like to split that NO_JUMP flag even more.

I like the AT_BENEATH | AT_XDEV split, but I think XDEV should be
split further, and I think the symlink avoidance should be split more
too.

As mentioned last time, at least for the git usage, even relative
symlinks are a no-no - not because they'd escape, but simply because
git wants to see the *unique* name, and resolve relative symlinks to
either the symlink, or to the actual file it points to.

So I think that we'd want an additional flag that says "no symlinks at all".

And I think the "no mountpoint" traversal might be splittable too.

Yes, sometimes you'd probably want to say "stay exactly inside this
filesystem" (like find -xdev). So no arguments against AT_XDEV that
refuses any mount traversal (kind of like my "no symlink traversal"
thing).

But at other points you might want to just guarantee that the walk
stays below a certain starting point and doesn't escape.

That could still allow crossing mount-points, but only if they are
non-bind mounts and cannot let us escape.

I'm not sure if that's testable, though.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1636135

On Thu, May 04, 2017 at 06:27:10PM -0700, Linus Torvalds wrote:

> As mentioned last time, at least for the git usage, even relative
> symlinks are a no-no - not because they'd escape, but simply because
> git wants to see the *unique* name, and resolve relative symlinks to
> either the symlink, or to the actual file it points to.
> 
> So I think that we'd want an additional flag that says "no symlinks at all".

OK, that's easily done.

> And I think the "no mountpoint" traversal might be splittable too.
> 
> Yes, sometimes you'd probably want to say "stay exactly inside this
> filesystem" (like find -xdev). So no arguments against AT_XDEV that
> refuses any mount traversal (kind of like my "no symlink traversal"
> thing).
> 
> But at other points you might want to just guarantee that the walk
> stays below a certain starting point and doesn't escape.
> 
> That could still allow crossing mount-points, but only if they are
> non-bind mounts and cannot let us escape.
> 
> I'm not sure if that's testable, though.

This one isn't, unfortunately - there is no difference between bind and
no-bind; vfsmounts form a tree and both normal mount and bind add leaves
to it.  Moreover, mount -t ext2 /dev/sdc7 /mnt; mount -t ext2 /dev/sdc7 /tmp/a
yield the same state as mount -t ext2 /dev/sdc7; mount --bind /mnt /tmp/a.
There is no way to tell the difference, simply because there *is* no
difference.  Moreover, either can be followed by umount /mnt and you'll get
the same state as you would have after a solitary mount of the same fs on
/tmp/a.

Ho-hum...  So:

			AT_BENEATH	AT_XDEV		AT_NO_SYMLINKS
absolute pathname:	EXDEV
non-relative symlink:	EXDEV		?		ELOOP
relative symlink:					ELOOP
.. from starting point:	EXDEV
.. crossing mountpoint:			EXDEV
crossing into mountpoint:		EXDEV

1) What should AT_XDEV do about absolute symlinks?  Nothing special?  EXDEV?
EXDEV if we are not on root?
2) What should AT_BENEATH | AT_NO_SYMLINKS do on absolute symlinks?  My
preference would be "AT_NO_SYMLINKS wins, ELOOP for you", but that's based
mostly upon the convenience of implementation.
3) What effect should AT_NO_SYMLINKS have upon the final component?  Same
as AT_SYMLINK_NOFOLLOW?

[toc] | [prev] | [next] | [standalone]

#1636150

On Thu, May 4, 2017 at 8:00 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> That could still allow crossing mount-points, but only if they are
>> non-bind mounts and cannot let us escape.
>>
>> I'm not sure if that's testable, though.
>
> This one isn't, unfortunately - there is no difference between bind and
> no-bind; vfsmounts form a tree and both normal mount and bind add leaves
> to it.  Moreover, mount -t ext2 /dev/sdc7 /mnt; mount -t ext2 /dev/sdc7 /tmp/a
> yield the same state as mount -t ext2 /dev/sdc7; mount --bind /mnt /tmp/a.
> There is no way to tell the difference, simply because there *is* no
> difference.  Moreover, either can be followed by umount /mnt and you'll get
> the same state as you would have after a solitary mount of the same fs on
> /tmp/a.

Fair enough.

> Ho-hum...  So:
>
>                         AT_BENEATH      AT_XDEV         AT_NO_SYMLINKS
> absolute pathname:      EXDEV
> non-relative symlink:   EXDEV           ?               ELOOP
> relative symlink:                                       ELOOP
> .. from starting point: EXDEV
> .. crossing mountpoint:                 EXDEV
> crossing into mountpoint:               EXDEV
>
> 1) What should AT_XDEV do about absolute symlinks?  Nothing special?  EXDEV?
> EXDEV if we are not on root?

My mental model would say that AT_XDEV without AT_BENEATH would
_logically_ result in "EXDEV if / is a different vfsmount", accept the
absolute path otherwise.

But honestly, just returning EXDEV unconditionally for an absolute
symlink might just be the simpler and more straightforward thing to
do.

Because testing the particular vfsmount of / simply doesn't seem to be
a very useful operation.  I dunno.

> 2) What should AT_BENEATH | AT_NO_SYMLINKS do on absolute symlinks?  My
> preference would be "AT_NO_SYMLINKS wins, ELOOP for you", but that's based
> mostly upon the convenience of implementation.

I think either is fine, and convenience wins.

> 3) What effect should AT_NO_SYMLINKS have upon the final component?  Same
> as AT_SYMLINK_NOFOLLOW?

I actually would suggest "error if it's followed".

So if you use AT_SYMLINK_NOFOLLOW | AT_NO_SYMLINKS, then you do *not*
get an error if the last component (but nothing before it) is a
symlink, and the end result is the symlink itself.

If you use just AT_NO_SYMLINKS, then the lack of NOFOLLOW implies that
you'd follow the symlink to look it up, and then AT_NO_SYMLINKS means
that you get an error (ELOOP).

So the user gets to choose, and gets to basically indicate whether
it's fine to end at a dangling symlink or not. Which is exactly what
AT_SYMLINK_NOFOLLOW is all about.

No?

                   Linus

[toc] | [prev] | [next] | [standalone]

#1636156

On Thu, May 4, 2017 at 9:01 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, May 4, 2017 at 8:00 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>
>>> That could still allow crossing mount-points, but only if they are
>>> non-bind mounts and cannot let us escape.
>>>
>>> I'm not sure if that's testable, though.
>>
>> This one isn't, unfortunately - there is no difference between bind and
>> no-bind; vfsmounts form a tree and both normal mount and bind add leaves
>> to it.  Moreover, mount -t ext2 /dev/sdc7 /mnt; mount -t ext2 /dev/sdc7 /tmp/a
>> yield the same state as mount -t ext2 /dev/sdc7; mount --bind /mnt /tmp/a.
>> There is no way to tell the difference, simply because there *is* no
>> difference.  Moreover, either can be followed by umount /mnt and you'll get
>> the same state as you would have after a solitary mount of the same fs on
>> /tmp/a.
>
> Fair enough.
>
>> Ho-hum...  So:
>>
>>                         AT_BENEATH      AT_XDEV         AT_NO_SYMLINKS
>> absolute pathname:      EXDEV
>> non-relative symlink:   EXDEV           ?               ELOOP
>> relative symlink:                                       ELOOP
>> .. from starting point: EXDEV
>> .. crossing mountpoint:                 EXDEV
>> crossing into mountpoint:               EXDEV
>>
>> 1) What should AT_XDEV do about absolute symlinks?  Nothing special?  EXDEV?
>> EXDEV if we are not on root?
>
> My mental model would say that AT_XDEV without AT_BENEATH would
> _logically_ result in "EXDEV if / is a different vfsmount", accept the
> absolute path otherwise.
>
> But honestly, just returning EXDEV unconditionally for an absolute
> symlink might just be the simpler and more straightforward thing to
> do.
>
> Because testing the particular vfsmount of / simply doesn't seem to be
> a very useful operation.  I dunno.

My intuition is that, regardless of whether it's obviously useful to
test the vfsmount, we should allow / if it's the same mount for
orthogonality and because it seems more likely to be the expected
behavior.

>
>> 3) What effect should AT_NO_SYMLINKS have upon the final component?  Same
>> as AT_SYMLINK_NOFOLLOW?
>
> I actually would suggest "error if it's followed".
>
> So if you use AT_SYMLINK_NOFOLLOW | AT_NO_SYMLINKS, then you do *not*
> get an error if the last component (but nothing before it) is a
> symlink, and the end result is the symlink itself.
>
> If you use just AT_NO_SYMLINKS, then the lack of NOFOLLOW implies that
> you'd follow the symlink to look it up, and then AT_NO_SYMLINKS means
> that you get an error (ELOOP).
>
> So the user gets to choose, and gets to basically indicate whether
> it's fine to end at a dangling symlink or not. Which is exactly what
> AT_SYMLINK_NOFOLLOW is all about.

Sounds reasonable to me.

--Andy

[toc] | [prev] | [next] | [standalone]

#1636128

+CC drysdale in case he has thoughts on this

On Fri, May 5, 2017 at 2:30 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, May 01, 2017 at 07:36:52PM +0200, Jann Horn wrote:
>
>> Oh, nice!
>>
>> It looks like this is somewhat similar to the old O_BENEATH proposal,
>> but because the intentions behind the proposals are different
>> (application sandboxing versus permitting an application to restrict its
>> own filesystem accesses), the semantics differ: AT_NO_JUMPS
>> doesn't prevent starting the path with "/", but does prevent mountpoint
>> traversal. Is that correct?
>
> It prevents both, actually - I missed that in description, but this
>         if (unlikely(nd->flags & LOOKUP_NO_JUMPS))
>                 return -ELOOP;
> in nd_jump_root() affects absolute pathnames same way as it affects
> absolute symlinks.
>
> It's not quite O_BENEATH, and IMO it's saner that way - a/b/c/../d is
> bloody well allowed, and so are relative symlinks that do not lead out of
> the subtree.  If somebody has a good argument in favour of flat-out
> ban on .. (_other_ than "other guys do it that way, and it doesn't need
> to make sense 'cuz security!!1!!!", please), I'd be glad to hear it.

One annoying edgecase might be what happens when one thread
does an AT_BENEATH walk while another thread mutates the directory
structure. If some directory that is currently being traversed by the walk
is moved out of the directory at which the walk started, would that be
detected somehow, or could the walk then follow ".." path
components up to the root directory of the current process / of the
namespace the fd is referring to? As in:

Thread 1 starts an AT_BENEATH path walk using an O_PATH fd
pointing to /srv/www/example.org/foo; the path given to the syscall is
"bar/../../../../etc/passwd". The path walk enters the "bar" directory.
Thread 2 moves /srv/www/example.org/foo/bar to
/srv/www/example.org/bar.
Thread 1 processes the rest of the path ("../../../../etc/passwd"), never
hitting /srv/www/example.org/foo in the process.

I'm not really familiar with the VFS internals, but from a coarse look
at the patch, it seems like it wouldn't block this?

[toc] | [prev] | [next] | [standalone]

#1636149

On Thu, May 4, 2017 at 7:47 PM, Jann Horn <jannh@google.com> wrote:
>
> Thread 1 starts an AT_BENEATH path walk using an O_PATH fd
> pointing to /srv/www/example.org/foo; the path given to the syscall is
> "bar/../../../../etc/passwd". The path walk enters the "bar" directory.
> Thread 2 moves /srv/www/example.org/foo/bar to
> /srv/www/example.org/bar.
> Thread 1 processes the rest of the path ("../../../../etc/passwd"), never
> hitting /srv/www/example.org/foo in the process.
>
> I'm not really familiar with the VFS internals, but from a coarse look
> at the patch, it seems like it wouldn't block this?

I think you're right.

I guess it would be safe for the RCU case due to the sequence number
check, but not the non-RCU case.

Al?

                 Linus

[toc] | [prev] | [next] | [standalone]

#1636157

On Thu, May 04, 2017 at 08:46:49PM -0700, Linus Torvalds wrote:
> On Thu, May 4, 2017 at 7:47 PM, Jann Horn <jannh@google.com> wrote:
> >
> > Thread 1 starts an AT_BENEATH path walk using an O_PATH fd
> > pointing to /srv/www/example.org/foo; the path given to the syscall is
> > "bar/../../../../etc/passwd". The path walk enters the "bar" directory.
> > Thread 2 moves /srv/www/example.org/foo/bar to
> > /srv/www/example.org/bar.
> > Thread 1 processes the rest of the path ("../../../../etc/passwd"), never
> > hitting /srv/www/example.org/foo in the process.
> >
> > I'm not really familiar with the VFS internals, but from a coarse look
> > at the patch, it seems like it wouldn't block this?
> 
> I think you're right.
> 
> I guess it would be safe for the RCU case due to the sequence number
> check, but not the non-RCU case.

	Yes and no...  FWIW, to exclude that it would suffice to have
mount --rbind /src/www/example.org/foo /srv/www/example.org/foo done first.
Then this kind of race will end up with -ENOENT due to path_connected()
logics in follow_dotdot_rcu()/follow_dotdot().  I'm not sure about the
intended applications, though - is that thing supposed to be used along with
some horror like seccomp, or...?

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web