* [PATCH 24/24] devpts: handle fsid mappings
From: Christian Brauner @ 2020-02-11 16:57 UTC (permalink / raw)
To: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn
Cc: smbarber, Alexander Viro, Alexey Dobriyan, Serge Hallyn,
James Morris, Kees Cook, Jonathan Corbet, linux-kernel,
linux-fsdevel, containers, linux-security-module, linux-api,
Christian Brauner
In-Reply-To: <20200211165753.356508-1-christian.brauner@ubuntu.com>
When a uid or gid mount option is specified with devpts have it lookup the
corresponding kfsids in the fsid mappings. If no fsid mappings are setup the
behavior is unchanged, i.e. fsids are looked up in the id mappings.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
fs/devpts/inode.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 42e5a766d33c..139958892572 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -24,6 +24,7 @@
#include <linux/parser.h>
#include <linux/fsnotify.h>
#include <linux/seq_file.h>
+#include <linux/fsuidgid.h>
#define DEVPTS_DEFAULT_MODE 0600
/*
@@ -277,7 +278,7 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
case Opt_uid:
if (match_int(&args[0], &option))
return -EINVAL;
- uid = make_kuid(current_user_ns(), option);
+ uid = make_kfsuid(current_user_ns(), option);
if (!uid_valid(uid))
return -EINVAL;
opts->uid = uid;
@@ -286,7 +287,7 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
case Opt_gid:
if (match_int(&args[0], &option))
return -EINVAL;
- gid = make_kgid(current_user_ns(), option);
+ gid = make_kfsgid(current_user_ns(), option);
if (!gid_valid(gid))
return -EINVAL;
opts->gid = gid;
@@ -410,7 +411,7 @@ static int devpts_show_options(struct seq_file *seq, struct dentry *root)
from_kuid_munged(&init_user_ns, opts->uid));
if (opts->setgid)
seq_printf(seq, ",gid=%u",
- from_kgid_munged(&init_user_ns, opts->gid));
+ from_kfsgid_munged(&init_user_ns, opts->gid));
seq_printf(seq, ",mode=%03o", opts->mode);
seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
if (opts->max < NR_UNIX98_PTY_MAX)
--
2.25.0
^ permalink raw reply related
* [PATCH 09/24] capability: privileged_wrt_inode_uidgid(): handle fsid mappings
From: Christian Brauner @ 2020-02-11 16:57 UTC (permalink / raw)
To: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn
Cc: smbarber, Alexander Viro, Alexey Dobriyan, Serge Hallyn,
James Morris, Kees Cook, Jonathan Corbet, linux-kernel,
linux-fsdevel, containers, linux-security-module, linux-api,
Christian Brauner
In-Reply-To: <20200211165753.356508-1-christian.brauner@ubuntu.com>
Switch privileged_wrt_inode_uidgid() to lookup fsids in the fsid mappings. If
no fsid mappings are setup the behavior is unchanged, i.e. fsids are looked up
in the id mappings.
Filesystems that share a superblock in all user namespaces they are mounted in
will retain their old semantics even with the introduction of fsidmappings.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
kernel/capability.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/kernel/capability.c b/kernel/capability.c
index 1444f3954d75..de7edd5c9900 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -19,6 +19,8 @@
#include <linux/pid_namespace.h>
#include <linux/user_namespace.h>
#include <linux/uaccess.h>
+#include <linux/fsuidgid.h>
+#include <linux/fs.h>
/*
* Leveraged for setting/resetting capabilities
@@ -484,10 +486,15 @@ EXPORT_SYMBOL(file_ns_capable);
*
* Return true if the inode uid and gid are within the namespace.
*/
-bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct inode *inode)
+bool privileged_wrt_inode_uidgid(struct user_namespace *ns,
+ const struct inode *inode)
{
- return kuid_has_mapping(ns, inode->i_uid) &&
- kgid_has_mapping(ns, inode->i_gid);
+ if (is_userns_visible(inode->i_sb->s_iflags))
+ return kuid_has_mapping(ns, inode->i_uid) &&
+ kgid_has_mapping(ns, inode->i_gid);
+
+ return kfsuid_has_mapping(ns, inode->i_uid) &&
+ kfsgid_has_mapping(ns, inode->i_gid);
}
/**
--
2.25.0
^ permalink raw reply related
* [PATCH 22/24] sys:__sys_setresuid(): handle fsid mappings
From: Christian Brauner @ 2020-02-11 16:57 UTC (permalink / raw)
To: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn
Cc: smbarber, Alexander Viro, Alexey Dobriyan, Serge Hallyn,
James Morris, Kees Cook, Jonathan Corbet, linux-kernel,
linux-fsdevel, containers, linux-security-module, linux-api,
Christian Brauner
In-Reply-To: <20200211165753.356508-1-christian.brauner@ubuntu.com>
Switch setresuid() to lookup fsids in the fsid mappings. If no fsid mappings
are setup the behavior is unchanged, i.e. fsids are looked up in the id
mappings.
During setresuid() the kfsuid is set to the keuid corresponding the euid that is
requested by userspace. If the requested euid is -1 the kfsuid is reset to the
current keuid. For the latter case this means we need to lookup the
corresponding userspace euid corresponding to the current keuid in the id
mappings and translate this euid into the corresponding kfsuid in the fsid
mappings.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
kernel/sys.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 41551c01c3eb..3b98ce84607d 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -650,11 +650,12 @@ long __sys_setresuid(uid_t ruid, uid_t euid, uid_t suid)
const struct cred *old;
struct cred *new;
int retval;
- kuid_t kruid, keuid, ksuid;
+ kuid_t kruid, keuid, ksuid, kfsuid;
kruid = make_kuid(ns, ruid);
keuid = make_kuid(ns, euid);
ksuid = make_kuid(ns, suid);
+ kfsuid = make_kfsuid(ns, euid);
if ((ruid != (uid_t) -1) && !uid_valid(kruid))
return -EINVAL;
@@ -665,6 +666,9 @@ long __sys_setresuid(uid_t ruid, uid_t euid, uid_t suid)
if ((suid != (uid_t) -1) && !uid_valid(ksuid))
return -EINVAL;
+ if ((euid != (uid_t) -1) && !uid_valid(kfsuid))
+ return -EINVAL;
+
new = prepare_creds();
if (!new)
return -ENOMEM;
@@ -692,11 +696,15 @@ long __sys_setresuid(uid_t ruid, uid_t euid, uid_t suid)
goto error;
}
}
- if (euid != (uid_t) -1)
+ if (euid != (uid_t) -1) {
new->euid = keuid;
+ } else {
+ uid_t fsuid = from_kuid_munged(new->user_ns, new->euid);
+ kfsuid = make_kfsuid(ns, fsuid);
+ }
if (suid != (uid_t) -1)
new->suid = ksuid;
- new->fsuid = new->euid;
+ new->fsuid = kfsuid;
retval = security_task_fix_setuid(new, old, LSM_SETID_RES);
if (retval < 0)
--
2.25.0
^ permalink raw reply related
* [PATCH 04/24] fsuidgid: add fsid mapping helpers
From: Christian Brauner @ 2020-02-11 16:57 UTC (permalink / raw)
To: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn
Cc: smbarber, Alexander Viro, Alexey Dobriyan, Serge Hallyn,
James Morris, Kees Cook, Jonathan Corbet, linux-kernel,
linux-fsdevel, containers, linux-security-module, linux-api,
Christian Brauner
In-Reply-To: <20200211165753.356508-1-christian.brauner@ubuntu.com>
This adds a set of helpers to translate between kfsuid/kfsgid and their
userspace fsuid/fsgid counter parts relative to a given user namespace.
- kuid_t make_kfsuid(struct user_namespace *from, uid_t fsuid)
Maps a user-namespace fsuid pair into a kfsuid.
If no fsuid mappings have been written it behaves identical to calling
make_kuid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
- kgid_t make_kfsgid(struct user_namespace *from, gid_t fsgid)
Maps a user-namespace fsgid pair into a kfsgid.
If no fsgid mappings have been written it behaves identical to calling
make_kgid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
- uid_t from_kfsuid(struct user_namespace *to, kuid_t fsuid)
Creates a fsuid from a kfsuid user-namespace pair if possible.
If no fsuid mappings have been written it behaves identical to calling
from_kuid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
- gid_t from_kfsgid(struct user_namespace *to, kgid_t fsgid)
Creates a fsgid from a kfsgid user-namespace pair if possible.
If no fsgid mappings have been written it behaves identical to calling
make_kgid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
- uid_t from_kfsuid_munged(struct user_namespace *to, kuid_t fsuid)
Always creates a fsuid from a kfsuid user-namespace pair.
If no fsuid mappings have been written it behaves identical to calling
from_kuid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
- gid_t from_kfsgid_munged(struct user_namespace *to, kgid_t fsgid)
Always creates a fsgid from a kfsgid user-namespace pair if possible.
If no fsgid mappings have been written it behaves identical to calling
make_kgid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
- bool kfsuid_has_mapping(struct user_namespace *ns, kuid_t uid)
Check whether this kfsuid has a mapping in the provided user namespace.
If no fsuid mappings have been written it behaves identical to calling
from_kuid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
- bool kfsgid_has_mapping(struct user_namespace *ns, kgid_t gid)
Check whether this kfsgid has a mapping in the provided user namespace.
If no fsgid mappings have been written it behaves identical to calling
make_kgid(). This ensures backwards compatibility for workloads unaware
or not in need of fsid mappings.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
include/linux/fsuidgid.h | 70 +++++++++++++++
kernel/user_namespace.c | 189 ++++++++++++++++++++++++++++++++++++---
2 files changed, 246 insertions(+), 13 deletions(-)
create mode 100644 include/linux/fsuidgid.h
diff --git a/include/linux/fsuidgid.h b/include/linux/fsuidgid.h
new file mode 100644
index 000000000000..0ebfdaa796ab
--- /dev/null
+++ b/include/linux/fsuidgid.h
@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FSUIDGID_H
+#define _LINUX_FSUIDGID_H
+
+#include <linux/uidgid.h>
+
+#ifdef CONFIG_USER_NS_FSID
+
+extern kuid_t make_kfsuid(struct user_namespace *from, uid_t fsuid);
+extern kgid_t make_kfsgid(struct user_namespace *from, gid_t fsgid);
+extern uid_t from_kfsuid(struct user_namespace *to, kuid_t kfsuid);
+extern gid_t from_kfsgid(struct user_namespace *to, kgid_t kfsgid);
+extern uid_t from_kfsuid_munged(struct user_namespace *to, kuid_t kfsuid);
+extern gid_t from_kfsgid_munged(struct user_namespace *to, kgid_t kfsgid);
+
+static inline bool kfsuid_has_mapping(struct user_namespace *ns, kuid_t kfsuid)
+{
+ return from_kfsuid(ns, kfsuid) != (uid_t) -1;
+}
+
+static inline bool kfsgid_has_mapping(struct user_namespace *ns, kgid_t kfsgid)
+{
+ return from_kfsgid(ns, kfsgid) != (gid_t) -1;
+}
+
+#else
+
+static inline kuid_t make_kfsuid(struct user_namespace *from, uid_t fsuid)
+{
+ return make_kuid(from, fsuid);
+}
+
+static inline kgid_t make_kfsgid(struct user_namespace *from, gid_t fsgid)
+{
+ return make_kgid(from, fsgid);
+}
+
+static inline uid_t from_kfsuid(struct user_namespace *to, kuid_t kfsuid)
+{
+ return from_kuid(to, kfsuid);
+}
+
+static inline gid_t from_kfsgid(struct user_namespace *to, kgid_t kfsgid)
+{
+ return from_kgid(to, kfsgid);
+}
+
+static inline uid_t from_kfsuid_munged(struct user_namespace *to, kuid_t kfsuid)
+{
+ return from_kuid_munged(to, kfsuid);
+}
+
+static inline gid_t from_kfsgid_munged(struct user_namespace *to, kgid_t kfsgid)
+{
+ return from_kgid_munged(to, kfsgid);
+}
+
+static inline bool kfsuid_has_mapping(struct user_namespace *ns, kuid_t kfsuid)
+{
+ return kuid_has_mapping(ns, kfsuid);
+}
+
+static inline bool kfsgid_has_mapping(struct user_namespace *ns, kgid_t kfsgid)
+{
+ return kgid_has_mapping(ns, kfsgid);
+}
+
+#endif /* CONFIG_USER_NS_FSID */
+
+#endif /* _LINUX_FSUIDGID_H */
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index cbdf456f95f0..398be02de5c3 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -20,13 +20,14 @@
#include <linux/fs_struct.h>
#include <linux/bsearch.h>
#include <linux/sort.h>
+#include <linux/fsuidgid.h>
static struct kmem_cache *user_ns_cachep __read_mostly;
static DEFINE_MUTEX(userns_state_mutex);
static bool new_idmap_permitted(const struct file *file,
struct user_namespace *ns, int cap_setid,
- struct uid_gid_map *map);
+ struct uid_gid_map *map, bool map_fsid);
static void free_user_ns(struct work_struct *work);
static struct ucounts *inc_user_namespaces(struct user_namespace *ns, kuid_t uid)
@@ -583,6 +584,166 @@ projid_t from_kprojid_munged(struct user_namespace *targ, kprojid_t kprojid)
}
EXPORT_SYMBOL(from_kprojid_munged);
+#ifdef CONFIG_USER_NS_FSID
+/**
+ * make_kfsuid - Map a user-namespace fsuid pair into a kuid.
+ * @ns: User namespace that the fsuid is in
+ * @fsuid: User identifier
+ *
+ * Maps a user-namespace fsuid pair into a kernel internal kfsuid,
+ * and returns that kfsuid.
+ *
+ * When there is no mapping defined for the user-namespace kfsuid
+ * pair INVALID_UID is returned. Callers are expected to test
+ * for and handle INVALID_UID being returned. INVALID_UID
+ * may be tested for using uid_valid().
+ */
+kuid_t make_kfsuid(struct user_namespace *ns, uid_t fsuid)
+{
+ unsigned extents = ns->fsuid_map.nr_extents;
+ smp_rmb();
+
+ /* Map the fsuid to a global kernel fsuid */
+ if (extents == 0)
+ return KUIDT_INIT(map_id_down(&ns->uid_map, fsuid));
+
+ return KUIDT_INIT(map_id_down(&ns->fsuid_map, fsuid));
+}
+EXPORT_SYMBOL(make_kfsuid);
+
+/**
+ * from_kfsuid - Create a fsuid from a kfsuid user-namespace pair.
+ * @targ: The user namespace we want a fsuid in.
+ * @kfsuid: The kernel internal fsuid to start with.
+ *
+ * Map @kfsuid into the user-namespace specified by @targ and
+ * return the resulting fsuid.
+ *
+ * There is always a mapping into the initial user_namespace.
+ *
+ * If @kfsuid has no mapping in @targ (uid_t)-1 is returned.
+ */
+uid_t from_kfsuid(struct user_namespace *targ, kuid_t kfsuid)
+{
+ unsigned extents = targ->fsuid_map.nr_extents;
+ smp_rmb();
+
+ /* Map the fsuid from a global kernel fsuid */
+ if (extents == 0)
+ return map_id_up(&targ->uid_map, __kuid_val(kfsuid));
+
+ return map_id_up(&targ->fsuid_map, __kuid_val(kfsuid));
+}
+EXPORT_SYMBOL(from_kfsuid);
+
+/**
+ * from_kfsuid_munged - Create a fsuid from a kfsuid user-namespace pair.
+ * @targ: The user namespace we want a fsuid in.
+ * @kfsuid: The kernel internal fsuid to start with.
+ *
+ * Map @kfsuid into the user-namespace specified by @targ and
+ * return the resulting fsuid.
+ *
+ * There is always a mapping into the initial user_namespace.
+ *
+ * Unlike from_kfsuid from_kfsuid_munged never fails and always
+ * returns a valid fsuid. This makes from_kfsuid_munged appropriate
+ * for use in syscalls like stat and getuid where failing the
+ * system call and failing to provide a valid fsuid are not an
+ * options.
+ *
+ * If @kfsuid has no mapping in @targ overflowuid is returned.
+ */
+uid_t from_kfsuid_munged(struct user_namespace *targ, kuid_t kfsuid)
+{
+ uid_t fsuid;
+ fsuid = from_kfsuid(targ, kfsuid);
+
+ if (fsuid == (uid_t) -1)
+ fsuid = overflowuid;
+ return fsuid;
+}
+EXPORT_SYMBOL(from_kfsuid_munged);
+
+/**
+ * make_kfsgid - Map a user-namespace fsgid pair into a kfsgid.
+ * @ns: User namespace that the fsgid is in
+ * @fsgid: User identifier
+ *
+ * Maps a user-namespace fsgid pair into a kernel internal kfsgid,
+ * and returns that kfsgid.
+ *
+ * When there is no mapping defined for the user-namespace fsgid
+ * pair INVALID_GID is returned. Callers are expected to test
+ * for and handle INVALID_GID being returned. INVALID_GID
+ * may be tested for using gid_valid().
+ */
+kgid_t make_kfsgid(struct user_namespace *ns, gid_t fsgid)
+{
+ unsigned extents = ns->fsgid_map.nr_extents;
+ smp_rmb();
+
+ /* Map the fsgid to a global kernel fsgid */
+ if (extents == 0)
+ return KGIDT_INIT(map_id_down(&ns->gid_map, fsgid));
+
+ return KGIDT_INIT(map_id_down(&ns->fsgid_map, fsgid));
+}
+EXPORT_SYMBOL(make_kfsgid);
+
+/**
+ * from_kfsgid - Create a fsgid from a kfsgid user-namespace pair.
+ * @targ: The user namespace we want a fsgid in.
+ * @kfsgid: The kernel internal fsgid to start with.
+ *
+ * Map @kfsgid into the user-namespace specified by @targ and
+ * return the resulting fsgid.
+ *
+ * There is always a mapping into the initial user_namespace.
+ *
+ * If @kfsgid has no mapping in @targ (gid_t)-1 is returned.
+ */
+gid_t from_kfsgid(struct user_namespace *targ, kgid_t kfsgid)
+{
+ unsigned extents = targ->fsgid_map.nr_extents;
+ smp_rmb();
+
+ /* Map the fsgid from a global kernel fsgid */
+ if (extents == 0)
+ return map_id_up(&targ->gid_map, __kgid_val(kfsgid));
+
+ return map_id_up(&targ->fsgid_map, __kgid_val(kfsgid));
+}
+EXPORT_SYMBOL(from_kfsgid);
+
+/**
+ * from_kfsgid_munged - Create a fsgid from a kfsgid user-namespace pair.
+ * @targ: The user namespace we want a fsgid in.
+ * @kfsgid: The kernel internal fsgid to start with.
+ *
+ * Map @kfsgid into the user-namespace specified by @targ and
+ * return the resulting fsgid.
+ *
+ * There is always a mapping into the initial user_namespace.
+ *
+ * Unlike from_kfsgid from_kfsgid_munged never fails and always
+ * returns a valid fsgid. This makes from_kfsgid_munged appropriate
+ * for use in syscalls like stat and getgid where failing the
+ * system call and failing to provide a valid fsgid are not options.
+ *
+ * If @kfsgid has no mapping in @targ overflowgid is returned.
+ */
+gid_t from_kfsgid_munged(struct user_namespace *targ, kgid_t kfsgid)
+{
+ gid_t fsgid;
+ fsgid = from_kfsgid(targ, kfsgid);
+
+ if (fsgid == (gid_t) -1)
+ fsgid = overflowgid;
+ return fsgid;
+}
+EXPORT_SYMBOL(from_kfsgid_munged);
+#endif /* CONFIG_USER_NS_FSID */
static int uid_m_show(struct seq_file *seq, void *v)
{
@@ -659,7 +820,7 @@ static int fsuid_m_show(struct seq_file *seq, void *v)
if ((lower_ns == ns) && lower_ns->parent)
lower_ns = lower_ns->parent;
- lower = from_kuid(lower_ns, KUIDT_INIT(extent->lower_first));
+ lower = from_kfsuid(lower_ns, KUIDT_INIT(extent->lower_first));
seq_printf(seq, "%10u %10u %10u\n",
extent->first,
@@ -680,7 +841,7 @@ static int fsgid_m_show(struct seq_file *seq, void *v)
if ((lower_ns == ns) && lower_ns->parent)
lower_ns = lower_ns->parent;
- lower = from_kgid(lower_ns, KGIDT_INIT(extent->lower_first));
+ lower = from_kfsgid(lower_ns, KGIDT_INIT(extent->lower_first));
seq_printf(seq, "%10u %10u %10u\n",
extent->first,
@@ -931,7 +1092,7 @@ static ssize_t map_write(struct file *file, const char __user *buf,
size_t count, loff_t *ppos,
int cap_setid,
struct uid_gid_map *map,
- struct uid_gid_map *parent_map)
+ struct uid_gid_map *parent_map, bool map_fsid)
{
struct seq_file *seq = file->private_data;
struct user_namespace *ns = seq->private;
@@ -1051,7 +1212,7 @@ static ssize_t map_write(struct file *file, const char __user *buf,
ret = -EPERM;
/* Validate the user is allowed to use user id's mapped to. */
- if (!new_idmap_permitted(file, ns, cap_setid, &new_map))
+ if (!new_idmap_permitted(file, ns, cap_setid, &new_map, map_fsid))
goto out;
ret = -EPERM;
@@ -1129,7 +1290,7 @@ ssize_t proc_uid_map_write(struct file *file, const char __user *buf,
return -EPERM;
return map_write(file, buf, size, ppos, CAP_SETUID,
- &ns->uid_map, &ns->parent->uid_map);
+ &ns->uid_map, &ns->parent->uid_map, false);
}
ssize_t proc_gid_map_write(struct file *file, const char __user *buf,
@@ -1146,7 +1307,7 @@ ssize_t proc_gid_map_write(struct file *file, const char __user *buf,
return -EPERM;
return map_write(file, buf, size, ppos, CAP_SETGID,
- &ns->gid_map, &ns->parent->gid_map);
+ &ns->gid_map, &ns->parent->gid_map, false);
}
ssize_t proc_projid_map_write(struct file *file, const char __user *buf,
@@ -1164,7 +1325,7 @@ ssize_t proc_projid_map_write(struct file *file, const char __user *buf,
/* Anyone can set any valid project id no capability needed */
return map_write(file, buf, size, ppos, -1,
- &ns->projid_map, &ns->parent->projid_map);
+ &ns->projid_map, &ns->parent->projid_map, false);
}
#ifdef CONFIG_USER_NS_FSID
@@ -1182,7 +1343,7 @@ ssize_t proc_fsuid_map_write(struct file *file, const char __user *buf,
return -EPERM;
return map_write(file, buf, size, ppos, CAP_SETUID, &ns->fsuid_map,
- &ns->parent->fsuid_map);
+ &ns->parent->fsuid_map, true);
}
ssize_t proc_fsgid_map_write(struct file *file, const char __user *buf,
@@ -1199,13 +1360,13 @@ ssize_t proc_fsgid_map_write(struct file *file, const char __user *buf,
return -EPERM;
return map_write(file, buf, size, ppos, CAP_SETGID, &ns->fsgid_map,
- &ns->parent->fsgid_map);
+ &ns->parent->fsgid_map, true);
}
#endif
static bool new_idmap_permitted(const struct file *file,
struct user_namespace *ns, int cap_setid,
- struct uid_gid_map *new_map)
+ struct uid_gid_map *new_map, bool map_fsid)
{
const struct cred *cred = file->f_cred;
/* Don't allow mappings that would allow anything that wouldn't
@@ -1215,11 +1376,13 @@ static bool new_idmap_permitted(const struct file *file,
uid_eq(ns->owner, cred->euid)) {
u32 id = new_map->extent[0].lower_first;
if (cap_setid == CAP_SETUID) {
- kuid_t uid = make_kuid(ns->parent, id);
+ kuid_t uid = map_fsid ? make_kfsuid(ns->parent, id) :
+ make_kuid(ns->parent, id);
if (uid_eq(uid, cred->euid))
return true;
} else if (cap_setid == CAP_SETGID) {
- kgid_t gid = make_kgid(ns->parent, id);
+ kgid_t gid = map_fsid ? make_kfsgid(ns->parent, id) :
+ make_kgid(ns->parent, id);
if (!(ns->flags & USERNS_SETGROUPS_ALLOWED) &&
gid_eq(gid, cred->egid))
return true;
--
2.25.0
^ permalink raw reply related
* [PATCH 23/24] sys:__sys_setresgid(): handle fsid mappings
From: Christian Brauner @ 2020-02-11 16:57 UTC (permalink / raw)
To: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn
Cc: smbarber, Alexander Viro, Alexey Dobriyan, Serge Hallyn,
James Morris, Kees Cook, Jonathan Corbet, linux-kernel,
linux-fsdevel, containers, linux-security-module, linux-api,
Christian Brauner
In-Reply-To: <20200211165753.356508-1-christian.brauner@ubuntu.com>
Switch setresgid() to lookup fsids in the fsid mappings. If no fsid mappings are
setup the behavior is unchanged, i.e. fsids are looked up in the id mappings.
During setresgid() the kfsgid is set to the kegid corresponding the egid that is
requested by userspace. If the requested egid is -1 the kfsgid is reset to the
current kegid. For the latter case this means we need to lookup the
corresponding userspace egid corresponding to the current kegid in the id
mappings and translate this egid into the corresponding kfsgid in the fsid
mappings.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
kernel/sys.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 3b98ce84607d..674d0ba4887c 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -750,11 +750,12 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
const struct cred *old;
struct cred *new;
int retval;
- kgid_t krgid, kegid, ksgid;
+ kgid_t krgid, kegid, ksgid, kfsgid;
krgid = make_kgid(ns, rgid);
kegid = make_kgid(ns, egid);
ksgid = make_kgid(ns, sgid);
+ kfsgid = make_kfsgid(ns, egid);
if ((rgid != (gid_t) -1) && !gid_valid(krgid))
return -EINVAL;
@@ -762,6 +763,8 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
return -EINVAL;
if ((sgid != (gid_t) -1) && !gid_valid(ksgid))
return -EINVAL;
+ if ((egid != (gid_t) -1) && !gid_valid(kfsgid))
+ return -EINVAL;
new = prepare_creds();
if (!new)
@@ -783,11 +786,15 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
if (rgid != (gid_t) -1)
new->gid = krgid;
- if (egid != (gid_t) -1)
+ if (egid != (gid_t) -1) {
new->egid = kegid;
+ } else {
+ gid_t fsgid = from_kgid_munged(new->user_ns, new->egid);
+ kfsgid = make_kfsgid(ns, fsgid);
+ }
if (sgid != (gid_t) -1)
new->sgid = ksgid;
- new->fsgid = new->egid;
+ new->fsgid = kfsgid;
return commit_creds(new);
--
2.25.0
^ permalink raw reply related
* [PATCH 21/24] sys:__sys_setregid(): handle fsid mappings
From: Christian Brauner @ 2020-02-11 16:57 UTC (permalink / raw)
To: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn
Cc: smbarber, Alexander Viro, Alexey Dobriyan, Serge Hallyn,
James Morris, Kees Cook, Jonathan Corbet, linux-kernel,
linux-fsdevel, containers, linux-security-module, linux-api,
Christian Brauner
In-Reply-To: <20200211165753.356508-1-christian.brauner@ubuntu.com>
Switch setregid() to lookup fsids in the fsid mappings. If no fsid mappings are
setup the behavior is unchanged, i.e. fsids are looked up in the id mappings.
During setregid() the kfsgid is set to the kegid corresponding the egid that is
requested by userspace. If the requested egid is -1 the kfsgid is reset to the
current kegid. For the latter case this means we need to lookup the
corresponding userspace egid corresponding to the current kegid in the id
mappings and translate this egid into the corresponding kfsgid in the fsid
mappings.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
kernel/sys.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index ef1104c9df56..41551c01c3eb 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -354,15 +354,18 @@ long __sys_setregid(gid_t rgid, gid_t egid)
const struct cred *old;
struct cred *new;
int retval;
- kgid_t krgid, kegid;
+ kgid_t krgid, kegid, kfsgid;
krgid = make_kgid(ns, rgid);
kegid = make_kgid(ns, egid);
+ kfsgid = make_kfsgid(ns, egid);
if ((rgid != (gid_t) -1) && !gid_valid(krgid))
return -EINVAL;
if ((egid != (gid_t) -1) && !gid_valid(kegid))
return -EINVAL;
+ if ((egid != (gid_t) -1) && !gid_valid(kfsgid))
+ return -EINVAL;
new = prepare_creds();
if (!new)
@@ -386,12 +389,15 @@ long __sys_setregid(gid_t rgid, gid_t egid)
new->egid = kegid;
else
goto error;
+ } else {
+ gid_t fsgid = from_kgid_munged(new->user_ns, new->egid);
+ kfsgid = make_kfsgid(ns, fsgid);
}
if (rgid != (gid_t) -1 ||
(egid != (gid_t) -1 && !gid_eq(kegid, old->gid)))
new->sgid = new->egid;
- new->fsgid = new->egid;
+ new->fsgid = kfsgid;
return commit_creds(new);
--
2.25.0
^ permalink raw reply related
* Re: [PATCH 01/24] user_namespace: introduce fsid mappings infrastructure
From: Randy Dunlap @ 2020-02-11 17:26 UTC (permalink / raw)
To: Christian Brauner, Stéphane Graber, Eric W. Biederman,
Aleksa Sarai, Jann Horn
Cc: smbarber, Alexander Viro, Alexey Dobriyan, Serge Hallyn,
James Morris, Kees Cook, Jonathan Corbet, linux-kernel,
linux-fsdevel, containers, linux-security-module, linux-api
In-Reply-To: <20200211165753.356508-2-christian.brauner@ubuntu.com>
On 2/11/20 8:57 AM, Christian Brauner wrote:
> diff --git a/init/Kconfig b/init/Kconfig
> index a34064a031a5..4da082e4f787 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1102,6 +1102,17 @@ config USER_NS
>
> If unsure, say N.
>
> +config USER_NS_FSID
> + bool "User namespace fsid mappings"
> + depends on USER_NS
> + default n
> + help
> + This allows containers, to alter their filesystem id mappings.
no comma ^^^^
> + With this containers with different id mappings can still share
> + the same filesystem.
> +
> + If unsure, say N.
> +
> config PID_NS
> bool "PID Namespaces"
> default y
--
~Randy
^ permalink raw reply
* Re: [RFC PATCH 0/2] ima: uncompressed module appraisal support
From: Eric Snowberg @ 2020-02-11 17:33 UTC (permalink / raw)
To: Mimi Zohar
Cc: Nayna, dmitry.kasatkin, jmorris, serge, dhowells, geert, gregkh,
nayna, tglx, bauerman, mpe, linux-integrity,
linux-security-module, linux-kernel, Roberto Sassu
In-Reply-To: <1581366829.5585.898.camel@linux.ibm.com>
> On Feb 10, 2020, at 1:33 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2020-02-10 at 12:24 -0700, Eric Snowberg wrote:
>>> On Feb 10, 2020, at 10:09 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
>>>>
>>>> Ok, understood, “modsig” refers to strictly kernel module appended signatures
>>>> without regard to the keyring that verifies it. Since there are inconsistencies
>>>> here, would you consider something like my first patch? It will verify an
>>>> uncompressed kernel module containing an appended signature when the public key
>>>> is contained within the kernel keyring instead of the ima keyring. Why force a
>>>> person to add the same keys into the ima keyring for validation? Especially when
>>>> the kernel keyring is now used to verify appended signatures in the compressed
>>>> modules.
>>>
>>> Different use case scenarios have different requirements. Suppose for
>>> example that the group creating the kernel image is not the same as
>>> using it. The group using the kernel image could sign all files,
>>> including kernel modules (imasig), with their own private key. Only
>>> files that they signed would be permitted. Your proposal would break
>>> the current expectations, allowing kernel modules signed by someone
>>> else to be loaded.
>>>
>>
>> All the end user needs to do is compress any module created by the group that built
>> the original kernel image to work around the scenario above. Then the appended
>> signature in the compressed module will be verified by the kernel keyring. Does
>> this mean there is a security problem that should be fixed, if this is a concern?
>
> Again, the issue isn't compressed/uncompressed kernel modules, but the
> syscall used to load the kernel module. IMA can prevent using the the
> init_module syscall. Refer to the ima_load_data() LOADING_MODULE
> case.
Within the ima_load_data() LOADING_MODULE case, to prevent IMA from using
the init_module syscall, is_module_sig_enforced() must return false. Currently
when is_module_sig_enforced() returns true, the kernel keyring is always used
for verification.
What if I change this part of my patch from
+ if (rc && func == MODULE_CHECK)
to
+ sig_enforce = is_module_sig_enforced();
+ if (sig_enforce && rc && func == MODULE_CHECK)
Now when the init_module syscall is available, finit_module syscall will use
both the ima keyring and kernel keyring for verification. When the
init_module syscall is blocked from use, the finit_module syscall will only use
the ima keyring for validation. I believe this would satisfy both your use
case and mine.
^ permalink raw reply
* Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
From: Alexei Starovoitov @ 2020-02-11 17:58 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
kernel-team
In-Reply-To: <20200211124334.GA96694@google.com>
On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
>
> > Pros:
> > - no changes to security/ directory
>
> * We do need to initialize the BPF LSM as a proper LSM (i.e. in
> security/bpf) because it needs access to security blobs. This is
> only possible statically for now as they should be set after boot
> time to provide guarantees to any helper that uses information in
> security blobs. I have proposed how we could make this dynamic as a
> discussion topic for the BPF conference:
>
> https://lore.kernel.org/bpf/20200127171516.GA2710@chromium.org
>
> As you can see from some of the prototype use cases e.g:
>
> https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c
>
> that they do rely on security blobs and that they are key in doing
> meaningful security analysis.
above example doesn't use security blob from bpf prog.
Are you referring to
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/security/bpf/ops.c#L455
Then it's a bpf helper that is using it. And that helper could have been
implemented differently. I think it should be a separate discussion on merits
of such helper, its api, and its implementation.
At the same time I agree that additional scratch space accessible by lsm in
inode->i_security, cred->security and other kernel structs is certainly
necessary, but it's a nack to piggy back on legacy lsm way of doing it. The
implementation of bpf_lsm_blob_sizes.lbs_inode fits one single purpose. It's
fine for builtin LSM where blob sizes and code that uses it lives in one place
in the kernel and being modified at once when need for more space arises. For
bpf progs such approach is a non starter. Progs need to have flexible amount
scratch space. Thankfully this problem is not new. It was solved already.
Please see how bpf_sk_storage is implemented. It's a flexible amount of scratch
spaces available to bpf progs that is available in every socket. It's done on
demand. No space is wasted when progs are not using it. Not all sockets has to
have it either. I strongly believe that the same approach should be used for
scratch space in inode, cred, etc. It can be a union of existing 'void
*security' pointer or a new pointer. net/core/bpf_sk_storage.c implementation
is not socket specific. It can be generalized for inode, cred, task, etc.
> * When using the semantic provided by fexit, the BPF LSM program will
> always be executed and will be able to override / clobber the
> decision of LSMs which appear before it in the ordered list. This
> semantic is very different from what we currently have (i.e. the BPF
> LSM hook is only called if all the other LSMs allow the action) and
> seems to be bypassing the LSM framework.
It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
trampoline generator specific to lsm progs.
> * Not all security_* wrappers simply call the attached hooks and return
> their exit code and not all of them pass the same arguments to the
> hook e.g. security_bprm_check, security_file_open,
> security_task_alloc to just name a few. Illustrating this further
> using security_task_alloc as an example:
>
> rc = call_int_hook(task_alloc, 0, task, clone_flags);
> if (unlikely(rc))
> security_task_free(task);
> return rc;
>
> Which means we would leak task_structs in this case. While
> call_int_hook is sort of equivalent to the fexit trampoline for most
> hooks, it's not really the case for some (quite important) LSM hooks.
let's look at them one by one.
1.
security_bprm_check() calling ima_bprm_check.
I think it's layering violation.
Was it that hard to convince vfs folks to add it in fs/exec.c where
it belongs?
2.
security_file_open() calling fsnotify_perm().
Same layering violation and it's clearly broken.
When CONFIG_SECURITY is not defined:
static inline int security_file_open(struct file *file)
{
return 0;
}
There is no call to fsnotify_perm().
So fsnotify_open/mkdir/etc() work fine with and without CONFIG_SECURITY,
but fsnotify_perm() events can be lost depending on kconfig.
fsnotify_perm() should be moved in fs/open.c.
3.
security_task_alloc(). hmm.
when CONFIG_SECURITY is enabled and corresponding LSM with
non zero blob_sizes.lbs_task is registered that hook allocates
memory even if task_alloc is empty.
Doing security_task_free() in that hook also looks wrong.
It should have been:
diff --git a/kernel/fork.c b/kernel/fork.c
index ef82feb4bddc..a0d31e781341 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2062,7 +2062,7 @@ static __latent_entropy struct task_struct *copy_process(
shm_init_task(p);
retval = security_task_alloc(p, clone_flags);
if (retval)
- goto bad_fork_cleanup_audit;
+ goto bad_fork_cleanup_security;
Same issue with security_file_alloc().
I think this layering issues should be fixed, but it's not a blocker for
lsm-bpf to proceed. Using fexit mechanism and bpf_sk_storage generalization is
all that is needed. None of it should touch security/*.
^ permalink raw reply related
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-11 17:58 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler, Simon McVittie
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
keescook, penguin-kernel, paul
In-Reply-To: <af6b224c-4a32-40fa-77ae-1e2a5580054f@tycho.nsa.gov>
On 2/11/20 7:59 AM, Stephen Smalley wrote:
> On 2/10/20 2:00 PM, John Johansen wrote:
>> On 2/10/20 10:32 AM, Casey Schaufler wrote:
>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
>>> there is no need to exactly duplicate what is in attr/current (later
>>> SO_PEERSEC). I already plan to omit the "mode" component of the
>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would
>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
>>> have to do.
>>>
>>
>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.
>
> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings.
>
> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC. Is that correct?
>
> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT? It can certainly split it up per LSM. But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage? Or will the changes to the format described above create issues there?
>
>
libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC.
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-02-11 18:35 UTC (permalink / raw)
To: John Johansen, Stephen Smalley, Simon McVittie
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
keescook, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <d3dfd552-cab1-f50e-f207-b6308d0d5990@canonical.com>
On 2/11/2020 9:58 AM, John Johansen wrote:
> On 2/11/20 7:59 AM, Stephen Smalley wrote:
>> On 2/10/20 2:00 PM, John Johansen wrote:
>>> On 2/10/20 10:32 AM, Casey Schaufler wrote:
>>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
>>>> there is no need to exactly duplicate what is in attr/current (later
>>>> SO_PEERSEC). I already plan to omit the "mode" component of the
>>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would
>>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
>>>> have to do.
>>>>
>>>
>>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.
>>
>> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings.
>>
>> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC. Is that correct?
>>
>> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT?
There hasn't been any serious thought put into liblsm. To date
there hasn't been an LSM level interface to worry about except
for /sys/kernel/security/lsm. My notions of what a liblsm ought
provide would seem archaic to most of the people here. I can make
proposals if there's a notion that liblsm makes sense.
>> It can certainly split it up per LSM. But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage? Or will the changes to the format described above create issues there?
>>
>>
> libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC.
So it doesn't seem that there would be significant difficulties
switching from "current" to "context". It won't be transparent,
but we're providing "display" to address that.
^ permalink raw reply
* BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Jann Horn @ 2020-02-11 18:44 UTC (permalink / raw)
To: Alexei Starovoitov, KP Singh
Cc: kernel list, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Matthew Garrett,
Christian Brauner, Mickaël Salaün, Florent Revest,
Brendan Jackman, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Kernel Team
In-Reply-To: <20200211175825.szxaqaepqfbd2wmg@ast-mbp>
On Tue, Feb 11, 2020 at 6:58 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
[...]
> > * When using the semantic provided by fexit, the BPF LSM program will
> > always be executed and will be able to override / clobber the
> > decision of LSMs which appear before it in the ordered list. This
> > semantic is very different from what we currently have (i.e. the BPF
> > LSM hook is only called if all the other LSMs allow the action) and
> > seems to be bypassing the LSM framework.
>
> It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
> trampoline generator specific to lsm progs.
[...]
> Using fexit mechanism and bpf_sk_storage generalization is
> all that is needed. None of it should touch security/*.
If I understand your suggestion correctly, that seems like a terrible
idea to me from the perspective of inspectability and debuggability.
If at runtime, a function can branch off elsewhere to modify its
decision, I want to see that in the source code. If someone e.g.
changes the parameters or the locking rules around a security hook,
how are they supposed to understand the implications if that happens
through some magic fexit trampoline that is injected at runtime?
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Alexei Starovoitov @ 2020-02-11 19:09 UTC (permalink / raw)
To: Jann Horn
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com>
On Tue, Feb 11, 2020 at 07:44:05PM +0100, Jann Horn wrote:
> On Tue, Feb 11, 2020 at 6:58 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
> [...]
> > > * When using the semantic provided by fexit, the BPF LSM program will
> > > always be executed and will be able to override / clobber the
> > > decision of LSMs which appear before it in the ordered list. This
> > > semantic is very different from what we currently have (i.e. the BPF
> > > LSM hook is only called if all the other LSMs allow the action) and
> > > seems to be bypassing the LSM framework.
> >
> > It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
> > trampoline generator specific to lsm progs.
> [...]
> > Using fexit mechanism and bpf_sk_storage generalization is
> > all that is needed. None of it should touch security/*.
>
> If I understand your suggestion correctly, that seems like a terrible
> idea to me from the perspective of inspectability and debuggability.
> If at runtime, a function can branch off elsewhere to modify its
> decision, I want to see that in the source code. If someone e.g.
> changes the parameters or the locking rules around a security hook,
> how are they supposed to understand the implications if that happens
> through some magic fexit trampoline that is injected at runtime?
I'm not following the concern. There is error injection facility that is
heavily used with and without bpf. In this case there is really no difference
whether trampoline is used with direct call or indirect callback via function
pointer. Both will jump to bpf prog. The _source code_ of bpf program will
_always_ be available for humans to examine via "bpftool prog dump" since BTF
is required. So from inspectability and debuggability point of view lsm+bpf
stuff is way more visible than any builtin LSM. At any time people will be able
to see what exactly is running on the system. Assuming folks can read C code.
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-11 19:11 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, Simon McVittie
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
keescook, penguin-kernel, paul
In-Reply-To: <397af078-a44b-6fcd-e125-8fdb0b441097@schaufler-ca.com>
On 2/11/20 10:35 AM, Casey Schaufler wrote:
> On 2/11/2020 9:58 AM, John Johansen wrote:
>> On 2/11/20 7:59 AM, Stephen Smalley wrote:
>>> On 2/10/20 2:00 PM, John Johansen wrote:
>>>> On 2/10/20 10:32 AM, Casey Schaufler wrote:
>>>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
>>>>> there is no need to exactly duplicate what is in attr/current (later
>>>>> SO_PEERSEC). I already plan to omit the "mode" component of the
>>>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would
>>>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
>>>>> have to do.
>>>>>
>>>>
>>>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.
>>>
>>> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings.
>>>
>>> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC. Is that correct?
>>>
>>> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT?
>
> There hasn't been any serious thought put into liblsm. To date
> there hasn't been an LSM level interface to worry about except
> for /sys/kernel/security/lsm. My notions of what a liblsm ought
> provide would seem archaic to most of the people here. I can make
> proposals if there's a notion that liblsm makes sense.
>
>
>>> It can certainly split it up per LSM. But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage? Or will the changes to the format described above create issues there?
>>>
>>>
>> libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC.
>
> So it doesn't seem that there would be significant difficulties
> switching from "current" to "context". It won't be transparent,
> but we're providing "display" to address that.
>
>
right
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Jann Horn @ 2020-02-11 19:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <20200211190943.sysdbz2zuz5666nq@ast-mbp>
On Tue, Feb 11, 2020 at 8:09 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Tue, Feb 11, 2020 at 07:44:05PM +0100, Jann Horn wrote:
> > On Tue, Feb 11, 2020 at 6:58 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
> > [...]
> > > > * When using the semantic provided by fexit, the BPF LSM program will
> > > > always be executed and will be able to override / clobber the
> > > > decision of LSMs which appear before it in the ordered list. This
> > > > semantic is very different from what we currently have (i.e. the BPF
> > > > LSM hook is only called if all the other LSMs allow the action) and
> > > > seems to be bypassing the LSM framework.
> > >
> > > It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
> > > trampoline generator specific to lsm progs.
> > [...]
> > > Using fexit mechanism and bpf_sk_storage generalization is
> > > all that is needed. None of it should touch security/*.
> >
> > If I understand your suggestion correctly, that seems like a terrible
> > idea to me from the perspective of inspectability and debuggability.
> > If at runtime, a function can branch off elsewhere to modify its
> > decision, I want to see that in the source code. If someone e.g.
> > changes the parameters or the locking rules around a security hook,
> > how are they supposed to understand the implications if that happens
> > through some magic fexit trampoline that is injected at runtime?
>
> I'm not following the concern. There is error injection facility that is
> heavily used with and without bpf. In this case there is really no difference
> whether trampoline is used with direct call or indirect callback via function
> pointer. Both will jump to bpf prog. The _source code_ of bpf program will
> _always_ be available for humans to examine via "bpftool prog dump" since BTF
> is required. So from inspectability and debuggability point of view lsm+bpf
> stuff is way more visible than any builtin LSM. At any time people will be able
> to see what exactly is running on the system. Assuming folks can read C code.
You said that you want to use fexit without touching security/, which
AFAIU means that the branch from security_*() to the BPF LSM will be
invisible in the *kernel's* source code unless the reader already
knows about the BPF LSM. But maybe I'm just misunderstanding your
idea.
If a random developer is trying to change the locking rules around
security_blah(), and wants to e.g. figure out whether it's okay to
call that thing with a spinlock held, or whether one of the arguments
is actually used, or stuff like that, the obvious way to verify that
is to follow all the direct and indirect calls made from
security_blah(). It's tedious, but it works, unless something is
hooked up to it in a way that is visible in no way in the source code.
I agree that the way in which the call happens behind the scenes
doesn't matter all that much - I don't really care all that much
whether it's an indirect call, a runtime-patched direct call in inline
assembly, or an fexit hook. What I do care about is that someone
reading through any affected function can immediately see that the
branch exists - in other words, ideally, I'd like it to be something
happening in the method body, but if you think that's unacceptable, I
think there should at least be a function attribute that makes it very
clear what's going on.
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Alexei Starovoitov @ 2020-02-11 20:10 UTC (permalink / raw)
To: Jann Horn
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <CAG48ez2gvo1dA4P1L=ASz7TRfbH-cgLZLmOPmr0NweayL-efLw@mail.gmail.com>
On Tue, Feb 11, 2020 at 08:36:18PM +0100, Jann Horn wrote:
> On Tue, Feb 11, 2020 at 8:09 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Tue, Feb 11, 2020 at 07:44:05PM +0100, Jann Horn wrote:
> > > On Tue, Feb 11, 2020 at 6:58 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > > On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
> > > [...]
> > > > > * When using the semantic provided by fexit, the BPF LSM program will
> > > > > always be executed and will be able to override / clobber the
> > > > > decision of LSMs which appear before it in the ordered list. This
> > > > > semantic is very different from what we currently have (i.e. the BPF
> > > > > LSM hook is only called if all the other LSMs allow the action) and
> > > > > seems to be bypassing the LSM framework.
> > > >
> > > > It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
> > > > trampoline generator specific to lsm progs.
> > > [...]
> > > > Using fexit mechanism and bpf_sk_storage generalization is
> > > > all that is needed. None of it should touch security/*.
> > >
> > > If I understand your suggestion correctly, that seems like a terrible
> > > idea to me from the perspective of inspectability and debuggability.
> > > If at runtime, a function can branch off elsewhere to modify its
> > > decision, I want to see that in the source code. If someone e.g.
> > > changes the parameters or the locking rules around a security hook,
> > > how are they supposed to understand the implications if that happens
> > > through some magic fexit trampoline that is injected at runtime?
> >
> > I'm not following the concern. There is error injection facility that is
> > heavily used with and without bpf. In this case there is really no difference
> > whether trampoline is used with direct call or indirect callback via function
> > pointer. Both will jump to bpf prog. The _source code_ of bpf program will
> > _always_ be available for humans to examine via "bpftool prog dump" since BTF
> > is required. So from inspectability and debuggability point of view lsm+bpf
> > stuff is way more visible than any builtin LSM. At any time people will be able
> > to see what exactly is running on the system. Assuming folks can read C code.
>
> You said that you want to use fexit without touching security/, which
> AFAIU means that the branch from security_*() to the BPF LSM will be
> invisible in the *kernel's* source code unless the reader already
> knows about the BPF LSM. But maybe I'm just misunderstanding your
> idea.
>
> If a random developer is trying to change the locking rules around
> security_blah(), and wants to e.g. figure out whether it's okay to
> call that thing with a spinlock held, or whether one of the arguments
> is actually used, or stuff like that, the obvious way to verify that
> is to follow all the direct and indirect calls made from
> security_blah(). It's tedious, but it works, unless something is
> hooked up to it in a way that is visible in no way in the source code.
>
> I agree that the way in which the call happens behind the scenes
> doesn't matter all that much - I don't really care all that much
> whether it's an indirect call, a runtime-patched direct call in inline
> assembly, or an fexit hook. What I do care about is that someone
> reading through any affected function can immediately see that the
> branch exists - in other words, ideally, I'd like it to be something
> happening in the method body, but if you think that's unacceptable, I
> think there should at least be a function attribute that makes it very
> clear what's going on.
Got it. Then let's whitelist them ?
All error injection points are marked with ALLOW_ERROR_INJECTION().
We can do something similar here, but let's do it via BTF and avoid
abusing yet another elf section for this mark.
I think BTF_TYPE_EMIT() should work. Just need to pick explicit enough
name and extensive comment about what is going on.
Locking rules and cleanup around security_blah() shouldn't change though.
Like security_task_alloc() should be paired with security_task_free().
And so on. With bpf_sk_storage like logic the alloc/free of scratch
space will be similar to the way socket and bpf progs deal with it.
Some of the lsm hooks are in critical path. Like security_socket_sendmsg().
retpoline hurts. If we go with indirect calls right now it will be harder to
optimize later. It took us long time to come up with bpf trampoline and build
bpf dispatcher on top of it to remove single indirect call from XDP runtime.
For bpf+lsm would be good to avoid it from the start.
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Jann Horn @ 2020-02-11 20:33 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <20200211201039.om6xqoscfle7bguz@ast-mbp>
()On Tue, Feb 11, 2020 at 9:10 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Tue, Feb 11, 2020 at 08:36:18PM +0100, Jann Horn wrote:
> > On Tue, Feb 11, 2020 at 8:09 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > On Tue, Feb 11, 2020 at 07:44:05PM +0100, Jann Horn wrote:
> > > > On Tue, Feb 11, 2020 at 6:58 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
> > > > [...]
> > > > > > * When using the semantic provided by fexit, the BPF LSM program will
> > > > > > always be executed and will be able to override / clobber the
> > > > > > decision of LSMs which appear before it in the ordered list. This
> > > > > > semantic is very different from what we currently have (i.e. the BPF
> > > > > > LSM hook is only called if all the other LSMs allow the action) and
> > > > > > seems to be bypassing the LSM framework.
> > > > >
> > > > > It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
> > > > > trampoline generator specific to lsm progs.
> > > > [...]
> > > > > Using fexit mechanism and bpf_sk_storage generalization is
> > > > > all that is needed. None of it should touch security/*.
> > > >
> > > > If I understand your suggestion correctly, that seems like a terrible
> > > > idea to me from the perspective of inspectability and debuggability.
> > > > If at runtime, a function can branch off elsewhere to modify its
> > > > decision, I want to see that in the source code. If someone e.g.
> > > > changes the parameters or the locking rules around a security hook,
> > > > how are they supposed to understand the implications if that happens
> > > > through some magic fexit trampoline that is injected at runtime?
> > >
> > > I'm not following the concern. There is error injection facility that is
> > > heavily used with and without bpf. In this case there is really no difference
> > > whether trampoline is used with direct call or indirect callback via function
> > > pointer. Both will jump to bpf prog. The _source code_ of bpf program will
> > > _always_ be available for humans to examine via "bpftool prog dump" since BTF
> > > is required. So from inspectability and debuggability point of view lsm+bpf
> > > stuff is way more visible than any builtin LSM. At any time people will be able
> > > to see what exactly is running on the system. Assuming folks can read C code.
> >
> > You said that you want to use fexit without touching security/, which
> > AFAIU means that the branch from security_*() to the BPF LSM will be
> > invisible in the *kernel's* source code unless the reader already
> > knows about the BPF LSM. But maybe I'm just misunderstanding your
> > idea.
> >
> > If a random developer is trying to change the locking rules around
> > security_blah(), and wants to e.g. figure out whether it's okay to
> > call that thing with a spinlock held, or whether one of the arguments
> > is actually used, or stuff like that, the obvious way to verify that
> > is to follow all the direct and indirect calls made from
> > security_blah(). It's tedious, but it works, unless something is
> > hooked up to it in a way that is visible in no way in the source code.
> >
> > I agree that the way in which the call happens behind the scenes
> > doesn't matter all that much - I don't really care all that much
> > whether it's an indirect call, a runtime-patched direct call in inline
> > assembly, or an fexit hook. What I do care about is that someone
> > reading through any affected function can immediately see that the
> > branch exists - in other words, ideally, I'd like it to be something
> > happening in the method body, but if you think that's unacceptable, I
> > think there should at least be a function attribute that makes it very
> > clear what's going on.
>
> Got it. Then let's whitelist them ?
> All error injection points are marked with ALLOW_ERROR_INJECTION().
> We can do something similar here, but let's do it via BTF and avoid
> abusing yet another elf section for this mark.
> I think BTF_TYPE_EMIT() should work. Just need to pick explicit enough
> name and extensive comment about what is going on.
Sounds reasonable to me. :)
> Locking rules and cleanup around security_blah() shouldn't change though.
> Like security_task_alloc() should be paired with security_task_free().
> And so on. With bpf_sk_storage like logic the alloc/free of scratch
> space will be similar to the way socket and bpf progs deal with it.
>
> Some of the lsm hooks are in critical path. Like security_socket_sendmsg().
> retpoline hurts. If we go with indirect calls right now it will be harder to
> optimize later. It took us long time to come up with bpf trampoline and build
> bpf dispatcher on top of it to remove single indirect call from XDP runtime.
> For bpf+lsm would be good to avoid it from the start.
Just out of curiosity: Are fexit hooks really much cheaper than indirect calls?
AFAIK ftrace on x86-64 replaces the return pointer for fexit
instrumentation (see prepare_ftrace_return()). So when the function
returns, there is one return misprediction for branching into
return_to_handler(), and then the processor's internal return stack
will probably be misaligned so that after ftrace_return_to_handler()
is done running, all the following returns will also be mispredicted.
So I would've thought that fexit hooks would have at least roughly the
same impact as indirect calls - indirect calls via retpoline do one
mispredicted branch, fexit hooks do at least two AFAICS. But I guess
indirect calls could still be slower if fexit benefits from having all
the mispredicted pointers stored on the cache-hot stack while the
indirect branch target is too infrequently accessed to be in L1D, or
something like that?
^ permalink raw reply
* Re: [PATCH 00/24] user_namespace: introduce fsid mappings
From: Jann Horn @ 2020-02-11 20:55 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, smbarber,
Alexander Viro, Alexey Dobriyan, Serge Hallyn, James Morris,
Kees Cook, Jonathan Corbet, kernel list, linux-fsdevel,
Linux Containers, linux-security-module, Linux API
In-Reply-To: <20200211165753.356508-1-christian.brauner@ubuntu.com>
On Tue, Feb 11, 2020 at 5:59 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> This is the implementation of shiftfs which was cooked up during lunch at
> Linux Plumbers 2019 the day after the container's microconference. The
> idea is a design-stew from Stéphane, Aleksa, Eric, and myself. Back then
> we all were quite busy with other work and couldn't really sit down and
> implement it. But I took a few days last week to do this work, including
> demos and performance testing.
> This implementation does not require us to touch the vfs substantially
> at all. Instead, we implement shiftfs via fsid mappings.
> With this patch, it took me 20 mins to port both LXD and LXC to support
> shiftfs via fsid mappings.
>
> For anyone wanting to play with this the branch can be pulled from:
> https://github.com/brauner/linux/tree/fsid_mappings
> https://gitlab.com/brauner/linux/-/tree/fsid_mappings
> https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=fsid_mappings
>
> The main use case for shiftfs for us is in allowing shared writable
> storage to multiple containers using non-overlapping id mappings.
> In such a scenario you want the fsids to be valid and identical in both
> containers for the shared mount. A demo for this exists in [3].
> If you don't want to read on, go straight to the other demos below in
> [1] and [2].
I guess essentially this means that you want to have UID separation
between containers to prevent the containers - or their owners - from
interfering between each other, but for filesystem access, you don't
want to isolate them from each other using DAC controls on the files
and folders inside the containers' directory hierarchies, instead
relying on mode-0700 parent directories to restrict access to the
container owner? Or would you still have separate UIDs for e.g. the
container's UID range 0-65535, and then map the shared UID range at
100000, or something like that?
> People not as familiar with user namespaces might not be aware that fsid
> mappings already exist. Right now, fsid mappings are always identical to
> id mappings. Specifically, the kernel will lookup fsuids in the uid
> mappings and fsgids in the gid mappings of the relevant user namespace.
That's a bit like saying that a kernel without CONFIG_USER_NS still
has user ID mappings, they just happen to be identity mappings. :P
> With this patch series we simply introduce the ability to create fsid
> mappings that are different from the id mappings of a user namespace.
>
> In the usual case of running an unprivileged container we will have
> setup an id mapping, e.g. 0 100000 100000. The on-disk mapping will
> correspond to this id mapping, i.e. all files which we want to appear as
> 0:0 inside the user namespace will be chowned to 100000:100000 on the
> host. This works, because whenever the kernel needs to do a filesystem
> access it will lookup the corresponding uid and gid in the idmapping
> tables of the container.
> Now think about the case where we want to have an id mapping of 0 100000
> 100000 but an on-disk mapping of 0 300000 100000 which is needed to e.g.
> share a single on-disk mapping with multiple containers that all have
> different id mappings.
> This will be problematic. Whenever a filesystem access is requested, the
> kernel will now try to lookup a mapping for 300000 in the id mapping
> tables of the user namespace but since there is none the files will
> appear to be owned by the overflow id, i.e. usually 65534:65534 or
> nobody:nogroup.
>
> With fsid mappings we can solve this by writing an id mapping of 0
> 100000 100000 and an fsid mapping of 0 300000 100000. On filesystem
> access the kernel will now lookup the mapping for 300000 in the fsid
> mapping tables of the user namespace. And since such a mapping exists,
> the corresponding files will have correct ownership.
Sorry to bring up something as disgusting as setuid execution, but:
What happens when there's a setuid root file with ->i_uid==300000? I
guess the only way to make that work inside the containers would be
something like make_kuid(current_user_ns(),
from_kfsuid(current_user_ns(), inode->i_uid)) in the setuid execve
path?
> A note on proc (and sys), the proc filesystem is special in sofar as it
> only has a single superblock that is (currently but might be about to
> change) visible in all user namespaces (same goes for sys). This means
> it has special semantics in many ways, including how file ownership and
> access works. The fsid mapping implementation does not alter how proc
> (and sys) ownership works. proc and sys will both continue to lookup
> filesystem access in id mapping tables.
In your example, a process with namespaced UID set (0, 0, 0, 0) will
have kernel UIDs (100000, 100000, 100000, 300000), right? And then if
I want to open /proc/$pid/personality of another process with the same
UIDs, may_open() will call inode_permission() -> do_inode_permission()
-> generic_permission() -> acl_permission_check(), which will compare
current_fsuid() (which is 300000) against inode->i_uid. But
inode->i_uid was filled by proc_pid_make_inode()->task_dump_owner(),
which set inode->i_uid to 100000, right?
Also, e.g. __ptrace_may_access() uses cred->fsuid for a comparison
with another task's real/effective/saved UID.
[...]
> # Demos
> [1]: Create a container with different id and fsid mappings.
> https://asciinema.org/a/300233
> [2]: Create a container with id mappings but without fsid mappings.
> https://asciinema.org/a/300234
> [3]: Share storage between multiple containers with non-overlapping id
> mappings.
> https://asciinema.org/a/300235
(I really dislike this asciinema thing; if you want to quickly glance
through the output instead of reading at the same speed as it was
typed, a simple pastebin works much better unless you absolutely have
to show things that use stuff like ncurses UI.)
^ permalink raw reply
* [PATCH] apparmor: Replace zero-length array with flexible-array member
From: Gustavo A. R. Silva @ 2020-02-11 21:04 UTC (permalink / raw)
To: John Johansen, James Morris, Serge E. Hallyn
Cc: linux-security-module, linux-kernel, Gustavo A. R. Silva
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertenly introduced[3] to the codebase from now on.
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
security/apparmor/apparmorfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index fefee040bf79..cc81080efb63 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -804,7 +804,7 @@ static ssize_t query_label(char *buf, size_t buf_len,
struct multi_transaction {
struct kref count;
ssize_t size;
- char data[0];
+ char data[];
};
#define MULTI_TRANSACTION_LIMIT (PAGE_SIZE - sizeof(struct multi_transaction))
--
2.25.0
^ permalink raw reply related
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Jann Horn @ 2020-02-11 21:32 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <CAG48ez1qGqF9z7APajFyzjZh82YxFV9sHE64f5kdKBeH9J3YPg@mail.gmail.com>
On Tue, Feb 11, 2020 at 9:33 PM Jann Horn <jannh@google.com> wrote:
> On Tue, Feb 11, 2020 at 9:10 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Tue, Feb 11, 2020 at 08:36:18PM +0100, Jann Horn wrote:
> > > On Tue, Feb 11, 2020 at 8:09 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > > On Tue, Feb 11, 2020 at 07:44:05PM +0100, Jann Horn wrote:
> > > > > On Tue, Feb 11, 2020 at 6:58 PM Alexei Starovoitov
> > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > > On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
> > > > > [...]
> > > > > > > * When using the semantic provided by fexit, the BPF LSM program will
> > > > > > > always be executed and will be able to override / clobber the
> > > > > > > decision of LSMs which appear before it in the ordered list. This
> > > > > > > semantic is very different from what we currently have (i.e. the BPF
> > > > > > > LSM hook is only called if all the other LSMs allow the action) and
> > > > > > > seems to be bypassing the LSM framework.
> > > > > >
> > > > > > It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
> > > > > > trampoline generator specific to lsm progs.
> > > > > [...]
> > > > > > Using fexit mechanism and bpf_sk_storage generalization is
> > > > > > all that is needed. None of it should touch security/*.
[...]
> > Some of the lsm hooks are in critical path. Like security_socket_sendmsg().
> > retpoline hurts. If we go with indirect calls right now it will be harder to
> > optimize later. It took us long time to come up with bpf trampoline and build
> > bpf dispatcher on top of it to remove single indirect call from XDP runtime.
> > For bpf+lsm would be good to avoid it from the start.
>
> Just out of curiosity: Are fexit hooks really much cheaper than indirect calls?
>
> AFAIK ftrace on x86-64 replaces the return pointer for fexit
> instrumentation (see prepare_ftrace_return()). So when the function
> returns, there is one return misprediction for branching into
> return_to_handler(), and then the processor's internal return stack
> will probably be misaligned so that after ftrace_return_to_handler()
> is done running, all the following returns will also be mispredicted.
>
> So I would've thought that fexit hooks would have at least roughly the
> same impact as indirect calls - indirect calls via retpoline do one
> mispredicted branch, fexit hooks do at least two AFAICS. But I guess
> indirect calls could still be slower if fexit benefits from having all
> the mispredicted pointers stored on the cache-hot stack while the
> indirect branch target is too infrequently accessed to be in L1D, or
> something like that?
Ah, kpsingh explained to me that BPF trampolines work differently from
normal ftrace and don't do the return address poking. Still, the
processor's internal call stack will be misaligned by the BPF
trampoline, right? Since there is one more call than return, if e.g.
security_blah() is hooked, then the kernel will probably predict
security_blah+5 as the target when returning from the trampoline to
security_blah's parent, then when returning from security_blah's
parent to the grandparent predict the parent, and so on, right?
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Alexei Starovoitov @ 2020-02-11 21:38 UTC (permalink / raw)
To: Jann Horn
Cc: KP Singh, kernel list, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team
In-Reply-To: <CAG48ez1qGqF9z7APajFyzjZh82YxFV9sHE64f5kdKBeH9J3YPg@mail.gmail.com>
On Tue, Feb 11, 2020 at 09:33:49PM +0100, Jann Horn wrote:
> >
> > Got it. Then let's whitelist them ?
> > All error injection points are marked with ALLOW_ERROR_INJECTION().
> > We can do something similar here, but let's do it via BTF and avoid
> > abusing yet another elf section for this mark.
> > I think BTF_TYPE_EMIT() should work. Just need to pick explicit enough
> > name and extensive comment about what is going on.
>
> Sounds reasonable to me. :)
awesome :)
> > Locking rules and cleanup around security_blah() shouldn't change though.
> > Like security_task_alloc() should be paired with security_task_free().
> > And so on. With bpf_sk_storage like logic the alloc/free of scratch
> > space will be similar to the way socket and bpf progs deal with it.
> >
> > Some of the lsm hooks are in critical path. Like security_socket_sendmsg().
> > retpoline hurts. If we go with indirect calls right now it will be harder to
> > optimize later. It took us long time to come up with bpf trampoline and build
> > bpf dispatcher on top of it to remove single indirect call from XDP runtime.
> > For bpf+lsm would be good to avoid it from the start.
>
> Just out of curiosity: Are fexit hooks really much cheaper than indirect calls?
>
> AFAIK ftrace on x86-64 replaces the return pointer for fexit
> instrumentation (see prepare_ftrace_return()). So when the function
> returns, there is one return misprediction for branching into
> return_to_handler(), and then the processor's internal return stack
> will probably be misaligned so that after ftrace_return_to_handler()
> is done running, all the following returns will also be mispredicted.
>
> So I would've thought that fexit hooks would have at least roughly the
> same impact as indirect calls - indirect calls via retpoline do one
> mispredicted branch, fexit hooks do at least two AFAICS. But I guess
> indirect calls could still be slower if fexit benefits from having all
> the mispredicted pointers stored on the cache-hot stack while the
> indirect branch target is too infrequently accessed to be in L1D, or
> something like that?
For fexit I've tried ftrace style of overwriting return address in the stack,
but it was slower than "leave; add rsp, 8; ret". So I went with that.
Looks like skipping one frame makes intel return stack prediction work better
than overwriting. I tested on broadwell only though. Later I realized that I
can do "jmp bpf_trampoline" instead of "call bpf_trampoline", so this extra
'add rsp, 8' can be removed and both direct jump and return stack predictors
will be happy. Will be posting this patch soon.
Old perf numbers of fexit vs original vs kprobe:
https://lore.kernel.org/bpf/20191122011515.255371-1-ast@kernel.org/
^ permalink raw reply
* Re: [PATCH 1/2] crypto: rename sm3-256 to sm3 in hash_algo_name
From: Mimi Zohar @ 2020-02-11 21:49 UTC (permalink / raw)
To: Tianjia Zhang, herbert, davem, dmitry.kasatkin, jmorris, serge,
ebiggers
Cc: linux-crypto, linux-integrity, linux-security-module,
linux-kernel
In-Reply-To: <20200210124440.23929-2-tianjia.zhang@linux.alibaba.com>
On Mon, 2020-02-10 at 20:44 +0800, Tianjia Zhang wrote:
> The name sm3-256 is defined in hash_algo_name in hash_info, but the
> algorithm name implemented in sm3_generic.c is sm3, which will cause
> the sm3-256 algorithm to be not found in some application scenarios of
> the hash algorithm, and an ENOENT error will occur. For example,
> IMA, keys, and other subsystems that reference hash_algo_name all use
> the hash algorithm of sm3.
>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
The "hash_map" needs to be updated to reflect this change.
static struct tpm2_hash tpm2_hash_map[] = {
{HASH_ALGO_SHA1, TPM_ALG_SHA1},
{HASH_ALGO_SHA256, TPM_ALG_SHA256},
{HASH_ALGO_SHA384, TPM_ALG_SHA384},
{HASH_ALGO_SHA512, TPM_ALG_SHA512},
{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
};
Mimi
> ---
> crypto/hash_info.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/hash_info.c b/crypto/hash_info.c
> index c754cb75dd1a..a49ff96bde77 100644
> --- a/crypto/hash_info.c
> +++ b/crypto/hash_info.c
> @@ -26,7 +26,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
> [HASH_ALGO_TGR_128] = "tgr128",
> [HASH_ALGO_TGR_160] = "tgr160",
> [HASH_ALGO_TGR_192] = "tgr192",
> - [HASH_ALGO_SM3_256] = "sm3-256",
> + [HASH_ALGO_SM3_256] = "sm3",
> [HASH_ALGO_STREEBOG_256] = "streebog256",
> [HASH_ALGO_STREEBOG_512] = "streebog512",
> };
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-11 22:45 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <20200210150519.538333-8-gladkov.alexey@gmail.com>
On Mon, Feb 10, 2020 at 04:05:15PM +0100, Alexey Gladkov wrote:
> This allows to flush dcache entries of a task on multiple procfs mounts
> per pid namespace.
>
> The RCU lock is used because the number of reads at the task exit time
> is much larger than the number of procfs mounts.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
> fs/proc/base.c | 20 +++++++++++++++-----
> fs/proc/root.c | 27 ++++++++++++++++++++++++++-
> include/linux/pid_namespace.h | 2 ++
> include/linux/proc_fs.h | 2 ++
> 4 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 4ccb280a3e79..24b7c620ded3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
> .permission = proc_pid_permission,
> };
>
> -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
> {
> struct dentry *dentry, *leader, *dir;
> char buf[10 + 1];
> @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> name.name = buf;
> name.len = snprintf(buf, sizeof(buf), "%u", pid);
> /* no ->d_hash() rejects on procfs */
> - dentry = d_hash_and_lookup(mnt->mnt_root, &name);
> + dentry = d_hash_and_lookup(mnt_root, &name);
> if (dentry) {
> d_invalidate(dentry);
... which can block
> dput(dentry);
... and so can this
> + rcu_read_lock();
> + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) {
> + mnt_root = fs_info->m_super->s_root;
> + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
... making that more than slightly unsafe.
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Mimi Zohar @ 2020-02-11 23:10 UTC (permalink / raw)
To: david.safford, Janne Karhunen, linux-integrity,
linux-security-module
Cc: Ken Goldman, monty.wiseman, Amir Goldstein, linux-fsdevel
In-Reply-To: <fab03a0b8cc9dc93f2d0db51071521ce82e2b96b.camel@gmail.com>
On Tue, 2020-02-11 at 11:10 -0500, david.safford@gmail.com wrote:
> On Mon, 2020-02-10 at 15:24 -0500, Mimi Zohar wrote:
> > On Mon, 2020-02-10 at 13:18 -0500, david.safford@gmail.com wrote:
> > > On Thu, 2020-02-06 at 09:13 -0500, Mimi Zohar wrote:
> > > > Hi Janne,
> > > >
> > > > On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> > > > > On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> > > > > > Some systems can end up carrying lots of entries in the ima
> > > > > > measurement list. Since every entry is using a bit of kernel
> > > > > > memory, allow the sysadmin to export the measurement list to
> > > > > > the filesystem to free up some memory.
> > > > >
> > > > > Hopefully this addressed comments from everyone. The flush event can
> > > > > now be triggered by the admin anytime and unique file names can be
> > > > > used for each flush (log.1, log.2, ...) etc, so getting to the correct
> > > > > item should be easy.
> > > > >
> > > > > While it can now be argued that since this is an admin-driven event,
> > > > > kernel does not need to write the file. However, the intention is to
> > > > > bring out a second patch a bit later that adds a variable to define
> > > > > the max number of entries to be kept in the kernel memory and
> > > > > workqueue based automatic flushing. In those cases the kernel has to
> > > > > be able to write the file without any help from the admin..
> > > >
> > > > The implications of exporting and removing records from the IMA-
> > > > measurement list needs to be considered carefully. Verifying a TPM
> > > > quote will become dependent on knowing where the measurements are
> > > > stored. The existing measurement list is stored in kernel memory and,
> > > > barring a kernel memory attack, is protected from modification.
> > > > Before upstreaming this or a similar patch, there needs to be a
> > > > discussion as to how the measurement list will be protected once is it
> > > > exported to userspace.
> > >
> > > "Protected" here can mean two different aspects: cryptographically
> > > protected from tampering, which is covered with the TPM_QUOTE, and
> > > availability protected from even accidental deletion, which is what
> > > I suspect you are concerned about. Certainly my original TLV patches
> > > were too flippant about this, as userspace had to be trusted not to
> > > drop any records. In this patch, the kernel writes the data in an
> > > atomic fashion. Either all records are successfully written, or none
> > > are, and an error is returned.
> >
> > A third aspect, which I'm concerned about, is removing records from
> > the measurement list. This changes the existing userspace
> > expectations of returning the entire measurement list. Now userspace
> > will need some out of band method of knowing where to look for the
> > measurements.
>
> This is a feature, not a bug. :-)
> There is no reason to resend the same data for every attestation,
> nor is there any reason to store already attested measurements anywhere
> on the client. By versioning the log file names, userspace gets a
> simple way to know what has and has not been attested, and for small
> embedded devices we don't need to waste memory or filesystem space
> on the data already attested.
This new feature will require setting up some infrastructure for
storing the partial measurement list(s) in order to validate a TPM
quote. Userspace already can save partial measurement list(s) without
any kernel changes. The entire measurement list does not need to be
read each time. lseek can read past the last record previously read.
The only new aspect is truncating the in kernel measurement list in
order to free kernel memory.
< snip>
> > > > Instead of exporting the measurement records, one option as suggested
> > > > by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> > > > file for backing store. The existing securityfs measurement lists
> > > > would then read from this private copy of the anonymous file.
> > >
> > > This doesn't help in use cases where we really do want to
> > > export to a persistent file, without userspace help.
> >
> > Is to prevent needing to carry the measurement list across kexec the
> > only reason for the kernel needing to write to a persistent file?
>
> Well, that and the other reasons mentioned, such as completely freeing
> the data from the client after attestation, and simplicity of the
> mechanism.
Until there is proof that the measurement list can be exported to a
file before kexec, instead of carrying the measurement list across
kexec, and a TPM quote can be validated after the kexec, there isn't a
compelling reason for the kernel needing to truncate the measurement
list.
Mimi
^ permalink raw reply
* Re: [PATCH 1/2] crypto: rename sm3-256 to sm3 in hash_algo_name
From: Mimi Zohar @ 2020-02-11 23:23 UTC (permalink / raw)
To: Tianjia Zhang, herbert, davem, dmitry.kasatkin, jmorris, serge,
ebiggers
Cc: linux-crypto, linux-integrity, linux-security-module,
linux-kernel
In-Reply-To: <1581457759.5125.18.camel@linux.ibm.com>
On Tue, 2020-02-11 at 16:49 -0500, Mimi Zohar wrote:
> On Mon, 2020-02-10 at 20:44 +0800, Tianjia Zhang wrote:
> > The name sm3-256 is defined in hash_algo_name in hash_info, but the
> > algorithm name implemented in sm3_generic.c is sm3, which will cause
> > the sm3-256 algorithm to be not found in some application scenarios of
> > the hash algorithm, and an ENOENT error will occur. For example,
> > IMA, keys, and other subsystems that reference hash_algo_name all use
> > the hash algorithm of sm3.
> >
> > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
>
> The "hash_map" needs to be updated to reflect this change.
>
> static struct tpm2_hash tpm2_hash_map[] = {
> {HASH_ALGO_SHA1, TPM_ALG_SHA1},
> {HASH_ALGO_SHA256, TPM_ALG_SHA256},
> {HASH_ALGO_SHA384, TPM_ALG_SHA384},
> {HASH_ALGO_SHA512, TPM_ALG_SHA512},
> {HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
> };
Never mind, the enum name "HASH_ALGO_SM3_256" didn't change. Just the
string changed.
Mimi
^ 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