LinuxLists.cc - [RFC PATCH 0/5] ima: namespacing IMA audit messages

[permalink] [raw]

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

From: Yuqiong Sun <[email protected]>

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).

Signed-off-by: Yuqiong Sun <[email protected]>

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()

Signed-off-by: Mehmet Kayaalp <[email protected]>
---
fs/proc/namespaces.c | 3 +
include/linux/ima.h | 37 ++++++++
include/linux/nsproxy.h | 1 +
include/linux/proc_ns.h | 2 +
init/Kconfig | 8 ++
kernel/nsproxy.c | 15 ++++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 9 ++
security/integrity/ima/ima_init.c | 4 +
security/integrity/ima/ima_ns.c | 183 ++++++++++++++++++++++++++++++++++++++
10 files changed, 263 insertions(+)
create mode 100644 security/integrity/ima/ima_ns.c

diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index 3803b24..222a64e 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -32,6 +32,9 @@ static const struct proc_ns_operations *ns_entries[] = {
#ifdef CONFIG_CGROUPS
&cgroupns_operations,
#endif
+#ifdef CONFIG_IMA_NS
+ &imans_operations,
+#endif
};

static const char *proc_ns_get_link(struct dentry *dentry,
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e..11e4841 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -105,4 +105,41 @@ 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 ns_common ns;
+ struct ima_namespace *parent;
+};
+
+extern struct ima_namespace init_ima_ns;
+
+#ifdef CONFIG_IMA_NS
+void put_ima_ns(struct ima_namespace *ns);
+struct ima_namespace *copy_ima(unsigned long flags,
+ struct user_namespace *user_ns,
+ struct ima_namespace *old_ns);
+static inline struct ima_namespace *get_current_ns(void)
+{
+ return current->nsproxy->ima_ns;
+}
+#else
+static inline void put_ima_ns(struct ima_namespace *ns)
+{
+ return;
+}
+
+static inline struct ima_namespace *copy_ima(unsigned long flags,
+ 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/nsproxy.h b/include/linux/nsproxy.h
index ac0d65b..a97031d 100644
--- a/include/linux/nsproxy.h
+++ b/include/linux/nsproxy.h
@@ -35,6 +35,7 @@ struct nsproxy {
struct pid_namespace *pid_ns_for_children;
struct net *net_ns;
struct cgroup_namespace *cgroup_ns;
+ struct ima_namespace *ima_ns;
};
extern struct nsproxy init_nsproxy;

diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
index 58ab28d..c7c1239 100644
--- a/include/linux/proc_ns.h
+++ b/include/linux/proc_ns.h
@@ -31,6 +31,7 @@ extern const struct proc_ns_operations pidns_for_children_operations;
extern const struct proc_ns_operations userns_operations;
extern const struct proc_ns_operations mntns_operations;
extern const struct proc_ns_operations cgroupns_operations;
+extern const struct proc_ns_operations imans_operations;

/*
* We always define these enumerators
@@ -42,6 +43,7 @@ enum {
PROC_USER_INIT_INO = 0xEFFFFFFDU,
PROC_PID_INIT_INO = 0xEFFFFFFCU,
PROC_CGROUP_INIT_INO = 0xEFFFFFFBU,
+ PROC_IMA_INIT_INO = 0xEFFFFFFAU,
};

#ifdef CONFIG_PROC_FS
diff --git a/init/Kconfig b/init/Kconfig
index 1d3475f..339f84d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1287,6 +1287,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 f6c5d33..85be341 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;

@@ -44,6 +45,9 @@ struct nsproxy init_nsproxy = {
#ifdef CONFIG_CGROUPS
.cgroup_ns = &init_cgroup_ns,
#endif
+#ifdef CONFIG_IMA_NS
+ .ima_ns = &init_ima_ns,
+#endif
};

static inline struct nsproxy *create_nsproxy(void)
@@ -110,8 +114,17 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
goto out_net;
}

+ new_nsp->ima_ns = copy_ima(flags, user_ns, tsk->nsproxy->ima_ns);
+ if (IS_ERR(new_nsp->ima_ns)) {
+ err = PTR_ERR(new_nsp->ima_ns);
+ goto out_ima;
+ }
+
return new_nsp;

+out_ima:
+ if (new_nsp->ima_ns)
+ put_ima_ns(new_nsp->ima_ns);
out_net:
put_cgroup_ns(new_nsp->cgroup_ns);
out_cgroup:
@@ -181,6 +194,8 @@ void free_nsproxy(struct nsproxy *ns)
if (ns->pid_ns_for_children)
put_pid_ns(ns->pid_ns_for_children);
put_cgroup_ns(ns->cgroup_ns);
+ if (ns->ima_ns)
+ put_ima_ns(ns->ima_ns);
put_net(ns->net_ns);
kmem_cache_free(nsproxy_cachep, ns);
}
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 29f198b..20493f0 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -8,5 +8,6 @@ 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-$(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 d52b487..8a8234a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -291,6 +291,15 @@ static inline int ima_read_xattr(struct dentry *dentry,

#endif /* CONFIG_IMA_APPRAISE */

+#ifdef CONFIG_IMA_NS
+int ima_ns_init(void);
+#else
+static inline int ima_ns_init(void)
+{
+ return 0;
+}
+#endif /* CONFIG_IMA_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 2967d49..7f884a7 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_ns.c b/security/integrity/ima/ima_ns.c
new file mode 100644
index 0000000..383217b
--- /dev/null
+++ b/security/integrity/ima/ima_ns.c
@@ -0,0 +1,183 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: Yuqiong Sun <[email protected]>
+ *
+ * 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/proc_ns.h>
+#include <linux/kref.h>
+#include <linux/slab.h>
+#include <linux/ima.h>
+
+#include "ima.h"
+
+static void get_ima_ns(struct ima_namespace *ns);
+
+int ima_init_namespace(struct ima_namespace *ns)
+{
+ return 0;
+}
+
+int ima_ns_init(void)
+{
+ return ima_init_namespace(&init_ima_ns);
+}
+
+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;
+ int err;
+
+ ns = create_ima_ns();
+ if (!ns)
+ return ERR_PTR(-ENOMEM);
+
+ err = ns_alloc_inum(&ns->ns);
+ if (err) {
+ kfree(ns);
+ return ERR_PTR(err);
+ }
+
+ ns->ns.ops = &imans_operations;
+ 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(unsigned long flags,
+ struct user_namespace *user_ns,
+ struct ima_namespace *old_ns)
+{
+ struct ima_namespace *new_ns;
+
+ BUG_ON(!old_ns);
+ get_ima_ns(old_ns);
+
+ if (!(flags & CLONE_NEWNS))
+ return 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);
+ ns_free_inum(&ns->ns);
+ kfree(ns);
+}
+
+static void free_ima_ns(struct kref *kref)
+{
+ struct ima_namespace *ns;
+
+ ns = container_of(kref, struct ima_namespace, kref);
+ destroy_ima_ns(ns);
+}
+
+static void get_ima_ns(struct ima_namespace *ns)
+{
+ kref_get(&ns->kref);
+}
+
+void put_ima_ns(struct ima_namespace *ns)
+{
+ kref_put(&ns->kref, free_ima_ns);
+}
+
+static inline struct ima_namespace *to_ima_ns(struct ns_common *ns)
+{
+ return container_of(ns, struct ima_namespace, ns);
+}
+
+static struct ns_common *imans_get(struct task_struct *task)
+{
+ struct ima_namespace *ns = NULL;
+ struct nsproxy *nsproxy;
+
+ task_lock(task);
+ nsproxy = task->nsproxy;
+ if (nsproxy) {
+ ns = nsproxy->ima_ns;
+ get_ima_ns(ns);
+ }
+ task_unlock(task);
+
+ return ns ? &ns->ns : NULL;
+}
+
+static void imans_put(struct ns_common *ns)
+{
+ put_ima_ns(to_ima_ns(ns));
+}
+
+static int imans_install(struct nsproxy *nsproxy, struct ns_common *new)
+{
+ struct ima_namespace *ns = to_ima_ns(new);
+
+ if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN) ||
+ !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
+ return -EPERM;
+
+ get_ima_ns(ns);
+ put_ima_ns(nsproxy->ima_ns);
+ nsproxy->ima_ns = ns;
+ return 0;
+}
+
+const struct proc_ns_operations imans_operations = {
+ .name = "ima",
+ .type = CLONE_NEWNS,
+ .get = imans_get,
+ .put = imans_put,
+ .install = imans_install,
+};
+
+struct ima_namespace init_ima_ns = {
+ .kref = KREF_INIT(2),
+ .user_ns = &init_user_ns,
+ .ns.inum = PROC_IMA_INIT_INO,
+#ifdef CONFIG_IMA_NS
+ .ns.ops = &imans_operations,
+#endif
+ .parent = NULL,
+};
+EXPORT_SYMBOL(init_ima_ns);
--
2.9.4

2017-07-20 22:55:08

[permalink] [raw]

Subject: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

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. A lazy clean up of the rbtree that removes free'd inodes could be
implemented to reclaim the invalid entries.

Signed-off-by: Mehmet Kayaalp <[email protected]>
---
include/linux/ima.h | 3 +
security/integrity/ima/ima.h | 16 ++++++
security/integrity/ima/ima_ns.c | 120 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 139 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 11e4841..3fdf56f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -111,6 +111,9 @@ struct ima_namespace {
struct user_namespace *user_ns;
struct ns_common 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 8a8234a..5ab769a 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);
@@ -293,11 +301,19 @@ static inline int ima_read_xattr(struct dentry *dentry,

#ifdef CONFIG_IMA_NS
int ima_ns_init(void);
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+ struct inode *inode);
#else
static inline int ima_ns_init(void)
{
return 0;
}
+
+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 */
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 383217b..5ec5a4b 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -20,6 +20,9 @@ static void get_ima_ns(struct ima_namespace *ns);

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;
}

@@ -98,10 +101,24 @@ struct ima_namespace *copy_ima(unsigned long flags,
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);
ns_free_inum(&ns->ns);
+ free_ns_status_cache(ns);
kfree(ns);
}

@@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
.parent = NULL,
};
EXPORT_SYMBOL(init_ima_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->i_ino > status->i_ino)
+ 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.9.4

2017-07-20 22:55:54

[permalink] [raw]

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

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
none of the IMA_NS_STATUS_ACTIONS (currently only IMA_AUDIT) are 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 <[email protected]>
---
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 | 21 +++++++++++++++++++++
5 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 339f84d..6bfc579 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1294,7 +1294,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 5ab769a..78921b7 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,10 +300,17 @@ 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
+
#ifdef CONFIG_IMA_NS
int ima_ns_init(void);
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 int ima_ns_init(void)
{
@@ -314,6 +322,20 @@ static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
{
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 c2edba8..4a77072 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -287,15 +287,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++)
@@ -316,7 +318,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 2aebb79..fb002f2 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -160,6 +160,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];
@@ -170,6 +171,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;
@@ -196,6 +198,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) {
@@ -211,9 +219,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)))
@@ -251,7 +260,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);

out_digsig:
if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) &&
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 5ec5a4b..562a0812 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -301,3 +301,24 @@ 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 & (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.9.4

2017-07-20 22:56:26

[permalink] [raw]

Subject: [RFC PATCH 4/5] ima: differentiate auditing policy rules from "audit" actions

From: Mimi Zohar <[email protected]>

The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and
the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY
to reflect the IMA policy rules.

Signed-off-by: Mimi Zohar <[email protected]>
---
include/uapi/linux/audit.h | 3 ++-
security/integrity/ima/ima_policy.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..649d4c4 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -144,7 +144,8 @@
#define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */
#define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */
#define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */
-#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */
+#define AUDIT_INTEGRITY_RULE 1805 /* IMA audit action policy msgs */
+#define AUDIT_INTEGRITY_POLICY 1806 /* IMA policy rules */

#define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f443662..9844eb1 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -613,7 +613,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
bool uid_token;
int result = 0;

- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
+ ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_POLICY);

entry->uid = INVALID_UID;
entry->fowner = INVALID_UID;
--
2.9.4

2017-07-20 22:57:02

[permalink] [raw]

Subject: [RFC PATCH 5/5] ima: Add ns_mnt, dev, ino fields to IMA audit measurement msgs

From: Guilherme Magalhaes <[email protected]>

Extending audit measurement record with mount namespace id, file inode,
and device name. These fields uniquely identify a pathname considering
different mount namespaces. The file inode on a given device is unique
and these fields are required to identify a namespace id since this id
can be released and later reused by a different process.

Signed-off-by: Guilherme Magalhaes <[email protected]>

Changelog:
* Change the field name from "mnt_ns" to "ns_mnt"

Signed-off-by: Mehmet Kayaalp <[email protected]>
---
security/integrity/ima/ima_api.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 4a77072..084b126 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -18,6 +18,7 @@
#include <linux/fs.h>
#include <linux/xattr.h>
#include <linux/evm.h>
+#include <linux/proc_ns.h>

#include "ima.h"

@@ -296,6 +297,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
char algo_hash[sizeof(hash) + strlen(algo_name) + 2];
int i;
unsigned long flags = iint_flags(iint, status);
+ struct ns_common *ns;

if (flags & IMA_AUDITED)
return;
@@ -314,6 +316,14 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_format(ab, " hash=");
snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
audit_log_untrustedstring(ab, algo_hash);
+ ns = mntns_operations.get(current);
+ if (!IS_ERR_OR_NULL(ns)) {
+ audit_log_format(ab, " ns_mnt=%u", ns->inum);
+ mntns_operations.put(ns);
+ }
+ audit_log_format(ab, " dev=");
+ audit_log_untrustedstring(ab, iint->inode->i_sb->s_id);
+ audit_log_format(ab, " ino=%lu", iint->inode->i_ino);

audit_log_task_info(ab, current);
audit_log_end(ab);
--
2.9.4

2017-07-25 17:53:14

[permalink] [raw]

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

On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> From: Yuqiong Sun <[email protected]>
>
> 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).
>
> Signed-off-by: Yuqiong Sun <[email protected]>
>
> Changelog:
> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag

Hi,

So this means that every mount namespace clone will clone a new IMA
namespace. Is that really ok?

2017-07-25 18:49:21

by James Bottomley

[permalink] [raw]

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

On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> >
> > From: Yuqiong Sun <[email protected]>
> >
> > 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).
> >
> > Signed-off-by: Yuqiong Sun <[email protected]>
> >
> > Changelog:
> > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>
> Hi,
>
> So this means that every mount namespace clone will clone a new IMA
> namespace. Is that really ok?

Based on what: space concerns (struct ima_ns is reasonably small)? or
whether tying it to the mount namespace is the correct thing to do. On
the latter, it does seem that this should be a property of either the
mount or user ns rather than its own separate ns. I could see a use
where even a container might want multiple ima keyrings within the
container (say containerised apache service with multiple tenants), so
instinct tells me that mount ns is the correct granularity for this.

James

2017-07-25 19:04:02

[permalink] [raw]

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

On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> > >
> > > From: Yuqiong Sun <[email protected]>
> > >
> > > 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).
> > >
> > > Signed-off-by: Yuqiong Sun <[email protected]>
> > >
> > > Changelog:
> > > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> >
> > Hi,
> >
> > So this means that every mount namespace clone will clone a new IMA
> > namespace.??Is that really ok?
>
> Based on what: space concerns (struct ima_ns is reasonably small)? or
> whether tying it to the mount namespace is the correct thing to do. ?On

Mostly the latter. The other would be not so much space concerns as
time concerns. Many things use new mounts namespaces, and we wouldn't
want multiple IMA calls on all file accesses by all of those.

> the latter, it does seem that this should be a property of either the
> mount or user ns rather than its own separate ns. ?I could see a use
> where even a container might want multiple ima keyrings within the
> container (say containerised apache service with multiple tenants), so
> instinct tells me that mount ns is the correct granularity for this.

I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns as
the trigger for requesting a new ima ns on the next clone(CLONE_NEWNS).

