All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC v2 0/3] Namespace IMA log entries
@ 2021-12-01 13:20 James Bottomley
  2021-12-01 13:20 ` [RFC v2 1/3] userns: add ima_ns_info field containing a settable namespace label James Bottomley
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: James Bottomley @ 2021-12-01 13:20 UTC (permalink / raw)
  To: linux-integrity
  Cc: containers, Mimi Zohar, Dmitry Kasatkin, Stefan Berger,
	Eric W . Biederman, krzysztof.struczynski, Roberto Sassu,
	Serge E . Hallyn, Michael Peters, Luke Hinds, Lily Sturmann,
	Patrick Uiterwijk, Christian Brauner

v2: drop the user namespace uuid and instead add an IMA specific label
(residing in an opaque IMA structure pointed to by the user_ns) that
is settable once before use, but if not set reverts to a randomly
generated uuid.

Over the past five years there have been several attempts to namespace
IMA [1,2,3].  All of them were eventually fairly huge patch series,
which try to solve every issue and potential issue all at once, making
them incredibly difficult to review and highly dependent on an array
of non-IMA features which causes huge upporting difficulty as the
patch progresses.  Given this, I thought we'd try a different way:
introduce a minimal namespacing of IMA and try to build on it in
subsequent patches.

This first patch set namespaces IMA by tying it to the user
namespace. We're still discussing whether this is a good idea, so I'll
pass on the justification and note that the only addition is a
ima_ns_info pointer which points to a structure that has a lifetime
longer than the namespace, so the whole machinery for managing this
could be transferred to a different namespace.  Within this pointer is
a label for the IMA namespace, which has a set API (but no exposure in
the current patch se) and if the label isn't set before the namespace
causes an IMA log entry, a uuid is placed into the label.

All this patch set does is add a new template 'ima-ns' which includes
the namespace label (added by the first patch) in the IMA log.  Using
uuids gives us probabalistically unique identifiers for all namespaces
without having to check them for uniqueness.

Once we have the container being logged, it stands to reason that the
ima inode cache needs to record one event per namespace per inode
instead of the one global event per inode, so if I enter the ima
namespace and execute the same measured command, it will log again
with the new namespace uuid even if the hash is the same:

 > ls
 > grep ls /sys/kernel/security/integrity/ima/ascii_runtime_measurements
10 c70c7b851f83c8c71ee7b508c8468383c0d2c154 ima-ns sha256:1f7f27ef1052e33731c9ff56a36ac3af4437e3f95ad55f6813c320b087b5d356 /usr/bin/ls 6582e360-1354-42b9-a6ef-ee1993d982da
 > unshare --user -r
 # ls
 # exit
 > grep ls /sys/kernel/security/integrity/ima/ascii_runtime_measurements
 10 c70c7b851f83c8c71ee7b508c8468383c0d2c154 ima-ns sha256:1f7f27ef1052e33731c9ff56a36ac3af4437e3f95ad55f6813c320b087b5d356 /usr/bin/ls 6582e360-1354-42b9-a6ef-ee1993d982da
10 144a73d85e9cf999c4abbc99f3c41e9422c8016e ima-ns sha256:1f7f27ef1052e33731c9ff56a36ac3af4437e3f95ad55f6813c320b087b5d356 /usr/bin/ls e496e384-4133-4d57-b93a-1812b83badf2

Note that this namespacing if the iint cache is in the third patch and
could be dropped if there's huge opposition.

Some things to note are that the IMA securityfs entries aren't
virtualized.  This is planned for a follow up patch (so currently the
admin can't even view the ima log in the container).  Everything
that's logged goes through the main IMA log and the physical TPM.
This means that the admin of the physical system can attest to the
log, but the containers would have to trust the admins attestation of
their log pieces.  The initial IMA policy is also inherited from the
physical system and can't currently be changed.

The rough plan of action for follow up patches is

1. Namespace securityfs so container admin can read the IMA files like
   log which would only show entries related to the container (so only
   entries generated by the current and all child namespaces) and
   policy.

2. Add per namespace policies by writing to the policy file in the
   container.  Obviously implementation of this would have to preserve
   the security of the system, so the new namespace couldn't stop
   logging something the physical host required to be logged, but it
   could add additional classes of things to log.

3. Add the ima keyrings and the ability to appraise inside the container.

There could be other phases beyond this, including possibly optionally
attaching a vtpm to the container to provide local quotes but this
should be need driven.

Some non problems of this approach are:

* The continuous growth of the IMA log.  This is already a problem
  with non-namespaced IMA.  One can argue that the above
  implementation makes the problem worse, but it is unarguable that if
  the problem were solved generally it would no logner be an issue for
  containers.

* attesting to the in-container IMA log.  Given it's being logged
  through the physical TPM, the physical system owner will have to
  publish a mechanism for attesting to particular container entries of
  the log.

[1] https://lore.kernel.org/all/20200818152037.11869-1-krzysztof.struczynski@huawei.com
[2] https://lore.kernel.org/all/20180511144230.75384-1-stefanb@linux.vnet.ibm.com
[3] https://lore.kernel.org/all/1494511203-8397-1-git-send-email-guilherme.magalhaes@hpe.com

James

---

James Bottomley (3):
  userns: add ima_ns_info field containing a settable namespace label
  ima: show the namespace label in the ima-ns template
  ima: make the integrity inode cache per namespace

 include/linux/ima.h                       |  15 +-
 include/linux/user_namespace.h            |   7 +
 kernel/user.c                             |   1 +
 kernel/user_namespace.c                   |   6 +
 security/integrity/iint.c                 |   4 +-
 security/integrity/ima/Kconfig            |   6 +-
 security/integrity/ima/Makefile           |   2 +-
 security/integrity/ima/ima.h              |  26 +++-
 security/integrity/ima/ima_api.c          |   7 +-
 security/integrity/ima/ima_main.c         |  21 +--
 security/integrity/ima/ima_ns.c           | 169 ++++++++++++++++++++++
 security/integrity/ima/ima_policy.c       |   2 +-
 security/integrity/ima/ima_template.c     |   6 +-
 security/integrity/ima/ima_template_lib.c |  17 +++
 security/integrity/ima/ima_template_lib.h |   4 +
 security/integrity/integrity.h            |  11 +-
 16 files changed, 284 insertions(+), 20 deletions(-)
 create mode 100644 security/integrity/ima/ima_ns.c

-- 
2.33.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [RFC v2 1/3] userns: add ima_ns_info field containing a settable namespace label
  2021-12-01 13:20 [RFC v2 0/3] Namespace IMA log entries James Bottomley
@ 2021-12-01 13:20 ` James Bottomley
  2021-12-01 13:20 ` [RFC v2 2/3] ima: show the namespace label in the ima-ns template James Bottomley
  2021-12-01 13:20 ` [RFC v2 3/3] ima: make the integrity inode cache per namespace James Bottomley
  2 siblings, 0 replies; 4+ messages in thread
