Skip to contentSkip to conversation

ima: define a new policy option named "force"

PATCH In review Mimi Zohar 3 participants 4 messages Dec 8, 2017 – Dec 11, 2017

Subsystems (4)
lore
Share & tools
lore

Mimi Zohar

author
#0 lore raw

PATCH 1/1 ima: define a new policy option named "force"

There are times instead of relying on previously cached status information we want to force the file to be re-measured, re-appraised, and re-audited.

This patch defines a new policy option named "force", which forces files to be re-measured, re-appraised or re-audited.

---
 Documentation/ABI/testing/ima_policy |  2 +-
 security/integrity/ima/ima_main.c    | 22 ++++++++++++++++++++--
 security/integrity/ima/ima_policy.c  |  8 +++++++-
 security/integrity/integrity.h       |  1 +
 4 files changed, 29 insertions(+), 4 deletions(-)
Documentation/ABI/testing/ima_policy +1 -1
index 2028f2d093b2..b0e8143c681f 100644
@@ -24,7 +24,7 @@ Description:
 				[euid=] [fowner=]]
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
-			option:	[[appraise_type=]] [permit_directio]
+			option:	[[appraise_type=]] [permit_directio] [force]
 
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
security/integrity/ima/ima_main.c +20 -2
index 4dce3626dd4d..2a483184bc9a 100644
@@ -228,9 +228,27 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
 				 IMA_ACTION_FLAGS);
 
-	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags))
-		/* reset all flags if ima_inode_setxattr was called */
+	/*
+	 * Reset the measure, appraise and audit cached flags either if
+	 * ima_inode_setxattr was called or based on policy, forcing
+	 * the file to be re-evaluated.
+	 */
+	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) {
 		iint->flags &= ~IMA_DONE_MASK;
+	} else if (action & IMA_FORCE) {
+		if (action & IMA_MEASURE) {
+			iint->measured_pcrs = 0;
+			iint->flags &=
+			    ~(IMA_COLLECTED | IMA_MEASURE | IMA_MEASURED);
+		}
+		if (action & IMA_APPRAISE)
+			iint->flags &=
+			    ~(IMA_COLLECTED | IMA_APPRAISE | IMA_APPRAISED |
+			      IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK);
+		if (action & IMA_AUDIT)
+			iint->flags &=
+			    ~(IMA_COLLECTED | IMA_AUDIT | IMA_AUDITED);
+	}
 
 	/* Determine if already appraised/measured based on bitmask
 	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
security/integrity/ima/ima_policy.c +7 -1
index 93dcf1bf92a8..878ae1a06e1e 100644
@@ -533,7 +533,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_pcr, Opt_force
 };
 
 static match_table_t policy_tokens = {
@@ -566,6 +566,7 @@ static match_table_t policy_tokens = {
 	{Opt_appraise_type, "appraise_type=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
+	{Opt_force, "force"},
 	{Opt_err, NULL}
 };
 
@@ -895,6 +896,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->flags |= IMA_PCR;
 
 			break;
+		case Opt_force:
+			entry->flags |= IMA_FORCE;
+			break;
 		case Opt_err:
 			ima_log_string(ab, "UNKNOWN", p);
 			result = -EINVAL;
@@ -1168,6 +1172,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_FORCE)
+		seq_puts(m, "force ");
 	rcu_read_unlock();
 	seq_puts(m, "\n");
 	return 0;
security/integrity/integrity.h +1 -0
index 50a8e3365df7..4e16b1212d0f 100644
@@ -35,6 +35,7 @@
 #define IMA_PERMIT_DIRECTIO	0x02000000
 #define IMA_NEW_FILE		0x04000000
 #define EVM_IMMUTABLE_DIGSIG	0x08000000
+#define IMA_FORCE		0x10000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)
Signature · 1 line
2.7.4
#1 lore raw
Quoted 3 lines · Mimi Zohar

There are times instead of relying on previously cached status information we want to force the file to be re-measured, re-appraised, and re-audited.

Can you give an example of when this would be needed?

Signature · 2 lines
James Morris
<[email protected]>
author
#2 lore raw
Quoted 7 lines · James Morris
Quoted 3 lines · Mimi Zohar

There are times instead of relying on previously cached status information we want to force the file to be re-measured, re-appraised, and re-audited.

Can you give an example of when this would be needed?

Up to Sascha Hauer's patch "ima: Use i_version only when filesystem supports it", which is queued to be upstreamed, the cached flags are reset only if the i_version changed, causing the file to be re- evaluated. After that patch, the cached flags are also reset if i_version is not enabled.

That leaves the case where i_version is enabled for the filesystem, but the local kernel is not responsible for updating it. This patch is mainly for filesystems, where we can't trust the filesystem properly increments i_version.

Eric/Seth, with Sasha's patch is this patch still needed for fuse filesystems?

Mimi

#3 lore raw
Quoted 15 lines · Mimi Zohar
Quoted 7 lines · James Morris
Quoted 3 lines · Mimi Zohar

There are times instead of relying on previously cached status information we want to force the file to be re-measured, re-appraised, and re-audited.

Can you give an example of when this would be needed?

Up to Sascha Hauer's patch "ima: Use i_version only when filesystem supports it", which is queued to be upstreamed, the cached flags are reset only if the i_version changed, causing the file to be re- evaluated. After that patch, the cached flags are also reset if i_version is not enabled.

Quoted 7 lines

That leaves the case where i_version is enabled for the filesystem, but the local kernel is not responsible for updating it. This patch is mainly for filesystems, where we can't trust the filesystem properly increments i_version.

Eric/Seth, with Sasha's patch is this patch still needed for fuse filesystems?

I think so. With fuse the file data is being generated by a userspace process, so the concern is that the process could change the file data without the kernel's knowledge and IMA would still use the cached result. And fuse is often mounted by unprivileged users.

It looks like Sascha's patch only addresses the issue for file data changes that happen via the kernel and not for data which could change outside of the kernel's knowledge.

Seth