diff mbox series

[RFC,6/8] ima: Use digest cache for measurement

Message ID 20240214143525.2205481-7-roberto.sassu@huaweicloud.com (mailing list archive)
State New
Headers show
Series ima: Integrate with digest_cache LSM | expand

Commit Message

Roberto Sassu Feb. 14, 2024, 2:35 p.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

Introduce a new measurement style using digest caches, which can be
performed exclusively on non-standard PCRs, to avoid ambiguity.

While a measurement on the standard PCR means that a file was accessed and
had the measured content, a measurement with the digest cache means only
that the calculated file digest was not found in any of the measured digest
lists (any digest list used for the search must be measured, otherwise IMA
wouldn't use it).

The new measurement style does not tell: whether or not the file was
actually accessed (since its measurement is skipped even if it was); in
which sequence files were accessed. So, one has to guess that the system
might have possibly accessed any of the files whose digest is in the
measured digest lists, in any order.

However, it has the following benefits: the IMA measurement list can be
much shorter, system performance can be much better due to less PCR extend
operations (see the performance evaluation in the digest_cache LSM
documentation); the PCR can be predictable as long as the set of measured
digest lists does not change (which obviously happens during software
updates).

The PCR can be predictable because the digest_cache LSM has a prefetching
mechanism that reads digest lists in a deterministic order, until it
finds the digest list containing the digest calculated by IMA from an
accessed file. If IMA measures digest lists, the PCR is extended in a
deterministic order too.

Predictable PCR means that a TPM key can be made dependent on specific PCR
values (or a OR of them, depending on the key policy). Accessing a file
with an unknown digest immediately makes that TPM key unusable, requiring a
reboot to use it again.

This mechanism can be used for the so called implicit remote attestation,
where the ability of a system to respond to challenges based on the private
part of the TPM key means that the system has the expected PCR values
(which would mean that the integrity of the system is ok). This is opposed
to the explicit remote attestation, where a system has to send all its
measurements, to prove to a remote party about its integrity.

If the IMA policy allows the usage of the digest cache for the current file
access (except for DIGEST_LIST_CHECK hook, not supported), call
digest_cache_get() in process_measurement() to get a digest cache for that
file, and call digest_cache_lookup() to search the calculated file digest.

Doing the lookup is necessary to retrieve the digest cache containing the
digest, since digest_cache_get() might only return a directory digest
cache, useful only to iterate over the digest caches of the directory
entries.

If digest_cache_lookup() returns a positive value (digest cache reference
in the digest_cache_found_t form), call digest_cache_from_found_t() to get
an usable digest cache pointer, and digest_cache_verif_get() to get
the verification result of the corresponding digest list, and AND it with
the policy mask.

Then, pass the AND result to ima_store_measurement() and, if the result has
the IMA_DIGEST_CACHE_MEASURE_CONTENT flag set, behave as if the file was
successfully added to the IMA measurement list (i.e. set the IMA_MEASURED
flag and the PCR flag from the value specified in the matching policy
rule), but actually don't do it.

Finally, release the digest cache reference acquired with
digest_cache_get(), by calling digest_cache_put().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h      |  3 ++-
 security/integrity/ima/ima_api.c  | 15 ++++++++++++++-
 security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++++++---
 3 files changed, 45 insertions(+), 5 deletions(-)

Comments

Mimi Zohar March 8, 2024, 4:08 p.m. UTC | #1 
Hi Roberto,

> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 3fc48214850a..48a09747ae7a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -222,7 +222,9 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	bool violation_check;
>  	enum hash_algo hash_algo;
>  	unsigned int allowed_algos = 0;
> -	u64 verif_mask = 0;
> +	u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
> +	struct digest_cache *digest_cache = NULL, *found_cache;
> +	digest_cache_found_t found;
>  
>  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>  		return 0;
> @@ -233,7 +235,7 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	 */
>  	action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
>  				mask, func, &pcr, &template_desc, NULL,
> -				&allowed_algos, NULL);
> +				&allowed_algos, &policy_mask);
>  	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
>  			    func == MMAP_CHECK_REQPROT) &&
>  			   (ima_policy_flag & IMA_MEASURE));
> @@ -364,10 +366,34 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
>  		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
>  
> +	/*
> +	 * For now we don't support nested verification with digest caches.

I haven't reviewed the digest_cache LSM patch set yet.  What does 'nested' mean
in this context?  Why mention it here?

> +	 * Since we allow IMA policy rules without func=, we have to enforce
> +	 * this restriction here.
> +	 */
> +	if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> +		digest_cache = digest_cache_get(file_dentry(file));

So whether or not a DIGEST_LIST_CHECK policy rule even exists,
digest_cache_get() will be called.  Similarly, even if a digest_cache list
hasn't been measured or appraised, digest_cache_get() will be called.

Basically every file in policy will check the digest_cache.

