| Message ID | YeEVSaMEVJb3cQkq@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] crypto: api - Disallow sha1 in FIPS-mode while allowing hmac(sha1) | expand |
Hi Herbert, many thanks for this, I think this approach can be applied as-is to the ffdheXYZ(dh) situation. I have some questions/comments inline. Herbert Xu <herbert@gondor.apana.org.au> writes: > On Tue, Jan 11, 2022 at 09:34:19PM +1100, Herbert Xu wrote: >> >> You're right. The real issue is that any algorithm with no tests >> at all is allowed in FIPS mode. That's clearly suboptimal. But we >> can't just ban every unknown algorithm because we rely on that >> to let things like echainiv through. >> >> Let me figure out a way to differentiate these two cases. > > So what I've done is modify hmac so that if the underlying algo > is FIPS_INTERNAL, then we pre-emptively set FIPS_INTERNAL on the > hmac side as well. This can then be cleared if the hmac algorithm > is explicitly listed as fips_allowed. 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? > ---8<--- > Currently we do not distinguish between algorithms that fail on > the self-test vs. those which are disabled in FIPS mode (not allowed). > Both are marked as having failed the self-test. > > As it has been requested that we need to disable sha1 in FIPS > mode while still allowing hmac(sha1) this approach needs to change. 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... > This patch allows this scenario by adding a new flag FIPS_INTERNAL > to indicate those algorithms that have passed the self-test and are > not FIPS-allowed. They can then be used for the self-testing of > other algorithms or by those that are explicitly allowed to use them > (currently just hmac). > > Note that as a side-effect of this patch algorithms which are not > FIPS-allowed will now return ENOENT instead of ELIBBAD. Hopefully > this is not an issue as some people were relying on this already. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/crypto/algapi.c b/crypto/algapi.c > index a366cb3e8aa1..09fb75806e87 100644 > --- a/crypto/algapi.c > +++ b/crypto/algapi.c > @@ -322,8 +322,16 @@ void crypto_alg_tested(const char *name, int err) > found: > q->cra_flags |= CRYPTO_ALG_DEAD; > alg = test->adult; > - if (err || list_empty(&alg->cra_list)) > + > + if (list_empty(&alg->cra_list)) > + goto complete; > + > + if (err == -ECANCELED) > + alg->cra_flags |= CRYPTO_ALG_FIPS_INTERNAL; > + else if (err) > goto complete; > + else > + alg->cra_flags &= ~CRYPTO_ALG_FIPS_INTERNAL; > > alg->cra_flags |= CRYPTO_ALG_TESTED; > > 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? 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? AFAICS, ... > else if (!crypto_mod_get(alg)) > alg = ERR_PTR(-EAGAIN); > crypto_mod_put(&larval->alg); > @@ -233,6 +235,7 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg) > static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type, > u32 mask) > { > + const u32 fips = CRYPTO_ALG_FIPS_INTERNAL; > struct crypto_alg *alg; > u32 test = 0; > > @@ -240,8 +243,20 @@ static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type, > test |= CRYPTO_ALG_TESTED; > > down_read(&crypto_alg_sem); > - alg = __crypto_alg_lookup(name, type | test, mask | test); > - if (!alg && test) { > + alg = __crypto_alg_lookup(name, (type | test) & ~fips, > + (mask | test) & ~fips); > + if (alg) { > + if (((type | mask) ^ fips) & fips) > + mask |= fips; > + mask &= fips; > + > + if (!crypto_is_larval(alg) && > + ((type ^ alg->cra_flags) & mask)) { > + /* Algorithm is disallowed in FIPS mode. */ > + crypto_mod_put(alg); > + alg = ERR_PTR(-ENOENT); > + } > + } else if (test) { ... this check here would not be needed anymore then? But I might be missing something. > alg = __crypto_alg_lookup(name, type, mask); > if (alg && !crypto_is_larval(alg)) { > /* Test failed */ > diff --git a/crypto/hmac.c b/crypto/hmac.c > index 25856aa7ccbf..af82e3eeb7d0 100644 > --- a/crypto/hmac.c > +++ b/crypto/hmac.c > @@ -169,6 +169,7 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) > struct crypto_alg *alg; > struct shash_alg *salg; > u32 mask; > + u32 type; > int err; > int ds; > int ss; > @@ -182,8 +183,9 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) > return -ENOMEM; > spawn = shash_instance_ctx(inst); > > + type = CRYPTO_ALG_FIPS_INTERNAL; > err = crypto_grab_shash(spawn, shash_crypto_instance(inst), > - crypto_attr_alg_name(tb[1]), 0, mask); > + crypto_attr_alg_name(tb[1]), type, mask); > if (err) > goto err_free_inst; > salg = crypto_spawn_shash_alg(spawn); > @@ -204,6 +206,7 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) > if (err) > goto err_free_inst; > > + inst->alg.base.cra_flags = alg->cra_flags & CRYPTO_ALG_FIPS_INTERNAL; > inst->alg.base.cra_priority = alg->cra_priority; > inst->alg.base.cra_blocksize = alg->cra_blocksize; > inst->alg.base.cra_alignmask = alg->cra_alignmask; > diff --git a/crypto/testmgr.c b/crypto/testmgr.c > index 5831d4bbc64f..995d44db6154 100644 > --- a/crypto/testmgr.c > +++ b/crypto/testmgr.c > @@ -1664,7 +1664,8 @@ static int test_hash_vs_generic_impl(const char *generic_driver, > if (strcmp(generic_driver, driver) == 0) /* Already the generic impl? */ > return 0; > > - generic_tfm = crypto_alloc_shash(generic_driver, 0, 0); > + generic_tfm = crypto_alloc_shash(generic_driver, > + CRYPTO_ALG_FIPS_INTERNAL, 0); > if (IS_ERR(generic_tfm)) { > err = PTR_ERR(generic_tfm); > if (err == -ENOENT) { > @@ -2387,7 +2388,8 @@ static int test_aead_vs_generic_impl(struct aead_extra_tests_ctx *ctx) > if (strcmp(generic_driver, driver) == 0) /* Already the generic impl? */ > return 0; > > - generic_tfm = crypto_alloc_aead(generic_driver, 0, 0); > + generic_tfm = crypto_alloc_aead(generic_driver, > + CRYPTO_ALG_FIPS_INTERNAL, 0); > if (IS_ERR(generic_tfm)) { > err = PTR_ERR(generic_tfm); > if (err == -ENOENT) { > @@ -2986,7 +2988,8 @@ static int test_skcipher_vs_generic_impl(const char *generic_driver, > if (strcmp(generic_driver, driver) == 0) /* Already the generic impl? */ > return 0; > > - generic_tfm = crypto_alloc_skcipher(generic_driver, 0, 0); > + generic_tfm = crypto_alloc_skcipher(generic_driver, > + CRYPTO_ALG_FIPS_INTERNAL, 0); > if (IS_ERR(generic_tfm)) { > err = PTR_ERR(generic_tfm); > if (err == -ENOENT) { > @@ -5328,7 +5331,6 @@ static const struct alg_test_desc alg_test_descs[] = { > }, { > .alg = "sha1", > .test = alg_test_hash, > - .fips_allowed = 1, > .suite = { > .hash = __VECS(sha1_tv_template) > } > @@ -5613,6 +5615,13 @@ static int alg_find_test(const char *alg) > return -1; > } > > +static int alg_fips_disabled(const char *driver, const char *alg) > +{ > + pr_info("alg: %s (%s) is disabled due to FIPS\n", alg, driver); > + > + return -ECANCELED; > +} > + > int alg_test(const char *driver, const char *alg, u32 type, u32 mask) > { > int i; > @@ -5637,9 +5646,6 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask) > if (i < 0) > goto notest; > > - if (fips_enabled && !alg_test_descs[i].fips_allowed) > - goto non_fips_alg; > - > rc = alg_test_cipher(alg_test_descs + i, driver, type, mask); > goto test_done; > } > @@ -5649,8 +5655,7 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask) > if (i < 0 && j < 0) > goto notest; > > - if (fips_enabled && ((i >= 0 && !alg_test_descs[i].fips_allowed) || > - (j >= 0 && !alg_test_descs[j].fips_allowed))) > + if (fips_enabled && (j >= 0 && !alg_test_descs[j].fips_allowed)) > goto non_fips_alg; > > rc = 0; > @@ -5671,16 +5676,22 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask) > } > WARN(1, "alg: self-tests for %s (%s) failed (rc=%d)", > driver, alg, rc); > - } else { > - if (fips_enabled) > - pr_info("alg: self-tests for %s (%s) passed\n", > - driver, alg); > + } else if (fips_enabled) { > + pr_info("alg: self-tests for %s (%s) passed\n", > + driver, alg); > + > + if (i >= 0 && !alg_test_descs[i].fips_allowed) > + rc = alg_fips_disabled(driver, alg); > } > > return rc; > > notest: > printk(KERN_INFO "alg: No test for %s (%s)\n", alg, driver); > + > + if (type & CRYPTO_ALG_FIPS_INTERNAL) > + return alg_fips_disabled(driver, alg); > + > return 0; > non_fips_alg: > return -EINVAL; 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... Many thanks again! Nicolai > diff --git a/include/linux/crypto.h b/include/linux/crypto.h > index 855869e1fd32..df3f68dfe8c7 100644 > --- a/include/linux/crypto.h > +++ b/include/linux/crypto.h > @@ -132,6 +132,15 @@ > */ > #define CRYPTO_ALG_ALLOCATES_MEMORY 0x00010000 > > +/* > + * Mark an algorithm as a service implementation only usable by a > + * template and never by a normal user of the kernel crypto API. > + * This is intended to be used by algorithms that are themselves > + * not FIPS-approved but may instead be used to implement parts of > + * a FIPS-approved algorithm (e.g., sha1 vs. hmac(sha1)). > + */ > +#define CRYPTO_ALG_FIPS_INTERNAL 0x00020000 > + > /* > * Transform masks and values (for crt_flags). > */
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. > 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? 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. > 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. > 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? Thanks,
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
Am Freitag, 14. Januar 2022, 11:55:26 CET schrieb Herbert Xu: Hi Herbert, > > 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 SHA-1 is approved in all use cases except signatures. Ciao Stephan
On Fri, 2022-01-14 at 13:35 +0100, Stephan Mueller wrote: > Am Freitag, 14. Januar 2022, 11:55:26 CET schrieb Herbert Xu: > > Hi Herbert, > > > > 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 > > SHA-1 is approved in all use cases except signatures. Actually, even that's not quite true: you can't use it in a FIPS compliant system to *generate* signatures, but you can still use it in a FIPS compliant system to verify legacy signatures (signatures created before sha-1 was deprecated). It's still also completely acceptable as a hash for HMAC. The supporting document is this one: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf The bottom line is removing SHA-1 to achieve "FIPS compliance" is the wrong approach. You just have to make sure you can never use it to generate signatures. James
Am Freitag, 14. Januar 2022, 11:55:26 CET schrieb Herbert Xu: Hi Herbert, > > > 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? As we consider FIPS 140-3, we can allow a "degrated mode of operation". A degraded mode of operation disables only the algorithm that caused the failure. With a failing self test and not having a panic(), the offending algorithm implementation will not be available to the kernel crypto API and thus to a user. In this case, we can replace the panic with a graceful error. If that change is applied, I would like to mention to anybody that wants to backport the change: this change is not appropriate for FIPS 140-2. Ciao Stephan
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.
Am Freitag, 28. Januar 2022, 15:14:39 CET schrieb Nicolai Stange: Hi Nicolai, > 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? Are we sure that we always will have power-up tests of the compound algorithms when we disable the lower-level algorithm testing? For example, consider the DH work you are preparing: we currently have a self test for dh - which then will be marked as FIPS_INTERNAL and not executed. Would we now have self tests for modpXXX(dh) or ffdheXXX(dh)? If not, how would it be guaranteed that DH is tested? The important part is that the algorithm testing is guaranteed. I see a number of alg_test_null in testmgr.c. I see the potential that some algorithms do not get tested at all when we skip FIPS_INTERNAL algorithms. From a FIPS perspective it is permissible that compound algo power up tests are claimed to cover respective lower-level algos. > > 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. Ciao Stephan
Hi Stephan, Stephan Mueller <smueller@chronox.de> writes: > Am Freitag, 28. Januar 2022, 15:14:39 CET schrieb Nicolai Stange: > >> 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? > > Are we sure that we always will have power-up tests of the compound algorithms > when we disable the lower-level algorithm testing? Yes. The compound algorithms having ->fips_allowed == 1 and alg_test_null entries are: authenc(hmac(sha1),ctr(aes)) authenc(hmac(sha1),rfc3686(ctr(aes))) authenc(hmac(sha256),ctr(aes)) authenc(hmac(sha256),rfc3686(ctr(aes))) authenc(hmac(sha384),ctr(aes)) authenc(hmac(sha384),rfc3686(ctr(aes))) authenc(hmac(sha512),ctr(aes)) authenc(hmac(sha512),rfc3686(ctr(aes))) The hmac(sha*), ctr(aes) and rfc3686(ctr(aes)) all have ->fips_allowed == 1 and proper non-alg_test_null test entries. So no change here. cbc(paes) ctr(paes) ecb(paes) ofb(paes) xts(paes) xts4096(paes) xts512(paes) cts(cbc(paes)) As ecb(paes) has only a alg_test_null() entry, no test would have been performed at all before this change for these. So no change here either. ecb(cipher_null) No change here either. pkcs1pad(rsa,sha224) pkcs1pad(rsa,sha384) pkcs1pad(rsa,sha512) The sha* and rsa all have ->fips_allowed == 1 and proper non-alg_test_null test entries. So no change here. > > For example, consider the DH work you are preparing: we currently have a self > test for dh - which then will be marked as FIPS_INTERNAL and not executed. > Would we now have self tests for modpXXX(dh) or ffdheXXX(dh)? Yes, exactly. > If not, how would it be guaranteed that DH is tested? > > The important part is that the algorithm testing is guaranteed. I see a number > of alg_test_null in testmgr.c. I see the potential that some algorithms do not > get tested at all when we skip FIPS_INTERNAL algorithms. See above. But of course one needs to be careful not to add ->fips_allowed + alg_test_null entries for compound algorithms where any of the template arguments isn't approved in the future. > From a FIPS perspective it is permissible that compound algo power up tests > are claimed to cover respective lower-level algos. Perfect. Thanks, Nicolai
diff --git a/crypto/algapi.c b/crypto/algapi.c index a366cb3e8aa1..09fb75806e87 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -322,8 +322,16 @@ void crypto_alg_tested(const char *name, int err) found: q->cra_flags |= CRYPTO_ALG_DEAD; alg = test->adult; - if (err || list_empty(&alg->cra_list)) + + if (list_empty(&alg->cra_list)) + goto complete; + + if (err == -ECANCELED) + alg->cra_flags |= CRYPTO_ALG_FIPS_INTERNAL; + else if (err) goto complete; + else + alg->cra_flags &= ~CRYPTO_ALG_FIPS_INTERNAL; alg->cra_flags |= CRYPTO_ALG_TESTED; 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); else if (!crypto_mod_get(alg)) alg = ERR_PTR(-EAGAIN); crypto_mod_put(&larval->alg); @@ -233,6 +235,7 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg) static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type, u32 mask) { + const u32 fips = CRYPTO_ALG_FIPS_INTERNAL; struct crypto_alg *alg; u32 test = 0; @@ -240,8 +243,20 @@ static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type, test |= CRYPTO_ALG_TESTED; down_read(&crypto_alg_sem); - alg = __crypto_alg_lookup(name, type | test, mask | test); - if (!alg && test) { + alg = __crypto_alg_lookup(name, (type | test) & ~fips, + (mask | test) & ~fips); + if (alg) { + if (((type | mask) ^ fips) & fips) + mask |= fips; + mask &= fips; + + if (!crypto_is_larval(alg) && + ((type ^ alg->cra_flags) & mask)) { + /* Algorithm is disallowed in FIPS mode. */ + crypto_mod_put(alg); + alg = ERR_PTR(-ENOENT); + } + } else if (test) { alg = __crypto_alg_lookup(name, type, mask); if (alg && !crypto_is_larval(alg)) { /* Test failed */ diff --git a/crypto/hmac.c b/crypto/hmac.c index 25856aa7ccbf..af82e3eeb7d0 100644 --- a/crypto/hmac.c +++ b/crypto/hmac.c @@ -169,6 +169,7 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) struct crypto_alg *alg; struct shash_alg *salg; u32 mask; + u32 type; int err; int ds; int ss; @@ -182,8 +183,9 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) return -ENOMEM; spawn = shash_instance_ctx(inst); + type = CRYPTO_ALG_FIPS_INTERNAL; err = crypto_grab_shash(spawn, shash_crypto_instance(inst), - crypto_attr_alg_name(tb[1]), 0, mask); + crypto_attr_alg_name(tb[1]), type, mask); if (err) goto err_free_inst; salg = crypto_spawn_shash_alg(spawn); @@ -204,6 +206,7 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) if (err) goto err_free_inst; + inst->alg.base.cra_flags = alg->cra_flags & CRYPTO_ALG_FIPS_INTERNAL; inst->alg.base.cra_priority = alg->cra_priority; inst->alg.base.cra_blocksize = alg->cra_blocksize; inst->alg.base.cra_alignmask = alg->cra_alignmask; diff --git a/crypto/testmgr.c b/crypto/testmgr.c index 5831d4bbc64f..995d44db6154 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -1664,7 +1664,8 @@ static int test_hash_vs_generic_impl(const char *generic_driver, if (strcmp(generic_driver, driver) == 0) /* Already the generic impl? */ return 0; - generic_tfm = crypto_alloc_shash(generic_driver, 0, 0); + generic_tfm = crypto_alloc_shash(generic_driver, + CRYPTO_ALG_FIPS_INTERNAL, 0); if (IS_ERR(generic_tfm)) { err = PTR_ERR(generic_tfm); if (err == -ENOENT) { @@ -2387,7 +2388,8 @@ static int test_aead_vs_generic_impl(struct aead_extra_tests_ctx *ctx) if (strcmp(generic_driver, driver) == 0) /* Already the generic impl? */ return 0; - generic_tfm = crypto_alloc_aead(generic_driver, 0, 0); + generic_tfm = crypto_alloc_aead(generic_driver, + CRYPTO_ALG_FIPS_INTERNAL, 0); if (IS_ERR(generic_tfm)) { err = PTR_ERR(generic_tfm); if (err == -ENOENT) { @@ -2986,7 +2988,8 @@ static int test_skcipher_vs_generic_impl(const char *generic_driver, if (strcmp(generic_driver, driver) == 0) /* Already the generic impl? */ return 0; - generic_tfm = crypto_alloc_skcipher(generic_driver, 0, 0); + generic_tfm = crypto_alloc_skcipher(generic_driver, + CRYPTO_ALG_FIPS_INTERNAL, 0); if (IS_ERR(generic_tfm)) { err = PTR_ERR(generic_tfm); if (err == -ENOENT) { @@ -5328,7 +5331,6 @@ static const struct alg_test_desc alg_test_descs[] = { }, { .alg = "sha1", .test = alg_test_hash, - .fips_allowed = 1, .suite = { .hash = __VECS(sha1_tv_template) } @@ -5613,6 +5615,13 @@ static int alg_find_test(const char *alg) return -1; } +static int alg_fips_disabled(const char *driver, const char *alg) +{ + pr_info("alg: %s (%s) is disabled due to FIPS\n", alg, driver); + + return -ECANCELED; +} + int alg_test(const char *driver, const char *alg, u32 type, u32 mask) { int i; @@ -5637,9 +5646,6 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask) if (i < 0) goto notest; - if (fips_enabled && !alg_test_descs[i].fips_allowed) - goto non_fips_alg; - rc = alg_test_cipher(alg_test_descs + i, driver, type, mask); goto test_done; } @@ -5649,8 +5655,7 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask) if (i < 0 && j < 0) goto notest; - if (fips_enabled && ((i >= 0 && !alg_test_descs[i].fips_allowed) || - (j >= 0 && !alg_test_descs[j].fips_allowed))) + if (fips_enabled && (j >= 0 && !alg_test_descs[j].fips_allowed)) goto non_fips_alg; rc = 0; @@ -5671,16 +5676,22 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask) } WARN(1, "alg: self-tests for %s (%s) failed (rc=%d)", driver, alg, rc); - } else { - if (fips_enabled) - pr_info("alg: self-tests for %s (%s) passed\n", - driver, alg); + } else if (fips_enabled) { + pr_info("alg: self-tests for %s (%s) passed\n", + driver, alg); + + if (i >= 0 && !alg_test_descs[i].fips_allowed) + rc = alg_fips_disabled(driver, alg); } return rc; notest: printk(KERN_INFO "alg: No test for %s (%s)\n", alg, driver); + + if (type & CRYPTO_ALG_FIPS_INTERNAL) + return alg_fips_disabled(driver, alg); + return 0; non_fips_alg: return -EINVAL; diff --git a/include/linux/crypto.h b/include/linux/crypto.h index 855869e1fd32..df3f68dfe8c7 100644 --- a/include/linux/crypto.h +++ b/include/linux/crypto.h @@ -132,6 +132,15 @@ */ #define CRYPTO_ALG_ALLOCATES_MEMORY 0x00010000 +/* + * Mark an algorithm as a service implementation only usable by a + * template and never by a normal user of the kernel crypto API. + * This is intended to be used by algorithms that are themselves + * not FIPS-approved but may instead be used to implement parts of + * a FIPS-approved algorithm (e.g., sha1 vs. hmac(sha1)). + */ +#define CRYPTO_ALG_FIPS_INTERNAL 0x00020000 + /* * Transform masks and values (for crt_flags). */