* [PATCH v3 1/3] namei: implement O_BENEATH-style AT_* flags
From: Aleksa Sarai @ 2018-10-09 7:02 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Christian Brauner, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, Andy Lutomirski, David Howells, Jann Horn,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
linux-kernel, linux-arch, linux-api
In-Reply-To: <20181009070230.12884-1-cyphar@cyphar.com>
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 <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Suggested-by: David Drysdale <drysdale@google.com>
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 <cyphar@cyphar.com>
---
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(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 4137d96534a6..e343618736f7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- 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));
diff --git a/fs/namei.c b/fs/namei.c
index fb913148d4d1..76eacd3af89b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -845,6 +845,12 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
+ if (unlikely(nd->flags & LOOKUP_XDEV)) {
+ if (nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -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))
+ return ERR_PTR(-ELOOP);
+ /* Not currently safe. */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return ERR_PTR(-EXDEV);
+ }
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 == '/'))
;
}
@@ -1271,12 +1286,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_XDEV))
+ ret = -EXDEV;
+ 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;
@@ -1333,6 +1352,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_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1353,8 +1374,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
struct inode *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) {
struct dentry *old = nd->path.dentry;
struct dentry *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)
static int follow_dotdot(struct nameidata *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) {
int ret = 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)
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))
+ return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
if (nd->flags & LOOKUP_RCU) {
@@ -1720,6 +1759,8 @@ static int pick_link(struct nameidata *nd, struct path *link,
{
int error;
struct saved *last;
+ if (unlikely(nd->flags & LOOKUP_NO_SYMLINKS))
+ return -ELOOP;
if (unlikely(nd->total_link_count++ >= MAXSYMLINKS)) {
path_to_nameidata(link, nd);
return -ELOOP;
@@ -2168,13 +2209,70 @@ static int link_path_walk(const char *name, struct nameidata *nd)
}
}
+/*
+ * Configure nd->path based on the nd->dfd. This is only used as part of
+ * path_init().
+ */
+static inline int dirfd_path_init(struct nameidata *nd)
+{
+ if (nd->dfd == AT_FDCWD) {
+ if (nd->flags & LOOKUP_RCU) {
+ struct fs_struct *fs = current->fs;
+ unsigned seq;
+
+ 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 */
+ struct fd f = fdget_raw(nd->dfd);
+ struct dentry *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);
+ }
+ return 0;
+}
+
/* must be paired with terminate_walk() */
static const char *path_init(struct nameidata *nd, unsigned flags)
{
+ int error;
const char *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))
+ return ERR_PTR(error);
+ }
if (*s == '/') {
- set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
- } else if (nd->dfd == AT_FDCWD) {
- if (flags & LOOKUP_RCU) {
- struct fs_struct *fs = current->fs;
- unsigned seq;
-
- 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;
- }
- return s;
- } else {
- /* Caller must check execute permissions on the starting path component */
- struct fd f = fdget_raw(nd->dfd);
- struct dentry *dentry;
-
- if (!f.file)
- return ERR_PTR(-EBADF);
-
- dentry = f.file->f_path.dentry;
-
- if (*s && unlikely(!d_can_lookup(dentry))) {
- fdput(f);
- return ERR_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);
return s;
}
+ if (likely(!nd->path.mnt)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
+ return s;
}
static const char *trailing_symlink(struct nameidata *nd)
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..80f5f566a5ff 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -988,6 +988,14 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_DIRECTORY;
if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
+ if (flags & O_BENEATH)
+ lookup_flags |= LOOKUP_BENEATH;
+ if (flags & O_XDEV)
+ lookup_flags |= LOOKUP_XDEV;
+ if (flags & O_NOPROCLINKS)
+ lookup_flags |= LOOKUP_NO_PROCLINKS;
+ if (flags & O_NOSYMLINKS)
+ lookup_flags |= LOOKUP_NO_SYMLINKS;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/fs/stat.c b/fs/stat.c
index f8e6fb2c3657..d319a468c704 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -170,8 +170,8 @@ int vfs_statx(int dfd, const char __user *filename, int flags,
int error = -EINVAL;
unsigned int lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT;
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
- AT_EMPTY_PATH | KSTAT_QUERY_FLAGS)) != 0)
+ if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
+ KSTAT_QUERY_FLAGS))
return -EINVAL;
if (flags & AT_SYMLINK_NOFOLLOW)
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..ad5bba4b5b12 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,8 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
+ O_NOPROCLINKS | O_NOSYMLINKS)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..5ff7f3362d1b 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -47,6 +47,13 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_EMPTY 0x4000
#define LOOKUP_DOWN 0x8000
+/* Scoping flags for lookup. */
+#define LOOKUP_BENEATH 0x010000 /* No escaping from starting point. */
+#define LOOKUP_XDEV 0x020000 /* No mountpoint crossing. */
+#define LOOKUP_NO_PROCLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
+#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
+ Implies LOOKUP_NO_PROCLINKS. */
+
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/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..c2bf5983e46a 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,23 @@
#define O_NDELAY O_NONBLOCK
#endif
+/*
+ * These are identical to their AT_* counterparts (which affect the entireity
+ * of path resolution).
+ */
+#ifndef O_BENEATH
+#define O_BENEATH 00040000000 /* *Not* the same as capsicum's O_BENEATH! */
+#endif
+#ifndef O_XDEV
+#define O_XDEV 00100000000
+#endif
+#ifndef O_NOPROCLINKS
+#define O_NOPROCLINKS 00200000000
+#endif
+#ifndef O_NOSYMLINKS
+#define O_NOSYMLINKS 01000000000
+#endif
+
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
#define F_SETFD 2 /* set/clear close_on_exec */
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 594b85f7cb86..551a9e2166a8 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -92,5 +92,13 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
+/* Flags which affect path *resolution*, not just last-component handling. */
+#define AT_BENEATH 0x10000 /* No absolute paths or ".." escaping
+ (in-path or through symlinks) */
+#define AT_XDEV 0x20000 /* No mountpoint crossing. */
+#define AT_NO_PROCLINKS 0x40000 /* No /proc/$pid/fd/... "symlinks". */
+#define AT_NO_SYMLINKS 0x80000 /* No symlinks *at all*.
+ Implies AT_NO_PROCLINKS. */
+
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.19.0
^ permalink raw reply related
* [PATCH v3 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Aleksa Sarai @ 2018-10-09 7:02 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Christian Brauner, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, Andy Lutomirski, David Howells, Jann Horn,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
linux-kernel, linux-arch, linux-api
In-Reply-To: <20181009070230.12884-1-cyphar@cyphar.com>
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 <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
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(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index e343618736f7..4c36c5b9fdb9 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- 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));
diff --git a/fs/namei.c b/fs/namei.c
index 76eacd3af89b..b31aef27df22 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1094,7 +1094,7 @@ const char *get_link(struct nameidata *nd)
if (unlikely(nd->flags & LOOKUP_NO_PROCLINKS))
return ERR_PTR(-ELOOP);
/* Not currently safe. */
- if (unlikely(nd->flags & LOOKUP_BENEATH))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)))
return ERR_PTR(-EXDEV);
}
if (IS_ERR_OR_NULL(res))
@@ -1742,7 +1742,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
* 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))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT)))
return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
@@ -2255,7 +2255,7 @@ static inline int dirfd_path_init(struct nameidata *nd)
}
fdput(f);
}
- if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
nd->root = nd->path;
if (!(nd->flags & LOOKUP_RCU))
path_get(&nd->root);
@@ -2301,7 +2301,7 @@ 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)) {
+ if (unlikely(flags & (LOOKUP_CHROOT | LOOKUP_XDEV))) {
error = dirfd_path_init(nd);
if (unlikely(error))
return ERR_PTR(error);
diff --git a/fs/open.c b/fs/open.c
index 80f5f566a5ff..81d148f626cd 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -996,6 +996,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_NO_PROCLINKS;
if (flags & O_NOSYMLINKS)
lookup_flags |= LOOKUP_NO_SYMLINKS;
+ if (flags & O_THISROOT)
+ lookup_flags |= LOOKUP_CHROOT;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index ad5bba4b5b12..95480cd4c09d 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
- O_NOPROCLINKS | O_NOSYMLINKS)
+ O_NOPROCLINKS | O_NOSYMLINKS | O_THISROOT)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 5ff7f3362d1b..7ec9e2d84649 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -53,6 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_NO_PROCLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
Implies LOOKUP_NO_PROCLINKS. */
+#define LOOKUP_CHROOT 0x100000 /* Treat dirfd as %current->fs->root. */
extern int path_pts(struct path *path);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index c2bf5983e46a..11206b0e927c 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -113,6 +113,9 @@
#ifndef O_NOSYMLINKS
#define O_NOSYMLINKS 01000000000
#endif
+#ifndef O_THISROOT
+#define O_THISROOT 02000000000
+#endif
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 551a9e2166a8..ea978457b68f 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -99,6 +99,8 @@
#define AT_NO_PROCLINKS 0x40000 /* No /proc/$pid/fd/... "symlinks". */
#define AT_NO_SYMLINKS 0x80000 /* No symlinks *at all*.
Implies AT_NO_PROCLINKS. */
+#define AT_THIS_ROOT 0x100000 /* Path resolution acts as though
+ it is chroot-ed into dirfd. */
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.19.0
^ permalink raw reply related
* [PATCH v3 3/3] namei: aggressively check for nd->root escape on ".." resolution
From: Aleksa Sarai @ 2018-10-09 7:02 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Aleksa Sarai, Jann Horn, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, Andy Lutomirski, David Howells, Christian Brauner,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
linux-kernel, linux-arch, linux-api
In-Reply-To: <20181009070230.12884-1-cyphar@cyphar.com>
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 <cyphar@cyphar.com>
---
fs/namei.c | 79 +++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 61 insertions(+), 18 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index b31aef27df22..12cd8d8987ea 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -493,7 +493,7 @@ struct nameidata {
struct path root;
struct inode *inode; /* path.dentry.d_inode */
unsigned int flags;
- unsigned seq, m_seq;
+ unsigned seq, m_seq, r_seq;
int last_type;
unsigned depth;
int total_link_count;
@@ -508,6 +508,7 @@ struct nameidata {
struct inode *link_inode;
unsigned root_seq;
int dfd;
+ char *dpathbuf;
} __randomize_layout;
static void set_nameidata(struct nameidata *p, int dfd, struct filename *name)
@@ -530,6 +531,7 @@ static void restore_nameidata(void)
old->total_link_count = now->total_link_count;
if (now->stack != now->internal)
kfree(now->stack);
+ kfree(now->dpathbuf);
}
static int __nd_alloc_stack(struct nameidata *nd)
@@ -580,6 +582,22 @@ static inline int nd_alloc_stack(struct nameidata *nd)
return __nd_alloc_stack(nd);
}
+static inline int nd_alloc_dpathbuf(struct nameidata *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;
+ }
+ }
+ return 0;
+}
+
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);
+ if (unlikely(IS_ERR(pathptr)))
+ return PTR_ERR(pathptr);
+ }
}
return 0;
}
@@ -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) {
struct dentry *root = nd->root.dentry;
struct inode *inode = root->d_inode;
@@ -2289,7 +2334,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
if (flags & LOOKUP_RCU) {
nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
nd->root_seq = nd->seq;
- nd->m_seq = read_seqbegin(&mount_lock);
} else {
path_get(&nd->path);
}
@@ -2300,7 +2344,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.mnt = NULL;
nd->path.dentry = NULL;
- nd->m_seq = read_seqbegin(&mount_lock);
if (unlikely(flags & (LOOKUP_CHROOT | LOOKUP_XDEV))) {
error = dirfd_path_init(nd);
if (unlikely(error))
@@ -2407,7 +2450,7 @@ int filename_lookup(int dfd, struct filename *name, unsigned flags,
struct path *path, struct path *root)
{
int retval;
- struct nameidata nd;
+ struct nameidata nd = {};
if (IS_ERR(name))
return PTR_ERR(name);
if (unlikely(root)) {
@@ -2450,7 +2493,7 @@ static struct filename *filename_parentat(int dfd, struct filename *name,
struct qstr *last, int *type)
{
int retval;
- struct nameidata nd;
+ struct nameidata nd = {};
if (IS_ERR(name))
return name;
@@ -2779,7 +2822,7 @@ static int
filename_mountpoint(int dfd, struct filename *name, struct path *path,
unsigned int flags)
{
- struct nameidata nd;
+ struct nameidata nd = {};
int error;
if (IS_ERR(name))
return PTR_ERR(name);
@@ -3626,7 +3669,7 @@ static struct file *path_openat(struct nameidata *nd,
struct file *do_filp_open(int dfd, struct filename *pathname,
const struct open_flags *op)
{
- struct nameidata nd;
+ struct nameidata nd = {};
int flags = op->lookup_flags;
struct file *filp;
@@ -3643,7 +3686,7 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
const char *name, const struct open_flags *op)
{
- struct nameidata nd;
+ struct nameidata nd = {};
struct file *file;
struct filename *filename;
int flags = op->lookup_flags | LOOKUP_ROOT;
--
2.19.0
^ permalink raw reply related
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-09 8:33 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810041130380.12951@chino.kir.corp.google.com>
On Thu 04-10-18 11:34:11, David Rientjes wrote:
> On Thu, 4 Oct 2018, Michal Hocko wrote:
>
> > > And prior to the offending commit, there were three ways to control thp
> > > but two ways to determine if a mapping was eligible for thp based on the
> > > implementation detail of one of those ways.
> >
> > Yes, it is really unfortunate that we have ever allowed to leak such an
> > internal stuff like VMA flags to userspace.
> >
>
> Right, I don't like userspace dependencies on VmFlags in smaps myself, but
> it's the only way we have available that shows whether a single mapping is
> eligible to be backed by thp :/
Which is not the case due to reasons mentioned earlier. It only speaks
about madvise status on the VMA.
> > > If there are three ways to
> > > control thp, userspace is still in the dark wrt which takes precedence
> > > over the other: we have PR_SET_THP_DISABLE but globally sysfs has it set
> > > to "always", or we have MADV_HUGEPAGE set per smaps but PR_SET_THP_DISABLE
> > > shown in /proc/pid/status, etc.
> > >
> > > Which one is the ultimate authority?
> >
> > Isn't our documentation good enough? If not then we should document it
> > properly.
> >
>
> No, because the offending commit actually changed the precedence itself:
> PR_SET_THP_DISABLE used to be honored for future mappings and the commit
> changed that for all current mappings.
Which is the actual and the full point of the fix as described in the
changelog. The original implementation was poor and inconsistent.
> So as a result of the commit
> itself we would have had to change the documentation and userspace can't
> be expected to keep up with yet a fourth variable: kernel version. It
> really needs to be simpler, just a per-mapping specifier.
As I've said, if you really need a per-vma granularity then make it a
dedicated line in the output with a clear semantic. Do not make VMA
flags even more confusing.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [RFC v5 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-10-09 10:37 UTC (permalink / raw)
To: linux-kernel
Cc: Eric Biederman, Dmitry Safonov, linux-api, James Bottomley,
Alexander Viro, linux-fsdevel, Andrei Vagin, containers,
Jann Horn, Laurent Vivier
v5: Use READ_ONCE()/WRITE_ONCE()
move mount pointer struct init to bm_fill_super() and add smp_wmb()
remove useless NULL value init
add WARN_ON_ONCE()
v4: first user namespace is initialized with &init_binfmt_ns,
all new user namespaces are initialized with a NULL and use
the one of the first parent that is not NULL. The pointer
is initialized to a valid value the first time the binfmt_misc
fs is mounted in the current user namespace.
This allows to not change the way it was working before:
new ns inherits values from its parent, and if parent value is modified
(or parent creates its own binfmt entry by mounting the fs) child
inherits it (unless it has itself mounted the fs).
v3: create a structure to store binfmt_misc data,
add a pointer to this structure in the user_namespace structure,
in init_user_ns structure this pointer points to an init_binfmt_ns
structure. And all new user namespaces point to this init structure.
A new binfmt namespace structure is allocated if the binfmt_misc
filesystem is mounted in a user namespace that is not the initial
one but its binfmt namespace pointer points to the initial one.
add override_creds()/revert_creds() around open_exec() in
bm_register_write()
v2: no new namespace, binfmt_misc data are now part of
the mount namespace
I put this in mount namespace instead of user namespace
because the mount namespace is already needed and
I don't want to force to have the user namespace for that.
As this is a filesystem, it seems logic to have it here.
This allows to define a new interpreter for each new container.
But the main goal is to be able to chroot to a directory
using a binfmt_misc interpreter without being root.
I have a modified version of unshare at:
git@github.com:vivier/util-linux.git branch unshare-chroot
with some new options to unshare binfmt_misc namespace and to chroot
to a directory.
If you have a directory /chroot/powerpc/jessie containing debian for powerpc
binaries and a qemu-ppc interpreter, you can do for instance:
$ uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
$ ./unshare --map-root-user --fork --pid \
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
--root=/chroot/powerpc/jessie /bin/bash -l
# uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
# id
uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
# ls -l
total 5940
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
-rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
If you want to use the qemu binary provided by your distro, you can use
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
With the 'F' flag, qemu-ppc-static will be then loaded from the main root
filesystem before switching to the chroot.
Laurent Vivier (1):
ns: add binfmt_misc to the user namespace
fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
include/linux/user_namespace.h | 13 ++++
kernel/user.c | 13 ++++
kernel/user_namespace.c | 3 +
4 files changed, 107 insertions(+), 28 deletions(-)
--
2.17.1
^ permalink raw reply
* [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-09 10:37 UTC (permalink / raw)
To: linux-kernel
Cc: Eric Biederman, Dmitry Safonov, linux-api, James Bottomley,
Alexander Viro, linux-fsdevel, Andrei Vagin, containers,
Jann Horn, Laurent Vivier
In-Reply-To: <20181009103752.21482-1-laurent@vivier.eu>
This patch allows to have a different binfmt_misc configuration
for each new user namespace. By default, the binfmt_misc configuration
is the one of the previous level, but if the binfmt_misc filesystem is
mounted in the new namespace a new empty binfmt instance is created and
used in this namespace.
For instance, using "unshare" we can start a chroot of an another
architecture and configure the binfmt_misc interpreter without being root
to run the binaries in this chroot.
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
include/linux/user_namespace.h | 13 ++++
kernel/user.c | 13 ++++
kernel/user_namespace.c | 3 +
4 files changed, 107 insertions(+), 28 deletions(-)
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index aa4a7a23ff99..1e0029d097d9 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -38,9 +38,6 @@ enum {
VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
};
-static LIST_HEAD(entries);
-static int enabled = 1;
-
enum {Enabled, Magic};
#define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
#define MISC_FMT_OPEN_BINARY (1 << 30)
@@ -60,10 +57,7 @@ typedef struct {
struct file *interp_file;
} Node;
-static DEFINE_RWLOCK(entries_lock);
static struct file_system_type bm_fs_type;
-static struct vfsmount *bm_mnt;
-static int entry_count;
/*
* Max length of the register string. Determined by:
@@ -80,18 +74,32 @@ static int entry_count;
*/
#define MAX_REGISTER_LENGTH 1920
+static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
+{
+ struct binfmt_namespace *b_ns;
+
+ while (ns) {
+ b_ns = READ_ONCE(ns->binfmt_ns);
+ if (b_ns)
+ return b_ns;
+ ns = ns->parent;
+ }
+ WARN_ON_ONCE(1);
+ return NULL;
+}
+
/*
* Check if we support the binfmt
* if we do, return the node, else NULL
* locking is done in load_misc_binary
*/
-static Node *check_file(struct linux_binprm *bprm)
+static Node *check_file(struct binfmt_namespace *ns, struct linux_binprm *bprm)
{
char *p = strrchr(bprm->interp, '.');
struct list_head *l;
/* Walk all the registered handlers. */
- list_for_each(l, &entries) {
+ list_for_each(l, &ns->entries) {
Node *e = list_entry(l, Node, list);
char *s;
int j;
@@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
struct file *interp_file = NULL;
int retval;
int fd_binary = -1;
+ struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
retval = -ENOEXEC;
- if (!enabled)
+ if (!ns->enabled)
return retval;
/* to keep locking time low, we copy the interpreter string */
- read_lock(&entries_lock);
- fmt = check_file(bprm);
+ read_lock(&ns->entries_lock);
+ fmt = check_file(ns, bprm);
if (fmt)
dget(fmt->dentry);
- read_unlock(&entries_lock);
+ read_unlock(&ns->entries_lock);
if (!fmt)
return retval;
@@ -609,19 +618,19 @@ static void bm_evict_inode(struct inode *inode)
kfree(e);
}
-static void kill_node(Node *e)
+static void kill_node(struct binfmt_namespace *ns, Node *e)
{
struct dentry *dentry;
- write_lock(&entries_lock);
+ write_lock(&ns->entries_lock);
list_del_init(&e->list);
- write_unlock(&entries_lock);
+ write_unlock(&ns->entries_lock);
dentry = e->dentry;
drop_nlink(d_inode(dentry));
d_drop(dentry);
dput(dentry);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->bm_mnt, &ns->entry_count);
}
/* /<entry> */
@@ -651,6 +660,9 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
struct dentry *root;
Node *e = file_inode(file)->i_private;
int res = parse_command(buffer, count);
+ struct binfmt_namespace *ns;
+
+ ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
switch (res) {
case 1:
@@ -667,7 +679,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
inode_lock(d_inode(root));
if (!list_empty(&e->list))
- kill_node(e);
+ kill_node(ns, e);
inode_unlock(d_inode(root));
break;
@@ -693,6 +705,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
struct inode *inode;
struct super_block *sb = file_inode(file)->i_sb;
struct dentry *root = sb->s_root, *dentry;
+ struct binfmt_namespace *ns;
int err = 0;
e = create_entry(buffer, count);
@@ -716,7 +729,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (!inode)
goto out2;
- err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
+ ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
+ err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
+ &ns->entry_count);
if (err) {
iput(inode);
inode = NULL;
@@ -725,12 +740,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (e->flags & MISC_FMT_OPEN_FILE) {
struct file *f;
+ const struct cred *old_cred;
+ old_cred = override_creds(file->f_cred);
f = open_exec(e->interpreter);
+ revert_creds(old_cred);
if (IS_ERR(f)) {
err = PTR_ERR(f);
pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->bm_mnt,
+ &ns->entry_count);
iput(inode);
inode = NULL;
goto out2;
@@ -743,9 +762,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
inode->i_fop = &bm_entry_operations;
d_instantiate(dentry, inode);
- write_lock(&entries_lock);
- list_add(&e->list, &entries);
- write_unlock(&entries_lock);
+ write_lock(&ns->entries_lock);
+ list_add(&e->list, &ns->entries);
+ write_unlock(&ns->entries_lock);
err = 0;
out2:
@@ -770,7 +789,9 @@ static const struct file_operations bm_register_operations = {
static ssize_t
bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
{
- char *s = enabled ? "enabled\n" : "disabled\n";
+ struct binfmt_namespace *ns =
+ binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
+ char *s = ns->enabled ? "enabled\n" : "disabled\n";
return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
}
@@ -778,25 +799,28 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
static ssize_t bm_status_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
+ struct binfmt_namespace *ns;
int res = parse_command(buffer, count);
struct dentry *root;
+ ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
switch (res) {
case 1:
/* Disable all handlers. */
- enabled = 0;
+ ns->enabled = 0;
break;
case 2:
/* Enable all handlers. */
- enabled = 1;
+ ns->enabled = 1;
break;
case 3:
/* Delete all handlers. */
root = file_inode(file)->i_sb->s_root;
inode_lock(d_inode(root));
- while (!list_empty(&entries))
- kill_node(list_first_entry(&entries, Node, list));
+ while (!list_empty(&ns->entries))
+ kill_node(ns, list_first_entry(&ns->entries,
+ Node, list));
inode_unlock(d_inode(root));
break;
@@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
static int bm_fill_super(struct super_block *sb, void *data, int silent)
{
int err;
+ struct user_namespace *ns = sb->s_user_ns;
static const struct tree_descr bm_files[] = {
[2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
[3] = {"register", &bm_register_operations, S_IWUSR},
/* last one */ {""}
};
+ /* create a new binfmt namespace
+ * if we are not in the first user namespace
+ * but the binfmt namespace is the first one
+ */
+ if (READ_ONCE(ns->binfmt_ns) == NULL) {
+ struct binfmt_namespace *new_ns;
+
+ new_ns = kmalloc(sizeof(struct binfmt_namespace),
+ GFP_KERNEL);
+ if (new_ns == NULL)
+ return -ENOMEM;
+ INIT_LIST_HEAD(&new_ns->entries);
+ new_ns->enabled = 1;
+ rwlock_init(&new_ns->entries_lock);
+ new_ns->bm_mnt = NULL;
+ new_ns->entry_count = 0;
+ /* ensure new_ns is completely initialized before sharing it */
+ smp_wmb();
+ WRITE_ONCE(ns->binfmt_ns, new_ns);
+ }
+
err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
if (!err)
sb->s_op = &s_ops;
@@ -838,7 +884,10 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
static struct dentry *bm_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
- return mount_single(fs_type, flags, data, bm_fill_super);
+ struct user_namespace *ns = current_user_ns();
+
+ return mount_ns(fs_type, flags, data, ns, ns,
+ bm_fill_super);
}
static struct linux_binfmt misc_format = {
@@ -849,6 +898,7 @@ static struct linux_binfmt misc_format = {
static struct file_system_type bm_fs_type = {
.owner = THIS_MODULE,
.name = "binfmt_misc",
+ .fs_flags = FS_USERNS_MOUNT,
.mount = bm_mount,
.kill_sb = kill_litter_super,
};
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index d6b74b91096b..5c6e7e63b97e 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -52,6 +52,16 @@ enum ucount_type {
UCOUNT_COUNTS,
};
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+struct binfmt_namespace {
+ struct list_head entries;
+ rwlock_t entries_lock;
+ int enabled;
+ struct vfsmount *bm_mnt;
+ int entry_count;
+} __randomize_layout;
+#endif
+
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -76,6 +86,9 @@ struct user_namespace {
#endif
struct ucounts *ucounts;
int ucount_max[UCOUNT_COUNTS];
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ struct binfmt_namespace *binfmt_ns;
+#endif
} __randomize_layout;
struct ucounts {
diff --git a/kernel/user.c b/kernel/user.c
index 0df9b1640b2a..912916d435aa 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -19,6 +19,16 @@
#include <linux/user_namespace.h>
#include <linux/proc_ns.h>
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+static struct binfmt_namespace init_binfmt_ns = {
+ .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
+ .enabled = 1,
+ .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
+ .bm_mnt = NULL,
+ .entry_count = 0,
+};
+#endif
+
/*
* userns count is 1 for root user, 1 for init_uts_ns,
* and 1 for... ?
@@ -66,6 +76,9 @@ struct user_namespace init_user_ns = {
.persistent_keyring_register_sem =
__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
#endif
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ .binfmt_ns = &init_binfmt_ns,
+#endif
};
EXPORT_SYMBOL_GPL(init_user_ns);
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index e5222b5fb4fe..990cf5950a89 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -195,6 +195,9 @@ static void free_user_ns(struct work_struct *work)
kfree(ns->projid_map.forward);
kfree(ns->projid_map.reverse);
}
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ kfree(ns->binfmt_ns);
+#endif
retire_userns_sysctls(ns);
#ifdef CONFIG_PERSISTENT_KEYRINGS
key_put(ns->persistent_keyring_register);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-09 12:39 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module
In-Reply-To: <20181008181815.pwnqxngj22mhm2vj@brauner.io>
On Mon, Oct 8, 2018 at 8:18 PM Christian Brauner <christian@brauner.io> wrote:
> On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:
> > On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > > > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen wrote:
> > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > > index 44a31ac8373a..17685803a2af 100644
> > > > > > --- a/kernel/seccomp.c
> > > > > > +++ b/kernel/seccomp.c
> > > > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > > > >
> > > > > > return ret;
> > > > > > }
> > > > > > +
> > > > > > +long seccomp_new_listener(struct task_struct *task,
> > > > > > + unsigned long filter_off)
> > > > > > +{
> > > > > > + struct seccomp_filter *filter;
> > > > > > + struct file *listener;
> > > > > > + int fd;
> > > > > > +
> > > > > > + if (!capable(CAP_SYS_ADMIN))
> > > > > > + return -EACCES;
> > > > >
> > > > > I know this might have been discussed a while back but why exactly do we
> > > > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > > > use ptrace from in there?
> > > >
> > > > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > > > . Basically, the problem is that this doesn't just give you capability
> > > > over the target task, but also over every other task that has the same
> > > > filter installed; you need some sort of "is the caller capable over
> > > > the filter and anyone who uses it" check.
> > >
> > > Thanks.
> > > But then this new ptrace feature as it stands is imho currently broken.
> > > If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> > > are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> > > if you are ns_cpabable(CAP_SYS_ADMIN)
Actually, you don't need CAP_SYS_ADMIN for seccomp() at all as long as
you enable the NNP flag, I think?
> > > then either the new ptrace() api
> > > extension should be fixed to allow for this too or the seccomp() way of
> > > retrieving the pid - which I really think we want - needs to be fixed to
> > > require capable(CAP_SYS_ADMIN) too.
> > > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> > > the preferred way to solve this.
> > > Everything else will just be confusing.
> >
> > First you say "broken", then you say "confusing". Which one do you mean?
>
> Both. It's broken in so far as it places a seemingly unnecessary
> restriction that could be fixed. You outlined one possible fix yourself
> in the link you provided.
If by "possible fix" you mean "check whether the seccomp filter is
only attached to a single task": That wouldn't fundamentally change
the situation, it would only add an additional special case.
> And it's confusing in so far as there is a way
> via seccomp() to get the fd without said requirement.
I don't find it confusing at all. seccomp() and ptrace() are very
different situations: When you use seccomp(), infrastructure is
already in place for ensuring that your filter is only applied to
processes over which you are capable, and propagation is limited by
inheritance from your task down. When you use ptrace(), you need a
pretty different sort of access check that checks whether you're
privileged over ancestors, siblings and so on of the target task.
But thinking about it more, I think that CAP_SYS_ADMIN over the saved
current->mm->user_ns of the task that installed the filter (stored as
a "struct user_namespace *" in the filter) should be acceptable.
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2018-10-09 12:43 UTC (permalink / raw)
To: Laurent Vivier
Cc: kernel list, Eric W. Biederman, dima, Linux API, James Bottomley,
Al Viro, linux-fsdevel, avagin, containers
In-Reply-To: <20181009103752.21482-2-laurent@vivier.eu>
On Tue, Oct 9, 2018 at 12:38 PM Laurent Vivier <laurent@vivier.eu> wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the previous level, but if the binfmt_misc filesystem is
> mounted in the new namespace a new empty binfmt instance is created and
> used in this namespace.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
[...]
> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
> static int bm_fill_super(struct super_block *sb, void *data, int silent)
> {
> int err;
> + struct user_namespace *ns = sb->s_user_ns;
> static const struct tree_descr bm_files[] = {
> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
> [3] = {"register", &bm_register_operations, S_IWUSR},
> /* last one */ {""}
> };
>
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + /* ensure new_ns is completely initialized before sharing it */
> + smp_wmb();
> + WRITE_ONCE(ns->binfmt_ns, new_ns);
> + }
You're still not preventing a concurrent race of two mount() calls,
right? What prevents two instances of this code block from running
concurrently in two different namespaces? I think you want to take
some sort of global lock around this.
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-09 13:06 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Eric W. Biederman, dima, Linux API, James Bottomley,
Al Viro, linux-fsdevel, avagin, containers
In-Reply-To: <CAG48ez1OVKqRZe6AtTwXSc27AjNF596nt-46J9WPffKu7fNb8Q@mail.gmail.com>
Le 09/10/2018 à 14:43, Jann Horn a écrit :
> On Tue, Oct 9, 2018 at 12:38 PM Laurent Vivier <laurent@vivier.eu> wrote:
>> This patch allows to have a different binfmt_misc configuration
>> for each new user namespace. By default, the binfmt_misc configuration
>> is the one of the previous level, but if the binfmt_misc filesystem is
>> mounted in the new namespace a new empty binfmt instance is created and
>> used in this namespace.
>>
>> For instance, using "unshare" we can start a chroot of an another
>> architecture and configure the binfmt_misc interpreter without being root
>> to run the binaries in this chroot.
> [...]
>> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
>> static int bm_fill_super(struct super_block *sb, void *data, int silent)
>> {
>> int err;
>> + struct user_namespace *ns = sb->s_user_ns;
>> static const struct tree_descr bm_files[] = {
>> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
>> [3] = {"register", &bm_register_operations, S_IWUSR},
>> /* last one */ {""}
>> };
>>
>> + /* create a new binfmt namespace
>> + * if we are not in the first user namespace
>> + * but the binfmt namespace is the first one
>> + */
>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
>> + struct binfmt_namespace *new_ns;
>> +
>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>> + GFP_KERNEL);
>> + if (new_ns == NULL)
>> + return -ENOMEM;
>> + INIT_LIST_HEAD(&new_ns->entries);
>> + new_ns->enabled = 1;
>> + rwlock_init(&new_ns->entries_lock);
>> + new_ns->bm_mnt = NULL;
>> + new_ns->entry_count = 0;
>> + /* ensure new_ns is completely initialized before sharing it */
>> + smp_wmb();
>> + WRITE_ONCE(ns->binfmt_ns, new_ns);
>> + }
>
> You're still not preventing a concurrent race of two mount() calls,
> right? What prevents two instances of this code block from running
> concurrently in two different namespaces? I think you want to take
> some sort of global lock around this.
>
My guess was we have only one binfmt superblock by user namespace, so as
we can't have duplicate superblock, we will not have duplicate binfmt_ns
structure. This function is only called once in the namespace and I
think the superblock creation is already protected by some kind of lock.
But I'm not a VFS expert, if someone wants to clarify the situation,
please go ahead.
Thanks,
Laurent
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2018-10-09 13:15 UTC (permalink / raw)
To: Laurent Vivier
Cc: kernel list, Eric W. Biederman, dima, Linux API, James Bottomley,
Al Viro, linux-fsdevel, avagin, containers
In-Reply-To: <9059ed5a-6a0d-7f4d-7854-48b3ae4cca76@vivier.eu>
On Tue, Oct 9, 2018 at 3:06 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 09/10/2018 à 14:43, Jann Horn a écrit :
> > On Tue, Oct 9, 2018 at 12:38 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >> This patch allows to have a different binfmt_misc configuration
> >> for each new user namespace. By default, the binfmt_misc configuration
> >> is the one of the previous level, but if the binfmt_misc filesystem is
> >> mounted in the new namespace a new empty binfmt instance is created and
> >> used in this namespace.
> >>
> >> For instance, using "unshare" we can start a chroot of an another
> >> architecture and configure the binfmt_misc interpreter without being root
> >> to run the binaries in this chroot.
> > [...]
> >> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
> >> static int bm_fill_super(struct super_block *sb, void *data, int silent)
> >> {
> >> int err;
> >> + struct user_namespace *ns = sb->s_user_ns;
> >> static const struct tree_descr bm_files[] = {
> >> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
> >> [3] = {"register", &bm_register_operations, S_IWUSR},
> >> /* last one */ {""}
> >> };
> >>
> >> + /* create a new binfmt namespace
> >> + * if we are not in the first user namespace
> >> + * but the binfmt namespace is the first one
> >> + */
> >> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> >> + struct binfmt_namespace *new_ns;
> >> +
> >> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> >> + GFP_KERNEL);
> >> + if (new_ns == NULL)
> >> + return -ENOMEM;
> >> + INIT_LIST_HEAD(&new_ns->entries);
> >> + new_ns->enabled = 1;
> >> + rwlock_init(&new_ns->entries_lock);
> >> + new_ns->bm_mnt = NULL;
> >> + new_ns->entry_count = 0;
> >> + /* ensure new_ns is completely initialized before sharing it */
> >> + smp_wmb();
> >> + WRITE_ONCE(ns->binfmt_ns, new_ns);
> >> + }
> >
> > You're still not preventing a concurrent race of two mount() calls,
> > right? What prevents two instances of this code block from running
> > concurrently in two different namespaces? I think you want to take
> > some sort of global lock around this.
> >
>
> My guess was we have only one binfmt superblock by user namespace, so as
> we can't have duplicate superblock, we will not have duplicate binfmt_ns
> structure. This function is only called once in the namespace and I
> think the superblock creation is already protected by some kind of lock.
Ah! Nevermind, I missed the mount_ns().
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-09 13:28 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module
In-Reply-To: <CAG48ez3PEce4ynRnDk=yfsAOuFGH=YMmuf13CAxEt07Yjk3ZCw@mail.gmail.com>
On Tue, Oct 09, 2018 at 02:39:53PM +0200, Jann Horn wrote:
> On Mon, Oct 8, 2018 at 8:18 PM Christian Brauner <christian@brauner.io> wrote:
> > On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:
> > > On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> > > > On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > > > > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen wrote:
> > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > > > index 44a31ac8373a..17685803a2af 100644
> > > > > > > --- a/kernel/seccomp.c
> > > > > > > +++ b/kernel/seccomp.c
> > > > > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > > > > >
> > > > > > > return ret;
> > > > > > > }
> > > > > > > +
> > > > > > > +long seccomp_new_listener(struct task_struct *task,
> > > > > > > + unsigned long filter_off)
> > > > > > > +{
> > > > > > > + struct seccomp_filter *filter;
> > > > > > > + struct file *listener;
> > > > > > > + int fd;
> > > > > > > +
> > > > > > > + if (!capable(CAP_SYS_ADMIN))
> > > > > > > + return -EACCES;
> > > > > >
> > > > > > I know this might have been discussed a while back but why exactly do we
> > > > > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > > > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > > > > use ptrace from in there?
> > > > >
> > > > > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > > > > . Basically, the problem is that this doesn't just give you capability
> > > > > over the target task, but also over every other task that has the same
> > > > > filter installed; you need some sort of "is the caller capable over
> > > > > the filter and anyone who uses it" check.
> > > >
> > > > Thanks.
> > > > But then this new ptrace feature as it stands is imho currently broken.
> > > > If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> > > > are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> > > > if you are ns_cpabable(CAP_SYS_ADMIN)
>
> Actually, you don't need CAP_SYS_ADMIN for seccomp() at all as long as
> you enable the NNP flag, I think?
Yes, if you turn on NNP you don't even need sys_admin.
>
> > > > then either the new ptrace() api
> > > > extension should be fixed to allow for this too or the seccomp() way of
> > > > retrieving the pid - which I really think we want - needs to be fixed to
> > > > require capable(CAP_SYS_ADMIN) too.
> > > > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> > > > the preferred way to solve this.
> > > > Everything else will just be confusing.
> > >
> > > First you say "broken", then you say "confusing". Which one do you mean?
> >
> > Both. It's broken in so far as it places a seemingly unnecessary
> > restriction that could be fixed. You outlined one possible fix yourself
> > in the link you provided.
>
> If by "possible fix" you mean "check whether the seccomp filter is
> only attached to a single task": That wouldn't fundamentally change
> the situation, it would only add an additional special case.
>
> > And it's confusing in so far as there is a way
> > via seccomp() to get the fd without said requirement.
>
> I don't find it confusing at all. seccomp() and ptrace() are very
Fine, then that's a matter of opinion. I find it counterintuitive that
you can get an fd without privileges via one interface but not via
another.
> different situations: When you use seccomp(), infrastructure is
Sure. Note, that this is _one_ of the reasons why I want to make sure we
keep the native seccomp() only based way of getting an fd without
forcing userspace to switching to a differnet kernel api.
> already in place for ensuring that your filter is only applied to
> processes over which you are capable, and propagation is limited by
> inheritance from your task down. When you use ptrace(), you need a
> pretty different sort of access check that checks whether you're
> privileged over ancestors, siblings and so on of the target task.
So, don't get me wrong I'm not arguing against the ptrace() interface in
general. If this is something that people find useful, fine. But, I
would like to have a simple single-syscall pure-seccomp() based way of
getting an fd, i.e. what we have in patch 1 of this series.
>
> But thinking about it more, I think that CAP_SYS_ADMIN over the saved
> current->mm->user_ns of the task that installed the filter (stored as
> a "struct user_namespace *" in the filter) should be acceptable.
Hm... Why not CAP_SYS_PTRACE?
One more thing. Citing from [1]
> I think there's a security problem here. Imagine the following scenario:
>
> 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> 2. task A forks off a child B
> 3. task B uses setuid(1) to drop its privileges
> 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> or via execve()
> 5. task C (the attacker, uid==1) attaches to task B via ptrace
> 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
Sorry, to be late to the party but would this really pass
__ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
that it would... Doesn't look like it would get past:
tcred = __task_cred(task);
if (uid_eq(caller_uid, tcred->euid) &&
uid_eq(caller_uid, tcred->suid) &&
uid_eq(caller_uid, tcred->uid) &&
gid_eq(caller_gid, tcred->egid) &&
gid_eq(caller_gid, tcred->sgid) &&
gid_eq(caller_gid, tcred->gid))
goto ok;
if (ptrace_has_cap(tcred->user_ns, mode))
goto ok;
rcu_read_unlock();
return -EPERM;
ok:
rcu_read_unlock();
mm = task->mm;
if (mm &&
((get_dumpable(mm) != SUID_DUMP_USER) &&
!ptrace_has_cap(mm->user_ns, mode)))
return -EPERM;
> 7. because the seccomp filter is shared by task A and task B, task C
> is now able to influence syscall results for syscalls performed by
> task A
[1]: https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-09 13:36 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <20181009132850.fp6yne2vgmfpi27k@brauner.io>
+cc selinux people explicitly, since they probably have opinions on this
On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> On Tue, Oct 09, 2018 at 02:39:53PM +0200, Jann Horn wrote:
> > On Mon, Oct 8, 2018 at 8:18 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:
> > > > On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > > > > > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen wrote:
> > > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > > > > index 44a31ac8373a..17685803a2af 100644
> > > > > > > > --- a/kernel/seccomp.c
> > > > > > > > +++ b/kernel/seccomp.c
> > > > > > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > > > > > >
> > > > > > > > return ret;
> > > > > > > > }
> > > > > > > > +
> > > > > > > > +long seccomp_new_listener(struct task_struct *task,
> > > > > > > > + unsigned long filter_off)
> > > > > > > > +{
> > > > > > > > + struct seccomp_filter *filter;
> > > > > > > > + struct file *listener;
> > > > > > > > + int fd;
> > > > > > > > +
> > > > > > > > + if (!capable(CAP_SYS_ADMIN))
> > > > > > > > + return -EACCES;
> > > > > > >
> > > > > > > I know this might have been discussed a while back but why exactly do we
> > > > > > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > > > > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > > > > > use ptrace from in there?
> > > > > >
> > > > > > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > > > > > . Basically, the problem is that this doesn't just give you capability
> > > > > > over the target task, but also over every other task that has the same
> > > > > > filter installed; you need some sort of "is the caller capable over
> > > > > > the filter and anyone who uses it" check.
> > > > >
> > > > > Thanks.
> > > > > But then this new ptrace feature as it stands is imho currently broken.
> > > > > If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> > > > > are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> > > > > if you are ns_cpabable(CAP_SYS_ADMIN)
> >
> > Actually, you don't need CAP_SYS_ADMIN for seccomp() at all as long as
> > you enable the NNP flag, I think?
>
> Yes, if you turn on NNP you don't even need sys_admin.
>
> >
> > > > > then either the new ptrace() api
> > > > > extension should be fixed to allow for this too or the seccomp() way of
> > > > > retrieving the pid - which I really think we want - needs to be fixed to
> > > > > require capable(CAP_SYS_ADMIN) too.
> > > > > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> > > > > the preferred way to solve this.
> > > > > Everything else will just be confusing.
> > > >
> > > > First you say "broken", then you say "confusing". Which one do you mean?
> > >
> > > Both. It's broken in so far as it places a seemingly unnecessary
> > > restriction that could be fixed. You outlined one possible fix yourself
> > > in the link you provided.
> >
> > If by "possible fix" you mean "check whether the seccomp filter is
> > only attached to a single task": That wouldn't fundamentally change
> > the situation, it would only add an additional special case.
> >
> > > And it's confusing in so far as there is a way
> > > via seccomp() to get the fd without said requirement.
> >
> > I don't find it confusing at all. seccomp() and ptrace() are very
>
> Fine, then that's a matter of opinion. I find it counterintuitive that
> you can get an fd without privileges via one interface but not via
> another.
>
> > different situations: When you use seccomp(), infrastructure is
>
> Sure. Note, that this is _one_ of the reasons why I want to make sure we
> keep the native seccomp() only based way of getting an fd without
> forcing userspace to switching to a differnet kernel api.
>
> > already in place for ensuring that your filter is only applied to
> > processes over which you are capable, and propagation is limited by
> > inheritance from your task down. When you use ptrace(), you need a
> > pretty different sort of access check that checks whether you're
> > privileged over ancestors, siblings and so on of the target task.
>
> So, don't get me wrong I'm not arguing against the ptrace() interface in
> general. If this is something that people find useful, fine. But, I
> would like to have a simple single-syscall pure-seccomp() based way of
> getting an fd, i.e. what we have in patch 1 of this series.
Yeah, I also prefer the seccomp() one.
> > But thinking about it more, I think that CAP_SYS_ADMIN over the saved
> > current->mm->user_ns of the task that installed the filter (stored as
> > a "struct user_namespace *" in the filter) should be acceptable.
>
> Hm... Why not CAP_SYS_PTRACE?
Because LSMs like SELinux add extra checks that apply even if you have
CAP_SYS_PTRACE, and this would subvert those. The only capability I
know of that lets you bypass LSM checks by design (if no LSM blocks
the capability itself) is CAP_SYS_ADMIN.
> One more thing. Citing from [1]
>
> > I think there's a security problem here. Imagine the following scenario:
> >
> > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > 2. task A forks off a child B
> > 3. task B uses setuid(1) to drop its privileges
> > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > or via execve()
> > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
>
> Sorry, to be late to the party but would this really pass
> __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> that it would... Doesn't look like it would get past:
>
> tcred = __task_cred(task);
> if (uid_eq(caller_uid, tcred->euid) &&
> uid_eq(caller_uid, tcred->suid) &&
> uid_eq(caller_uid, tcred->uid) &&
> gid_eq(caller_gid, tcred->egid) &&
> gid_eq(caller_gid, tcred->sgid) &&
> gid_eq(caller_gid, tcred->gid))
> goto ok;
> if (ptrace_has_cap(tcred->user_ns, mode))
> goto ok;
> rcu_read_unlock();
> return -EPERM;
> ok:
> rcu_read_unlock();
> mm = task->mm;
> if (mm &&
> ((get_dumpable(mm) != SUID_DUMP_USER) &&
> !ptrace_has_cap(mm->user_ns, mode)))
> return -EPERM;
Which specific check would prevent task C from attaching to task B? If
the UIDs match, the first "goto ok" executes; and you're dumpable, so
you don't trigger the second "return -EPERM".
> > 7. because the seccomp filter is shared by task A and task B, task C
> > is now able to influence syscall results for syscalls performed by
> > task A
>
> [1]: https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-09 13:49 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <CAG48ez2EV8Z=Egn7hNpXd86bHdvQhVxStqE1N=U2L2R7sA-gjg@mail.gmail.com>
On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> +cc selinux people explicitly, since they probably have opinions on this
>
> On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > On Tue, Oct 09, 2018 at 02:39:53PM +0200, Jann Horn wrote:
> > > On Mon, Oct 8, 2018 at 8:18 PM Christian Brauner <christian@brauner.io> wrote:
> > > > On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:
> > > > > On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > > > > > > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen wrote:
> > > > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > > > > > index 44a31ac8373a..17685803a2af 100644
> > > > > > > > > --- a/kernel/seccomp.c
> > > > > > > > > +++ b/kernel/seccomp.c
> > > > > > > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > > > > > > >
> > > > > > > > > return ret;
> > > > > > > > > }
> > > > > > > > > +
> > > > > > > > > +long seccomp_new_listener(struct task_struct *task,
> > > > > > > > > + unsigned long filter_off)
> > > > > > > > > +{
> > > > > > > > > + struct seccomp_filter *filter;
> > > > > > > > > + struct file *listener;
> > > > > > > > > + int fd;
> > > > > > > > > +
> > > > > > > > > + if (!capable(CAP_SYS_ADMIN))
> > > > > > > > > + return -EACCES;
> > > > > > > >
> > > > > > > > I know this might have been discussed a while back but why exactly do we
> > > > > > > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > > > > > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > > > > > > use ptrace from in there?
> > > > > > >
> > > > > > > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > > > > > > . Basically, the problem is that this doesn't just give you capability
> > > > > > > over the target task, but also over every other task that has the same
> > > > > > > filter installed; you need some sort of "is the caller capable over
> > > > > > > the filter and anyone who uses it" check.
> > > > > >
> > > > > > Thanks.
> > > > > > But then this new ptrace feature as it stands is imho currently broken.
> > > > > > If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> > > > > > are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> > > > > > if you are ns_cpabable(CAP_SYS_ADMIN)
> > >
> > > Actually, you don't need CAP_SYS_ADMIN for seccomp() at all as long as
> > > you enable the NNP flag, I think?
> >
> > Yes, if you turn on NNP you don't even need sys_admin.
> >
> > >
> > > > > > then either the new ptrace() api
> > > > > > extension should be fixed to allow for this too or the seccomp() way of
> > > > > > retrieving the pid - which I really think we want - needs to be fixed to
> > > > > > require capable(CAP_SYS_ADMIN) too.
> > > > > > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> > > > > > the preferred way to solve this.
> > > > > > Everything else will just be confusing.
> > > > >
> > > > > First you say "broken", then you say "confusing". Which one do you mean?
> > > >
> > > > Both. It's broken in so far as it places a seemingly unnecessary
> > > > restriction that could be fixed. You outlined one possible fix yourself
> > > > in the link you provided.
> > >
> > > If by "possible fix" you mean "check whether the seccomp filter is
> > > only attached to a single task": That wouldn't fundamentally change
> > > the situation, it would only add an additional special case.
> > >
> > > > And it's confusing in so far as there is a way
> > > > via seccomp() to get the fd without said requirement.
> > >
> > > I don't find it confusing at all. seccomp() and ptrace() are very
> >
> > Fine, then that's a matter of opinion. I find it counterintuitive that
> > you can get an fd without privileges via one interface but not via
> > another.
> >
> > > different situations: When you use seccomp(), infrastructure is
> >
> > Sure. Note, that this is _one_ of the reasons why I want to make sure we
> > keep the native seccomp() only based way of getting an fd without
> > forcing userspace to switching to a differnet kernel api.
> >
> > > already in place for ensuring that your filter is only applied to
> > > processes over which you are capable, and propagation is limited by
> > > inheritance from your task down. When you use ptrace(), you need a
> > > pretty different sort of access check that checks whether you're
> > > privileged over ancestors, siblings and so on of the target task.
> >
> > So, don't get me wrong I'm not arguing against the ptrace() interface in
> > general. If this is something that people find useful, fine. But, I
> > would like to have a simple single-syscall pure-seccomp() based way of
> > getting an fd, i.e. what we have in patch 1 of this series.
>
> Yeah, I also prefer the seccomp() one.
>
> > > But thinking about it more, I think that CAP_SYS_ADMIN over the saved
> > > current->mm->user_ns of the task that installed the filter (stored as
> > > a "struct user_namespace *" in the filter) should be acceptable.
> >
> > Hm... Why not CAP_SYS_PTRACE?
>
> Because LSMs like SELinux add extra checks that apply even if you have
> CAP_SYS_PTRACE, and this would subvert those. The only capability I
> know of that lets you bypass LSM checks by design (if no LSM blocks
> the capability itself) is CAP_SYS_ADMIN.
>
> > One more thing. Citing from [1]
> >
> > > I think there's a security problem here. Imagine the following scenario:
> > >
> > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > 2. task A forks off a child B
> > > 3. task B uses setuid(1) to drop its privileges
> > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > or via execve()
> > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> >
> > Sorry, to be late to the party but would this really pass
> > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > that it would... Doesn't look like it would get past:
> >
> > tcred = __task_cred(task);
> > if (uid_eq(caller_uid, tcred->euid) &&
> > uid_eq(caller_uid, tcred->suid) &&
> > uid_eq(caller_uid, tcred->uid) &&
> > gid_eq(caller_gid, tcred->egid) &&
> > gid_eq(caller_gid, tcred->sgid) &&
> > gid_eq(caller_gid, tcred->gid))
> > goto ok;
> > if (ptrace_has_cap(tcred->user_ns, mode))
> > goto ok;
> > rcu_read_unlock();
> > return -EPERM;
> > ok:
> > rcu_read_unlock();
> > mm = task->mm;
> > if (mm &&
> > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > !ptrace_has_cap(mm->user_ns, mode)))
> > return -EPERM;
>
> Which specific check would prevent task C from attaching to task B? If
> the UIDs match, the first "goto ok" executes; and you're dumpable, so
> you don't trigger the second "return -EPERM".
You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
have if you did a setuid to an unpriv user. (But I always find that code
confusing.)
>
> > > 7. because the seccomp filter is shared by task A and task B, task C
> > > is now able to influence syscall results for syscalls performed by
> > > task A
> >
> > [1]: https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-09 13:50 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <20181009134923.2fvf5roghqgaj5gq@brauner.io>
On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > +cc selinux people explicitly, since they probably have opinions on this
> >
> > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Tue, Oct 09, 2018 at 02:39:53PM +0200, Jann Horn wrote:
> > > > On Mon, Oct 8, 2018 at 8:18 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:
> > > > > > On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > > > > > > > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen wrote:
> > > > > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > > > > > > index 44a31ac8373a..17685803a2af 100644
> > > > > > > > > > --- a/kernel/seccomp.c
> > > > > > > > > > +++ b/kernel/seccomp.c
> > > > > > > > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > > > > > > > >
> > > > > > > > > > return ret;
> > > > > > > > > > }
> > > > > > > > > > +
> > > > > > > > > > +long seccomp_new_listener(struct task_struct *task,
> > > > > > > > > > + unsigned long filter_off)
> > > > > > > > > > +{
> > > > > > > > > > + struct seccomp_filter *filter;
> > > > > > > > > > + struct file *listener;
> > > > > > > > > > + int fd;
> > > > > > > > > > +
> > > > > > > > > > + if (!capable(CAP_SYS_ADMIN))
> > > > > > > > > > + return -EACCES;
> > > > > > > > >
> > > > > > > > > I know this might have been discussed a while back but why exactly do we
> > > > > > > > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > > > > > > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > > > > > > > use ptrace from in there?
> > > > > > > >
> > > > > > > > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > > > > > > > . Basically, the problem is that this doesn't just give you capability
> > > > > > > > over the target task, but also over every other task that has the same
> > > > > > > > filter installed; you need some sort of "is the caller capable over
> > > > > > > > the filter and anyone who uses it" check.
> > > > > > >
> > > > > > > Thanks.
> > > > > > > But then this new ptrace feature as it stands is imho currently broken.
> > > > > > > If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> > > > > > > are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> > > > > > > if you are ns_cpabable(CAP_SYS_ADMIN)
> > > >
> > > > Actually, you don't need CAP_SYS_ADMIN for seccomp() at all as long as
> > > > you enable the NNP flag, I think?
> > >
> > > Yes, if you turn on NNP you don't even need sys_admin.
> > >
> > > >
> > > > > > > then either the new ptrace() api
> > > > > > > extension should be fixed to allow for this too or the seccomp() way of
> > > > > > > retrieving the pid - which I really think we want - needs to be fixed to
> > > > > > > require capable(CAP_SYS_ADMIN) too.
> > > > > > > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> > > > > > > the preferred way to solve this.
> > > > > > > Everything else will just be confusing.
> > > > > >
> > > > > > First you say "broken", then you say "confusing". Which one do you mean?
> > > > >
> > > > > Both. It's broken in so far as it places a seemingly unnecessary
> > > > > restriction that could be fixed. You outlined one possible fix yourself
> > > > > in the link you provided.
> > > >
> > > > If by "possible fix" you mean "check whether the seccomp filter is
> > > > only attached to a single task": That wouldn't fundamentally change
> > > > the situation, it would only add an additional special case.
> > > >
> > > > > And it's confusing in so far as there is a way
> > > > > via seccomp() to get the fd without said requirement.
> > > >
> > > > I don't find it confusing at all. seccomp() and ptrace() are very
> > >
> > > Fine, then that's a matter of opinion. I find it counterintuitive that
> > > you can get an fd without privileges via one interface but not via
> > > another.
> > >
> > > > different situations: When you use seccomp(), infrastructure is
> > >
> > > Sure. Note, that this is _one_ of the reasons why I want to make sure we
> > > keep the native seccomp() only based way of getting an fd without
> > > forcing userspace to switching to a differnet kernel api.
> > >
> > > > already in place for ensuring that your filter is only applied to
> > > > processes over which you are capable, and propagation is limited by
> > > > inheritance from your task down. When you use ptrace(), you need a
> > > > pretty different sort of access check that checks whether you're
> > > > privileged over ancestors, siblings and so on of the target task.
> > >
> > > So, don't get me wrong I'm not arguing against the ptrace() interface in
> > > general. If this is something that people find useful, fine. But, I
> > > would like to have a simple single-syscall pure-seccomp() based way of
> > > getting an fd, i.e. what we have in patch 1 of this series.
> >
> > Yeah, I also prefer the seccomp() one.
> >
> > > > But thinking about it more, I think that CAP_SYS_ADMIN over the saved
> > > > current->mm->user_ns of the task that installed the filter (stored as
> > > > a "struct user_namespace *" in the filter) should be acceptable.
> > >
> > > Hm... Why not CAP_SYS_PTRACE?
> >
> > Because LSMs like SELinux add extra checks that apply even if you have
> > CAP_SYS_PTRACE, and this would subvert those. The only capability I
> > know of that lets you bypass LSM checks by design (if no LSM blocks
> > the capability itself) is CAP_SYS_ADMIN.
> >
> > > One more thing. Citing from [1]
> > >
> > > > I think there's a security problem here. Imagine the following scenario:
> > > >
> > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > 2. task A forks off a child B
> > > > 3. task B uses setuid(1) to drop its privileges
> > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > or via execve()
> > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > >
> > > Sorry, to be late to the party but would this really pass
> > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > that it would... Doesn't look like it would get past:
> > >
> > > tcred = __task_cred(task);
> > > if (uid_eq(caller_uid, tcred->euid) &&
> > > uid_eq(caller_uid, tcred->suid) &&
> > > uid_eq(caller_uid, tcred->uid) &&
> > > gid_eq(caller_gid, tcred->egid) &&
> > > gid_eq(caller_gid, tcred->sgid) &&
> > > gid_eq(caller_gid, tcred->gid))
> > > goto ok;
> > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > goto ok;
> > > rcu_read_unlock();
> > > return -EPERM;
> > > ok:
> > > rcu_read_unlock();
> > > mm = task->mm;
> > > if (mm &&
> > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > !ptrace_has_cap(mm->user_ns, mode)))
> > > return -EPERM;
> >
> > Which specific check would prevent task C from attaching to task B? If
> > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > you don't trigger the second "return -EPERM".
>
> You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> have if you did a setuid to an unpriv user. (But I always find that code
> confusing.)
Only if the target hasn't gone through execve() since setuid().
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-09 14:09 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <CAG48ez1hnKkm7mzXuZLP4n6x8K1JqmTwQb-e79oJguTaH1mkUw@mail.gmail.com>
On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > +cc selinux people explicitly, since they probably have opinions on this
> > >
> > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > On Tue, Oct 09, 2018 at 02:39:53PM +0200, Jann Horn wrote:
> > > > > On Mon, Oct 8, 2018 at 8:18 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > On Mon, Oct 08, 2018 at 06:42:00PM +0200, Jann Horn wrote:
> > > > > > > On Mon, Oct 8, 2018 at 6:21 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > On Mon, Oct 08, 2018 at 05:33:22PM +0200, Jann Horn wrote:
> > > > > > > > > On Mon, Oct 8, 2018 at 5:16 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > > > On Thu, Sep 27, 2018 at 09:11:16AM -0600, Tycho Andersen wrote:
> > > > > > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > > > > > > > index 44a31ac8373a..17685803a2af 100644
> > > > > > > > > > > --- a/kernel/seccomp.c
> > > > > > > > > > > +++ b/kernel/seccomp.c
> > > > > > > > > > > @@ -1777,4 +1777,35 @@ static struct file *init_listener(struct task_struct *task,
> > > > > > > > > > >
> > > > > > > > > > > return ret;
> > > > > > > > > > > }
> > > > > > > > > > > +
> > > > > > > > > > > +long seccomp_new_listener(struct task_struct *task,
> > > > > > > > > > > + unsigned long filter_off)
> > > > > > > > > > > +{
> > > > > > > > > > > + struct seccomp_filter *filter;
> > > > > > > > > > > + struct file *listener;
> > > > > > > > > > > + int fd;
> > > > > > > > > > > +
> > > > > > > > > > > + if (!capable(CAP_SYS_ADMIN))
> > > > > > > > > > > + return -EACCES;
> > > > > > > > > >
> > > > > > > > > > I know this might have been discussed a while back but why exactly do we
> > > > > > > > > > require CAP_SYS_ADMIN in init_userns and not in the target userns? What
> > > > > > > > > > if I want to do a setns()fd, CLONE_NEWUSER) to the target process and
> > > > > > > > > > use ptrace from in there?
> > > > > > > > >
> > > > > > > > > See https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjJWsO5pCvnH68P+RKO8Ow@mail.gmail.com/
> > > > > > > > > . Basically, the problem is that this doesn't just give you capability
> > > > > > > > > over the target task, but also over every other task that has the same
> > > > > > > > > filter installed; you need some sort of "is the caller capable over
> > > > > > > > > the filter and anyone who uses it" check.
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > > But then this new ptrace feature as it stands is imho currently broken.
> > > > > > > > If you can install a seccomp filter with SECCOMP_RET_USER_NOTIF if you
> > > > > > > > are ns_cpabable(CAP_SYS_ADMIN) and also get an fd via seccomp() itself
> > > > > > > > if you are ns_cpabable(CAP_SYS_ADMIN)
> > > > >
> > > > > Actually, you don't need CAP_SYS_ADMIN for seccomp() at all as long as
> > > > > you enable the NNP flag, I think?
> > > >
> > > > Yes, if you turn on NNP you don't even need sys_admin.
> > > >
> > > > >
> > > > > > > > then either the new ptrace() api
> > > > > > > > extension should be fixed to allow for this too or the seccomp() way of
> > > > > > > > retrieving the pid - which I really think we want - needs to be fixed to
> > > > > > > > require capable(CAP_SYS_ADMIN) too.
> > > > > > > > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho -
> > > > > > > > the preferred way to solve this.
> > > > > > > > Everything else will just be confusing.
> > > > > > >
> > > > > > > First you say "broken", then you say "confusing". Which one do you mean?
> > > > > >
> > > > > > Both. It's broken in so far as it places a seemingly unnecessary
> > > > > > restriction that could be fixed. You outlined one possible fix yourself
> > > > > > in the link you provided.
> > > > >
> > > > > If by "possible fix" you mean "check whether the seccomp filter is
> > > > > only attached to a single task": That wouldn't fundamentally change
> > > > > the situation, it would only add an additional special case.
> > > > >
> > > > > > And it's confusing in so far as there is a way
> > > > > > via seccomp() to get the fd without said requirement.
> > > > >
> > > > > I don't find it confusing at all. seccomp() and ptrace() are very
> > > >
> > > > Fine, then that's a matter of opinion. I find it counterintuitive that
> > > > you can get an fd without privileges via one interface but not via
> > > > another.
> > > >
> > > > > different situations: When you use seccomp(), infrastructure is
> > > >
> > > > Sure. Note, that this is _one_ of the reasons why I want to make sure we
> > > > keep the native seccomp() only based way of getting an fd without
> > > > forcing userspace to switching to a differnet kernel api.
> > > >
> > > > > already in place for ensuring that your filter is only applied to
> > > > > processes over which you are capable, and propagation is limited by
> > > > > inheritance from your task down. When you use ptrace(), you need a
> > > > > pretty different sort of access check that checks whether you're
> > > > > privileged over ancestors, siblings and so on of the target task.
> > > >
> > > > So, don't get me wrong I'm not arguing against the ptrace() interface in
> > > > general. If this is something that people find useful, fine. But, I
> > > > would like to have a simple single-syscall pure-seccomp() based way of
> > > > getting an fd, i.e. what we have in patch 1 of this series.
> > >
> > > Yeah, I also prefer the seccomp() one.
> > >
> > > > > But thinking about it more, I think that CAP_SYS_ADMIN over the saved
> > > > > current->mm->user_ns of the task that installed the filter (stored as
> > > > > a "struct user_namespace *" in the filter) should be acceptable.
> > > >
> > > > Hm... Why not CAP_SYS_PTRACE?
> > >
> > > Because LSMs like SELinux add extra checks that apply even if you have
> > > CAP_SYS_PTRACE, and this would subvert those. The only capability I
> > > know of that lets you bypass LSM checks by design (if no LSM blocks
> > > the capability itself) is CAP_SYS_ADMIN.
> > >
> > > > One more thing. Citing from [1]
> > > >
> > > > > I think there's a security problem here. Imagine the following scenario:
> > > > >
> > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > 2. task A forks off a child B
> > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > or via execve()
> > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > >
> > > > Sorry, to be late to the party but would this really pass
> > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > that it would... Doesn't look like it would get past:
> > > >
> > > > tcred = __task_cred(task);
> > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > uid_eq(caller_uid, tcred->suid) &&
> > > > uid_eq(caller_uid, tcred->uid) &&
> > > > gid_eq(caller_gid, tcred->egid) &&
> > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > gid_eq(caller_gid, tcred->gid))
> > > > goto ok;
> > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > goto ok;
> > > > rcu_read_unlock();
> > > > return -EPERM;
> > > > ok:
> > > > rcu_read_unlock();
> > > > mm = task->mm;
> > > > if (mm &&
> > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > return -EPERM;
> > >
> > > Which specific check would prevent task C from attaching to task B? If
> > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > you don't trigger the second "return -EPERM".
> >
> > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > have if you did a setuid to an unpriv user. (But I always find that code
> > confusing.)
>
> Only if the target hasn't gone through execve() since setuid().
Sorry if I want to know this in excessive detail but I'd like to
understand this properly so bear with me :)
- If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
- If task B has setuid()ed, exeved()ed it will get its dumpable flag set
to /proc/sys/fs/suid_dumpable which by default is 0. So C won't pass
(get_dumpable(mm) != SUID_DUMP_USER).
In both cases PTRACE_ATTACH shouldn't work. Now, if
/proc/sys/fs/suid_dumpable is 1 I'd find it acceptable for this to work.
This is an administrator choice.
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-09 14:28 UTC (permalink / raw)
To: Christian Brauner
Cc: Kees Cook, Jann Horn, Linux API, Linux Containers, Akihiro Suda,
Oleg Nesterov, LKML, Eric W . Biederman,
linux-fsdevel@vger.kernel.org, Christian Brauner, Andy Lutomirski
In-Reply-To: <20181008145803.ycawjwhc3mwkdogf@brauner.io>
On Mon, Oct 08, 2018 at 04:58:05PM +0200, Christian Brauner wrote:
> On Thu, Sep 27, 2018 at 04:48:39PM -0600, Tycho Andersen wrote:
> > On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> > > I have to say, I'm vaguely nervous about changing the semantics here
> > > for passing back the fd as the return code from the seccomp() syscall.
> > > Alternatives seem less appealing, though: changing the meaning of the
> > > uargs parameter when SECCOMP_FILTER_FLAG_NEW_LISTENER is set, for
> > > example. Hmm.
> >
> > From my perspective we can drop this whole thing. The only thing I'll
> > ever use is the ptrace version. Someone at some point (I don't
> > remember who, maybe stgraber) suggested this version would be useful
> > as well.
>
> So I think we want to have the ability to get an fd via seccomp().
> Especially, if we all we worry about are weird semantics. When we
> discussed this we knew the whole patchset was going to be weird. :)
>
> This is a seccomp feature so seccomp should - if feasible - equip you
> with everything to use it in a meaningful way without having to go
> through a different kernel api. I know ptrace and seccomp are
> already connected but I still find this cleaner. :)
>
> Another thing is that the container itself might be traced for some
> reason while you still might want to get an fd out.
Sure, I don't see the problem here.
> Also, I wonder what happens if you want to filter the ptrace() syscall
> itself? Then you'd deadlock?
No, are you confusing the tracee with the tracer here? Filtering
ptrace() will happen just like any other syscall... what would you
deadlock with?
> Also, it seems that getting an fd via ptrace requires CAP_SYS_ADMIN in
> the inital user namespace (which I just realized now) whereas getting
> the fd via seccomp() doesn't seem to.
Yep, I'll leave this discussion to the other thread.
Tycho
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Tycho Andersen @ 2018-10-09 15:16 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Dmitry Safonov, linux-api, containers, Jann Horn,
James Bottomley, Eric Biederman, linux-fsdevel, Alexander Viro
In-Reply-To: <20181009103752.21482-2-laurent@vivier.eu>
On Tue, Oct 09, 2018 at 12:37:52PM +0200, Laurent Vivier wrote:
> @@ -80,18 +74,32 @@ static int entry_count;
> */
> #define MAX_REGISTER_LENGTH 1920
>
> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> +{
> + struct binfmt_namespace *b_ns;
> +
> + while (ns) {
> + b_ns = READ_ONCE(ns->binfmt_ns);
> + if (b_ns)
> + return b_ns;
> + ns = ns->parent;
> + }
> + WARN_ON_ONCE(1);
It looks like we warn here,
> @@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> struct file *interp_file = NULL;
> int retval;
> int fd_binary = -1;
> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!ns->enabled)
...but then in cases like this we immediately dereference the pointer
anyways and crash. Can we return some other error code here in the !ns
case so we don't crash?
Tycho
^ permalink raw reply
* Re: [PATCH v3 3/3] namei: aggressively check for nd->root escape on ".." resolution
From: Jann Horn @ 2018-10-09 15:19 UTC (permalink / raw)
To: cyphar
Cc: Al Viro, Eric W. Biederman, jlayton, Bruce Fields, Arnd Bergmann,
Andy Lutomirski, David Howells, christian, Tycho Andersen,
David Drysdale, dev, containers, linux-fsdevel, kernel list,
linux-arch, Linux API
In-Reply-To: <20181009070230.12884-4-cyphar@cyphar.com>
On Tue, Oct 9, 2018 at 9:03 AM Aleksa Sarai <cyphar@cyphar.com> 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.
> 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 <cyphar@cyphar.com>
> ---
> fs/namei.c | 79 +++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 61 insertions(+), 18 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index b31aef27df22..12cd8d8987ea 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
[...]
> +static inline int nd_alloc_dpathbuf(struct nameidata *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;
> + }
> + }
> + return 0;
> +}
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.
> 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?
> + if (unlikely(IS_ERR(pathptr)))
> + return PTR_ERR(pathptr);
> + }
> }
> return 0;
> }
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-09 15:19 UTC (permalink / raw)
To: Tycho Andersen
Cc: linux-kernel, Dmitry Safonov, linux-api, containers, Jann Horn,
James Bottomley, Eric Biederman, linux-fsdevel, Alexander Viro
In-Reply-To: <20181009151641.GB10149@cisco>
Le 09/10/2018 à 17:16, Tycho Andersen a écrit :
> On Tue, Oct 09, 2018 at 12:37:52PM +0200, Laurent Vivier wrote:
>> @@ -80,18 +74,32 @@ static int entry_count;
>> */
>> #define MAX_REGISTER_LENGTH 1920
>>
>> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
>> +{
>> + struct binfmt_namespace *b_ns;
>> +
>> + while (ns) {
>> + b_ns = READ_ONCE(ns->binfmt_ns);
>> + if (b_ns)
>> + return b_ns;
>> + ns = ns->parent;
>> + }
>> + WARN_ON_ONCE(1);
>
> It looks like we warn here,
>
>> @@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
>> struct file *interp_file = NULL;
>> int retval;
>> int fd_binary = -1;
>> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>>
>> retval = -ENOEXEC;
>> - if (!enabled)
>> + if (!ns->enabled)
>
> ...but then in cases like this we immediately dereference the pointer
> anyways and crash. Can we return some other error code here in the !ns
> case so we don't crash?
My concern here is I don't want to add code to check an error case that
cannot happen. The first namespace binfmt_ns pointer is initialized with
&init_binfmt_ns, so the return value cannot be NULL.
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-09 15:26 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <20181009140932.e5w5lgbgucbl72kt@brauner.io>
On Tue, Oct 9, 2018 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
> On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> > On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > One more thing. Citing from [1]
> > > > >
> > > > > > I think there's a security problem here. Imagine the following scenario:
> > > > > >
> > > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > > 2. task A forks off a child B
> > > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > > or via execve()
> > > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > > >
> > > > > Sorry, to be late to the party but would this really pass
> > > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > > that it would... Doesn't look like it would get past:
> > > > >
> > > > > tcred = __task_cred(task);
> > > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > > uid_eq(caller_uid, tcred->suid) &&
> > > > > uid_eq(caller_uid, tcred->uid) &&
> > > > > gid_eq(caller_gid, tcred->egid) &&
> > > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > > gid_eq(caller_gid, tcred->gid))
> > > > > goto ok;
> > > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > > goto ok;
> > > > > rcu_read_unlock();
> > > > > return -EPERM;
> > > > > ok:
> > > > > rcu_read_unlock();
> > > > > mm = task->mm;
> > > > > if (mm &&
> > > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > > return -EPERM;
> > > >
> > > > Which specific check would prevent task C from attaching to task B? If
> > > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > > you don't trigger the second "return -EPERM".
> > >
> > > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > > have if you did a setuid to an unpriv user. (But I always find that code
> > > confusing.)
> >
> > Only if the target hasn't gone through execve() since setuid().
>
> Sorry if I want to know this in excessive detail but I'd like to
> understand this properly so bear with me :)
> - If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
> execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
Yeah.
> - If task B has setuid()ed, exeved()ed it will get its dumpable flag set
> to /proc/sys/fs/suid_dumpable
Not if you changed all UIDs (e.g. by calling setuid() as root). In
that case, setup_new_exec() calls "set_dumpable(current->mm,
SUID_DUMP_USER)".
> which by default is 0. So C won't pass
> (get_dumpable(mm) != SUID_DUMP_USER).
> In both cases PTRACE_ATTACH shouldn't work. Now, if
> /proc/sys/fs/suid_dumpable is 1 I'd find it acceptable for this to work.
> This is an administrator choice.
^ permalink raw reply
* Re: [netfilter-core] [PATCH 07/11] UAPI: netfilter: Fix symbol collision issues [ver #2]
From: David Howells @ 2018-10-09 15:35 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: dhowells, linux-api, linux-kbuild, coreteam, netfilter-devel,
linux-kernel
In-Reply-To: <20180928130704.kcmsnsecp6aqpk66@salvia>
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> David, may I upstream this or you will pass it to Greg? I can just
> take this, as you prefer.
Feel free to take it.
David
^ permalink raw reply
* Re: [PATCH 10/11] UAPI: ndctl: Remove use of PAGE_SIZE [ver #2]
From: David Howells @ 2018-10-09 15:36 UTC (permalink / raw)
To: Dan Williams
Cc: dhowells, Linux API, linux-kbuild, linux-nvdimm,
Linux Kernel Mailing List
In-Reply-To: <CAPcyv4gmMwRAG=VxWNyeacxdsPCsaT+BtR+27+bHT=6heAZ3sQ@mail.gmail.com>
Dan Williams <dan.j.williams@intel.com> wrote:
> Let me know if you want me to pick this up for 4.20.
If you could pick this and also 09?
Thanks,
David
^ permalink raw reply
* Re: [PATCH v3 3/3] namei: aggressively check for nd->root escape on ".." resolution
From: Aleksa Sarai @ 2018-10-09 15:37 UTC (permalink / raw)
To: Jann Horn
Cc: cyphar, Al Viro, Eric W. Biederman, jlayton, Bruce Fields,
Arnd Bergmann, Andy Lutomirski, David Howells, christian,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
kernel list, linux-arch, Linux API
In-Reply-To: <CAG48ez3Ae5b20g2UquekfBNaUw1tLt4nWy2pYm-LNRMbDVpHfA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6238 bytes --]
On 2018-10-09, 'Jann Horn' via dev <jannh@google.com> wrote:
> On Tue, Oct 9, 2018 at 9:03 AM Aleksa Sarai <cyphar@cyphar.com> 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.
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...
> > +static inline int nd_alloc_dpathbuf(struct nameidata *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;
> > + }
> > + }
> > + return 0;
> > +}
>
> 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.
> > + 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/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 04/11] UAPI: bcache: Fix use of embedded flexible array
From: David Howells @ 2018-10-09 15:41 UTC (permalink / raw)
To: Jan Engelhardt
Cc: dhowells, linux-api, linux-kbuild, Coly Li, Kent Overstreet,
linux-bcache, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.1810021622320.11696@n3.vanv.qr>
Jan Engelhardt <jengelh@inai.de> wrote:
> """it [the array size expression] shall be a converted constant expression of
> type std::size_t and its value shall be greater than zero."""
> —http://eel.is/c++draft/dcl.array
Interesting. You're not actually quoting the full sentence:
If the constant-expression is present, it shall be a converted
constant expression of type std::size_t and its value shall be
greater than zero.
This suggests that:
__u64 ptr[]
is actually valid since:
D1 [ constant-expressionopt ] attribute-specifier-seqopt
suggests that the part between the brackets is optional.
David
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Kirill Tkhai @ 2018-10-09 16:15 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel@vger.kernel.org
Cc: Eric Biederman, Dmitry Safonov, linux-api@vger.kernel.org,
James Bottomley, Alexander Viro, linux-fsdevel@vger.kernel.org,
Andrei Vagin (C), containers@lists.linux-foundation.org,
Jann Horn
In-Reply-To: <20181009103752.21482-2-laurent@vivier.eu>
On 09.10.2018 13:37, Laurent Vivier wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the previous level, but if the binfmt_misc filesystem is
> mounted in the new namespace a new empty binfmt instance is created and
> used in this namespace.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
> include/linux/user_namespace.h | 13 ++++
> kernel/user.c | 13 ++++
> kernel/user_namespace.c | 3 +
> 4 files changed, 107 insertions(+), 28 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index aa4a7a23ff99..1e0029d097d9 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -38,9 +38,6 @@ enum {
> VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
> };
>
> -static LIST_HEAD(entries);
> -static int enabled = 1;
> -
> enum {Enabled, Magic};
> #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> #define MISC_FMT_OPEN_BINARY (1 << 30)
> @@ -60,10 +57,7 @@ typedef struct {
> struct file *interp_file;
> } Node;
>
> -static DEFINE_RWLOCK(entries_lock);
> static struct file_system_type bm_fs_type;
> -static struct vfsmount *bm_mnt;
> -static int entry_count;
>
> /*
> * Max length of the register string. Determined by:
> @@ -80,18 +74,32 @@ static int entry_count;
> */
> #define MAX_REGISTER_LENGTH 1920
>
> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> +{
> + struct binfmt_namespace *b_ns;
> +
> + while (ns) {
> + b_ns = READ_ONCE(ns->binfmt_ns);
> + if (b_ns)
> + return b_ns;
> + ns = ns->parent;
> + }
> + WARN_ON_ONCE(1);
> + return NULL;
> +}
> +
> /*
> * Check if we support the binfmt
> * if we do, return the node, else NULL
> * locking is done in load_misc_binary
> */
> -static Node *check_file(struct linux_binprm *bprm)
> +static Node *check_file(struct binfmt_namespace *ns, struct linux_binprm *bprm)
> {
> char *p = strrchr(bprm->interp, '.');
> struct list_head *l;
>
> /* Walk all the registered handlers. */
> - list_for_each(l, &entries) {
> + list_for_each(l, &ns->entries) {
> Node *e = list_entry(l, Node, list);
> char *s;
> int j;
> @@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> struct file *interp_file = NULL;
> int retval;
> int fd_binary = -1;
> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!ns->enabled)
> return retval;
>
> /* to keep locking time low, we copy the interpreter string */
> - read_lock(&entries_lock);
> - fmt = check_file(bprm);
> + read_lock(&ns->entries_lock);
> + fmt = check_file(ns, bprm);
> if (fmt)
> dget(fmt->dentry);
> - read_unlock(&entries_lock);
> + read_unlock(&ns->entries_lock);
> if (!fmt)
> return retval;
>
> @@ -609,19 +618,19 @@ static void bm_evict_inode(struct inode *inode)
> kfree(e);
> }
>
> -static void kill_node(Node *e)
> +static void kill_node(struct binfmt_namespace *ns, Node *e)
> {
> struct dentry *dentry;
>
> - write_lock(&entries_lock);
> + write_lock(&ns->entries_lock);
> list_del_init(&e->list);
> - write_unlock(&entries_lock);
> + write_unlock(&ns->entries_lock);
>
> dentry = e->dentry;
> drop_nlink(d_inode(dentry));
> d_drop(dentry);
> dput(dentry);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt, &ns->entry_count);
> }
>
> /* /<entry> */
> @@ -651,6 +660,9 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> struct dentry *root;
> Node *e = file_inode(file)->i_private;
> int res = parse_command(buffer, count);
> + struct binfmt_namespace *ns;
> +
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
>
> switch (res) {
> case 1:
> @@ -667,7 +679,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> inode_lock(d_inode(root));
>
> if (!list_empty(&e->list))
> - kill_node(e);
> + kill_node(ns, e);
>
> inode_unlock(d_inode(root));
> break;
> @@ -693,6 +705,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> struct inode *inode;
> struct super_block *sb = file_inode(file)->i_sb;
> struct dentry *root = sb->s_root, *dentry;
> + struct binfmt_namespace *ns;
> int err = 0;
>
> e = create_entry(buffer, count);
> @@ -716,7 +729,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> + &ns->entry_count);
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -725,12 +740,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>
> if (e->flags & MISC_FMT_OPEN_FILE) {
> struct file *f;
> + const struct cred *old_cred;
>
> + old_cred = override_creds(file->f_cred);
> f = open_exec(e->interpreter);
> + revert_creds(old_cred);
> if (IS_ERR(f)) {
> err = PTR_ERR(f);
> pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt,
> + &ns->entry_count);
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +762,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> inode->i_fop = &bm_entry_operations;
>
> d_instantiate(dentry, inode);
> - write_lock(&entries_lock);
> - list_add(&e->list, &entries);
> - write_unlock(&entries_lock);
> + write_lock(&ns->entries_lock);
> + list_add(&e->list, &ns->entries);
> + write_unlock(&ns->entries_lock);
>
> err = 0;
> out2:
> @@ -770,7 +789,9 @@ static const struct file_operations bm_register_operations = {
> static ssize_t
> bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> {
> - char *s = enabled ? "enabled\n" : "disabled\n";
> + struct binfmt_namespace *ns =
> + binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + char *s = ns->enabled ? "enabled\n" : "disabled\n";
>
> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
> }
> @@ -778,25 +799,28 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> static ssize_t bm_status_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> + struct binfmt_namespace *ns;
> int res = parse_command(buffer, count);
> struct dentry *root;
>
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> switch (res) {
> case 1:
> /* Disable all handlers. */
> - enabled = 0;
> + ns->enabled = 0;
> break;
> case 2:
> /* Enable all handlers. */
> - enabled = 1;
> + ns->enabled = 1;
> break;
> case 3:
> /* Delete all handlers. */
> root = file_inode(file)->i_sb->s_root;
> inode_lock(d_inode(root));
>
> - while (!list_empty(&entries))
> - kill_node(list_first_entry(&entries, Node, list));
> + while (!list_empty(&ns->entries))
> + kill_node(ns, list_first_entry(&ns->entries,
> + Node, list));
>
> inode_unlock(d_inode(root));
> break;
> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
> static int bm_fill_super(struct super_block *sb, void *data, int silent)
> {
> int err;
> + struct user_namespace *ns = sb->s_user_ns;
> static const struct tree_descr bm_files[] = {
> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
> [3] = {"register", &bm_register_operations, S_IWUSR},
> /* last one */ {""}
> };
>
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + /* ensure new_ns is completely initialized before sharing it */
> + smp_wmb();
(I haven't dived into patch logic, here just small barrier remark from quick sight).
smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
you want something like below in read hunk:
+ b_ns = READ_ONCE(ns->binfmt_ns);
+ if (b_ns) {
+ smp_rmb();
+ return b_ns;
+ }
> + WRITE_ONCE(ns->binfmt_ns, new_ns);
> + }
> +
> err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> if (!err)
> sb->s_op = &s_ops;
> @@ -838,7 +884,10 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> - return mount_single(fs_type, flags, data, bm_fill_super);
> + struct user_namespace *ns = current_user_ns();
> +
> + return mount_ns(fs_type, flags, data, ns, ns,
> + bm_fill_super);
> }
>
> static struct linux_binfmt misc_format = {
> @@ -849,6 +898,7 @@ static struct linux_binfmt misc_format = {
> static struct file_system_type bm_fs_type = {
> .owner = THIS_MODULE,
> .name = "binfmt_misc",
> + .fs_flags = FS_USERNS_MOUNT,
> .mount = bm_mount,
> .kill_sb = kill_litter_super,
> };
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index d6b74b91096b..5c6e7e63b97e 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -52,6 +52,16 @@ enum ucount_type {
> UCOUNT_COUNTS,
> };
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +struct binfmt_namespace {
> + struct list_head entries;
> + rwlock_t entries_lock;
> + int enabled;
> + struct vfsmount *bm_mnt;
> + int entry_count;
> +} __randomize_layout;
> +#endif
> +
> struct user_namespace {
> struct uid_gid_map uid_map;
> struct uid_gid_map gid_map;
> @@ -76,6 +86,9 @@ struct user_namespace {
> #endif
> struct ucounts *ucounts;
> int ucount_max[UCOUNT_COUNTS];
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + struct binfmt_namespace *binfmt_ns;
> +#endif
> } __randomize_layout;
>
> struct ucounts {
> diff --git a/kernel/user.c b/kernel/user.c
> index 0df9b1640b2a..912916d435aa 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -19,6 +19,16 @@
> #include <linux/user_namespace.h>
> #include <linux/proc_ns.h>
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +static struct binfmt_namespace init_binfmt_ns = {
> + .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
> + .enabled = 1,
> + .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
> + .bm_mnt = NULL,
> + .entry_count = 0,
> +};
> +#endif
> +
> /*
> * userns count is 1 for root user, 1 for init_uts_ns,
> * and 1 for... ?
> @@ -66,6 +76,9 @@ struct user_namespace init_user_ns = {
> .persistent_keyring_register_sem =
> __RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
> #endif
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + .binfmt_ns = &init_binfmt_ns,
> +#endif
> };
> EXPORT_SYMBOL_GPL(init_user_ns);
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index e5222b5fb4fe..990cf5950a89 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -195,6 +195,9 @@ static void free_user_ns(struct work_struct *work)
> kfree(ns->projid_map.forward);
> kfree(ns->projid_map.reverse);
> }
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + kfree(ns->binfmt_ns);
> +#endif
> retire_userns_sysctls(ns);
> #ifdef CONFIG_PERSISTENT_KEYRINGS
> key_put(ns->persistent_keyring_register);
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox