Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

2017-05-02 Thread 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...


Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

2017-05-02 Thread Pavel Machek
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


signature.asc
Description: Digital signature


Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

2017-03-20 Thread 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  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=143945842819081=2


Re: [RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

2017-03-19 Thread Linus Torvalds
On Sun, Mar 19, 2017 at 10:24 AM, 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)
> * 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


[RFC] AT_NO_JUMPS/LOOKUP_NO_JUMPS

2017-03-19 Thread 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 
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 

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, >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 = >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(_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, >path);
nd->inode =