Message ID | 20170319172414.GT29622@ZenIV.linux.org.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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
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
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...
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 */
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>