From: James Bottomley @ 2021-12-01 13:20 UTC (permalink / raw)
  To: linux-integrity
  Cc: containers, Mimi Zohar, Dmitry Kasatkin, Stefan Berger,
	Eric W . Biederman, krzysztof.struczynski, Roberto Sassu,
	Serge E . Hallyn, Michael Peters, Luke Hinds, Lily Sturmann,
	Patrick Uiterwijk, Christian Brauner

As a precursor to namespacing IMA a way of uniquely identifying the
namespace to appear in the IMA log is needed.  This log may be
transported away from the running system and may be analyzed even
after the system has been rebooted.  Thus we need a way of identifying
namespaces in the log which is unique.  A label is added inside the
opaque ima_ns_info structure (to make it clear this is really
something which belongs to IMA) which is added to the user_namespace.

The label is read with ima_ns_info_get_label() and is settable with
the ima_ns_info_set_label() API.  The label can only be set once and
if the label isn't set before it is read, it is set to a random uuid.
Before a label is accepted for set, it is checked for uniqueness
against every previously used label to ensure we never get a duplicate
namespace label in the log.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

v2: replace per user_ns uuid with ima_ns_info containing a settable label
---
 include/linux/ima.h             | 14 ++++++-
 include/linux/user_namespace.h  |  4 ++
 kernel/user.c                   |  1 +
 kernel/user_namespace.c         |  6 +++
 security/integrity/ima/Makefile |  2 +-
 security/integrity/ima/ima.h    |  8 ++++
 security/integrity/ima/ima_ns.c | 73 +++++++++++++++++++++++++++++++++
 7 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 security/integrity/ima/ima_ns.c

diff --git a/include/linux/ima.h b/include/linux/ima.h
index b6ab66a546ae..3391567b661e 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -13,6 +13,7 @@
 #include <linux/kexec.h>
 #include <crypto/hash_info.h>
 struct linux_binprm;
+struct ima_ns_info;
 
 #ifdef CONFIG_IMA
 extern enum hash_algo ima_get_current_hash_algo(void);
@@ -39,7 +40,8 @@ extern int ima_measure_critical_data(const char *event_label,
 				     const char *event_name,
 				     const void *buf, size_t buf_len,
 				     bool hash, u8 *digest, size_t digest_len);
-
+extern void ima_init_user_ns(struct user_namespace *ns);
+extern void ima_free_user_ns(struct user_namespace *ns);
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
 extern void ima_appraise_parse_cmdline(void);
 #else
@@ -153,6 +155,16 @@ static inline int ima_measure_critical_data(const char *event_label,
 	return -ENOENT;
 }
 
+static inline void ima_init_user_ns(struct user_namespace *ns)
+{
+	return;
+}
+
+static inline void ima_free_user_ns(struct user_namespace *ns)
+{
+	return;
+}
+
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 33a4240e6a6f..88d2a39596ba 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_USER_NAMESPACE_H
 #define _LINUX_USER_NAMESPACE_H
 
+#include <linux/ima.h>
 #include <linux/kref.h>
 #include <linux/nsproxy.h>
 #include <linux/ns_common.h>
