linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Allow trusted filesystems to provide IMA hashes directly
@ 2019-02-26 21:50 Matthew Garrett
  2019-02-26 21:50 ` [PATCH V2 1/4] VFS: Add a call to obtain a file's hash Matthew Garrett
                   ` (3 more replies)
  0 siblings, 4 replies; 41+ messages in thread
From: Matthew Garrett @ 2019-02-26 21:50 UTC (permalink / raw)
  To: linux-integrity; +Cc: zohar, dmitry.kasatkin, linux-fsdevel, miklos

When an IMA measurement is triggered, IMA is forced to read the entire
file and hash it. This can take a significant amount of time for large
files. If the filesystem has a secure mechanism for storing the file's
hash then it makes sense to allow the filesystem to simply return that
rather than forcing the entire file to be read.

This patchset adds an additional VFS call for providing the hash, and
teaches IMA how to use it. An additional parameter is added to the IMA
policy in order to indicate that a specific filesystem is trusted to
provide the hashes. Mounts that would otherwise match the policy but
which were mounted by a non-privileged user will still fall back to
reading the entire file to obtain the hash. Finally, a kernel parameter
is added to force hashes to be generated even if the policy says
otherwise.

This has been developed for FUSE, so the patchset includes some
additional supporting code. It adds an additional subtype parameter to
IMA policy to permit policy matching against specific FUSE filesystem
types. The expectation is that an LSM is used to restrict which
filesystems are able to mount with this subtype, preventing cases where
an untrusted FUSE filesystem is able to pretend to be a trusted one.

The use of FUSE (or any network filesystem) with IMA is already only
viable with specific security controls - an untrusted filesystem can
provide one set of data to the kernel when generating the initial
hashes, but a different set of data when the executable is actually run.
As a result, it's reasonable to assert that any setup relying on IMA
should already be imposing restrictions that ensure that FUSE
filesystems are only mounted by trustworthy executables. If this is the
case, there is no additional security concern raised by these patches.



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

* [PATCH V2 1/4] VFS: Add a call to obtain a file's hash
  2019-02-26 21:50 Allow trusted filesystems to provide IMA hashes directly Matthew Garrett
@ 2019-02-26 21:50 ` Matthew Garrett
  2019-02-26 21:50 ` [PATCH V2 2/4] IMA: Allow rule matching on filesystem subtype Matthew Garrett
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 41+ messages in thread
From: Matthew Garrett @ 2019-02-26 21:50 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, dmitry.kasatkin, linux-fsdevel, miklos, Matthew Garrett

From: Matthew Garrett <mjg59@google.com>

IMA wants to know what the hash of a file is, and currently does so by
reading the entire file and generating the hash. Some filesystems may
have the ability to store the hash in a secure manner resistant to
offline attacks (eg, filesystem-level file signing), and in that case
it's a performance win for IMA to be able to use that rather than having
to re-hash everything. This patch simply adds VFS-level support for
calling down to filesystems.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 fs/read_write.c    | 24 ++++++++++++++++++++++++
 include/linux/fs.h |  6 +++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index ff3c5e6f87cf..2b86f912be34 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -2150,3 +2150,27 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	return ret;
 }
 EXPORT_SYMBOL(vfs_dedupe_file_range);
+
+/**
+ * vfs_gethash - obtain a file's hash
+ * @file:	file structure in question
+ * @hash_algo:	the hash algorithm requested
+ * @buf:	buffer to return the hash in
+ * @size:	size allocated for the buffer by the caller
+ *
+ * This function allows filesystems that support securely storing the hash
+ * of a file to return it rather than forcing the kernel to recalculate it.
+ * Filesystems that cannot provide guarantees about the hash being resistant
+ * to offline attack should not implement this functionality.
+ *
+ * Returns 0 on success, -EOPNOTSUPP if the filesystem doesn't support it.
+ */
+int vfs_get_hash(struct file *file, enum hash_algo hash, uint8_t *buf,
+		 size_t size)
+{
+	if (!file->f_op->get_hash)
+		return -EOPNOTSUPP;
+
+	return file->f_op->get_hash(file, hash, buf, size);
+}
+EXPORT_SYMBOL(vfs_get_hash);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 29d8e2cfed0e..c6de2855cfdb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -40,6 +40,7 @@
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
+#include <uapi/linux/hash_info.h>
 
 struct backing_dev_info;
 struct bdi_writeback;
@@ -1819,6 +1820,8 @@ struct file_operations {
 				   struct file *file_out, loff_t pos_out,
 				   loff_t len, unsigned int remap_flags);
 	int (*fadvise)(struct file *, loff_t, loff_t, int);
+	int (*get_hash)(struct file *, enum hash_algo hash, uint8_t *buf,
+			size_t size);
 } __randomize_layout;
 
 struct inode_operations {
@@ -1895,7 +1898,8 @@ extern int vfs_dedupe_file_range(struct file *file,
 extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 					struct file *dst_file, loff_t dst_pos,
 					loff_t len, unsigned int remap_flags);
-
+extern int vfs_get_hash(struct file *file, enum hash_algo hash, uint8_t *buf,
+			size_t size);
 
 struct super_operations {
    	struct inode *(*alloc_inode)(struct super_block *sb);
-- 
2.21.0.rc2.261.ga7da99ff1b-goog


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

* [PATCH V2 2/4] IMA: Allow rule matching on filesystem subtype
  2019-02-26 21:50 Allow trusted filesystems to provide IMA hashes directly Matthew Garrett
  2019-02-26 21:50 ` [PATCH V2 1/4] VFS: Add a call to obtain a file's hash Matthew Garrett
@ 2019-02-26 21:50 ` Matthew Garrett
  2019-02-26 21:50 ` [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes Matthew Garrett
  2019-02-26 21:50 ` [PATCH V2 4/4] FUSE: Allow filesystems to provide gethash methods Matthew Garrett
  3 siblings, 0 replies; 41+ messages in thread
From: Matthew Garrett @ 2019-02-26 21:50 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, dmitry.kasatkin, linux-fsdevel, miklos, Matthew Garrett,
	Matthew Garrett

IMA currently allows rules to match on the filesystem type. Certain
filesystem types permit subtypes (eg, fuse). Add support to IMA to allow
rules to match on subtypes as well as types.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 Documentation/ABI/testing/ima_policy |  4 +++-
 security/integrity/ima/ima_policy.c  | 26 +++++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 74c6702de74e..09a5def7e28a 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -21,7 +21,7 @@ Description:
 			audit | hash | dont_hash
 		condition:= base | lsm  [option]
 			base:	[[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
-				[euid=] [fowner=] [fsname=]]
+				[euid=] [fowner=] [fsname=] [subtype=]]
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [permit_directio]
@@ -33,6 +33,8 @@ Description:
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
 			fsuuid:= file system UUID (e.g 8bcbe394-4f13-4144-be8e-5aa9ea2ce2f6)
+			fsname:= file system type (e.g fuse)
+			subtype:= file system subtype (e.g ntfs3g)
 			uid:= decimal value
 			euid:= decimal value
 			fowner:= decimal value
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8bc8a1c8cb3f..dcecb6aae5ec 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -35,6 +35,7 @@
 #define IMA_EUID	0x0080
 #define IMA_PCR		0x0100
 #define IMA_FSNAME	0x0200
+#define IMA_SUBTYPE	0x0400
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -80,6 +81,7 @@ struct ima_rule_entry {
 		int type;	/* audit type */
 	} lsm[MAX_LSM_RULES];
 	char *fsname;
+	char *subtype;
 };
 
 /*
@@ -306,6 +308,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 	if ((rule->flags & IMA_FSNAME)
 	    && strcmp(rule->fsname, inode->i_sb->s_type->name))
 		return false;
+	if ((rule->flags & IMA_SUBTYPE)
+	    && (inode->i_sb->s_subtype == NULL ||
+		strcmp(rule->subtype, inode->i_sb->s_subtype)))
+		return false;
 	if ((rule->flags & IMA_FSUUID) &&
 	    !uuid_equal(&rule->fsuuid, &inode->i_sb->s_uuid))
 		return false;
@@ -672,7 +678,7 @@ enum {
 	Opt_audit, Opt_hash, Opt_dont_hash,
 	Opt_obj_user, Opt_obj_role, Opt_obj_type,
 	Opt_subj_user, Opt_subj_role, Opt_subj_type,
-	Opt_func, Opt_mask, Opt_fsmagic, Opt_fsname,
+	Opt_func, Opt_mask, Opt_fsmagic, Opt_fsname, Opt_subtype,
 	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
@@ -698,6 +704,7 @@ static const match_table_t policy_tokens = {
 	{Opt_mask, "mask=%s"},
 	{Opt_fsmagic, "fsmagic=%s"},
 	{Opt_fsname, "fsname=%s"},
+	{Opt_subtype, "subtype=%s"},
 	{Opt_fsuuid, "fsuuid=%s"},
 	{Opt_uid_eq, "uid=%s"},
 	{Opt_euid_eq, "euid=%s"},
@@ -923,6 +930,17 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			result = 0;
 			entry->flags |= IMA_FSNAME;
 			break;
+		case Opt_subtype:
+			ima_log_string(ab, "subtype", args[0].from);
+
+			entry->subtype = kstrdup(args[0].from, GFP_KERNEL);
+			if (!entry->subtype) {
+				result = -ENOMEM;
+				break;
+			}
+			result = 0;
+			entry->flags |= IMA_SUBTYPE;
+			break;
 		case Opt_fsuuid:
 			ima_log_string(ab, "fsuuid", args[0].from);
 
@@ -1254,6 +1272,12 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_SUBTYPE) {
+		snprintf(tbuf, sizeof(tbuf), "%s", entry->subtype);
+		seq_printf(m, pt(Opt_subtype), tbuf);
+		seq_puts(m, " ");
+	}
+
 	if (entry->flags & IMA_PCR) {
 		snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
 		seq_printf(m, pt(Opt_pcr), tbuf);
-- 
2.21.0.rc2.261.ga7da99ff1b-goog


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

* [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-02-26 21:50 Allow trusted filesystems to provide IMA hashes directly Matthew Garrett
  2019-02-26 21:50 ` [PATCH V2 1/4] VFS: Add a call to obtain a file's hash Matthew Garrett
  2019-02-26 21:50 ` [PATCH V2 2/4] IMA: Allow rule matching on filesystem subtype Matthew Garrett
@ 2019-02-26 21:50 ` Matthew Garrett
  2019-02-28 16:03   ` Mimi Zohar
  2019-02-26 21:50 ` [PATCH V2 4/4] FUSE: Allow filesystems to provide gethash methods Matthew Garrett
  3 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-02-26 21:50 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, dmitry.kasatkin, linux-fsdevel, miklos, Matthew Garrett

From: Matthew Garrett <mjg59@google.com>

Some filesystems may be able to provide hashes in an out of band manner,
and allowing them to do so is a performance win. This is especially true
of FUSE-based filesystems where otherwise we recalculate the hash on
every measurement. Make use of this if policy says we should, but provide
a parameter to force recalculation rather than trusting the filesystem.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 Documentation/ABI/testing/ima_policy          |  5 +++-
 .../admin-guide/kernel-parameters.txt         |  5 ++++
 security/integrity/ima/ima.h                  |  3 ++-
 security/integrity/ima/ima_api.c              |  5 +++-
 security/integrity/ima/ima_crypto.c           | 23 ++++++++++++++++++-
 security/integrity/ima/ima_policy.c           |  8 ++++++-
 security/integrity/ima/ima_template_lib.c     |  2 +-
 security/integrity/integrity.h                |  1 +
 8 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 09a5def7e28a..6a517282068d 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -24,7 +24,8 @@ Description:
 				[euid=] [fowner=] [fsname=] [subtype=]]
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
-			option:	[[appraise_type=]] [permit_directio]
+			option:	[[appraise_type=] [permit_directio]
+			         [trust_vfs]]
 
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
@@ -41,6 +42,8 @@ Description:
 		lsm:  	are LSM specific
 		option:	appraise_type:= [imasig]
 			pcr:= decimal value
+			permit_directio:= allow directio accesses
+			trust_vfs:= trust VFS-provided hash values
 
 		default policy:
 			# PROC_SUPER_MAGIC
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 858b6c0b9a15..d3054a67e700 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1640,6 +1640,11 @@
 			different crypto accelerators. This option can be used
 			to achieve best performance for particular HW.
 
+	ima.force_hash= [IMA] Force hash calculation in IMA
+			Format: <bool>
+			Always calculate hashes rather than trusting the
+			filesystem to provide them to us.
+
 	init=		[KNL]
 			Format: <full_path>
 			Run specified binary instead of /sbin/init as init
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cc12f3449a72..24d9b3a3bfc0 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -133,7 +133,8 @@ int ima_fs_init(void);
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename);
-int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
+int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash,
+		       bool trust_vfs);
 int ima_calc_buffer_hash(const void *buf, loff_t len,
 			 struct ima_digest_data *hash);
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7505fb122d4..0def9cf43549 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -206,6 +206,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 	int length;
 	void *tmpbuf;
 	u64 i_version;
+	bool trust_vfs;
 	struct {
 		struct ima_digest_data hdr;
 		char digest[IMA_MAX_DIGEST_SIZE];
@@ -225,10 +226,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 	/* Initialize hash digest to 0's in case of failure */
 	memset(&hash.digest, 0, sizeof(hash.digest));
 
+	trust_vfs = iint->flags & IMA_TRUST_VFS;
+
 	if (buf)
 		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
 	else
-		result = ima_calc_file_hash(file, &hash.hdr);
+		result = ima_calc_file_hash(file, &hash.hdr, trust_vfs);
 
 	if (result && result != -EBADF && result != -EINVAL)
 		goto out;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index acf2c7df7145..94105089ad41 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -32,6 +32,10 @@ static unsigned long ima_ahash_minsize;
 module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644);
 MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use");
 
