Re: [PATCH v5 18/18] ima: Write modsig to the measurement list

[Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>]

Hello Mimi,

Thanks for your review.

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:
>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 6a2d960fbd92..0d3390de7432 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -246,7 +246,35 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>  		rc = ima_appraise_measurement(func, iint, file, buf, size,
>>  					      pathname, &xattr_value,
>>  					      &xattr_len, opened);
>> -	if (action & IMA_MEASURE)
>> +
>> +	/*
>> +	 * MODSIG has one corner case we need to deal with here:
>> +	 *
>> +	 * Suppose the policy has one measure rule for one hook and an appraise
>> +	 * rule for a different hook. Suppose also that the template requires
>> +	 * the signature to be stored in the measurement list.
>> +	 *
>> +	 * If a file containing a MODSIG is measured by the first hook before
>> +	 * being appraised by the second one, the corresponding entry in the
>> +	 * measurement list will not contain the MODSIG because we only fetch it
>> +	 * for IMA_APPRAISAL. We don't fetch it earlier because if the file has
>> +	 * both a DIGSIG and a MODSIG it's not possible to know which one will
>> +	 * be valid without actually doing the appraisal.
>> +	 *
>> +	 * Therefore, after appraisal of a MODSIG signature we need to store the
>> +	 * measurement again if the current template requires storing the
>> +	 * signature.
>
> Yes, all true, but this long comment doesn't belong here in the middle
> of process_measurement().
>
>> +	 * With the opposite ordering (the appraise rule triggering before the
>> +	 * measurement rule) there is the same problem but it's not possible to
>> +	 * do anything about it because at the time we are appraising the
>> +	 * signature it's impossible to know whether a measurement will ever
>> +	 * need to be stored for this file.
>> +	 */
>
> With the template format "ima-sig", the verified file signature needs
> to be included in the measurement list. Based on this file signature,
> the attestation server can validate the signature.
>
> In this case, where the appraisal comes first followed by the
> measurement, the appraised file signature is included in the
> measurement list. I don't see the problem here.

I think I forgot that during appraisal the modsig is copied into the
iint cache and that it will be used when the measure rule is trigerred.
I'll drop that last paragraph.

>
>> +	if ((action & IMA_MEASURE) || ((iint->flags & IMA_MEASURE) &&
>> +				       xattr_value &&
>> +				       xattr_value->type == IMA_MODSIG &&
>> +				       ima_current_template_has_sig()))
>
> Like the clean up you did elsewhere, this new set of tests should be
> made into a function. The comment could placed along with the new
> function.

Ok. I didn't create a function because these tests are only done here,
but I agree that it will make the code clearer, and be a better place
for the big comment as well. Will do in the next version.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center