> +
> +	if (digest_cache) {
> +		found = digest_cache_lookup(file_dentry(file), digest_cache,
> +					    iint->ima_hash->digest,
> +					    iint->ima_hash->algo);
> +		/* AND what is allowed by the policy, and what IMA verified. */
> +		if (found) {
> +			found_cache = digest_cache_from_found_t(found);
> +			verif_mask_ptr = digest_cache_verif_get(found_cache,
> +								"ima");

Instead of using "verif_{set,get}' consider using '{set,get}_usage', where usage
here means measure or appraise.

> +			if (verif_mask_ptr)
> +				allow_mask = policy_mask & *verif_mask_ptr;
> +		}
> +
> +		digest_cache_put(digest_cache);
> +	}
> +

I'm wondering if it makes sense to create IMA wrappers for each of the
digest_cache functions - checking the digest_cache for the hash, setting the
digest_cache permitted usage, etc - and put all of them in a separate
ima_digest_cache.c file.  The file would only be included in the Makefile if
digest_cache is configured.

In this file you could define a static local global variable to detect whether
the digest_cache is ready to be used.  Only after successfully measuring and
appraising a digest_cache list, based on policy, set the variable.

>  	if (action & IMA_MEASURE)
>  		ima_store_measurement(iint, file, pathname,
>  				      xattr_value, xattr_len, modsig, pcr,
> -				      template_desc);
> +				      template_desc, allow_mask);

'allowed_usage'?

>  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
>  		rc = ima_check_blacklist(iint, modsig, pcr);
>  		if (rc != -EPERM) {

thanks,

Mimi
Roberto Sassu March 8, 2024, 4:27 p.m. UTC | #2 
On Fri, 2024-03-08 at 11:08 -0500, Mimi Zohar wrote:
> Hi Roberto,
> 
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 3fc48214850a..48a09747ae7a 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -222,7 +222,9 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  	bool violation_check;
> >  	enum hash_algo hash_algo;
> >  	unsigned int allowed_algos = 0;
> > -	u64 verif_mask = 0;
> > +	u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
> > +	struct digest_cache *digest_cache = NULL, *found_cache;
> > +	digest_cache_found_t found;
> >  
> >  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> >  		return 0;
> > @@ -233,7 +235,7 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  	 */
> >  	action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
> >  				mask, func, &pcr, &template_desc, NULL,
> > -				&allowed_algos, NULL);
> > +				&allowed_algos, &policy_mask);
> >  	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
> >  			    func == MMAP_CHECK_REQPROT) &&
> >  			   (ima_policy_flag & IMA_MEASURE));
> > @@ -364,10 +366,34 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
> >  		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> >  
> > +	/*
> > +	 * For now we don't support nested verification with digest caches.
> 
> I haven't reviewed the digest_cache LSM patch set yet.  What does 'nested' mean
> in this context?  Why mention it here?

This is the reason for the check func != DIGEST_LIST_CHECK. Before I
had this check in validate_rule(), but then realized that I would have
to reject rules without 'func='.

Not doing this check means that we could use digest caches also for
verifying digest lists (this is the meaning of nested verification).
This possibility is actually not remote, we would need this
functionality to verify Debian-based distributions.

Now, leaving aside the fact that Debian packages still use MD5 for
checksums (I heard that they would like to switch to SHA256 but didn't
verify if it happened), this would be the chain of verification:

/bin/cat -> md5sums -> coreutils (DEB pkg) -> Packages.gz -> Release

Release is the only file signed with PGP.

I already tried to verify a chain, it works, but I didn't want to
include too many functionality at the beginning. DEB format is not yet
supported, for RPMs chained verification it is not needed.

It would be fantastic if md5sums (or sha256sums) is directly signed
tough.

> > +	 * Since we allow IMA policy rules without func=, we have to enforce
> > +	 * this restriction here.
> > +	 */
> > +	if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> > +		digest_cache = digest_cache_get(file_dentry(file));
> 
> So whether or not a DIGEST_LIST_CHECK policy rule even exists,
> digest_cache_get() will be called.  Similarly, even if a digest_cache list
> hasn't been measured or appraised, digest_cache_get() will be called.
> 
> Basically every file in policy will check the digest_cache.

Only if there is 'digest_cache=content' in the matching rule.

