[PATCH v2] ima: export the measurement list when needed

[PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

Some systems can end up carrying lots of entries in the ima
measurement list. Since every entry is using a bit of kernel
memory, allow the sysadmin to export the measurement list to
the filesystem to free up some memory.

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 security/integrity/ima/ima.h       |   7 +-
 security/integrity/ima/ima_fs.c    | 171 +++++++++++++++++++++++++++++
 security/integrity/ima/ima_queue.c |   2 +-
 3 files changed, 175 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 19769bf5f6ab..78f0b706848d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -151,20 +151,19 @@ int template_desc_init_fields(const char *template_fmt,
 struct ima_template_desc *ima_template_desc_current(void);
 struct ima_template_desc *lookup_template_desc(const char *name);
 bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
+void ima_free_template_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
+int ima_export_list(const char *from);
 int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
 
-/*
- * used to protect h_table and sha_table
- */
-extern spinlock_t ima_queue_lock;
+extern struct mutex ima_extend_list_mutex;
 
 struct ima_h_table {
 	atomic_long_t len;	/* number of stored measurements in the list */
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 2000e8df0301..b60a241b0d8b 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -22,10 +22,17 @@
 #include <linux/rcupdate.h>
 #include <linux/parser.h>
 #include <linux/vmalloc.h>
+#include <linux/fs_struct.h>
+#include <linux/syscalls.h>
 
 #include "ima.h"
 
+#define secfs_mnt	"/sys/kernel/security"
+#define am_filename	"/integrity/ima/ascii_runtime_measurements"
+
 static DEFINE_MUTEX(ima_write_mutex);
+static DEFINE_MUTEX(ima_list_mutex);
+static char *ima_msmt_list_name;
 
 bool ima_canonical_fmt;
 static int __init default_canonical_fmt_setup(char *str)
@@ -362,6 +369,7 @@ static struct dentry *ascii_runtime_measurements;
 static struct dentry *runtime_measurements_count;
 static struct dentry *violations;
 static struct dentry *ima_policy;
+static struct dentry *ima_list_name;
 
 enum ima_fs_flags {
 	IMA_FS_BUSY,
@@ -449,6 +457,162 @@ static const struct file_operations ima_measure_policy_ops = {
 	.llseek = generic_file_llseek,
 };
 
+static void ima_free_list(void)
+{
+	struct ima_queue_entry *qe, *e;
+
+	list_for_each_entry_safe(qe, e, &ima_measurements, later) {
+		hlist_del_rcu(&qe->hnext);
+		atomic_long_dec(&ima_htable.len);
+
+		list_del_rcu(&qe->later);
+		ima_free_template_entry(qe->entry);
+		kfree(qe);
+	}
+}
+
+static int ima_unlink_file(const char *filename)
+{
+	struct filename *file;
+
+	file = getname_kernel(filename);
+	if (IS_ERR(file))
+		return -EINVAL;
+
+	return do_unlinkat(AT_FDCWD, file);
+}
+
+int ima_export_list(const char *from)
+{
+	static bool init_export = true;
+
+	struct file *file_out = NULL;
+	struct file *file_in = NULL;
+	const char *to = ima_msmt_list_name;
+	ssize_t bytesin, bytesout;
+	mm_segment_t fs;
+	struct path root;
+	loff_t offin = 0, offout = 0;
+	char data[512];
+	int err = 0;
+
+	if (to == NULL)
+		goto out_err;
+	if (from == NULL)
+		from = secfs_mnt am_filename;
+
+	pr_info("exporting msmt list to %s\n", to);
+	fs = get_fs();
+	set_fs(KERNEL_DS);
+
+	if (init_export) {
+		ima_unlink_file(to);
+		init_export = false;
+	}
+	/*
+	 * Use the root of the init task..
+	 */
+	task_lock(&init_task);
+	get_fs_root(init_task.fs, &root);
+	task_unlock(&init_task);
+
+	file_out = file_open_root(root.dentry, root.mnt, to,
+				  O_CREAT|O_WRONLY|O_APPEND|O_NOFOLLOW,
+				  0600);
+	if (IS_ERR(file_out)) {
+		err = PTR_ERR(file_out);
+		pr_err("failed to open %s, err %d\n", to, err);
+		file_out = NULL;
+		goto out_close;
+	}
+	file_in = file_open_root(root.dentry, root.mnt, from, O_RDONLY, 0);
+	if (IS_ERR(file_in)) {
+		err = PTR_ERR(file_in);
+		pr_err("failed to open %s, err %d\n", from, err);
+		file_in = NULL;
+		goto out_close;
+	}
+	mutex_lock(&ima_extend_list_mutex);
+	do {
+		bytesin = vfs_read(file_in, data, 512, &offin);
+		if (bytesin < 0) {
+			pr_err("read error at %lld\n", offin);
+			err = -EIO;
+			goto out_unlock;
+		}
+		bytesout = vfs_write(file_out, data, bytesin, &offout);
+		if (bytesin != bytesout) {
+			/*
+			 * If we fail writing, don't free the list and allow
+			 * a retry later on.
+			 */
+			pr_err("write error at %lld\n", offout);
+			err = -EIO;
+			goto out_unlock;
+		}
+	} while (bytesin == 512);
+	ima_free_list();
+
+out_unlock:
+	mutex_unlock(&ima_extend_list_mutex);
+out_close:
+	if (file_in)
+		filp_close(file_in, NULL);
+	if (file_out)
+		filp_close(file_out, NULL);
+
+	path_put(&root);
+	set_fs(fs);
+out_err:
+	return err;
+}
+
+static ssize_t ima_write_list_name(struct file *filp,
+				   const char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if ((count <= 1) || (count >= 255))
+		return -EINVAL;
+
+	if (*buf != '/')
+		return -EINVAL;
+
+	mutex_lock(&ima_list_mutex);
+	kfree(ima_msmt_list_name);
+
+	ima_msmt_list_name = kzalloc(count, GFP_KERNEL);
+	if (!ima_msmt_list_name) {
+		err = -ENOMEM;
+		goto out_unlock;
+	}
+	err = copy_from_user(ima_msmt_list_name, buf, count);
+	if (err) {
+		kfree(ima_msmt_list_name);
+		ima_msmt_list_name = NULL;
+		goto out_unlock;
+	}
+	if (ima_msmt_list_name[count-1] == '\n')
+		ima_msmt_list_name[count-1] = 0;
+
+	err = ima_export_list(NULL);
+out_unlock:
+	mutex_unlock(&ima_list_mutex);
+	if (err) {
+		pr_err("list export failed with %d\n", err);
+		return err;
+	}
+	return count;
+}
+
+static const struct file_operations ima_list_export_ops = {
+	.write = ima_write_list_name,
+};
+
 int __init ima_fs_init(void)
 {
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
@@ -493,6 +657,11 @@ int __init ima_fs_init(void)
 	if (IS_ERR(ima_policy))
 		goto out;
 
+	ima_list_name = securityfs_create_file("list_name", 0200, ima_dir,
+					       NULL, &ima_list_export_ops);
+	if (IS_ERR(ima_list_name))
+		goto out;
+
 	return 0;
 out:
 	securityfs_remove(violations);
@@ -502,5 +671,7 @@ int __init ima_fs_init(void)
 	securityfs_remove(ima_symlink);
 	securityfs_remove(ima_dir);
 	securityfs_remove(ima_policy);
+	securityfs_remove(ima_list_name);
+
 	return -1;
 }
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 1ce8b1701566..77c538ec8474 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -44,7 +44,7 @@ struct ima_h_table ima_htable = {
  * and extending the TPM PCR aggregate. Since tpm_extend can take
  * long (and the tpm driver uses a mutex), we can't use the spinlock.
  */
-static DEFINE_MUTEX(ima_extend_list_mutex);
+DEFINE_MUTEX(ima_extend_list_mutex);
 
 /* lookup up the digest value in the hash table, and return the entry */
 static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
-- 
2.17.1

Re: [PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Some systems can end up carrying lots of entries in the ima
> measurement list. Since every entry is using a bit of kernel
> memory, allow the sysadmin to export the measurement list to
> the filesystem to free up some memory.

Hopefully this addressed comments from everyone. The flush event can
now be triggered by the admin anytime and unique file names can be
used for each flush (log.1, log.2, ...) etc, so getting to the correct
item should be easy.

While it can now be argued that since this is an admin-driven event,
kernel does not need to write the file. However, the intention is to
bring out a second patch a bit later that adds a variable to define
the max number of entries to be kept in the kernel memory and
workqueue based automatic flushing. In those cases the kernel has to
be able to write the file without any help from the admin..


--
Janne

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

Hi Janne,

On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> >
> > Some systems can end up carrying lots of entries in the ima
> > measurement list. Since every entry is using a bit of kernel
> > memory, allow the sysadmin to export the measurement list to
> > the filesystem to free up some memory.
> 
> Hopefully this addressed comments from everyone. The flush event can
> now be triggered by the admin anytime and unique file names can be
> used for each flush (log.1, log.2, ...) etc, so getting to the correct
> item should be easy.
> 
> While it can now be argued that since this is an admin-driven event,
> kernel does not need to write the file. However, the intention is to
> bring out a second patch a bit later that adds a variable to define
> the max number of entries to be kept in the kernel memory and
> workqueue based automatic flushing. In those cases the kernel has to
> be able to write the file without any help from the admin..

I don't think it is common, and probably not acceptable, for the
kernel to open a file for writing.
 
As exporting the binary measurement list should be the equivalent of
displaying the binary measurement list and redirecting the output to a
file, the same mechanism used for displaying the binary measurement
list should be re-used for exporting it.  Just as carrying the
measurement list across kexec re-uses the same method.

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

On Wed, Jan 22, 2020 at 5:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:

> > While it can now be argued that since this is an admin-driven event,
> > kernel does not need to write the file. However, the intention is to
> > bring out a second patch a bit later that adds a variable to define
> > the max number of entries to be kept in the kernel memory and
> > workqueue based automatic flushing. In those cases the kernel has to
> > be able to write the file without any help from the admin..
>
> I don't think it is common, and probably not acceptable, for the
> kernel to open a file for writing.

Ok. It just means that the kernel cannot do its own memory management
and will depend on the user flushing the memory often enough to
prevent something bad from happening. Is this more common in the
kernel than writing out a file?


-- 
Janne

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

On Thu, 2020-01-23 at 10:41 +0200, Janne Karhunen wrote:
> On Wed, Jan 22, 2020 at 5:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> > > While it can now be argued that since this is an admin-driven event,
> > > kernel does not need to write the file. However, the intention is to
> > > bring out a second patch a bit later that adds a variable to define
> > > the max number of entries to be kept in the kernel memory and
> > > workqueue based automatic flushing. In those cases the kernel has to
> > > be able to write the file without any help from the admin..
> >
> > I don't think it is common, and probably not acceptable, for the
> > kernel to open a file for writing.
> 
> Ok. It just means that the kernel cannot do its own memory management
> and will depend on the user flushing the memory often enough to
> prevent something bad from happening. Is this more common in the
> kernel than writing out a file?

Ok, there are examples of both passing a file descriptor and passing a
pathname from userspace, but even in the case of passing a pathname,
userspace normally creates the file.

There's been discussion in the past of defining an integrity
capability.  Are we at that point where we really do need to define an
integrity capability or is everyone comfortable with relying on
CAP_SYS_ADMIN?

When implementing this feature of exporting and truncating the
measurement list, please keep in mind how this would work in the
context of IMA namespaces.

thanks,

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

On Sun, Jan 26, 2020 at 7:01 PM Mimi Zohar <zohar@linux.ibm.com> wrote:

> > > I don't think it is common, and probably not acceptable, for the
> > > kernel to open a file for writing.
> >
> > Ok. It just means that the kernel cannot do its own memory management
> > and will depend on the user flushing the memory often enough to
> > prevent something bad from happening. Is this more common in the
> > kernel than writing out a file?
>
> Ok, there are examples of both passing a file descriptor and passing a
> pathname from userspace, but even in the case of passing a pathname,
> userspace normally creates the file.

Sorry, I was slow to get your proposal. I'll try to see how that would
look like.


> There's been discussion in the past of defining an integrity
> capability.  Are we at that point where we really do need to define an
> integrity capability or is everyone comfortable with relying on
> CAP_SYS_ADMIN?

Every time something like this is being proposed there is a lot of
shouting from people that they want their root user (renamed as
CAP_SYS_ADMIN) back. I'd be happy with such bit and several others,
too.


> When implementing this feature of exporting and truncating the
> measurement list, please keep in mind how this would work in the
> context of IMA namespaces.

That could be rough. I'll try to think about it.


--
Janne

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

Hi Janne,

On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> >
> > Some systems can end up carrying lots of entries in the ima
> > measurement list. Since every entry is using a bit of kernel
> > memory, allow the sysadmin to export the measurement list to
> > the filesystem to free up some memory.
> 
> Hopefully this addressed comments from everyone. The flush event can
> now be triggered by the admin anytime and unique file names can be
> used for each flush (log.1, log.2, ...) etc, so getting to the correct
> item should be easy.
> 
> While it can now be argued that since this is an admin-driven event,
> kernel does not need to write the file. However, the intention is to
> bring out a second patch a bit later that adds a variable to define
> the max number of entries to be kept in the kernel memory and
> workqueue based automatic flushing. In those cases the kernel has to
> be able to write the file without any help from the admin..

The implications of exporting and removing records from the IMA-
measurement list needs to be considered carefully.  Verifying a TPM
quote will become dependent on knowing where the measurements are
stored.  The existing measurement list is stored in kernel memory and,
barring a kernel memory attack, is protected from modification.
 Before upstreaming this or a similar patch, there needs to be a
discussion as to how the measurement list will be protected once is it
exported to userspace.

This patch now attempts to address two very different scenarios.  The
first scenario is where userspace is requesting exporting and removing
of the measurement list records.  The other scenario is the kernel
exporting and removing of the measurement list records.  Conflating
these two different use cases might not be the right solution, as we
originally thought.

The kernel already exports the IMA measurement list to userspace via a
securityfs file.  From a userspace perspective, missing is the ability
of removing N number of records.  In this scenario, userspace would be
responsible for safely storing the measurements (e.g. blockchain).
 The kernel would only be responsible for limiting permission, perhaps
based on a capability, before removing records from the measurement
list. 

In the kernel usecase, somehow the kernel would need to safely export
the measurement list, or some portion of the measurement list, to a
file and then delete that portion.  What protects the exported records
stored in a file from modification?

Instead of exporting the measurement records, one option as suggested
by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
file for backing store.  The existing securityfs measurement lists
would then read from this private copy of the anonymous file.

I've Cc'ed fsdevel for additional comments/suggestions.

thanks,

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

On Thu, Feb 6, 2020 at 4:14 PM Mimi Zohar <zohar@linux.ibm.com> wrote:

> The implications of exporting and removing records from the IMA-
> measurement list needs to be considered carefully.  Verifying a TPM
> quote will become dependent on knowing where the measurements are
> stored.  The existing measurement list is stored in kernel memory and,
> barring a kernel memory attack, is protected from modification.
>  Before upstreaming this or a similar patch, there needs to be a
> discussion as to how the measurement list will be protected once is it
> exported to userspace.
>
> This patch now attempts to address two very different scenarios.  The
> first scenario is where userspace is requesting exporting and removing
> of the measurement list records.  The other scenario is the kernel
> exporting and removing of the measurement list records.  Conflating
> these two different use cases might not be the right solution, as we
> originally thought.
>
> The kernel already exports the IMA measurement list to userspace via a
> securityfs file.  From a userspace perspective, missing is the ability
> of removing N number of records.  In this scenario, userspace would be
> responsible for safely storing the measurements (e.g. blockchain).
>  The kernel would only be responsible for limiting permission, perhaps
> based on a capability, before removing records from the measurement
> list.

This is a good point. I will adapt the patch to this.


> In the kernel usecase, somehow the kernel would need to safely export
> the measurement list, or some portion of the measurement list, to a
> file and then delete that portion.  What protects the exported records
> stored in a file from modification?

Are we looking at protecting this file from a root exploit and the
potential DOS it might cause? In the original patch the file was root
writable only. As far as further limitations go, the easiest would
probably be to use the file immutable bit. If the kernel opens the
file and sets the immutable bit, it is the only entity that can ever
write to it - not even another root task could directly write to it.
The kernel could, as long as it keeps the file open.


> Instead of exporting the measurement records, one option as suggested
> by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> file for backing store.  The existing securityfs measurement lists
> would then read from this private copy of the anonymous file.
>
> I've Cc'ed fsdevel for additional comments/suggestions.

I didn't quickly see what the actual problem is that the vfs_tmpfile
solves in this context, will check.


--
Janne

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

On Mon, 2020-02-10 at 10:04 +0200, Janne Karhunen wrote:
> On Thu, Feb 6, 2020 at 4:14 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> > The implications of exporting and removing records from the IMA-
> > measurement list needs to be considered carefully.  Verifying a TPM
> > quote will become dependent on knowing where the measurements are
> > stored.  The existing measurement list is stored in kernel memory and,
> > barring a kernel memory attack, is protected from modification.
> >  Before upstreaming this or a similar patch, there needs to be a
> > discussion as to how the measurement list will be protected once is it
> > exported to userspace.
> >
> > This patch now attempts to address two very different scenarios.  The
> > first scenario is where userspace is requesting exporting and removing
> > of the measurement list records.  The other scenario is the kernel
> > exporting and removing of the measurement list records.  Conflating
> > these two different use cases might not be the right solution, as we
> > originally thought.
> >
> > The kernel already exports the IMA measurement list to userspace via a
> > securityfs file.  From a userspace perspective, missing is the ability
> > of removing N number of records.  In this scenario, userspace would be
> > responsible for safely storing the measurements (e.g. blockchain).
> >  The kernel would only be responsible for limiting permission, perhaps
> > based on a capability, before removing records from the measurement
> > list.
> 
> This is a good point. I will adapt the patch to this.
> 
> 
> > In the kernel usecase, somehow the kernel would need to safely export
> > the measurement list, or some portion of the measurement list, to a
> > file and then delete that portion.  What protects the exported records
> > stored in a file from modification?
> 
> Are we looking at protecting this file from a root exploit and the
> potential DOS it might cause? In the original patch the file was root
> writable only. As far as further limitations go, the easiest would
> probably be to use the file immutable bit. If the kernel opens the
> file and sets the immutable bit, it is the only entity that can ever
> write to it - not even another root task could directly write to it.
> The kernel could, as long as it keeps the file open.

The problem being addressed is freeing kernel memory instead of
letting the measurement list grow unbounded.  One solution is to
remove measurement list records, as you did, but that changes the
existing userspace expectations of returning the entire measurement
list.  In the userspace scenario, removing measurement list records is
the requirement.  For the kernel scenario, I don't think it is a
requirement.

> 
> > Instead of exporting the measurement records, one option as suggested
> > by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> > file for backing store.  The existing securityfs measurement lists
> > would then read from this private copy of the anonymous file.
> >
> > I've Cc'ed fsdevel for additional comments/suggestions.
> 
> I didn't quickly see what the actual problem is that the vfs_tmpfile
> solves in this context, will check.

The existing IMA measurement list is by design, as coined by George
Wilson, a "deliberate memory leak".  Fixing the "Deliberate IMA event
log memory leak" should be the problem description.  Amir's suggestion
of using a vfs_tmpfile seems like a reasonable solution.

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[david.safford]

On Thu, 2020-02-06 at 09:13 -0500, Mimi Zohar wrote:
> Hi Janne,
> 
> On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> > On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> > > Some systems can end up carrying lots of entries in the ima
> > > measurement list. Since every entry is using a bit of kernel
> > > memory, allow the sysadmin to export the measurement list to
> > > the filesystem to free up some memory.
> > 
> > Hopefully this addressed comments from everyone. The flush event can
> > now be triggered by the admin anytime and unique file names can be
> > used for each flush (log.1, log.2, ...) etc, so getting to the correct
> > item should be easy.
> > 
> > While it can now be argued that since this is an admin-driven event,
> > kernel does not need to write the file. However, the intention is to
> > bring out a second patch a bit later that adds a variable to define
> > the max number of entries to be kept in the kernel memory and
> > workqueue based automatic flushing. In those cases the kernel has to
> > be able to write the file without any help from the admin..
> 
> The implications of exporting and removing records from the IMA-
> measurement list needs to be considered carefully.  Verifying a TPM
> quote will become dependent on knowing where the measurements are
> stored.  The existing measurement list is stored in kernel memory and,
> barring a kernel memory attack, is protected from modification.
>  Before upstreaming this or a similar patch, there needs to be a
> discussion as to how the measurement list will be protected once is it
> exported to userspace.

"Protected" here can mean two different aspects: cryptographically
protected from tampering, which is covered with the TPM_QUOTE, and
availability protected from even accidental deletion, which is what
I suspect you are concerned about. Certainly my original TLV patches
were too flippant about this, as userspace had to be trusted not to
drop any records. In this patch, the kernel writes the data in an
atomic fashion. Either all records are successfully written, or none
are, and an error is returned.

> This patch now attempts to address two very different scenarios.  The
> first scenario is where userspace is requesting exporting and removing
> of the measurement list records.  The other scenario is the kernel
> exporting and removing of the measurement list records.  Conflating
> these two different use cases might not be the right solution, as we
> originally thought.

Actually there are at least four significant use cases: userspace
requested, and kernel initiated, both for running out of memory or
for saving the list prior to a kexec. Exporting everything to a file
prior to kexec can really simplify all the vaious use cases of 
template vs TLV formatted lists across kexec. (Consider a modern
TLV firmware kernel wanting to boot an older kernel that only
understands template formats. How simple it would be for the first
kernel to export its list to a file, and the second kernel keeps
its list in template.)

I have been testing this patch on all of these scenarios, and it
provides a simple, powerful approach for all of them.

> The kernel already exports the IMA measurement list to userspace via a
> securityfs file.  From a userspace perspective, missing is the ability
> of removing N number of records.  In this scenario, userspace would be
> responsible for safely storing the measurements (e.g. blockchain).
>  The kernel would only be responsible for limiting permission, perhaps
> based on a capability, before removing records from the measurement
> list. 

I don't think we want to export 'N' records, as this would
be really hard to understand and coordinate with userspace.
Exporting all or none seems simpler.

> In the kernel usecase, somehow the kernel would need to safely export
> the measurement list, or some portion of the measurement list, to a
> file and then delete that portion.  What protects the exported records
> stored in a file from modification?

Tampering is prevented with the TPM_QUOTE. Accidental deletion is
protected with CAP_SYS_ADMIN. If CAP_SYS_ADMIN is untrusted, you 
have bigger problems, and even then it will be detected.

> Instead of exporting the measurement records, one option as suggested
> by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> file for backing store.  The existing securityfs measurement lists
> would then read from this private copy of the anonymous file.

This doesn't help in use cases where we really do want to
export to a persistent file, without userspace help.

> I've Cc'ed fsdevel for additional comments/suggestions.
> 
> thanks,
> 
> Mimi
> 

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

On Mon, 2020-02-10 at 13:18 -0500, david.safford@gmail.com wrote:
> On Thu, 2020-02-06 at 09:13 -0500, Mimi Zohar wrote:
> > Hi Janne,
> > 
> > On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> > > On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> > > > Some systems can end up carrying lots of entries in the ima
> > > > measurement list. Since every entry is using a bit of kernel
> > > > memory, allow the sysadmin to export the measurement list to
> > > > the filesystem to free up some memory.
> > > 
> > > Hopefully this addressed comments from everyone. The flush event can
> > > now be triggered by the admin anytime and unique file names can be
> > > used for each flush (log.1, log.2, ...) etc, so getting to the correct
> > > item should be easy.
> > > 
> > > While it can now be argued that since this is an admin-driven event,
> > > kernel does not need to write the file. However, the intention is to
> > > bring out a second patch a bit later that adds a variable to define
> > > the max number of entries to be kept in the kernel memory and
> > > workqueue based automatic flushing. In those cases the kernel has to
> > > be able to write the file without any help from the admin..
> > 
> > The implications of exporting and removing records from the IMA-
> > measurement list needs to be considered carefully.  Verifying a TPM
> > quote will become dependent on knowing where the measurements are
> > stored.  The existing measurement list is stored in kernel memory and,
> > barring a kernel memory attack, is protected from modification.
> >  Before upstreaming this or a similar patch, there needs to be a
> > discussion as to how the measurement list will be protected once is it
> > exported to userspace.
> 
> "Protected" here can mean two different aspects: cryptographically
> protected from tampering, which is covered with the TPM_QUOTE, and
> availability protected from even accidental deletion, which is what
> I suspect you are concerned about. Certainly my original TLV patches
> were too flippant about this, as userspace had to be trusted not to
> drop any records. In this patch, the kernel writes the data in an
> atomic fashion. Either all records are successfully written, or none
> are, and an error is returned.

A third aspect, which I'm concerned about, is removing records from
the measurement list.  This changes the existing userspace
expectations of returning the entire measurement list.  Now userspace
will need some out of band method of knowing where to look for the
measurements.

> 
> > This patch now attempts to address two very different scenarios.  The
> > first scenario is where userspace is requesting exporting and removing
> > of the measurement list records.  The other scenario is the kernel
> > exporting and removing of the measurement list records.  Conflating
> > these two different use cases might not be the right solution, as we
> > originally thought.
> 
> Actually there are at least four significant use cases: userspace
> requested, and kernel initiated, both for running out of memory or
> for saving the list prior to a kexec. Exporting everything to a file
> prior to kexec can really simplify all the vaious use cases of 
> template vs TLV formatted lists across kexec. (Consider a modern
> TLV firmware kernel wanting to boot an older kernel that only
> understands template formats. How simple it would be for the first
> kernel to export its list to a file, and the second kernel keeps
> its list in template.)

When Thiago and I added support for carrying the measurement list
across kexec, there were a number of additional measurements after the
kexec load.  These additional measurements will need to be safely
written out to file in order to validate the TPM quote.

> I have been testing this patch on all of these scenarios, and it
> provides a simple, powerful approach for all of them.
 
Were you able to walk the measurement list and validate the TPM quote
after a kexec?

> 
> > The kernel already exports the IMA measurement list to userspace via a
> > securityfs file.  From a userspace perspective, missing is the ability
> > of removing N number of records.  In this scenario, userspace would be
> > responsible for safely storing the measurements (e.g. blockchain).
> >  The kernel would only be responsible for limiting permission, perhaps
> > based on a capability, before removing records from the measurement
> > list. 
> 
> I don't think we want to export 'N' records, as this would
> be really hard to understand and coordinate with userspace.
> Exporting all or none seems simpler.

Userspace already has the ability of exporting the measurement list.
 However, beetween saving the measurement list to a file and telling
the kernel to delete the records from the kernel, additional
measurement could have been added.

> 
> > In the kernel usecase, somehow the kernel would need to safely export
> > the measurement list, or some portion of the measurement list, to a
> > file and then delete that portion.  What protects the exported records
> > stored in a file from modification?
> 
> Tampering is prevented with the TPM_QUOTE. Accidental deletion is
> protected with CAP_SYS_ADMIN. If CAP_SYS_ADMIN is untrusted, you 
> have bigger problems, and even then it will be detected.

Agreed, attestation will detect any tampering, but up to now we didn't
have to rely on DAC/MAC to prevent tampering of the measurement list.

> > Instead of exporting the measurement records, one option as suggested
> > by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> > file for backing store.  The existing securityfs measurement lists
> > would then read from this private copy of the anonymous file.
> 
> This doesn't help in use cases where we really do want to
> export to a persistent file, without userspace help.

Is to prevent needing to carry the measurement list across kexec the
only reason for the kernel needing to write to a persistent file?

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

On Mon, Feb 10, 2020 at 10:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote:

> A third aspect, which I'm concerned about, is removing records from
> the measurement list.  This changes the existing userspace
> expectations of returning the entire measurement list.  Now userspace
> will need some out of band method of knowing where to look for the
> measurements.

Well, the original patch has a mechanism with one bit flip. You can
read the list name given that you are in the right namespace and/or
mount.


> > > The kernel already exports the IMA measurement list to userspace via a
> > > securityfs file.  From a userspace perspective, missing is the ability
> > > of removing N number of records.  In this scenario, userspace would be
> > > responsible for safely storing the measurements (e.g. blockchain).
> > >  The kernel would only be responsible for limiting permission, perhaps
> > > based on a capability, before removing records from the measurement
> > > list.
> >
> > I don't think we want to export 'N' records, as this would
> > be really hard to understand and coordinate with userspace.
> > Exporting all or none seems simpler.
>
> Userspace already has the ability of exporting the measurement list.
>  However, beetween saving the measurement list to a file and telling
> the kernel to delete the records from the kernel, additional
> measurement could have been added.

This is not an issue as long as there is no 'ALL' alias. We can't
agree on what 'ALL' is, but we can agree on 'N'.


> > Tampering is prevented with the TPM_QUOTE. Accidental deletion is
> > protected with CAP_SYS_ADMIN. If CAP_SYS_ADMIN is untrusted, you
> > have bigger problems, and even then it will be detected.
>
> Agreed, attestation will detect any tampering, but up to now we didn't
> have to rely on DAC/MAC to prevent tampering of the measurement list.

True. How about storing a hmac of the file in a securityfs entry?


--
Janne

Re: [PATCH v2] ima: export the measurement list when needed

[david.safford]

On Mon, 2020-02-10 at 15:24 -0500, Mimi Zohar wrote:
> On Mon, 2020-02-10 at 13:18 -0500, david.safford@gmail.com wrote:
> > On Thu, 2020-02-06 at 09:13 -0500, Mimi Zohar wrote:
> > > Hi Janne,
> > > 
> > > On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> > > > On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> > > > > Some systems can end up carrying lots of entries in the ima
> > > > > measurement list. Since every entry is using a bit of kernel
> > > > > memory, allow the sysadmin to export the measurement list to
> > > > > the filesystem to free up some memory.
> > > > 
> > > > Hopefully this addressed comments from everyone. The flush event can
> > > > now be triggered by the admin anytime and unique file names can be
> > > > used for each flush (log.1, log.2, ...) etc, so getting to the correct
> > > > item should be easy.
> > > > 
> > > > While it can now be argued that since this is an admin-driven event,
> > > > kernel does not need to write the file. However, the intention is to
> > > > bring out a second patch a bit later that adds a variable to define
> > > > the max number of entries to be kept in the kernel memory and
> > > > workqueue based automatic flushing. In those cases the kernel has to
> > > > be able to write the file without any help from the admin..
> > > 
> > > The implications of exporting and removing records from the IMA-
> > > measurement list needs to be considered carefully.  Verifying a TPM
> > > quote will become dependent on knowing where the measurements are
> > > stored.  The existing measurement list is stored in kernel memory and,
> > > barring a kernel memory attack, is protected from modification.
> > >  Before upstreaming this or a similar patch, there needs to be a
> > > discussion as to how the measurement list will be protected once is it
> > > exported to userspace.
> > 
> > "Protected" here can mean two different aspects: cryptographically
> > protected from tampering, which is covered with the TPM_QUOTE, and
> > availability protected from even accidental deletion, which is what
> > I suspect you are concerned about. Certainly my original TLV patches
> > were too flippant about this, as userspace had to be trusted not to
> > drop any records. In this patch, the kernel writes the data in an
> > atomic fashion. Either all records are successfully written, or none
> > are, and an error is returned.
> 
> A third aspect, which I'm concerned about, is removing records from
> the measurement list.  This changes the existing userspace
> expectations of returning the entire measurement list.  Now userspace
> will need some out of band method of knowing where to look for the
> measurements.

This is a feature, not a bug. :-)
There is no reason to resend the same data for every attestation,
nor is there any reason to store already attested measurements anywhere
on the client. By versioning the log file names, userspace gets a
simple way to know what has and has not been attested, and for small
embedded devices we don't need to waste memory or filesystem space
on the data already attested.

> When Thiago and I added support for carrying the measurement list
> across kexec, there were a number of additional measurements after the
> kexec load.  These additional measurements will need to be safely
> written out to file in order to validate the TPM quote.
> 
> > I have been testing this patch on all of these scenarios, and it
> > provides a simple, powerful approach for all of them.
>  
> Were you able to walk the measurement list and validate the TPM quote
> after a kexec?

I'm still working on this. (I've mainly been making sure this works
for normal template and TLV lists.) I should be able to write out the
remaining kexec measurements, but haven't actually validated that
yet...

> > > The kernel already exports the IMA measurement list to userspace via a
> > > securityfs file.  From a userspace perspective, missing is the ability
> > > of removing N number of records.  In this scenario, userspace would be
> > > responsible for safely storing the measurements (e.g. blockchain).
> > >  The kernel would only be responsible for limiting permission, perhaps
> > > based on a capability, before removing records from the measurement
> > > list. 
> > 
> > I don't think we want to export 'N' records, as this would
> > be really hard to understand and coordinate with userspace.
> > Exporting all or none seems simpler.
> 
> Userspace already has the ability of exporting the measurement list.
>  However, beetween saving the measurement list to a file and telling
> the kernel to delete the records from the kernel, additional
> measurement could have been added.

This method of exporting is atomic, so only those items exported
get deleted.

> > > In the kernel usecase, somehow the kernel would need to safely export
> > > the measurement list, or some portion of the measurement list, to a
> > > file and then delete that portion.  What protects the exported records
> > > stored in a file from modification?
> > 
> > Tampering is prevented with the TPM_QUOTE. Accidental deletion is
> > protected with CAP_SYS_ADMIN. If CAP_SYS_ADMIN is untrusted, you 
> > have bigger problems, and even then it will be detected.
> 
> Agreed, attestation will detect any tampering, but up to now we didn't
> have to rely on DAC/MAC to prevent tampering of the measurement list.

The userspace attestation process has always been able to tamper or
delete the list data during its attestation, but we can detect this
remotely.

> > > Instead of exporting the measurement records, one option as suggested
> > > by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> > > file for backing store.  The existing securityfs measurement lists
> > > would then read from this private copy of the anonymous file.
> > 
> > This doesn't help in use cases where we really do want to
> > export to a persistent file, without userspace help.
> 
> Is to prevent needing to carry the measurement list across kexec the
> only reason for the kernel needing to write to a persistent file?

Well, that and the other reasons mentioned, such as completely freeing
the data from the client after attestation, and simplicity of the
mechanism.

dave

> Mimi
> 

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

On Tue, 2020-02-11 at 11:10 -0500, david.safford@gmail.com wrote:
> On Mon, 2020-02-10 at 15:24 -0500, Mimi Zohar wrote:
> > On Mon, 2020-02-10 at 13:18 -0500, david.safford@gmail.com wrote:
> > > On Thu, 2020-02-06 at 09:13 -0500, Mimi Zohar wrote:
> > > > Hi Janne,
> > > > 
> > > > On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> > > > > On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> > > > > > Some systems can end up carrying lots of entries in the ima
> > > > > > measurement list. Since every entry is using a bit of kernel
> > > > > > memory, allow the sysadmin to export the measurement list to
> > > > > > the filesystem to free up some memory.
> > > > > 
> > > > > Hopefully this addressed comments from everyone. The flush event can
> > > > > now be triggered by the admin anytime and unique file names can be
> > > > > used for each flush (log.1, log.2, ...) etc, so getting to the correct
> > > > > item should be easy.
> > > > > 
> > > > > While it can now be argued that since this is an admin-driven event,
> > > > > kernel does not need to write the file. However, the intention is to
> > > > > bring out a second patch a bit later that adds a variable to define
> > > > > the max number of entries to be kept in the kernel memory and
> > > > > workqueue based automatic flushing. In those cases the kernel has to
> > > > > be able to write the file without any help from the admin..
> > > > 
> > > > The implications of exporting and removing records from the IMA-
> > > > measurement list needs to be considered carefully.  Verifying a TPM
> > > > quote will become dependent on knowing where the measurements are
> > > > stored.  The existing measurement list is stored in kernel memory and,
> > > > barring a kernel memory attack, is protected from modification.
> > > >  Before upstreaming this or a similar patch, there needs to be a
> > > > discussion as to how the measurement list will be protected once is it
> > > > exported to userspace.
> > > 
> > > "Protected" here can mean two different aspects: cryptographically
> > > protected from tampering, which is covered with the TPM_QUOTE, and
> > > availability protected from even accidental deletion, which is what
> > > I suspect you are concerned about. Certainly my original TLV patches
> > > were too flippant about this, as userspace had to be trusted not to
> > > drop any records. In this patch, the kernel writes the data in an
> > > atomic fashion. Either all records are successfully written, or none
> > > are, and an error is returned.
> > 
> > A third aspect, which I'm concerned about, is removing records from
> > the measurement list.  This changes the existing userspace
> > expectations of returning the entire measurement list.  Now userspace
> > will need some out of band method of knowing where to look for the
> > measurements.
> 
> This is a feature, not a bug. :-)
> There is no reason to resend the same data for every attestation,
> nor is there any reason to store already attested measurements anywhere
> on the client. By versioning the log file names, userspace gets a
> simple way to know what has and has not been attested, and for small
> embedded devices we don't need to waste memory or filesystem space
> on the data already attested.

This new feature will require setting up some infrastructure for
storing the partial measurement list(s) in order to validate a TPM
quote.  Userspace already can save partial measurement list(s) without
any kernel changes.  The entire measurement list does not need to be
read each time.  lseek can read past the last record previously read.
 The only new aspect is truncating the in kernel measurement list in
order to free kernel memory.

< snip> 

> > > > Instead of exporting the measurement records, one option as suggested
> > > > by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> > > > file for backing store.  The existing securityfs measurement lists
> > > > would then read from this private copy of the anonymous file.
> > > 
> > > This doesn't help in use cases where we really do want to
> > > export to a persistent file, without userspace help.
> > 
> > Is to prevent needing to carry the measurement list across kexec the
> > only reason for the kernel needing to write to a persistent file?
> 
> Well, that and the other reasons mentioned, such as completely freeing
> the data from the client after attestation, and simplicity of the
> mechanism.

Until there is proof that the measurement list can be exported to a
file before kexec, instead of carrying the measurement list across
kexec, and a TPM quote can be validated after the kexec, there isn't a
compelling reason for the kernel needing to truncate the measurement
list.

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[david.safford]

On Tue, 2020-02-11 at 18:10 -0500, Mimi Zohar wrote:
> On Tue, 2020-02-11 at 11:10 -0500, david.safford@gmail.com wrote:

> > <snip>
> > 
> This new feature will require setting up some infrastructure for
> storing the partial measurement list(s) in order to validate a TPM
> quote.  Userspace already can save partial measurement list(s) without
> any kernel changes.  The entire measurement list does not need to be
> read each time.  lseek can read past the last record previously read.
>  The only new aspect is truncating the in kernel measurement list in
> order to free kernel memory.

This is a pretty important new feature.
A lot of people can't use IMA because of the memory issue.
Also, I really think we need to let administrators choose the tradeoffs
of keeping the list in memory, on a local file, or only on the 
attestation server, as best fits their use cases.
> 
> < snip> 
> 
> Until there is proof that the measurement list can be exported to a
> file before kexec, instead of carrying the measurement list across
> kexec, and a TPM quote can be validated after the kexec, there isn't a
> compelling reason for the kernel needing to truncate the measurement
> list.

If this approach doesn't work with all the kexec use cases, then it is 
useless, and the ball is in my court to prove that it does. Fortunately
I have to test that anyway for the coming TLV support.

Working on it...

dave

> Mimi
> 

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

On Wed, 2020-02-12 at 16:08 -0500, david.safford@gmail.com wrote:
> On Tue, 2020-02-11 at 18:10 -0500, Mimi Zohar wrote:
> > On Tue, 2020-02-11 at 11:10 -0500, david.safford@gmail.com wrote:
> 
> > > <snip>
> > > 
> > This new feature will require setting up some infrastructure for
> > storing the partial measurement list(s) in order to validate a TPM
> > quote.  Userspace already can save partial measurement list(s) without
> > any kernel changes.  The entire measurement list does not need to be
> > read each time.  lseek can read past the last record previously read.
> >  The only new aspect is truncating the in kernel measurement list in
> > order to free kernel memory.
> 
> This is a pretty important new feature.
> A lot of people can't use IMA because of the memory issue.
> Also, I really think we need to let administrators choose the tradeoffs
> of keeping the list in memory, on a local file, or only on the 
> attestation server, as best fits their use cases.

Dave, I understand that some use cases require the ability of
truncating the measurement list.  We're discussing how to truncate the
measurement list.  For example, in addition to the existing securityfs
binary_runtime_measurements file, we could define a new securityfs
file indicating the number of records to delete.

> > 
> > < snip> 
> > 
> > Until there is proof that the measurement list can be exported to a
> > file before kexec, instead of carrying the measurement list across
> > kexec, and a TPM quote can be validated after the kexec, there isn't a
> > compelling reason for the kernel needing to truncate the measurement
> > list.
> 
> If this approach doesn't work with all the kexec use cases, then it is 
> useless, and the ball is in my court to prove that it does. Fortunately
> I have to test that anyway for the coming TLV support.
> 
> Working on it...

Testing could be done independently of the TLV support.  To verify
that you aren't loosing any measurements, boot with a measurement
policy like "ima_policy=tcb" on the boot command line.

thanks,

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

On Thu, Feb 13, 2020 at 3:03 AM Mimi Zohar <zohar@linux.ibm.com> wrote:

> > This is a pretty important new feature.
> > A lot of people can't use IMA because of the memory issue.
> > Also, I really think we need to let administrators choose the tradeoffs
> > of keeping the list in memory, on a local file, or only on the
> > attestation server, as best fits their use cases.
>
> Dave, I understand that some use cases require the ability of
> truncating the measurement list.  We're discussing how to truncate the
> measurement list.  For example, in addition to the existing securityfs
> binary_runtime_measurements file, we could define a new securityfs
> file indicating the number of records to delete.

I don't have strong opinions either way, just let me know how to adapt
the patch and we will get it done asap. I'd prefer a solution where
the kernel can initiate the flush, but if not then not.

Thanks everyone for all the help.


--
Janne

Re: [PATCH v2] ima: export the measurement list when needed

[Mimi Zohar]

On Thu, 2020-02-13 at 08:41 +0200, Janne Karhunen wrote:
> On Thu, Feb 13, 2020 at 3:03 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> > > This is a pretty important new feature.
> > > A lot of people can't use IMA because of the memory issue.
> > > Also, I really think we need to let administrators choose the tradeoffs
> > > of keeping the list in memory, on a local file, or only on the
> > > attestation server, as best fits their use cases.
> >
> > Dave, I understand that some use cases require the ability of
> > truncating the measurement list.  We're discussing how to truncate the
> > measurement list.  For example, in addition to the existing securityfs
> > binary_runtime_measurements file, we could define a new securityfs
> > file indicating the number of records to delete.
> 
> I don't have strong opinions either way, just let me know how to adapt
> the patch and we will get it done asap. I'd prefer a solution where
> the kernel can initiate the flush, but if not then not.

If the measurement list isn't stored in kernel memory, then we would
have the best of both worlds.  The measurement list staying intact for
attestation, with userspace's ability to truncate the measurement list
as desired.  Barring any implementation details, I see this as a win-
win solution.

Mimi

Re: [PATCH v2] ima: export the measurement list when needed

[Ken Goldman]

On 2/11/2020 11:10 AM, david.safford@gmail.com wrote:
> There is no reason to resend the same data for every attestation,
> nor is there any reason to store already attested measurements anywhere
> on the client. By versioning the log file names, userspace gets a
> simple way to know what has and has not been attested, and for small
> embedded devices we don't need to waste memory or filesystem space
> on the data already attested.

Yes ... no.

There isn't any reason to resend measurements that a verifier has 
already received.  In fact, the logic I coded also said "if PCR 10 
didn't change, there's no need to even ask for the measurement log".

However, don't you have to save already attested measurements for
the case where there are multiple verifiers?  A different verifier would 
have to receive all measurements.

Re: [PATCH v2] ima: export the measurement list when needed

[david.safford]

On Thu, 2020-02-13 at 15:11 -0500, Ken Goldman wrote:
> On 2/11/2020 11:10 AM, david.safford@gmail.com wrote:
> > There is no reason to resend the same data for every attestation,
> > nor is there any reason to store already attested measurements anywhere
> > on the client. By versioning the log file names, userspace gets a
> > simple way to know what has and has not been attested, and for small
> > embedded devices we don't need to waste memory or filesystem space
> > on the data already attested.
> 
> Yes ... no.
> 
> There isn't any reason to resend measurements that a verifier has 
> already received.  In fact, the logic I coded also said "if PCR 10 
> didn't change, there's no need to even ask for the measurement log".
> 
> However, don't you have to save already attested measurements for
> the case where there are multiple verifiers?  A different verifier would 
> have to receive all measurements.

Certainly for the use case of multiple verifiers, the client would not 
delete the data until all verifiers had acknowledged receipt.

My main point was that there are at least some use cases in which the
client does not need to keep the measurement list, and IMA should 
support the option of exporting and deleting the list in these cases.

dave

Re: [PATCH v2] ima: export the measurement list when needed

[david.safford]

On Wed, 2020-01-08 at 13:17 +0200, Janne Karhunen wrote:
> Some systems can end up carrying lots of entries in the ima
> measurement list. Since every entry is using a bit of kernel
> memory, allow the sysadmin to export the measurement list to
> the filesystem to free up some memory.
> 
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>

I like this approach, as it will work easily for measurement lists in 
any format, and it will work for user or kernel triggering.

I'm getting an OOPS, though, whenever I write a filename to the 
securityfs file (e.g. echo /var/log/ima.log > list_name).
Here's the relevant from syslog:

  BUG: unable to handle page fault for address: 00005650a0e7fe30
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0001) - permissions violation

  Oops: 0001 [#1] SMP NOPTI

  RIP: 0010:ima_write_list_name+0x35/0x114

I haven't had time to debug this. Any suggestions?

Also, one embedded comment follows...

> ---
>  security/integrity/ima/ima.h       |   7 +-
>  security/integrity/ima/ima_fs.c    | 171 +++++++++++++++++++++++++++++
>  security/integrity/ima/ima_queue.c |   2 +-
>  3 files changed, 175 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 19769bf5f6ab..78f0b706848d 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -151,20 +151,19 @@ int template_desc_init_fields(const char *template_fmt,
>  struct ima_template_desc *ima_template_desc_current(void);
>  struct ima_template_desc *lookup_template_desc(const char *name);
>  bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
> +void ima_free_template_entry(struct ima_template_entry *entry);
>  int ima_restore_measurement_entry(struct ima_template_entry *entry);
>  int ima_restore_measurement_list(loff_t bufsize, void *buf);
>  int ima_measurements_show(struct seq_file *m, void *v);
>  unsigned long ima_get_binary_runtime_size(void);
>  int ima_init_template(void);
>  void ima_init_template_list(void);
> +int ima_export_list(const char *from);
>  int __init ima_init_digests(void);
>  int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>  			  void *lsm_data);
>  
> -/*
> - * used to protect h_table and sha_table
> - */
> -extern spinlock_t ima_queue_lock;
> +extern struct mutex ima_extend_list_mutex;
>  
>  struct ima_h_table {
>  	atomic_long_t len;	/* number of stored measurements in the list */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 2000e8df0301..b60a241b0d8b 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -22,10 +22,17 @@
>  #include <linux/rcupdate.h>
>  #include <linux/parser.h>
>  #include <linux/vmalloc.h>
> +#include <linux/fs_struct.h>
> +#include <linux/syscalls.h>
>  
>  #include "ima.h"
>  
> +#define secfs_mnt	"/sys/kernel/security"
> +#define am_filename	"/integrity/ima/ascii_runtime_measurements"

You probably really want to export the binary data, as that's 
what's needed for attestation. (Or both, but that's trickier.)

dave

> +
>  static DEFINE_MUTEX(ima_write_mutex);
> +static DEFINE_MUTEX(ima_list_mutex);
> +static char *ima_msmt_list_name;
>  
> 

Re: [PATCH v2] ima: export the measurement list when needed

[Janne Karhunen]

On Fri, Jan 24, 2020 at 4:46 PM <david.safford@gmail.com> wrote:

> > Some systems can end up carrying lots of entries in the ima
> > measurement list. Since every entry is using a bit of kernel
> > memory, allow the sysadmin to export the measurement list to
> > the filesystem to free up some memory.
> >
> > Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
>
> I like this approach, as it will work easily for measurement lists in
> any format, and it will work for user or kernel triggering.

Yes, that was the point..


> I'm getting an OOPS, though, whenever I write a filename to the
> securityfs file (e.g. echo /var/log/ima.log > list_name).
> Here's the relevant from syslog:
>
>   BUG: unable to handle page fault for address: 00005650a0e7fe30
>   #PF: supervisor read access in kernel mode
>   #PF: error_code(0x0001) - permissions violation
>
>   Oops: 0001 [#1] SMP NOPTI
>
>   RIP: 0010:ima_write_list_name+0x35/0x114
>
> I haven't had time to debug this. Any suggestions?

Interesting. I'm not a X86 man, but it looks like a SMAP trap to me.
In other words, the kernel was not allowed to read that
"/var/log/ima.log" string as SMAP protection was active for that piece
of memory. I was under the impression that the SMAP is disabled while
the copy_from_user is running..

I'll try on the X86 hardware and see if I can reproduce, maybe its off
by one read or something. Only X86 target I tested was QEMU.


> > +#define secfs_mnt    "/sys/kernel/security"
> > +#define am_filename  "/integrity/ima/ascii_runtime_measurements"
>
> You probably really want to export the binary data, as that's
> what's needed for attestation. (Or both, but that's trickier.)

That's why the file name was left as an argument to the export
function; you can export either one. I suppose we'd need some setting
to tell it which one to export.


--
Janne