[RFC PATCH v2 0/3] ima: namespacing IMA

[RFC PATCH v2 0/3] ima: namespacing IMA

[Stefan Berger]

This patch set implements an IMA namespace data structure that gets
created alongside a mount namespace with CLONE_NEWNS, and lays down the
foundation for namespacing the different aspects of IMA (eg. IMA-audit,
IMA-measurement, IMA-appraisal).

The original PoC patches [1] created a new CLONE_NEWIMA flag to
explicitly control when a new IMA namespace should be created. Based on
comments, we elected to hang the IMA namepace off of existing namespaces,
and the mount namespace made the most sense. In this version of the patches
we are adding a pointer to the mnt_namespace pointing to the ima_namespace.
Both are now tied together and joining the mnt_namespace with setns()
also joins the ima_namespace that was created along with it.

The first patch creates the ima_namespace data, while the second patch
puts the iint->flags in the namespace. The third patch uses these flags
for namespacing the IMA-audit messages, enabling the same file to be
audited each time it is accessed in a new namespace.

Mehmet Kayaalp (2):
  ima: Add ns_status for storing namespaced iint data
  ima: mamespace audit status flags

Yuqiong Sun (1):
  ima: extend clone() with IMA namespace support

 fs/mount.h                               |  14 --
 fs/namespace.c                           |  29 +++-
 include/linux/ima.h                      |  70 ++++++++++
 include/linux/mount.h                    |  20 ++-
 init/Kconfig                             |  10 ++
 kernel/nsproxy.c                         |   1 +
 security/integrity/ima/Makefile          |   3 +-
 security/integrity/ima/ima.h             |  47 ++++++-
 security/integrity/ima/ima_api.c         |   8 +-
 security/integrity/ima/ima_init.c        |   4 +
 security/integrity/ima/ima_init_ima_ns.c |  44 ++++++
 security/integrity/ima/ima_main.c        |  15 +-
 security/integrity/ima/ima_ns.c          | 230 +++++++++++++++++++++++++++++++
 13 files changed, 469 insertions(+), 26 deletions(-)
 create mode 100644 security/integrity/ima/ima_init_ima_ns.c
 create mode 100644 security/integrity/ima/ima_ns.c

-- 
2.13.6

[RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

From: Yuqiong Sun <suny@us.ibm.com>

Add new CONFIG_IMA_NS config option.  Let clone() create a new IMA
namespace upon CLONE_NEWNS flag. Add ima_ns data structure in nsproxy.
ima_ns is allocated and freed upon IMA namespace creation and exit.
Currently, the ima_ns contains no useful IMA data but only a dummy
interface. This patch creates the framework for namespacing the different
aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal).

Changelog:
* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
* Use existing ima.h headers
* Move the ima_namespace.c to security/integrity/ima/ima_ns.c
* Fix typo INFO->INO
* Each namespace free's itself, removed recursively free'ing
  until init_ima_ns from free_ima_ns()
* Moved ima_init_ns and related functions into own file that is
  always compiled
* Fixed putting of imans->parent
* Move IMA namespace creation from nsproxy into mount namespace
  code

Signed-off-by: Yuqiong Sun <suny@us.ibm.com>
Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 fs/mount.h                               | 14 -----
 fs/namespace.c                           | 29 ++++++++--
 include/linux/ima.h                      | 67 +++++++++++++++++++++++
 include/linux/mount.h                    | 20 ++++++-
 init/Kconfig                             |  8 +++
 kernel/nsproxy.c                         |  1 +
 security/integrity/ima/Makefile          |  3 +-
 security/integrity/ima/ima.h             |  4 ++
 security/integrity/ima/ima_init.c        |  4 ++
 security/integrity/ima/ima_init_ima_ns.c | 38 +++++++++++++
 security/integrity/ima/ima_ns.c          | 91 ++++++++++++++++++++++++++++++++
 11 files changed, 260 insertions(+), 19 deletions(-)
 create mode 100644 security/integrity/ima/ima_init_ima_ns.c
 create mode 100644 security/integrity/ima/ima_ns.c

diff --git a/fs/mount.h b/fs/mount.h
index f39bc9da4d73..e19ebde97756 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -5,20 +5,6 @@
 #include <linux/ns_common.h>
 #include <linux/fs_pin.h>
 
-struct mnt_namespace {
-	atomic_t		count;
-	struct ns_common	ns;
-	struct mount *	root;
-	struct list_head	list;
-	struct user_namespace	*user_ns;
-	struct ucounts		*ucounts;
-	u64			seq;	/* Sequence number to prevent loops */
-	wait_queue_head_t poll;
-	u64 event;
-	unsigned int		mounts; /* # of mounts in the namespace */
-	unsigned int		pending_mounts;
-} __randomize_layout;
-
 struct mnt_pcp {
 	int mnt_count;
 	int mnt_writers;
diff --git a/fs/namespace.c b/fs/namespace.c
index 9d1374ab6e06..7f886c02278b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -26,6 +26,7 @@
 #include <linux/bootmem.h>
 #include <linux/task_work.h>
 #include <linux/sched/task.h>
+#include <linux/ima.h>
 
 #include "pnode.h"
 #include "internal.h"
@@ -2858,6 +2859,7 @@ static void dec_mnt_namespaces(struct ucounts *ucounts)
 
 static void free_mnt_ns(struct mnt_namespace *ns)
 {
+	put_ima_ns(ns->ima_ns);
 	ns_free_inum(&ns->ns);
 	dec_mnt_namespaces(ns->ucounts);
 	put_user_ns(ns->user_ns);
@@ -2873,11 +2875,13 @@ static void free_mnt_ns(struct mnt_namespace *ns)
  */
 static atomic64_t mnt_ns_seq = ATOMIC64_INIT(1);
 
-static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
+static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns,
+					  struct ima_namespace *ima_ns)
 {
 	struct mnt_namespace *new_ns;
 	struct ucounts *ucounts;
 	int ret;
+	int err;
 
 	ucounts = inc_mnt_namespaces(user_ns);
 	if (!ucounts)
@@ -2894,6 +2898,20 @@ static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
 		dec_mnt_namespaces(ucounts);
 		return ERR_PTR(ret);
 	}
+
+	if (ima_ns == NULL) {
+		new_ns->ima_ns = get_ima_ns(&init_ima_ns);
+	} else {
+		new_ns->ima_ns = copy_ima(user_ns, ima_ns);
+		if (IS_ERR(new_ns->ima_ns)) {
+			err = PTR_ERR(new_ns->ima_ns);
+			ns_free_inum(&new_ns->ns);
+			kfree(new_ns);
+			dec_mnt_namespaces(ucounts);
+			return ERR_PTR(err);
+		}
+	}
+
 	new_ns->ns.ops = &mntns_operations;
 	new_ns->seq = atomic64_add_return(1, &mnt_ns_seq);
 	atomic_set(&new_ns->count, 1);
@@ -2920,6 +2938,7 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
 	int copy_flags;
 
 	BUG_ON(!ns);
+	BUG_ON(!ns->ima_ns);
 
 	if (likely(!(flags & CLONE_NEWNS))) {
 		get_mnt_ns(ns);
@@ -2928,7 +2947,7 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
 
 	old = ns->root;
 
-	new_ns = alloc_mnt_ns(user_ns);
+	new_ns = alloc_mnt_ns(user_ns, ns->ima_ns);
 	if (IS_ERR(new_ns))
 		return new_ns;
 
@@ -2989,7 +3008,8 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
  */
 static struct mnt_namespace *create_mnt_ns(struct vfsmount *m)
 {
-	struct mnt_namespace *new_ns = alloc_mnt_ns(&init_user_ns);
+	struct mnt_namespace *new_ns = alloc_mnt_ns(&init_user_ns,
+						    NULL);
 	if (!IS_ERR(new_ns)) {
 		struct mount *mnt = real_mount(m);
 		mnt->mnt_ns = new_ns;
@@ -3497,6 +3517,9 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns)
 	set_fs_root(fs, &root);
 
 	path_put(&root);
+
+	imans_install(nsproxy, ns);
+
 	return 0;
 }
 
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e0eb60..fd150dfde277 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -12,6 +12,7 @@
 
 #include <linux/fs.h>
 #include <linux/kexec.h>
+#include <linux/mount.h>
 struct linux_binprm;
 
 #ifdef CONFIG_IMA
@@ -105,4 +106,70 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
 	return 0;
 }
 #endif /* CONFIG_IMA_APPRAISE */
+
+struct ima_namespace {
+	struct kref kref;
+	struct user_namespace *user_ns;
+	struct ima_namespace *parent;
+};
+
+extern struct ima_namespace init_ima_ns;
+
+void imans_install(struct nsproxy *nsproxy, struct ns_common *new);
+
+static inline struct ima_namespace *to_ima_ns(struct ns_common *ns)
+{
+	return container_of(ns, struct mnt_namespace, ns)->ima_ns;
+}
+
+#ifdef CONFIG_IMA_NS
+
+void free_ima_ns(struct kref *kref);
+
+static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
+{
+	BUG_ON(!ns);
+	if (ns)
+		kref_get(&ns->kref);
+	return ns;
+}
+
+static inline void put_ima_ns(struct ima_namespace *ns)
+{
+	BUG_ON(!ns);
+	if (ns)
+		kref_put(&ns->kref, free_ima_ns);
+}
+
+struct ima_namespace *copy_ima(struct user_namespace *user_ns,
+			       struct ima_namespace *old_ns);
+
+static inline struct ima_namespace *get_current_ns(void)
+{
+	return current->nsproxy->mnt_ns->ima_ns;
+}
+
+#else
+
+static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
+{
+	return ns;
+}
+
+static inline void put_ima_ns(struct ima_namespace *ns)
+{
+	return;
+}
+
+static inline struct ima_namespace *copy_ima(struct user_namespace *user_ns,
+					     struct ima_namespace *old_ns)
+{
+	return old_ns;
+}
+
+static inline struct ima_namespace *get_current_ns(void)
+{
+	return NULL;
+}
+#endif /* CONFIG_IMA_NS */
 #endif /* _LINUX_IMA_H */
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 45b1f56c6c2f..361c962ebd3d 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -16,11 +16,29 @@
 #include <linux/spinlock.h>
 #include <linux/seqlock.h>
 #include <linux/atomic.h>
+#include <linux/ns_common.h>
+#include <linux/wait.h>
 
 struct super_block;
 struct vfsmount;
 struct dentry;
-struct mnt_namespace;
+struct ima_namespace;
+
+struct mnt_namespace {
+	atomic_t		count;
+	struct ns_common	ns;
+	struct mount *	root;
+	struct list_head	list;
+	struct user_namespace	*user_ns;
+	struct ucounts		*ucounts;
+	u64			seq;	/* Sequence number to prevent loops */
+	wait_queue_head_t poll;
+	u64 event;
+	unsigned int		mounts; /* # of mounts in the namespace */
+	unsigned int		pending_mounts;
+	struct ima_namespace    *ima_ns;
+} __randomize_layout;
+
 
 #define MNT_NOSUID	0x01
 #define MNT_NODEV	0x02
diff --git a/init/Kconfig b/init/Kconfig
index a9a2e2c86671..a1ad5384e081 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -931,6 +931,14 @@ config NET_NS
 	help
 	  Allow user space to create what appear to be multiple instances
 	  of the network stack.
+config IMA_NS
+	bool "IMA namespace"
+	depends on IMA
+	default y
+	help
+	  Allow the creation of IMA namespaces for each mount namespace.
+	  Namespaced IMA data enables having IMA features work separately
+	  for each mount namespace.
 
 endif # NAMESPACES
 
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index f6c5d330059a..7d1a35362186 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -27,6 +27,7 @@
 #include <linux/syscalls.h>
 #include <linux/cgroup.h>
 #include <linux/perf_event.h>
+#include <linux/ima.h>
 
 static struct kmem_cache *nsproxy_cachep;
 
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index d921dc4f9eb0..cc60f726e651 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -7,7 +7,8 @@
 obj-$(CONFIG_IMA) += ima.o
 
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
-	 ima_policy.o ima_template.o ima_template_lib.o
+	 ima_policy.o ima_template.o ima_template_lib.o ima_init_ima_ns.o
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_NS) += ima_ns.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
 obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..e98c11c7cf75 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -291,6 +291,10 @@ static inline int ima_read_xattr(struct dentry *dentry,
 
 #endif /* CONFIG_IMA_APPRAISE */
 
+int ima_ns_init(void);
+struct ima_namespace;
+int ima_init_namespace(struct ima_namespace *ns);
+
 /* LSM based policy rules require audit */
 #ifdef CONFIG_IMA_LSM_RULES
 
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 2967d497a665..7f884a71fa1c 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -137,5 +137,9 @@ int __init ima_init(void)
 
 	ima_init_policy();
 
+	rc = ima_ns_init();
+	if (rc != 0)
+		return rc;
+
 	return ima_fs_init();
 }
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
new file mode 100644
index 000000000000..4b081dbfac07
--- /dev/null
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2016-2018 IBM Corporation
+ * Author: Yuqiong Sun <suny@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#include <linux/export.h>
+#include <linux/user_namespace.h>
+#include <linux/ima.h>
+
+int ima_init_namespace(struct ima_namespace *ns)
+{
+	return 0;
+}
+
+int __init ima_ns_init(void)
+{
+	return ima_init_namespace(&init_ima_ns);
+}
+
+struct ima_namespace init_ima_ns = {
+	.kref = KREF_INIT(2),
+	.user_ns = &init_user_ns,
+	.parent = NULL,
+};
+EXPORT_SYMBOL(init_ima_ns);
+
+void imans_install(struct nsproxy *nsproxy, struct ns_common *new)
+{
+	struct ima_namespace *ns = to_ima_ns(new);
+
+	get_ima_ns(ns);
+	put_ima_ns(nsproxy->mnt_ns->ima_ns);
+	nsproxy->mnt_ns->ima_ns = ns;
+}
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
new file mode 100644
index 000000000000..7ab4322c88ae
--- /dev/null
+++ b/security/integrity/ima/ima_ns.c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2016-2018 IBM Corporation
+ * Author: Yuqiong Sun <suny@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#include <linux/user_namespace.h>
+#include <linux/kref.h>
+#include <linux/slab.h>
+#include <linux/ima.h>
+#include <linux/mount.h>
+
+#include "ima.h"
+
+static struct ima_namespace *create_ima_ns(void)
+{
+	struct ima_namespace *ima_ns;
+
+	ima_ns = kmalloc(sizeof(*ima_ns), GFP_KERNEL);
+	if (ima_ns)
+		kref_init(&ima_ns->kref);
+
+	return ima_ns;
+}
+
+/**
+ * Clone a new ns copying an original ima namespace, setting refcount to 1
+ *
+ * @old_ns: old ima namespace to clone
+ * @user_ns: user namespace that current task runs in
+ * Return ERR_PTR(-ENOMEM) on error (failure to kmalloc), new ns otherwise
+ */
+static struct ima_namespace *clone_ima_ns(struct user_namespace *user_ns,
+					  struct ima_namespace *old_ns)
+{
+	struct ima_namespace *ns;
+
+	ns = create_ima_ns();
+	if (!ns)
+		return ERR_PTR(-ENOMEM);
+
+	get_ima_ns(old_ns);
+	ns->parent = old_ns;
+	ns->user_ns = get_user_ns(user_ns);
+
+	ima_init_namespace(ns);
+
+	return ns;
+}
+
+/**
+ * Copy task's ima namespace, or clone it if flags specifies CLONE_NEWNS.
+ *
+ * @flags: flags used in the clone syscall
+ * @user_ns: user namespace that current task runs in
+ * @old_ns: old ima namespace to clone
+ */
+
+struct ima_namespace *copy_ima(struct user_namespace *user_ns,
+			       struct ima_namespace *old_ns)
+{
+	struct ima_namespace *new_ns;
+
+	BUG_ON(!old_ns);
+	get_ima_ns(old_ns);
+
+	new_ns = clone_ima_ns(user_ns, old_ns);
+	put_ima_ns(old_ns);
+
+	return new_ns;
+}
+
+static void destroy_ima_ns(struct ima_namespace *ns)
+{
+	put_user_ns(ns->user_ns);
+	put_ima_ns(ns->parent);
+	kfree(ns);
+}
+
+void free_ima_ns(struct kref *kref)
+{
+	struct ima_namespace *ns;
+
+	ns = container_of(kref, struct ima_namespace, kref);
+	BUG_ON(ns == &init_ima_ns);
+
+	destroy_ima_ns(ns);
+}
-- 
2.13.6

