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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