2017-07-25 19:09:28

by James Bottomley

[permalink] [raw]

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

On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> >
> > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > >
> > > On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> > > >
> > > >
> > > > From: Yuqiong Sun <[email protected]>
> > > >
> > > > 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).
> > > >
> > > > Signed-off-by: Yuqiong Sun <[email protected]>
> > > >
> > > > Changelog:
> > > > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > >
> > > Hi,
> > >
> > > So this means that every mount namespace clone will clone a new
> > > IMA namespace.  Is that really ok?
> >
> > Based on what: space concerns (struct ima_ns is reasonably small)?
> > or whether tying it to the mount namespace is the correct thing to
> > do. On
>
> Mostly the latter.  The other would be not so much space concerns as
> time concerns.  Many things use new mounts namespaces, and we
> wouldn't want multiple IMA calls on all file accesses by all of
> those.
>
> >
> > the latter, it does seem that this should be a property of either
> > the mount or user ns rather than its own separate ns. I could see
> > a use where even a container might want multiple ima keyrings
> > within the container (say containerised apache service with
> > multiple tenants), so instinct tells me that mount ns is the
> > correct granularity for this.
>
> I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> as the trigger for requesting a new ima ns on the next
> clone(CLONE_NEWNS).

I could go with that, but what about the trigger being installing or
updating the keyring? That's the only operation that needs namespace
separation, so on mount ns clone, you get a pointer to the old ima_ns
until you do something that requires a new key, which then triggers the
copy of the namespace and installing it?

James

2017-07-25 19:43:12

[permalink] [raw]

Subject: Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

...
> +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);
> ns_free_inum(&ns->ns);
> + free_ns_status_cache(ns);
> kfree(ns);
> }
>
> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> .parent = NULL,
> };
> EXPORT_SYMBOL(init_ima_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->i_ino > status->i_ino)
> + 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;
> +}
...
> +
> +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);
> + }

What prevents the status from being freed between the read_lock
in ima_ns_status_find() and the write_lock in the following line?

IIUC it's that ns is always current's ima_ns, which will pin the ns
and cause no statuses to be freed. But then the ns should probably
not be passed in here? Or a comment should say that ns must be
pinned?

Just trying to make sure I understand the locking.

> + 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.9.4

2017-07-25 19:48:18

[permalink] [raw]

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

On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > >
> > > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > >
> > > > On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> > > > >
> > > > >
> > > > > From: Yuqiong Sun <[email protected]>
> > > > >
> > > > > 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).
> > > > >
> > > > > Signed-off-by: Yuqiong Sun <[email protected]>
> > > > >
> > > > > Changelog:
> > > > > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > > >
> > > > Hi,
> > > >
> > > > So this means that every mount namespace clone will clone a new
> > > > IMA namespace.  Is that really ok?
> > >
> > > Based on what: space concerns (struct ima_ns is reasonably small)?
> > > or whether tying it to the mount namespace is the correct thing to
> > > do. On
> >
> > Mostly the latter.  The other would be not so much space concerns as
> > time concerns.  Many things use new mounts namespaces, and we
> > wouldn't want multiple IMA calls on all file accesses by all of
> > those.
> >
> > >
> > > the latter, it does seem that this should be a property of either
> > > the mount or user ns rather than its own separate ns. I could see
> > > a use where even a container might want multiple ima keyrings
> > > within the container (say containerised apache service with
> > > multiple tenants), so instinct tells me that mount ns is the
> > > correct granularity for this.
> >
> > I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> > as the trigger for requesting a new ima ns on the next
> > clone(CLONE_NEWNS).
>
> I could go with that, but what about the trigger being installing or
> updating the keyring? That's the only operation that needs namespace
> separation, so on mount ns clone, you get a pointer to the old ima_ns
> until you do something that requires a new key, which then triggers the
> copy of the namespace and installing it?

It isn't just the keyrings that need to be namespaced, but the
measurement list and policy as well.

IMA-measurement, IMA-appraisal and IMA-audit are all policy based.

As soon as the namespace starts, measurements should be added to the
namespace specific measurement list, not it's parent.

Mimi

2017-07-25 20:11:38

[permalink] [raw]

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

On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
>>>>>>
>>>>>> From: Yuqiong Sun <[email protected]>
>>>>>>
>>>>>> 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).
>>>>>>
>>>>>> Signed-off-by: Yuqiong Sun <[email protected]>
>>>>>>
>>>>>> Changelog:
>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>>>>> Hi,
>>>>>
>>>>> So this means that every mount namespace clone will clone a new
>>>>> IMA namespace. Is that really ok?
>>>> Based on what: space concerns (struct ima_ns is reasonably small)?
>>>> or whether tying it to the mount namespace is the correct thing to
>>>> do. On
>>> Mostly the latter. The other would be not so much space concerns as
>>> time concerns. Many things use new mounts namespaces, and we
>>> wouldn't want multiple IMA calls on all file accesses by all of
>>> those.
>>>
>>>> the latter, it does seem that this should be a property of either
>>>> the mount or user ns rather than its own separate ns. I could see
>>>> a use where even a container might want multiple ima keyrings
>>>> within the container (say containerised apache service with
>>>> multiple tenants), so instinct tells me that mount ns is the
>>>> correct granularity for this.
>>> I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
>>> as the trigger for requesting a new ima ns on the next
>>> clone(CLONE_NEWNS).
>> I could go with that, but what about the trigger being installing or
>> updating the keyring? That's the only operation that needs namespace
>> separation, so on mount ns clone, you get a pointer to the old ima_ns
>> until you do something that requires a new key, which then triggers the
>> copy of the namespace and installing it?
> It isn't just the keyrings that need to be namespaced, but the
> measurement list and policy as well.
>
> IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
>
> As soon as the namespace starts, measurements should be added to the
> namespace specific measurement list, not it's parent.

IMA is about measuring things, logging what was executed, and finally
someone looking at the measurement log and detecting 'things'. So at
least one attack that needs to be prevented is a malicious person
opening an IMA namespace, executing something malicious, and not leaving
any trace on the host because all the logs went into the measurement
list of the IMA namespace, which disappeared. That said, I am wondering
whether there has to be a minimum set of namespaces (PID, UTS)
providing enough 'isolation' that someone may actually open an IMA
namespace and run their code. To avoid leaving no traces one could argue
to implement recursive logging, so something that is logged inside the
namespace will be detected in all parent containers up to the
init_ima_ns (host) because it's logged (and TPM extended) there as well.
The challenge with that is that logging costs memory and that can be
abused as well until the machine needs a reboot... I guess the solution
could be requesting an IMA namespace in one way or another but requiring
several other namespace flags in the clone() to actually 'get' it.
Jumping namespaces with setns() may have to be restricted as well once
there is an IMA namespace.

Stefan

>
> Mimi
>

2017-07-25 20:15:41

[permalink] [raw]

Subject: Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
> ...
> > +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);
> > ns_free_inum(&ns->ns);
> > + free_ns_status_cache(ns);
> > kfree(ns);
> > }
> >
> > @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> > .parent = NULL,
> > };
> > EXPORT_SYMBOL(init_ima_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->i_ino > status->i_ino)
> > + 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;
> > +}
> ...
> > +
> > +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);
> > + }
>
> What prevents the status from being freed between the read_lock
> in ima_ns_status_find() and the write_lock in the following line?
>
> IIUC it's that ns is always current's ima_ns, which will pin the ns
> and cause no statuses to be freed. But then the ns should probably
> not be passed in here? Or a comment should say that ns must be
> pinned?
>
> Just trying to make sure I understand the locking.

iint's are only freed after the last reference to the inode is deleted
in __fput(). Refer to ima_file_free(). ns_status is a bit different
in that they are freed on namespace cleanup.

Mimi

> > + 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.9.
>

2017-07-25 20:25:24

[permalink] [raw]

Subject: Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

On 07/25/2017 04:15 PM, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
>> ...
>>> +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);
>>> ns_free_inum(&ns->ns);
>>> + free_ns_status_cache(ns);
>>> kfree(ns);
>>> }
>>>
>>> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
>>> .parent = NULL,
>>> };
>>> EXPORT_SYMBOL(init_ima_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->i_ino > status->i_ino)
>>> + 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;
>>> +}
>> ...
>>> +
>>> +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);
>>> + }
>> What prevents the status from being freed between the read_lock
>> in ima_ns_status_find() and the write_lock in the following line?
>>
>> IIUC it's that ns is always current's ima_ns, which will pin the ns
>> and cause no statuses to be freed. But then the ns should probably
>> not be passed in here? Or a comment should say that ns must be
>> pinned?
>>
>> Just trying to make sure I understand the locking.
> iint's are only freed after the last reference to the inode is deleted
> in __fput(). Refer to ima_file_free(). ns_status is a bit different
> in that they are freed on namespace cleanup.

It should be possible to move the write_lock() above the

status = ima_ns_status_find(ns, inode);

and instead call __ima_ns_status_find() with the write_lock() held.

Stefan

>
> Mimi
>
>>> + 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.9.

2017-07-25 20:31:44

by James Bottomley

[permalink] [raw]

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

On Tue, 2017-07-25 at 15:48 -0400, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> >
> > On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > >
> > > On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > >
> > > >
> > > > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
[...]
> > > > the latter, it does seem that this should be a property of
> > > > either the mount or user ns rather than its own separate ns. I
> > > > could see a use where even a container might want multiple ima
> > > > keyrings within the container (say containerised apache service
> > > > with multiple tenants), so instinct tells me that mount ns is
> > > > the correct granularity for this.
> > >
> > > I wonder whether we could use echo 1 >
> > > /sys/kernel/security/ima/newns
> > > as the trigger for requesting a new ima ns on the next
> > > clone(CLONE_NEWNS).
> >
> > I could go with that, but what about the trigger being installing
> > or updating the keyring? That's the only operation that needs
> > namespace separation, so on mount ns clone, you get a pointer to
> > the old ima_ns until you do something that requires a new key,
> > which then triggers the copy of the namespace and installing it?
>
> It isn't just the keyrings that need to be namespaced, but the
> measurement list and policy as well.

OK, so trigger to do a just in time copy would be new key or new
policy. The measurement list is basically just a has of a file taken
at a policy point. Presumably it doesn't change if we install a new
policy or key, so it sounds like it should be tied to the underlying
mount point? I'm thinking if we set up a hundred mount ns each
pointing to /var/container, we don't want /var/container/bin/something
to have 100 separate measurements each with the same hash.

> IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
>
> As soon as the namespace starts, measurements should be added to the
> namespace specific measurement list, not it's parent.

Would the measurement in a child namespace yield a different
measurement in the parent? I'm thinking not, because a measurement is
just a hash. Now if the signature of the hash in the xattr needs a
different key, obviously this differs, but the expensive part
(computing the hash) shouldn't change.

James

> Mimi
>
> _______________________________________________
> Containers mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/containers

2017-07-25 20:46:18

[permalink] [raw]

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

On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> >>>>>>
> >>>>>>From: Yuqiong Sun <[email protected]>
> >>>>>>
> >>>>>>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).
> >>>>>>
> >>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> >>>>>>
> >>>>>>Changelog:
> >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> >>>>>Hi,
> >>>>>
> >>>>>So this means that every mount namespace clone will clone a new
> >>>>>IMA namespace. Is that really ok?
> >>>>Based on what: space concerns (struct ima_ns is reasonably small)?
> >>>>or whether tying it to the mount namespace is the correct thing to
> >>>>do. On
> >>>Mostly the latter. The other would be not so much space concerns as
> >>>time concerns. Many things use new mounts namespaces, and we
> >>>wouldn't want multiple IMA calls on all file accesses by all of
> >>>those.
> >>>
> >>>>the latter, it does seem that this should be a property of either
> >>>>the mount or user ns rather than its own separate ns. I could see
> >>>>a use where even a container might want multiple ima keyrings
> >>>>within the container (say containerised apache service with
> >>>>multiple tenants), so instinct tells me that mount ns is the
> >>>>correct granularity for this.
> >>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> >>>as the trigger for requesting a new ima ns on the next
> >>>clone(CLONE_NEWNS).
> >>I could go with that, but what about the trigger being installing or
> >>updating the keyring? That's the only operation that needs namespace
> >>separation, so on mount ns clone, you get a pointer to the old ima_ns
> >>until you do something that requires a new key, which then triggers the
> >>copy of the namespace and installing it?
> >It isn't just the keyrings that need to be namespaced, but the
> >measurement list and policy as well.
> >
> >IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
> >
> >As soon as the namespace starts, measurements should be added to the
> >namespace specific measurement list, not it's parent.

Shouldn't it be both?

If not, then it seems to me this must be tied to user namespace.

> IMA is about measuring things, logging what was executed, and
> finally someone looking at the measurement log and detecting
> 'things'. So at least one attack that needs to be prevented is a
> malicious person opening an IMA namespace, executing something
> malicious, and not leaving any trace on the host because all the
> logs went into the measurement list of the IMA namespace, which
> disappeared. That said, I am wondering whether there has to be a
> minimum set of namespaces (PID, UTS) providing enough 'isolation'
> that someone may actually open an IMA namespace and run their code.
> To avoid leaving no traces one could argue to implement recursive
> logging, so something that is logged inside the namespace will be
> detected in all parent containers up to the init_ima_ns (host)
> because it's logged (and TPM extended) there as well. The challenge
> with that is that logging costs memory and that can be abused as
> well until the machine needs a reboot... I guess the solution could
> be requesting an IMA namespace in one way or another but requiring
> several other namespace flags in the clone() to actually 'get' it.
> Jumping namespaces with setns() may have to be restricted as well
> once there is an IMA namespace.

Wait. So if I create a new IMA namespace, the things I run in
that namespace are not subject to the parent namespace policy?

-serge

2017-07-25 20:47:35

[permalink] [raw]

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

On Tue, 2017-07-25 at 13:31 -0700, James Bottomley wrote:
> On Tue, 2017-07-25 at 15:48 -0400, Mimi Zohar wrote:
> > On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > >
> > > On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > >
> > > > On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > > >
> > > > >
> > > > > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> [...]
> > > > > the latter, it does seem that this should be a property of
> > > > > either the mount or user ns rather than its own separate ns. I
> > > > > could see a use where even a container might want multiple ima
> > > > > keyrings within the container (say containerised apache service
> > > > > with multiple tenants), so instinct tells me that mount ns is
> > > > > the correct granularity for this.
> > > >
> > > > I wonder whether we could use echo 1 >
> > > > /sys/kernel/security/ima/newns
> > > > as the trigger for requesting a new ima ns on the next
> > > > clone(CLONE_NEWNS).
> > >
> > > I could go with that, but what about the trigger being installing
> > > or updating the keyring? That's the only operation that needs
> > > namespace separation, so on mount ns clone, you get a pointer to
> > > the old ima_ns until you do something that requires a new key,
> > > which then triggers the copy of the namespace and installing it?
> >
> > It isn't just the keyrings that need to be namespaced, but the
> > measurement list and policy as well.
>
> OK, so trigger to do a just in time copy would be new key or new
> policy.

The kernel has support for an initial builtin policy, which can be
later replaced. The builtin policies, if specified, begin measuring
files very early in the boot process. Similarly for namespace, we
would want to start measuring files as early as possible.

> The measurement list is basically just a has of a file taken
> at a policy point. Presumably it doesn't change if we install a new
> policy or key, so it sounds like it should be tied to the underlying
> mount point? I'm thinking if we set up a hundred mount ns each
> pointing to /var/container, we don't want /var/container/bin/something
> to have 100 separate measurements each with the same hash.
>
> > IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
> >
> > As soon as the namespace starts, measurements should be added to the
> > namespace specific measurement list, not it's parent.
>
> Would the measurement in a child namespace yield a different
> measurement in the parent? I'm thinking not, because a measurement is
> just a hash. Now if the signature of the hash in the xattr needs a
> different key, obviously this differs, but the expensive part
> (computing the hash) shouldn't change.