@@ -99,6 +100,9 @@ struct user_namespace {
 #endif
 	struct ucounts		*ucounts;
 	long ucount_max[UCOUNT_COUNTS];
+#ifdef CONFIG_IMA
+	struct ima_ns_info	*ima_ns_info;
+#endif
 } __randomize_layout;
 
 struct ucounts {
diff --git a/kernel/user.c b/kernel/user.c
index e2cf8c22b539..40c0fb44ecad 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -67,6 +67,7 @@ struct user_namespace init_user_ns = {
 	.keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list),
 	.keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem),
 #endif
+	/* ima_ns_info is initialized in user_namespaces_init() */
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 6b2e3ca7ee99..08b6aaf0382b 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <linux/export.h>
+#include <linux/ima.h>
 #include <linux/nsproxy.h>
 #include <linux/slab.h>
 #include <linux/sched/signal.h>
@@ -142,6 +143,9 @@ int create_user_ns(struct cred *new)
 		goto fail_keyring;
 
 	set_cred_user_ns(new, ns);
+
+	ima_init_user_ns(ns);
+
 	return 0;
 fail_keyring:
 #ifdef CONFIG_PERSISTENT_KEYRINGS
@@ -198,6 +202,7 @@ static void free_user_ns(struct work_struct *work)
 		}
 		retire_userns_sysctls(ns);
 		key_free_user_ns(ns);
+		ima_free_user_ns(ns);
 		ns_free_inum(&ns->ns);
 		kmem_cache_free(user_ns_cachep, ns);
 		dec_user_namespaces(ucounts);
