linux-kernel - Re: [v2 PATCH] crypto: api - Disallow sha1 in FIPS-mode while allowing hmac(sha1)

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]

Message-ID: <87a6fyh4xh.fsf@suse.de>
Date:   Fri, 14 Jan 2022 13:34:18 +0100
From:   Nicolai Stange <nstange@...e.de>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Nicolai Stange <nstange@...e.de>,
        Stephan Mueller <smueller@...onox.de>,
        "David S. Miller" <davem@...emloft.net>,
        Hannes Reinecke <hare@...e.de>, Torsten Duwe <duwe@...e.de>,
        Zaibo Xu <xuzaibo@...wei.com>,
        Giovanni Cabiddu <giovanni.cabiddu@...el.com>,
        David Howells <dhowells@...hat.com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        qat-linux@...el.com, keyrings@...r.kernel.org, simo@...hat.com,
        Eric Biggers <ebiggers@...nel.org>, Petr Vorel <pvorel@...e.cz>
Subject: Re: [v2 PATCH] crypto: api - Disallow sha1 in FIPS-mode while allowing hmac(sha1)

Herbert Xu <herbert@...dor.apana.org.au> writes:

> On Fri, Jan 14, 2022 at 10:09:02AM +0100, Nicolai Stange wrote:
>>
>> I wonder whether this can be made more generic. I.e. would it be possible
>> to make crypto_grab_spawn() to or in FIPS_INTERNAL into type (iff !(mask
>> & FIPS_INTERNAL)) and to make crypto_register_instance() to propagate
>> FIPS_INTERNAL from the template instance's spawns upwards into the
>> instance's ->cra_flags?
>
> We could certainly do something like that in future.  But it
> isn't that easy because crypto_register_instance doesn't know
> what the paramter algorithm(s) is/are.

I was thinking of simply walking through the inst->spawns list for that.


>
>> On an unrelated note, this will break trusted_key_tpm_ops->init() in
>> FIPS mode, because trusted_shash_alloc() would fail to get a hold of
>> sha1. AFAICT, this could potentially make the init_trusted() module_init
>> to fail, and, as encrypted-keys.ko imports key_type_trusted, prevent the
>> loading of that one as well. Not sure that's desired...
>
> Well if sha1 is supposed to be forbidden in FIPS mode why should
> TPM be allowed to use it? 

Yes, I only wanted to point out that doing that could potentially have
unforseen consequences as it currently stands, like
e.g. encrypted-keys.ko loading failures, even though the latter doesn't
even seem to use sha1 by itself.

However, this scenario would be possible only for CONFIG_TRUSTED_KEYS=m,
CONFIG_TEE=n and tpm_default_chip() returning something.


> If it must be allowed, then we could change TPM to set the
> FIPS_INTERNAL flag.
>
> I think I'll simply leave out the line that actually disables sha1
> for now.
>
>> > diff --git a/crypto/api.c b/crypto/api.c
>> > index cf0869dd130b..549f9aced1da 100644
>> > --- a/crypto/api.c
>> > +++ b/crypto/api.c
>> > @@ -223,6 +223,8 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
>> >  	else if (crypto_is_test_larval(larval) &&
>> >  		 !(alg->cra_flags & CRYPTO_ALG_TESTED))
>> >  		alg = ERR_PTR(-EAGAIN);
>> > +	else if (alg->cra_flags & CRYPTO_ALG_FIPS_INTERNAL)
>> > +		alg = ERR_PTR(-EAGAIN);
>> 
>> I might be mistaken, but I think this would cause hmac(sha1)
>> instantiation to fail if sha1 is not already there. I.e. if hmac(sha1)
>> instantiation would load sha1, then it would invoke crypto_larval_wait()
>> on the sha1 larval, see the FIPS_INTERNAL and fail?
>
> When EAGAIN is returned the lookup is automatically retried.

Ah right, just found the loop in cryptomgr_probe().


>
>> However, wouldn't it be possible to simply implement FIPS_INTERNAL
>> lookups in analogy to the INTERNAL ones instead? That is, would adding
>> 
>> 	if (!((type | mask) & CRYPTO_ALG_FIPS_INTERNAL))
>> 		mask |= CRYPTO_ALG_FIPS_INTERNAL;
>> 
>> to the preamble of crypto_alg_mod_lookup() work instead?
>
> If you do that then you will end up creating an infinite number
> of failed templates if you lookup something like hmac(md5).  Once
> the first hmac(md5) is created it must then match all subsequent
> lookups of hmac(md5) in order to prevent any further creation.

Thanks for the explanation, it makes sense now.

>
>> This looks all good to me, but as !->fips_allowed tests aren't skipped
>> over anymore now, it would perhaps make sense to make their failure
>> non-fatal in FIPS mode. Because in FIPS mode a failure could mean a
>> panic and some of the existing TVs might not pass because of e.g. some
>> key length checks or so active only for fips_enabled...
>
> You mean a buggy non-FIPS algorithm that fails when tested in
> FIPS mode?

Yes, I can't tell how realistic that is though.


> I guess we could skip the panic in that case if everyone is happy with
> that.  Stephan?
>

Thanks,

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Ivo Totev