Depending on the measurement list template format (eg. ima-ng, ima-
sig, custom template format), the template data would contain the file
hash, but in addition it might contain the file signature. As keys
could be namespace specific, the file signatures could be different.

Mimi

2017-07-25 20:49:39

[permalink] [raw]

Subject: Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

On Tue, Jul 25, 2017 at 04:15:25PM -0400, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
> > ...
> > > +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);
> > > ns_free_inum(&ns->ns);
> > > + free_ns_status_cache(ns);
> > > kfree(ns);
> > > }
> > >
> > > @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> > > .parent = NULL,
> > > };
> > > EXPORT_SYMBOL(init_ima_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->i_ino > status->i_ino)
> > > + 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;
> > > +}
> > ...
> > > +
> > > +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);
> > > + }
> >
> > What prevents the status from being freed between the read_lock
> > in ima_ns_status_find() and the write_lock in the following line?
> >
> > IIUC it's that ns is always current's ima_ns, which will pin the ns
> > and cause no statuses to be freed. But then the ns should probably
> > not be passed in here? Or a comment should say that ns must be
> > pinned?
> >
> > Just trying to make sure I understand the locking.
>
> iint's are only freed after the last reference to the inode is deleted
> in __fput(). ?Refer to ima_file_free(). ?ns_status is a bit different
> in that they are freed on namespace cleanup.

Ok, thanks - that sounds ok then.

2017-07-25 20:58:19

[permalink] [raw]

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

On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> > >>>>>>
> > >>>>>>From: Yuqiong Sun <[email protected]>
> > >>>>>>
> > >>>>>>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).
> > >>>>>>
> > >>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> > >>>>>>
> > >>>>>>Changelog:
> > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > >>>>>Hi,
> > >>>>>
> > >>>>>So this means that every mount namespace clone will clone a new
> > >>>>>IMA namespace. Is that really ok?
> > >>>>Based on what: space concerns (struct ima_ns is reasonably small)?
> > >>>>or whether tying it to the mount namespace is the correct thing to
> > >>>>do. On
> > >>>Mostly the latter. The other would be not so much space concerns as
> > >>>time concerns. Many things use new mounts namespaces, and we
> > >>>wouldn't want multiple IMA calls on all file accesses by all of
> > >>>those.
> > >>>
> > >>>>the latter, it does seem that this should be a property of either
> > >>>>the mount or user ns rather than its own separate ns. I could see
> > >>>>a use where even a container might want multiple ima keyrings
> > >>>>within the container (say containerised apache service with
> > >>>>multiple tenants), so instinct tells me that mount ns is the
> > >>>>correct granularity for this.
> > >>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> > >>>as the trigger for requesting a new ima ns on the next
> > >>>clone(CLONE_NEWNS).
> > >>I could go with that, but what about the trigger being installing or
> > >>updating the keyring? That's the only operation that needs namespace
> > >>separation, so on mount ns clone, you get a pointer to the old ima_ns
> > >>until you do something that requires a new key, which then triggers the
> > >>copy of the namespace and installing it?
> > >It isn't just the keyrings that need to be namespaced, but the
> > >measurement list and policy as well.
> > >
> > >IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
> > >
> > >As soon as the namespace starts, measurements should be added to the
> > >namespace specific measurement list, not it's parent.
>
> Shouldn't it be both?

The policy defines which files are measured. The namespace policy
could be different than it's parent's policy, and the parent's policy
could be different than the native policy. Basically, file
measurements need to be added to the namespace measurement list,
recursively, up to the native measurement list.

Mimi

>
> If not, then it seems to me this must be tied to user namespace.
>
> > IMA is about measuring things, logging what was executed, and
> > finally someone looking at the measurement log and detecting
> > 'things'. So at least one attack that needs to be prevented is a
> > malicious person opening an IMA namespace, executing something
> > malicious, and not leaving any trace on the host because all the
> > logs went into the measurement list of the IMA namespace, which
> > disappeared. That said, I am wondering whether there has to be a
> > minimum set of namespaces (PID, UTS) providing enough 'isolation'
> > that someone may actually open an IMA namespace and run their code.
> > To avoid leaving no traces one could argue to implement recursive
> > logging, so something that is logged inside the namespace will be
> > detected in all parent containers up to the init_ima_ns (host)
> > because it's logged (and TPM extended) there as well. The challenge
> > with that is that logging costs memory and that can be abused as
> > well until the machine needs a reboot... I guess the solution could
> > be requesting an IMA namespace in one way or another but requiring
> > several other namespace flags in the clone() to actually 'get' it.
> > Jumping namespaces with setns() may have to be restricted as well
> > once there is an IMA namespace.
>
> Wait. So if I create a new IMA namespace, the things I run in
> that namespace are not subject to the parent namespace policy?

2017-07-25 21:07:57

[permalink] [raw]

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

On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> > > >>>>>>
> > > >>>>>>From: Yuqiong Sun <[email protected]>
> > > >>>>>>
> > > >>>>>>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).
> > > >>>>>>
> > > >>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> > > >>>>>>
> > > >>>>>>Changelog:
> > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > > >>>>>Hi,
> > > >>>>>
> > > >>>>>So this means that every mount namespace clone will clone a new
> > > >>>>>IMA namespace. Is that really ok?
> > > >>>>Based on what: space concerns (struct ima_ns is reasonably small)?
> > > >>>>or whether tying it to the mount namespace is the correct thing to
> > > >>>>do. On
> > > >>>Mostly the latter. The other would be not so much space concerns as
> > > >>>time concerns. Many things use new mounts namespaces, and we
> > > >>>wouldn't want multiple IMA calls on all file accesses by all of
> > > >>>those.
> > > >>>
> > > >>>>the latter, it does seem that this should be a property of either
> > > >>>>the mount or user ns rather than its own separate ns. I could see
> > > >>>>a use where even a container might want multiple ima keyrings
> > > >>>>within the container (say containerised apache service with
> > > >>>>multiple tenants), so instinct tells me that mount ns is the
> > > >>>>correct granularity for this.
> > > >>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> > > >>>as the trigger for requesting a new ima ns on the next
> > > >>>clone(CLONE_NEWNS).
> > > >>I could go with that, but what about the trigger being installing or
> > > >>updating the keyring? That's the only operation that needs namespace
> > > >>separation, so on mount ns clone, you get a pointer to the old ima_ns
> > > >>until you do something that requires a new key, which then triggers the
> > > >>copy of the namespace and installing it?
> > > >It isn't just the keyrings that need to be namespaced, but the
> > > >measurement list and policy as well.
> > > >
> > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
> > > >
> > > >As soon as the namespace starts, measurements should be added to the
> > > >namespace specific measurement list, not it's parent.
> >
> > Shouldn't it be both?
>
> The policy defines which files are measured. ?The namespace policy
> could be different than it's parent's policy, and the parent's policy
> could be different than the native policy. ?Basically, file
> measurements need to be added to the namespace measurement list,
> recursively, up to the native measurement list.

Yes, but if a task t1 is in namespace ns2 which is a child of namespace ns1,
and it accesses a file which ns1's policy says must be measured, then will
ns1's required measurement happen (and be appended to the ns1 measurement
list), whether or not ns2's policy requires it?

2017-07-25 21:35:50

[permalink] [raw]

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

On 07/25/2017 04:46 PM, Serge E. Hallyn wrote:
> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
>> On 07/25/2017 03:48 PM, Mimi Zohar wrote:
>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
>>>>>>>> From: Yuqiong Sun <[email protected]>
>>>>>>>>
>>>>>>>> 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).
>>>>>>>>
>>>>>>>> Signed-off-by: Yuqiong Sun <[email protected]>
>>>>>>>>
>>>>>>>> Changelog:
>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>>>>>>> Hi,
>>>>>>>
>>>>>>> So this means that every mount namespace clone will clone a new
>>>>>>> IMA namespace. Is that really ok?
>>>>>> Based on what: space concerns (struct ima_ns is reasonably small)?
>>>>>> or whether tying it to the mount namespace is the correct thing to
>>>>>> do. On
>>>>> Mostly the latter. The other would be not so much space concerns as
>>>>> time concerns. Many things use new mounts namespaces, and we
>>>>> wouldn't want multiple IMA calls on all file accesses by all of
>>>>> those.
>>>>>
>>>>>> the latter, it does seem that this should be a property of either
>>>>>> the mount or user ns rather than its own separate ns. I could see
>>>>>> a use where even a container might want multiple ima keyrings
>>>>>> within the container (say containerised apache service with
>>>>>> multiple tenants), so instinct tells me that mount ns is the
>>>>>> correct granularity for this.
>>>>> I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
>>>>> as the trigger for requesting a new ima ns on the next
>>>>> clone(CLONE_NEWNS).
>>>> I could go with that, but what about the trigger being installing or
>>>> updating the keyring? That's the only operation that needs namespace
>>>> separation, so on mount ns clone, you get a pointer to the old ima_ns
>>>> until you do something that requires a new key, which then triggers the
>>>> copy of the namespace and installing it?
>>> It isn't just the keyrings that need to be namespaced, but the
>>> measurement list and policy as well.
>>>
>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
>>>
>>> As soon as the namespace starts, measurements should be added to the
>>> namespace specific measurement list, not it's parent.
> Shouldn't it be both?
>
> If not, then it seems to me this must be tied to user namespace.
>
>> IMA is about measuring things, logging what was executed, and
>> finally someone looking at the measurement log and detecting
>> 'things'. So at least one attack that needs to be prevented is a
>> malicious person opening an IMA namespace, executing something
>> malicious, and not leaving any trace on the host because all the
>> logs went into the measurement list of the IMA namespace, which
>> disappeared. That said, I am wondering whether there has to be a
>> minimum set of namespaces (PID, UTS) providing enough 'isolation'
>> that someone may actually open an IMA namespace and run their code.
>> To avoid leaving no traces one could argue to implement recursive
>> logging, so something that is logged inside the namespace will be
>> detected in all parent containers up to the init_ima_ns (host)
>> because it's logged (and TPM extended) there as well. The challenge
>> with that is that logging costs memory and that can be abused as
>> well until the machine needs a reboot... I guess the solution could
>> be requesting an IMA namespace in one way or another but requiring
>> several other namespace flags in the clone() to actually 'get' it.
>> Jumping namespaces with setns() may have to be restricted as well
>> once there is an IMA namespace.
> Wait. So if I create a new IMA namespace, the things I run in
> that namespace are not subject to the parent namespace policy?

We would treat the IMA namespace policy independently of the host
policy. A user can sign his containers' files with his own keys, upload
the container to the cloud and run them with keys that are different
than those of the host. The keys (actually certs) would be found in the
container, same for the policy.

Stefan

>
> -serge
>

2017-07-25 22:22:44

[permalink] [raw]

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

On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
> On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> > > > >>>>>>
> > > > >>>>>>From: Yuqiong Sun <[email protected]>
> > > > >>>>>>
> > > > >>>>>>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).
> > > > >>>>>>
> > > > >>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> > > > >>>>>>
> > > > >>>>>>Changelog:
> > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > > > >>>>>Hi,
> > > > >>>>>
> > > > >>>>>So this means that every mount namespace clone will clone a new
> > > > >>>>>IMA namespace. Is that really ok?
> > > > >>>>Based on what: space concerns (struct ima_ns is reasonably small)?
> > > > >>>>or whether tying it to the mount namespace is the correct thing to
> > > > >>>>do. On
> > > > >>>Mostly the latter. The other would be not so much space concerns as
> > > > >>>time concerns. Many things use new mounts namespaces, and we
> > > > >>>wouldn't want multiple IMA calls on all file accesses by all of
> > > > >>>those.
> > > > >>>
> > > > >>>>the latter, it does seem that this should be a property of either
> > > > >>>>the mount or user ns rather than its own separate ns. I could see
> > > > >>>>a use where even a container might want multiple ima keyrings
> > > > >>>>within the container (say containerised apache service with
> > > > >>>>multiple tenants), so instinct tells me that mount ns is the
> > > > >>>>correct granularity for this.
> > > > >>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> > > > >>>as the trigger for requesting a new ima ns on the next
> > > > >>>clone(CLONE_NEWNS).
> > > > >>I could go with that, but what about the trigger being installing or
> > > > >>updating the keyring? That's the only operation that needs namespace
> > > > >>separation, so on mount ns clone, you get a pointer to the old ima_ns
> > > > >>until you do something that requires a new key, which then triggers the
> > > > >>copy of the namespace and installing it?
> > > > >It isn't just the keyrings that need to be namespaced, but the
> > > > >measurement list and policy as well.
> > > > >
> > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
> > > > >
> > > > >As soon as the namespace starts, measurements should be added to the
> > > > >namespace specific measurement list, not it's parent.
> > >
> > > Shouldn't it be both?
> >
> > The policy defines which files are measured. The namespace policy
> > could be different than it's parent's policy, and the parent's policy
> > could be different than the native policy. Basically, file
> > measurements need to be added to the namespace measurement list,
> > recursively, up to the native measurement list.
>
> Yes, but if a task t1 is in namespace ns2 which is a child of namespace ns1,
> and it accesses a file which ns1's policy says must be measured, then will
> ns1's required measurement happen (and be appended to the ns1 measurement
> list), whether or not ns2's policy requires it?

Yes, as the file needs to be measured only in the ns1 policy, the
measurement would exist in the ns1 measurement list, but not in the
ns2 measurement list. The pseudo code snippet below might help.

do {
.
.

/* calculate file hash based on xattr algorithm */
collect_measurement()

/* recursively added to each namespace based on policy */
ima_store_measurement()

/* Based on the specific namespace policy and keys. */
if (!once) {
once = 1;
result = ima_appraise_measurement()
}

ima_audit_measurement()

} while ((ns = ns->parent));

return result;

Mimi

2017-07-27 12:51:41

[permalink] [raw]

Subject: RE: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

> -----Original Message-----
> From: Mimi Zohar [mailto:[email protected]]
> Sent: terça-feira, 25 de julho de 2017 18:29
> To: Serge E. Hallyn <[email protected]>
> Cc: Mehmet Kayaalp <[email protected]>; Yuqiong Sun
> <[email protected]>; containers <[email protected]
> foundation.org>; linux-kernel <[email protected]>; David Safford
> <[email protected]>; James Bottomley
> <[email protected]>; linux-security-module <linux-
> [email protected]>; ima-devel <linux-ima-
> [email protected]>; Yuqiong Sun <[email protected]>
> Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA
> namespace support
>
> On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
> > On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> > > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> > > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp
> wrote:
> > > > > >>>>>>
> > > > > >>>>>>From: Yuqiong Sun <[email protected]>
> > > > > >>>>>>
> > > > > >>>>>>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).
> > > > > >>>>>>
> > > > > >>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> > > > > >>>>>>
> > > > > >>>>>>Changelog:
> > > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > > > > >>>>>Hi,
> > > > > >>>>>
> > > > > >>>>>So this means that every mount namespace clone will clone a
> > > > > >>>>>new IMA namespace. Is that really ok?
> > > > > >>>>Based on what: space concerns (struct ima_ns is reasonably
> small)?
> > > > > >>>>or whether tying it to the mount namespace is the correct
> > > > > >>>>thing to do. On
> > > > > >>>Mostly the latter. The other would be not so much space
> > > > > >>>concerns as time concerns. Many things use new mounts
> > > > > >>>namespaces, and we wouldn't want multiple IMA calls on all
> > > > > >>>file accesses by all of those.
> > > > > >>>
> > > > > >>>>the latter, it does seem that this should be a property of
> > > > > >>>>either the mount or user ns rather than its own separate ns.
> > > > > >>>>I could see a use where even a container might want multiple
> > > > > >>>>ima keyrings within the container (say containerised apache
> > > > > >>>>service with multiple tenants), so instinct tells me that
> > > > > >>>>mount ns is the correct granularity for this.
> > > > > >>>I wonder whether we could use echo 1 >
> > > > > >>>/sys/kernel/security/ima/newns as the trigger for requesting
> > > > > >>>a new ima ns on the next clone(CLONE_NEWNS).
> > > > > >>I could go with that, but what about the trigger being
> > > > > >>installing or updating the keyring? That's the only operation
> > > > > >>that needs namespace separation, so on mount ns clone, you get
> > > > > >>a pointer to the old ima_ns until you do something that
> > > > > >>requires a new key, which then triggers the copy of the namespace
> and installing it?
> > > > > >It isn't just the keyrings that need to be namespaced, but the
> > > > > >measurement list and policy as well.
> > > > > >
> > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy
> based.
> > > > > >
> > > > > >As soon as the namespace starts, measurements should be added
> > > > > >to the namespace specific measurement list, not it's parent.
> > > >
> > > > Shouldn't it be both?
> > >
> > > The policy defines which files are measured. The namespace policy
> > > could be different than it's parent's policy, and the parent's
> > > policy could be different than the native policy. Basically, file
> > > measurements need to be added to the namespace measurement list,
> > > recursively, up to the native measurement list.
> >
> > Yes, but if a task t1 is in namespace ns2 which is a child of
> > namespace ns1, and it accesses a file which ns1's policy says must be
> > measured, then will ns1's required measurement happen (and be
> appended
> > to the ns1 measurement list), whether or not ns2's policy requires it?
>
> Yes, as the file needs to be measured only in the ns1 policy, the
> measurement would exist in the ns1 measurement list, but not in the
> ns2 measurement list. The pseudo code snippet below might help.

