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

From: Nicolai Stange <nstange@suse.de>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Nicolai Stange <nstange@suse.de>,
	Stephan Mueller <smueller@chronox.de>,
	"David S. Miller" <davem@davemloft.net>,
	Hannes Reinecke <hare@suse.de>, Torsten Duwe <duwe@suse.de>,
	Zaibo Xu <xuzaibo@huawei.com>,
	Giovanni Cabiddu <giovanni.cabiddu@intel.com>,
	David Howells <dhowells@redhat.com>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
	qat-linux@intel.com, keyrings@vger.kernel.org, simo@redhat.com,
	Eric Biggers <ebiggers@kernel.org>, Petr Vorel <pvorel@suse.cz>
Subject: Re: [v2 PATCH] crypto: api - Disallow sha1 in FIPS-mode while allowing hmac(sha1)
Date: Fri, 14 Jan 2022 13:34:18 +0100	[thread overview]
Message-ID: <87a6fyh4xh.fsf@suse.de> (raw)
In-Reply-To: <YeFWnscvXtv73KBl@gondor.apana.org.au> (Herbert Xu's message of "Fri, 14 Jan 2022 21:55:26 +1100")

Herbert Xu <herbert@gondor.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