[RFC PATCH v2 2/3] ima: Add ns_status for storing namespaced iint data

[Stefan Berger]

From: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>

This patch adds an rbtree to the IMA namespace structure that stores a
namespaced version of iint->flags in ns_status struct. Similar to the
integrity_iint_cache, both the iint ns_struct are looked up using the
inode pointer value. The lookup, allocate, and insertion code is also
similar, except ns_struct is not free'd when the inode is free'd.
Instead, the lookup verifies the i_ino and i_generation fields are also a
match. This could be replaced by a lazy clean up of the rbtree.

Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
---
 include/linux/ima.h                      |   3 +
 security/integrity/ima/ima.h             |  19 +++++
 security/integrity/ima/ima_init_ima_ns.c |   6 ++
 security/integrity/ima/ima_ns.c          | 117 +++++++++++++++++++++++++++++++
 4 files changed, 145 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index fd150dfde277..52c9d338819c 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -111,6 +111,9 @@ struct ima_namespace {
 	struct kref kref;
 	struct user_namespace *user_ns;
 	struct ima_namespace *parent;
+	struct rb_root ns_status_tree;
+	rwlock_t ns_status_lock;
+	struct kmem_cache *ns_status_cache;
 };
 
 extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e98c11c7cf75..e51a39ff75ff 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -128,6 +128,14 @@ static inline void ima_load_kexec_buffer(void) {}
  */
 extern bool ima_canonical_fmt;
 
+struct ns_status {
+	struct rb_node rb_node;
+	struct inode *inode;
+	ino_t i_ino;
+	u32 i_generation;
+	unsigned long flags;
+};
+
 /* Internal IMA function definitions */
 int ima_init(void);
 int ima_fs_init(void);
@@ -295,6 +303,17 @@ int ima_ns_init(void);
 struct ima_namespace;
 int ima_init_namespace(struct ima_namespace *ns);
 
+#ifdef CONFIG_IMA_NS
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+				    struct inode *inode);
+#else
+static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+						  struct inode *inode)
+{
+	return NULL;
+}
+#endif /* CONFIG_IMA_NS */
+
 /* LSM based policy rules require audit */
 #ifdef CONFIG_IMA_LSM_RULES
 
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 4b081dbfac07..9e0d5fafdfba 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -10,9 +10,15 @@
 #include <linux/export.h>
 #include <linux/user_namespace.h>
 #include <linux/ima.h>
+#include <linux/slab.h>
+
+#include "ima.h"
 
 int ima_init_namespace(struct ima_namespace *ns)
 {
+	ns->ns_status_tree = RB_ROOT;
+	rwlock_init(&ns->ns_status_lock);
+	ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
 	return 0;
 }
 
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 7ab4322c88ae..03acf1431868 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -73,10 +73,24 @@ struct ima_namespace *copy_ima(struct user_namespace *user_ns,
 	return new_ns;
 }
 
+static void free_ns_status_cache(struct ima_namespace *ns)
+{
+	struct ns_status *status, *next;
+
+	write_lock(&ns->ns_status_lock);
+	rbtree_postorder_for_each_entry_safe(status, next,
+					     &ns->ns_status_tree, rb_node)
+		kmem_cache_free(ns->ns_status_cache, status);
+	ns->ns_status_tree = RB_ROOT;
+	write_unlock(&ns->ns_status_lock);
+	kmem_cache_destroy(ns->ns_status_cache);
+}
+
 static void destroy_ima_ns(struct ima_namespace *ns)
 {
 	put_user_ns(ns->user_ns);
 	put_ima_ns(ns->parent);
+	free_ns_status_cache(ns);
 	kfree(ns);
 }
 
@@ -89,3 +103,106 @@ void free_ima_ns(struct kref *kref)
 
 	destroy_ima_ns(ns);
 }
+
+/*
+ * __ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
+					      struct inode *inode)
+{
+	struct ns_status *status;
+	struct rb_node *n = ns->ns_status_tree.rb_node;
+
+	while (n) {
+		status = rb_entry(n, struct ns_status, rb_node);
+
+		if (inode < status->inode)
+			n = n->rb_left;
+		else if (inode > status->inode)
+			n = n->rb_right;
+		else
+			break;
+	}
+	if (!n)
+		return NULL;
+
+	return status;
+}
+
+/*
+ * ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
+					    struct inode *inode)
+{
+	struct ns_status *status;
+
+	read_lock(&ns->ns_status_lock);
+	status = __ima_ns_status_find(ns, inode);
+	read_unlock(&ns->ns_status_lock);
+
+	return status;
+}
+
+void insert_ns_status(struct ima_namespace *ns, struct inode *inode,
+		      struct ns_status *status)
+{
+	struct rb_node **p;
+	struct rb_node *node, *parent = NULL;
+	struct ns_status *test_status;
+
+	p = &ns->ns_status_tree.rb_node;
+	while (*p) {
+		parent = *p;
+		test_status = rb_entry(parent, struct ns_status, rb_node);
+		if (inode < test_status->inode)
+			p = &(*p)->rb_left;
+		else
+			p = &(*p)->rb_right;
+	}
+	node = &status->rb_node;
+	rb_link_node(node, parent, p);
+	rb_insert_color(node, &ns->ns_status_tree);
+}
+
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+				    struct inode *inode)
+{
+	struct ns_status *status;
+	int skip_insert = 0;
+
+	status = ima_ns_status_find(ns, inode);
+	if (status) {
+		/*
+		 * Unlike integrity_iint_cache we are not free'ing the
+		 * ns_status data when the inode is free'd. So, in addition to
+		 * checking the inode pointer, we need to make sure the
+		 * (i_generation, i_ino) pair matches as well. In the future
+		 * we might want to add support for lazily walking the rbtree
+		 * to clean it up.
+		 */
+		if (inode->i_ino == status->i_ino &&
+		    inode->i_generation == status->i_generation)
+			return status;
+
+		/* Same inode number is reused, overwrite the ns_status */
+		skip_insert = 1;
+	} else {
+		status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
+		if (!status)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	write_lock(&ns->ns_status_lock);
+
+	if (!skip_insert)
+		insert_ns_status(ns, inode, status);
+
+	status->inode = inode;
+	status->i_ino = inode->i_ino;
+	status->i_generation = inode->i_generation;
+	status->flags = 0UL;
+	write_unlock(&ns->ns_status_lock);
+
+	return status;
+}
-- 
2.13.6

[RFC PATCH v2 3/3] ima: mamespace audit status flags

[Stefan Berger]

From: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>

The iint cache stores whether the file is measured, appraised, audited
etc. This patch moves the IMA_AUDITED flag into the per-namespace
ns_status, enabling IMA audit mechanism to audit the same file each time
it is accessed in a new namespace.

The ns_status is not looked up if the CONFIG_IMA_NS is disabled or if
any of the IMA_NS_STATUS_ACTIONS (currently only IMA_AUDIT) is not
enabled.

Read and write operations on the iint flags is replaced with function
calls. For reading, iint_flags() returns the bitwise AND of iint->flags
and ns_status->flags. The ns_status flags are masked with
IMA_NS_STATUS_FLAGS (currently only IMA_AUDITED). Similarly
set_iint_flags() only writes the masked portion to the ns_status flags,
while the iint flags is set as before. The ns_status parameter added to
ima_audit_measurement() is used with the above functions to query and
set the ns_status flags.

Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
---
 init/Kconfig                      |  4 +++-
 security/integrity/ima/ima.h      | 24 +++++++++++++++++++++++-
 security/integrity/ima/ima_api.c  |  8 +++++---
 security/integrity/ima/ima_main.c | 15 ++++++++++++---
 security/integrity/ima/ima_ns.c   | 22 ++++++++++++++++++++++
 5 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index a1ad5384e081..f792ae235424 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -938,7 +938,9 @@ config IMA_NS
 	help
 	  Allow the creation of IMA namespaces for each mount namespace.
 	  Namespaced IMA data enables having IMA features work separately
-	  for each mount namespace.
+	  for each mount namespace. Currently, only the audit status flags
+	  are stored in the namespace, which allows the same file to be
+	  audited each time it is accessed in a new namespace.
 
 endif # NAMESPACES
 
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e51a39ff75ff..86fd3c1c0caf 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -210,7 +210,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, int pcr);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
-			   const unsigned char *filename);
+			   const unsigned char *filename,
+			   struct ns_status *status);
 int ima_alloc_init_template(struct ima_event_data *event_data,
 			    struct ima_template_entry **entry);
 int ima_store_template(struct ima_template_entry *entry, int violation,
@@ -299,6 +300,9 @@ static inline int ima_read_xattr(struct dentry *dentry,
 
 #endif /* CONFIG_IMA_APPRAISE */
 
+#define IMA_NS_STATUS_ACTIONS   IMA_AUDIT
+#define IMA_NS_STATUS_FLAGS     IMA_AUDITED
+
 int ima_ns_init(void);
 struct ima_namespace;
 int ima_init_namespace(struct ima_namespace *ns);
@@ -306,12 +310,30 @@ int ima_init_namespace(struct ima_namespace *ns);
 #ifdef CONFIG_IMA_NS
 struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
 				    struct inode *inode);
+unsigned long iint_flags(struct integrity_iint_cache *iint,
+			 struct ns_status *status);
+unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+			     struct ns_status *status, unsigned long flags);
 #else
 static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
 						  struct inode *inode)
 {
 	return NULL;
 }
