Messages in this thread | | | From | Nicolai Stange <> | Subject | Re: [v2 PATCH] crypto: api - Disallow sha1 in FIPS-mode while allowing hmac(sha1) | Date | Fri, 14 Jan 2022 13:34:18 +0100 |
| |
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
| |