This algorithm is potentially extending a PCR in TPM multiple times for a single file event under a given namespace and duplicating entries. Any concerns with performance and memory footprint?
What is the reason to adding a measurement to multiple namespace measurement lists? How are these lists going to be used? For Remote Attestation we need a single list (the native one) which has the complete list of measurements and in the same order they were extended in the TPM. Additionally, when namespaces are released, would the measurement list under that namespace disappear? How to store this list considering the namespaces may have a short life and be reused thousands of times?
Should the native measurement list have all measurements triggered in the whole system, including the ones made under other namespaces? Following the algorithm below, if the file is not in the policy of the 'native'/initial namespace, the measurement is not added to the native measurement list.
Each measurement entry in the list could have new fields to identify the namespace. Since the namespaces can be reused, a timestamp or others fields could be added to uniquely identify the namespace id.
Regarding namespace hierarchy and IMA policy, we could assume that if a given namespace has no policy set, the policy of that namespace parent is applied and then the native/initial namespace should always have a set policy.

>
> do {
> .
> .
>
> /* calculate file hash based on xattr algorithm */
> collect_measurement()
>
> /* recursively added to each namespace based on policy */
> ima_store_measurement()
>
> /* Based on the specific namespace policy and keys. */
> if (!once) {
> once = 1;
> result = ima_appraise_measurement()
> }
>
> ima_audit_measurement()
>
> } while ((ns = ns->parent));
>
> return result;
>
> Mimi
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most engaging
> tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-ima-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel

2017-07-27 14:40:58

[permalink] [raw]

Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D-
CL) wrote:
>
>
> > On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
> > > On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> > > > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> > > > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > > > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > > > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > > > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp
> > wrote:
> > > > > > >>>>>>
> > > > > > >>>>>>From: Yuqiong Sun <[email protected]>
> > > > > > >>>>>>
> > > > > > >>>>>>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).
> > > > > > >>>>>>
> > > > > > >>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> > > > > > >>>>>>
> > > > > > >>>>>>Changelog:
> > > > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > > > > > >>>>>Hi,
> > > > > > >>>>>
> > > > > > >>>>>So this means that every mount namespace clone will clone a
> > > > > > >>>>>new IMA namespace. Is that really ok?
> > > > > > >>>>Based on what: space concerns (struct ima_ns is reasonably
> > small)?
> > > > > > >>>>or whether tying it to the mount namespace is the correct
> > > > > > >>>>thing to do. On
> > > > > > >>>Mostly the latter. The other would be not so much space
> > > > > > >>>concerns as time concerns. Many things use new mounts
> > > > > > >>>namespaces, and we wouldn't want multiple IMA calls on all
> > > > > > >>>file accesses by all of those.
> > > > > > >>>
> > > > > > >>>>the latter, it does seem that this should be a property of
> > > > > > >>>>either the mount or user ns rather than its own separate ns.
> > > > > > >>>>I could see a use where even a container might want multiple
> > > > > > >>>>ima keyrings within the container (say containerised apache
> > > > > > >>>>service with multiple tenants), so instinct tells me that
> > > > > > >>>>mount ns is the correct granularity for this.
> > > > > > >>>I wonder whether we could use echo 1 >
> > > > > > >>>/sys/kernel/security/ima/newns as the trigger for requesting
> > > > > > >>>a new ima ns on the next clone(CLONE_NEWNS).
> > > > > > >>I could go with that, but what about the trigger being
> > > > > > >>installing or updating the keyring? That's the only operation
> > > > > > >>that needs namespace separation, so on mount ns clone, you get
> > > > > > >>a pointer to the old ima_ns until you do something that
> > > > > > >>requires a new key, which then triggers the copy of the namespace
> > and installing it?
> > > > > > >It isn't just the keyrings that need to be namespaced, but the
> > > > > > >measurement list and policy as well.
> > > > > > >
> > > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy
> > based.
> > > > > > >
> > > > > > >As soon as the namespace starts, measurements should be added
> > > > > > >to the namespace specific measurement list, not it's parent.
> > > > >
> > > > > Shouldn't it be both?
> > > >
> > > > The policy defines which files are measured. The namespace policy
> > > > could be different than it's parent's policy, and the parent's
> > > > policy could be different than the native policy. Basically, file
> > > > measurements need to be added to the namespace measurement list,
> > > > recursively, up to the native measurement list.
> > >
> > > Yes, but if a task t1 is in namespace ns2 which is a child of
> > > namespace ns1, and it accesses a file which ns1's policy says must be
> > > measured, then will ns1's required measurement happen (and be
> > appended
> > > to the ns1 measurement list), whether or not ns2's policy requires it?
> >
> > Yes, as the file needs to be measured only in the ns1 policy, the
> > measurement would exist in the ns1 measurement list, but not in the
> > ns2 measurement list. The pseudo code snippet below might help.
>
> This algorithm is potentially extending a PCR in TPM multiple times
> for a single file event under a given namespace and duplicating
> entries. Any concerns with performance and memory footprint?

Going forward we assume associated with each container will be a vTPM.
The namespace measurements will extend a vTPM. As the container comes
and goes the associated measurement list, keyring, and vTPM will come
and go as well. For this reason, based on policy, the same file
measurement might appear in multiple measurement lists.

> What is the reason to adding a measurement to multiple namespace
> measurement lists? How are these lists going to be used? For Remote
> Attestation we need a single list (the native one) which has the
> complete list of measurements and in the same order they were
> extended in the TPM. Additionally, when namespaces are released,
> would the measurement list under that namespace disappear? How to
> store this list considering the namespaces may have a short life and
> be reused thousands of times?

Different scenarios have different requirements. You're assuming that
only the system owner is interested in the measurement list, not the
container owner.

The current builtin measurement policies measure everything executed
on the system and anything accessed by real root. The namespace
policy would probably be similar, but instead of measuring files
accessed by real root, it would be files accessed by root in the
namespace.

> Should the native measurement list have all measurements triggered
> in the whole system, including the ones made under other namespaces?
> Following the algorithm below, if the file is not in the policy of
> the 'native'/initial namespace, the measurement is not added to the
> native measurement list.

Correct. The policy controls what is included measured, appraised,
and audited.

> Each measurement entry in the list could have new fields to identify
> the namespace. Since the namespaces can be reused, a timestamp or
> others fields could be added to uniquely identify the namespace id.

The more fields included in the measurement list, the more
measurements will be added to the measurement list. Wouldn't it be
enough to know that a certain file has been accessed/executed on the
system and base any analytics/forensics on the IMA-audit data.

> Regarding namespace hierarchy and IMA policy, we could assume that
> if a given namespace has no policy set, the policy of that namespace
> parent is applied and then the native/initial namespace should
> always have a set policy.

We shouldn't assume measure, appraise, or audit by default. Just like
it is up to the native system to define a policy or if there is a
policy, the "container" owner should define the policy, or if there is
a policy.

Mimi

> >
> > do {
> > .
> > .
> >
> > /* calculate file hash based on xattr algorithm */
> > collect_measurement()
> >
> > /* recursively added to each namespace based on policy */
> > ima_store_measurement()
> >
> > /* Based on the specific namespace policy and keys. */
> > if (!once) {
> > once = 1;
> > result = ima_appraise_measurement()
> > }
> >
> > ima_audit_measurement()
> >
> > } while ((ns = ns->parent));
> >
> > return result;
> >
> > Mimi

2017-07-27 17:18:30

[permalink] [raw]

Subject: RE: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

> -----Original Message-----
> From: Mimi Zohar [mailto:[email protected]]
> Sent: quinta-feira, 27 de julho de 2017 11:39
> To: Magalhaes, Guilherme (Brazil R&D-CL)
> <[email protected]>; Serge E. Hallyn <[email protected]>
> Cc: Mehmet Kayaalp <[email protected]>; Yuqiong Sun
> <[email protected]>; containers <[email protected]
> foundation.org>; linux-kernel <[email protected]>; David Safford
> <[email protected]>; James Bottomley
> <[email protected]>; linux-security-module <linux-
> [email protected]>; ima-devel <linux-ima-
> [email protected]>; Yuqiong Sun <[email protected]>
> Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA
> namespace support
>
> On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D-
> CL) wrote:
> >
> >
> > > On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
> > > > On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> > > > > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> > > > > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > > > > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > > > > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > > > > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > > > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley
> wrote:
> > > > > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > > > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp
> > > wrote:
> > > > > > > >>>>>>
> > > > > > > >>>>>>From: Yuqiong Sun <[email protected]>
> > > > > > > >>>>>>
> > > > > > > >>>>>>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).
> > > > > > > >>>>>>
> > > > > > > >>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> > > > > > > >>>>>>
> > > > > > > >>>>>>Changelog:
> > > > > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA
> flag
> > > > > > > >>>>>Hi,
> > > > > > > >>>>>
> > > > > > > >>>>>So this means that every mount namespace clone will clone
> a
> > > > > > > >>>>>new IMA namespace. Is that really ok?
> > > > > > > >>>>Based on what: space concerns (struct ima_ns is reasonably
> > > small)?
> > > > > > > >>>>or whether tying it to the mount namespace is the correct
> > > > > > > >>>>thing to do. On
> > > > > > > >>>Mostly the latter. The other would be not so much space
> > > > > > > >>>concerns as time concerns. Many things use new mounts
> > > > > > > >>>namespaces, and we wouldn't want multiple IMA calls on all
> > > > > > > >>>file accesses by all of those.
> > > > > > > >>>
> > > > > > > >>>>the latter, it does seem that this should be a property of
> > > > > > > >>>>either the mount or user ns rather than its own separate ns.
> > > > > > > >>>>I could see a use where even a container might want multiple
> > > > > > > >>>>ima keyrings within the container (say containerised apache
> > > > > > > >>>>service with multiple tenants), so instinct tells me that
> > > > > > > >>>>mount ns is the correct granularity for this.
> > > > > > > >>>I wonder whether we could use echo 1 >
> > > > > > > >>>/sys/kernel/security/ima/newns as the trigger for requesting
> > > > > > > >>>a new ima ns on the next clone(CLONE_NEWNS).
> > > > > > > >>I could go with that, but what about the trigger being
> > > > > > > >>installing or updating the keyring? That's the only operation
> > > > > > > >>that needs namespace separation, so on mount ns clone, you
> get
> > > > > > > >>a pointer to the old ima_ns until you do something that
> > > > > > > >>requires a new key, which then triggers the copy of the
> namespace
> > > and installing it?
> > > > > > > >It isn't just the keyrings that need to be namespaced, but the
> > > > > > > >measurement list and policy as well.
> > > > > > > >
> > > > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy
> > > based.
> > > > > > > >
> > > > > > > >As soon as the namespace starts, measurements should be
> added
> > > > > > > >to the namespace specific measurement list, not it's parent.
> > > > > >
> > > > > > Shouldn't it be both?
> > > > >
> > > > > The policy defines which files are measured. ?The namespace policy
> > > > > could be different than it's parent's policy, and the parent's
> > > > > policy could be different than the native policy. ?Basically, file
> > > > > measurements need to be added to the namespace measurement
> list,
> > > > > recursively, up to the native measurement list.
> > > >
> > > > Yes, but if a task t1 is in namespace ns2 which is a child of
> > > > namespace ns1, and it accesses a file which ns1's policy says must be
> > > > measured, then will ns1's required measurement happen (and be
> > > appended
> > > > to the ns1 measurement list), whether or not ns2's policy requires it?
> > >
> > > Yes, as the file needs to be measured only in the ns1 policy, the
> > > measurement would exist in the ns1 measurement list, but not in the
> > > ns2 measurement list. ?The pseudo code snippet below might help.
> >
> > This algorithm is potentially extending a PCR in TPM multiple times
> > for a single file event under a given namespace and duplicating
> > entries. Any concerns with performance and memory footprint?
>
> Going forward we assume associated with each container will be a vTPM.
> The namespace measurements will extend a vTPM. ?As the container comes
> and goes the associated measurement list, keyring, and vTPM will come
> and go as well. ?For this reason, based on policy, the same file
> measurement might appear in multiple measurement lists.

My concern is that the base of namespacing the measurement lists is on the
integration of containers with vTPM. Associating vTPM with containers as
they are today is not a simple integration since vTPM requires a VM and
containers do not.
I cannot envision this association right now, but it might be possible after
some research to understand the existent possibilities. For example, Intel
SGX or clear containers may help with this integration. However, these
technologies have trade-offs which could restrict adoption.

--
Guilherme

>
> > What is the reason to adding a measurement to multiple namespace
> > measurement lists? How are these lists going to be used? For Remote
> > Attestation we need a single list (the native one) which has the
> > complete list of measurements and in the same order they were
> > extended in the TPM. Additionally, when namespaces are released,
> > would the measurement list under that namespace disappear? How to
> > store this list considering the namespaces may have a short life and
> > be reused thousands of times?
>
> Different scenarios have different requirements. ?You're assuming that
> only the system owner is interested in the measurement list, not the
> container owner.
>
> The current builtin measurement policies measure everything executed
> on the system and anything accessed by real root. ?The namespace
> policy would probably be similar, but instead of measuring files
> accessed by real root, it would be files accessed by root in the
> namespace.
>
> > Should the native measurement list have all measurements triggered
> > in the whole system, including the ones made under other namespaces?
> > Following the algorithm below, if the file is not in the policy of
> > the 'native'/initial namespace, the measurement is not added to the
> > native measurement list.
>
> Correct. ?The policy controls what is included measured, appraised,
> and audited.
>
> > Each measurement entry in the list could have new fields to identify
> > the namespace. Since the namespaces can be reused, a timestamp or
> > others fields could be added to uniquely identify the namespace id.
>
> The more fields included in the measurement list, the more
> measurements will be added to the measurement list. ?Wouldn't it be
> enough to know that a certain file has been accessed/executed on the
> system and base any analytics/forensics on the IMA-audit data.
>
> > Regarding namespace hierarchy and IMA policy, we could assume that
> > if a given namespace has no policy set, the policy of that namespace
> > parent is applied and then the native/initial namespace should
> > always have a set policy.
>
> We shouldn't assume measure, appraise, or audit by default. ?Just like
> it is up to the native system to define a policy or if there is a
> policy, the "container" owner should define the policy, or if there is
> a policy.
>
> Mimi
>
> > >
> > > do {
> > > .
> > > .
> > >
> > > /* calculate file hash based on xattr algorithm */
> > > collect_measurement()
> > >
> > > /* recursively added to each namespace based on policy */
> > > ima_store_measurement()
> > >
> > > /* Based on the specific namespace policy and keys. */
> > > if (!once) {
> > > once = 1;
> > > result = ima_appraise_measurement()
> > > }
> > >
> > > ima_audit_measurement()
> > >
> > > } while ((ns = ns->parent));
> > >
> > > return result;
> > >
> > > Mimi