+
+static inline unsigned long iint_flags(struct integrity_iint_cache *iint,
+				       struct ns_status *status)
+{
+	return iint->flags;
+}
+
+static inline unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+					   struct ns_status *status,
+					   unsigned long flags)
+{
+	iint->flags = flags;
+	return flags;
+}
 #endif /* CONFIG_IMA_NS */
 
 /* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7e8db0ea4c0..ee55dfd6afdb 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -304,15 +304,17 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 }
 
 void ima_audit_measurement(struct integrity_iint_cache *iint,
-			   const unsigned char *filename)
+			   const unsigned char *filename,
+			   struct ns_status *status)
 {
 	struct audit_buffer *ab;
 	char hash[(iint->ima_hash->length * 2) + 1];
 	const char *algo_name = hash_algo_name[iint->ima_hash->algo];
 	char algo_hash[sizeof(hash) + strlen(algo_name) + 2];
 	int i;
+	unsigned long flags = iint_flags(iint, status);
 
-	if (iint->flags & IMA_AUDITED)
+	if (flags & IMA_AUDITED)
 		return;
 
 	for (i = 0; i < iint->ima_hash->length; i++)
@@ -333,7 +335,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
 	audit_log_task_info(ab, current);
 	audit_log_end(ab);
 
-	iint->flags |= IMA_AUDITED;
+	set_iint_flags(iint, status, flags | IMA_AUDITED);
 }
 
 /*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 770654694efc..9328e57d3f09 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -164,6 +164,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 {
 	struct inode *inode = file_inode(file);
 	struct integrity_iint_cache *iint = NULL;
+	struct ns_status *status = NULL;
 	struct ima_template_desc *template_desc;
 	char *pathbuf = NULL;
 	char filename[NAME_MAX];
@@ -174,6 +175,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	int xattr_len = 0;
 	bool violation_check;
 	enum hash_algo hash_algo;
+	unsigned long flags;
 
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
 		return 0;
@@ -200,6 +202,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		iint = integrity_inode_get(inode);
 		if (!iint)
 			goto out;
+
+		if (action & IMA_NS_STATUS_ACTIONS) {
+			status = ima_get_ns_status(get_current_ns(), inode);
+			if (IS_ERR(status))
+				goto out;
+		}
 	}
 
 	if (violation_check) {
@@ -215,9 +223,10 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
 	 *  IMA_AUDIT, IMA_AUDITED)
 	 */
-	iint->flags |= action;
+	flags = iint_flags(iint, status);
+	flags = set_iint_flags(iint, status, flags | action);
 	action &= IMA_DO_MASK;
-	action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
+	action &= ~((flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
 
 	/* If target pcr is already measured, unset IMA_MEASURE action */
 	if ((action & IMA_MEASURE) && (iint->measured_pcrs & (0x1 << pcr)))
@@ -252,7 +261,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		rc = ima_appraise_measurement(func, iint, file, pathname,
 					      xattr_value, xattr_len, opened);
 	if (action & IMA_AUDIT)
-		ima_audit_measurement(iint, pathname);
+		ima_audit_measurement(iint, pathname, status);
 
 	if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
 		rc = 0;
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 03acf1431868..fc1d88c69b54 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -206,3 +206,25 @@ struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
 
 	return status;
 }
+
+#define IMA_NS_STATUS_ACTIONS	IMA_AUDIT
+#define IMA_NS_STATUS_FLAGS	IMA_AUDITED
+
+unsigned long iint_flags(struct integrity_iint_cache *iint,
+			 struct ns_status *status)
+{
+	if (!status)
+		return iint->flags;
+
+	return (iint->flags & ~IMA_NS_STATUS_FLAGS) | (status->flags & IMA_NS_STATUS_FLAGS);
+}
+
+unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+			     struct ns_status *status, unsigned long flags)
+{
+	iint->flags = flags;
+	if (status)
+		status->flags = flags & IMA_NS_STATUS_FLAGS;
+
+	return flags;
+}
-- 
2.13.6

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Eric W. Biederman]

Stefan Berger <stefanb@linux.vnet.ibm.com> writes:

> From: Yuqiong Sun <suny@us.ibm.com>
>
> Add new CONFIG_IMA_NS config option.  Let clone() create a new IMA
> namespace upon CLONE_NEWNS flag. Add ima_ns data structure in nsproxy.
> ima_ns is allocated and freed upon IMA namespace creation and exit.
> Currently, the ima_ns contains no useful IMA data but only a dummy
> interface. This patch creates the framework for namespacing the different
> aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal).

IMA is not path based.  The only thing that belongs to a mount
namespace are paths.  Therefore IMA is completely inappropriate to
be joint with a mount namespace.

I saw that Serge even recently mentioned that you need to take
this aspect of the changes back to the drawing board.  With my
namespace maintainer hat on I repeat that.

>From a 10,000 foot view I can already tell that this is hopeless.
So for binding IMA namspaces and CLONE_NEWNS:

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I am not nacking IMA namespacing just inappropriately tying ima
namespaces to mount namespaces.  These should be completely separate
entities.

Eric


