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, 28 Jan 2022 15:14:39 +0100	[thread overview]
Message-ID: <87v8y4dk1c.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:
>
>> 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?  I guess we could skip the panic in that case if
> everyone is happy with that.  Stephan?

One more thing I just realized: dracut's fips module ([1]) modprobes
tcrypt (*) and failure is considered fatal, i.e. the system would not
boot up.

First of all this would mean that tcrypt_test() needs to ignore
-ECANCELED return values from alg_test() in FIPS mode, in addition to
the -EINVAL it is already prepared for.

However, chances are that some of the !fips_allowed algorithms looped
over by tcrypt are not available (i.e. not enabled at build time) and as
this change here makes alg_test() to unconditionally attempt a test
execution now, this would fail with -ENOENT AFAICS.

One way to work around this is to make tcrypt_test() to ignore -ENOENT
in addition to -EINVAL and -ECANCELED.

It might be undesirable though that the test executions triggered from
tcrypt would still instantiate/load a ton of !fips_allowed algorithms at
boot, most of which will effectively be inaccessible (because they're
not used as FIPS_INTERNAL arguments to fips_allowed == 1 template
instances).

So how about making alg_test() to skip the !fips_allowed tests in FIPS
mode as before, but to return -ECANCELED and eventually set
FIPS_INTERNAL as implemented with this patch here.

This would imply that FIPS_INTERNAL algorithms by themselves remain
untested, but I think this might be Ok as they would be usable only as
template arguments in fips_allowed instantiations. That is, they will
still receive some form of testing when the larger construction they're
part of gets tested.

For example, going with the "dh" example, where "dh" and "ffdhe3072(dh)"
would have fips_allowed unset and set respecively, ffdhe3072(dh) as
a whole would get tested, but not the "dh" argument individually.

Stephan, would this approach work from a FIPS 140-3 perspective?

Thanks!

Nicolai

[1] https://git.kernel.org/pub/scm/boot/dracut/dracut.git/tree/modules.d/01fips/fips.sh#n106
(*) I'm not sure why this is being done, but it is what it is.

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