@@ -1386,6 +1391,7 @@ const struct proc_ns_operations userns_operations = {
 static __init int user_namespaces_init(void)
 {
 	user_ns_cachep = KMEM_CACHE(user_namespace, SLAB_PANIC | SLAB_ACCOUNT);
+	ima_init_user_ns(&init_user_ns);
 	return 0;
 }
 subsys_initcall(user_namespaces_init);
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 2499f2485c04..1741aa5d97bc 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -7,7 +7,7 @@
 obj-$(CONFIG_IMA) += ima.o
 
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
-	 ima_policy.o ima_template.o ima_template_lib.o
+	 ima_policy.o ima_template.o ima_template_lib.o ima_ns.o
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
 ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index be965a8715e4..b3f90971a4f7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -301,6 +301,14 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
 void ima_policy_stop(struct seq_file *m, void *v);
 int ima_policy_show(struct seq_file *m, void *v);
 
+/* IMA Namespace related functions */
+struct ima_ns_info {
+	char label[40];		/* UUIDs are 37 ascii characters */
+	struct list_head entry;
+};
+const char *ima_ns_info_get_label(struct user_namespace *ns);
+int ima_ns_info_set_label(struct user_namespace *ns, const char *label);
+
 /* Appraise integrity measurements */
 #define IMA_APPRAISE_ENFORCE	0x01
 #define IMA_APPRAISE_FIX	0x02
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
new file mode 100644
index 000000000000..a0358806149b
--- /dev/null
+++ b/security/integrity/ima/ima_ns.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * IMA Namespace Support routines
+ */
+
+#include <linux/ima.h>
+#include <linux/mutex.h>
+#include <linux/user_namespace.h>
+
+#include "ima.h"
+
+static struct kmem_cache *ima_ns_info_cache __read_mostly;
+static DEFINE_MUTEX(ns_info_list_lock);
+static LIST_HEAD(ima_ns_info_list);
+
+int ima_ns_info_set_label(struct user_namespace *ns, const char *label)
+{
+	bool found;
+	struct ima_ns_info *entry;
+
+	if (ns->ima_ns_info->label[0] != '\0')
+		/* label is already set */
+		return -EINVAL;
+
+	if (strlen(label) >= sizeof(ns->ima_ns_info->label))
+		return -E2BIG;
+
+	mutex_lock(&ns_info_list_lock);
+	list_for_each_entry(entry, &ima_ns_info_list, entry) {
+		if (strcmp(entry->label, label) == 0) {
+			found = true;
+			break;
+		}
+	}
+	mutex_unlock(&ns_info_list_lock);
+	if (found)
+		return -EEXIST;
+
+	strcpy(ns->ima_ns_info->label, label);
+
+	return 0;
+}
+
+const char *ima_ns_info_get_label(struct user_namespace *ns)
+{
+	if (unlikely(ns->ima_ns_info->label[0] == '\0')) {
+		uuid_t uuid;
+
+		uuid_gen(&uuid);
+		sprintf(ns->ima_ns_info->label, "%pU", &uuid);
+	}
+	return ns->ima_ns_info->label;
+}
+
+void ima_init_user_ns(struct user_namespace *ns)
+{
+	struct ima_ns_info *insi;
+
+	if (unlikely(!ima_ns_info_cache))
+		ima_ns_info_cache = KMEM_CACHE(ima_ns_info, SLAB_PANIC);
+
+	insi = kmem_cache_zalloc(ima_ns_info_cache, GFP_KERNEL);
+	ns->ima_ns_info = insi;
+	INIT_LIST_HEAD(&insi->entry);
+	mutex_lock(&ns_info_list_lock);
+	list_add_tail(&insi->entry, &ima_ns_info_list);
+	mutex_unlock(&ns_info_list_lock);
+}
+
+void ima_free_user_ns(struct user_namespace *ns)
+{
+}
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [RFC v2 2/3] ima: show the namespace label in the ima-ns template
  2021-12-01 13:20 [RFC v2 0/3] Namespace IMA log entries James Bottomley
  2021-12-01 13:20 ` [RFC v2 1/3] userns: add ima_ns_info field containing a settable namespace label James Bottomley
@ 2021-12-01 13:20 ` James Bottomley
  2021-12-01 13:20 ` [RFC v2 3/3] ima: make the integrity inode cache per namespace James Bottomley
  2 siblings, 0 replies; 4+ messages in thread
From: James Bottomley @ 2021-12-01 13:20 UTC (permalink / raw)
  To: linux-integrity
  Cc: containers, Mimi Zohar, Dmitry Kasatkin, Stefan Berger,
	Eric W . Biederman, krzysztof.struczynski, Roberto Sassu,
	Serge E . Hallyn, Michael Peters, Luke Hinds, Lily Sturmann,
	Patrick Uiterwijk, Christian Brauner

Use the user namespace as the basis for namespacing IMA.  The
implementation is very simple: add a new template called 'ima-ns'
which exports the label of the user namespace into the IMA log.  This
can be used to uniquely separate every container in the IMA log.

Note that the admin of the user namespace still cannot read back the
IMA log because the IMA securityfs entries are not yet namespace
aware.  However, root in the init_user_ns can read the log and see the
containers.

You can get the label of init_user_ns from the boot_aggregate entry
which is always the first one recorded in the log.  Any execution with
a different uuid is in a new IMA namespace.

A sample of the log showing entry into a container is:

10 7766c926c9db8dd4c923f96be5635b04593029c1 ima-ns sha256:0000000000000000000000000000000000000000000000000000000000000000 boot_aggregate 6582e360-1354-42b9-a6ef-ee1993d982da
[...]
10 e0355132472d4d0ae1cc044412b4033bd5e1a48a ima-ns sha256:353e4d6b807056757fb5df31bafe7df80605bec20b445d5e9afd949ca4147d49 /usr/bin/unshare 6582e360-1354-42b9-a6ef-ee1993d982da
10 f257f5a12fd6e28d32b367a2e453c3badd0e8774 ima-ns sha256:2a7c66fc7e19acc100ee2b777b71179043fade8b81968828522cf31e6a96eaa7 /usr/bin/bash e496e384-4133-4d57-b93a-1812b83badf2
10 1bb206dbdf18f75e4515aeef378ba50e555a9291 ima-ns sha256:795fb52db49b211450c7242dbcad00d782a7b8174f669c259f74a7ccabe03a90 /usr/bin/id e496e384-4133-4d57-b93a-1812b83badf2

The label setting API is not yet wired up because that will require
namespace aware securityfs.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

v2: use the ima label (which is still a uuid) instead of the uuid of the
    user namespace.
---
 include/linux/ima.h                       |  1 +
 security/integrity/ima/Kconfig            |  6 +++++-
 security/integrity/ima/ima_template.c     |  6 +++++-
 security/integrity/ima/ima_template_lib.c | 14 ++++++++++++++
 security/integrity/ima/ima_template_lib.h |  4 ++++
 5 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 3391567b661e..124710a29f17 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,7 @@
 #include <linux/fs.h>
 #include <linux/security.h>
 #include <linux/kexec.h>
+#include <linux/user_namespace.h>
 #include <crypto/hash_info.h>
 struct linux_binprm;
 struct ima_ns_info;
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index f3a9cc201c8c..4f0ce241b585 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -69,7 +69,8 @@ choice
 	  hash, defined as 20 bytes, and a null terminated pathname,
 	  limited to 255 characters.  The 'ima-ng' measurement list
 	  template permits both larger hash digests and longer
-	  pathnames.
+	  pathnames.  The 'ima-ns' adds the namespace uuid to the
+	  'ima-ng' template.
 
 	config IMA_TEMPLATE
 		bool "ima"
@@ -77,6 +78,8 @@ choice
 		bool "ima-ng (default)"
 	config IMA_SIG_TEMPLATE
 		bool "ima-sig"
+	config IMA_NS_TEMPLATE
+		bool "ima-ns"
 endchoice
 
 config IMA_DEFAULT_TEMPLATE
@@ -85,6 +88,7 @@ config IMA_DEFAULT_TEMPLATE
 	default "ima" if IMA_TEMPLATE
 	default "ima-ng" if IMA_NG_TEMPLATE
 	default "ima-sig" if IMA_SIG_TEMPLATE
+	default "ima-ns" if IMA_NS_TEMPLATE
 
 choice
 	prompt "Default integrity hash algorithm"
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 694560396be0..cd9c4c4588a4 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -24,6 +24,7 @@ static struct ima_template_desc builtin_templates[] = {
 	{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"},
 	{.name = "evm-sig",
 	 .fmt = "d-ng|n-ng|evmsig|xattrnames|xattrlengths|xattrvalues|iuid|igid|imode"},
+	{.name = "ima-ns", .fmt = "d-ng|n-ng|ns"},
 	{.name = "", .fmt = ""},	/* placeholder for a custom format */
 };
 
@@ -64,6 +65,9 @@ static const struct ima_template_field supported_fields[] = {
 	{.field_id = "xattrvalues",
 	 .field_init = ima_eventinodexattrvalues_init,
 	 .field_show = ima_show_template_sig},
+	{.field_id = "ns",
+	 .field_init = ima_ns_label_init,
+	 .field_show = ima_show_template_string},
 };
 
 /*
@@ -72,7 +76,7 @@ static const struct ima_template_field supported_fields[] = {
  * description as 'd-ng' and 'n-ng' respectively.
  */
 #define MAX_TEMPLATE_NAME_LEN \
-	sizeof("d-ng|n-ng|evmsig|xattrnames|xattrlengths|xattrvalues|iuid|igid|imode")
+	sizeof("d-ng|n-ng|evmsig|xattrnames|xattrlengths|xattrvalues|iuid|igid|imode|ns")
 
 static struct ima_template_desc *ima_template;
 static struct ima_template_desc *ima_buf_template;
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index ca017cae73eb..0a3125727534 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -685,3 +685,17 @@ int ima_eventinodexattrvalues_init(struct ima_event_data *event_data,
 {
 	return ima_eventinodexattrs_init_common(event_data, field_data, 'v');
 }
+
+/*
+ *  ima_ns_init - include the namespace label as part of the template
+ *  data
+ */
+int ima_ns_label_init(struct ima_event_data *event_data,
+		      struct ima_field_data *field_data)
+{
+	const char *label = ima_ns_info_get_label(current_user_ns());
+
+	return ima_write_template_field_data(label, strlen(label),
+					     DATA_FMT_STRING,
+					     field_data);
+}
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index c71f1de95753..bd712bffe116 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -29,6 +29,8 @@ void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
 			   struct ima_field_data *field_data);
 void ima_show_template_uint(struct seq_file *m, enum ima_show_type show,
 			    struct ima_field_data *field_data);
+void ima_show_template_ns_label(struct seq_file *m, enum ima_show_type show,
+				struct ima_field_data *field_data);
 int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
 		  int maxfields, struct ima_field_data *fields, int *curfields,
 		  unsigned long *len_mask, int enforce_mask, char *bufname);
@@ -62,4 +64,6 @@ int ima_eventinodexattrlengths_init(struct ima_event_data *event_data,
 				    struct ima_field_data *field_data);
 int ima_eventinodexattrvalues_init(struct ima_event_data *event_data,
 				   struct ima_field_data *field_data);
+int ima_ns_label_init(struct ima_event_data *event_data,
+		      struct ima_field_data *field_data);
 #endif /* __LINUX_IMA_TEMPLATE_LIB_H */
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [RFC v2 3/3] ima: make the integrity inode cache per namespace
  2021-12-01 13:20 [RFC v2 0/3] Namespace IMA log entries James Bottomley
  2021-12-01 13:20 ` [RFC v2 1/3] userns: add ima_ns_info field containing a settable namespace label James Bottomley
  2021-12-01 13:20 ` [RFC v2 2/3] ima: show the namespace label in the ima-ns template James Bottomley
@ 2021-12-01 13:20 ` James Bottomley
  2 siblings, 0 replies; 4+ messages in thread
From: James Bottomley @ 2021-12-01 13:20 UTC (permalink / raw)
  To: linux-integrity
  Cc: containers, Mimi Zohar, Dmitry Kasatkin, Stefan Berger,
	Eric W . Biederman, krzysztof.struczynski, Roberto Sassu,
	Serge E . Hallyn, Michael Peters, Luke Hinds, Lily Sturmann,
	Patrick Uiterwijk, Christian Brauner

Currently we get one entry in the IMA log per unique file event.  So,
if you have a measurement policy and it measures a particular binary
it will not get measured again if it is subsequently executed. For
Namespaced IMA, the correct behaviour seems to be to log once per
inode per namespace (so every unique execution in a namespace gets a
separate log entry).  The reason for this are 1. principle of least
surprise, so the container log always contains deterministic entries
based on runtime independent of any preceding execution and
2. security: a malicious namespace admin could infer hidden entries in
the log simply by executing various file operations and seeing if they
show up in the log or not.  Since logging once per inode per namespace
is different from current behaviour, it is only activated if the
namespace appears in the log template (so there's no behaviour change
for any of the original templates).

Expand the iint cache to have a list of namespaces, per iint entry,
the inode has been seen in by moving the action flags and the
measured_pcrs into a per-namespace structure.  The lifetime of these
additional list entries is tied to the lifetime of the iint entry and
the namespace, so if either is deleted, the new entry is.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 include/linux/user_namespace.h            |  3 +
 kernel/user.c                             |  2 +-
 security/integrity/iint.c                 |  4 +-
 security/integrity/ima/ima.h              | 18 ++++-
 security/integrity/ima/ima_api.c          |  7 +-
 security/integrity/ima/ima_main.c         | 21 ++---
 security/integrity/ima/ima_ns.c           | 96 +++++++++++++++++++++++
 security/integrity/ima/ima_policy.c       |  2 +-
 security/integrity/ima/ima_template_lib.c |  3 +
 security/integrity/integrity.h            | 11 ++-
 10 files changed, 150 insertions(+), 17 deletions(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 88d2a39596ba..52f62bf7523d 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -9,6 +9,7 @@
 #include <linux/sched.h>
 #include <linux/workqueue.h>
 #include <linux/rwsem.h>
+#include <linux/rwlock.h>
 #include <linux/sysctl.h>
 #include <linux/err.h>
 
@@ -102,6 +103,8 @@ struct user_namespace {
 	long ucount_max[UCOUNT_COUNTS];
 #ifdef CONFIG_IMA
 	struct ima_ns_info	*ima_ns_info;
+	struct list_head	ima_inode_list;
+	rwlock_t		ima_inode_list_lock;
 #endif
 } __randomize_layout;
 
diff --git a/kernel/user.c b/kernel/user.c
index 40c0fb44ecad..fea2ac611037 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -67,7 +67,7 @@ struct user_namespace init_user_ns = {
 	.keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list),
 	.keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem),
 #endif
-	/* ima_ns_info is initialized in user_namespaces_init() */
+	/* all ima fields initialized in user_namespaces_init() */
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 8638976f7990..f714532feb7d 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -71,6 +71,7 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
 static void iint_free(struct integrity_iint_cache *iint)
 {
 	kfree(iint->ima_hash);
+	ima_ns_iint_list_free(iint);
 	iint->ima_hash = NULL;
 	iint->version = 0;
 	iint->flags = 0UL;
@@ -81,7 +82,7 @@ static void iint_free(struct integrity_iint_cache *iint)
 	iint->ima_read_status = INTEGRITY_UNKNOWN;
 	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
-	iint->measured_pcrs = 0;
+	INIT_LIST_HEAD(&iint->ns_list);
 	kmem_cache_free(iint_cache, iint);
 }
 
@@ -170,6 +171,7 @@ static void init_once(void *foo)
 	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
 	mutex_init(&iint->mutex);
+	INIT_LIST_HEAD(&iint->ns_list);
 }
 
 static int __init integrity_iintcache_init(void)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b3f90971a4f7..49f5377d2d24 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -119,6 +119,16 @@ struct ima_kexec_hdr {
 	u64 count;
 };
 