> Changelog:
> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> * Use existing ima.h headers
> * Move the ima_namespace.c to security/integrity/ima/ima_ns.c
> * Fix typo INFO->INO
> * Each namespace free's itself, removed recursively free'ing
>   until init_ima_ns from free_ima_ns()
> * Moved ima_init_ns and related functions into own file that is
>   always compiled
> * Fixed putting of imans->parent
> * Move IMA namespace creation from nsproxy into mount namespace
>   code
>
> Signed-off-by: Yuqiong Sun <suny@us.ibm.com>
> Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  fs/mount.h                               | 14 -----
>  fs/namespace.c                           | 29 ++++++++--
>  include/linux/ima.h                      | 67 +++++++++++++++++++++++
>  include/linux/mount.h                    | 20 ++++++-
>  init/Kconfig                             |  8 +++
>  kernel/nsproxy.c                         |  1 +
>  security/integrity/ima/Makefile          |  3 +-
>  security/integrity/ima/ima.h             |  4 ++
>  security/integrity/ima/ima_init.c        |  4 ++
>  security/integrity/ima/ima_init_ima_ns.c | 38 +++++++++++++
>  security/integrity/ima/ima_ns.c          | 91 ++++++++++++++++++++++++++++++++
>  11 files changed, 260 insertions(+), 19 deletions(-)
>  create mode 100644 security/integrity/ima/ima_init_ima_ns.c
>  create mode 100644 security/integrity/ima/ima_ns.c
>
> diff --git a/fs/mount.h b/fs/mount.h
> index f39bc9da4d73..e19ebde97756 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -5,20 +5,6 @@
>  #include <linux/ns_common.h>
>  #include <linux/fs_pin.h>
>  
> -struct mnt_namespace {
> -	atomic_t		count;
> -	struct ns_common	ns;
> -	struct mount *	root;
> -	struct list_head	list;
> -	struct user_namespace	*user_ns;
> -	struct ucounts		*ucounts;
> -	u64			seq;	/* Sequence number to prevent loops */
> -	wait_queue_head_t poll;
> -	u64 event;
> -	unsigned int		mounts; /* # of mounts in the namespace */
> -	unsigned int		pending_mounts;
> -} __randomize_layout;
> -
>  struct mnt_pcp {
>  	int mnt_count;
>  	int mnt_writers;
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 9d1374ab6e06..7f886c02278b 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -26,6 +26,7 @@
>  #include <linux/bootmem.h>
>  #include <linux/task_work.h>
>  #include <linux/sched/task.h>
> +#include <linux/ima.h>
>  
>  #include "pnode.h"
>  #include "internal.h"
> @@ -2858,6 +2859,7 @@ static void dec_mnt_namespaces(struct ucounts *ucounts)
>  
>  static void free_mnt_ns(struct mnt_namespace *ns)
>  {
> +	put_ima_ns(ns->ima_ns);
>  	ns_free_inum(&ns->ns);
>  	dec_mnt_namespaces(ns->ucounts);
>  	put_user_ns(ns->user_ns);
> @@ -2873,11 +2875,13 @@ static void free_mnt_ns(struct mnt_namespace *ns)
>   */
>  static atomic64_t mnt_ns_seq = ATOMIC64_INIT(1);
>  
> -static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
> +static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns,
> +					  struct ima_namespace *ima_ns)
>  {
>  	struct mnt_namespace *new_ns;
>  	struct ucounts *ucounts;
>  	int ret;
> +	int err;
>  
>  	ucounts = inc_mnt_namespaces(user_ns);
>  	if (!ucounts)
> @@ -2894,6 +2898,20 @@ static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
>  		dec_mnt_namespaces(ucounts);
>  		return ERR_PTR(ret);
>  	}
> +
> +	if (ima_ns == NULL) {
> +		new_ns->ima_ns = get_ima_ns(&init_ima_ns);
> +	} else {
> +		new_ns->ima_ns = copy_ima(user_ns, ima_ns);
> +		if (IS_ERR(new_ns->ima_ns)) {
> +			err = PTR_ERR(new_ns->ima_ns);
> +			ns_free_inum(&new_ns->ns);
> +			kfree(new_ns);
> +			dec_mnt_namespaces(ucounts);
> +			return ERR_PTR(err);
> +		}
> +	}
> +
>  	new_ns->ns.ops = &mntns_operations;
>  	new_ns->seq = atomic64_add_return(1, &mnt_ns_seq);
>  	atomic_set(&new_ns->count, 1);
> @@ -2920,6 +2938,7 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
>  	int copy_flags;
>  
>  	BUG_ON(!ns);
> +	BUG_ON(!ns->ima_ns);
>  
>  	if (likely(!(flags & CLONE_NEWNS))) {
>  		get_mnt_ns(ns);
> @@ -2928,7 +2947,7 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
>  
>  	old = ns->root;
>  
> -	new_ns = alloc_mnt_ns(user_ns);
> +	new_ns = alloc_mnt_ns(user_ns, ns->ima_ns);
>  	if (IS_ERR(new_ns))
>  		return new_ns;
>  
> @@ -2989,7 +3008,8 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
>   */
>  static struct mnt_namespace *create_mnt_ns(struct vfsmount *m)
>  {
> -	struct mnt_namespace *new_ns = alloc_mnt_ns(&init_user_ns);
> +	struct mnt_namespace *new_ns = alloc_mnt_ns(&init_user_ns,
> +						    NULL);
>  	if (!IS_ERR(new_ns)) {
>  		struct mount *mnt = real_mount(m);
>  		mnt->mnt_ns = new_ns;
> @@ -3497,6 +3517,9 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns)
>  	set_fs_root(fs, &root);
>  
>  	path_put(&root);
> +
> +	imans_install(nsproxy, ns);
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 0e4647e0eb60..fd150dfde277 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -12,6 +12,7 @@
>  
>  #include <linux/fs.h>
>  #include <linux/kexec.h>
> +#include <linux/mount.h>
>  struct linux_binprm;
>  
>  #ifdef CONFIG_IMA
> @@ -105,4 +106,70 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
>  	return 0;
>  }
>  #endif /* CONFIG_IMA_APPRAISE */
> +
> +struct ima_namespace {
> +	struct kref kref;
> +	struct user_namespace *user_ns;
> +	struct ima_namespace *parent;
> +};
> +
> +extern struct ima_namespace init_ima_ns;
> +
> +void imans_install(struct nsproxy *nsproxy, struct ns_common *new);
> +
> +static inline struct ima_namespace *to_ima_ns(struct ns_common *ns)
> +{
> +	return container_of(ns, struct mnt_namespace, ns)->ima_ns;
> +}
> +
> +#ifdef CONFIG_IMA_NS
> +
> +void free_ima_ns(struct kref *kref);
> +
> +static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
> +{
> +	BUG_ON(!ns);
> +	if (ns)
> +		kref_get(&ns->kref);
> +	return ns;
> +}
> +
> +static inline void put_ima_ns(struct ima_namespace *ns)
> +{
> +	BUG_ON(!ns);
> +	if (ns)
> +		kref_put(&ns->kref, free_ima_ns);
> +}
> +
> +struct ima_namespace *copy_ima(struct user_namespace *user_ns,
> +			       struct ima_namespace *old_ns);
> +
> +static inline struct ima_namespace *get_current_ns(void)
> +{
> +	return current->nsproxy->mnt_ns->ima_ns;
> +}
> +
> +#else
> +
> +static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
> +{
> +	return ns;
> +}
> +
> +static inline void put_ima_ns(struct ima_namespace *ns)
> +{
> +	return;
> +}
> +
> +static inline struct ima_namespace *copy_ima(struct user_namespace *user_ns,
> +					     struct ima_namespace *old_ns)
> +{
> +	return old_ns;
> +}
> +
> +static inline struct ima_namespace *get_current_ns(void)
> +{
> +	return NULL;
> +}
> +#endif /* CONFIG_IMA_NS */
>  #endif /* _LINUX_IMA_H */
> diff --git a/include/linux/mount.h b/include/linux/mount.h
> index 45b1f56c6c2f..361c962ebd3d 100644
> --- a/include/linux/mount.h
> +++ b/include/linux/mount.h
> @@ -16,11 +16,29 @@
>  #include <linux/spinlock.h>
>  #include <linux/seqlock.h>
>  #include <linux/atomic.h>
> +#include <linux/ns_common.h>
> +#include <linux/wait.h>
>  
>  struct super_block;
>  struct vfsmount;
>  struct dentry;
> -struct mnt_namespace;
> +struct ima_namespace;
> +
> +struct mnt_namespace {
> +	atomic_t		count;
> +	struct ns_common	ns;
> +	struct mount *	root;
> +	struct list_head	list;
> +	struct user_namespace	*user_ns;
> +	struct ucounts		*ucounts;
> +	u64			seq;	/* Sequence number to prevent loops */
> +	wait_queue_head_t poll;
> +	u64 event;
> +	unsigned int		mounts; /* # of mounts in the namespace */
> +	unsigned int		pending_mounts;
> +	struct ima_namespace    *ima_ns;
> +} __randomize_layout;
> +
>  
>  #define MNT_NOSUID	0x01
>  #define MNT_NODEV	0x02
> diff --git a/init/Kconfig b/init/Kconfig
> index a9a2e2c86671..a1ad5384e081 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -931,6 +931,14 @@ config NET_NS
>  	help
>  	  Allow user space to create what appear to be multiple instances
>  	  of the network stack.
> +config IMA_NS
> +	bool "IMA namespace"
> +	depends on IMA
> +	default y
> +	help
> +	  Allow the creation of IMA namespaces for each mount namespace.
> +	  Namespaced IMA data enables having IMA features work separately
> +	  for each mount namespace.
>  
>  endif # NAMESPACES
>  
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d330059a..7d1a35362186 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -27,6 +27,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/cgroup.h>
>  #include <linux/perf_event.h>
> +#include <linux/ima.h>
>  
>  static struct kmem_cache *nsproxy_cachep;
>  
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index d921dc4f9eb0..cc60f726e651 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -7,7 +7,8 @@
>  obj-$(CONFIG_IMA) += ima.o
>  
>  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> -	 ima_policy.o ima_template.o ima_template_lib.o
> +	 ima_policy.o ima_template.o ima_template_lib.o ima_init_ima_ns.o
>  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> +ima-$(CONFIG_IMA_NS) += ima_ns.o
>  ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
>  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d52b487ad259..e98c11c7cf75 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -291,6 +291,10 @@ static inline int ima_read_xattr(struct dentry *dentry,
>  
>  #endif /* CONFIG_IMA_APPRAISE */
>  
> +int ima_ns_init(void);
> +struct ima_namespace;
> +int ima_init_namespace(struct ima_namespace *ns);
> +
>  /* LSM based policy rules require audit */
>  #ifdef CONFIG_IMA_LSM_RULES
>  
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 2967d497a665..7f884a71fa1c 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -137,5 +137,9 @@ int __init ima_init(void)
>  
>  	ima_init_policy();
>  
> +	rc = ima_ns_init();
> +	if (rc != 0)
> +		return rc;
> +
>  	return ima_fs_init();
>  }
> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
> new file mode 100644
> index 000000000000..4b081dbfac07
> --- /dev/null
> +++ b/security/integrity/ima/ima_init_ima_ns.c
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2016-2018 IBM Corporation
> + * Author: Yuqiong Sun <suny@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#include <linux/export.h>
> +#include <linux/user_namespace.h>
> +#include <linux/ima.h>
> +
> +int ima_init_namespace(struct ima_namespace *ns)
> +{
> +	return 0;
> +}
> +
> +int __init ima_ns_init(void)
> +{
> +	return ima_init_namespace(&init_ima_ns);
> +}
> +
> +struct ima_namespace init_ima_ns = {
> +	.kref = KREF_INIT(2),
> +	.user_ns = &init_user_ns,
> +	.parent = NULL,
> +};
> +EXPORT_SYMBOL(init_ima_ns);
> +
> +void imans_install(struct nsproxy *nsproxy, struct ns_common *new)
> +{
> +	struct ima_namespace *ns = to_ima_ns(new);
> +
> +	get_ima_ns(ns);
> +	put_ima_ns(nsproxy->mnt_ns->ima_ns);
> +	nsproxy->mnt_ns->ima_ns = ns;
> +}
> diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
> new file mode 100644
> index 000000000000..7ab4322c88ae
> --- /dev/null
> +++ b/security/integrity/ima/ima_ns.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (C) 2016-2018 IBM Corporation
> + * Author: Yuqiong Sun <suny@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#include <linux/user_namespace.h>
> +#include <linux/kref.h>
> +#include <linux/slab.h>
> +#include <linux/ima.h>
> +#include <linux/mount.h>
> +
> +#include "ima.h"
> +
> +static struct ima_namespace *create_ima_ns(void)
> +{
> +	struct ima_namespace *ima_ns;
> +
> +	ima_ns = kmalloc(sizeof(*ima_ns), GFP_KERNEL);
> +	if (ima_ns)
> +		kref_init(&ima_ns->kref);
> +
> +	return ima_ns;
> +}
> +
> +/**
> + * Clone a new ns copying an original ima namespace, setting refcount to 1
> + *
> + * @old_ns: old ima namespace to clone
> + * @user_ns: user namespace that current task runs in
> + * Return ERR_PTR(-ENOMEM) on error (failure to kmalloc), new ns otherwise
> + */
> +static struct ima_namespace *clone_ima_ns(struct user_namespace *user_ns,
> +					  struct ima_namespace *old_ns)
> +{
> +	struct ima_namespace *ns;
> +
> +	ns = create_ima_ns();
> +	if (!ns)
> +		return ERR_PTR(-ENOMEM);
> +
> +	get_ima_ns(old_ns);
> +	ns->parent = old_ns;
> +	ns->user_ns = get_user_ns(user_ns);
> +
> +	ima_init_namespace(ns);
> +
> +	return ns;
> +}
> +
> +/**
> + * Copy task's ima namespace, or clone it if flags specifies CLONE_NEWNS.
> + *
> + * @flags: flags used in the clone syscall
> + * @user_ns: user namespace that current task runs in
> + * @old_ns: old ima namespace to clone
> + */
> +
> +struct ima_namespace *copy_ima(struct user_namespace *user_ns,
> +			       struct ima_namespace *old_ns)
> +{
> +	struct ima_namespace *new_ns;
> +
> +	BUG_ON(!old_ns);
> +	get_ima_ns(old_ns);
> +
> +	new_ns = clone_ima_ns(user_ns, old_ns);
> +	put_ima_ns(old_ns);
> +
> +	return new_ns;
> +}
> +
> +static void destroy_ima_ns(struct ima_namespace *ns)
> +{
> +	put_user_ns(ns->user_ns);
> +	put_ima_ns(ns->parent);
> +	kfree(ns);
> +}
> +
> +void free_ima_ns(struct kref *kref)
> +{
> +	struct ima_namespace *ns;
> +
> +	ns = container_of(kref, struct ima_namespace, kref);
> +	BUG_ON(ns == &init_ima_ns);
> +
> +	destroy_ima_ns(ns);
> +}

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

On 03/15/2018 06:40 AM, Eric W. Biederman wrote:
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>
>> From: Yuqiong Sun <suny@us.ibm.com>
>>
>> Add new CONFIG_IMA_NS config option.  Let clone() create a new IMA
>> namespace upon CLONE_NEWNS flag. Add ima_ns data structure in nsproxy.
>> ima_ns is allocated and freed upon IMA namespace creation and exit.
>> Currently, the ima_ns contains no useful IMA data but only a dummy
>> interface. This patch creates the framework for namespacing the different
>> aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal).
> IMA is not path based.  The only thing that belongs to a mount
> namespace are paths.  Therefore IMA is completely inappropriate to
> be joint with a mount namespace.

IMA measures the files described by these paths. The files also may hold 
signatures (security.ima xattr) needed for IMA appraisal.

>
> I saw that Serge even recently mentioned that you need to take
> this aspect of the changes back to the drawing board.  With my
> namespace maintainer hat on I repeat that.

Drawing board is here now (tuning on the text...):

http://kernsec.org/wiki/index.php/IMA_Namespacing_design_considerations

>
>  From a 10,000 foot view I can already tell that this is hopeless.
> So for binding IMA namspaces and CLONE_NEWNS:
>
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> I am not nacking IMA namespacing just inappropriately tying ima
> namespaces to mount namespaces.  These should be completely separate
> entities.

Let's say we go down the road of spawning it independently. Can we use 
the unused clone flag 0x1000? Or should we come up with new 
unshare2()/clone2() syscalls to extend the clone bits to 64 bit? Or use 
a sysfs/securityfs file to spawn a new IMA namespace? Make this a 
generic file not an IMA specific one?

    Stefan

>
> Eric
>
>
>> Changelog:
>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>> * Use existing ima.h headers
>> * Move the ima_namespace.c to security/integrity/ima/ima_ns.c
>> * Fix typo INFO->INO
>> * Each namespace free's itself, removed recursively free'ing
>>    until init_ima_ns from free_ima_ns()
>> * Moved ima_init_ns and related functions into own file that is
>>    always compiled
>> * Fixed putting of imans->parent
>> * Move IMA namespace creation from nsproxy into mount namespace
>>    code
>>
>> Signed-off-by: Yuqiong Sun <suny@us.ibm.com>
>> Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   fs/mount.h                               | 14 -----
>>   fs/namespace.c                           | 29 ++++++++--
>>   include/linux/ima.h                      | 67 +++++++++++++++++++++++
>>   include/linux/mount.h                    | 20 ++++++-
>>   init/Kconfig                             |  8 +++
>>   kernel/nsproxy.c                         |  1 +
>>   security/integrity/ima/Makefile          |  3 +-
>>   security/integrity/ima/ima.h             |  4 ++
>>   security/integrity/ima/ima_init.c        |  4 ++
>>   security/integrity/ima/ima_init_ima_ns.c | 38 +++++++++++++
>>   security/integrity/ima/ima_ns.c          | 91 ++++++++++++++++++++++++++++++++
>>   11 files changed, 260 insertions(+), 19 deletions(-)
>>   create mode 100644 security/integrity/ima/ima_init_ima_ns.c
>>   create mode 100644 security/integrity/ima/ima_ns.c
>>
>> diff --git a/fs/mount.h b/fs/mount.h
>> index f39bc9da4d73..e19ebde97756 100644
>> --- a/fs/mount.h
>> +++ b/fs/mount.h
>> @@ -5,20 +5,6 @@
>>   #include <linux/ns_common.h>
>>   #include <linux/fs_pin.h>
>>   
>> -struct mnt_namespace {
>> -	atomic_t		count;
>> -	struct ns_common	ns;
>> -	struct mount *	root;
>> -	struct list_head	list;
>> -	struct user_namespace	*user_ns;
>> -	struct ucounts		*ucounts;
>> -	u64			seq;	/* Sequence number to prevent loops */
>> -	wait_queue_head_t poll;
>> -	u64 event;
>> -	unsigned int		mounts; /* # of mounts in the namespace */
>> -	unsigned int		pending_mounts;
>> -} __randomize_layout;
>> -
>>   struct mnt_pcp {
>>   	int mnt_count;
>>   	int mnt_writers;
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index 9d1374ab6e06..7f886c02278b 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/bootmem.h>
>>   #include <linux/task_work.h>
>>   #include <linux/sched/task.h>
>> +#include <linux/ima.h>
>>   
>>   #include "pnode.h"
>>   #include "internal.h"
>> @@ -2858,6 +2859,7 @@ static void dec_mnt_namespaces(struct ucounts *ucounts)
>>   
>>   static void free_mnt_ns(struct mnt_namespace *ns)
>>   {
>> +	put_ima_ns(ns->ima_ns);
>>   	ns_free_inum(&ns->ns);
>>   	dec_mnt_namespaces(ns->ucounts);
>>   	put_user_ns(ns->user_ns);
>> @@ -2873,11 +2875,13 @@ static void free_mnt_ns(struct mnt_namespace *ns)
>>    */
>>   static atomic64_t mnt_ns_seq = ATOMIC64_INIT(1);
>>   
>> -static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
>> +static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns,
>> +					  struct ima_namespace *ima_ns)
>>   {
>>   	struct mnt_namespace *new_ns;
>>   	struct ucounts *ucounts;
>>   	int ret;
>> +	int err;
>>   
>>   	ucounts = inc_mnt_namespaces(user_ns);
>>   	if (!ucounts)
>> @@ -2894,6 +2898,20 @@ static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
>>   		dec_mnt_namespaces(ucounts);
>>   		return ERR_PTR(ret);
>>   	}
>> +
>> +	if (ima_ns == NULL) {
>> +		new_ns->ima_ns = get_ima_ns(&init_ima_ns);
>> +	} else {
>> +		new_ns->ima_ns = copy_ima(user_ns, ima_ns);
>> +		if (IS_ERR(new_ns->ima_ns)) {
>> +			err = PTR_ERR(new_ns->ima_ns);
>> +			ns_free_inum(&new_ns->ns);
>> +			kfree(new_ns);
>> +			dec_mnt_namespaces(ucounts);
>> +			return ERR_PTR(err);
>> +		}
>> +	}
>> +
>>   	new_ns->ns.ops = &mntns_operations;
>>   	new_ns->seq = atomic64_add_return(1, &mnt_ns_seq);
>>   	atomic_set(&new_ns->count, 1);
>> @@ -2920,6 +2938,7 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
>>   	int copy_flags;
>>   
>>   	BUG_ON(!ns);
>> +	BUG_ON(!ns->ima_ns);
>>   
>>   	if (likely(!(flags & CLONE_NEWNS))) {
>>   		get_mnt_ns(ns);
>> @@ -2928,7 +2947,7 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
>>   
>>   	old = ns->root;
>>   
>> -	new_ns = alloc_mnt_ns(user_ns);
>> +	new_ns = alloc_mnt_ns(user_ns, ns->ima_ns);
>>   	if (IS_ERR(new_ns))
>>   		return new_ns;
>>   
>> @@ -2989,7 +3008,8 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
>>    */
>>   static struct mnt_namespace *create_mnt_ns(struct vfsmount *m)
>>   {
>> -	struct mnt_namespace *new_ns = alloc_mnt_ns(&init_user_ns);
>> +	struct mnt_namespace *new_ns = alloc_mnt_ns(&init_user_ns,
>> +						    NULL);
>>   	if (!IS_ERR(new_ns)) {
>>   		struct mount *mnt = real_mount(m);
>>   		mnt->mnt_ns = new_ns;
>> @@ -3497,6 +3517,9 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns)
>>   	set_fs_root(fs, &root);
>>   
>>   	path_put(&root);
>> +
>> +	imans_install(nsproxy, ns);
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> index 0e4647e0eb60..fd150dfde277 100644
>> --- a/include/linux/ima.h
>> +++ b/include/linux/ima.h
>> @@ -12,6 +12,7 @@
>>   
>>   #include <linux/fs.h>
>>   #include <linux/kexec.h>
>> +#include <linux/mount.h>
>>   struct linux_binprm;
>>   
>>   #ifdef CONFIG_IMA
>> @@ -105,4 +106,70 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
>>   	return 0;
>>   }
>>   #endif /* CONFIG_IMA_APPRAISE */
>> +
>> +struct ima_namespace {
>> +	struct kref kref;
>> +	struct user_namespace *user_ns;
>> +	struct ima_namespace *parent;
>> +};
>> +
>> +extern struct ima_namespace init_ima_ns;
>> +
>> +void imans_install(struct nsproxy *nsproxy, struct ns_common *new);
>> +
>> +static inline struct ima_namespace *to_ima_ns(struct ns_common *ns)
>> +{
>> +	return container_of(ns, struct mnt_namespace, ns)->ima_ns;
>> +}
>> +
>> +#ifdef CONFIG_IMA_NS
>> +
>> +void free_ima_ns(struct kref *kref);
>> +
>> +static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
>> +{
>> +	BUG_ON(!ns);
>> +	if (ns)
>> +		kref_get(&ns->kref);
>> +	return ns;
>> +}
>> +
>> +static inline void put_ima_ns(struct ima_namespace *ns)
>> +{
>> +	BUG_ON(!ns);
>> +	if (ns)
>> +		kref_put(&ns->kref, free_ima_ns);
>> +}
>> +
>> +struct ima_namespace *copy_ima(struct user_namespace *user_ns,
>> +			       struct ima_namespace *old_ns);
>> +
>> +static inline struct ima_namespace *get_current_ns(void)
>> +{
>> +	return current->nsproxy->mnt_ns->ima_ns;
>> +}
>> +
>> +#else
>> +
>> +static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
>> +{
>> +	return ns;
>> +}
>> +
>> +static inline void put_ima_ns(struct ima_namespace *ns)
>> +{
>> +	return;
>> +}
>> +
>> +static inline struct ima_namespace *copy_ima(struct user_namespace *user_ns,
>> +					     struct ima_namespace *old_ns)
>> +{
>> +	return old_ns;
>> +}
>> +
>> +static inline struct ima_namespace *get_current_ns(void)
>> +{
>> +	return NULL;
>> +}
>> +#endif /* CONFIG_IMA_NS */
>>   #endif /* _LINUX_IMA_H */
>> diff --git a/include/linux/mount.h b/include/linux/mount.h
>> index 45b1f56c6c2f..361c962ebd3d 100644
>> --- a/include/linux/mount.h
>> +++ b/include/linux/mount.h
>> @@ -16,11 +16,29 @@
>>   #include <linux/spinlock.h>
>>   #include <linux/seqlock.h>
>>   #include <linux/atomic.h>
>> +#include <linux/ns_common.h>
>> +#include <linux/wait.h>
>>   
>>   struct super_block;
>>   struct vfsmount;
>>   struct dentry;
>> -struct mnt_namespace;
>> +struct ima_namespace;
>> +
>> +struct mnt_namespace {
>> +	atomic_t		count;
>> +	struct ns_common	ns;
>> +	struct mount *	root;
>> +	struct list_head	list;
>> +	struct user_namespace	*user_ns;
>> +	struct ucounts		*ucounts;
>> +	u64			seq;	/* Sequence number to prevent loops */
>> +	wait_queue_head_t poll;
>> +	u64 event;
>> +	unsigned int		mounts; /* # of mounts in the namespace */
>> +	unsigned int		pending_mounts;
>> +	struct ima_namespace    *ima_ns;
>> +} __randomize_layout;
>> +
>>   
>>   #define MNT_NOSUID	0x01
>>   #define MNT_NODEV	0x02
>> diff --git a/init/Kconfig b/init/Kconfig
>> index a9a2e2c86671..a1ad5384e081 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -931,6 +931,14 @@ config NET_NS
>>   	help
>>   	  Allow user space to create what appear to be multiple instances
>>   	  of the network stack.
>> +config IMA_NS
>> +	bool "IMA namespace"
>> +	depends on IMA
>> +	default y
>> +	help
>> +	  Allow the creation of IMA namespaces for each mount namespace.
>> +	  Namespaced IMA data enables having IMA features work separately
>> +	  for each mount namespace.
>>   
>>   endif # NAMESPACES
>>   
>> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
>> index f6c5d330059a..7d1a35362186 100644
>> --- a/kernel/nsproxy.c
>> +++ b/kernel/nsproxy.c
>> @@ -27,6 +27,7 @@
>>   #include <linux/syscalls.h>
>>   #include <linux/cgroup.h>
>>   #include <linux/perf_event.h>
>> +#include <linux/ima.h>
>>   
>>   static struct kmem_cache *nsproxy_cachep;
>>   
>> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
>> index d921dc4f9eb0..cc60f726e651 100644
>> --- a/security/integrity/ima/Makefile
>> +++ b/security/integrity/ima/Makefile
>> @@ -7,7 +7,8 @@
>>   obj-$(CONFIG_IMA) += ima.o
>>   
>>   ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
>> -	 ima_policy.o ima_template.o ima_template_lib.o
>> +	 ima_policy.o ima_template.o ima_template_lib.o ima_init_ima_ns.o
>>   ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
>> +ima-$(CONFIG_IMA_NS) += ima_ns.o
>>   ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
>>   obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index d52b487ad259..e98c11c7cf75 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -291,6 +291,10 @@ static inline int ima_read_xattr(struct dentry *dentry,
>>   
>>   #endif /* CONFIG_IMA_APPRAISE */
>>   
>> +int ima_ns_init(void);
>> +struct ima_namespace;
>> +int ima_init_namespace(struct ima_namespace *ns);
>> +
>>   /* LSM based policy rules require audit */
>>   #ifdef CONFIG_IMA_LSM_RULES
>>   
>> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
>> index 2967d497a665..7f884a71fa1c 100644
>> --- a/security/integrity/ima/ima_init.c
>> +++ b/security/integrity/ima/ima_init.c
>> @@ -137,5 +137,9 @@ int __init ima_init(void)
>>   
>>   	ima_init_policy();
>>   
>> +	rc = ima_ns_init();
>> +	if (rc != 0)
>> +		return rc;
>> +
>>   	return ima_fs_init();
>>   }
>> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
>> new file mode 100644
>> index 000000000000..4b081dbfac07
>> --- /dev/null
>> +++ b/security/integrity/ima/ima_init_ima_ns.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Copyright (C) 2016-2018 IBM Corporation
>> + * Author: Yuqiong Sun <suny@us.ibm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, version 2 of the License.
>> + */
>> +
>> +#include <linux/export.h>
>> +#include <linux/user_namespace.h>
>> +#include <linux/ima.h>
>> +
>> +int ima_init_namespace(struct ima_namespace *ns)
>> +{
>> +	return 0;
>> +}
>> +
>> +int __init ima_ns_init(void)
>> +{
>> +	return ima_init_namespace(&init_ima_ns);
>> +}
>> +
>> +struct ima_namespace init_ima_ns = {
>> +	.kref = KREF_INIT(2),
>> +	.user_ns = &init_user_ns,
>> +	.parent = NULL,
>> +};
>> +EXPORT_SYMBOL(init_ima_ns);
>> +
>> +void imans_install(struct nsproxy *nsproxy, struct ns_common *new)
>> +{
>> +	struct ima_namespace *ns = to_ima_ns(new);
>> +
>> +	get_ima_ns(ns);
>> +	put_ima_ns(nsproxy->mnt_ns->ima_ns);
>> +	nsproxy->mnt_ns->ima_ns = ns;
>> +}
>> diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
>> new file mode 100644
>> index 000000000000..7ab4322c88ae
>> --- /dev/null
>> +++ b/security/integrity/ima/ima_ns.c
>> @@ -0,0 +1,91 @@
>> +/*
>> + * Copyright (C) 2016-2018 IBM Corporation
>> + * Author: Yuqiong Sun <suny@us.ibm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, version 2 of the License.
>> + */
>> +
>> +#include <linux/user_namespace.h>
>> +#include <linux/kref.h>
>> +#include <linux/slab.h>
>> +#include <linux/ima.h>
>> +#include <linux/mount.h>
>> +
>> +#include "ima.h"
>> +
>> +static struct ima_namespace *create_ima_ns(void)
>> +{
>> +	struct ima_namespace *ima_ns;
>> +
>> +	ima_ns = kmalloc(sizeof(*ima_ns), GFP_KERNEL);
>> +	if (ima_ns)
>> +		kref_init(&ima_ns->kref);
>> +
>> +	return ima_ns;
>> +}
>> +
>> +/**
>> + * Clone a new ns copying an original ima namespace, setting refcount to 1
>> + *
>> + * @old_ns: old ima namespace to clone
>> + * @user_ns: user namespace that current task runs in
>> + * Return ERR_PTR(-ENOMEM) on error (failure to kmalloc), new ns otherwise
>> + */
>> +static struct ima_namespace *clone_ima_ns(struct user_namespace *user_ns,
>> +					  struct ima_namespace *old_ns)
>> +{
>> +	struct ima_namespace *ns;
>> +
>> +	ns = create_ima_ns();
>> +	if (!ns)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	get_ima_ns(old_ns);
>> +	ns->parent = old_ns;
>> +	ns->user_ns = get_user_ns(user_ns);
>> +
>> +	ima_init_namespace(ns);
>> +
>> +	return ns;
>> +}
>> +
>> +/**
>> + * Copy task's ima namespace, or clone it if flags specifies CLONE_NEWNS.
>> + *
>> + * @flags: flags used in the clone syscall
>> + * @user_ns: user namespace that current task runs in
>> + * @old_ns: old ima namespace to clone
>> + */
>> +
>> +struct ima_namespace *copy_ima(struct user_namespace *user_ns,
>> +			       struct ima_namespace *old_ns)
>> +{
>> +	struct ima_namespace *new_ns;
>> +
>> +	BUG_ON(!old_ns);
>> +	get_ima_ns(old_ns);
>> +
>> +	new_ns = clone_ima_ns(user_ns, old_ns);
>> +	put_ima_ns(old_ns);
>> +
>> +	return new_ns;
>> +}
>> +
>> +static void destroy_ima_ns(struct ima_namespace *ns)
>> +{
>> +	put_user_ns(ns->user_ns);
>> +	put_ima_ns(ns->parent);
>> +	kfree(ns);
>> +}
>> +
>> +void free_ima_ns(struct kref *kref)
>> +{
>> +	struct ima_namespace *ns;
>> +
>> +	ns = container_of(kref, struct ima_namespace, kref);
>> +	BUG_ON(ns == &init_ima_ns);
>> +
>> +	destroy_ima_ns(ns);
>> +}

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[James Bottomley]

On Thu, 2018-03-15 at 11:26 -0400, Stefan Berger wrote:
> On 03/15/2018 06:40 AM, Eric W. Biederman wrote:
> > 
> > Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
> > 
> > > 
> > > From: Yuqiong Sun <suny@us.ibm.com>
> > > 
> > > Add new CONFIG_IMA_NS config option.  Let clone() create a new
> > > IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure in
> > > nsproxy.  ima_ns is allocated and freed upon IMA namespace
> > > creation and exit.  Currently, the ima_ns contains no useful IMA
> > > data but only a dummy interface. This patch creates the framework
> > > for namespacing the different aspects of IMA (eg. IMA-audit, IMA-
> > > measurement, IMA-appraisal).
> > IMA is not path based.  The only thing that belongs to a mount
> > namespace are paths.  Therefore IMA is completely inappropriate to
> > be joint with a mount namespace.

Just to be clear: The mount namespace is not only about paths it's also
about subtree properties.  However, the point still stands that IMA has
a dependency on neither.

> IMA measures the files described by these paths. The files also may
> hold signatures (security.ima xattr) needed for IMA appraisal.

The xattr is an inode property, which isn't namespaced by the mount_ns.

When we had this discussion last year, we talked about possibly using
the user_ns instead.  It makes sense because for IMA signatures you're
going to need some type of keyring namespace and there's already one
hanging off the user_ns:

commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
Author: David Howells <dhowells@redhat.com>
Date:   Tue Sep 24 10:35:19 2013 +0100

    KEYS: Add per-user_namespace registers for persistent per-UID
kerberos caches

> > I saw that Serge even recently mentioned that you need to take
> > this aspect of the changes back to the drawing board.  With my
> > namespace maintainer hat on I repeat that.
> 
> Drawing board is here now (tuning on the text...):
> 
> http://kernsec.org/wiki/index.php/IMA_Namespacing_design_consideratio
> ns

You mention an abuse case here which is basically a way of relaxing
security policy.  Cannot we fix that by making policy hierarchical, so
a child namespace must have the same or a more strict policy than the
parent?

> >  From a 10,000 foot view I can already tell that this is hopeless.
> > So for binding IMA namspaces and CLONE_NEWNS:
> > 
> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > I am not nacking IMA namespacing just inappropriately tying ima
> > namespaces to mount namespaces.  These should be completely
> > separate entities.
> 
> Let's say we go down the road of spawning it independently. Can we
> use the unused clone flag 0x1000? Or should we come up with new 
> unshare2()/clone2() syscalls to extend the clone bits to 64 bit? Or
> use a sysfs/securityfs file to spawn a new IMA namespace? Make this a
> generic file not an IMA specific one?

If, as a result of discussions, it turns out that a separate namespace
is the correct way to proceed, I'm sure we can sort out the details of
how we cope with the flag paucity problem.

James

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

On 03/15/2018 01:33 PM, James Bottomley wrote:
> On Thu, 2018-03-15 at 11:26 -0400, Stefan Berger wrote:
>> On 03/15/2018 06:40 AM, Eric W. Biederman wrote:
>>> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>>>
>>>> From: Yuqiong Sun <suny@us.ibm.com>
>>>>
>>>> Add new CONFIG_IMA_NS config option.  Let clone() create a new
>>>> IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure in
>>>> nsproxy.  ima_ns is allocated and freed upon IMA namespace
>>>> creation and exit.  Currently, the ima_ns contains no useful IMA
>>>> data but only a dummy interface. This patch creates the framework
>>>> for namespacing the different aspects of IMA (eg. IMA-audit, IMA-
>>>> measurement, IMA-appraisal).
>>> IMA is not path based.  The only thing that belongs to a mount
>>> namespace are paths.  Therefore IMA is completely inappropriate to
>>> be joint with a mount namespace.
> Just to be clear: The mount namespace is not only about paths it's also
> about subtree properties.  However, the point still stands that IMA has
> a dependency on neither.
>
>> IMA measures the files described by these paths. The files also may
>> hold signatures (security.ima xattr) needed for IMA appraisal.
> The xattr is an inode property, which isn't namespaced by the mount_ns.
>
> When we had this discussion last year, we talked about possibly using
> the user_ns instead.  It makes sense because for IMA signatures you're

'using the user_ns' I suppose means hooking IMA namespace to it...

> going to need some type of keyring namespace and there's already one
> hanging off the user_ns:
>
> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
> Author: David Howells <dhowells@redhat.com>
> Date:   Tue Sep 24 10:35:19 2013 +0100
>
>      KEYS: Add per-user_namespace registers for persistent per-UID
> kerberos caches

The benefit for IMA would be that this would then tie the keys needed 
for appraising to the IMA namespace's policy.
However, if you have an appraise policy in your IMA namespace, which is 
now hooked to the user namespace, and you join that user namespace but 
your files don't have signatures, nothing will execute anymore. That's 
now a side effect of joining this user namespace unless we have a magic 
exception. My feeling is, people may not like that...

>
>>> I saw that Serge even recently mentioned that you need to take
>>> this aspect of the changes back to the drawing board.  With my
>>> namespace maintainer hat on I repeat that.
>> Drawing board is here now (tuning on the text...):
>>
>> http://kernsec.org/wiki/index.php/IMA_Namespacing_design_consideratio
>> ns
> You mention an abuse case here which is basically a way of relaxing
> security policy.  Cannot we fix that by making policy hierarchical, so
> a child namespace must have the same or a more strict policy than the
> parent?

I updated the page now with a hopefully better idea. So that root cannot 
escape IMA-appraisal by spawning IMA namespaces and setting an IMA NULL 
policy, root's activities will *always* be evaluated against the 
init_ima_ns policy with keys found in the init_user_ns. In other word, 
if there is an appraisal policy (rule) on the host's init_ima_ns and 
root does something as uid 0 in any namespace, file activity for 
appraising purposes must pass signature checking. So just spawning a MNT 
namespace, mounting a filesystem with unknown apps won't execute any of 
them unless the stuff is signed.

>
>>>   From a 10,000 foot view I can already tell that this is hopeless.
>>> So for binding IMA namspaces and CLONE_NEWNS:
>>>
>>> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>>
>>> I am not nacking IMA namespacing just inappropriately tying ima
>>> namespaces to mount namespaces.  These should be completely
>>> separate entities.
>> Let's say we go down the road of spawning it independently. Can we
>> use the unused clone flag 0x1000? Or should we come up with new
>> unshare2()/clone2() syscalls to extend the clone bits to 64 bit? Or
>> use a sysfs/securityfs file to spawn a new IMA namespace? Make this a
>> generic file not an IMA specific one?
> If, as a result of discussions, it turns out that a separate namespace
> is the correct way to proceed, I'm sure we can sort out the details of
> how we cope with the flag paucity problem.

Well, it's the side effects that people may not like when an IMA policy 
is active now and hooked to a USER namespace, as pointed out above. If 
we don't like the side effects, better create our own independent namespace.

    Stefan

>
> James
>
>

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[James Bottomley]

On Thu, 2018-03-15 at 14:26 -0400, Stefan Berger wrote:
> On 03/15/2018 01:33 PM, James Bottomley wrote:
> > 
> > On Thu, 2018-03-15 at 11:26 -0400, Stefan Berger wrote:
[...]
> > > 
> > > IMA measures the files described by these paths. The files also
> > > may hold signatures (security.ima xattr) needed for IMA
> > > appraisal.
> > The xattr is an inode property, which isn't namespaced by the
> > mount_ns.
> > 
> > When we had this discussion last year, we talked about possibly
> > using the user_ns instead.  It makes sense because for IMA
> > signatures you're
> 
> 'using the user_ns' I suppose means hooking IMA namespace to it...

Yes, making it belong to the user ns instead of the mnt ns.

I'm not saying it has to, but I equally don't see a good reason IMA has
to have its own namespace, especially as the keyrings are tied to the
user_ns.

> > going to need some type of keyring namespace and there's already
> > one hanging off the user_ns:
> > 
> > commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
> > Author: David Howells <dhowells@redhat.com>
> > Date:   Tue Sep 24 10:35:19 2013 +0100
> > 
> >      KEYS: Add per-user_namespace registers for persistent per-UID
> > kerberos caches
> 
> The benefit for IMA would be that this would then tie the keys needed
> for appraising to the IMA namespace's policy.
> However, if you have an appraise policy in your IMA namespace, which
> is now hooked to the user namespace, and you join that user namespace
> but your files don't have signatures, nothing will execute anymore.
> That's now a side effect of joining this user namespace unless we
> have a magic  exception. My feeling is, people may not like that...

Agree, but I think the magic might be to populate the ima keyring with
the parent on user_ns creation.  That way the user_ns owner can delete
the parent keys if they don't like them, but by default the parent
appraisal policy should just work.

> > > > I saw that Serge even recently mentioned that you need to take
> > > > this aspect of the changes back to the drawing board.  With my
> > > > namespace maintainer hat on I repeat that.
> > > Drawing board is here now (tuning on the text...):
> > > 
> > > http://kernsec.org/wiki/index.php/IMA_Namespacing_design_consider
> > > ations
> > You mention an abuse case here which is basically a way of relaxing
> > security policy.  Cannot we fix that by making policy hierarchical,
> > so a child namespace must have the same or a more strict policy
> > than the parent?
> 
> I updated the page now with a hopefully better idea. So that root
> cannot escape IMA-appraisal by spawning IMA namespaces and setting an
> IMA NULL policy, root's activities will *always* be evaluated against
> the init_ima_ns policy with keys found in the init_user_ns. In other
> word, if there is an appraisal policy (rule) on the host's
> init_ima_ns and root does something as uid 0 in any namespace, file
> activity for appraising purposes must pass signature checking. So
> just spawning a MNT namespace, mounting a filesystem with unknown
> apps won't execute any of them unless the stuff is signed.

So this would mean that most orchestration systems' ideas of privileged
containers (i.e. containers which run real root) would be unable to
have their own IMA namespace ... that's also going to be surprising.

James

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

On 03/15/2018 02:45 PM, James Bottomley wrote:
> On Thu, 2018-03-15 at 14:26 -0400, Stefan Berger wrote:
>> On 03/15/2018 01:33 PM, James Bottomley wrote:
>>> On Thu, 2018-03-15 at 11:26 -0400, Stefan Berger wrote:
> [...]
>>>> IMA measures the files described by these paths. The files also
>>>> may hold signatures (security.ima xattr) needed for IMA
>>>> appraisal.
>>> The xattr is an inode property, which isn't namespaced by the
>>> mount_ns.
>>>
>>> When we had this discussion last year, we talked about possibly
>>> using the user_ns instead.  It makes sense because for IMA
>>> signatures you're
>> 'using the user_ns' I suppose means hooking IMA namespace to it...
> Yes, making it belong to the user ns instead of the mnt ns.
>
> I'm not saying it has to, but I equally don't see a good reason IMA has
> to have its own namespace, especially as the keyrings are tied to the
> user_ns.
>
>>> going to need some type of keyring namespace and there's already
>>> one hanging off the user_ns:
>>>
>>> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
>>> Author: David Howells <dhowells@redhat.com>
>>> Date:   Tue Sep 24 10:35:19 2013 +0100
>>>
>>>       KEYS: Add per-user_namespace registers for persistent per-UID
>>> kerberos caches
>> The benefit for IMA would be that this would then tie the keys needed
>> for appraising to the IMA namespace's policy.
>> However, if you have an appraise policy in your IMA namespace, which
>> is now hooked to the user namespace, and you join that user namespace
>> but your files don't have signatures, nothing will execute anymore.
>> That's now a side effect of joining this user namespace unless we
>> have a magic  exception. My feeling is, people may not like that...
> Agree, but I think the magic might be to populate the ima keyring with
> the parent on user_ns creation.  That way the user_ns owner can delete
> the parent keys if they don't like them, but by default the parent
> appraisal policy should just work.

That may add keys to your keyring but doesn't get you signatures on your 
files. Or modify the IMA appraisal policy you just activated by joining 
the user ns to allow you accessing the files without signatures.

>
>>>>> I saw that Serge even recently mentioned that you need to take
>>>>> this aspect of the changes back to the drawing board.  With my
>>>>> namespace maintainer hat on I repeat that.
>>>> Drawing board is here now (tuning on the text...):
>>>>
>>>> http://kernsec.org/wiki/index.php/IMA_Namespacing_design_consider
>>>> ations
>>> You mention an abuse case here which is basically a way of relaxing
>>> security policy.  Cannot we fix that by making policy hierarchical,
>>> so a child namespace must have the same or a more strict policy
>>> than the parent?
>> I updated the page now with a hopefully better idea. So that root
>> cannot escape IMA-appraisal by spawning IMA namespaces and setting an
>> IMA NULL policy, root's activities will *always* be evaluated against
>> the init_ima_ns policy with keys found in the init_user_ns. In other
>> word, if there is an appraisal policy (rule) on the host's
>> init_ima_ns and root does something as uid 0 in any namespace, file
>> activity for appraising purposes must pass signature checking. So
>> just spawning a MNT namespace, mounting a filesystem with unknown
>> apps won't execute any of them unless the stuff is signed.
> So this would mean that most orchestration systems' ideas of privileged
> containers (i.e. containers which run real root) would be unable to
> have their own IMA namespace ... that's also going to be surprising.

Real root should not be able to escape file appraisal policy on the 
host. The better way is to activate user namespaces anyway I thought... 
So, yes, this has side effects as well.

    Stefan

>
> James
>

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[James Bottomley]

On Thu, 2018-03-15 at 14:51 -0400, Stefan Berger wrote:
> On 03/15/2018 02:45 PM, James Bottomley wrote:
[...]
> > > > going to need some type of keyring namespace and there's
> > > > already
> > > > one hanging off the user_ns:
> > > > 
> > > > commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
> > > > Author: David Howells <dhowells@redhat.com>
> > > > Date:   Tue Sep 24 10:35:19 2013 +0100
> > > > 
> > > >       KEYS: Add per-user_namespace registers for persistent
> > > > per-UID
> > > > kerberos caches
> > > The benefit for IMA would be that this would then tie the keys
> > > needed for appraising to the IMA namespace's policy.
> > > However, if you have an appraise policy in your IMA namespace,
> > > which is now hooked to the user namespace, and you join that user
> > > namespace but your files don't have signatures, nothing will
> > > execute anymore. That's now a side effect of joining this user
> > > namespace unless we have a magic  exception. My feeling is,
> > > people may not like that...
> > Agree, but I think the magic might be to populate the ima keyring
> > with the parent on user_ns creation.  That way the user_ns owner
> > can delete the parent keys if they don't like them, but by default
> > the parent appraisal policy should just work.
> 
> That may add keys to your keyring but doesn't get you signatures on
> your  files.

But it doesn't need to.  The only way we'd get a failure is if the file
is already being appraised and we lose access to the key.  If the
parent policy isn't appraisal, entering the IMA NS won't cause
appraisal to be turned on unless the owner asks for it, in which case
it's caveat emptor: As it works today, if as root I add a default
appraisal policy to IMA without either a key or xattrs, I get an
unusable system.

James

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

On 03/15/2018 03:01 PM, James Bottomley wrote:
> On Thu, 2018-03-15 at 14:51 -0400, Stefan Berger wrote:
>> On 03/15/2018 02:45 PM, James Bottomley wrote:
> [...]
>>>>> going to need some type of keyring namespace and there's
>>>>> already
>>>>> one hanging off the user_ns:
>>>>>
>>>>> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
>>>>> Author: David Howells <dhowells@redhat.com>
>>>>> Date:   Tue Sep 24 10:35:19 2013 +0100
>>>>>
>>>>>        KEYS: Add per-user_namespace registers for persistent
>>>>> per-UID
>>>>> kerberos caches
>>>> The benefit for IMA would be that this would then tie the keys
>>>> needed for appraising to the IMA namespace's policy.
>>>> However, if you have an appraise policy in your IMA namespace,
>>>> which is now hooked to the user namespace, and you join that user
>>>> namespace but your files don't have signatures, nothing will
>>>> execute anymore. That's now a side effect of joining this user
>>>> namespace unless we have a magic  exception. My feeling is,
>>>> people may not like that...
>>> Agree, but I think the magic might be to populate the ima keyring
>>> with the parent on user_ns creation.  That way the user_ns owner
>>> can delete the parent keys if they don't like them, but by default
>>> the parent appraisal policy should just work.
>> That may add keys to your keyring but doesn't get you signatures on
>> your  files.
> But it doesn't need to.  The only way we'd get a failure is if the file
> is already being appraised and we lose access to the key.  If the

Well, the configuration I talked about above was assuming that we have 
an appraisal policy active in the IMA namespace, which is now tied to 
the user namespace that was just joined.

If we are fine with the side effects of an IMA policy active as part of 
a user namespace then let's go with it. The side effects in case of an 
active IMA appraisal may then be that files cannot be read/accessed, or 
file measurements or IMA auditing may occur.

The alternative is we have an independent IMA namespace. If one joins 
the USER namespace and there are no IMA-related side effects. If one 
joins the IMA namespace its IMA policy should start being enforced. If 
the current active USER namespace has the keys that go with the 
signatures of the filesystem, then we're fine from the appraisal 
perspective. If not, then IMA namespace joining may prevent file accesses.

> parent policy isn't appraisal, entering the IMA NS won't cause

Why parent policy? The policy of the namespace that was joined should be 
the active one, no ?

> appraisal to be turned on unless the owner asks for it, in which case
> it's caveat emptor: As it works today, if as root I add a default
> appraisal policy to IMA without either a key or xattrs, I get an
> unusable system.

    Stefan

>
> James
>

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Eric W. Biederman]

Stefan Berger <stefanb@linux.vnet.ibm.com> writes:

> On 03/15/2018 03:01 PM, James Bottomley wrote:
>> On Thu, 2018-03-15 at 14:51 -0400, Stefan Berger wrote:
>>> On 03/15/2018 02:45 PM, James Bottomley wrote:
>> [...]
>>>>>> going to need some type of keyring namespace and there's
>>>>>> already
>>>>>> one hanging off the user_ns:
>>>>>>
>>>>>> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
>>>>>> Author: David Howells <dhowells@redhat.com>
>>>>>> Date:   Tue Sep 24 10:35:19 2013 +0100
>>>>>>
>>>>>>        KEYS: Add per-user_namespace registers for persistent
>>>>>> per-UID
>>>>>> kerberos caches
>>>>> The benefit for IMA would be that this would then tie the keys
>>>>> needed for appraising to the IMA namespace's policy.
>>>>> However, if you have an appraise policy in your IMA namespace,
>>>>> which is now hooked to the user namespace, and you join that user
>>>>> namespace but your files don't have signatures, nothing will
>>>>> execute anymore. That's now a side effect of joining this user
>>>>> namespace unless we have a magic  exception. My feeling is,
>>>>> people may not like that...
>>>> Agree, but I think the magic might be to populate the ima keyring
>>>> with the parent on user_ns creation.  That way the user_ns owner
>>>> can delete the parent keys if they don't like them, but by default
>>>> the parent appraisal policy should just work.
>>> That may add keys to your keyring but doesn't get you signatures on
>>> your  files.
>> But it doesn't need to.  The only way we'd get a failure is if the file
>> is already being appraised and we lose access to the key.  If the
>
> Well, the configuration I talked about above was assuming that we have
> an appraisal policy active in the IMA namespace, which is now tied to
> the user namespace that was just joined.
>
> If we are fine with the side effects of an IMA policy active as part
> of a user namespace then let's go with it. The side effects in case of
> an active IMA appraisal may then be that files cannot be
> read/accessed, or file measurements or IMA auditing may occur.
>
> The alternative is we have an independent IMA namespace. If one joins
> the USER namespace and there are no IMA-related side effects. If one
> joins the IMA namespace its IMA policy should start being enforced. If
> the current active USER namespace has the keys that go with the
> signatures of the filesystem, then we're fine from the appraisal
> perspective. If not, then IMA namespace joining may prevent file
> accesses.
>
>> parent policy isn't appraisal, entering the IMA NS won't cause
>
> Why parent policy? The policy of the namespace that was joined should
> be the active one, no ?

Unless I am completely blind we should never stop enforcing the parent's
polciy.  We should only add policy to enforce for the scope of a
container.

In practice this is just the containers policy as the container is most
likely a do whatever you want to in the parent policy.  But not always
and not explicitly.

Mount namespaces are not hierarchical, user namespaces are.  Which makes
them much more appropriate for being part of nested policy enforcement.

>From previous conversations I remember that there is a legitimate
bootstrap problem for IMA.  That needs to be looked at, and I am not
seeing that mentioned.

Eric

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

On 03/15/2018 03:20 PM, Eric W. Biederman wrote:
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>
>> On 03/15/2018 03:01 PM, James Bottomley wrote:
>>> On Thu, 2018-03-15 at 14:51 -0400, Stefan Berger wrote:
>>>> On 03/15/2018 02:45 PM, James Bottomley wrote:
>>> [...]
>>>>>>> going to need some type of keyring namespace and there's
>>>>>>> already
>>>>>>> one hanging off the user_ns:
>>>>>>>
>>>>>>> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
>>>>>>> Author: David Howells <dhowells@redhat.com>
>>>>>>> Date:   Tue Sep 24 10:35:19 2013 +0100
>>>>>>>
>>>>>>>         KEYS: Add per-user_namespace registers for persistent
>>>>>>> per-UID
>>>>>>> kerberos caches
>>>>>> The benefit for IMA would be that this would then tie the keys
>>>>>> needed for appraising to the IMA namespace's policy.
>>>>>> However, if you have an appraise policy in your IMA namespace,
>>>>>> which is now hooked to the user namespace, and you join that user
>>>>>> namespace but your files don't have signatures, nothing will
>>>>>> execute anymore. That's now a side effect of joining this user
>>>>>> namespace unless we have a magic  exception. My feeling is,
>>>>>> people may not like that...
>>>>> Agree, but I think the magic might be to populate the ima keyring
>>>>> with the parent on user_ns creation.  That way the user_ns owner
>>>>> can delete the parent keys if they don't like them, but by default
>>>>> the parent appraisal policy should just work.
>>>> That may add keys to your keyring but doesn't get you signatures on
>>>> your  files.
>>> But it doesn't need to.  The only way we'd get a failure is if the file
>>> is already being appraised and we lose access to the key.  If the
>> Well, the configuration I talked about above was assuming that we have
>> an appraisal policy active in the IMA namespace, which is now tied to
>> the user namespace that was just joined.
>>
>> If we are fine with the side effects of an IMA policy active as part
>> of a user namespace then let's go with it. The side effects in case of
>> an active IMA appraisal may then be that files cannot be
>> read/accessed, or file measurements or IMA auditing may occur.
>>
>> The alternative is we have an independent IMA namespace. If one joins
>> the USER namespace and there are no IMA-related side effects. If one
>> joins the IMA namespace its IMA policy should start being enforced. If
>> the current active USER namespace has the keys that go with the
>> signatures of the filesystem, then we're fine from the appraisal
>> perspective. If not, then IMA namespace joining may prevent file
>> accesses.
>>
>>> parent policy isn't appraisal, entering the IMA NS won't cause
>> Why parent policy? The policy of the namespace that was joined should
>> be the active one, no ?
> Unless I am completely blind we should never stop enforcing the parent's
> polciy.  We should only add policy to enforce for the scope of a
> container.

What we want is an independent policy for each IMA namespace.

What we don't want is that root can abuse his power to spawn new 
namespaces and circumvent a file appraisal policy on the host (in 
init_ima_ns). Because of that root's activities are subject to the IMA 
policy of the currently active IMA namespace and the one of init_ima_ns 
(and possibly all the ones in between). If root is working in a child 
IMA namespace and file appraisal fails due to the policy in init_ima_ns 
and keys found in .ima or _ima keyrings attached to init_user_ns, the 
file access will be denied.

Besides that root's activities will always be measured and audited 
following the policy in init_ima_ns. This tries to prevent that root 
spawns an IMA namespace with a NULL policy and does things in the TCB 
and tries to escape the logging.

>
> In practice this is just the containers policy as the container is most
> likely a do whatever you want to in the parent policy.  But not always
> and not explicitly.
>
> Mount namespaces are not hierarchical, user namespaces are.  Which makes
> them much more appropriate for being part of nested policy enforcement.

We don't want additive or hierarchical policies - at least I don't. They 
should be independent. Only exception are activities of root that are 
always iteratively evaluated against policies of the current IMA NS and 
the init_ima_ns and possibly all the ones in between.

>
>  From previous conversations I remember that there is a legitimate
> bootstrap problem for IMA.  That needs to be looked at, and I am not
> seeing that mentioned.

IMA's log should not have a gap. So ideally we shouldn't have to write 
something into sysfs to spawn a new IMA namespace so that we don't miss 
whatever setup may have happened to get there, including the writing 
into procfs. IMA should be there right from the start. So a clone flag 
would be ideal for that.

>
> Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Eric W. Biederman]

Stefan Berger <stefanb@linux.vnet.ibm.com> writes:

> On 03/15/2018 03:20 PM, Eric W. Biederman wrote:
>> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>>
>>> On 03/15/2018 03:01 PM, James Bottomley wrote:
>>>> On Thu, 2018-03-15 at 14:51 -0400, Stefan Berger wrote:
>>>>> On 03/15/2018 02:45 PM, James Bottomley wrote:
>>>> [...]
>>>>>>>> going to need some type of keyring namespace and there's
>>>>>>>> already
>>>>>>>> one hanging off the user_ns:
>>>>>>>>
>>>>>>>> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
>>>>>>>> Author: David Howells <dhowells@redhat.com>
>>>>>>>> Date:   Tue Sep 24 10:35:19 2013 +0100
>>>>>>>>
>>>>>>>>         KEYS: Add per-user_namespace registers for persistent
>>>>>>>> per-UID
>>>>>>>> kerberos caches
>>>>>>> The benefit for IMA would be that this would then tie the keys
>>>>>>> needed for appraising to the IMA namespace's policy.
>>>>>>> However, if you have an appraise policy in your IMA namespace,
>>>>>>> which is now hooked to the user namespace, and you join that user
>>>>>>> namespace but your files don't have signatures, nothing will
>>>>>>> execute anymore. That's now a side effect of joining this user
>>>>>>> namespace unless we have a magic  exception. My feeling is,
>>>>>>> people may not like that...
>>>>>> Agree, but I think the magic might be to populate the ima keyring
>>>>>> with the parent on user_ns creation.  That way the user_ns owner
>>>>>> can delete the parent keys if they don't like them, but by default
>>>>>> the parent appraisal policy should just work.
>>>>> That may add keys to your keyring but doesn't get you signatures on
>>>>> your  files.
>>>> But it doesn't need to.  The only way we'd get a failure is if the file
>>>> is already being appraised and we lose access to the key.  If the
>>> Well, the configuration I talked about above was assuming that we have
>>> an appraisal policy active in the IMA namespace, which is now tied to
>>> the user namespace that was just joined.
>>>
>>> If we are fine with the side effects of an IMA policy active as part
>>> of a user namespace then let's go with it. The side effects in case of
>>> an active IMA appraisal may then be that files cannot be
>>> read/accessed, or file measurements or IMA auditing may occur.
>>>
>>> The alternative is we have an independent IMA namespace. If one joins
>>> the USER namespace and there are no IMA-related side effects. If one
>>> joins the IMA namespace its IMA policy should start being enforced. If
>>> the current active USER namespace has the keys that go with the
>>> signatures of the filesystem, then we're fine from the appraisal
>>> perspective. If not, then IMA namespace joining may prevent file
>>> accesses.
>>>
>>>> parent policy isn't appraisal, entering the IMA NS won't cause
>>> Why parent policy? The policy of the namespace that was joined should
>>> be the active one, no ?
>> Unless I am completely blind we should never stop enforcing the parent's
>> polciy.  We should only add policy to enforce for the scope of a
>> container.
>
> What we want is an independent policy for each IMA namespace.
>
> What we don't want is that root can abuse his power to spawn new namespaces and
> circumvent a file appraisal policy on the host (in init_ima_ns). Because of that
> root's activities are subject to the IMA policy of the currently active IMA
> namespace and the one of init_ima_ns (and possibly all the ones in between). If
> root is working in a child IMA namespace and file appraisal fails due to the
> policy in init_ima_ns and keys found in .ima or _ima keyrings attached to
> init_user_ns, the file access will be denied.
>
> Besides that root's activities will always be measured and audited following the
> policy in init_ima_ns. This tries to prevent that root spawns an IMA namespace
> with a NULL policy and does things in the TCB and tries to escape the
> logging.

That sounds exactly like my definition of hierarchical namespace
enforcement.

And please keep in mind that everyone is allowed to use CLONE_NEWNS now.
You just have to spawn a new user namespace first so you have the caps.

>> In practice this is just the containers policy as the container is most
>> likely a do whatever you want to in the parent policy.  But not always
>> and not explicitly.
>>
>> Mount namespaces are not hierarchical, user namespaces are.  Which makes
>> them much more appropriate for being part of nested policy enforcement.
>
> We don't want additive or hierarchical policies - at least I don't. They should
> be independent. Only exception are activities of root that are always
> iteratively evaluated against policies of the current IMA NS and the init_ima_ns
> and possibly all the ones in between.

I believe that is what I meant by a nested/hiearchical policy
enforcement.

>>  From previous conversations I remember that there is a legitimate
>> bootstrap problem for IMA.  That needs to be looked at, and I am not
>> seeing that mentioned.
>
> IMA's log should not have a gap. So ideally we shouldn't have to write something
> into sysfs to spawn a new IMA namespace so that we don't miss whatever setup may
> have happened to get there, including the writing into procfs. IMA should be
> there right from the start. So a clone flag would be ideal for that.

Please make that securityfs not sysfs.  Sysfs should be about the
hardware not these higher level software details.  I really don't want
to have to namespace sysfs more than I already have.

As for the no gaps requirement.  That is a powerful lever for ruling out
solutions that don't work as well.



Eric

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

On 03/15/2018 03:15 PM, Stefan Berger wrote:
> On 03/15/2018 03:01 PM, James Bottomley wrote:
>> On Thu, 2018-03-15 at 14:51 -0400, Stefan Berger wrote:
>>> On 03/15/2018 02:45 PM, James Bottomley wrote:
>> [...]
>>>>>> going to need some type of keyring namespace and there's
>>>>>> already
>>>>>> one hanging off the user_ns:
>>>>>>
>>>>>> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
>>>>>> Author: David Howells <dhowells@redhat.com>
>>>>>> Date:   Tue Sep 24 10:35:19 2013 +0100
>>>>>>
>>>>>>        KEYS: Add per-user_namespace registers for persistent
>>>>>> per-UID
>>>>>> kerberos caches
>>>>> The benefit for IMA would be that this would then tie the keys
>>>>> needed for appraising to the IMA namespace's policy.
>>>>> However, if you have an appraise policy in your IMA namespace,
>>>>> which is now hooked to the user namespace, and you join that user
>>>>> namespace but your files don't have signatures, nothing will
>>>>> execute anymore. That's now a side effect of joining this user
>>>>> namespace unless we have a magic  exception. My feeling is,
>>>>> people may not like that...
>>>> Agree, but I think the magic might be to populate the ima keyring
>>>> with the parent on user_ns creation.  That way the user_ns owner
>>>> can delete the parent keys if they don't like them, but by default
>>>> the parent appraisal policy should just work.
>>> That may add keys to your keyring but doesn't get you signatures on
>>> your  files.
>> But it doesn't need to.  The only way we'd get a failure is if the file
>> is already being appraised and we lose access to the key.  If the
>
> Well, the configuration I talked about above was assuming that we have 
> an appraisal policy active in the IMA namespace, which is now tied to 
> the user namespace that was just joined.
>
> If we are fine with the side effects of an IMA policy active as part 
> of a user namespace then let's go with it. The side effects in case of 
> an active IMA appraisal may then be that files cannot be 
> read/accessed, or file measurements or IMA auditing may occur.
>
> The alternative is we have an independent IMA namespace. If one joins 
> the USER namespace and there are no IMA-related side effects. If one 
> joins the IMA namespace its IMA policy should start being enforced. If 
> the current active USER namespace has the keys that go with the 
> signatures of the filesystem, then we're fine from the appraisal 
> perspective. If not, then IMA namespace joining may prevent file 
> accesses.

With these differences pointed out, which path do we want to go now ? 
Eric ? James ?

    Stefan

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Mimi Zohar]