> > +
> > +	if (digest_cache) {
> > +		found = digest_cache_lookup(file_dentry(file), digest_cache,
> > +					    iint->ima_hash->digest,
> > +					    iint->ima_hash->algo);
> > +		/* AND what is allowed by the policy, and what IMA verified. */
> > +		if (found) {
> > +			found_cache = digest_cache_from_found_t(found);
> > +			verif_mask_ptr = digest_cache_verif_get(found_cache,
> > +								"ima");
> 
> Instead of using "verif_{set,get}' consider using '{set,get}_usage', where usage
> here means measure or appraise.

Usage might make sense for IMA, not sure for any other user.

> > +			if (verif_mask_ptr)
> > +				allow_mask = policy_mask & *verif_mask_ptr;
> > +		}
> > +
> > +		digest_cache_put(digest_cache);
> > +	}
> > +
> 
> I'm wondering if it makes sense to create IMA wrappers for each of the
> digest_cache functions - checking the digest_cache for the hash, setting the
> digest_cache permitted usage, etc - and put all of them in a separate
> ima_digest_cache.c file.  The file would only be included in the Makefile if
> digest_cache is configured.

It would be fine for me.

> In this file you could define a static local global variable to detect whether
> the digest_cache is ready to be used.  Only after successfully measuring and
> appraising a digest_cache list, based on policy, set the variable.

Ok, something similar to ima_policy_flag.

> >  	if (action & IMA_MEASURE)
> >  		ima_store_measurement(iint, file, pathname,
> >  				      xattr_value, xattr_len, modsig, pcr,
> > -				      template_desc);
> > +				      template_desc, allow_mask);
> 
> 'allowed_usage'?

Sure, I started renaming them.

Thanks

Roberto

> >  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
> >  		rc = ima_check_blacklist(iint, modsig, pcr);
> >  		if (rc != -EPERM) {
> 
> thanks,
> 
> Mimi
diff mbox series

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2dbcaf0a9402..cf04f5a22234 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -382,7 +382,8 @@  void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
 			   const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, const struct modsig *modsig, int pcr,
-			   struct ima_template_desc *template_desc);
+			   struct ima_template_desc *template_desc,
+			   u64 digest_cache_mask);
 int process_buffer_measurement(struct mnt_idmap *idmap,
 			       struct inode *inode, const void *buf, int size,
 			       const char *eventname, enum ima_hooks func,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 87e286ace43c..b216f86c983d 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -345,7 +345,8 @@  void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
 			   const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, const struct modsig *modsig, int pcr,
-			   struct ima_template_desc *template_desc)
+			   struct ima_template_desc *template_desc,
+			   u64 digest_cache_mask)
 {
 	static const char op[] = "add_template_measure";
 	static const char audit_cause[] = "ENOMEM";
@@ -369,6 +370,18 @@  void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
 	if (iint->measured_pcrs & (0x1 << pcr) && !modsig)
 		return;
 
+	/*
+	 * If digest cache usage was authorized with the IMA policy, the digest
+	 * list the digest cache was populated from was measured, and the file
+	 * digest was found in the digest cache, mark the file as successfully
+	 * measured.
+	 */
+	if (digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT) {
+		iint->flags |= IMA_MEASURED;
+		iint->measured_pcrs |= (0x1 << pcr);
+		return;
+	}
+
 	result = ima_alloc_init_template(&event_data, &entry, template_desc);
 	if (result < 0) {
 		integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3fc48214850a..48a09747ae7a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -222,7 +222,9 @@  static int process_measurement(struct file *file, const struct cred *cred,
 	bool violation_check;
 	enum hash_algo hash_algo;
 	unsigned int allowed_algos = 0;
-	u64 verif_mask = 0;
+	u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
+	struct digest_cache *digest_cache = NULL, *found_cache;
+	digest_cache_found_t found;
 
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
 		return 0;
@@ -233,7 +235,7 @@  static int process_measurement(struct file *file, const struct cred *cred,
 	 */
 	action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
 				mask, func, &pcr, &template_desc, NULL,
-				&allowed_algos, NULL);
+				&allowed_algos, &policy_mask);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
 			    func == MMAP_CHECK_REQPROT) &&
 			   (ima_policy_flag & IMA_MEASURE));
@@ -364,10 +366,34 @@  static int process_measurement(struct file *file, const struct cred *cred,
 	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
+	/*
+	 * For now we don't support nested verification with digest caches.
+	 * Since we allow IMA policy rules without func=, we have to enforce
+	 * this restriction here.
+	 */
+	if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
+		digest_cache = digest_cache_get(file_dentry(file));
+
+	if (digest_cache) {
+		found = digest_cache_lookup(file_dentry(file), digest_cache,
+					    iint->ima_hash->digest,
+					    iint->ima_hash->algo);
+		/* AND what is allowed by the policy, and what IMA verified. */
+		if (found) {
+			found_cache = digest_cache_from_found_t(found);
+			verif_mask_ptr = digest_cache_verif_get(found_cache,
+								"ima");
+			if (verif_mask_ptr)
+				allow_mask = policy_mask & *verif_mask_ptr;
+		}
+
+		digest_cache_put(digest_cache);
+	}
+
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len, modsig, pcr,
-				      template_desc);
+				      template_desc, allow_mask);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
 		rc = ima_check_blacklist(iint, modsig, pcr);
 		if (rc != -EPERM) {