+/* IMA cache of per-user-namespace flags */
+struct ima_ns_cache {
+	struct user_namespace *ns;
+	struct integrity_iint_cache *iint;
+	struct list_head ns_list;
+	struct list_head iint_list;
+	unsigned long measured_pcrs;
+	unsigned long flags;
+};
+
 extern const int read_idmap[];
 
 #ifdef CONFIG_HAVE_IMA_KEXEC
@@ -263,7 +273,8 @@ int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
 			    struct file *file, void *buf, loff_t size,
 			    enum hash_algo algo, struct modsig *modsig);
-void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
+void ima_store_measurement(struct integrity_iint_cache *iint,
+			   struct ima_ns_cache *nsc, struct file *file,
 			   const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, const struct modsig *modsig, int pcr,
@@ -306,8 +317,13 @@ struct ima_ns_info {
 	char label[40];		/* UUIDs are 37 ascii characters */
 	struct list_head entry;
 };
+extern bool ima_ns_in_template;
+void ima_init_ns(void);
 const char *ima_ns_info_get_label(struct user_namespace *ns);
 int ima_ns_info_set_label(struct user_namespace *ns, const char *label);
+struct ima_ns_cache *ima_ns_cache_get(struct integrity_iint_cache *iint,
+				      struct user_namespace *ns);
+void ima_ns_cache_clear(struct integrity_iint_cache *iint);
 
 /* Appraise integrity measurements */
 #define IMA_APPRAISE_ENFORCE	0x01
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index a64fb0130b01..d613ea1ee378 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -298,6 +298,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
  * Must be called with iint->mutex held.
  */
 void ima_store_measurement(struct integrity_iint_cache *iint,
+			   struct ima_ns_cache *nsc,
 			   struct file *file, const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, const struct modsig *modsig, int pcr,
@@ -322,7 +323,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 	 * appraisal, but a file measurement from earlier might already exist in
 	 * the measurement list.
 	 */
-	if (iint->measured_pcrs & (0x1 << pcr) && !modsig)
+	if (nsc->measured_pcrs & (0x1 << pcr) && !modsig)
 		return;
 
 	result = ima_alloc_init_template(&event_data, &entry, template_desc);
@@ -334,8 +335,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 
 	result = ima_store_template(entry, violation, inode, filename, pcr);
 	if ((!result || result == -EEXIST) && !(file->f_flags & O_DIRECT)) {
-		iint->flags |= IMA_MEASURED;
-		iint->measured_pcrs |= (0x1 << pcr);
+		nsc->flags |= IMA_MEASURED;
+		nsc->measured_pcrs |= (0x1 << pcr);
 	}
 	if (result < 0)
 		ima_free_template_entry(entry);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 465865412100..049710203fac 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -168,8 +168,8 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 		if (!IS_I_VERSION(inode) ||
 		    !inode_eq_iversion(inode, iint->version) ||
 		    (iint->flags & IMA_NEW_FILE)) {
-			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
-			iint->measured_pcrs = 0;
+			iint->flags &= ~IMA_NEW_FILE;
+			ima_ns_cache_clear(iint);
 			if (update)
 				ima_update_xattr(iint, file);
 		}
@@ -204,6 +204,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 {
 	struct inode *inode = file_inode(file);
 	struct integrity_iint_cache *iint = NULL;
+	struct ima_ns_cache *nsc = NULL;
 	struct ima_template_desc *template_desc = NULL;
 	char *pathbuf = NULL;
 	char filename[NAME_MAX];
@@ -274,20 +275,20 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	    ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
 	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) &&
 	     !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) {
-		iint->flags &= ~IMA_DONE_MASK;
-		iint->measured_pcrs = 0;
+		ima_ns_cache_clear(iint);
 	}
+	nsc = ima_ns_cache_get(iint, current_user_ns());
 
 	/* Determine if already appraised/measured based on bitmask
 	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
 	 *  IMA_AUDIT, IMA_AUDITED)
 	 */
-	iint->flags |= action;
+	nsc->flags |= action;
 	action &= IMA_DO_MASK;
-	action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
+	action &= ~((nsc->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)))
+	if ((action & IMA_MEASURE) && (nsc->measured_pcrs & (0x1 << pcr)))
 		action ^= IMA_MEASURE;
 
 	/* HASH sets the digital signature and update flags, nothing else */
