diff mbox series

[RFC,8/8] ima: Detect if digest cache changed since last measurement/appraisal

Message ID 20240214143525.2205481-9-roberto.sassu@huaweicloud.com (mailing list archive)
State Handled Elsewhere
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>

IMA invalidates the cached verification result on file content/metadata
update, so that the file is evaluated again at next access.

While until now checking modifications on the file was sufficient to
determine if the cached verification result is still valid, that no longer
applies if that verification result was obtained with digest caches.

In that case, it is also necessary to check modifications on the digest
lists and on the security.digest_list xattr of the files for which digest
caches are used.

The digest_cache LSM offers the digest_cache_changed() function, which
tells if a file would use a different digest cache than the one passed as
argument. digest_cache_get() might return a different digest cache if the
digest list was modified/deleted/renamed or the security.digest_list xattr
was modified.

Hold a digest cache reference in the IMA integrity metadata, when using it
for measurement/appraisal. At every file access, check if that reference is
still actual by passing it to digest_cache_changed(). If not, reset the
integrity status and do the verification again.

Finally, move the digest_cache_put() call from process_measurement() to
ima_iint_free(), unless the digest cache changed. In that case, still
release the reference in process_measurement().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h      |  1 +
 security/integrity/ima/ima_iint.c |  3 +++
 security/integrity/ima/ima_main.c | 22 ++++++++++++++++++----
 3 files changed, 22 insertions(+), 4 deletions(-)

Comments

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

> b/security/integrity/ima/ima_main.c
> index a66522a22cbc..e1b2f5737753 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -301,6 +301,15 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  		}
>  	}
>  
> +	/* Check if digest cache changed since last measurement/appraisal. */
> +	if (iint->digest_cache &&
> +	    digest_cache_changed(inode, iint->digest_cache)) {
> +		iint->flags &= ~IMA_DONE_MASK;
> +		iint->measured_pcrs = 0;
> +		digest_cache_put(iint->digest_cache);
> +		iint->digest_cache = NULL;
> +	}
> +
>  	/* Determine if already appraised/measured based on bitmask
>  	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
>  	 *  IMA_AUDIT, IMA_AUDITED)
> @@ -371,8 +380,15 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	 * 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 (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK) {
> +		if (!iint->digest_cache) {
> +			/* Released by ima_iint_free(). */
> +			digest_cache = digest_cache_get(file_dentry(file));
> +			iint->digest_cache = digest_cache;
> +		} else {
> +			digest_cache = iint->digest_cache;
> +		}

Simple cleanup:
		if (!iint->digest_cache)
			iint->digest_cache =digest_cache_get(file_dentry(file));

		digest_cache = iint->digest_cache;

> +	}
>  
>  	if (digest_cache) {
>  		found = digest_cache_lookup(file_dentry(file), digest_cache,
> @@ -386,8 +402,6 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  			if (verif_mask_ptr)
>  				allow_mask = policy_mask & *verif_mask_ptr;
>  		}
> -
> -		digest_cache_put(digest_cache);

Keeping a reference to the digest_cache list for each file in the iint cache
until the file is re-accessed, might take a while to free.

I'm wondering if it necessary to keep a reference to the digest_cache.  Or is it
possible to just compare the existing iint->digest_cache pointer with the
current digest_cache pointer?

thanks,

Mimi

>  	}
>  
>  	if (action & IMA_MEASURE)
Roberto Sassu March 11, 2024, 9:11 a.m. UTC | #2 
On Fri, 2024-03-08 at 12:35 -0500, Mimi Zohar wrote:
> Hi Roberto,
> 
> > b/security/integrity/ima/ima_main.c
> > index a66522a22cbc..e1b2f5737753 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -301,6 +301,15 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  		}
> >  	}
> >  
> > +	/* Check if digest cache changed since last measurement/appraisal. */
> > +	if (iint->digest_cache &&
> > +	    digest_cache_changed(inode, iint->digest_cache)) {
> > +		iint->flags &= ~IMA_DONE_MASK;
> > +		iint->measured_pcrs = 0;
> > +		digest_cache_put(iint->digest_cache);
> > +		iint->digest_cache = NULL;
> > +	}
> > +
> >  	/* Determine if already appraised/measured based on bitmask
> >  	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
> >  	 *  IMA_AUDIT, IMA_AUDITED)
> > @@ -371,8 +380,15 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  	 * 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 (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK) {
> > +		if (!iint->digest_cache) {
> > +			/* Released by ima_iint_free(). */
> > +			digest_cache = digest_cache_get(file_dentry(file));
> > +			iint->digest_cache = digest_cache;
> > +		} else {
> > +			digest_cache = iint->digest_cache;
> > +		}
> 
> Simple cleanup:
> 		if (!iint->digest_cache)
> 			iint->digest_cache =digest_cache_get(file_dentry(file));
> 
> 		digest_cache = iint->digest_cache;

Thanks.