2017-07-27 17:49:47

[permalink] [raw]

Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

On 07/27/2017 01:18 PM, Magalhaes, Guilherme (Brazil R&D-CL) wrote:
>
>> -----Original Message-----
>> From: Mimi Zohar [mailto:[email protected]]
>> Sent: quinta-feira, 27 de julho de 2017 11:39
>> To: Magalhaes, Guilherme (Brazil R&D-CL)
>> <[email protected]>; Serge E. Hallyn <[email protected]>
>> Cc: Mehmet Kayaalp <[email protected]>; Yuqiong Sun
>> <[email protected]>; containers <[email protected]
>> foundation.org>; linux-kernel <[email protected]>; David Safford
>> <[email protected]>; James Bottomley
>> <[email protected]>; linux-security-module <linux-
>> [email protected]>; ima-devel <linux-ima-
>> [email protected]>; Yuqiong Sun <[email protected]>
>> Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA
>> namespace support
>>
>> On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D-
>> CL) wrote:
>>>
>>>> On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
>>>>> On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
>>>>>> On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
>>>>>>> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
>>>>>>>> On 07/25/2017 03:48 PM, Mimi Zohar wrote:
>>>>>>>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
>>>>>>>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
>>>>>>>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley
>> wrote:
>>>>>>>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
>>>>>>>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp
>>>> wrote:
>>>>>>>>>>>>>> From: Yuqiong Sun <[email protected]>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Yuqiong Sun <[email protected]>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changelog:
>>>>>>>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA
>> flag
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> So this means that every mount namespace clone will clone
>> a
>>>>>>>>>>>>> new IMA namespace. Is that really ok?
>>>>>>>>>>>> Based on what: space concerns (struct ima_ns is reasonably
>>>> small)?
>>>>>>>>>>>> or whether tying it to the mount namespace is the correct
>>>>>>>>>>>> thing to do. On
>>>>>>>>>>> Mostly the latter. The other would be not so much space
>>>>>>>>>>> concerns as time concerns. Many things use new mounts
>>>>>>>>>>> namespaces, and we wouldn't want multiple IMA calls on all
>>>>>>>>>>> file accesses by all of those.
>>>>>>>>>>>
>>>>>>>>>>>> the latter, it does seem that this should be a property of
>>>>>>>>>>>> either the mount or user ns rather than its own separate ns.
>>>>>>>>>>>> I could see a use where even a container might want multiple
>>>>>>>>>>>> ima keyrings within the container (say containerised apache
>>>>>>>>>>>> service with multiple tenants), so instinct tells me that
>>>>>>>>>>>> mount ns is the correct granularity for this.
>>>>>>>>>>> I wonder whether we could use echo 1 >
>>>>>>>>>>> /sys/kernel/security/ima/newns as the trigger for requesting
>>>>>>>>>>> a new ima ns on the next clone(CLONE_NEWNS).
>>>>>>>>>> I could go with that, but what about the trigger being
>>>>>>>>>> installing or updating the keyring? That's the only operation
>>>>>>>>>> that needs namespace separation, so on mount ns clone, you
>> get
>>>>>>>>>> a pointer to the old ima_ns until you do something that
>>>>>>>>>> requires a new key, which then triggers the copy of the
>> namespace
>>>> and installing it?
>>>>>>>>> It isn't just the keyrings that need to be namespaced, but the
>>>>>>>>> measurement list and policy as well.
>>>>>>>>>
>>>>>>>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy
>>>> based.
>>>>>>>>> As soon as the namespace starts, measurements should be
>> added
>>>>>>>>> to the namespace specific measurement list, not it's parent.
>>>>>>> Shouldn't it be both?
>>>>>> The policy defines which files are measured. The namespace policy
>>>>>> could be different than it's parent's policy, and the parent's
>>>>>> policy could be different than the native policy. Basically, file
>>>>>> measurements need to be added to the namespace measurement
>> list,
>>>>>> recursively, up to the native measurement list.
>>>>> Yes, but if a task t1 is in namespace ns2 which is a child of
>>>>> namespace ns1, and it accesses a file which ns1's policy says must be
>>>>> measured, then will ns1's required measurement happen (and be
>>>> appended
>>>>> to the ns1 measurement list), whether or not ns2's policy requires it?
>>>> Yes, as the file needs to be measured only in the ns1 policy, the
>>>> measurement would exist in the ns1 measurement list, but not in the
>>>> ns2 measurement list. The pseudo code snippet below might help.
>>> This algorithm is potentially extending a PCR in TPM multiple times
>>> for a single file event under a given namespace and duplicating
>>> entries. Any concerns with performance and memory footprint?
>> Going forward we assume associated with each container will be a vTPM.
>> The namespace measurements will extend a vTPM. As the container comes
>> and goes the associated measurement list, keyring, and vTPM will come
>> and go as well. For this reason, based on policy, the same file
>> measurement might appear in multiple measurement lists.
> My concern is that the base of namespacing the measurement lists is on the
> integration of containers with vTPM. Associating vTPM with containers as
> they are today is not a simple integration since vTPM requires a VM and
> containers do not.

There's a vTPM proxy driver in the kernel that enables spawning a
frontend /dev/tpm%d and an anonymous backend file descriptor where a
vTPM can listen on for TPM commands. I integrated this with 'swtpm' and
I have been working on integrating this into runc. Currently each
container started with runc can get one (or multiple) vTPMs and
/dev/tpm0 [and /dev/tpmrm0 in case of TPM2] then appear inside the
container.

What is missing for integration with IMA namespacing are patches for:

1) IMA to use a tpm_chip to talk to rather than issue TPM commands with
TPM_ANY_NUM as parameter to TPM driver functions
2) an ioctl for the vtpm proxy driver to attach a vtpm proxy instance's
tpm_chip to an IMA namespace; this ioctl should be called after the
creation of the IMA namespace (by container mgmt. stack [runc])
3) - some way for the IMA namespace to release the vTPM proxy's
'tpm_chip' and other resources once the IMA namespace is deleted

I have these patches in some form and would post them at a later stage
of namespacing IMA.

swtpm: https://github.com/stefanberger/swtpm
libtpms: https://github.com/stefanberger/libtpms
runc pull request: https://github.com/opencontainers/runc/pull/1082

Stefan

2017-07-27 19:40:36

[permalink] [raw]

Subject: RE: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

> -----Original Message-----
> From: Stefan Berger [mailto:[email protected]]
> Sent: quinta-feira, 27 de julho de 2017 14:50
> To: Magalhaes, Guilherme (Brazil R&D-CL)
> <[email protected]>; Mimi Zohar
> <[email protected]>; Serge E. Hallyn <[email protected]>
> Cc: Mehmet Kayaalp <[email protected]>; Yuqiong Sun
> <[email protected]>; containers <[email protected]
> foundation.org>; linux-kernel <[email protected]>; David Safford
> <[email protected]>; James Bottomley
> <[email protected]>; linux-security-module <linux-
> [email protected]>; ima-devel <linux-ima-
> [email protected]>; Yuqiong Sun <[email protected]>
> Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA
> namespace support
>
> On 07/27/2017 01:18 PM, Magalhaes, Guilherme (Brazil R&D-CL) wrote:
> >
> >> -----Original Message-----
> >> From: Mimi Zohar [mailto:[email protected]]
> >> Sent: quinta-feira, 27 de julho de 2017 11:39
> >> To: Magalhaes, Guilherme (Brazil R&D-CL)
> >> <[email protected]>; Serge E. Hallyn <[email protected]>
> >> Cc: Mehmet Kayaalp <[email protected]>; Yuqiong Sun
> >> <[email protected]>; containers <[email protected]
> >> foundation.org>; linux-kernel <[email protected]>; David
> Safford
> >> <[email protected]>; James Bottomley
> >> <[email protected]>; linux-security-module
> <linux-
> >> [email protected]>; ima-devel <linux-ima-
> >> [email protected]>; Yuqiong Sun <[email protected]>
> >> Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with
> IMA
> >> namespace support
> >>
> >> On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D-
> >> CL) wrote:
> >>>
> >>>> On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
> >>>>> On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> >>>>>> On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> >>>>>>> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> >>>>>>>> On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> >>>>>>>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> >>>>>>>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> >>>>>>>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley
> >> wrote:
> >>>>>>>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> >>>>>>>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp
> >>>> wrote:
> >>>>>>>>>>>>>> From: Yuqiong Sun <[email protected]>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> 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).
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Signed-off-by: Yuqiong Sun <[email protected]>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Changelog:
> >>>>>>>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA
> >> flag
> >>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> So this means that every mount namespace clone will clone
> >> a
> >>>>>>>>>>>>> new IMA namespace. Is that really ok?
> >>>>>>>>>>>> Based on what: space concerns (struct ima_ns is reasonably
> >>>> small)?
> >>>>>>>>>>>> or whether tying it to the mount namespace is the correct
> >>>>>>>>>>>> thing to do. On
> >>>>>>>>>>> Mostly the latter. The other would be not so much space
> >>>>>>>>>>> concerns as time concerns. Many things use new mounts
> >>>>>>>>>>> namespaces, and we wouldn't want multiple IMA calls on all
> >>>>>>>>>>> file accesses by all of those.
> >>>>>>>>>>>
> >>>>>>>>>>>> the latter, it does seem that this should be a property of
> >>>>>>>>>>>> either the mount or user ns rather than its own separate ns.
> >>>>>>>>>>>> I could see a use where even a container might want multiple
> >>>>>>>>>>>> ima keyrings within the container (say containerised apache
> >>>>>>>>>>>> service with multiple tenants), so instinct tells me that
> >>>>>>>>>>>> mount ns is the correct granularity for this.
> >>>>>>>>>>> I wonder whether we could use echo 1 >
> >>>>>>>>>>> /sys/kernel/security/ima/newns as the trigger for requesting
> >>>>>>>>>>> a new ima ns on the next clone(CLONE_NEWNS).
> >>>>>>>>>> I could go with that, but what about the trigger being
> >>>>>>>>>> installing or updating the keyring? That's the only operation
> >>>>>>>>>> that needs namespace separation, so on mount ns clone, you
> >> get
> >>>>>>>>>> a pointer to the old ima_ns until you do something that
> >>>>>>>>>> requires a new key, which then triggers the copy of the
> >> namespace
> >>>> and installing it?
> >>>>>>>>> It isn't just the keyrings that need to be namespaced, but the
> >>>>>>>>> measurement list and policy as well.
> >>>>>>>>>
> >>>>>>>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy
> >>>> based.
> >>>>>>>>> As soon as the namespace starts, measurements should be
> >> added
> >>>>>>>>> to the namespace specific measurement list, not it's parent.
> >>>>>>> Shouldn't it be both?
> >>>>>> The policy defines which files are measured. The namespace policy
> >>>>>> could be different than it's parent's policy, and the parent's
> >>>>>> policy could be different than the native policy. Basically, file
> >>>>>> measurements need to be added to the namespace measurement
> >> list,
> >>>>>> recursively, up to the native measurement list.
> >>>>> Yes, but if a task t1 is in namespace ns2 which is a child of
> >>>>> namespace ns1, and it accesses a file which ns1's policy says must be
> >>>>> measured, then will ns1's required measurement happen (and be
> >>>> appended
> >>>>> to the ns1 measurement list), whether or not ns2's policy requires it?
> >>>> Yes, as the file needs to be measured only in the ns1 policy, the
> >>>> measurement would exist in the ns1 measurement list, but not in the
> >>>> ns2 measurement list. The pseudo code snippet below might help.
> >>> This algorithm is potentially extending a PCR in TPM multiple times
> >>> for a single file event under a given namespace and duplicating
> >>> entries. Any concerns with performance and memory footprint?
> >> Going forward we assume associated with each container will be a vTPM.
> >> The namespace measurements will extend a vTPM. As the container
> comes
> >> and goes the associated measurement list, keyring, and vTPM will come
> >> and go as well. For this reason, based on policy, the same file
> >> measurement might appear in multiple measurement lists.
> > My concern is that the base of namespacing the measurement lists is on
> the
> > integration of containers with vTPM. Associating vTPM with containers as
> > they are today is not a simple integration since vTPM requires a VM and
> > containers do not.
>
> There's a vTPM proxy driver in the kernel that enables spawning a
> frontend /dev/tpm%d and an anonymous backend file descriptor where a
> vTPM can listen on for TPM commands. I integrated this with 'swtpm' and
> I have been working on integrating this into runc. Currently each
> container started with runc can get one (or multiple) vTPMs and
> /dev/tpm0 [and /dev/tpmrm0 in case of TPM2] then appear inside the
> container.
>

This is an interesting solution especially for nested namespaces with the
recursive application of measurements and a having list per container.

Following the TCG specs/requirements, what could we say about security
guarantees of real TPMs Vs this vTPM implementation?

--
Guilherme

> What is missing for integration with IMA namespacing are patches for:
>
> 1) IMA to use a tpm_chip to talk to rather than issue TPM commands with
> TPM_ANY_NUM as parameter to TPM driver functions
> 2) an ioctl for the vtpm proxy driver to attach a vtpm proxy instance's
> tpm_chip to an IMA namespace; this ioctl should be called after the
> creation of the IMA namespace (by container mgmt. stack [runc])
> 3) - some way for the IMA namespace to release the vTPM proxy's
> 'tpm_chip' and other resources once the IMA namespace is deleted
>
> I have these patches in some form and would post them at a later stage
> of namespacing IMA.
>
> swtpm: https://github.com/stefanberger/swtpm
> libtpms: https://github.com/stefanberger/libtpms
> runc pull request: https://github.com/opencontainers/runc/pull/1082
>
> Stefan

2017-07-27 20:52:07

[permalink] [raw]

Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

On 07/27/2017 03:39 PM, Magalhaes, Guilherme (Brazil R&D-CL) wrote:
>
>> There's a vTPM proxy driver in the kernel that enables spawning a
>> frontend /dev/tpm%d and an anonymous backend file descriptor where a
>> vTPM can listen on for TPM commands. I integrated this with 'swtpm' and
>> I have been working on integrating this into runc. Currently each
>> container started with runc can get one (or multiple) vTPMs and
>> /dev/tpm0 [and /dev/tpmrm0 in case of TPM2] then appear inside the
>> container.
>>
> This is an interesting solution especially for nested namespaces with the
> recursive application of measurements and a having list per container.
>
> Following the TCG specs/requirements, what could we say about security
> guarantees of real TPMs Vs this vTPM implementation?

A non-root user may not be able to do access the (permanent) state of
the vTPM state files since the container management stack would restrict
access to the files using DAC. Access to runtime data is also prevented
since the vTPM would not run under the account of the non-root user.

To protect the vTPM's permanent state file from access by a root user it
comes down to preventing the root user from getting a hold of the key
used for encrypting that file. Encrypting the state of the vTPM is
probably the best we can do to approximate a temper-resistant chip, but
preventing the root user from accessing the key may be more challenging.
Preventing root from accessing runtime data could be achieved by using
XGS or a similar technology.

Stefan

2017-07-28 14:20:21

[permalink] [raw]

Subject: RE: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

> > Each measurement entry in the list could have new fields to identify
> > the namespace. Since the namespaces can be reused, a timestamp or
> > others fields could be added to uniquely identify the namespace id.
>
> The more fields included in the measurement list, the more
> measurements will be added to the measurement list. Wouldn't it be
> enough to know that a certain file has been accessed/executed on the
> system and base any analytics/forensics on the IMA-audit data.

With the recursive application of policy through the namespace hierarchy,
a measurement added to the parent namespace could be misleading since
the file pathname makes sense in the current namespace but possibly not
for the parent namespace. This is the reason why I believe some new field
might be needed in the IMA template format to indicate or uniquely
identify the namespace.

