From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3yNLbL5k95zDqlH for ; Fri, 27 Oct 2017 09:02:37 +1100 (AEDT) Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v9QLxZoI063186 for ; Thu, 26 Oct 2017 18:02:35 -0400 Received: from e18.ny.us.ibm.com (e18.ny.us.ibm.com [129.33.205.208]) by mx0b-001b2d01.pphosted.com with ESMTP id 2dumpuh6mc-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 26 Oct 2017 18:02:35 -0400 Received: from localhost by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 26 Oct 2017 18:02:34 -0400 References: <20171018005331.2688-1-bauerman@linux.vnet.ibm.com> <20171018005331.2688-19-bauerman@linux.vnet.ibm.com> <1509048454.5886.108.camel@linux.vnet.ibm.com> From: Thiago Jung Bauermann To: Mimi Zohar Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, linux-crypto@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Dmitry Kasatkin , James Morris , "Serge E. Hallyn" , David Howells , David Woodhouse , Jessica Yu , Rusty Russell , Herbert Xu , "David S. Miller" , "AKASHI\, Takahiro" Subject: Re: [PATCH v5 18/18] ima: Write modsig to the measurement list In-reply-to: <1509048454.5886.108.camel@linux.vnet.ibm.com> Date: Thu, 26 Oct 2017 20:02:23 -0200 MIME-Version: 1.0 Content-Type: text/plain Message-Id: <87r2tpwosw.fsf@linux.vnet.ibm.com> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hello Mimi, Thanks for your review. Mimi Zohar 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