linux-kernel - Re: [PATCH v3 7/7] ima: Support module-style appended signatures for appraisal

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] [day] [month] [year] [list]

Date:   Thu, 03 Aug 2017 18:01:06 -0300
From:   Thiago Jung Bauermann <bauerman@...ux.vnet.ibm.com>
To:     Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:     linux-security-module@...r.kernel.org,
        linux-ima-devel@...ts.sourceforge.net, 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 v3 7/7] ima: Support module-style appended signatures for appraisal


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

> On Wed, 2017-08-02 at 18:52 -0400, Mimi Zohar wrote:
>> On Wed, 2017-08-02 at 14:42 -0300, Thiago Jung Bauermann wrote:
>> > Mimi Zohar <zohar@...ux.vnet.ibm.com> writes:
>
>> > >> @@ -229,8 +251,24 @@ int ima_appraise_measurement(enum ima_hooks func,
>> > >>  		goto out;
>> > >>  	}
>> > >> 
>> > >> -	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
>> > >> -	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
>> > >> +	/*
>> > >> +	 * Appended signatures aren't protected by EVM but we still call
>> > >> +	 * evm_verifyxattr to check other security xattrs, if they exist.
>> > >> +	 */
>> > >> +	if (appraising_modsig) {
>> > >> +		xattr_value_evm = NULL;
>> > >> +		xattr_len_evm = 0;
>> > >> +	} else {
>> > >> +		xattr_value_evm = xattr_value;
>> > >> +		xattr_len_evm = xattr_len;
>> > >> +	}
>> > >> +
>> > >> +	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
>> > >> +				 xattr_len_evm, iint);
>> > >> +	if (appraising_modsig && status == INTEGRITY_FAIL) {
>> > >> +		cause = "invalid-HMAC";
>> > >> +		goto out;
>> > >
>> > > "modsig" is special, because having any security xattrs is not
>> > > required. This test doesn't prevent status from being set to
>> > > "missing-HMAC". This test is redundant with the original tests below.
>> > 
>> > Indeed, that is wrong. I'm still a bit fuzzy about how EVM works and how
>> > it interacts with IMA. The only way I can think of singling out modsig
>> > without reintroduced the complex expression you didn't like in v2 is as
>> > below. What do you think?
>> 
>> The original code, without any extra tests, should be fine.
>
> There is one major difference.
>
> EVM verifies a file's metadata has not been modified based on either
> an HMAC or signature stored as security.evm. Prior to the appended
> signatures patch set, all files in policy required a security.evm
> xattr. With IMA enabled we could guarantee that at least one security
> xattr existed. The only exception were new files, which hadn't yet
> been labeled.
>
> With appended signatures, there is now no guarantee that at least one
> security xattr exists.
>
> Perhaps the code snippet below will help clarify the meaning of the
> integrity_status results.
>
>     switch (status) {
> case INTEGRITY_PASS:
> case INTEGRITY_UNKNOWN:   
>        break;		
>     case INTEGRITY_NOXATTRS:/* no EVM protected xattrs */
> if (appraising_modsig)
> break;
> case INTEGRITY_NOLABEL:/* no security.evm xattr */
> cause = "missing-HMAC";
> fail = 1;
> break;
> case INTEGRITY_FAIL:/* invalid HMAC/signature */
> default:
> cause = "invalid-HMAC";
> fail = 1;
> break;
> }

Thanks! I'll use the switch above in the next version of the patch.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center