Message-ID: <523a37b29820fe98151b1d608f77b88cc026a91d.camel@huaweicloud.com>
Date: Fri, 08 Mar 2024 17:27:31 +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 6/8] ima: Use digest cache for measurement

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

Powered by blists - more mailing lists