linux-kernel - Re: [RFC][PATCH 8/8] ima: Detect if digest cache changed since last measurement/appraisal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 11 Mar 2024 10:11:54 +0100
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Mimi Zohar <zohar@...ux.ibm.com>, corbet@....net,
 dmitry.kasatkin@...il.com,  eric.snowberg@...cle.com, paul@...l-moore.com,
 jmorris@...ei.org, serge@...lyn.com
Cc: linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, 
 linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org, 
 wufan@...ux.microsoft.com, pbrobinson@...il.com, zbyszek@...waw.pl,
 hch@....de,  mjg59@...f.ucam.org, pmatilai@...hat.com, jannh@...gle.com,
 dhowells@...hat.com,  jikos@...nel.org, mkoutny@...e.com, ppavlu@...e.com,
 petr.vorel@...il.com,  petrtesarik@...weicloud.com, mzerqung@...inter.de,
 kgold@...ux.ibm.com, Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [RFC][PATCH 8/8] ima: Detect if digest cache changed since last
 measurement/appraisal

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)
>