--
Guilherme

2017-07-31 11:32:13

[permalink] [raw]

Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA namespace support

On Fri, 2017-07-28 at 14:19 +0000, Magalhaes, Guilherme (Brazil R&D-
CL) wrote:
> > > Each measurement entry in the list could have new fields to identify
> > > the namespace. Since the namespaces can be reused, a timestamp or
> > > others fields could be added to uniquely identify the namespace id.
> >
> > The more fields included in the measurement list, the more
> > measurements will be added to the measurement list. Wouldn't it be
> > enough to know that a certain file has been accessed/executed on the
> > system and base any analytics/forensics on the IMA-audit data.
>
> With the recursive application of policy through the namespace hierarchy,
> a measurement added to the parent namespace could be misleading since
> the file pathname makes sense in the current namespace but possibly not
> for the parent namespace.

Fair enough.

> This is the reason why I believe some new field
> might be needed in the IMA template format to indicate or uniquely
> identify the namespace.

I would probably include information to uniquely identify the file
(eg. UUID, mountpoint), not the namespace.

Mimi

2017-08-01 17:17:06

by Tycho Andersen

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] ima: mamespace audit status flags

Hi Mehmet,

On Thu, Jul 20, 2017 at 06:50:31PM -0400, Mehmet Kayaalp wrote:
> --- a/security/integrity/ima/ima_ns.c
> +++ b/security/integrity/ima/ima_ns.c
> @@ -301,3 +301,24 @@ 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
> +

Seems like these are defined in ima.h above in the patch, and
re-defined here?

> +unsigned long iint_flags(struct integrity_iint_cache *iint,
> + struct ns_status *status)
> +{
> + if (!status)
> + return iint->flags;
> +
> + return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS);

Just to confirm, is there any situation where:

iint->flags & IMA_NS_STATUS_FLAGS != status->flags & IMA_NS_STATUS_FLAGS

? i.e. can this line just be:

return status->flags & IMA_NS_STATUS_FLAGS;

Tycho

> +}
> +
> +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.9.4
>

2017-08-01 17:21:32

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] ima: mamespace audit status flags

> On Aug 1, 2017, at 1:17 PM, Tycho Andersen <[email protected]> wrote:
>
> Hi Mehmet,
>
> On Thu, Jul 20, 2017 at 06:50:31PM -0400, Mehmet Kayaalp wrote:
>> --- a/security/integrity/ima/ima_ns.c
>> +++ b/security/integrity/ima/ima_ns.c
>> @@ -301,3 +301,24 @@ 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
>> +
>
> Seems like these are defined in ima.h above in the patch, and
> re-defined here?

Yes, it should be in the ima.h only.

>> +unsigned long iint_flags(struct integrity_iint_cache *iint,
>> + struct ns_status *status)
>> +{
>> + if (!status)
>> + return iint->flags;
>> +
>> + return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS);
>
> Just to confirm, is there any situation where:
>
> iint->flags & IMA_NS_STATUS_FLAGS != status->flags & IMA_NS_STATUS_FLAGS
>
> ? i.e. can this line just be:
>
> return status->flags & IMA_NS_STATUS_FLAGS;
>

As Guilherme had pointed out, the first & should be |.

Mehmet

2017-08-02 21:48:45

by Tycho Andersen

[permalink] [raw]

Subject: Re: [RFC PATCH 3/5] ima: mamespace audit status flags

On Tue, Aug 01, 2017 at 01:25:31PM -0400, Mehmet Kayaalp wrote:
> >> +unsigned long iint_flags(struct integrity_iint_cache *iint,
> >> + struct ns_status *status)
> >> +{
> >> + if (!status)
> >> + return iint->flags;
> >> +
> >> + return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS);
> >
> > Just to confirm, is there any situation where:
> >
> > iint->flags & IMA_NS_STATUS_FLAGS != status->flags & IMA_NS_STATUS_FLAGS
> >
> > ? i.e. can this line just be:
> >
> > return status->flags & IMA_NS_STATUS_FLAGS;
> >
>
> As Guilherme had pointed out, the first & should be |.

Sorry, that mail got filtered somehow, thanks. Per your discussion, I
guess the most defensive way is:

iint->flags & ~IMA_NS_STATUS_FLAGS | status->flags & IMA_NS_STATUS_FLAGS

in case something comes along and sets IMA_AUDITED on the root iint,
we don't want it to propagate to this ns' status unnecessarily.

Anyway, thanks!

Tycho

2017-08-11 15:01:05

[permalink] [raw]

Subject: Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

On 07/20/2017 06:50 PM, Mehmet Kayaalp wrote:
> 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. A lazy clean up of the rbtree that removes free'd inodes could be
> implemented to reclaim the invalid entries.
>
> Signed-off-by: Mehmet Kayaalp <[email protected]>
> ---
> include/linux/ima.h | 3 +
> security/integrity/ima/ima.h | 16 ++++++
> security/integrity/ima/ima_ns.c | 120 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 139 insertions(+)
>
>
> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> .parent = NULL,
> };
> EXPORT_SYMBOL(init_ima_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->i_ino > status->i_ino)
> + n = n->rb_right;

Above you are comparing with the inode ptr, here with i_ino. Why can you
not compare with inode both times. Also the insertion only seems to
consider the inode ptr.

Stefan

2018-03-08 13:41:02

[permalink] [raw]

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

On 07/20/2017 06:50 PM, Mehmet Kayaalp wrote:
> From: Yuqiong Sun <[email protected]>
>
> 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).
>
> Signed-off-by: Yuqiong Sun <[email protected]>
>
> 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()

With this patch we would use CLONE_NEWNS and create an IMA and mount
namespace at the same time. However, the code below creates two inodes
to handle the two namespaces separately via setns(). The consequence of
this is that an application doing a setns() on the created mount
namespace will not join the IMA namespace that was created as part of
the clone(CLONE_NEWNS). Is that expected behavior? If not, we'll have to
modify the code below and remove its proc_ns_operations and modify the
mount namespace's proc_ns_operations to call into IMA code as well. I
don't know of another namespace that would do something like this. Is
that an acceptable solution?

Stefan

