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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 26 Oct 2017 20:02:23 -0200
From:   Thiago Jung Bauermann <bauerman@...ux.vnet.ibm.com>
To:     Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
        linux-crypto@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        James Morris <james.l.morris@...cle.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        David Howells <dhowells@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Jessica Yu <jeyu@...hat.com>,
        Rusty Russell <rusty@...tcorp.com.au>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        "AKASHI\, Takahiro" <takahiro.akashi@...aro.org>
Subject: Re: [PATCH v5 18/18] ima: Write modsig to the measurement list


Hello Mimi,

Thanks for your review.

Mimi Zohar <zohar@...ux.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