| Message ID | 20191105090553.6350-6-cyphar@cyphar.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | open: introduce openat2(2) syscall | expand |
On Tue, Nov 05, 2019 at 08:05:49PM +1100, Aleksa Sarai wrote: > @@ -2277,12 +2277,20 @@ static const char *path_init(struct nameidata *nd, unsigned flags) > > nd->m_seq = read_seqbegin(&mount_lock); > > - /* Figure out the starting path and root (if needed). */ > - if (*s == '/') { > + /* Absolute pathname -- fetch the root. */ > + if (flags & LOOKUP_IN_ROOT) { > + /* With LOOKUP_IN_ROOT, act as a relative path. */ > + while (*s == '/') > + s++; Er... Why bother skipping slashes? I mean, not only link_path_walk() will skip them just fine, you are actually risking breakage in this: if (*s && unlikely(!d_can_lookup(dentry))) { fdput(f); return ERR_PTR(-ENOTDIR); } which is downstream from there with you patch, AFAICS.
On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Tue, Nov 05, 2019 at 08:05:49PM +1100, Aleksa Sarai wrote: > > > @@ -2277,12 +2277,20 @@ static const char *path_init(struct nameidata *nd, unsigned flags) > > > > nd->m_seq = read_seqbegin(&mount_lock); > > > > - /* Figure out the starting path and root (if needed). */ > > - if (*s == '/') { > > + /* Absolute pathname -- fetch the root. */ > > + if (flags & LOOKUP_IN_ROOT) { > > + /* With LOOKUP_IN_ROOT, act as a relative path. */ > > + while (*s == '/') > > + s++; > > Er... Why bother skipping slashes? I mean, not only link_path_walk() > will skip them just fine, you are actually risking breakage in this: > if (*s && unlikely(!d_can_lookup(dentry))) { > fdput(f); > return ERR_PTR(-ENOTDIR); > } > which is downstream from there with you patch, AFAICS. I switched to stripping the slashes at your suggestion a few revisions ago[1], and had (wrongly) assumed we needed to handle "/" somehow in path_init(). But you're quite right about link_path_walk() -- and I'd be more than happy to drop it. [1]: https://lore.kernel.org/lkml/20190712125552.GL17978@ZenIV.linux.org.uk/
On Wed, Nov 13, 2019 at 01:44:14PM +1100, Aleksa Sarai wrote: > On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Nov 05, 2019 at 08:05:49PM +1100, Aleksa Sarai wrote: > > > > > @@ -2277,12 +2277,20 @@ static const char *path_init(struct nameidata *nd, unsigned flags) > > > > > > nd->m_seq = read_seqbegin(&mount_lock); > > > > > > - /* Figure out the starting path and root (if needed). */ > > > - if (*s == '/') { > > > + /* Absolute pathname -- fetch the root. */ > > > + if (flags & LOOKUP_IN_ROOT) { > > > + /* With LOOKUP_IN_ROOT, act as a relative path. */ > > > + while (*s == '/') > > > + s++; > > > > Er... Why bother skipping slashes? I mean, not only link_path_walk() > > will skip them just fine, you are actually risking breakage in this: > > if (*s && unlikely(!d_can_lookup(dentry))) { > > fdput(f); > > return ERR_PTR(-ENOTDIR); > > } > > which is downstream from there with you patch, AFAICS. > > I switched to stripping the slashes at your suggestion a few revisions > ago[1], and had (wrongly) assumed we needed to handle "/" somehow in > path_init(). But you're quite right about link_path_walk() -- and I'd be > more than happy to drop it. That, IIRC, was about untangling the weirdness around multiple calls of dirfd_path_init() and basically went "we might want just strip the slashes in case of that flag very early in the entire thing, so that later the normal logics for absolute/relative would DTRT". Since your check is right next to checking for absolute pathnames (and not in the very beginning of path_init()), we might as well turn the check for absolute pathname into *s == '/' && !(flags & LOOKUP_IN_ROOT) and be done with that.
On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Wed, Nov 13, 2019 at 01:44:14PM +1100, Aleksa Sarai wrote: > > On 2019-11-13, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > On Tue, Nov 05, 2019 at 08:05:49PM +1100, Aleksa Sarai wrote: > > > > > > > @@ -2277,12 +2277,20 @@ static const char *path_init(struct nameidata *nd, unsigned flags) > > > > > > > > nd->m_seq = read_seqbegin(&mount_lock); > > > > > > > > - /* Figure out the starting path and root (if needed). */ > > > > - if (*s == '/') { > > > > + /* Absolute pathname -- fetch the root. */ > > > > + if (flags & LOOKUP_IN_ROOT) { > > > > + /* With LOOKUP_IN_ROOT, act as a relative path. */ > > > > + while (*s == '/') > > > > + s++; > > > > > > Er... Why bother skipping slashes? I mean, not only link_path_walk() > > > will skip them just fine, you are actually risking breakage in this: > > > if (*s && unlikely(!d_can_lookup(dentry))) { > > > fdput(f); > > > return ERR_PTR(-ENOTDIR); > > > } > > > which is downstream from there with you patch, AFAICS. > > > > I switched to stripping the slashes at your suggestion a few revisions > > ago[1], and had (wrongly) assumed we needed to handle "/" somehow in > > path_init(). But you're quite right about link_path_walk() -- and I'd be > > more than happy to drop it. > > That, IIRC, was about untangling the weirdness around multiple calls of > dirfd_path_init() and basically went "we might want just strip the slashes > in case of that flag very early in the entire thing, so that later the > normal logics for absolute/relative would DTRT". Ah okay, I'd misunderstood the point you were making in that thread. > Since your check is right next to checking for absolute pathnames (and > not in the very beginning of path_init()), we might as well turn the > check for absolute pathname into *s == '/' && !(flags & > LOOKUP_IN_ROOT) and be done with that. Yup, agreed.
diff --git a/fs/namei.c b/fs/namei.c index 54fdbdfbeb94..a3d199a60708 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2277,12 +2277,20 @@ static const char *path_init(struct nameidata *nd, unsigned flags) nd->m_seq = read_seqbegin(&mount_lock); - /* Figure out the starting path and root (if needed). */ - if (*s == '/') { + /* Absolute pathname -- fetch the root. */ + if (flags & LOOKUP_IN_ROOT) { + /* With LOOKUP_IN_ROOT, act as a relative path. */ + while (*s == '/') + s++; + } else if (*s == '/') { error = nd_jump_root(nd); if (unlikely(error)) return ERR_PTR(error); - } else if (nd->dfd == AT_FDCWD) { + return s; + } + + /* Relative pathname -- get the starting-point it is relative to. */ + if (nd->dfd == AT_FDCWD) { if (flags & LOOKUP_RCU) { struct fs_struct *fs = current->fs; unsigned seq; @@ -2322,6 +2330,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags) } fdput(f); } + /* For scoped-lookups we need to set the root to the dirfd as well. */ if (flags & LOOKUP_IS_SCOPED) { nd->root = nd->path; diff --git a/include/linux/namei.h b/include/linux/namei.h index 12f4f36835c2..96b374e08230 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -46,8 +46,9 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND}; #define LOOKUP_NO_MAGICLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */ #define LOOKUP_NO_XDEV 0x080000 /* No mountpoint crossing. */ #define LOOKUP_BENEATH 0x100000 /* No escaping from starting point. */ +#define LOOKUP_IN_ROOT 0x200000 /* Treat dirfd as %current->fs->root. */ /* LOOKUP_* flags which do scope-related checks based on the dirfd. */ -#define LOOKUP_IS_SCOPED LOOKUP_BENEATH +#define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT) extern int path_pts(struct path *path);
/* Background. */ Container runtimes or other administrative management processes will often interact with root filesystems while in the host mount namespace, because the cost of doing a chroot(2) on every operation is too prohibitive (especially in Go, which cannot safely use vfork). However, a malicious program can trick the management process into doing operations on files outside of the root filesystem through careful crafting of symlinks. Most programs that need this feature have attempted to make this process safe, by doing all of the path resolution in userspace (with symlinks being scoped to the root of the malicious root filesystem). Unfortunately, this method is prone to foot-guns and usually such implementations have subtle security bugs. Thus, what userspace needs is a way to resolve a path as though it were in a chroot(2) -- with all absolute symlinks being resolved relative to the dirfd root (and ".." components being stuck under the dirfd root). It is much simpler and more straight-forward to provide this functionality in-kernel (because it can be done far more cheaply and correctly). More classical applications that also have this problem (which have their own potentially buggy userspace path sanitisation code) include web servers, archive extraction tools, network file servers, and so on. /* Userspace API. */ LOOKUP_IN_ROOT will be exposed to userspace through openat2(2). /* Semantics. */ Unlike most other LOOKUP flags (most notably LOOKUP_FOLLOW), LOOKUP_IN_ROOT applies to all components of the path. With LOOKUP_IN_ROOT, any path component which attempts to cross the starting point of the pathname lookup (the dirfd passed to openat) will remain at the starting point. Thus, all absolute paths and symlinks will be scoped within the starting point. There is a slight change in behaviour regarding pathnames -- if the pathname is absolute then the dirfd is still used as the root of resolution of LOOKUP_IN_ROOT is specified (this is to avoid obvious foot-guns, at the cost of a minor API inconsistency). As with LOOKUP_BENEATH, Jann's security concern about ".."[1] applies to LOOKUP_IN_ROOT -- therefore ".." resolution is blocked. This restriction will be lifted in a future patch, but requires more work to ensure that permitting ".." is done safely. Magic-link jumps are also blocked, because they can beam the path lookup across the starting point. It would be possible to detect and block only the "bad" crossings with path_is_under() checks, but it's unclear whether it makes sense to permit magic-links at all. However, userspace is recommended to pass LOOKUP_NO_MAGICLINKS if they want to ensure that magic-link crossing is entirely disabled. /* Testing. */ LOOKUP_IN_ROOT is tested as part of the openat2(2) selftests. [1]: https://lore.kernel.org/lkml/CAG48ez1jzNvxB+bfOBnERFGp=oMM0vHWuLD6EULmne3R6xa53w@mail.gmail.com/ Cc: Christian Brauner <christian.brauner@ubuntu.com> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> --- fs/namei.c | 15 ++++++++++++--- include/linux/namei.h | 3 ++- 2 files changed, 14 insertions(+), 4 deletions(-)