>
> Signed-off-by: Mehmet Kayaalp <[email protected]>
> ---
> fs/proc/namespaces.c | 3 +
> include/linux/ima.h | 37 ++++++++
> include/linux/nsproxy.h | 1 +
> include/linux/proc_ns.h | 2 +
> init/Kconfig | 8 ++
> kernel/nsproxy.c | 15 ++++
> security/integrity/ima/Makefile | 1 +
> security/integrity/ima/ima.h | 9 ++
> security/integrity/ima/ima_init.c | 4 +
> security/integrity/ima/ima_ns.c | 183 ++++++++++++++++++++++++++++++++++++++
> 10 files changed, 263 insertions(+)
> create mode 100644 security/integrity/ima/ima_ns.c
>
> diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
> index 3803b24..222a64e 100644
> --- a/fs/proc/namespaces.c
> +++ b/fs/proc/namespaces.c
> @@ -32,6 +32,9 @@ static const struct proc_ns_operations *ns_entries[] = {
> #ifdef CONFIG_CGROUPS
> &cgroupns_operations,
> #endif
> +#ifdef CONFIG_IMA_NS
> + &imans_operations,
> +#endif
> };
>
> static const char *proc_ns_get_link(struct dentry *dentry,
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 0e4647e..11e4841 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -105,4 +105,41 @@ 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 ns_common ns;
> + struct ima_namespace *parent;
> +};
> +
> +extern struct ima_namespace init_ima_ns;
> +
> +#ifdef CONFIG_IMA_NS
> +void put_ima_ns(struct ima_namespace *ns);
> +struct ima_namespace *copy_ima(unsigned long flags,
> + struct user_namespace *user_ns,
> + struct ima_namespace *old_ns);
> +static inline struct ima_namespace *get_current_ns(void)
> +{
> + return current->nsproxy->ima_ns;
> +}
> +#else
> +static inline void put_ima_ns(struct ima_namespace *ns)
> +{
> + return;
> +}
> +
> +static inline struct ima_namespace *copy_ima(unsigned long flags,
> + 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/nsproxy.h b/include/linux/nsproxy.h
> index ac0d65b..a97031d 100644
> --- a/include/linux/nsproxy.h
> +++ b/include/linux/nsproxy.h
> @@ -35,6 +35,7 @@ struct nsproxy {
> struct pid_namespace *pid_ns_for_children;
> struct net *net_ns;
> struct cgroup_namespace *cgroup_ns;
> + struct ima_namespace *ima_ns;
> };
> extern struct nsproxy init_nsproxy;
>
> diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
> index 58ab28d..c7c1239 100644
> --- a/include/linux/proc_ns.h
> +++ b/include/linux/proc_ns.h
> @@ -31,6 +31,7 @@ extern const struct proc_ns_operations pidns_for_children_operations;
> extern const struct proc_ns_operations userns_operations;
> extern const struct proc_ns_operations mntns_operations;
> extern const struct proc_ns_operations cgroupns_operations;
> +extern const struct proc_ns_operations imans_operations;
>
> /*
> * We always define these enumerators
> @@ -42,6 +43,7 @@ enum {
> PROC_USER_INIT_INO = 0xEFFFFFFDU,
> PROC_PID_INIT_INO = 0xEFFFFFFCU,
> PROC_CGROUP_INIT_INO = 0xEFFFFFFBU,
> + PROC_IMA_INIT_INO = 0xEFFFFFFAU,
> };
>
> #ifdef CONFIG_PROC_FS
> diff --git a/init/Kconfig b/init/Kconfig
> index 1d3475f..339f84d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1287,6 +1287,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 f6c5d33..85be341 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;
>
> @@ -44,6 +45,9 @@ struct nsproxy init_nsproxy = {
> #ifdef CONFIG_CGROUPS
> .cgroup_ns = &init_cgroup_ns,
> #endif
> +#ifdef CONFIG_IMA_NS
> + .ima_ns = &init_ima_ns,
> +#endif
> };
>
> static inline struct nsproxy *create_nsproxy(void)
> @@ -110,8 +114,17 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
> goto out_net;
> }
>
> + new_nsp->ima_ns = copy_ima(flags, user_ns, tsk->nsproxy->ima_ns);
> + if (IS_ERR(new_nsp->ima_ns)) {
> + err = PTR_ERR(new_nsp->ima_ns);
> + goto out_ima;
> + }
> +
> return new_nsp;
>
> +out_ima:
> + if (new_nsp->ima_ns)
> + put_ima_ns(new_nsp->ima_ns);
> out_net:
> put_cgroup_ns(new_nsp->cgroup_ns);
> out_cgroup:
> @@ -181,6 +194,8 @@ void free_nsproxy(struct nsproxy *ns)
> if (ns->pid_ns_for_children)
> put_pid_ns(ns->pid_ns_for_children);
> put_cgroup_ns(ns->cgroup_ns);
> + if (ns->ima_ns)
> + put_ima_ns(ns->ima_ns);
> put_net(ns->net_ns);
> kmem_cache_free(nsproxy_cachep, ns);
> }
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index 29f198b..20493f0 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -8,5 +8,6 @@ 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-$(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 d52b487..8a8234a 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -291,6 +291,15 @@ static inline int ima_read_xattr(struct dentry *dentry,
>
> #endif /* CONFIG_IMA_APPRAISE */
>
> +#ifdef CONFIG_IMA_NS
> +int ima_ns_init(void);
> +#else
> +static inline int ima_ns_init(void)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_IMA_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 2967d49..7f884a7 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_ns.c b/security/integrity/ima/ima_ns.c
> new file mode 100644
> index 0000000..383217b
> --- /dev/null
> +++ b/security/integrity/ima/ima_ns.c
> @@ -0,0 +1,183 @@
> +/*
> + * Copyright (C) 2008 IBM Corporation
> + * Author: Yuqiong Sun <[email protected]>
> + *
> + * 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/proc_ns.h>
> +#include <linux/kref.h>
> +#include <linux/slab.h>
> +#include <linux/ima.h>
> +
> +#include "ima.h"
> +
> +static void get_ima_ns(struct ima_namespace *ns);
> +
> +int ima_init_namespace(struct ima_namespace *ns)
> +{
> + return 0;
> +}
> +
> +int ima_ns_init(void)
> +{
> + return ima_init_namespace(&init_ima_ns);
> +}
> +
> +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;
> + int err;
> +
> + ns = create_ima_ns();
> + if (!ns)
> + return ERR_PTR(-ENOMEM);
> +
> + err = ns_alloc_inum(&ns->ns);
> + if (err) {
> + kfree(ns);
> + return ERR_PTR(err);
> + }
> +
> + ns->ns.ops = &imans_operations;
> + 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(unsigned long flags,
> + struct user_namespace *user_ns,
> + struct ima_namespace *old_ns)
> +{
> + struct ima_namespace *new_ns;
> +
> + BUG_ON(!old_ns);
> + get_ima_ns(old_ns);
> +
> + if (!(flags & CLONE_NEWNS))
> + return 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);
> + ns_free_inum(&ns->ns);
> + kfree(ns);
> +}
> +
> +static void free_ima_ns(struct kref *kref)
> +{
> + struct ima_namespace *ns;
> +
> + ns = container_of(kref, struct ima_namespace, kref);
> + destroy_ima_ns(ns);
> +}
> +
> +static void get_ima_ns(struct ima_namespace *ns)
> +{
> + kref_get(&ns->kref);
> +}
> +
> +void put_ima_ns(struct ima_namespace *ns)
> +{
> + kref_put(&ns->kref, free_ima_ns);
> +}
> +
> +static inline struct ima_namespace *to_ima_ns(struct ns_common *ns)
> +{
> + return container_of(ns, struct ima_namespace, ns);
> +}
> +
> +static struct ns_common *imans_get(struct task_struct *task)
> +{
> + struct ima_namespace *ns = NULL;
> + struct nsproxy *nsproxy;
> +
> + task_lock(task);
> + nsproxy = task->nsproxy;
> + if (nsproxy) {
> + ns = nsproxy->ima_ns;
> + get_ima_ns(ns);
> + }
> + task_unlock(task);
> +
> + return ns ? &ns->ns : NULL;
> +}
> +
> +static void imans_put(struct ns_common *ns)
> +{
> + put_ima_ns(to_ima_ns(ns));
> +}
> +
> +static int imans_install(struct nsproxy *nsproxy, struct ns_common *new)
> +{
> + struct ima_namespace *ns = to_ima_ns(new);
> +
> + if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN) ||
> + !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + get_ima_ns(ns);
> + put_ima_ns(nsproxy->ima_ns);
> + nsproxy->ima_ns = ns;
> + return 0;
> +}
> +
> +const struct proc_ns_operations imans_operations = {
> + .name = "ima",
> + .type = CLONE_NEWNS,
> + .get = imans_get,
> + .put = imans_put,
> + .install = imans_install,
> +};
> +
> +struct ima_namespace init_ima_ns = {
> + .kref = KREF_INIT(2),
> + .user_ns = &init_user_ns,
> + .ns.inum = PROC_IMA_INIT_INO,
> +#ifdef CONFIG_IMA_NS
> + .ns.ops = &imans_operations,
> +#endif
> + .parent = NULL,
> +};
> +EXPORT_SYMBOL(init_ima_ns);

2018-03-08 14:06:39

[permalink] [raw]

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

On 07/25/2017 04:46 PM, Serge E. Hallyn wrote:
> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
>> On 07/25/2017 03:48 PM, Mimi Zohar wrote:
>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
>>>>>>>> From: Yuqiong Sun <[email protected]>
>>>>>>>>
>>>>>>>> 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).
>>>>>>>>
>>>>>>>> Signed-off-by: Yuqiong Sun <[email protected]>
>>>>>>>>
>>>>>>>> Changelog:
>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>>>>>>> Hi,
>>>>>>>
>>>>>>> So this means that every mount namespace clone will clone a new
>>>>>>> IMA namespace. Is that really ok?
>>>>>> Based on what: space concerns (struct ima_ns is reasonably small)?
>>>>>> or whether tying it to the mount namespace is the correct thing to
>>>>>> do. On
>>>>> Mostly the latter. The other would be not so much space concerns as
>>>>> time concerns. Many things use new mounts namespaces, and we
>>>>> wouldn't want multiple IMA calls on all file accesses by all of
>>>>> those.
>>>>>
>>>>>> the latter, it does seem that this should be a property of either
>>>>>> the mount or user ns rather than its own separate ns. I could see
>>>>>> a use where even a container might want multiple ima keyrings
>>>>>> within the container (say containerised apache service with
>>>>>> multiple tenants), so instinct tells me that mount ns is the
>>>>>> correct granularity for this.
>>>>> I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
>>>>> as the trigger for requesting a new ima ns on the next
>>>>> clone(CLONE_NEWNS).
>>>> I could go with that, but what about the trigger being installing or
>>>> updating the keyring? That's the only operation that needs namespace
>>>> separation, so on mount ns clone, you get a pointer to the old ima_ns
>>>> until you do something that requires a new key, which then triggers the
>>>> copy of the namespace and installing it?
>>> It isn't just the keyrings that need to be namespaced, but the
>>> measurement list and policy as well.
>>>
>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
>>>
>>> As soon as the namespace starts, measurements should be added to the
>>> namespace specific measurement list, not it's parent.
> Shouldn't it be both?
>
> If not, then it seems to me this must be tied to user namespace.
>
>> IMA is about measuring things, logging what was executed, and
>> finally someone looking at the measurement log and detecting
>> 'things'. So at least one attack that needs to be prevented is a
>> malicious person opening an IMA namespace, executing something
>> malicious, and not leaving any trace on the host because all the
>> logs went into the measurement list of the IMA namespace, which
>> disappeared. That said, I am wondering whether there has to be a
>> minimum set of namespaces (PID, UTS) providing enough 'isolation'
>> that someone may actually open an IMA namespace and run their code.
>> To avoid leaving no traces one could argue to implement recursive
>> logging, so something that is logged inside the namespace will be
>> detected in all parent containers up to the init_ima_ns (host)
>> because it's logged (and TPM extended) there as well. The challenge
>> with that is that logging costs memory and that can be abused as
>> well until the machine needs a reboot... I guess the solution could
>> be requesting an IMA namespace in one way or another but requiring
>> several other namespace flags in the clone() to actually 'get' it.
>> Jumping namespaces with setns() may have to be restricted as well
>> once there is an IMA namespace.
> Wait. So if I create a new IMA namespace, the things I run in
> that namespace are not subject to the parent namespace policy?

We'll let an IMA namespace set its own policy and rules in that policy
will decide whether the child namespaces' measurements will also be
logged. This is to avoid a potentially huge log on the host. However,
the activities of root in namespaces may need to be logged independently
of what the policy rules say so that root's activities in the TCB will
always be tracked also if he operates in a temporary mount/IMA namespace
pair (and sharing the rest of the namespaces with the host).

Stefan

>
> -serge
>

2018-03-08 20:20:41

[permalink] [raw]

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

Quoting Stefan Berger ([email protected]):
> On 07/20/2017 06:50 PM, Mehmet Kayaalp wrote:
> >From: Yuqiong Sun <[email protected]>
> >
> >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).
> >
> >Signed-off-by: Yuqiong Sun <[email protected]>
> >
> >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()
>
> With this patch we would use CLONE_NEWNS and create an IMA and mount
> namespace at the same time. However, the code below creates two
> inodes to handle the two namespaces separately via setns(). The

... right.

Either the ima and mounts namespaces are so closely tied that ima_ns
should be unshared on every CLONE_NEWNS, or not. If they are, then
every setns(CLONE_NEWNS) must also change the ima_ns. That is not the
case here. Every clone creates a new ima_ns, but we're not forcing
tasks to be in the ima_ns that is matched with its mntns, and
furthermore we have another object lifecycle to worry about.

It still seems to me that the only sane way to do this is to have the
ima_ns be its own object; have it be owned by a user_ns; require
CAP_SYS_ADMIN (or better CAP_MAC_ADMIN) to your current userns to
clone a new one, maybe with no other tasks in userns yet, for good
measure. And support hierarchical measuring (so parents can still
get information about a child's actions).

If IMA is to be at all trustworthy for remote appraisal, then I do
not see how you can let a privileged insecure container completely
bypass IMA. The key difference between allowing new ima_ns with
mntns or only with userns is that after unsharing my user_ns, my
privilege with respect to the parent is lost. A new mntns doesn't
change anything about how I can corrupt the parent.

> consequence of this is that an application doing a setns() on the
> created mount namespace will not join the IMA namespace that was
> created as part of the clone(CLONE_NEWNS). Is that expected
> behavior? If not, we'll have to modify the code below and remove its
> proc_ns_operations and modify the mount namespace's
> proc_ns_operations to call into IMA code as well. I don't know of
> another namespace that would do something like this. Is that an
> acceptable solution?
>
> Stefan
>
> >
> >Signed-off-by: Mehmet Kayaalp <[email protected]>
> >---
> > fs/proc/namespaces.c | 3 +
> > include/linux/ima.h | 37 ++++++++
> > include/linux/nsproxy.h | 1 +
> > include/linux/proc_ns.h | 2 +
> > init/Kconfig | 8 ++
> > kernel/nsproxy.c | 15 ++++
> > security/integrity/ima/Makefile | 1 +
> > security/integrity/ima/ima.h | 9 ++
> > security/integrity/ima/ima_init.c | 4 +
> > security/integrity/ima/ima_ns.c | 183 ++++++++++++++++++++++++++++++++++++++
> > 10 files changed, 263 insertions(+)
> > create mode 100644 security/integrity/ima/ima_ns.c
> >
> >diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
> >index 3803b24..222a64e 100644
> >--- a/fs/proc/namespaces.c
> >+++ b/fs/proc/namespaces.c
> >@@ -32,6 +32,9 @@ static const struct proc_ns_operations *ns_entries[] = {
> > #ifdef CONFIG_CGROUPS
> > &cgroupns_operations,
> > #endif
> >+#ifdef CONFIG_IMA_NS
> >+ &imans_operations,
> >+#endif
> > };
> >
> > static const char *proc_ns_get_link(struct dentry *dentry,
> >diff --git a/include/linux/ima.h b/include/linux/ima.h
> >index 0e4647e..11e4841 100644
> >--- a/include/linux/ima.h
> >+++ b/include/linux/ima.h
> >@@ -105,4 +105,41 @@ 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 ns_common ns;
> >+ struct ima_namespace *parent;
> >+};
> >+
> >+extern struct ima_namespace init_ima_ns;
> >+
> >+#ifdef CONFIG_IMA_NS
> >+void put_ima_ns(struct ima_namespace *ns);
> >+struct ima_namespace *copy_ima(unsigned long flags,
> >+ struct user_namespace *user_ns,
> >+ struct ima_namespace *old_ns);
> >+static inline struct ima_namespace *get_current_ns(void)
> >+{
> >+ return current->nsproxy->ima_ns;
> >+}
> >+#else
> >+static inline void put_ima_ns(struct ima_namespace *ns)
> >+{
> >+ return;
> >+}
> >+
> >+static inline struct ima_namespace *copy_ima(unsigned long flags,
> >+ 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/nsproxy.h b/include/linux/nsproxy.h
> >index ac0d65b..a97031d 100644
> >--- a/include/linux/nsproxy.h
> >+++ b/include/linux/nsproxy.h
> >@@ -35,6 +35,7 @@ struct nsproxy {
> > struct pid_namespace *pid_ns_for_children;
> > struct net *net_ns;
> > struct cgroup_namespace *cgroup_ns;
> >+ struct ima_namespace *ima_ns;
> > };
> > extern struct nsproxy init_nsproxy;
> >
> >diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
> >index 58ab28d..c7c1239 100644
> >--- a/include/linux/proc_ns.h
> >+++ b/include/linux/proc_ns.h
> >@@ -31,6 +31,7 @@ extern const struct proc_ns_operations pidns_for_children_operations;
> > extern const struct proc_ns_operations userns_operations;
> > extern const struct proc_ns_operations mntns_operations;
> > extern const struct proc_ns_operations cgroupns_operations;
> >+extern const struct proc_ns_operations imans_operations;
> >
> > /*
> > * We always define these enumerators
> >@@ -42,6 +43,7 @@ enum {
> > PROC_USER_INIT_INO = 0xEFFFFFFDU,
> > PROC_PID_INIT_INO = 0xEFFFFFFCU,
> > PROC_CGROUP_INIT_INO = 0xEFFFFFFBU,
> >+ PROC_IMA_INIT_INO = 0xEFFFFFFAU,
> > };
> >
> > #ifdef CONFIG_PROC_FS
> >diff --git a/init/Kconfig b/init/Kconfig
> >index 1d3475f..339f84d 100644
> >--- a/init/Kconfig
> >+++ b/init/Kconfig
> >@@ -1287,6 +1287,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 f6c5d33..85be341 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;
> >
> >@@ -44,6 +45,9 @@ struct nsproxy init_nsproxy = {
> > #ifdef CONFIG_CGROUPS
> > .cgroup_ns = &init_cgroup_ns,
> > #endif
> >+#ifdef CONFIG_IMA_NS
> >+ .ima_ns = &init_ima_ns,
> >+#endif
> > };
> >
> > static inline struct nsproxy *create_nsproxy(void)
> >@@ -110,8 +114,17 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
> > goto out_net;
> > }
> >
> >+ new_nsp->ima_ns = copy_ima(flags, user_ns, tsk->nsproxy->ima_ns);
> >+ if (IS_ERR(new_nsp->ima_ns)) {
> >+ err = PTR_ERR(new_nsp->ima_ns);
> >+ goto out_ima;
> >+ }
> >+
> > return new_nsp;
> >
> >+out_ima:
> >+ if (new_nsp->ima_ns)
> >+ put_ima_ns(new_nsp->ima_ns);
> > out_net:
> > put_cgroup_ns(new_nsp->cgroup_ns);
> > out_cgroup:
> >@@ -181,6 +194,8 @@ void free_nsproxy(struct nsproxy *ns)
> > if (ns->pid_ns_for_children)
> > put_pid_ns(ns->pid_ns_for_children);
> > put_cgroup_ns(ns->cgroup_ns);
> >+ if (ns->ima_ns)
> >+ put_ima_ns(ns->ima_ns);
> > put_net(ns->net_ns);
> > kmem_cache_free(nsproxy_cachep, ns);
> > }
> >diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> >index 29f198b..20493f0 100644
> >--- a/security/integrity/ima/Makefile
> >+++ b/security/integrity/ima/Makefile
> >@@ -8,5 +8,6 @@ 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-$(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 d52b487..8a8234a 100644
> >--- a/security/integrity/ima/ima.h
> >+++ b/security/integrity/ima/ima.h
> >@@ -291,6 +291,15 @@ static inline int ima_read_xattr(struct dentry *dentry,
> >
> > #endif /* CONFIG_IMA_APPRAISE */
> >
> >+#ifdef CONFIG_IMA_NS
> >+int ima_ns_init(void);
> >+#else
> >+static inline int ima_ns_init(void)
> >+{
> >+ return 0;
> >+}
> >+#endif /* CONFIG_IMA_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 2967d49..7f884a7 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_ns.c b/security/integrity/ima/ima_ns.c
> >new file mode 100644
> >index 0000000..383217b
> >--- /dev/null
> >+++ b/security/integrity/ima/ima_ns.c
> >@@ -0,0 +1,183 @@
> >+/*
> >+ * Copyright (C) 2008 IBM Corporation
> >+ * Author: Yuqiong Sun <[email protected]>
> >+ *
> >+ * 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/proc_ns.h>
> >+#include <linux/kref.h>
> >+#include <linux/slab.h>
> >+#include <linux/ima.h>
> >+
> >+#include "ima.h"
> >+
> >+static void get_ima_ns(struct ima_namespace *ns);
> >+
> >+int ima_init_namespace(struct ima_namespace *ns)
> >+{
> >+ return 0;
> >+}
> >+
> >+int ima_ns_init(void)
> >+{
> >+ return ima_init_namespace(&init_ima_ns);
> >+}
> >+
> >+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;
> >+ int err;
> >+
> >+ ns = create_ima_ns();
> >+ if (!ns)
> >+ return ERR_PTR(-ENOMEM);
> >+
> >+ err = ns_alloc_inum(&ns->ns);
> >+ if (err) {
> >+ kfree(ns);
> >+ return ERR_PTR(err);
> >+ }
> >+
> >+ ns->ns.ops = &imans_operations;
> >+ 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(unsigned long flags,
> >+ struct user_namespace *user_ns,
> >+ struct ima_namespace *old_ns)
> >+{
> >+ struct ima_namespace *new_ns;
> >+
> >+ BUG_ON(!old_ns);
> >+ get_ima_ns(old_ns);
> >+
> >+ if (!(flags & CLONE_NEWNS))
> >+ return 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);
> >+ ns_free_inum(&ns->ns);
> >+ kfree(ns);
> >+}
> >+
> >+static void free_ima_ns(struct kref *kref)
> >+{
> >+ struct ima_namespace *ns;
> >+
> >+ ns = container_of(kref, struct ima_namespace, kref);
> >+ destroy_ima_ns(ns);
> >+}
> >+
> >+static void get_ima_ns(struct ima_namespace *ns)
> >+{
> >+ kref_get(&ns->kref);
> >+}
> >+
> >+void put_ima_ns(struct ima_namespace *ns)
> >+{
> >+ kref_put(&ns->kref, free_ima_ns);
> >+}
> >+
> >+static inline struct ima_namespace *to_ima_ns(struct ns_common *ns)
> >+{
> >+ return container_of(ns, struct ima_namespace, ns);
> >+}
> >+
> >+static struct ns_common *imans_get(struct task_struct *task)
> >+{
> >+ struct ima_namespace *ns = NULL;
> >+ struct nsproxy *nsproxy;
> >+
> >+ task_lock(task);
> >+ nsproxy = task->nsproxy;
> >+ if (nsproxy) {
> >+ ns = nsproxy->ima_ns;
> >+ get_ima_ns(ns);
> >+ }
> >+ task_unlock(task);
> >+
> >+ return ns ? &ns->ns : NULL;
> >+}
> >+
> >+static void imans_put(struct ns_common *ns)
> >+{
> >+ put_ima_ns(to_ima_ns(ns));
> >+}
> >+
> >+static int imans_install(struct nsproxy *nsproxy, struct ns_common *new)
> >+{
> >+ struct ima_namespace *ns = to_ima_ns(new);
> >+
> >+ if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN) ||
> >+ !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
> >+ return -EPERM;
> >+
> >+ get_ima_ns(ns);
> >+ put_ima_ns(nsproxy->ima_ns);
> >+ nsproxy->ima_ns = ns;
> >+ return 0;
> >+}
> >+
> >+const struct proc_ns_operations imans_operations = {
> >+ .name = "ima",
> >+ .type = CLONE_NEWNS,
> >+ .get = imans_get,
> >+ .put = imans_put,
> >+ .install = imans_install,
> >+};
> >+
> >+struct ima_namespace init_ima_ns = {
> >+ .kref = KREF_INIT(2),
> >+ .user_ns = &init_user_ns,
> >+ .ns.inum = PROC_IMA_INIT_INO,
> >+#ifdef CONFIG_IMA_NS
> >+ .ns.ops = &imans_operations,
> >+#endif
> >+ .parent = NULL,
> >+};
> >+EXPORT_SYMBOL(init_ima_ns);
>

2018-03-08 23:32:41

[permalink] [raw]

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

Quoting Stefan Berger ([email protected]):
> On 03/08/2018 03:19 PM, Serge E. Hallyn wrote:
> >Quoting Stefan Berger ([email protected]):
> >>On 07/20/2017 06:50 PM, Mehmet Kayaalp wrote:
> >>>From: Yuqiong Sun<[email protected]>
> >>>
> >>>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).
> >>>
> >>>Signed-off-by: Yuqiong Sun<[email protected]>
> >>>
> >>>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()
> >>With this patch we would use CLONE_NEWNS and create an IMA and mount
> >>namespace at the same time. However, the code below creates two
> >>inodes to handle the two namespaces separately via setns(). The
> >... right.
> >
> >Either the ima and mounts namespaces are so closely tied that ima_ns
> >should be unshared on every CLONE_NEWNS, or not. If they are, then
> >every setns(CLONE_NEWNS) must also change the ima_ns. That is not the
> >case here. Every clone creates a new ima_ns, but we're not forcing
> >tasks to be in the ima_ns that is matched with its mntns, and
> >furthermore we have another object lifecycle to worry about.
> >
> >It still seems to me that the only sane way to do this is to have the
> >ima_ns be its own object; have it be owned by a user_ns; require
> >CAP_SYS_ADMIN (or better CAP_MAC_ADMIN) to your current userns to
> >clone a new one, maybe with no other tasks in userns yet, for good
> >measure. And support hierarchical measuring (so parents can still
> >get information about a child's actions).
>
> I think there is a real benefit to keeping the IMA namespace with
> the mount namespace since the mount namespace carries the signatures
> in the xattrs and IMA the (appraisal) policy. The user namespace has

But xattrs have to do with the files and filesystem. Not with
mounts.

> the keys IMA needs for signature verification and if missing, the
> appraisal will fail (at least that is how it could work but Mimi
> tells me the pointer to the IMA keyring is cached). So there's an
> incentive to keep the otherwise 'loose' namespaces 'together.' If we
> were to associate the IMA namespace with the user namespace or be
> stand-alone, it is easier to just setns() the mount namespace and
> circumvent the IMA (appraisal) policy.

Sure but you won't have privilege over the previous namespace.
Now, you will over the uids you were delegated - almost seems like an
ima_ns should be assoicated with a segregated uid range.

> >If IMA is to be at all trustworthy for remote appraisal, then I do
>
> remote appraisal ? remote attestation ?

right attestation

> >not see how you can let a privileged insecure container completely
> >bypass IMA. The key difference between allowing new ima_ns with
> >mntns or only with userns is that after unsharing my user_ns, my
> >privilege with respect to the parent is lost. A new mntns doesn't
> >change anything about how I can corrupt the parent.
>
> Not quite following. After unsharing the user_ns IMA could be made
> to loose access to its keys from the previous user_ns and starting
> apps would fail appraisal then, unless the new user_ns IMA keyring
> has the same keys again.

It doesn't inherit the parent's to begin with? I guess I don't
know enough about how the keyring is managed.

2018-03-09 03:00:54

[permalink] [raw]

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

On Thu, Mar 08, 2018 at 09:04:52AM -0500, Stefan Berger wrote:
> On 07/25/2017 04:46 PM, Serge E. Hallyn wrote:
> >On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> >>On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> >>>On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> >>>>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> >>>>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> >>>>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> >>>>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> >>>>>>>>From: Yuqiong Sun <[email protected]>
> >>>>>>>>
> >>>>>>>>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).
> >>>>>>>>
> >>>>>>>>Signed-off-by: Yuqiong Sun <[email protected]>
> >>>>>>>>
> >>>>>>>>Changelog:
> >>>>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> >>>>>>>Hi,
> >>>>>>>
> >>>>>>>So this means that every mount namespace clone will clone a new
> >>>>>>>IMA namespace. Is that really ok?
> >>>>>>Based on what: space concerns (struct ima_ns is reasonably small)?
> >>>>>>or whether tying it to the mount namespace is the correct thing to
> >>>>>>do. On
> >>>>>Mostly the latter. The other would be not so much space concerns as
> >>>>>time concerns. Many things use new mounts namespaces, and we
> >>>>>wouldn't want multiple IMA calls on all file accesses by all of
> >>>>>those.
> >>>>>
> >>>>>>the latter, it does seem that this should be a property of either
> >>>>>>the mount or user ns rather than its own separate ns. I could see
> >>>>>>a use where even a container might want multiple ima keyrings
> >>>>>>within the container (say containerised apache service with
> >>>>>>multiple tenants), so instinct tells me that mount ns is the
> >>>>>>correct granularity for this.
> >>>>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> >>>>>as the trigger for requesting a new ima ns on the next
> >>>>>clone(CLONE_NEWNS).
> >>>>I could go with that, but what about the trigger being installing or
> >>>>updating the keyring? That's the only operation that needs namespace
> >>>>separation, so on mount ns clone, you get a pointer to the old ima_ns
> >>>>until you do something that requires a new key, which then triggers the
> >>>>copy of the namespace and installing it?
> >>>It isn't just the keyrings that need to be namespaced, but the
> >>>measurement list and policy as well.
> >>>
> >>>IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
> >>>
> >>>As soon as the namespace starts, measurements should be added to the
> >>>namespace specific measurement list, not it's parent.
> >Shouldn't it be both?
> >
> >If not, then it seems to me this must be tied to user namespace.
> >
> >>IMA is about measuring things, logging what was executed, and
> >>finally someone looking at the measurement log and detecting
> >>'things'. So at least one attack that needs to be prevented is a
> >>malicious person opening an IMA namespace, executing something
> >>malicious, and not leaving any trace on the host because all the
> >>logs went into the measurement list of the IMA namespace, which
> >>disappeared. That said, I am wondering whether there has to be a
> >>minimum set of namespaces (PID, UTS) providing enough 'isolation'
> >>that someone may actually open an IMA namespace and run their code.
> >>To avoid leaving no traces one could argue to implement recursive
> >>logging, so something that is logged inside the namespace will be
> >>detected in all parent containers up to the init_ima_ns (host)
> >>because it's logged (and TPM extended) there as well. The challenge
> >>with that is that logging costs memory and that can be abused as
> >>well until the machine needs a reboot... I guess the solution could
> >>be requesting an IMA namespace in one way or another but requiring
> >>several other namespace flags in the clone() to actually 'get' it.
> >>Jumping namespaces with setns() may have to be restricted as well
> >>once there is an IMA namespace.
> >Wait. So if I create a new IMA namespace, the things I run in
> >that namespace are not subject to the parent namespace policy?
>
> We'll let an IMA namespace set its own policy and rules in that
> policy will decide whether the child namespaces' measurements will
> also be logged. This is to avoid a potentially huge log on the host.
> However, the activities of root in namespaces may need to be logged
> independently of what the policy rules say so that root's activities
> in the TCB will always be tracked also if he operates in a temporary
> mount/IMA namespace pair (and sharing the rest of the namespaces
> with the host).

Thanks, Stefan. Is there a particular paper where I can get a good
idea of what is and is not part of the goals and threat model here?

My impression was that you are measuring things that are executed in an
effort to make sure that anything that can affect resource $x will
be at some point detectable. But if you allow containers (not in a
user namespace) to evade the ima measurements that seems to undermine
that, so that must not be your goal. And even if you insist on a
user namespace, if some resource is owned by $uid, then $uid can create
a new namespace, evade the detection, and run malicious code to affect
the resource.

Unless you're counting on the container runtime to set a proper new
ima policy? But if that's the case then you can't have every CLONE_NEWNS
start a new ima ns.

So I think I need to start from scratch.

thanks,
-serge

2018-03-09 13:54:29

[permalink] [raw]

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

On 03/08/2018 09:59 PM, Serge E. Hallyn wrote:
> On Thu, Mar 08, 2018 at 09:04:52AM -0500, Stefan Berger wrote:
>> On 07/25/2017 04:46 PM, Serge E. Hallyn wrote:
>>> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
>>>> On 07/25/2017 03:48 PM, Mimi Zohar wrote:
>>>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
>>>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
>>>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
>>>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
>>>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
>>>>>>>>>> From: Yuqiong Sun <[email protected]>
>>>>>>>>>>
>>>>>>>>>> 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).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Yuqiong Sun <[email protected]>
>>>>>>>>>>
>>>>>>>>>> Changelog:
>>>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> So this means that every mount namespace clone will clone a new
>>>>>>>>> IMA namespace. Is that really ok?
>>>>>>>> Based on what: space concerns (struct ima_ns is reasonably small)?
>>>>>>>> or whether tying it to the mount namespace is the correct thing to
>>>>>>>> do. On
>>>>>>> Mostly the latter. The other would be not so much space concerns as
>>>>>>> time concerns. Many things use new mounts namespaces, and we
>>>>>>> wouldn't want multiple IMA calls on all file accesses by all of
>>>>>>> those.
>>>>>>>
>>>>>>>> the latter, it does seem that this should be a property of either
>>>>>>>> the mount or user ns rather than its own separate ns. I could see
>>>>>>>> a use where even a container might want multiple ima keyrings
>>>>>>>> within the container (say containerised apache service with
>>>>>>>> multiple tenants), so instinct tells me that mount ns is the
>>>>>>>> correct granularity for this.
>>>>>>> I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
>>>>>>> as the trigger for requesting a new ima ns on the next
>>>>>>> clone(CLONE_NEWNS).
>>>>>> I could go with that, but what about the trigger being installing or
>>>>>> updating the keyring? That's the only operation that needs namespace
>>>>>> separation, so on mount ns clone, you get a pointer to the old ima_ns
>>>>>> until you do something that requires a new key, which then triggers the
>>>>>> copy of the namespace and installing it?
>>>>> It isn't just the keyrings that need to be namespaced, but the
>>>>> measurement list and policy as well.
>>>>>
>>>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
>>>>>
>>>>> As soon as the namespace starts, measurements should be added to the
>>>>> namespace specific measurement list, not it's parent.
>>> Shouldn't it be both?
>>>
>>> If not, then it seems to me this must be tied to user namespace.
>>>
>>>> IMA is about measuring things, logging what was executed, and
>>>> finally someone looking at the measurement log and detecting
>>>> 'things'. So at least one attack that needs to be prevented is a
>>>> malicious person opening an IMA namespace, executing something
>>>> malicious, and not leaving any trace on the host because all the
>>>> logs went into the measurement list of the IMA namespace, which
>>>> disappeared. That said, I am wondering whether there has to be a
>>>> minimum set of namespaces (PID, UTS) providing enough 'isolation'
>>>> that someone may actually open an IMA namespace and run their code.
>>>> To avoid leaving no traces one could argue to implement recursive
>>>> logging, so something that is logged inside the namespace will be
>>>> detected in all parent containers up to the init_ima_ns (host)
>>>> because it's logged (and TPM extended) there as well. The challenge
>>>> with that is that logging costs memory and that can be abused as
>>>> well until the machine needs a reboot... I guess the solution could
>>>> be requesting an IMA namespace in one way or another but requiring
>>>> several other namespace flags in the clone() to actually 'get' it.
>>>> Jumping namespaces with setns() may have to be restricted as well
>>>> once there is an IMA namespace.
>>> Wait. So if I create a new IMA namespace, the things I run in
>>> that namespace are not subject to the parent namespace policy?
>> We'll let an IMA namespace set its own policy and rules in that
>> policy will decide whether the child namespaces' measurements will
>> also be logged. This is to avoid a potentially huge log on the host.
>> However, the activities of root in namespaces may need to be logged
>> independently of what the policy rules say so that root's activities
>> in the TCB will always be tracked also if he operates in a temporary
>> mount/IMA namespace pair (and sharing the rest of the namespaces
>> with the host).
> Thanks, Stefan. Is there a particular paper where I can get a good
> idea of what is and is not part of the goals and threat model here?
Yuqiong is publishing a paper in this area. I believe the conference is
only later this year.

Our goals are to enable IMA measurements, appraisal, and auditing inside
a container using namespaces. IMA measurements are about logging files
that were read or executables that were started on a machine, appraisal
is about locking down the machine and only allowing files to be accessed
that have been properly signed. We certainly want to prevent the abuse
of namespaces by users to do things that go undetected. A primary
concern are activities of root in the TCB.

Support for IMA in namespaces should enable the following:

- IMA policy for container (similar to the host):
- there should be an initial default policy for every IMA namespace
that measures activities inside the container
- the policy can be overwritten once with a user-defined policy that
may activate appraisal

- IMA policy extensions due to namespacing:
- an IMA policy should allow rules that define whether activities in
(all) child namespaces is to be measured (prevents huge logs on the host)
- to prevent root from spawning new IMA namespaces and doing things
undetected in the TCB, all activities of root are (recursively) measured
in all IMA namespaces independent of whether the policy enables logging
in child namespaces

- IMA appraisal and keys:
- each IMA namespace should have its own keyring so that each
container can have its files signed with different keys
- it should be possible to enforce that only certified keys are
loaded onto a keyring, similar to .ima on the host

- IMA auditing:
- auditing should report activity in namespaces following the IMA
policy; root's activities in containers should be audited

- TPM and measurements:
- The IMA namespace that holds the logs should be configurable to
extend PCRs; since the single TPM of the host cannot be shared by
containers, each IMA namespace would have to be associated with its own
TPM instance (vTPM); measurement on the initial IMA namespace are
extended into the hardware TPM asdone today

A concrete 'ab-use' case that we are trying to avoid is the following:
- a user creating a privileged container that shares the host's mount
namespace: it would be unexpected if there is an IMA policy active on
the host that enforces file appraisal but in this case the IMA policy is
not enforced since a (hypothetical) IMA namespace of the host was not
joined since only the mount namespace of the host was joined. Now we
have two choices here: We tie the mount and IMA namespaces together via
sinlge clone flag, as proposed, and joining the mount namespace
automatically joins the associated IMA namespace (single setns()). Or we
make user space responsible for it and say if a mount namespace is
joined, find the associated IMA namespace (how to do that?), and join
both of them (2 setns() calls). Well, I think the former would preferred
over the latter.

Let's assume we tie MNT and IMA together, then there are other scenarios
with switching through the other namespaces (UTS, PID, IPC, NET, USER,
CGROUP). I am not sure what is supposed to happen other than logging the
activity active in the current IMA namespace:

What should happen with IMA logging, appraisal, and auditing if we
setns() through all available
- PID namespaces and send signals: log, appraise, and audit file
activity following IMA policy
- IPC namespaces and send messages via IPC: same as for PID
- UTS namespaces and setting hostname: same as for PID
- NET namespaces and sending network traffic: same as for PID?
- CGROUP namespaces and configuring cgroups: same as for PID?
- USER: should now the keys of this USER namespace be active or the keys
of the original user namespace used during the clone()? other than that,
same as for PID?

>
> My impression was that you are measuring things that are executed in an
> effort to make sure that anything that can affect resource $x will
> be at some point detectable. But if you allow containers (not in
> user namespace) to evade the ima measurements that seems to undermine
> that, so that must not be your goal. And even if you insist on a

Yes, we want to prevent 'abuse', especially by root. See above.

> user namespace, if some resource is owned by $uid, then $uid can create
> a new namespace, evade the detection, and run malicious code to affect
> the resource.
>
> Unless you're counting on the container runtime to set a proper new
> ima policy? But if that's the case then you can't have every CLONE_NEWNS
> start a new ima ns.

The container runtime will be able to overwrite a default IMA policy
that is active upon spawning an IMA namespace. This policy has to be
useful in so far that it must enable a gapless measurement chain and for
example show keys that were loaded into keyrings or the IMA policy that
was loaded.

Stefan

>
> So I think I need to start from scratch.
>
> thanks,
> -serge
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2018-03-11 22:59:47

by James Morris

[permalink] [raw]

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

On Fri, 9 Mar 2018, Stefan Berger wrote:

> Yuqiong is publishing a paper in this area. I believe the conference is only
> later this year.
>
> Our goals are to enable IMA measurements, appraisal, and auditing inside a
> container using namespaces.

This is excellent to have -- can you include this requirements analysis as
a file Documentation/security on the next posting?

Also, if you need a public space for managing these kinds of documents,
consider utilizing
http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity

- James
--
James Morris
<[email protected]>

2018-03-13 18:04:56