On Thu, 2018-03-15 at 15:35 -0500, Eric W. Biederman wrote:
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
> > On 03/15/2018 03:20 PM, Eric W. Biederman wrote:

[..]

> >>  From previous conversations I remember that there is a legitimate
> >> bootstrap problem for IMA.  That needs to be looked at, and I am not
> >> seeing that mentioned.
> >
> > IMA's log should not have a gap. So ideally we shouldn't have to write something
> > into sysfs to spawn a new IMA namespace so that we don't miss whatever setup may
> > have happened to get there, including the writing into procfs. IMA should be
> > there right from the start. So a clone flag would be ideal for that.
> 
> Please make that securityfs not sysfs.  Sysfs should be about the
> hardware not these higher level software details.  I really don't want
> to have to namespace sysfs more than I already have.
> 
> As for the no gaps requirement.  That is a powerful lever for ruling out
> solutions that don't work as well.

IMA-measurement and IMA-audit need to be enabled from the very
beginning.  The only reason we differentiate between IMA-measurement
and IMA-audit from IMA-appraisal is simply because the initramfs
doesn't include xattrs.  Once support for CPIO xattrs is upstreamed,
IMA-appraisal could then also be enabled from the very beginning.  For
now, we rely on the initramfs being measured (and appraised) and
enable IMA-appraisal before any files are accessed from real root.
 Systems with a custom /init today already can enable IMA-appraisal
