Re: [PATCH v2 6/6] ima: Support module-style appended signatures for appraisal

[Mimi Zohar]

On Tue, 2017-07-04 at 23:22 -0300, Thiago Jung Bauermann wrote:
> Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> writes:
> 
> > On Wed, 2017-06-21 at 14:45 -0300, Thiago Jung Bauermann wrote:
> >> Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> writes:
> >> > On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote:
> >> >> @@ -267,11 +276,18 @@ int ima_appraise_measurement(enum ima_hooks func,
> >> >>  		status = INTEGRITY_PASS;
> >> >>  		break;
> >> >>  	case EVM_IMA_XATTR_DIGSIG:
> >> >> +	case IMA_MODSIG:
> >> >>  		iint->flags |= IMA_DIGSIG;
> >> >> -		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> >> >> -					     (const char *)xattr_value, rc,
> >> >> -					     iint->ima_hash->digest,
> >> >> -					     iint->ima_hash->length);
> >> >> +
> >> >> +		if (xattr_value->type == EVM_IMA_XATTR_DIGSIG)
> >> >> +			rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> >> >> +						     (const char *)xattr_value,
> >> >> +						     rc, iint->ima_hash->digest,
> >> >> +						     iint->ima_hash->length);
> >> >> +		else
> >> >> +			rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA,
> >> >> +					       xattr_value);
> >> >> +
> >> >
> >> > Perhaps allowing IMA_MODSIG to flow into EVM_IMA_XATTR_DIGSIG on
> >> > failure, would help restore process_measurements() to the way it was.
> >> > Further explanation below.
> >> 
> >> It's not possible to simply flow into EVM_IMA_XATTR_DIGSIG on failure
> >> because after calling ima_read_xattr we need to run again all the logic
> >> before the switch statement. Instead, I'm currently testing the
> >> following change for v3, what do you think?
> >
> > I don't think we can assume that the same algorithm will be used for
> > signing the kernel image. Different entities would be signing the
> > kernel image with different requirements.
> >
> > Suppose for example a stock distro image comes signed using one
> > algorithm (appended signature), but the same kernel image is locally
> > signed using a different algorithm (xattr). Signature verification is
> > dependent on either the distro or local public key being loaded onto
> > the IMA keyring.
> 
> This example is good, but it raises one question: should the xattr
> signature cover the entire contents of the stock distro image (i.e.,
> also cover the appended signature), or should it ignore the appended
> signature and thus cover the same contents that the appended signature
> covers?
> 
> If the former, then we can't reuse the iint->ima_hash that was collected
> when trying to verify the appended signature because it doesn't cover
> the appended signature itself and won't match the hash expected by the
> xattr signature.
> 
> If the latter, then evmctl ima_sign needs to be modified to check
> whether there's an appended signature in the given file and ignore it
> when calculating the xattr signature.
> 
> Which is better?

I realize that having the same file hash for both the appended
signature and extended attribute would make things a lot easier, but
security.ima is a signature of the file as written to disk, meaning it
would include any appended signature

> 
> >> >> @@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> >> >>  		goto out_digsig;
> >> >>  	}
> >> >> 
> >> >> -	template_desc = ima_template_desc_current();
> >> >> -	if ((action & IMA_APPRAISE_SUBMASK) ||
> >> >> -		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
> >> >> -		/* read 'security.ima' */
> >> >> -		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
> >> >> -
> >> >> -	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
> >> >> -
> >> >> -	rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
> >> >> -	if (rc != 0) {
> >> >> -		if (file->f_flags & O_DIRECT)
> >> >> -			rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
> >> >> -		goto out_digsig;
> >> >> -	}
> >> >> -
> >> >
> >> > There are four stages: collect measurement, store measurement,
> >> > appraise measurement and audit measurement. "Collect" needs to be
> >> > done if any one of the other stages is needed.
> >> >
> >> >>  	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
> >> >>  		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> >> >> 
> >> >> +	if (iint->flags & IMA_MODSIG_ALLOWED)
> >> >> +		rc = measure_and_appraise(file, buf, size, func, opened, action,
> >> >> +					  iint, &xattr_value, &xattr_len,
> >> >> +					  pathname, true);
> >> >> +	if (!xattr_len)
> >> >> +		rc = measure_and_appraise(file, buf, size, func, opened, action,
> >> >> +					  iint, &xattr_value, &xattr_len,
> >> >> +					  pathname, false);
> >> >
> >> > I would rather see "collect" extended to support an appended signature
> >> > rather than trying to combine "collect" and "appraise" together.
> >> 
> >> I'm not sure I understand what you mean by "extend collect to support an
> >> appended signature", but the fundamental problem is that we don't know
> >> whether we need to fallback to the xattr sig until the appraise step
> >> because that's when we verify the signature, and by then the collect and
> >> store steps were already completed and would need to be done again if
> >> the hash algorithm in the xattr sig is different.
> >
> > The "appraise" stage could be moved before the "store" stage, like you
> > have. (This should be a separate patch explaining the need for moving
> > it.) Based on an argument to ima_collect_measurement() have it
> > "collect" either the appended signature or the xattr. Maybe something
> > like this:
> >
> > loop [ appended signature, xattr ] { <== list based on policy flags
> >   collect_measurement()
> >   if failure
> >     continue
> >   appraise_measurement()
> >   if success
> >     break
> > }
> >
> > store_measurement()
> 
> Ok, the above is what v2 already does, so the only change I made was to
> rename the function from measure_and_appraise to collect_and_appraise to
> match the IMA jargon.

I would prefer if you did not combine the "collect" and "appraise"
functions, but left them independent of each other.Â

Mimi