Hello Mimi, Thanks for your review. Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> 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