> > +	}
> >  
> >  	if (digest_cache) {
> >  		found = digest_cache_lookup(file_dentry(file), digest_cache,
> > @@ -386,8 +402,6 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> >  			if (verif_mask_ptr)
> >  				allow_mask = policy_mask & *verif_mask_ptr;
> >  		}
> > -
> > -		digest_cache_put(digest_cache);
> 
> Keeping a reference to the digest_cache list for each file in the iint cache
> until the file is re-accessed, might take a while to free.

Yes, that is the drawback...

> I'm wondering if it necessary to keep a reference to the digest_cache.  Or is it
> possible to just compare the existing iint->digest_cache pointer with the
> current digest_cache pointer?

If the pointer value is the same, it does not guarantee that it is the
same digest cache used for the previous verification. It might have
been freed and reallocated.

Maybe, if the digest_cache LSM is able to notify to IMA that the digest
cache changed, so that IMA resets its flags in the integrity metadata,
we would not need to pin it.

Roberto

> thanks,
> 
> Mimi
> 
> >  	}
> >  
> >  	if (action & IMA_MEASURE)
>
Mimi Zohar March 11, 2024, 12:19 p.m. UTC | #3 
On Mon, 2024-03-11 at 10:11 +0100, Roberto Sassu wrote:
> 
> > > @@ -386,8 +402,6 @@ static int process_measurement(struct file *file,
> > > const
> > > struct cred *cred,
> > >  			if (verif_mask_ptr)
> > >  				allow_mask = policy_mask & *verif_mask_ptr;
> > >  		}
> > > -
> > > -		digest_cache_put(digest_cache);
> > 
> > Keeping a reference to the digest_cache list for each file in the iint cache
> > until the file is re-accessed, might take a while to free.
> 
> Yes, that is the drawback...
> 
> > I'm wondering if it necessary to keep a reference to the digest_cache.  Or
> > is it
> > possible to just compare the existing iint->digest_cache pointer with the
> > current digest_cache pointer?
> 
> If the pointer value is the same, it does not guarantee that it is the
> same digest cache used for the previous verification. It might have
> been freed and reallocated.

Agreed.
> 
> Maybe, if the digest_cache LSM is able to notify to IMA that the digest
> cache changed, so that IMA resets its flags in the integrity metadata,
> we would not need to pin it.

Yes, something similar to the "ima_lsm_policy_notifier".

Mimi
diff mbox series

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 36faf2bc81b0..c25bde918cd5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -192,6 +192,7 @@  struct ima_iint_cache {
 	enum integrity_status ima_read_status:4;
 	enum integrity_status ima_creds_status:4;
 	struct ima_digest_data *ima_hash;
+	struct digest_cache *digest_cache;
 };
 
 extern struct lsm_blob_sizes ima_blob_sizes;
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index b4f476fae437..fd369809809f 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -68,6 +68,7 @@  static void ima_iint_init_always(struct ima_iint_cache *iint,
 	iint->ima_read_status = INTEGRITY_UNKNOWN;
 	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->measured_pcrs = 0;
+	iint->digest_cache = NULL;
 	mutex_init(&iint->mutex);
 	ima_iint_lockdep_annotate(iint, inode, nested);
 }
@@ -75,6 +76,8 @@  static void ima_iint_init_always(struct ima_iint_cache *iint,
 static void ima_iint_free(struct ima_iint_cache *iint)
 {
 	kfree(iint->ima_hash);
+	if (iint->digest_cache)
+		digest_cache_put(iint->digest_cache);
 	mutex_destroy(&iint->mutex);
 	kmem_cache_free(ima_iint_cache, iint);
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a66522a22cbc..e1b2f5737753 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -301,6 +301,15 @@  static int process_measurement(struct file *file, const struct cred *cred,
 		}
 	}
 
+	/* Check if digest cache changed since last measurement/appraisal. */
+	if (iint->digest_cache &&
+	    digest_cache_changed(inode, iint->digest_cache)) {
+		iint->flags &= ~IMA_DONE_MASK;
+		iint->measured_pcrs = 0;
+		digest_cache_put(iint->digest_cache);
+		iint->digest_cache = NULL;
+	}
+
 	/* Determine if already appraised/measured based on bitmask
 	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
 	 *  IMA_AUDIT, IMA_AUDITED)
@@ -371,8 +380,15 @@  static int process_measurement(struct file *file, const struct cred *cred,
 	 * 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 (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK) {
+		if (!iint->digest_cache) {
+			/* Released by ima_iint_free(). */
+			digest_cache = digest_cache_get(file_dentry(file));
+			iint->digest_cache = digest_cache;
+		} else {
+			digest_cache = iint->digest_cache;
+		}
+	}
 
 	if (digest_cache) {
 		found = digest_cache_lookup(file_dentry(file), digest_cache,
@@ -386,8 +402,6 @@  static int process_measurement(struct file *file, const struct cred *cred,
 			if (verif_mask_ptr)
 				allow_mask = policy_mask & *verif_mask_ptr;
 		}
-
-		digest_cache_put(digest_cache);
 	}
 
 	if (action & IMA_MEASURE)