from the very beginning.  

In terms of IMA namespacing, we shouldn't need to differentiate
between IMA-measurement and IMA-audit from IMA-appraisal.  All of them
should be initialized from the very beginning to capture all
measurements in the measurement list, audit the measurements and
appraise all files.

Requiring IMA namespacing to be joined to another namespace
complicates things, like the unnecessary creation of IMA namespaces.
 Just as there is an "owning" namespace for other namespaces, there
should be an "owning" IMA namespace, which is independent of either
the mount or user namespace.

(I hope I'm using the term "owning" properly here.)

Mimi

Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support

[Stefan Berger]

On 03/15/2018 03:01 PM, James Bottomley wrote:
> On Thu, 2018-03-15 at 14:51 -0400, Stefan Berger wrote:
>> On 03/15/2018 02:45 PM, James Bottomley wrote:
> [...]
>>>>> going to need some type of keyring namespace and there's
>>>>> already
>>>>> one hanging off the user_ns:
>>>>>
>>>>> commit f36f8c75ae2e7d4da34f4c908cebdb4aa42c977e
>>>>> Author: David Howells <dhowells@redhat.com>
>>>>> Date:   Tue Sep 24 10:35:19 2013 +0100
>>>>>
>>>>>        KEYS: Add per-user_namespace registers for persistent
>>>>> per-UID
>>>>> kerberos caches
>>>> The benefit for IMA would be that this would then tie the keys
>>>> needed for appraising to the IMA namespace's policy.
>>>> However, if you have an appraise policy in your IMA namespace,
>>>> which is now hooked to the user namespace, and you join that user
>>>> namespace but your files don't have signatures, nothing will
>>>> execute anymore. That's now a side effect of joining this user
>>>> namespace unless we have a magic  exception. My feeling is,
>>>> people may not like that...
>>> Agree, but I think the magic might be to populate the ima keyring
>>> with the parent on user_ns creation.  That way the user_ns owner
>>> can delete the parent keys if they don't like them, but by default
>>> the parent appraisal policy should just work.
>> That may add keys to your keyring but doesn't get you signatures on
>> your  files.
> But it doesn't need to.  The only way we'd get a failure is if the file
> is already being appraised and we lose access to the key.  If the
> parent policy isn't appraisal, entering the IMA NS won't cause
> appraisal to be turned on unless the owner asks for it, in which case
> it's caveat emptor: As it works today, if as root I add a default
> appraisal policy to IMA without either a key or xattrs, I get an
> unusable system.

When I post a next implementation for the spawning if an IMA namespace, 
what shall be the criterion for accepting it?

     Stefan

>
> James
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>