From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932453AbdJZWCi (ORCPT ); Thu, 26 Oct 2017 18:02:38 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:37956 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932400AbdJZWCg (ORCPT ); Thu, 26 Oct 2017 18:02:36 -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 X-TM-AS-GCONF: 00 x-cbid: 17102622-0044-0000-0000-000003A5DB7F X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007957; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000239; SDB=6.00936912; UDB=6.00472171; IPR=6.00717175; BA=6.00005660; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017730; XFM=3.00000015; UTC=2017-10-26 22:02:34 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17102622-0045-0000-0000-000007D4E91F Message-Id: <87r2tpwosw.fsf@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-26_09:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710260278 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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