@@ -297,7 +298,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		if ((xattr_value && xattr_len > 2) &&
 		    (xattr_value->type == EVM_IMA_XATTR_DIGSIG))
 			set_bit(IMA_DIGSIG, &iint->atomic_flags);
-		iint->flags |= IMA_HASHED;
+		nsc->flags |= IMA_HASHED;
 		action ^= IMA_HASH;
 		set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
 	}
@@ -342,7 +343,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
 	if (action & IMA_MEASURE)
-		ima_store_measurement(iint, file, pathname,
+		ima_store_measurement(iint, nsc, file, pathname,
 				      xattr_value, xattr_len, modsig, pcr,
 				      template_desc);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
@@ -1047,6 +1048,8 @@ static int __init init_ima(void)
 	if (error)
 		return error;
 
+	ima_init_ns();
+
 	error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
 	if (error)
 		pr_warn("Couldn't register LSM notifier, error %d\n", error);
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index a0358806149b..e5c4e1348e3d 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -13,6 +13,13 @@
 static struct kmem_cache *ima_ns_info_cache __read_mostly;
 static DEFINE_MUTEX(ns_info_list_lock);
 static LIST_HEAD(ima_ns_info_list);
+static struct kmem_cache *ns_cache __read_mostly;
+bool ima_ns_in_template __read_mostly;
+
+void __init ima_init_ns(void)
+{
+	ns_cache = KMEM_CACHE(ima_ns_cache, SLAB_PANIC);
+}
 
 int ima_ns_info_set_label(struct user_namespace *ns, const char *label)
 {
@@ -66,8 +73,97 @@ void ima_init_user_ns(struct user_namespace *ns)
 	mutex_lock(&ns_info_list_lock);
 	list_add_tail(&insi->entry, &ima_ns_info_list);
 	mutex_unlock(&ns_info_list_lock);
+	INIT_LIST_HEAD(&ns->ima_inode_list);
 }
 
 void ima_free_user_ns(struct user_namespace *ns)
 {
+	struct ima_ns_cache *entry;
+
+	/* no refs to ns left, so no need to lock */
+	while (!list_empty(&ns->ima_inode_list)) {
+		entry = list_entry(ns->ima_inode_list.next, struct ima_ns_cache,
+				   ns_list);
+
+		/* iint cache entry is still active to lock to delete */
+		write_lock(&entry->iint->ns_list_lock);
+		list_del(&entry->iint_list);
+		write_unlock(&entry->iint->ns_list_lock);
+
+		list_del(&entry->ns_list);
+		kmem_cache_free(ns_cache, entry);
+	}
+}
+
+struct ima_ns_cache *ima_ns_cache_get(struct integrity_iint_cache *iint,
+				      struct user_namespace *ns)
+{
+	struct ima_ns_cache *entry = NULL;
+
+	if (!ima_ns_in_template)
+		/*
+		 * if we're not logging the namespace, don't separate the
+		 * iint cache per namespace.  This preserves original
+		 * behaviour for the non-ns case.
+		 */
+		ns = &init_user_ns;
+
+	read_lock(&iint->ns_list_lock);
+	list_for_each_entry(entry, &iint->ns_list, ns_list)
+		if (entry->ns == ns)
+			break;
+	read_unlock(&iint->ns_list_lock);
+
+	if (entry && entry->ns == ns)
+		return entry;
+
+	entry = kmem_cache_zalloc(ns_cache, GFP_NOFS);
+	if (!entry)
+		return NULL;
+
+	/* no refs taken: entry is freed on either ns delete or iint delete */
+	entry->ns = ns;
+	entry->iint = iint;
+
+	write_lock(&iint->ns_list_lock);
+	list_add_tail(&entry->iint_list, &iint->ns_list);
+	write_unlock(&iint->ns_list_lock);
+
+	write_lock(&ns->ima_inode_list_lock);
+	list_add_tail(&entry->ns_list, &ns->ima_inode_list);
+	write_unlock(&ns->ima_inode_list_lock);
+
+	return entry;
+}
+
+/* clear the flags and measured PCR for every entry in the iint  */
+void ima_ns_cache_clear(struct integrity_iint_cache *iint)
+{
+	struct ima_ns_cache *entry;
+
+	read_lock(&iint->ns_list_lock);
+	list_for_each_entry(entry, &iint->ns_list, ns_list) {
+		entry->flags = 0;
+		entry->measured_pcrs = 0;
+	}
+	read_unlock(&iint->ns_list_lock);
+}
+
+void ima_ns_iint_list_free(struct integrity_iint_cache *iint)
+{
+	struct ima_ns_cache *entry;
+
+	/* iint locking unnecessary; no-one should have access to the list */
+	while (!list_empty(&iint->ns_list)) {
+		entry = list_entry(iint->ns_list.next, struct ima_ns_cache,
+				   iint_list);
+
+		/* namespace is still active so lock to delete */
+		write_lock(&entry->ns->ima_inode_list_lock);
+		list_del(&entry->ns_list);
+		write_unlock(&entry->ns->ima_inode_list_lock);
+
+		list_del(&entry->iint_list);
+		kmem_cache_free(ns_cache, entry);
+	}
 }
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 320ca80aacab..9434a1064da6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -50,7 +50,7 @@
 #define DONT_HASH	0x0200
 
 #define INVALID_PCR(a) (((a) < 0) || \
-	(a) >= (sizeof_field(struct integrity_iint_cache, measured_pcrs) * 8))
+	(a) >= (sizeof_field(struct ima_ns_cache, measured_pcrs) * 8))
 
 int ima_policy_flag;
 static int temp_ima_appraise;
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 0a3125727534..559b0fb239d7 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -695,6 +695,9 @@ int ima_ns_label_init(struct ima_event_data *event_data,
 {
 	const char *label = ima_ns_info_get_label(current_user_ns());
 
+	if (unlikely(!ima_ns_in_template))
+		ima_ns_in_template = true;
+
 	return ima_write_template_field_data(label, strlen(label),
 					     DATA_FMT_STRING,
 					     field_data);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..8755635da3ff 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -129,7 +129,6 @@ struct integrity_iint_cache {
 	struct inode *inode;	/* back pointer to inode in question */
 	u64 version;		/* track inode changes */
 	unsigned long flags;
-	unsigned long measured_pcrs;
 	unsigned long atomic_flags;
 	enum integrity_status ima_file_status:4;
 	enum integrity_status ima_mmap_status:4;
@@ -138,6 +137,8 @@ struct integrity_iint_cache {
 	enum integrity_status ima_creds_status:4;
 	enum integrity_status evm_status:4;
 	struct ima_digest_data *ima_hash;
+	struct list_head ns_list; /* list of namespaces inode seen in */
+	rwlock_t ns_list_lock;
 };
 
 /* rbtree tree calls to lookup, insert, delete
@@ -225,6 +226,14 @@ static inline void ima_load_x509(void)
 }
 #endif
 
+#ifdef CONFIG_IMA
+void ima_ns_iint_list_free(struct integrity_iint_cache *iint);
+#else
+void ima_ns_iint_list_free(struct integrity_iint_cache *iint)
+{
+}
+#endif
+
 #ifdef CONFIG_EVM_LOAD_X509
 void __init evm_load_x509(void);
 #else
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-12-01 13:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 13:20 [RFC v2 0/3] Namespace IMA log entries James Bottomley
2021-12-01 13:20 ` [RFC v2 1/3] userns: add ima_ns_info field containing a settable namespace label James Bottomley
2021-12-01 13:20 ` [RFC v2 2/3] ima: show the namespace label in the ima-ns template James Bottomley
2021-12-01 13:20 ` [RFC v2 3/3] ima: make the integrity inode cache per namespace James Bottomley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.