The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
Capsicum project[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.
As per the discussion in the AT_NO_JUMPS thread, AT_NO_JUMPS has been
split into separate flags.
* AT_XDEV blocks mountpoint crossings (both upwards and downwards).
openat("/", "tmp", AT_XDEV); // blocked
openat("/tmp", "..", AT_XDEV); // blocked
openat("/tmp", "/", AT_XDEV); // blocked
* AT_NO_PROCLINKS blocks all resolution through /proc/$pid/fd/$fd
"symlinks". Specifically, this blocks all jumps caused by a
filesystem using nd_jump_link() to shove you around in the
filesystem tree (these are referred to as "proclinks" in lieu of a
better name).
openat(AT_FDCWD, "/proc/self/root", AT_NO_PROCLINKS); // blocked
openat(AT_FDCWD, "/proc/self/fd/0", AT_NO_PROCLINKS); // blocked
openat(AT_FDCWD, "/proc/self/ns/mnt", AT_NO_PROCLINKS); // blocked
* AT_BENEATH disallows escapes from the starting dirfd using ".." or
absolute paths (either in the path or during symlink resolution).
Conceptually this flag ensures that you "stay below" the starting
point in the filesystem tree. ".." resolution is allowed if it
doesn't land you outside of the starting point (this is made safe
against races by patch 3 in this series).
openat("/root", "foo", AT_BENEATH); // *not* blocked
openat("/root", "a/../b", AT_BENEATH); // *not* blocked
openat("/root", "a/../../root/b", AT_BENEATH); // blocked
openat("/root", "/root", AT_BENEATH); // blocked
AT_BENEATH also currently disallows all "proclink" resolution
because they can trivially throw you outside of the starting point.
In a future patch we might allow such resolution (as long as it
stays within the root).
openat("/", "proc/self/exe", AT_BENEATH); // blocked
In addition, two more flags have been added to the series:
* AT_NO_SYMLINKS disallows *all* symlink resolution, and thus implies
AT_NO_PROCLINKS. Linus mentioned this is something that git would
like to have in the original discussion[5].
// assuming 'ln -s / /usr'
openat("/", "/usr/bin", AT_NO_SYMLINKS); // blocked
openat("/", "/proc/self/root", AT_NO_PROCLINKS); // blocked
* AT_THIS_ROOT is a very similar idea to AT_BENEATH, but it serves a
very different purpose. Rather than blocking resolutions if they
would go outside of the starting point, it treats the starting point
as a form of chroot(2). Container runtimes are one of the primary
justifications for this flag, as they currently have to implement
this sort of path handling racily in userspace[6].
The restrictions on "proclink" resolution are the same as with
AT_BENEATH (though in AT_THIS_ROOT's case it's not really clear how
"proclink" jumps outside of the root should be handled), and patch 3
in this series was also required to make ".." resolution safe.
Currently all of these flags are only enabled for openat(2) (and thus
have their own O_* flag names), but the corresponding AT_* flags have
been reserved so they can be added to syscalls where openat(O_PATH) is
not sufficient.
Patch changelog:
v2:
* Made ".." resolution with AT_THIS_ROOT and AT_BENEATH safe(r) with
some semi-aggressive __d_path checking (see patch 3).
* Disallowed "proclinks" with AT_THIS_ROOT and AT_BENEATH, in the
hopes they can be re-enabled once safe.
* Removed the selftests as they will be reimplemented as xfstests.
* Removed stat(2) support, since you can already get it through
O_PATH and fstatat(2).
[1]: https://lwn.net/Articles/721443/
[2]: https://lore.kernel.org/patchwork/patch/784221/
[3]: https://lwn.net/Articles/619151/
[4]: https://lwn.net/Articles/603929/
[5]: https://lwn.net/Articles/723057/
[6]: https://github.com/cyphar/filepath-securejoin
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <redacted>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: David Drysdale <redacted>
Cc: <redacted>
Cc: <redacted>
Cc: <redacted>
Aleksa Sarai (3):
namei: implement O_BENEATH-style AT_* flags
namei: implement AT_THIS_ROOT chroot-like path resolution
namei: aggressively check for nd->root escape on ".." resolution
fs/fcntl.c | 2 +-
fs/namei.c | 241 +++++++++++++++++++++++--------
fs/open.c | 10 ++
fs/stat.c | 4 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 8 +
include/uapi/asm-generic/fcntl.h | 20 +++
include/uapi/linux/fcntl.h | 10 ++
8 files changed, 230 insertions(+), 68 deletions(-)
--
2.19.0
Add the following flags to allow various restrictions on path
resolution (these affect the *entire* resolution, rather than just the
final path component -- as is the case with most other AT_* flags).
The primary justification for these flags is to allow for programs to be
far more strict about how they want path resolution to handle symlinks,
mountpoint crossings, and paths that escape the dirfd (through an
absolute path or ".." shenanigans).
This is of particular concern to container runtimes that want to be very
careful about malicious root filesystems that a container's init might
have screwed around with (and there is no real way to protect against
this in userspace if you consider potential races against a malicious
container's init). More classical applications (which have their own
potentially buggy userspace path sanitisation code) include web
servers, archive extraction tools, network file servers, and so on.
* AT_XDEV: Disallow mount-point crossing (both *down* into one, or *up*
from one). The primary "scoping" use is to blocking resolution that
crosses a bind-mount, which has a similar property to a symlink (in
the way that it allows for escape from the starting-point). Since it
is not possible to differentiate bind-mounts However since
bind-mounting requires privileges (in ways symlinks don't) this has
been split from LOOKUP_BENEATH. The naming is based on "find -xdev"
(though find(1) doesn't walk upwards, the semantics seem obvious).
* AT_NO_PROCLINK: Disallows ->get_link "symlink" jumping. This is a very
specific restriction, and it exists because /proc/$pid/fd/...
"symlinks" allow for access outside nd->root and pose risk to
container runtimes that don't want to be tricked into accessing a host
path (but do want to allow no-funny-business symlink resolution).
* AT_NO_SYMLINK: Disallows symlink jumping *of any kind*. Implies
AT_NO_PROCLINK (obviously).
* AT_BENEATH: Disallow "escapes" from the starting point of the
filesystem tree during resolution (you must stay "beneath" the
starting point at all times). Currently this is done by disallowing
".." and absolute paths (either in the given path or found during
symlink resolution) entirely, as well as all "proclink" jumping.
The wholesale banning of ".." is because it is currently not safe to
allow ".." resolution (races can cause the path to be moved outside of
the root -- this is conceptually similar to historical chroot(2)
escape attacks). Future patches in this series will address this, and
will re-enable ".." resolution once it is safe. With those patches,
".." resolution will only be allowed if it remains in the root
throughout resolution (such as "a/../b" not "a/../../outside/b").
The banning of "proclink" jumping is done because it is not clear
whether semantically they should be allowed -- while some "proclinks"
are safe there are many that can cause escapes (and once a resolution
is outside of the root, AT_BENEATH will no longer detect it). Future
patches may re-enable "proclink" jumping when such jumps would remain
inside the root.
The AT_NO_*LINK flags return -ELOOP if path resolution would violates
their requirement, while the others all return -EXDEV. Currently these
are only enabled for openat(2) (which has its own brand of O_* flags
with the same semantics). However the AT_* flags have been reserved for
future support in other *at(2) syscalls (though because of AT_EMPTY_PATH
many *at(2) operations will not need to support these flags directly).
This is a refresh of Al's AT_NO_JUMPS patchset[1] (which was a variation
on David Drysdale's O_BENEATH patchset[2], which in turn was based on
the Capsicum project[3]). Input from Linus and Andy in the AT_NO_JUMPS
thread[4] determined most of the API changes made in this refresh.
[1]: https://lwn.net/Articles/721443/
[2]: https://lwn.net/Articles/619151/
[3]: https://lwn.net/Articles/603929/
[4]: https://lwn.net/Articles/723057/
Cc: Eric Biederman <redacted>
Cc: Christian Brauner <christian@brauner.io>
Suggested-by: David Drysdale <redacted>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <redacted>
---
fs/fcntl.c | 2 +-
fs/namei.c | 174 ++++++++++++++++++++++---------
fs/open.c | 8 ++
fs/stat.c | 4 +-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 7 ++
include/uapi/asm-generic/fcntl.h | 17 +++
include/uapi/linux/fcntl.h | 8 ++
8 files changed, 167 insertions(+), 56 deletions(-)
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)*Exceptions:O_NONBLOCKisatwobitdefineonparisc;O_NDELAY*isdefinedasO_NONBLOCKonsomeplatformsandnotonothers.*/-BUILD_BUG_ON(21-1/* for O_RDONLY being 0 */!=+BUILD_BUG_ON(25-1/* for O_RDONLY being 0 */!=HWEIGHT32((VALID_OPEN_FLAGS&~(O_NONBLOCK|O_NDELAY))|__FMODE_EXEC|__FMODE_NONOTIFY));
@@ -1083,14 +1089,23 @@ const char *get_link(struct nameidata *nd)}else{res=get(dentry,inode,&last->done);}+/* If we just jumped it was because of a procfs-style link. */+if(unlikely(nd->flags&LOOKUP_JUMPED)){+if(unlikely(nd->flags&LOOKUP_NO_PROCLINKS))+returnERR_PTR(-ELOOP);+/* Not currently safe. */+if(unlikely(nd->flags&LOOKUP_BENEATH))+returnERR_PTR(-EXDEV);+}if(IS_ERR_OR_NULL(res))returnres;}if(*res=='/'){if(!nd->root.mnt)set_root(nd);-if(unlikely(nd_jump_root(nd)))-returnERR_PTR(-ECHILD);+error=nd_jump_root(nd);+if(unlikely(error))+returnERR_PTR(error);while(unlikely(*++res=='/'));}
@@ -1353,8 +1374,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)structinode*inode=nd->inode;while(1){-if(path_equal(&nd->path,&nd->root))+if(path_equal(&nd->path,&nd->root)){+if(unlikely(nd->flags&LOOKUP_BENEATH))+return-EXDEV;break;+}if(nd->path.dentry!=nd->path.mnt->mnt_root){structdentry*old=nd->path.dentry;structdentry*parent=old->d_parent;
@@ -1379,6 +1403,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)return-ECHILD;if(&mparent->mnt==nd->path.mnt)break;+if(unlikely(nd->flags&LOOKUP_XDEV))+return-EXDEV;/* we know that mountpoint was pinned */nd->path.dentry=mountpoint;nd->path.mnt=&mparent->mnt;
@@ -1393,6 +1419,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)return-ECHILD;if(!mounted)break;+if(unlikely(nd->flags&LOOKUP_XDEV))+return-EXDEV;nd->path.mnt=&mounted->mnt;nd->path.dentry=mounted->mnt.mnt_root;inode=nd->path.dentry->d_inode;
@@ -1481,8 +1509,11 @@ static int path_parent_directory(struct path *path)staticintfollow_dotdot(structnameidata*nd){while(1){-if(path_equal(&nd->path,&nd->root))+if(path_equal(&nd->path,&nd->root)){+if(unlikely(nd->flags&LOOKUP_BENEATH))+return-EXDEV;break;+}if(nd->path.dentry!=nd->path.mnt->mnt_root){intret=path_parent_directory(&nd->path);if(ret)
@@ -1491,6 +1522,8 @@ static int follow_dotdot(struct nameidata *nd)}if(!follow_up(&nd->path))break;+if(unlikely(nd->flags&LOOKUP_XDEV))+return-EXDEV;}follow_mount(&nd->path);nd->inode=nd->path.dentry->d_inode;
@@ -1705,6 +1738,12 @@ static inline int may_lookup(struct nameidata *nd)staticinlineinthandle_dots(structnameidata*nd,inttype){if(type==LAST_DOTDOT){+/*+*AT_BENEATHresolving".."isnotcurrentlysafe--racescancause+*ourparenttohavemovedoutsideoftherootandustoskipoverit.+*/+if(unlikely(nd->flags&LOOKUP_BENEATH))+return-EXDEV;if(!nd->root.mnt)set_root(nd);if(nd->flags&LOOKUP_RCU){
@@ -2168,13 +2209,70 @@ static int link_path_walk(const char *name, struct nameidata *nd)}}+/*+*Configurend->pathbasedonthend->dfd.Thisisonlyusedaspartof+*path_init().+*/+staticinlineintdirfd_path_init(structnameidata*nd)+{+if(nd->dfd==AT_FDCWD){+if(nd->flags&LOOKUP_RCU){+structfs_struct*fs=current->fs;+unsignedseq;++do{+seq=read_seqcount_begin(&fs->seq);+nd->path=fs->pwd;+nd->inode=nd->path.dentry->d_inode;+nd->seq=__read_seqcount_begin(&nd->path.dentry->d_seq);+}while(read_seqcount_retry(&fs->seq,seq));+}else{+get_fs_pwd(current->fs,&nd->path);+nd->inode=nd->path.dentry->d_inode;+}+}else{+/* Caller must check execute permissions on the starting path component */+structfdf=fdget_raw(nd->dfd);+structdentry*dentry;++if(!f.file)+return-EBADF;++dentry=f.file->f_path.dentry;++if(*nd->name->name&&unlikely(!d_can_lookup(dentry))){+fdput(f);+return-ENOTDIR;+}++nd->path=f.file->f_path;+if(nd->flags&LOOKUP_RCU){+nd->inode=nd->path.dentry->d_inode;+nd->seq=read_seqcount_begin(&nd->path.dentry->d_seq);+}else{+path_get(&nd->path);+nd->inode=nd->path.dentry->d_inode;+}+fdput(f);+}+if(unlikely(nd->flags&LOOKUP_BENEATH)){+nd->root=nd->path;+if(!(nd->flags&LOOKUP_RCU))+path_get(&nd->root);+}+return0;+}+/* must be paired with terminate_walk() */staticconstchar*path_init(structnameidata*nd,unsignedflags){+interror;constchar*s=nd->name->name;if(!*s)flags&=~LOOKUP_RCU;+if(flags&LOOKUP_NO_SYMLINKS)+flags|=LOOKUP_NO_PROCLINKS;if(flags&LOOKUP_RCU)rcu_read_lock();
@@ -2203,53 +2301,25 @@ static const char *path_init(struct nameidata *nd, unsigned flags)nd->path.dentry=NULL;nd->m_seq=read_seqbegin(&mount_lock);+if(unlikely(flags&LOOKUP_XDEV)){+error=dirfd_path_init(nd);+if(unlikely(error))+returnERR_PTR(error);+}if(*s=='/'){-set_root(nd);-if(likely(!nd_jump_root(nd)))-returns;-returnERR_PTR(-ECHILD);-}elseif(nd->dfd==AT_FDCWD){-if(flags&LOOKUP_RCU){-structfs_struct*fs=current->fs;-unsignedseq;--do{-seq=read_seqcount_begin(&fs->seq);-nd->path=fs->pwd;-nd->inode=nd->path.dentry->d_inode;-nd->seq=__read_seqcount_begin(&nd->path.dentry->d_seq);-}while(read_seqcount_retry(&fs->seq,seq));-}else{-get_fs_pwd(current->fs,&nd->path);-nd->inode=nd->path.dentry->d_inode;-}-returns;-}else{-/* Caller must check execute permissions on the starting path component */-structfdf=fdget_raw(nd->dfd);-structdentry*dentry;--if(!f.file)-returnERR_PTR(-EBADF);--dentry=f.file->f_path.dentry;--if(*s&&unlikely(!d_can_lookup(dentry))){-fdput(f);-returnERR_PTR(-ENOTDIR);-}--nd->path=f.file->f_path;-if(flags&LOOKUP_RCU){-nd->inode=nd->path.dentry->d_inode;-nd->seq=read_seqcount_begin(&nd->path.dentry->d_seq);-}else{-path_get(&nd->path);-nd->inode=nd->path.dentry->d_inode;-}-fdput(f);+if(likely(!nd->root.mnt))+set_root(nd);+error=nd_jump_root(nd);+if(unlikely(error))+s=ERR_PTR(error);returns;}+if(likely(!nd->path.mnt)){+error=dirfd_path_init(nd);+if(unlikely(error))+returnERR_PTR(error);+}+returns;}staticconstchar*trailing_symlink(structnameidata*nd)
The primary motivation for the need for this flag is container runtimes
which have to interact with malicious root filesystems in the host
namespaces. One of the first requirements for a container runtime to be
secure against a malicious rootfs is that they correctly scope symlinks
(that is, they should be scoped as though they are chroot(2)ed into the
container's rootfs) and ".."-style paths[*]. The already-existing
AT_XDEV and AT_NO_PROCLINKS[**] help defend against other potential
attacks in a malicious rootfs scenario.
Currently most container runtimes try to do this resolution in
userspace[1], causing many potential race conditions. In addition, the
"obvious" alternative (actually performing a {ch,pivot_}root(2))
requires a fork+exec (for some runtimes) which is *very* costly if
necessary for every filesystem operation involving a container.
[*] At the moment, ".." and "proclink" jumping are disallowed for the
same reason it is disabled for AT_BENEATH -- currently it is not
safe to allow it. Future patches may enable it unconditionally once
we have resolved the possible races (for "..") and semantics (for
"proclink" jumping).
The most significant openat(2) semantic change with AT_THIS_ROOT is that
absolute pathnames no longer cause dirfd to be ignored completely. The
rationale is that AT_THIS_ROOT must necessarily chroot-scope symlinks
with absolute paths to dirfd, and so doing it for the base path seems to
be the most consistent behaviour (and also avoids foot-gunning users who
want to scope paths that are absolute).
Currently this is only enabled for openat(2) (which has its own flag
O_THISROOT with the same semantics). However the AT_* flags have been
reserved for future support in other *at(2) syscalls (because of
AT_EMPTY_PATH many *at(2) operations do not need to support these flags
directly).
[1]: https://github.com/cyphar/filepath-securejoin
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <redacted>
Cc: Christian Brauner <christian@brauner.io>
Signed-off-by: Aleksa Sarai <redacted>
---
fs/fcntl.c | 2 +-
fs/namei.c | 8 ++++----
fs/open.c | 2 ++
include/linux/fcntl.h | 2 +-
include/linux/namei.h | 1 +
include/uapi/asm-generic/fcntl.h | 3 +++
include/uapi/linux/fcntl.h | 2 ++
7 files changed, 14 insertions(+), 6 deletions(-)
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)*Exceptions:O_NONBLOCKisatwobitdefineonparisc;O_NDELAY*isdefinedasO_NONBLOCKonsomeplatformsandnotonothers.*/-BUILD_BUG_ON(25-1/* for O_RDONLY being 0 */!=+BUILD_BUG_ON(26-1/* for O_RDONLY being 0 */!=HWEIGHT32((VALID_OPEN_FLAGS&~(O_NONBLOCK|O_NDELAY))|__FMODE_EXEC|__FMODE_NONOTIFY));
This patch allows for AT_BENEATH and AT_THIS_ROOT to safely permit ".."
resolution (in the case of AT_BENEATH the resolution will still fail if
".." resolution would resolve a path outside of the root -- while
AT_THIS_ROOT will chroot(2)-style scope it). "proclink" jumps are still
disallowed entirely because now they could result in inconsistent
behaviour if resolution encounters a subsequent "..".
The need for this patch is explained by observing there is a fairly
easy-to-exploit race condition with chroot(2) (and thus by extension
AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a path can be used to
"skip over" nd->root and thus escape to the filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
(though somewhat more privileged) attack using MS_MOVE.
With this patch, such cases will be detected *during* ".." resolution
(which is the weak point of chroot(2) -- since walking *into* a
subdirectory tautologically cannot result in you walking *outside*
nd->root -- except through a bind-mount or "proclink"). By detecting
this at ".." resolution (rather than checking only at the end of the
entire resolution) we can both correct escapes by jumping back to the
root (in the case of AT_THIS_ROOT), as well as avoid revealing to
attackers the structure of the filesystem outside of the root (through
timing attacks for instance).
In order to avoid a quadratic lookup with each ".." entry, we only
activate the slow path if a write through &rename_lock or &mount_lock
have occurred during path resolution (&rename_lock and &mount_lock are
re-taken to further optimise the lookup). Since the primary attack being
protected against is MS_MOVE or rename(2), not doing additional checks
unless a mount or rename have occurred avoids making the common case
slow.
The use of __d_path here might seem suspect, but on further inspection
of the most important race (a path was *inside* the root but is now
*outside*), there appears to be no attack potential. If __d_path occurs
before the rename, then the path will be resolved but since the path was
originally inside the root there is no escape. Subsequent ".." jumps are
guaranteed to check __d_path reachable (by construction, &rename_lock or
&mount_lock must have been taken after __d_path returned), and thus will
not be able to escape from the previously-inside-root path. Walking down
is still safe since the entire subtree was moved (either by rename(2) or
MS_MOVE) and because (as discussed above) walking down is safe.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Aleksa Sarai <redacted>
---
fs/namei.c | 79 +++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 61 insertions(+), 18 deletions(-)
@@ -580,6 +582,22 @@ static inline int nd_alloc_stack(struct nameidata *nd)return__nd_alloc_stack(nd);}+staticinlineintnd_alloc_dpathbuf(structnameidata*nd)+{+if(unlikely(!nd->dpathbuf)){+if(nd->flags&LOOKUP_RCU){+nd->dpathbuf=kmalloc(PATH_MAX,GFP_ATOMIC);+if(unlikely(!nd->dpathbuf))+return-ECHILD;+}else{+nd->dpathbuf=kmalloc(PATH_MAX,GFP_KERNEL);+if(unlikely(!nd->dpathbuf))+return-ENOMEM;+}+}+return0;+}+staticvoiddrop_links(structnameidata*nd){inti=nd->depth;
@@ -1738,18 +1756,40 @@ static inline int may_lookup(struct nameidata *nd)staticinlineinthandle_dots(structnameidata*nd,inttype){if(type==LAST_DOTDOT){-/*-*AT_BENEATHresolving".."isnotcurrentlysafe--racescancause-*ourparenttohavemovedoutsideoftherootandustoskipoverit.-*/-if(unlikely(nd->flags&(LOOKUP_BENEATH|LOOKUP_CHROOT)))-return-EXDEV;+interror=0;+if(!nd->root.mnt)set_root(nd);-if(nd->flags&LOOKUP_RCU){-returnfollow_dotdot_rcu(nd);-}else-returnfollow_dotdot(nd);+if(nd->flags&LOOKUP_RCU)+error=follow_dotdot_rcu(nd);+else+error=follow_dotdot(nd);+if(error)+returnerror;++if(unlikely(nd->flags&(LOOKUP_BENEATH|LOOKUP_CHROOT))){+char*pathptr;+boolm_retry=read_seqretry(&mount_lock,nd->m_seq);+boolr_retry=read_seqretry(&rename_lock,nd->r_seq);++/* Racing rename(2) or MS_MOVE? */+if(likely(!m_retry&&!r_retry))+return0;+if(m_retry&&!(nd->flags&LOOKUP_RCU))+nd->m_seq=read_seqbegin(&mount_lock);+if(r_retry)+nd->r_seq=read_seqbegin(&rename_lock);++error=nd_alloc_dpathbuf(nd);+if(error)+returnerror;+pathptr=__d_path(&nd->path,&nd->root,nd->dpathbuf,PATH_MAX);+if(unlikely(!pathptr))+/* Breakout -- go back to root! */+returnnd_jump_root(nd);+if(unlikely(IS_ERR(pathptr)))+returnPTR_ERR(pathptr);+}}return0;}
@@ -2279,6 +2319,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)nd->last_type=LAST_ROOT;/* if there are only slashes... */nd->flags=flags|LOOKUP_JUMPED|LOOKUP_PARENT;nd->depth=0;++nd->m_seq=read_seqbegin(&mount_lock);+if(unlikely(flags&(LOOKUP_BENEATH|LOOKUP_CHROOT)))+nd->r_seq=read_seqbegin(&rename_lock);+if(flags&LOOKUP_ROOT){structdentry*root=nd->root.dentry;structinode*inode=root->d_inode;
On Tue, Oct 9, 2018 at 9:03 AM Aleksa Sarai [off-list ref] wrote:
This patch allows for AT_BENEATH and AT_THIS_ROOT to safely permit ".."
resolution (in the case of AT_BENEATH the resolution will still fail if
".." resolution would resolve a path outside of the root -- while
AT_THIS_ROOT will chroot(2)-style scope it). "proclink" jumps are still
disallowed entirely because now they could result in inconsistent
behaviour if resolution encounters a subsequent "..".
The need for this patch is explained by observing there is a fairly
easy-to-exploit race condition with chroot(2) (and thus by extension
AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a path can be used to
"skip over" nd->root and thus escape to the filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
(though somewhat more privileged) attack using MS_MOVE.
With this patch, such cases will be detected *during* ".." resolution
(which is the weak point of chroot(2) -- since walking *into* a
subdirectory tautologically cannot result in you walking *outside*
nd->root -- except through a bind-mount or "proclink"). By detecting
this at ".." resolution (rather than checking only at the end of the
entire resolution) we can both correct escapes by jumping back to the
root (in the case of AT_THIS_ROOT), as well as avoid revealing to
attackers the structure of the filesystem outside of the root (through
timing attacks for instance).
In order to avoid a quadratic lookup with each ".." entry, we only
activate the slow path if a write through &rename_lock or &mount_lock
have occurred during path resolution (&rename_lock and &mount_lock are
re-taken to further optimise the lookup). Since the primary attack being
protected against is MS_MOVE or rename(2), not doing additional checks
unless a mount or rename have occurred avoids making the common case
slow.
The use of __d_path here might seem suspect, but on further inspection
of the most important race (a path was *inside* the root but is now
*outside*), there appears to be no attack potential. If __d_path occurs
before the rename, then the path will be resolved but since the path was
originally inside the root there is no escape. Subsequent ".." jumps are
guaranteed to check __d_path reachable (by construction, &rename_lock or
&mount_lock must have been taken after __d_path returned),
"after"? Don't you mean "before"? Otherwise I don't understand what
you're saying here.
quoted hunk
and thus will
not be able to escape from the previously-inside-root path. Walking down
is still safe since the entire subtree was moved (either by rename(2) or
MS_MOVE) and because (as discussed above) walking down is safe.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Aleksa Sarai <redacted>
---
fs/namei.c | 79 +++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 61 insertions(+), 18 deletions(-)
Note that a fixed-size path buffer means that if the path is very
long, e.g. because you followed long symlinks on the way down, this
can cause lookup failures.
quoted hunk
static void drop_links(struct nameidata *nd)
{
int i = nd->depth;
@@ -1738,18 +1756,40 @@ static inline int may_lookup(struct nameidata *nd) static inline int handle_dots(struct nameidata *nd, int type) { if (type == LAST_DOTDOT) {- /*- * AT_BENEATH resolving ".." is not currently safe -- races can cause- * our parent to have moved outside of the root and us to skip over it.- */- if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)))- return -EXDEV;+ int error = 0;+ if (!nd->root.mnt) set_root(nd);- if (nd->flags & LOOKUP_RCU) {- return follow_dotdot_rcu(nd);- } else- return follow_dotdot(nd);+ if (nd->flags & LOOKUP_RCU)+ error = follow_dotdot_rcu(nd);+ else+ error = follow_dotdot(nd);+ if (error)+ return error;++ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {+ char *pathptr;+ bool m_retry = read_seqretry(&mount_lock, nd->m_seq);+ bool r_retry = read_seqretry(&rename_lock, nd->r_seq);++ /* Racing rename(2) or MS_MOVE? */+ if (likely(!m_retry && !r_retry))+ return 0;+ if (m_retry && !(nd->flags & LOOKUP_RCU))+ nd->m_seq = read_seqbegin(&mount_lock);+ if (r_retry)+ nd->r_seq = read_seqbegin(&rename_lock);++ error = nd_alloc_dpathbuf(nd);+ if (error)+ return error;+ pathptr = __d_path(&nd->path, &nd->root, nd->dpathbuf, PATH_MAX);+ if (unlikely(!pathptr))+ /* Breakout -- go back to root! */+ return nd_jump_root(nd);
I find the semantics of this check odd - especially in the
LOOKUP_BENEATH case, but also in the LOOKUP_CHROOT case. Wouldn't it
make more sense to just throw an error here? Making /.. go back to the
root is one thing, but making ".." from anything that has escaped from
the root go back to the root seems less logical to me.
Imagine, for example:
Thread A (a webserver or whatever) looks up
"example.org/images/foo/../bar.png" under "/var/www" with
LOOKUP_BENEATH.
Thread B concurrently moves "/var/www/example.org/images" to
"/var/backup/example.org/images".
Now thread A can accidentally resolve its path to "/var/www/bar.png"
if the race happens the wrong way?
On 2018-10-09, 'Jann Horn' via dev [off-list ref] wrote:
On Tue, Oct 9, 2018 at 9:03 AM Aleksa Sarai [off-list ref] wrote:
quoted
This patch allows for AT_BENEATH and AT_THIS_ROOT to safely permit ".."
resolution (in the case of AT_BENEATH the resolution will still fail if
".." resolution would resolve a path outside of the root -- while
AT_THIS_ROOT will chroot(2)-style scope it). "proclink" jumps are still
disallowed entirely because now they could result in inconsistent
behaviour if resolution encounters a subsequent "..".
The need for this patch is explained by observing there is a fairly
easy-to-exploit race condition with chroot(2) (and thus by extension
AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a path can be used to
"skip over" nd->root and thus escape to the filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
(though somewhat more privileged) attack using MS_MOVE.
With this patch, such cases will be detected *during* ".." resolution
(which is the weak point of chroot(2) -- since walking *into* a
subdirectory tautologically cannot result in you walking *outside*
nd->root -- except through a bind-mount or "proclink"). By detecting
this at ".." resolution (rather than checking only at the end of the
entire resolution) we can both correct escapes by jumping back to the
root (in the case of AT_THIS_ROOT), as well as avoid revealing to
attackers the structure of the filesystem outside of the root (through
timing attacks for instance).
In order to avoid a quadratic lookup with each ".." entry, we only
activate the slow path if a write through &rename_lock or &mount_lock
have occurred during path resolution (&rename_lock and &mount_lock are
re-taken to further optimise the lookup). Since the primary attack being
protected against is MS_MOVE or rename(2), not doing additional checks
unless a mount or rename have occurred avoids making the common case
slow.
The use of __d_path here might seem suspect, but on further inspection
of the most important race (a path was *inside* the root but is now
*outside*), there appears to be no attack potential. If __d_path occurs
before the rename, then the path will be resolved but since the path was
originally inside the root there is no escape. Subsequent ".." jumps are
guaranteed to check __d_path reachable (by construction, &rename_lock or
&mount_lock must have been taken after __d_path returned),
"after"? Don't you mean "before"? Otherwise I don't understand what
you're saying here.
I meant that the attacker doing the rename must've taken &rename_lock
or &mount_lock after __d_path returns in the target process (because the
race being examined is that the rename occurs *after* __d_path) and thus
are guaranteed to be detected).
Maybe there's a better way to phrase what I mean...
Note that a fixed-size path buffer means that if the path is very
long, e.g. because you followed long symlinks on the way down, this
can cause lookup failures.
This is already an issue with __d_path (even if the buffer was larger)
because it will not output a path longer than PATH_MAX. I imagine this
is a pretty strong argument for why we should refactor __d_path so that
we can *just* use the escape checking to avoid -ENAMETOOLONG.
I can work on this, but I'll wait until after LPC to see what the
discussion there brings up.
quoted
+ error = nd_alloc_dpathbuf(nd);
+ if (error)
+ return error;
+ pathptr = __d_path(&nd->path, &nd->root, nd->dpathbuf, PATH_MAX);
+ if (unlikely(!pathptr))
+ /* Breakout -- go back to root! */
+ return nd_jump_root(nd);
I find the semantics of this check odd - especially in the
LOOKUP_BENEATH case, but also in the LOOKUP_CHROOT case. Wouldn't it
make more sense to just throw an error here? Making /.. go back to the
root is one thing, but making ".." from anything that has escaped from
the root go back to the root seems less logical to me.
For AT_BENEATH, nd_jump_root() will always return -EXDEV -- but I'll
take your point for AT_THIS_ROOT below.
Thread A (a webserver or whatever) looks up
"example.org/images/foo/../bar.png" under "/var/www" with
LOOKUP_BENEATH.
Thread B concurrently moves "/var/www/example.org/images" to
"/var/backup/example.org/images".
Now thread A can accidentally resolve its path to "/var/www/bar.png"
if the race happens the wrong way?
This is a good point. When I changed this from always being -EXDEV in my
other postings, I was thinking about "/.." rather than the case you've
outlined where ".." is in the path but it's not actually meant to go to
the root.
I agree -EXDEV makes much more sense here. I will add this to my tree
(but I'll wait until after LPC before I send out a new series).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On Tue, Oct 9, 2018 at 5:36 PM Aleksa Sarai [off-list ref] wrote:
On 2018-10-09, 'Jann Horn' via dev [off-list ref] wrote:
quoted
On Tue, Oct 9, 2018 at 9:03 AM Aleksa Sarai [off-list ref] wrote:
quoted
This patch allows for AT_BENEATH and AT_THIS_ROOT to safely permit ".."
resolution (in the case of AT_BENEATH the resolution will still fail if
".." resolution would resolve a path outside of the root -- while
AT_THIS_ROOT will chroot(2)-style scope it). "proclink" jumps are still
disallowed entirely because now they could result in inconsistent
behaviour if resolution encounters a subsequent "..".
The need for this patch is explained by observing there is a fairly
easy-to-exploit race condition with chroot(2) (and thus by extension
AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a path can be used to
"skip over" nd->root and thus escape to the filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
(though somewhat more privileged) attack using MS_MOVE.
With this patch, such cases will be detected *during* ".." resolution
(which is the weak point of chroot(2) -- since walking *into* a
subdirectory tautologically cannot result in you walking *outside*
nd->root -- except through a bind-mount or "proclink"). By detecting
this at ".." resolution (rather than checking only at the end of the
entire resolution) we can both correct escapes by jumping back to the
root (in the case of AT_THIS_ROOT), as well as avoid revealing to
attackers the structure of the filesystem outside of the root (through
timing attacks for instance).
In order to avoid a quadratic lookup with each ".." entry, we only
activate the slow path if a write through &rename_lock or &mount_lock
have occurred during path resolution (&rename_lock and &mount_lock are
re-taken to further optimise the lookup). Since the primary attack being
protected against is MS_MOVE or rename(2), not doing additional checks
unless a mount or rename have occurred avoids making the common case
slow.
The use of __d_path here might seem suspect, but on further inspection
of the most important race (a path was *inside* the root but is now
*outside*), there appears to be no attack potential. If __d_path occurs
before the rename, then the path will be resolved but since the path was
originally inside the root there is no escape. Subsequent ".." jumps are
guaranteed to check __d_path reachable (by construction, &rename_lock or
&mount_lock must have been taken after __d_path returned),
"after"? Don't you mean "before"? Otherwise I don't understand what
you're saying here.
I meant that the attacker doing the rename must've taken &rename_lock
or &mount_lock after __d_path returns in the target process (because the
race being examined is that the rename occurs *after* __d_path) and thus
are guaranteed to be detected).
Maybe there's a better way to phrase what I mean...
Aah, I thought you were referring to what the victim process is doing,
not what the racing attacker is doing.
First of all, dirfd_path_init() part should be in a separate commit. And I'm
really not happy with the logics in there. dirfd_path_init() itself is
kinda-sorta reasonable.
Sure, I can do that.
It is equivalent to setting the starting point for
relative pathnames + setting ->root for LOOKUP_BENEATH, right?
Right.
But the part in path_init() is too bloody convoluted for its own good. Let me
try to translate:
quoted
+ if (unlikely(flags & LOOKUP_XDEV)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
* if LOOKUP_XDEV is set, set the starting point as if it was a relative
pathname. If LOOKUP_BENEATH was set as well, set ->root to the same
point.
Right. This is for two reasons (though if you disagree with these
semantics we can change this as well):
1. It's not clear to me whether openat(somefd->"/", "/tmp", O_XDEV)
should return an -EXDEV or completely ignore the starting point. Same
argument with AT_FDCWD. I opted to make it so that the starting point
has to be on the same mountpoint, but I totally understand if you
feel this is insane -- and I'd be happy to change it. The real
problem comes from (2).
2. AT_THIS_ROOT chroot-scope absolute paths, and so in the second patch
LOOKUP_CHROOT also triggers this codepath. The main argument for this
semantic is somewhat elaborated in the cover letter -- but
the short version is because AT_THIS_ROOT has to chroot-scope
absolute symlinks it would be somewhat strange if it didn't scope
absolute paths you give it -- otherwise it could either be a footgun
or would require always returning -EXDEV here.
Though, as above, if you feel that the current semantics (absolute
paths override whatever dirfd you give), then -EXDEV is the
alternative I would pitch.
* if it's an absolute pathname,
quoted
if (*s == '/') {
... and we hadn't come here with LOOKUP_XDEV + LOOKUP_BENEATH, set ->root.
quoted
+ if (likely(!nd->root.mnt))
+ set_root(nd);
* if it's an absolute pathname, set the starting point to ->root. Note that
if we came here with LOOKUP_XDEV, we'll discard the starting point we'd
calculated.
We wouldn't discard it -- nd_jump_root() will check whether a mount
crossing was implied here (otherwise an absolute symlink could cause you
to cross a mountpoint).
But as above, if you'd prefer that absolute paths disable all dirfd
handling (as is the case now), I can remove this semantic.
quoted
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ s = ERR_PTR(error);
return s;
}
+ if (likely(!nd->path.mnt)) {
* if we didn't have LOOKUP_XDEV, set the starting point as if it was a relative
pathname (which it is) and, if LOOKUP_BENEATH is also there, set ->root there
as well.
Pardon me, but... huh? The reason for your two calls of dirfd_path_init() is,
AFAICS, the combination of absolute pathname with both LOOKUP_XDEV and
LOOKUP_BENEATH at the same time. That combination is treated as if the pathname
had been relative. Note that LOOKUP_BENEATH alone is ignored for absolute ones
(and with a good reason - it's a no-op on path_init() level in that case).
What the hell? It complicates your code and doesn't seem to provide any benefits
whatsoever
The reasoning for this is because of how AT_THIS_ROOT uses both of these
codepaths (it causes dirfd_path_init() to be called before the absolute
check, and also causes ->root to be set).
I wrote the features in parallel and then split out the code for
AT_THIS_ROOT so it could be discussed separately (and so removing it if
it was rejected would be simpler). But unfortunately this does result in
the dirfd_path_init() code looking completely superfluous without seeing
the second patch.
-- you could bloody well have passed the relative pathname to start with.
(I think you mean always doing dirfd_path_init() first here?)
Right, but I didn't want to discard nd->path unnecessarily -- if we do
all of the code to grab AT_FDCWD and then it is completely unused (not
even in the AT_XDEV sense of "unused") it seems like a waste.
Did I misunderstand your suggestion? Were you referring to userspace
just being able to "[pass] the relative pathname to start with"?
IDGI... Without that kludge it becomes simply "do as we currently do for absolute
pathnames, call dirfd_path_init() for relative ones". And I would argue that
taking LOOKUP_BENEATH handling out of dirfd_path_init() into path_init() (relative)
case would be a good idea.
Right, I could definitely do that -- though for AT_THIS_ROOT we'd
duplicate the ->root setting in both places.
As it is, the logics is very hard to follow.
Sorry about that. Would you prefer if the two patches (AT_BENEATH family
and AT_THIS_ROOT) were sent as a single patch -- with the
dirfd_path_init() code split out? Or that the second patch do all of the
structural changes to refactor dirfd_path_init() usage?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2018-10-13 15:10:14
On Tue, Oct 09, 2018 at 06:02:28PM +1100, Aleksa Sarai wrote:
First of all, dirfd_path_init() part should be in a separate commit. And I'm
really not happy with the logics in there. dirfd_path_init() itself is
kinda-sorta reasonable. It is equivalent to setting the starting point for
relative pathnames + setting ->root for LOOKUP_BENEATH, right?
But the part in path_init() is too bloody convoluted for its own good. Let me
try to translate:
+ if (unlikely(flags & LOOKUP_XDEV)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
* if LOOKUP_XDEV is set, set the starting point as if it was a relative
pathname. If LOOKUP_BENEATH was set as well, set ->root to the same
point.
* if it's an absolute pathname,
if (*s == '/') {
... and we hadn't come here with LOOKUP_XDEV + LOOKUP_BENEATH, set ->root.
+ if (likely(!nd->root.mnt))
+ set_root(nd);
* if it's an absolute pathname, set the starting point to ->root. Note that
if we came here with LOOKUP_XDEV, we'll discard the starting point we'd
calculated.
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ s = ERR_PTR(error);
return s;
}
+ if (likely(!nd->path.mnt)) {
* if we didn't have LOOKUP_XDEV, set the starting point as if it was a relative
pathname (which it is) and, if LOOKUP_BENEATH is also there, set ->root there
as well.
Pardon me, but... huh? The reason for your two calls of dirfd_path_init() is,
AFAICS, the combination of absolute pathname with both LOOKUP_XDEV and
LOOKUP_BENEATH at the same time. That combination is treated as if the pathname
had been relative. Note that LOOKUP_BENEATH alone is ignored for absolute ones
(and with a good reason - it's a no-op on path_init() level in that case).
What the hell? It complicates your code and doesn't seem to provide any benefits
whatsoever -- you could bloody well have passed the relative pathname to start with.
IDGI... Without that kludge it becomes simply "do as we currently do for absolute
pathnames, call dirfd_path_init() for relative ones". And I would argue that
taking LOOKUP_BENEATH handling out of dirfd_path_init() into path_init() (relative)
case would be a good idea.
As it is, the logics is very hard to follow.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2018-10-13 15:41:39
On Sat, Oct 13, 2018 at 08:33:19AM +0100, Al Viro wrote:
Pardon me, but... huh? The reason for your two calls of dirfd_path_init() is,
AFAICS, the combination of absolute pathname with both LOOKUP_XDEV and
LOOKUP_BENEATH at the same time. That combination is treated as if the pathname
had been relative. Note that LOOKUP_BENEATH alone is ignored for absolute ones
(and with a good reason - it's a no-op on path_init() level in that case).
What the hell? It complicates your code and doesn't seem to provide any benefits
whatsoever -- you could bloody well have passed the relative pathname to start with.
IDGI... Without that kludge it becomes simply "do as we currently do for absolute
pathnames, call dirfd_path_init() for relative ones". And I would argue that
taking LOOKUP_BENEATH handling out of dirfd_path_init() into path_init() (relative)
case would be a good idea.
As it is, the logics is very hard to follow.
... and it fails on LOOKUP_BENEATH anyway. Egads... So that's for your
LOOKUP_CHROOT ;-/ IMO that's awful, especially with the way you've spread those
LOOKUP_CHROOT cases between these two.
Why not simply have O_THISROOT pick root by dirfd and call file_open_root()?
And if something wants it for stat(), etc. just have them use it combined with
O_PATH and pass the result to ...at()...
Pardon me, but... huh? The reason for your two calls of dirfd_path_init() is,
AFAICS, the combination of absolute pathname with both LOOKUP_XDEV and
LOOKUP_BENEATH at the same time. That combination is treated as if the pathname
had been relative. Note that LOOKUP_BENEATH alone is ignored for absolute ones
(and with a good reason - it's a no-op on path_init() level in that case).
What the hell? It complicates your code and doesn't seem to provide any benefits
whatsoever -- you could bloody well have passed the relative pathname to start with.
IDGI... Without that kludge it becomes simply "do as we currently do for absolute
pathnames, call dirfd_path_init() for relative ones". And I would argue that
taking LOOKUP_BENEATH handling out of dirfd_path_init() into path_init() (relative)
case would be a good idea.
As it is, the logics is very hard to follow.
... and it fails on LOOKUP_BENEATH anyway. Egads... So that's for your
LOOKUP_CHROOT ;-/ IMO that's awful, especially with the way you've spread those
LOOKUP_CHROOT cases between these two.
Yeah, the ->root setting in dirfd_path_init() is ugly. :/
Why not simply have O_THISROOT pick root by dirfd and call file_open_root()?
Wouldn't this require replicating the dirfd_path_init()-like code inside
all of the path_*at() callers which use path_init()? Or is there another
common place we could put it?
And if something wants it for stat(), etc. just have them use it combined with
O_PATH and pass the result to ...at()...
This works for stat and quite a few other things (which is why I only
added openat(2) support for the moment), but I think we'd eventually
need something like this for renameat2(2) as well as a few other choice
*at(2) syscalls. Though I also think that more AT_EMPTY_PATH support
would removed the need for _most_ *at(2) implementations to use this.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
Note that a fixed-size path buffer means that if the path is very
long, e.g. because you followed long symlinks on the way down, this
can cause lookup failures.
This is already an issue with __d_path (even if the buffer was larger)
because it will not output a path longer than PATH_MAX. I imagine this
is a pretty strong argument for why we should refactor __d_path so that
we can *just* use the escape checking to avoid -ENAMETOOLONG.
Let me get it straight - the whole point of that buffer is to check
if __d_path() returns NULL? So you allocate it so that you would have
place to copy the path components into... only to have them completely
ignored?
How is that different from path_is_under()?
Note that a fixed-size path buffer means that if the path is very
long, e.g. because you followed long symlinks on the way down, this
can cause lookup failures.
This is already an issue with __d_path (even if the buffer was larger)
because it will not output a path longer than PATH_MAX. I imagine this
is a pretty strong argument for why we should refactor __d_path so that
we can *just* use the escape checking to avoid -ENAMETOOLONG.
Let me get it straight - the whole point of that buffer is to check
if __d_path() returns NULL? So you allocate it so that you would have
place to copy the path components into... only to have them completely
ignored?
Yes (and it was definitely the wrong thing to do).
Since writing that mail, I changed it to not have to allocate a buffer
-- though this is done in the fairly ugly way of changing prepend_path()
to be able to take @buffer==NULL which then skips all of the
string-related code, and then having a dumb wrapper which calls
prepend_path(root, path, NULL, NULL).
I was planning on sending out the updated patches after LPC.
How is that different from path_is_under()?
I didn't know about path_is_under() -- I just checked and it appears to
not take &rename_lock? From my understanding, in order to protect
against the rename attack you need to take &rename_lock (or check
against &rename_lock at least and retry if it changed).
I could definitely use path_is_under() if you prefer, though I think
that in this case we'd need to take &rename_lock (right?). Also is there
a speed issue with taking the write-side of a seqlock when we are just
reading -- is this more efficient than doing a retry like in __d_path?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2018-10-13 16:41:09
On Sat, Oct 13, 2018 at 07:53:26PM +1100, Aleksa Sarai wrote:
I didn't know about path_is_under() -- I just checked and it appears to
not take &rename_lock? From my understanding, in order to protect
against the rename attack you need to take &rename_lock (or check
against &rename_lock at least and retry if it changed).
I could definitely use path_is_under() if you prefer, though I think
that in this case we'd need to take &rename_lock (right?). Also is there
a speed issue with taking the write-side of a seqlock when we are just
reading -- is this more efficient than doing a retry like in __d_path?
???
1) it uses is_subdir(), which does deal with rename_lock
2) what it does is taking mount_lock.lock. I.e. the same
thing as the second retry in __d_path(). _If_ it shows
up in profiles, we can switch it to read_seqbegin_or_lock(),
but I'd like to see the profiling data first.
On Sat, Oct 13, 2018 at 07:53:26PM +1100, Aleksa Sarai wrote:
quoted
I didn't know about path_is_under() -- I just checked and it appears to
not take &rename_lock? From my understanding, in order to protect
against the rename attack you need to take &rename_lock (or check
against &rename_lock at least and retry if it changed).
I could definitely use path_is_under() if you prefer, though I think
that in this case we'd need to take &rename_lock (right?). Also is there
a speed issue with taking the write-side of a seqlock when we are just
reading -- is this more efficient than doing a retry like in __d_path?
???
1) it uses is_subdir(), which does deal with rename_lock
Oh -- complete brain-fart on my part. Sorry about that.
2) what it does is taking mount_lock.lock. I.e. the same
thing as the second retry in __d_path(). _If_ it shows
up in profiles, we can switch it to read_seqbegin_or_lock(),
but I'd like to see the profiling data first.
Sure, I'll switch it to use path_is_under().
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
Capsicum project[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.
I've been thinking about this problem a little more (from the UX side of
things) and I have a feeling that adding 5 different O_* flags related
to resolution -- rather than properties related to opening the file --
might be less than ideal (even though, as discussed in previous threads,
there is a need for these flags and for them to be separated).
There is *some* precedence for this with O_PATH[**] changing fairly
large semantics of openat(2) but there are some things about O_PATH
which I think could be improved.
What if we had a resolveat(2) which acted like openat(..., O_PATH) *but*
it allowed us to have new flags and to separate the scoping flags from
the (fairly limited) space of O_* flags. Then O_PATH could effectively
just be a legacy way of doing resolveat(2) -- with only O_CLOEXEC,
O_DIRECTORY, and O_NOFOLLOW support.
And the main things we could add would be:
* These resolution flags, with only support available from
resolveat(2) for the moment. The idea would be that AT_EMPTY_PATH
would be the recommended way to make use of this.
* Support for RESOLVE_{NOPERM,RDONLY,WRONLY,RDWR} (which after some
discussions with Eric last year might be necessary in order to make
/proc/$pid/fd/$fd re-opening of O_PATH descriptors safer -- which is
something that we use in both runc and LXC).
Is this idea palatable, or was this something considered during the
development of O_PATH and someone had an argument why augmenting O_PATH
is better than a new syscall?
[**] And while writing this paragraph I noticed that I didn't update the
O_PATH "flag whitelist" to allow the scoping flags to affect it. I
will include a fix for this in v4 (I must've lost it in an early
rebase before I sent v1).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>