+static bool ima_force_hash;
+module_param_named(force_hash, ima_force_hash, bool_enable_only, 0644);
+MODULE_PARM_DESC(force_hash, "Always calculate hashes");
+
 /* default is 0 - 1 page. */
 static int ima_maxorder;
 static unsigned int ima_bufsize = PAGE_SIZE;
@@ -402,11 +406,14 @@ static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
  * shash for the hash calculation.  If ahash fails, it falls back to using
  * shash.
  */
-int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
+int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash,
+		       bool trust_vfs)
 {
 	loff_t i_size;
 	int rc;
 	struct file *f = file;
+	struct dentry *dentry = file_dentry(file);
+	struct inode *inode = d_backing_inode(dentry);
 	bool new_file_instance = false, modified_flags = false;
 
 	/*
@@ -419,6 +426,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 		return -EINVAL;
 	}
 
+	/*
+	 * If policy says we should trust VFS-provided hashes, and
+	 * we're not configured to force hashing, and if the
+	 * filesystem is trusted, ask the VFS if it can obtain the
+	 * hash without us having to calculate it ourself.
+	 */
+	if (trust_vfs && !ima_force_hash &&
+	    !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER)) {
+		hash->length = hash_digest_size[hash->algo];
+		rc = vfs_get_hash(file, hash->algo, hash->digest, hash->length);
+		if (!rc)
+			return 0;
+	}
+
 	/* Open a new file instance in O_RDONLY if we cannot read */
 	if (!(file->f_mode & FMODE_READ)) {
 		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index dcecb6aae5ec..af632c31f856 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -683,7 +683,7 @@ enum {
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
 	Opt_appraise_type, Opt_permit_directio,
-	Opt_pcr, Opt_err
+	Opt_pcr, Opt_trust_vfs, Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -718,6 +718,7 @@ static const match_table_t policy_tokens = {
 	{Opt_appraise_type, "appraise_type=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
+	{Opt_trust_vfs, "trust_vfs"},
 	{Opt_err, NULL}
 };
 
@@ -1060,6 +1061,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;
 			break;
+		case Opt_trust_vfs:
+			entry->flags |= IMA_TRUST_VFS;
+			break;
 		case Opt_pcr:
 			if (entry->action != MEASURE) {
 				result = -EINVAL;
@@ -1356,6 +1360,8 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, "appraise_type=imasig ");
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
 		seq_puts(m, "permit_directio ");
+	if (entry->flags & IMA_TRUST_VFS)
+		seq_puts(m, "trust_vfs ");
 	rcu_read_unlock();
 	seq_puts(m, "\n");
 	return 0;
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 43752002c222..a36cdc6651b7 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -290,7 +290,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
 	inode = file_inode(event_data->file);
 	hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ?
 	    ima_hash_algo : HASH_ALGO_SHA1;
-	result = ima_calc_file_hash(event_data->file, &hash.hdr);
+	result = ima_calc_file_hash(event_data->file, &hash.hdr, false);
 	if (result) {
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
 				    event_data->filename, "collect_data",
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7de59f44cba3..a03f859c1602 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -36,6 +36,7 @@
 #define IMA_NEW_FILE		0x04000000
 #define EVM_IMMUTABLE_DIGSIG	0x08000000
 #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
+#define IMA_TRUST_VFS		0x20000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)
-- 
2.21.0.rc2.261.ga7da99ff1b-goog


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

* [PATCH V2 4/4] FUSE: Allow filesystems to provide gethash methods
  2019-02-26 21:50 Allow trusted filesystems to provide IMA hashes directly Matthew Garrett
                   ` (2 preceding siblings ...)
  2019-02-26 21:50 ` [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes Matthew Garrett
@ 2019-02-26 21:50 ` Matthew Garrett
  2019-02-27 14:26   ` Jann Horn
  3 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-02-26 21:50 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, dmitry.kasatkin, linux-fsdevel, miklos, Matthew Garrett

From: Matthew Garrett <mjg59@google.com>

FUSE implementations may have a secure way to provide file hashes (eg,
they're a front-end to a remote store that ties files to their hashes).
Allow filesystems to expose this information, but require an option to
be provided before it can be used. This is to avoid malicious users
being able to mount an unprivileged FUSE filesystem that provides
incorrect hashes.

A sufficiently malicious FUSE filesystem may still simply swap out its
contents after the hash has been obtained - this patchset does nothing
to change that, and sysadmins should have appropriate policy in place to
protect against that.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 fs/fuse/file.c            | 34 ++++++++++++++++++++++++++++++++++
 fs/fuse/fuse_i.h          |  7 +++++++
 fs/fuse/inode.c           | 15 +++++++++++++++
 include/uapi/linux/fuse.h |  6 ++++++
 4 files changed, 62 insertions(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a59c16bd90ac..a791d69385de 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -3099,6 +3099,38 @@ static ssize_t fuse_copy_file_range(struct file *file_in, loff_t pos_in,
 
 	inode_unlock(inode_out);
 
+	return err;
+};
+
+static int fuse_file_get_hash(struct file *file, enum hash_algo hash,
+			      uint8_t *buf, size_t size)
+{
+	struct fuse_file *ff = file->private_data;
+	struct fuse_conn *fc = ff->fc;
+	FUSE_ARGS(args);
+	struct fuse_gethash_in inarg;
+	int err = 0;
+
+	if (!fc->allow_gethash)
+		return -EOPNOTSUPP;
+
+	memset(&inarg, 0, sizeof(inarg));
+	inarg.size = size;
+	inarg.hash = hash;
+	args.in.h.opcode = FUSE_GETHASH;
+	args.in.h.nodeid = ff->nodeid;
+	args.in.numargs = 1;
+	args.in.args[0].size = sizeof(inarg);
+	args.in.args[0].value = &inarg;
+	args.out.numargs = 1;
+	args.out.args[0].size = size;
+	args.out.args[0].value = buf;
+
+	err = fuse_simple_request(fc, &args);
+
+	if (err == -ENOSYS)
+		err = -EOPNOTSUPP;
+
 	return err;
 }
 
@@ -3119,6 +3151,7 @@ static const struct file_operations fuse_file_operations = {
 	.poll		= fuse_file_poll,
 	.fallocate	= fuse_file_fallocate,
 	.copy_file_range = fuse_copy_file_range,
+	.get_hash	= fuse_file_get_hash,
 };
 
 static const struct file_operations fuse_direct_io_file_operations = {
@@ -3136,6 +3169,7 @@ static const struct file_operations fuse_direct_io_file_operations = {
 	.compat_ioctl	= fuse_file_compat_ioctl,
 	.poll		= fuse_file_poll,
 	.fallocate	= fuse_file_fallocate,
+	.get_hash	= fuse_file_get_hash,
 	/* no splice_read */
 };
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 2f2c92e6f8cb..f63920ebce85 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -705,6 +705,13 @@ struct fuse_conn {
 	/** Does the filesystem support copy_file_range? */
 	unsigned no_copy_file_range:1;
 
+	/*
+	 * Allow the underlying filesystem to the hash of a file. This is
+	 * used by IMA to avoid needing to calculate the hash on every
+	 * measurement
+	 */
+	unsigned allow_gethash:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index c2d4099429be..d0d4177fb099 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -70,6 +70,7 @@ struct fuse_mount_data {
 	unsigned group_id_present:1;
 	unsigned default_permissions:1;
 	unsigned allow_other:1;
+	unsigned allow_gethash:1;
 	unsigned max_read;
 	unsigned blksize;
 };
@@ -453,6 +454,7 @@ enum {
 	OPT_ALLOW_OTHER,
 	OPT_MAX_READ,
 	OPT_BLKSIZE,
+	OPT_ALLOW_GETHASH,
 	OPT_ERR
 };
 
@@ -465,6 +467,7 @@ static const match_table_t tokens = {
 	{OPT_ALLOW_OTHER,		"allow_other"},
 	{OPT_MAX_READ,			"max_read=%u"},
 	{OPT_BLKSIZE,			"blksize=%u"},
+	{OPT_ALLOW_GETHASH,		"allow_gethash"},
 	{OPT_ERR,			NULL}
 };
 
@@ -551,6 +554,15 @@ static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev,
 			d->blksize = value;
 			break;
 
+		case OPT_ALLOW_GETHASH:
+			/*
+			 * This is relevant to security decisions made in
+			 * the root namespace, so restrict it more strongly
+			 */
+			if (capable(CAP_SYS_ADMIN))
+				d->allow_gethash = 1;
+			break;
+
 		default:
 			return 0;
 		}
@@ -574,6 +586,8 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root)
 		seq_puts(m, ",default_permissions");
 	if (fc->allow_other)
 		seq_puts(m, ",allow_other");
+	if (fc->allow_gethash)
+		seq_puts(m, ",allow_gethash");
 	if (fc->max_read != ~0)
 		seq_printf(m, ",max_read=%u", fc->max_read);
 	if (sb->s_bdev && sb->s_blocksize != FUSE_DEFAULT_BLKSIZE)
@@ -1163,6 +1177,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent)
 	fc->user_id = d.user_id;
 	fc->group_id = d.group_id;
 	fc->max_read = max_t(unsigned, 4096, d.max_read);
+	fc->allow_gethash = d.allow_gethash;
 
 	/* Used by get_root_inode() */
 	sb->s_fs_info = fc;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index b4967d48bfda..d106ea3e52f3 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -394,6 +394,7 @@ enum fuse_opcode {
 	FUSE_RENAME2		= 45,
 	FUSE_LSEEK		= 46,
 	FUSE_COPY_FILE_RANGE	= 47,
+	FUSE_GETHASH		= 48,
 
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
@@ -817,4 +818,9 @@ struct fuse_copy_file_range_in {
 	uint64_t	flags;
 };
 
+struct fuse_gethash_in {
+	uint32_t	size;
+	uint32_t	hash;
+};
+
 #endif /* _LINUX_FUSE_H */
-- 
2.21.0.rc2.261.ga7da99ff1b-goog


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

* Re: [PATCH V2 4/4] FUSE: Allow filesystems to provide gethash methods
  2019-02-26 21:50 ` [PATCH V2 4/4] FUSE: Allow filesystems to provide gethash methods Matthew Garrett
@ 2019-02-27 14:26   ` Jann Horn
  0 siblings, 0 replies; 41+ messages in thread
From: Jann Horn @ 2019-02-27 14:26 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: linux-integrity, zohar, Dmitry Kasatkin, linux-fsdevel,
	Miklos Szeredi, Matthew Garrett

On Wed, Feb 27, 2019 at 3:24 PM Matthew Garrett
<matthewgarrett@google.com> wrote:
> FUSE implementations may have a secure way to provide file hashes (eg,
> they're a front-end to a remote store that ties files to their hashes).
> Allow filesystems to expose this information, but require an option to
> be provided before it can be used. This is to avoid malicious users
> being able to mount an unprivileged FUSE filesystem that provides
> incorrect hashes.
>
> A sufficiently malicious FUSE filesystem may still simply swap out its
> contents after the hash has been obtained - this patchset does nothing
> to change that, and sysadmins should have appropriate policy in place to
> protect against that.
[...]
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 2f2c92e6f8cb..f63920ebce85 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -705,6 +705,13 @@ struct fuse_conn {
>         /** Does the filesystem support copy_file_range? */
>         unsigned no_copy_file_range:1;
>
> +       /*
> +        * Allow the underlying filesystem to the hash of a file. This is

nit: "to provide the", or something like that?

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-02-26 21:50 ` [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes Matthew Garrett
@ 2019-02-28 16:03   ` Mimi Zohar
  2019-02-28 18:05     ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-02-28 16:03 UTC (permalink / raw)
  To: Matthew Garrett, linux-integrity
  Cc: dmitry.kasatkin, linux-fsdevel, miklos, Matthew Garrett

On Tue, 2019-02-26 at 13:50 -0800, Matthew Garrett wrote:
> From: Matthew Garrett <mjg59@google.com>
> 
> Some filesystems may be able to provide hashes in an out of band manner,
> and allowing them to do so is a performance win. This is especially true
> of FUSE-based filesystems where otherwise we recalculate the hash on
> every measurement.

To be more correct, please limit this statement to trusted mounted
fuse filesystem.

> Make use of this if policy says we should, but provide
> a parameter to force recalculation rather than trusting the filesystem.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> ---
>  Documentation/ABI/testing/ima_policy          |  5 +++-
>  .../admin-guide/kernel-parameters.txt         |  5 ++++
>  security/integrity/ima/ima.h                  |  3 ++-
>  security/integrity/ima/ima_api.c              |  5 +++-
>  security/integrity/ima/ima_crypto.c           | 23 ++++++++++++++++++-
>  security/integrity/ima/ima_policy.c           |  8 ++++++-
>  security/integrity/ima/ima_template_lib.c     |  2 +-
>  security/integrity/integrity.h                |  1 +
>  8 files changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 09a5def7e28a..6a517282068d 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -24,7 +24,8 @@ Description:
>  				[euid=] [fowner=] [fsname=] [subtype=]]
>  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
>  				 [obj_user=] [obj_role=] [obj_type=]]
> -			option:	[[appraise_type=]] [permit_directio]
> +			option:	[[appraise_type=] [permit_directio]
> +			         [trust_vfs]]

Let's generalize "trust_vfs" a bit.  How about introducing
"collect_type=", with the default being reading and calculating the
file hash?

>  
>  		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>  				[FIRMWARE_CHECK]
> @@ -41,6 +42,8 @@ Description:
>  		lsm:  	are LSM specific
>  		option:	appraise_type:= [imasig]
>  			pcr:= decimal value
> +			permit_directio:= allow directio accesses
> +			trust_vfs:= trust VFS-provided hash values
>  
>  		default policy:
>  			# PROC_SUPER_MAGIC
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 858b6c0b9a15..d3054a67e700 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1640,6 +1640,11 @@
>  			different crypto accelerators. This option can be used
>  			to achieve best performance for particular HW.
>  
> +	ima.force_hash= [IMA] Force hash calculation in IMA
> +			Format: <bool>
> +			Always calculate hashes rather than trusting the
> +			filesystem to provide them to us.

Unless the IMA policy specifically defines rules with "trust_vfs", the
default is to read the file, calculating the file hash.  I don't see a
need for this boot command line option.  I do think that "trust_vfs"
needs to be limited.  Perhaps limiting it to a specific mounted
filesystem?  Perhaps limiting it to systems requiring signed IMA
policies?

Mimi

> +
>  	init=		[KNL]
>  			Format: <full_path>
>  			Run specified binary instead of /sbin/init as init
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index cc12f3449a72..24d9b3a3bfc0 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -133,7 +133,8 @@ int ima_fs_init(void);
>  int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>  			   const char *op, struct inode *inode,
>  			   const unsigned char *filename);
> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash,
> +		       bool trust_vfs);
>  int ima_calc_buffer_hash(const void *buf, loff_t len,
>  			 struct ima_digest_data *hash);
>  int ima_calc_field_array_hash(struct ima_field_data *field_data,
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c7505fb122d4..0def9cf43549 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -206,6 +206,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>  	int length;
>  	void *tmpbuf;
>  	u64 i_version;
> +	bool trust_vfs;
>  	struct {
>  		struct ima_digest_data hdr;
>  		char digest[IMA_MAX_DIGEST_SIZE];
> @@ -225,10 +226,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>  	/* Initialize hash digest to 0's in case of failure */
>  	memset(&hash.digest, 0, sizeof(hash.digest));
>  
> +	trust_vfs = iint->flags & IMA_TRUST_VFS;
> +
>  	if (buf)
>  		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
>  	else
> -		result = ima_calc_file_hash(file, &hash.hdr);
> +		result = ima_calc_file_hash(file, &hash.hdr, trust_vfs);
>  
>  	if (result && result != -EBADF && result != -EINVAL)
>  		goto out;
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index acf2c7df7145..94105089ad41 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -32,6 +32,10 @@ static unsigned long ima_ahash_minsize;
>  module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644);
>  MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use");
>  
> +static bool ima_force_hash;
> +module_param_named(force_hash, ima_force_hash, bool_enable_only, 0644);
> +MODULE_PARM_DESC(force_hash, "Always calculate hashes");
> +
>  /* default is 0 - 1 page. */
>  static int ima_maxorder;
>  static unsigned int ima_bufsize = PAGE_SIZE;
> @@ -402,11 +406,14 @@ static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash)
>   * shash for the hash calculation.  If ahash fails, it falls back to using
>   * shash.
>   */
> -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash,
> +		       bool trust_vfs)
>  {
>  	loff_t i_size;
>  	int rc;
>  	struct file *f = file;
> +	struct dentry *dentry = file_dentry(file);
> +	struct inode *inode = d_backing_inode(dentry);
>  	bool new_file_instance = false, modified_flags = false;
>  
>  	/*
> @@ -419,6 +426,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * If policy says we should trust VFS-provided hashes, and
> +	 * we're not configured to force hashing, and if the
> +	 * filesystem is trusted, ask the VFS if it can obtain the
> +	 * hash without us having to calculate it ourself.
> +	 */
> +	if (trust_vfs && !ima_force_hash &&
> +	    !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER)) {
> +		hash->length = hash_digest_size[hash->algo];
> +		rc = vfs_get_hash(file, hash->algo, hash->digest, hash->length);
> +		if (!rc)
> +			return 0;
> +	}
> +
>  	/* Open a new file instance in O_RDONLY if we cannot read */
>  	if (!(file->f_mode & FMODE_READ)) {
>  		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index dcecb6aae5ec..af632c31f856 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -683,7 +683,7 @@ enum {
>  	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
>  	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
>  	Opt_appraise_type, Opt_permit_directio,
> -	Opt_pcr, Opt_err
> +	Opt_pcr, Opt_trust_vfs, Opt_err
>  };
>  
>  static const match_table_t policy_tokens = {
> @@ -718,6 +718,7 @@ static const match_table_t policy_tokens = {
>  	{Opt_appraise_type, "appraise_type=%s"},
>  	{Opt_permit_directio, "permit_directio"},
>  	{Opt_pcr, "pcr=%s"},
> +	{Opt_trust_vfs, "trust_vfs"},
>  	{Opt_err, NULL}
>  };
>  
> @@ -1060,6 +1061,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  		case Opt_permit_directio:
>  			entry->flags |= IMA_PERMIT_DIRECTIO;
>  			break;
> +		case Opt_trust_vfs:
> +			entry->flags |= IMA_TRUST_VFS;
> +			break;
>  		case Opt_pcr:
>  			if (entry->action != MEASURE) {
>  				result = -EINVAL;
> @@ -1356,6 +1360,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>  		seq_puts(m, "appraise_type=imasig ");
>  	if (entry->flags & IMA_PERMIT_DIRECTIO)
>  		seq_puts(m, "permit_directio ");
> +	if (entry->flags & IMA_TRUST_VFS)
> +		seq_puts(m, "trust_vfs ");
>  	rcu_read_unlock();
>  	seq_puts(m, "\n");
>  	return 0;
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 43752002c222..a36cdc6651b7 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -290,7 +290,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
>  	inode = file_inode(event_data->file);
>  	hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ?
>  	    ima_hash_algo : HASH_ALGO_SHA1;
> -	result = ima_calc_file_hash(event_data->file, &hash.hdr);
> +	result = ima_calc_file_hash(event_data->file, &hash.hdr, false);
>  	if (result) {
>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
>  				    event_data->filename, "collect_data",
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 7de59f44cba3..a03f859c1602 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -36,6 +36,7 @@
>  #define IMA_NEW_FILE		0x04000000
>  #define EVM_IMMUTABLE_DIGSIG	0x08000000
>  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
> +#define IMA_TRUST_VFS		0x20000000
>  
>  #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>  				 IMA_HASH | IMA_APPRAISE_SUBMASK)


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-02-28 16:03   ` Mimi Zohar
@ 2019-02-28 18:05     ` Mimi Zohar
  2019-02-28 21:41       ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-02-28 18:05 UTC (permalink / raw)
  To: Matthew Garrett, linux-integrity
  Cc: dmitry.kasatkin, linux-fsdevel, miklos, Matthew Garrett


> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index 09a5def7e28a..6a517282068d 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -24,7 +24,8 @@ Description:
> >  				[euid=] [fowner=] [fsname=] [subtype=]]
> >  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
> >  				 [obj_user=] [obj_role=] [obj_type=]]
> > -			option:	[[appraise_type=]] [permit_directio]
> > +			option:	[[appraise_type=] [permit_directio]
> > +			         [trust_vfs]]
> 
> Let's generalize "trust_vfs" a bit.  How about introducing
> "collect_type=", with the default being reading and calculating the
> file hash?

The naming might be based on the VFS name (e.g vfs_read, vfs_get_hash)
or on the file_operations name (eg. read, get_hash).

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-02-28 18:05     ` Mimi Zohar
@ 2019-02-28 21:41       ` Matthew Garrett
  2019-02-28 21:59         ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-02-28 21:41 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Feb 28, 2019 at 10:05 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
>
> > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > > index 09a5def7e28a..6a517282068d 100644
> > > --- a/Documentation/ABI/testing/ima_policy
> > > +++ b/Documentation/ABI/testing/ima_policy
> > > @@ -24,7 +24,8 @@ Description:
> > >                             [euid=] [fowner=] [fsname=] [subtype=]]
> > >                     lsm:    [[subj_user=] [subj_role=] [subj_type=]
> > >                              [obj_user=] [obj_role=] [obj_type=]]
> > > -                   option: [[appraise_type=]] [permit_directio]
> > > +                   option: [[appraise_type=] [permit_directio]
> > > +                            [trust_vfs]]
> >
> > Let's generalize "trust_vfs" a bit.  How about introducing
> > "collect_type=", with the default being reading and calculating the
> > file hash?
>
> The naming might be based on the VFS name (e.g vfs_read, vfs_get_hash)
> or on the file_operations name (eg. read, get_hash).

If collect_type=get_hash and the filesystem doesn't support the
get_hash type, should the behaviour be to fall back to read?

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-02-28 21:41       ` Matthew Garrett
@ 2019-02-28 21:59         ` Mimi Zohar
  2019-02-28 22:38           ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-02-28 21:59 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote:
> On Thu, Feb 28, 2019 at 10:05 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> >
> > > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > > > index 09a5def7e28a..6a517282068d 100644
> > > > --- a/Documentation/ABI/testing/ima_policy
> > > > +++ b/Documentation/ABI/testing/ima_policy
> > > > @@ -24,7 +24,8 @@ Description:
> > > >                             [euid=] [fowner=] [fsname=] [subtype=]]
> > > >                     lsm:    [[subj_user=] [subj_role=] [subj_type=]
> > > >                              [obj_user=] [obj_role=] [obj_type=]]
> > > > -                   option: [[appraise_type=]] [permit_directio]
> > > > +                   option: [[appraise_type=] [permit_directio]
> > > > +                            [trust_vfs]]
> > >
> > > Let's generalize "trust_vfs" a bit.  How about introducing
> > > "collect_type=", with the default being reading and calculating the
> > > file hash?
> >
> > The naming might be based on the VFS name (e.g vfs_read, vfs_get_hash)
> > or on the file_operations name (eg. read, get_hash).
> 
> If collect_type=get_hash and the filesystem doesn't support the
> get_hash type, should the behaviour be to fall back to read?

"get_hash" should be limited to a specific filesystem type and
subtype.  Based on the filesystem type and subtype, couldn't a warning
be emitted at policy load time.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-02-28 21:59         ` Mimi Zohar
@ 2019-02-28 22:38           ` Matthew Garrett
  2019-03-04 19:52             ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-02-28 22:38 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Feb 28, 2019 at 1:59 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote:
> > If collect_type=get_hash and the filesystem doesn't support the
> > get_hash type, should the behaviour be to fall back to read?
>
> "get_hash" should be limited to a specific filesystem type and
> subtype.  Based on the filesystem type and subtype, couldn't a warning
> be emitted at policy load time.

The policy may be loaded before the filesystem is mounted, so even if
we added a capabilities mechanism we wouldn't be able to verify it.
There's also potentially cases where a filesystem could support hash
retrieval for some files but not others, and in that case we'd
probably want to fall back to reading the file.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-02-28 22:38           ` Matthew Garrett
@ 2019-03-04 19:52             ` Matthew Garrett
  2019-03-04 20:32               ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-04 19:52 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Feb 28, 2019 at 2:38 PM Matthew Garrett <mjg59@google.com> wrote:
>
> On Thu, Feb 28, 2019 at 1:59 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote:
> > > If collect_type=get_hash and the filesystem doesn't support the
> > > get_hash type, should the behaviour be to fall back to read?
> >
> > "get_hash" should be limited to a specific filesystem type and
> > subtype.  Based on the filesystem type and subtype, couldn't a warning
> > be emitted at policy load time.
>
> The policy may be loaded before the filesystem is mounted, so even if
> we added a capabilities mechanism we wouldn't be able to verify it.
> There's also potentially cases where a filesystem could support hash
> retrieval for some files but not others, and in that case we'd
> probably want to fall back to reading the file.

To be clear, I'm entirely happy to make this change - I'd just like to
ensure that I do it the right way!

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-04 19:52             ` Matthew Garrett
@ 2019-03-04 20:32               ` Mimi Zohar
  2019-03-04 22:10                 ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-03-04 20:32 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Mon, 2019-03-04 at 11:52 -0800, Matthew Garrett wrote:
> On Thu, Feb 28, 2019 at 2:38 PM Matthew Garrett <mjg59@google.com> wrote:
> >
> > On Thu, Feb 28, 2019 at 1:59 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote:
> > > > If collect_type=get_hash and the filesystem doesn't support the
> > > > get_hash type, should the behaviour be to fall back to read?
> > >
> > > "get_hash" should be limited to a specific filesystem type and
> > > subtype.  Based on the filesystem type and subtype, couldn't a warning
> > > be emitted at policy load time.
> >
> > The policy may be loaded before the filesystem is mounted, so even if
> > we added a capabilities mechanism we wouldn't be able to verify it.
> > There's also potentially cases where a filesystem could support hash
> > retrieval for some files but not others, and in that case we'd
> > probably want to fall back to reading the file.
> 
> To be clear, I'm entirely happy to make this change - I'd just like to
> ensure that I do it the right way!

Falling back to reading the file is fine.  So we're assuming that the
person signing a policy containing "get_hash" understands the
ramifications.  And yes, only signed policies containing "get_hash"
should be loaded.

I'd really appreciate a regression test (eg. ltp, xfstests, or
kselftests).

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-04 20:32               ` Mimi Zohar
@ 2019-03-04 22:10                 ` Matthew Garrett
  2019-03-05 13:18                   ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-04 22:10 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Mon, Mar 4, 2019 at 12:32 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Mon, 2019-03-04 at 11:52 -0800, Matthew Garrett wrote:
> > To be clear, I'm entirely happy to make this change - I'd just like to
> > ensure that I do it the right way!
>
> Falling back to reading the file is fine.  So we're assuming that the
> person signing a policy containing "get_hash" understands the
> ramifications.  And yes, only signed policies containing "get_hash"
> should be loaded.

I'm not clear on why requiring signed policies is helpful here. If you
allow FUSE mounts at all then you need to trust the FUSE filesystem to
return good results, in which case you can trust it to return valid
hashes. If you don't trust the FUSE filesystem then generating the
hash via read doesn't win you anything - the filesystem can return one
set of data on the initial IMA hashing, and then return a second set
later. Requiring signed policy doesn't change that.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-04 22:10                 ` Matthew Garrett
@ 2019-03-05 13:18                   ` Mimi Zohar
  2019-03-05 18:39                     ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-03-05 13:18 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Mon, 2019-03-04 at 14:10 -0800, Matthew Garrett wrote:
> On Mon, Mar 4, 2019 at 12:32 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Mon, 2019-03-04 at 11:52 -0800, Matthew Garrett wrote:
> > > To be clear, I'm entirely happy to make this change - I'd just like to
> > > ensure that I do it the right way!
> >
> > Falling back to reading the file is fine.  So we're assuming that the
> > person signing a policy containing "get_hash" understands the
> > ramifications.  And yes, only signed policies containing "get_hash"
> > should be loaded.
> 
> I'm not clear on why requiring signed policies is helpful here. If you
> allow FUSE mounts at all then you need to trust the FUSE filesystem to
> return good results, in which case you can trust it to return valid
> hashes. If you don't trust the FUSE filesystem then generating the
> hash via read doesn't win you anything - the filesystem can return one
> set of data on the initial IMA hashing, and then return a second set
> later. Requiring signed policy doesn't change that.

You're defining a new generic file ops "get_hash", but are using FUSE,
a specific filesystem, as an example.  Requiring the IMA policy to be
signed when using "get_hash", is proof of the sysadmin's agreement to
bypass actually reading and calculating the file hash.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-05 13:18                   ` Mimi Zohar
@ 2019-03-05 18:39                     ` Matthew Garrett
  2019-03-05 19:51                       ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-05 18:39 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Tue, Mar 5, 2019 at 5:19 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2019-03-04 at 14:10 -0800, Matthew Garrett wrote:
> > I'm not clear on why requiring signed policies is helpful here. If you
> > allow FUSE mounts at all then you need to trust the FUSE filesystem to
> > return good results, in which case you can trust it to return valid
> > hashes. If you don't trust the FUSE filesystem then generating the
> > hash via read doesn't win you anything - the filesystem can return one
> > set of data on the initial IMA hashing, and then return a second set
> > later. Requiring signed policy doesn't change that.
>
> You're defining a new generic file ops "get_hash", but are using FUSE,
> a specific filesystem, as an example.  Requiring the IMA policy to be
> signed when using "get_hash", is proof of the sysadmin's agreement to
> bypass actually reading and calculating the file hash.

We can trust in-kernel filesystems to return reliable information.
Network filesystems have the same issue as FUSE - we're trusting that
the remote endpoint won't give us different information on successive
reads. What's the threat that's blocked by requiring signed policy
here?

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-05 18:39                     ` Matthew Garrett
@ 2019-03-05 19:51                       ` Mimi Zohar
  2019-03-05 20:27                         ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-03-05 19:51 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Tue, 2019-03-05 at 10:39 -0800, Matthew Garrett wrote:
> On Tue, Mar 5, 2019 at 5:19 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Mon, 2019-03-04 at 14:10 -0800, Matthew Garrett wrote:
> > > I'm not clear on why requiring signed policies is helpful here. If you
> > > allow FUSE mounts at all then you need to trust the FUSE filesystem to
> > > return good results, in which case you can trust it to return valid
> > > hashes. If you don't trust the FUSE filesystem then generating the
> > > hash via read doesn't win you anything - the filesystem can return one
> > > set of data on the initial IMA hashing, and then return a second set
> > > later. Requiring signed policy doesn't change that.
> >
> > You're defining a new generic file ops "get_hash", but are using FUSE,
> > a specific filesystem, as an example.  Requiring the IMA policy to be
> > signed when using "get_hash", is proof of the sysadmin's agreement to
> > bypass actually reading and calculating the file hash.
> 
> We can trust in-kernel filesystems to return reliable information.
> Network filesystems have the same issue as FUSE - we're trusting that
> the remote endpoint won't give us different information on successive
> reads. What's the threat that's blocked by requiring signed policy
> here?

Today, IMA calculates the file hash by reading the file.  If
"get_hash" is a generic filesystem ops, then any filesystem could
implement it, properly or not.  sysadmins shouldn't have to review
kernel code to understand the source of the file hash, but should be
able to assume that unless they explicitly authorize "get_hash" usage,
IMA reads the file and calculates the file hash.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-05 19:51                       ` Mimi Zohar
@ 2019-03-05 20:27                         ` Matthew Garrett
  2019-03-06 12:30                           ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-05 20:27 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Tue, Mar 5, 2019 at 11:51 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Tue, 2019-03-05 at 10:39 -0800, Matthew Garrett wrote:
> > We can trust in-kernel filesystems to return reliable information.
> > Network filesystems have the same issue as FUSE - we're trusting that
> > the remote endpoint won't give us different information on successive
> > reads. What's the threat that's blocked by requiring signed policy
> > here?
>
> Today, IMA calculates the file hash by reading the file.  If
> "get_hash" is a generic filesystem ops, then any filesystem could
> implement it, properly or not.  sysadmins shouldn't have to review
> kernel code to understand the source of the file hash, but should be
> able to assume that unless they explicitly authorize "get_hash" usage,
> IMA reads the file and calculates the file hash.

But what's the threat? If an attacker is in a position to inject
additional IMA policy then in general they're already in a position to
violate other security assumptions. Admins who have a threat model
that includes an attacker being able to do this are already requiring
signed policy. What's the threat that requiring signed policy for this
specific option mitigates?

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-05 20:27                         ` Matthew Garrett
@ 2019-03-06 12:30                           ` Mimi Zohar
  2019-03-06 18:31                             ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-03-06 12:30 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Tue, 2019-03-05 at 12:27 -0800, Matthew Garrett wrote:
> On Tue, Mar 5, 2019 at 11:51 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Tue, 2019-03-05 at 10:39 -0800, Matthew Garrett wrote:
> > > We can trust in-kernel filesystems to return reliable information.
> > > Network filesystems have the same issue as FUSE - we're trusting that
> > > the remote endpoint won't give us different information on successive
> > > reads. What's the threat that's blocked by requiring signed policy
> > > here?
> >
> > Today, IMA calculates the file hash by reading the file.  If
> > "get_hash" is a generic filesystem ops, then any filesystem could
> > implement it, properly or not.  sysadmins shouldn't have to review
> > kernel code to understand the source of the file hash, but should be
> > able to assume that unless they explicitly authorize "get_hash" usage,
> > IMA reads the file and calculates the file hash.
> 
> But what's the threat? If an attacker is in a position to inject
> additional IMA policy then in general they're already in a position to
> violate other security assumptions. Admins who have a threat model
> that includes an attacker being able to do this are already requiring
> signed policy. What's the threat that requiring signed policy for this
> specific option mitigates?

That might be true, but this "feature" isn't a minor change.  It
totally changes the IMA measurement list meaning, without any
indication of the change in meaning.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-06 12:30                           ` Mimi Zohar
@ 2019-03-06 18:31                             ` Matthew Garrett
  2019-03-06 22:38                               ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-06 18:31 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Wed, Mar 6, 2019 at 4:30 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2019-03-05 at 12:27 -0800, Matthew Garrett wrote:
> > But what's the threat? If an attacker is in a position to inject
> > additional IMA policy then in general they're already in a position to
> > violate other security assumptions. Admins who have a threat model
> > that includes an attacker being able to do this are already requiring
> > signed policy. What's the threat that requiring signed policy for this
> > specific option mitigates?
>
> That might be true, but this "feature" isn't a minor change.  It
> totally changes the IMA measurement list meaning, without any
> indication of the change in meaning.

Ok. Would annotating the audit message to indicate that the hash was
provided directly by the filesystem be sufficient? I'm not clear on
why an admin would set this flag without having read the documentation
for it - like many security features, enabling an inappropriate
combination of them may result in bad things happening. I'm not keen
on tying it to signing because:

a) There are multiple configurations where requiring signed policy
doesn't give a security benefit - if the IMA policy is part of a
verified or measured initramfs, we already have integrity guarantees
and adding an additional layer of signing doesn't win us anything (eg,
in this configuration the IMA key may be loaded from the initramfs as
well, so an attacker able to modify policy could add an additional
signing key).
b) Users who are already using signed policy won't get the additional
hint that you think is necessary.

I'm happy to add this if there's a real threat model around it, but
requiring signing for something other than security reasons seems like
it's conflating unrelated issues.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-06 18:31                             ` Matthew Garrett
@ 2019-03-06 22:38                               ` Mimi Zohar
  2019-03-06 23:36                                 ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-03-06 22:38 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Wed, 2019-03-06 at 10:31 -0800, Matthew Garrett wrote:
> On Wed, Mar 6, 2019 at 4:30 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2019-03-05 at 12:27 -0800, Matthew Garrett wrote:
> > > But what's the threat? If an attacker is in a position to inject
> > > additional IMA policy then in general they're already in a position to
> > > violate other security assumptions. Admins who have a threat model
> > > that includes an attacker being able to do this are already requiring
> > > signed policy. What's the threat that requiring signed policy for this
> > > specific option mitigates?
> >
> > That might be true, but this "feature" isn't a minor change.  It
> > totally changes the IMA measurement list meaning, without any
> > indication of the change in meaning.
> 
> Ok. Would annotating the audit message to indicate that the hash was
> provided directly by the filesystem be sufficient? 

The audit log doesn't have the same security properties as the TPM
quote and IMA measurement list.  Nor does the attestation server
necessarily have access to it.

> I'm not clear on
> why an admin would set this flag without having read the documentation
> for it - like many security features, enabling an inappropriate
> combination of them may result in bad things happening.  I'm not keen
> on tying it to signing because:
> 
> a) There are multiple configurations where requiring signed policy
> doesn't give a security benefit - if the IMA policy is part of a
> verified or measured initramfs, we already have integrity guarantees
> and adding an additional layer of signing doesn't win us anything (eg,
> in this configuration the IMA key may be loaded from the initramfs as
> well, so an attacker able to modify policy could add an additional
> signing key).

Agreed, as long as there is no possibility of additional files being
installed/downloaded to the rootfs or files on other filesystems being
accessed before the IMA keyring is locked or a custom policy is
installed, a verified or measured initramfs might be enough.

This implies that both the custom policy and the keys loaded onto the
IMA keyring are included in the initramfs, which isn't what is
currently being done today.  My preference would be to remove the
original method of loading unsigned IMA policies, but that could/would
break existing systems.

> b) Users who are already using signed policy won't get the additional
> hint that you think is necessary.

But they would have to knowingly add "get_hash" to the IMA policy and
have signed it.

> I'm happy to add this if there's a real threat model around it, but
> requiring signing for something other than security reasons seems like
> it's conflating unrelated issues.

A colleague said, relying on the filesystem to provide the file hash
extends the TCB to include filesystems.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-06 22:38                               ` Mimi Zohar
@ 2019-03-06 23:36                                 ` Matthew Garrett
  2019-03-07  1:54                                   ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-06 23:36 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Wed, Mar 6, 2019 at 2:39 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2019-03-06 at 10:31 -0800, Matthew Garrett wrote:
> > Ok. Would annotating the audit message to indicate that the hash was
> > provided directly by the filesystem be sufficient?
>
> The audit log doesn't have the same security properties as the TPM
> quote and IMA measurement list.  Nor does the attestation server
> necessarily have access to it.

Ok.

> > I'm not clear on
> > why an admin would set this flag without having read the documentation
> > for it - like many security features, enabling an inappropriate
> > combination of them may result in bad things happening.  I'm not keen
> > on tying it to signing because:
> >
> > a) There are multiple configurations where requiring signed policy
> > doesn't give a security benefit - if the IMA policy is part of a
> > verified or measured initramfs, we already have integrity guarantees
> > and adding an additional layer of signing doesn't win us anything (eg,
> > in this configuration the IMA key may be loaded from the initramfs as
> > well, so an attacker able to modify policy could add an additional
> > signing key).
>
> Agreed, as long as there is no possibility of additional files being
> installed/downloaded to the rootfs or files on other filesystems being
> accessed before the IMA keyring is locked or a custom policy is
> installed, a verified or measured initramfs might be enough.
>
> This implies that both the custom policy and the keys loaded onto the
> IMA keyring are included in the initramfs, which isn't what is
> currently being done today.  My preference would be to remove the
> original method of loading unsigned IMA policies, but that could/would
> break existing systems.

It's the way we handle IMA configuration - the policy is loaded and
further policy loads are disabled due to IMA_WRITE_POLICY being
default n. We're not currently making use of signing, but longer term
plan is for the keys to be loaded at the same time.

> > b) Users who are already using signed policy won't get the additional
> > hint that you think is necessary.
>
> But they would have to knowingly add "get_hash" to the IMA policy and
> have signed it.

But in the non-signed case they'd still need to knowingly add
"get_hash" to the IMA policy. Why does signing indicate stronger
understanding of policy? If my understanding of ima_match_policy()
correct, if there's already a measurement rule that applies to a
filesystem then adding an additional trust_vfs rule will be ignored,
so once the initial policy is loaded it's not possible for someone to
transition a filesystem from a full read to using the vfs call. IE, a
policy like:

measure
measure fsmagic=0x46555345 trust_vfs

is still going to perform the full measurement even on FUSE.

> > I'm happy to add this if there's a real threat model around it, but
> > requiring signing for something other than security reasons seems like
> > it's conflating unrelated issues.
>
> A colleague said, relying on the filesystem to provide the file hash
> extends the TCB to include filesystems.

The TCB already includes filesystems - IMA's measurements are only
accurate if the filesystem returns the same data on subsequent reads
(assuming i_version hasn't been updated). We assert that this is true,
but it the filesystem is outside the TCB then that assertion is
invalid.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-06 23:36                                 ` Matthew Garrett
@ 2019-03-07  1:54                                   ` Mimi Zohar
  2019-03-07  4:19                                     ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-03-07  1:54 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Wed, 2019-03-06 at 15:36 -0800, Matthew Garrett wrote:

> > But they would have to knowingly add "get_hash" to the IMA policy and
> > have signed it.
> 
> But in the non-signed case they'd still need to knowingly add
> "get_hash" to the IMA policy. Why does signing indicate stronger
> understanding of policy? 

Nobody is suggesting that signing the policy is a stronger indication
of understanding the policy.  Signing the policy simply limits which
policies may be loaded.

> If my understanding of ima_match_policy()
> correct, if there's already a measurement rule that applies to a
> filesystem then adding an additional trust_vfs rule will be ignored,
> so once the initial policy is loaded it's not possible for someone to
> transition a filesystem from a full read to using the vfs call. IE, a
> policy like:
> 
> measure
> measure fsmagic=0x46555345 trust_vfs
> 
> is still going to perform the full measurement even on FUSE.

This scenario assumes that a custom policy has already been loaded and
that there are default catchall rules.

I really do hope that anybody enabling support for loading multiple
policies requires those policies to be signed, like the builtin
appraise policy.

> 
> > > I'm happy to add this if there's a real threat model around it, but
> > > requiring signing for something other than security reasons seems like
> > > it's conflating unrelated issues.
> >
> > A colleague said, relying on the filesystem to provide the file hash
> > extends the TCB to include filesystems.
> 
> The TCB already includes filesystems - IMA's measurements are only
> accurate if the filesystem returns the same data on subsequent reads
> (assuming i_version hasn't been updated). We assert that this is true,
> but it the filesystem is outside the TCB then that assertion is
> invalid.

There is also a difference between trusting the filesystem "read" and
the filesystem "get_hash" implementation, that have yet to be written.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-07  1:54                                   ` Mimi Zohar
@ 2019-03-07  4:19                                     ` Matthew Garrett
  2019-03-07 20:48                                       ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-07  4:19 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Wed, Mar 6, 2019 at 5:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2019-03-06 at 15:36 -0800, Matthew Garrett wrote:
>
> > > But they would have to knowingly add "get_hash" to the IMA policy and
> > > have signed it.
> >
> > But in the non-signed case they'd still need to knowingly add
> > "get_hash" to the IMA policy. Why does signing indicate stronger
> > understanding of policy?
>
> Nobody is suggesting that signing the policy is a stronger indication
> of understanding the policy.  Signing the policy simply limits which
> policies may be loaded.

I'm not sure I understand the additional risk, though. Either a
filesystem is already being measured using a full read, in which case
adding an additional rule won't change behaviour, or it's not being
measured at all, in which case there's no incentive for an attacker to
add a new rule.

> > > > I'm happy to add this if there's a real threat model around it, but
> > > > requiring signing for something other than security reasons seems like
> > > > it's conflating unrelated issues.
> > >
> > > A colleague said, relying on the filesystem to provide the file hash
> > > extends the TCB to include filesystems.
> >
> > The TCB already includes filesystems - IMA's measurements are only
> > accurate if the filesystem returns the same data on subsequent reads
> > (assuming i_version hasn't been updated). We assert that this is true,
> > but it the filesystem is outside the TCB then that assertion is
> > invalid.
>
> There is also a difference between trusting the filesystem "read" and
> the filesystem "get_hash" implementation, that have yet to be written.

In both cases we're placing trust in the filesystem's correctness.
It's certainly possible for the get_hash call to be broken, but that's
something we can put additional testing into. The same argument
implies that enabling appraisal is increasing the amount of trust we
place in the filesystem - without it we don't need getxattr() to work,
and without digital signatures we don't need the kernel's RSA code to
work. Most new features result in us relying on more code, and in some
cases we're not even able to audit that code (eg, if you're not using
an encrypted filesystem then we're depending on the correctness of
your disk's firmware, and we know that there are implementations that
don't verify updates or which provide debug commands that allow
modification at runtime in a way that could be used to defeat IMA)

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-07  4:19                                     ` Matthew Garrett
@ 2019-03-07 20:48                                       ` Mimi Zohar
  2019-03-07 22:41                                         ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-03-07 20:48 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Wed, 2019-03-06 at 20:19 -0800, Matthew Garrett wrote:
> On Wed, Mar 6, 2019 at 5:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Wed, 2019-03-06 at 15:36 -0800, Matthew Garrett wrote:
> >
> > > > But they would have to knowingly add "get_hash" to the IMA policy and
> > > > have signed it.
> > >
> > > But in the non-signed case they'd still need to knowingly add
> > > "get_hash" to the IMA policy. Why does signing indicate stronger
> > > understanding of policy?
> >
> > Nobody is suggesting that signing the policy is a stronger indication
> > of understanding the policy.  Signing the policy simply limits which
> > policies may be loaded.
> 
> I'm not sure I understand the additional risk, though. Either a
> filesystem is already being measured using a full read, in which case
> adding an additional rule won't change behaviour, or it's not being
> measured at all, in which case there's no incentive for an attacker to
> add a new rule.

In an environment where the initramfs contains everything, including
the IMA policy, and is verified, then what you're saying is true - the
IMA policy doesn't need to be signed.  However, the existing dracut
and systemd IMA modules read the IMA policy not from the initramfs,
but from real root.  In this environment where the IMA policy is on
real root, an attacker could modify the IMA policy.  I realize this is
an existing problem, not particular to "get_hash'.  Unlike other
policy changes, the attestation server would have no way of detecting
this particular change.

> > There is also a difference between trusting the filesystem "read" and
> > the filesystem "get_hash" implementation, that have yet to be written.
> 
> In both cases we're placing trust in the filesystem's correctness.
> It's certainly possible for the get_hash call to be broken, but that's
> something we can put additional testing into.

The call itself wouldn't be broken, but determining if the filesystem
is actually calculating/re-calculating the file hash properly or
simply returning a stored/cached value.  I do see the benefit of the
filesystems being responsible for the file hash.  Perhaps I'm just
being overly cautious.  I'd like to hear other people's opinions.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-07 20:48                                       ` Mimi Zohar
@ 2019-03-07 22:41                                         ` Matthew Garrett
  2019-04-04 21:46                                           ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-03-07 22:41 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Mar 7, 2019 at 12:48 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2019-03-06 at 20:19 -0800, Matthew Garrett wrote:
> > I'm not sure I understand the additional risk, though. Either a
> > filesystem is already being measured using a full read, in which case
> > adding an additional rule won't change behaviour, or it's not being
> > measured at all, in which case there's no incentive for an attacker to
> > add a new rule.
>
> In an environment where the initramfs contains everything, including
> the IMA policy, and is verified, then what you're saying is true - the
> IMA policy doesn't need to be signed.  However, the existing dracut
> and systemd IMA modules read the IMA policy not from the initramfs,
> but from real root.  In this environment where the IMA policy is on
> real root, an attacker could modify the IMA policy.  I realize this is
> an existing problem, not particular to "get_hash'.  Unlike other
> policy changes, the attestation server would have no way of detecting
> this particular change.

Ah, got you. Would measuring the policy be sufficient here?

> > In both cases we're placing trust in the filesystem's correctness.
> > It's certainly possible for the get_hash call to be broken, but that's
> > something we can put additional testing into.
>
> The call itself wouldn't be broken, but determining if the filesystem
> is actually calculating/re-calculating the file hash properly or
> simply returning a stored/cached value.  I do see the benefit of the
> filesystems being responsible for the file hash.  Perhaps I'm just
> being overly cautious.  I'd like to hear other people's opinions.

Yup, happy to get further feedback on this.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-03-07 22:41                                         ` Matthew Garrett
@ 2019-04-04 21:46                                           ` Matthew Garrett
  2019-04-04 22:18                                             ` James Bottomley
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-04-04 21:46 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Mar 7, 2019 at 2:41 PM Matthew Garrett <mjg59@google.com> wrote:
> Yup, happy to get further feedback on this.

Anyone other than me and Mimi with thoughts here? :)

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-04 21:46                                           ` Matthew Garrett
@ 2019-04-04 22:18                                             ` James Bottomley
  2019-04-04 22:26                                               ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: James Bottomley @ 2019-04-04 22:18 UTC (permalink / raw)
  To: Matthew Garrett, Mimi Zohar
  Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, 2019-04-04 at 14:46 -0700, Matthew Garrett wrote:
> On Thu, Mar 7, 2019 at 2:41 PM Matthew Garrett <mjg59@google.com>
> wrote:
> > Yup, happy to get further feedback on this.
> 
> Anyone other than me and Mimi with thoughts here? :)

The obvious other thought is integration with fs-verity, which is a
filesystem maintained possibly signed merkel tree hash.  The problem
here is what does vfs_get_hash() actually mean?  The assumption seems
to be that it is the flat hash of the entire file which doesn't work
for merkle trees.  However, if it could be a representative hash of the
file which is produced however the filesystem decides, it could work
(well, unless the file is copied on to a different fs, of course ...).

James


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-04 22:18                                             ` James Bottomley
@ 2019-04-04 22:26                                               ` Matthew Garrett
  2019-04-04 22:35                                                 ` James Bottomley
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-04-04 22:26 UTC (permalink / raw)
  To: James Bottomley
  Cc: Mimi Zohar, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Apr 4, 2019 at 3:18 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> The obvious other thought is integration with fs-verity, which is a
> filesystem maintained possibly signed merkel tree hash.  The problem
> here is what does vfs_get_hash() actually mean?  The assumption seems
> to be that it is the flat hash of the entire file which doesn't work
> for merkle trees.  However, if it could be a representative hash of the
> file which is produced however the filesystem decides, it could work
> (well, unless the file is copied on to a different fs, of course ...).

We could always use fs-verity to store additional verifiable metadata
including actual hashes for consistency?

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-04 22:26                                               ` Matthew Garrett
@ 2019-04-04 22:35                                                 ` James Bottomley
  2019-04-05  1:50                                                   ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: James Bottomley @ 2019-04-04 22:35 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: Mimi Zohar, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, 2019-04-04 at 15:26 -0700, Matthew Garrett wrote:
> On Thu, Apr 4, 2019 at 3:18 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > The obvious other thought is integration with fs-verity, which is a
> > filesystem maintained possibly signed merkel tree hash.  The
> > problem here is what does vfs_get_hash() actually mean?  The
> > assumption seems to be that it is the flat hash of the entire file
> > which doesn't work for merkle trees.  However, if it could be a
> > representative hash of the file which is produced however the
> > filesystem decides, it could work (well, unless the file is copied
> > on to a different fs, of course ...).
> 
> We could always use fs-verity to store additional verifiable metadata
> including actual hashes for consistency?

Redundant information is always possible, but it can become
inconsistent and, because the hashes can't be derived from each other,
it's hard to tell if it is inconsistent without redoing the whole hash
with each method.

I was more wondering what, if any, problems would follow if we did let
the filesystem choose the hash method and simply used the top merkle
hash in place of the usual IMA hash?

James


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-04 22:35                                                 ` James Bottomley
@ 2019-04-05  1:50                                                   ` Matthew Garrett
  2019-04-05  2:26                                                     ` James Bottomley
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-04-05  1:50 UTC (permalink / raw)
  To: James Bottomley
  Cc: Mimi Zohar, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Apr 4, 2019 at 3:35 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> Redundant information is always possible, but it can become
> inconsistent and, because the hashes can't be derived from each other,
> it's hard to tell if it is inconsistent without redoing the whole hash
> with each method.

Part of the problem here is that IMA is effectively used for two
related but different purposes - measurement and appraisal. You
generally want measurements to be comparable across filesystems,
whereas appraisal doesn't need to be. So if we don't have comparable
measurements, there's less benefit in performing measurement (we have
no real idea what the expected measurements would be in advance).
That's less important for appraisal, but arguably we don't care about
appraisal of stuff on fs-verity backed filesystems to begin with
because we can just attest that they're legitimate?

> I was more wondering what, if any, problems would follow if we did let
> the filesystem choose the hash method and simply used the top merkle
> hash in place of the usual IMA hash?

We could definitely just pass it through as a separate hash type, and
my initial thinking was that fs-verity might be a reasonable use case
for that, but I'm not sure that it buys us much in the IMA case.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-05  1:50                                                   ` Matthew Garrett
@ 2019-04-05  2:26                                                     ` James Bottomley
  2019-04-05 20:55                                                       ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: James Bottomley @ 2019-04-05  2:26 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: Mimi Zohar, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, 2019-04-04 at 18:50 -0700, Matthew Garrett wrote:
> On Thu, Apr 4, 2019 at 3:35 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > Redundant information is always possible, but it can become
> > inconsistent and, because the hashes can't be derived from each
> > other, it's hard to tell if it is inconsistent without redoing the
> > whole hash with each method.
> 
> Part of the problem here is that IMA is effectively used for two
> related but different purposes - measurement and appraisal. You
> generally want measurements to be comparable across filesystems,
> whereas appraisal doesn't need to be.

Sure, but I think the only requirement for measurement is knowing how
to reproduce them.  As long as you know the algorithm the filesystem is
using ... i.e. it's recorded in the IMA log, you should be able to
verify them.

>  So if we don't have comparable measurements, there's less benefit in
> performing measurement (we have no real idea what the expected
> measurements would be in advance).

As long as the algorithm used for the measurement is recorded, I don't
think there's a problem.  The IMA log currently records the hash
algorithm and the actual hash, so if we take shaX to be a flat hash, we
could use shaX-merkle for fs-verity and everything would work.

> That's less important for appraisal, but arguably we don't care about
> appraisal of stuff on fs-verity backed filesystems to begin with
> because we can just attest that they're legitimate?

I think Ted mentioned they did like to sign the merkle tree to prove
the apk being installed was legitimate, so I think both measurement and
appraisal are relevant.

> > I was more wondering what, if any, problems would follow if we did
> > let the filesystem choose the hash method and simply used the top
> > merkle hash in place of the usual IMA hash?
> 
> We could definitely just pass it through as a separate hash type, and
> my initial thinking was that fs-verity might be a reasonable use case
> for that, but I'm not sure that it buys us much in the IMA case.

Unifying the interfaces for measurement and appraisal sounds like a
desirable thing.  IMA has just been debating measurement on mmap and
the per-page hashes of fs-verity seem to be tailor made for this.

Note: I'm not insisting on this, you just asked for other feedback and
I think it's a useful discussion.

James


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-05  2:26                                                     ` James Bottomley
@ 2019-04-05 20:55                                                       ` Matthew Garrett
  2019-04-29 22:51                                                         ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-04-05 20:55 UTC (permalink / raw)
  To: James Bottomley
  Cc: Mimi Zohar, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Thu, Apr 4, 2019 at 7:27 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Thu, 2019-04-04 at 18:50 -0700, Matthew Garrett wrote:
> > On Thu, Apr 4, 2019 at 3:35 PM James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > Redundant information is always possible, but it can become
> > > inconsistent and, because the hashes can't be derived from each
> > > other, it's hard to tell if it is inconsistent without redoing the
> > > whole hash with each method.
> >
> > Part of the problem here is that IMA is effectively used for two
> > related but different purposes - measurement and appraisal. You
> > generally want measurements to be comparable across filesystems,
> > whereas appraisal doesn't need to be.
>
> Sure, but I think the only requirement for measurement is knowing how
> to reproduce them.  As long as you know the algorithm the filesystem is
> using ... i.e. it's recorded in the IMA log, you should be able to
> verify them.

Mm. I think this is use-case dependent, but there are certainly use
cases where this would be sufficient. I think this would work on the
VFS side, but we'd need to extend IMA to allow you to write a policy
that specified the use of the fs-verity data on the appropriate
filesystems (right now IMA uses one hash type globally) - if anyone's
interested in deploying that, I'm happy to add support for it.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-05 20:55                                                       ` Matthew Garrett
@ 2019-04-29 22:51                                                         ` Matthew Garrett
  2019-05-02 20:25                                                           ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-04-29 22:51 UTC (permalink / raw)
  To: James Bottomley
  Cc: Mimi Zohar, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

Mimi, anything else I can do here?

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-04-29 22:51                                                         ` Matthew Garrett
@ 2019-05-02 20:25                                                           ` Mimi Zohar
  2019-05-02 22:37                                                             ` Matthew Garrett
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-05-02 20:25 UTC (permalink / raw)
  To: Matthew Garrett, James Bottomley
  Cc: linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos, Roberto Sassu

[Cc'ing Roberto]

Hi Matthew,

On Mon, 2019-04-29 at 15:51 -0700, Matthew Garrett wrote:
> Mimi, anything else I can do here?

Trying to remember where we were ...  The last issue, as I recall, is
somehow annotating the measurement list to indicate the source of the
file hash.

One solution might be:

Suppose instead of re-using the "d-ng" for the vfs hash, you defined a
new field named d-vfs.  Instead of the "ima-ng" or "d-ng|n-ng", the
template name could be "d-vfs|n-ng".

Intermixing of template formats is not a problem.  IMA already
supports multiple templates in the same list for carrying the
measurement list across kexec.  (There are no guarantees that the
current measurement list and the kexec'ed kernel will be the same
template format.)  The template format is currently defined at compile
time, with a run time option of changing it.

The issue then becomes how to dynamically switch between template formats, based on fields.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-05-02 20:25                                                           ` Mimi Zohar
@ 2019-05-02 22:37                                                             ` Matthew Garrett
  2019-05-02 23:02                                                               ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Garrett @ 2019-05-02 22:37 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: James Bottomley, linux-integrity, Dmitry Kasatkin, linux-fsdevel,
	miklos, Roberto Sassu

On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> Suppose instead of re-using the "d-ng" for the vfs hash, you defined a
> new field named d-vfs.  Instead of the "ima-ng" or "d-ng|n-ng", the
> template name could be "d-vfs|n-ng".

Is it legitimate to redefine d-ng such that if the hash comes from the
filesystem it adds an additional prefix? This will only occur if the
admin has explicitly enabled the trusted_vfs option, so we wouldn't
break any existing configurations. Otherwise, I'll look for the
cleanest approach for making this dynamic.

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-05-02 22:37                                                             ` Matthew Garrett
@ 2019-05-02 23:02                                                               ` Mimi Zohar
  2019-05-03  6:51                                                                 ` Roberto Sassu
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-05-02 23:02 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: James Bottomley, linux-integrity, Dmitry Kasatkin, linux-fsdevel,
	miklos, Roberto Sassu

On Thu, 2019-05-02 at 15:37 -0700, Matthew Garrett wrote:
> On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > Suppose instead of re-using the "d-ng" for the vfs hash, you defined a
> > new field named d-vfs.  Instead of the "ima-ng" or "d-ng|n-ng", the
> > template name could be "d-vfs|n-ng".
> 
> Is it legitimate to redefine d-ng such that if the hash comes from the
> filesystem it adds an additional prefix? This will only occur if the
> admin has explicitly enabled the trusted_vfs option, so we wouldn't
> break any existing configurations. Otherwise, I'll look for the
> cleanest approach for making this dynamic.

I would assume modifying d-ng would break existing attestation
servers.

Perhaps instead of making the template format dynamic based on fields,
as I suggested above, define a per policy rule template format option.

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-05-02 23:02                                                               ` Mimi Zohar
@ 2019-05-03  6:51                                                                 ` Roberto Sassu
  2019-05-03  8:17                                                                   ` Roberto Sassu
  0 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2019-05-03  6:51 UTC (permalink / raw)
  To: Mimi Zohar, Matthew Garrett
  Cc: James Bottomley, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On 5/3/2019 1:02 AM, Mimi Zohar wrote:
> On Thu, 2019-05-02 at 15:37 -0700, Matthew Garrett wrote:
>> On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> Suppose instead of re-using the "d-ng" for the vfs hash, you defined a
>>> new field named d-vfs.  Instead of the "ima-ng" or "d-ng|n-ng", the
>>> template name could be "d-vfs|n-ng".
>>
>> Is it legitimate to redefine d-ng such that if the hash comes from the
>> filesystem it adds an additional prefix? This will only occur if the
>> admin has explicitly enabled the trusted_vfs option, so we wouldn't
>> break any existing configurations. Otherwise, I'll look for the
>> cleanest approach for making this dynamic.
> 
> I would assume modifying d-ng would break existing attestation
> servers.

Yes, I would also prefer to avoid modification of d-ng.


> Perhaps instead of making the template format dynamic based on fields,
> as I suggested above, define a per policy rule template format option.

This should not be too complicated. The template to use will be returned
by ima_get_action() to process_measurement().

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-05-03  6:51                                                                 ` Roberto Sassu
@ 2019-05-03  8:17                                                                   ` Roberto Sassu
  2019-05-03 12:47                                                                     ` Mimi Zohar
  0 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2019-05-03  8:17 UTC (permalink / raw)
  To: Mimi Zohar, Matthew Garrett
  Cc: James Bottomley, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On 5/3/2019 8:51 AM, Roberto Sassu wrote:
> On 5/3/2019 1:02 AM, Mimi Zohar wrote:
>> On Thu, 2019-05-02 at 15:37 -0700, Matthew Garrett wrote:
>>> On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>> Suppose instead of re-using the "d-ng" for the vfs hash, you defined a
>>>> new field named d-vfs.  Instead of the "ima-ng" or "d-ng|n-ng", the
>>>> template name could be "d-vfs|n-ng".
>>>
>>> Is it legitimate to redefine d-ng such that if the hash comes from the
>>> filesystem it adds an additional prefix? This will only occur if the
>>> admin has explicitly enabled the trusted_vfs option, so we wouldn't
>>> break any existing configurations. Otherwise, I'll look for the
>>> cleanest approach for making this dynamic.
>>
>> I would assume modifying d-ng would break existing attestation
>> servers.
> 
> Yes, I would also prefer to avoid modification of d-ng.
> 
> 
>> Perhaps instead of making the template format dynamic based on fields,
>> as I suggested above, define a per policy rule template format option.
> 
> This should not be too complicated. The template to use will be returned
> by ima_get_action() to process_measurement().

Some time ago I made some patches:

https://sourceforge.net/p/linux-ima/mailman/message/31655784/

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-05-03  8:17                                                                   ` Roberto Sassu
@ 2019-05-03 12:47                                                                     ` Mimi Zohar
  2019-05-03 13:20                                                                       ` Roberto Sassu
  0 siblings, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2019-05-03 12:47 UTC (permalink / raw)
  To: Roberto Sassu, Matthew Garrett, Thiago Jung Bauermann
  Cc: James Bottomley, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On Fri, 2019-05-03 at 10:17 +0200, Roberto Sassu wrote:
> On 5/3/2019 8:51 AM, Roberto Sassu wrote:
> > On 5/3/2019 1:02 AM, Mimi Zohar wrote:

> >> Perhaps instead of making the template format dynamic based on fields,
> >> as I suggested above, define a per policy rule template format option.
> > 
> > This should not be too complicated. The template to use will be returned
> > by ima_get_action() to process_measurement().
> 
> Some time ago I made some patches:
> 
> https://sourceforge.net/p/linux-ima/mailman/message/31655784/

Thank you for the reference!  In addition to Matthew's VFS hash use
case, Thiago's appended signature support would benefit from a per
policy rule template.  Do you want, and have time, to add this
support?

Mimi


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

* Re: [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes
  2019-05-03 12:47                                                                     ` Mimi Zohar
@ 2019-05-03 13:20                                                                       ` Roberto Sassu
  0 siblings, 0 replies; 41+ messages in thread
From: Roberto Sassu @ 2019-05-03 13:20 UTC (permalink / raw)
  To: Mimi Zohar, Matthew Garrett, Thiago Jung Bauermann
  Cc: James Bottomley, linux-integrity, Dmitry Kasatkin, linux-fsdevel, miklos

On 5/3/2019 2:47 PM, Mimi Zohar wrote:
> On Fri, 2019-05-03 at 10:17 +0200, Roberto Sassu wrote:
>> On 5/3/2019 8:51 AM, Roberto Sassu wrote:
>>> On 5/3/2019 1:02 AM, Mimi Zohar wrote:
> 
>>>> Perhaps instead of making the template format dynamic based on fields,
>>>> as I suggested above, define a per policy rule template format option.
>>>
>>> This should not be too complicated. The template to use will be returned
>>> by ima_get_action() to process_measurement().
>>
>> Some time ago I made some patches:
>>
>> https://sourceforge.net/p/linux-ima/mailman/message/31655784/
> 
> Thank you for the reference!  In addition to Matthew's VFS hash use
> case, Thiago's appended signature support would benefit from a per
> policy rule template.  Do you want, and have time, to add this
> support?

No problem. At the moment I'm busy with the digest lists patch set. I
will see if I have time later.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

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

end of thread, other threads:[~2019-05-03 13:20 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-26 21:50 Allow trusted filesystems to provide IMA hashes directly Matthew Garrett
2019-02-26 21:50 ` [PATCH V2 1/4] VFS: Add a call to obtain a file's hash Matthew Garrett
2019-02-26 21:50 ` [PATCH V2 2/4] IMA: Allow rule matching on filesystem subtype Matthew Garrett
2019-02-26 21:50 ` [PATCH V2 3/4] IMA: Optionally make use of filesystem-provided hashes Matthew Garrett
2019-02-28 16:03   ` Mimi Zohar
2019-02-28 18:05     ` Mimi Zohar
2019-02-28 21:41       ` Matthew Garrett
2019-02-28 21:59         ` Mimi Zohar
2019-02-28 22:38           ` Matthew Garrett
2019-03-04 19:52             ` Matthew Garrett
2019-03-04 20:32               ` Mimi Zohar
2019-03-04 22:10                 ` Matthew Garrett
2019-03-05 13:18                   ` Mimi Zohar
2019-03-05 18:39                     ` Matthew Garrett
2019-03-05 19:51                       ` Mimi Zohar
2019-03-05 20:27                         ` Matthew Garrett
2019-03-06 12:30                           ` Mimi Zohar
2019-03-06 18:31                             ` Matthew Garrett
2019-03-06 22:38                               ` Mimi Zohar
2019-03-06 23:36                                 ` Matthew Garrett
2019-03-07  1:54                                   ` Mimi Zohar
2019-03-07  4:19                                     ` Matthew Garrett
2019-03-07 20:48                                       ` Mimi Zohar
2019-03-07 22:41                                         ` Matthew Garrett
2019-04-04 21:46                                           ` Matthew Garrett
2019-04-04 22:18                                             ` James Bottomley
2019-04-04 22:26                                               ` Matthew Garrett
2019-04-04 22:35                                                 ` James Bottomley
2019-04-05  1:50                                                   ` Matthew Garrett
2019-04-05  2:26                                                     ` James Bottomley
2019-04-05 20:55                                                       ` Matthew Garrett
2019-04-29 22:51                                                         ` Matthew Garrett
2019-05-02 20:25                                                           ` Mimi Zohar
2019-05-02 22:37                                                             ` Matthew Garrett
2019-05-02 23:02                                                               ` Mimi Zohar
2019-05-03  6:51                                                                 ` Roberto Sassu
2019-05-03  8:17                                                                   ` Roberto Sassu
2019-05-03 12:47                                                                     ` Mimi Zohar
2019-05-03 13:20                                                                       ` Roberto Sassu
2019-02-26 21:50 ` [PATCH V2 4/4] FUSE: Allow filesystems to provide gethash methods Matthew Garrett
2019-02-27 14:26   ` Jann Horn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).