| Message ID | 20211209090358.28231-4-nstange@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: dh - infrastructure for NVM in-band auth and FIPS conformance | expand |
On 12/9/21 10:03 AM, Nicolai Stange wrote: > DH users are supposed to set a struct dh instance's ->p and ->g domain > parameters (as well as the secret ->key), serialize the whole struct dh > instance via the crypto_dh_encode_key() helper and pass the encoded blob > on to the DH's ->set_secret(). All three currently available DH > implementations (generic, drivers/crypto/hisilicon/hpre/ and > drivers/crypto/qat/) would then proceed to call the crypto_dh_decode_key() > helper for unwrapping the encoded struct dh instance again. > > Up to now, the only DH user has been the keyctl(KEYCTL_DH_COMPUTE) syscall > and thus, all domain parameters have been coming from userspace. The domain > parameter encoding scheme for DH's ->set_secret() has been a perfectly > reasonable approach in this setting and the potential extra copy of ->p > and ->g during the encoding phase didn't harm much. > > However, recently, the need for working with the well-known safe-prime > groups' domain parameters from RFC 3526 and RFC 7919 resp. arose from two > independent developments: > - The NVME in-band authentication support currently being worked on ([1]) > needs to install the RFC 7919 ffdhe groups' domain parameters for DH > tfms. > - In FIPS mode, there's effectively no sensible way for the DH > implementation to conform to SP800-56Arev3 other than rejecting any > parameter set not corresponding to some approved safe-prime group > specified in either of these two RFCs. > > As the ->p arrays' lengths are in the range from 256 to 1024 bytes, it > would be nice if that extra copy during the crypto_dh_encode_key() step > from the NVME in-band authentication code could be avoided. Likewise, it > would be great if the DH implementation's FIPS handling code could avoid > attempting to match the input ->p and ->g against the individual approved > groups' parameters via memcmp() if it's known in advance that the input > corresponds to such one, as is the case for NVME. > > Introduce a enum dh_group_id for referring to any of the safe-prime groups > known to the kernel. The introduction of actual such safe-prime groups > alongside with their resp. P and G parameters will be deferred to later > patches. As of now, the new enum contains only a single member, > DH_GROUP_ID_UNKNOWN, which is meant to be associated with parameter sets > not corresponding to any of the groups known to the kernel, as is needed > to continue to support the current keyctl(KEYCTL_DH_COMPUTE) syscall > semantics. > > Add a new 'group_id' member of type enum group_id to struct dh. Make > crypto_dh_encode_key() include it in the serialization and to encode > ->p and ->g only if it equals DH_GROUP_ID_UNKNOWN. For all other possible > values of the encoded ->group_id, the receiving decoding primitive, > crypto_dh_decode_key(), is made to not decode ->p and ->g from the encoded > data, but to look them up in a central registry instead. > > The intended usage pattern is that users like NVME wouldn't set any of > the struct dh's ->p or ->g directly, but only the ->group_id for the group > they're interested in. They'd then proceed as usual and call > crypto_dh_encode_key() on the struct dh instance, pass the encoded result > on to DH's ->set_secret() and the latter would then invoke > crypto_dh_decode_key(), which would then in turn lookup the parameters > associated with the passed ->group_id. > > Note that this will avoid the extra copy of the ->p and ->g for the groups > (to be made) known to the kernel and also, that a future patch can easily > introduce a validation of ->group_id if in FIPS mode. > > As mentioned above, the introduction of actual safe-prime groups will be > deferred to later patches, so for now, only introduce an empty placeholder > array safe_prime_groups[] to be queried by crypto_dh_decode_key() for > domain parameters associated with a given ->group_id as outlined above. > Make its elements to be of the new internal struct safe_prime_group type. > Among the members ->group_id, ->p and ->p_size with obvious meaning, there > will also be a ->max_strength member for storing the maximum security > strength supported by the associated group -- its value will be needed for > the upcoming private key generation support. > > Finally, update the encoded secrets provided by the testmgr's DH test > vectors in order to account for the additional ->group_id field expected > by crypto_dh_decode_key() now. > > [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de > > Signed-off-by: Nicolai Stange <nstange@suse.de> > --- > crypto/dh_helper.c | 90 ++++++++++++++++++++++++++++++++++++--------- > crypto/testmgr.h | 16 +++++--- > include/crypto/dh.h | 6 +++ > 3 files changed, 88 insertions(+), 24 deletions(-) > > diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c > index aabc91e4f63f..9f21204e5dee 100644 > --- a/crypto/dh_helper.c > +++ b/crypto/dh_helper.c > @@ -10,7 +10,31 @@ > #include <crypto/dh.h> > #include <crypto/kpp.h> > > -#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 3 * sizeof(int)) > +#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 4 * sizeof(int)) > + > +static const struct safe_prime_group > +{ > + enum dh_group_id group_id; > + unsigned int max_strength; > + unsigned int p_size; > + const char *p; > +} safe_prime_groups[] = {}; > + > +/* 2 is used as a generator for all safe-prime groups. */ > +static const char safe_prime_group_g[] = { 2 }; > + > +static inline const struct safe_prime_group * > +get_safe_prime_group(enum dh_group_id group_id) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(safe_prime_groups); ++i) { > + if (safe_prime_groups[i].group_id == group_id) > + return &safe_prime_groups[i]; > + } > + > + return NULL; > +} > > static inline u8 *dh_pack_data(u8 *dst, u8 *end, const void *src, size_t size) > { > @@ -28,7 +52,10 @@ static inline const u8 *dh_unpack_data(void *dst, const void *src, size_t size) > > static inline unsigned int dh_data_size(const struct dh *p) > { > - return p->key_size + p->p_size + p->g_size; > + if (p->group_id == DH_GROUP_ID_UNKNOWN) > + return p->key_size + p->p_size + p->g_size; > + else > + return p->key_size; > } > > unsigned int crypto_dh_key_len(const struct dh *p) > @@ -45,18 +72,24 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params) > .type = CRYPTO_KPP_SECRET_TYPE_DH, > .len = len > }; > + int group_id; > > if (unlikely(!len)) > return -EINVAL; > > ptr = dh_pack_data(ptr, end, &secret, sizeof(secret)); > + group_id = (int)params->group_id; > + ptr = dh_pack_data(ptr, end, &group_id, sizeof(group_id)); Me being picky again. To my knowledge, 'int' doesn't have a fixed width, but is rather only guaranteed to hold certain values. So as soon as one relies on any fixed size (as this one does) I tend to use fixed size type like 'u32' to make it absolutely clear what is to be expected here. But the I don't know the conventions in the crypto code; if an 'int' is assumed to be 32 bits throughout the crypto code I guess we should be fine. Cheers, Hannes
Hannes Reinecke <hare@suse.de> writes: > On 12/9/21 10:03 AM, Nicolai Stange wrote: >> diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c >> index aabc91e4f63f..9f21204e5dee 100644 >> --- a/crypto/dh_helper.c >> +++ b/crypto/dh_helper.c >> @@ -45,18 +72,24 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params) >> .type = CRYPTO_KPP_SECRET_TYPE_DH, >> .len = len >> }; >> + int group_id; >> >> if (unlikely(!len)) >> return -EINVAL; >> >> ptr = dh_pack_data(ptr, end, &secret, sizeof(secret)); >> + group_id = (int)params->group_id; >> + ptr = dh_pack_data(ptr, end, &group_id, sizeof(group_id)); > > Me being picky again. > To my knowledge, 'int' doesn't have a fixed width, but is rather only > guaranteed to hold certain values. > So as soon as one relies on any fixed size (as this one does) I tend to > use fixed size type like 'u32' to make it absolutely clear what is to be > expected here. > > But the I don't know the conventions in the crypto code; if an 'int' is > assumed to be 32 bits throughout the crypto code I guess we should be fine. Yes, I thought about this, too. However, the other, already existing fields like ->key_size and ->p_size are getting serialized as unsigned ints and I decided to stick to that for ->group_id as well. Except for the testmgr vectors, the encoding is internal to the crypto_dh_encode_key() and crypto_dh_decode_key() pair anyway -- all that would happen if sizeof(int) != 4 is that the tests would fail. So, IMO, making the serialization of struct dh to use u32 throughout is not really in scope for this series and would probably deserve a patch on its own, if desired. Thanks, Nicolai
On 12/13/21 11:06 AM, Nicolai Stange wrote: > Hannes Reinecke <hare@suse.de> writes: > >> On 12/9/21 10:03 AM, Nicolai Stange wrote: >>> diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c >>> index aabc91e4f63f..9f21204e5dee 100644 >>> --- a/crypto/dh_helper.c >>> +++ b/crypto/dh_helper.c >>> @@ -45,18 +72,24 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params) >>> .type = CRYPTO_KPP_SECRET_TYPE_DH, >>> .len = len >>> }; >>> + int group_id; >>> >>> if (unlikely(!len)) >>> return -EINVAL; >>> >>> ptr = dh_pack_data(ptr, end, &secret, sizeof(secret)); >>> + group_id = (int)params->group_id; >>> + ptr = dh_pack_data(ptr, end, &group_id, sizeof(group_id)); >> >> Me being picky again. >> To my knowledge, 'int' doesn't have a fixed width, but is rather only >> guaranteed to hold certain values. >> So as soon as one relies on any fixed size (as this one does) I tend to >> use fixed size type like 'u32' to make it absolutely clear what is to be >> expected here. >> >> But the I don't know the conventions in the crypto code; if an 'int' is >> assumed to be 32 bits throughout the crypto code I guess we should be fine. > > Yes, I thought about this, too. However, the other, already existing > fields like ->key_size and ->p_size are getting serialized as unsigned > ints and I decided to stick to that for ->group_id as well. Except for > the testmgr vectors, the encoding is internal to the > crypto_dh_encode_key() and crypto_dh_decode_key() pair anyway -- all > that would happen if sizeof(int) != 4 is that the tests would fail. > > So, IMO, making the serialization of struct dh to use u32 throughout is > not really in scope for this series and would probably deserve a patch > on its own, if desired. > As I thought. So that's okay, then. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On Thu, Dec 09, 2021 at 10:03:43AM +0100, Nicolai Stange wrote: >> diff --git a/include/crypto/dh.h b/include/crypto/dh.h > index 67f3f6bca527..f0ed899e2168 100644 > --- a/include/crypto/dh.h > +++ b/include/crypto/dh.h > @@ -19,6 +19,11 @@ > * the KPP API function call of crypto_kpp_set_secret. > */ > > +/** enum dh_group_id - identify well-known domain parameter sets */ > +enum dh_group_id { > + DH_GROUP_ID_UNKNOWN = 0, /* Constants are used in test vectors. */ > +}; We try to avoid hard-coded ID lists like these in the Crypto API. I've had a look at your subsequent patches and I don't think you really need this. For instance, instead of shoehorning this into "dh", you could instead create new kpp algorithms modpXXXX and ffdheXXXX which can be templates around the underlying dh algorithm. Sure this might involve a copy of the parameters but given the speed of the algorithms that we're talking about here I don't think it's really relevant. That way the underlying drivers don't need to be touched at all. Yes I do realise that this means the keyrings DH user-space API cannot be used in FIPS mode, but that is probably a good thing as users who care about modp/ffdhe shouldn't really have to stuff the raw vectors into this interface just to access the kernel DH implementation. On a side note, are there really keyrings DH users out there in the wild? If not can we deprecate and remove this interface completely? Cheers,
Hello Herbert, Herbert Xu <herbert@gondor.apana.org.au> writes: > On Thu, Dec 09, 2021 at 10:03:43AM +0100, Nicolai Stange wrote: >>> diff --git a/include/crypto/dh.h b/include/crypto/dh.h >> index 67f3f6bca527..f0ed899e2168 100644 >> --- a/include/crypto/dh.h >> +++ b/include/crypto/dh.h >> @@ -19,6 +19,11 @@ >> * the KPP API function call of crypto_kpp_set_secret. >> */ >> >> +/** enum dh_group_id - identify well-known domain parameter sets */ >> +enum dh_group_id { >> + DH_GROUP_ID_UNKNOWN = 0, /* Constants are used in test vectors. */ >> +}; > > We try to avoid hard-coded ID lists like these in the Crypto API. Just for my understanding: the problem here is having a (single) enum for the representation of all the possible "known" groups in the first place or more that the individual group id enum members have hard-coded values assigned to them each? > I've had a look at your subsequent patches and I don't think you > really need this. > > For instance, instead of shoehorning this into "dh", you could > instead create new kpp algorithms modpXXXX and ffdheXXXX which > can be templates around the underlying dh algorithm. FWIW, when implementing this, I considered aligning more to the ecdh API, namely to register separate algorithms for each dh group as you suggested above: "dh-ffdhe2048" etc. in analogy to "ecdh-nist-p192" and alike. The various (three in total) ecdh-nist-* kpp_alg variants differ only in their ->init(), which would all set ->curve_id to the corresponding ECC_CURVE_NIST_* constant as appropriate. However, after some back and forth, I opted against doing something similar for dh at the time, because there are quite some more possible parameter sets than there are for ecdh, namely ten vs. three. If we were to render the KEYCTL_DH_COMPUTE functionality unusable in FIPS mode alltogether (see below), I could drop the MODP* group support, but that would still leave me with the five FFDHE* kpp_alg variants I had to at least provide separate test vectors for. I think that these five TVs would be quite redundant as they'd all merely test the implementation of dh_set_secret() + dh_compute_value() with different input parameters. This might be acceptable though, I only wanted to bring it up. Anyway, just to make sure I'm getting it right: when you're saying "template", you mean to implement a crypto_template for instantiating patterns like "dh(ffdhe2048)", "dh(ffdhe3072)" and so on? The dh(...) template instantiations would keep a crypto_spawn for referring to the underlying, non-template "dh" kpp_alg so that "dh" implementations of higher priority (hpre + qat) would take over once they'd become available, correct? AFAICS, it would make sense to introduce something like struct kpp_instance, crypto_kpp_spawn and associated helpers as a prerequisite then. Which wouldn't be a problem by me, just saying that it's not there yet. I'm not sure there would be other potential users of such an infrastructure, perhaps Stephan's SP800-108 KDF ([1]) is a candidate? > Sure this might involve a copy of the parameters but given the speed > of the algorithms that we're talking about here I don't think it's > really relevant. Ok, understood. I'm by no means a FIPS expert, but I bet I'd still have to somehow convey a flag like "this group parameter P is approved" from the frontend template instantiations like "dh(ffdhe2048)" to the underlying "dh" implementation and make the latter reject any non-approved groups. That is, with my limited experience with FIPS, I'd expect that disabling the only known path to get non-approved parameters into "dh", i.e. KEYCTL_DH_COMPUTE, would not be sufficient for achieving conformance. Note that those dh_group_id's previously serialized via crypto_dh_encode_key() served this purpose, in addition to enabling that optimization of not copying the Ps when possible. I'm not really sure, but simply introducing a flag like ->fips_approved to struct dh and serializing that as an alternative would probably not work out, because it would in theory still allow potential "dh" users to set it (e.g. by accident) even when specifying non-approved Ps. Stephan? > > That way the underlying drivers don't need to be touched at all. FWIW, I touched the drivers only for consistency with ecdh and the related drivers/crypto implementations, which all invoke the privkey generation from their resp. ->set_secret(). As an alternative, I could have also made crypto_dh_encode_key() to generate an ephemeral key on the fly for input ->key_size == 0. This wouldn't be much different from how I'd imagine a dh(...) template based approach to work: its ->set_secret() would take a private key, if any, and - generate a private key if none has been specified, - kmalloc() a buffer for crypto_dh_encode_key(), - serialize the key, P and G for the underlying "dh" implementation via crypto_dh_encode_key(), - pass the encoded result onwards to the underlying "dh"'s ->set_secret() and - kfree() the buffer again. So instead of having the proposed template's ->set_secret() wrapper around crypto_dh_encode_key() to take care of generating ephemeral keys, I could have made the latter to do that as well, I think. > Yes I do realise that this means the keyrings DH user-space API > cannot be used in FIPS mode, but that is probably a good thing > as users who care about modp/ffdhe shouldn't really have to stuff > the raw vectors into this interface just to access the kernel DH > implementation. The sole purpose of introducing the MODP* parameters with this patchset had been to keep KEYCTL_DH_COMPUTE functional in FIPS mode to the extent possible: NVMe in-band authentication OTOH needs only FFHDE*. If it would be acceptable or even desirable to render KEYCTL_DH_COMPUTE unusable in FIPS mode, I'd drop the MODP* related patches. However, it would perhaps be fair to reflect KEYCTL_DH_COMPUTE's new dependency on !fips_enabled in keyctl_capabilities() then, but this can probably be done with a separate follow-up patch. > > On a side note, are there really keyrings DH users out there in > the wild? If not can we deprecate and remove this interface > completely? I wondered the same when first looking into this -- a web search returned the Embedded Linux library ([2]), which seems to rely on KEYCTL_DH_COMPUTE for implementing TLS (web servers for embedded devices?). Then there's the keyctl(1) utility, which exposes this interface via its "dh_compute" subcommand. Lastly, there exist some Rust and Go bindings also -- hard to say if anything is using those. Thanks! Nicolai [1] https://lore.kernel.org/r/4642773.OV4Wx5bFTl@positron.chronox.de [2] https://git.kernel.org/pub/scm/libs/ell/ell.git/
On Mon, Dec 20, 2021 at 04:27:35PM +0100, Nicolai Stange wrote: > > Just for my understanding: the problem here is having a (single) enum > for the representation of all the possible "known" groups in the first > place or more that the individual group id enum members have hard-coded > values assigned to them each? Yes the fact that you need to have a list of all "known" groups is the issue. > However, after some back and forth, I opted against doing something > similar for dh at the time, because there are quite some more possible > parameter sets than there are for ecdh, namely ten vs. three. If we were I don't understand why we can't support ten or an even larger number of parameter sets. If you are concerned about code duplication then there are ways around that. Or do you have another specific concern in mind with respect to a large number of parameter sets under this scheme? > Anyway, just to make sure I'm getting it right: when you're saying > "template", you mean to implement a crypto_template for instantiating > patterns like "dh(ffdhe2048)", "dh(ffdhe3072)" and so on? The dh(...) > template instantiations would keep a crypto_spawn for referring to the > underlying, non-template "dh" kpp_alg so that "dh" implementations of > higher priority (hpre + qat) would take over once they'd become > available, correct? The template would work in the other dirirection. It would look like ffdhe2048(dh) with dh being implemented in either software or hardware. The template wrapper would simply supply the relevant parameters. Cheers,
Herbert Xu <herbert@gondor.apana.org.au> writes: > On Mon, Dec 20, 2021 at 04:27:35PM +0100, Nicolai Stange wrote: >> >> Just for my understanding: the problem here is having a (single) enum >> for the representation of all the possible "known" groups in the first >> place or more that the individual group id enum members have hard-coded >> values assigned to them each? > > Yes the fact that you need to have a list of all "known" groups is > the issue. Ok, understood. Thanks for the clarification. >> However, after some back and forth, I opted against doing something >> similar for dh at the time, because there are quite some more possible >> parameter sets than there are for ecdh, namely ten vs. three. If we were > > I don't understand why we can't support ten or an even larger > number of parameter sets. There's no real reason. I just didn't dare to promote what I considered mere input parameter sets to full-fledged crypto_alg instances with their associated overhead each: - the global crypto_alg_list will get longer, which might have an impact on the lookup searches, - every ffdheXYZ(dh) template instance will need to have individual TVs associated with it. However, I take it as that's fine and I'd be more than happy to implement the ffhdheXYZ(dh) template approach you suggested in a v3. > > If you are concerned about code duplication then there are ways > around that. Or do you have another specific concern in mind > with respect to a large number of parameter sets under this scheme? > >> Anyway, just to make sure I'm getting it right: when you're saying >> "template", you mean to implement a crypto_template for instantiating >> patterns like "dh(ffdhe2048)", "dh(ffdhe3072)" and so on? The dh(...) >> template instantiations would keep a crypto_spawn for referring to the >> underlying, non-template "dh" kpp_alg so that "dh" implementations of >> higher priority (hpre + qat) would take over once they'd become >> available, correct? > > The template would work in the other dirirection. It would look > like ffdhe2048(dh) with dh being implemented in either software or > hardware. > > The template wrapper would simply supply the relevant parameters. Makes sense. Thanks! Nicolai
Hi Herbert, Herbert Xu <herbert@gondor.apana.org.au> writes: > On Fri, Jan 07, 2022 at 01:44:34PM +1100, Herbert Xu wrote: >> >> I'm already writing this up for sha1 anyway so let me polish it >> off and I'll post it soon which you can then reuse it for dh. > > Here is something that seems to work for sha1/hmac. Please let > me know if you see any issues with this approach for dh. > > Thanks, > > ---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. > > 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). I haven't tried, but wouldn't this allow the instantiation of e.g. hmac(blake2s-256) in FIPS mode? Thanks, Nicolai > > 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> >
On Tue, Jan 11, 2022 at 08:50:18AM +0100, Nicolai Stange wrote: > > I haven't tried, but wouldn't this allow the instantiation of e.g. > hmac(blake2s-256) in FIPS mode? 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. Thanks,
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, 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.
diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c index aabc91e4f63f..9f21204e5dee 100644 --- a/crypto/dh_helper.c +++ b/crypto/dh_helper.c @@ -10,7 +10,31 @@ #include <crypto/dh.h> #include <crypto/kpp.h> -#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 3 * sizeof(int)) +#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 4 * sizeof(int)) + +static const struct safe_prime_group +{ + enum dh_group_id group_id; + unsigned int max_strength; + unsigned int p_size; + const char *p; +} safe_prime_groups[] = {}; + +/* 2 is used as a generator for all safe-prime groups. */ +static const char safe_prime_group_g[] = { 2 }; + +static inline const struct safe_prime_group * +get_safe_prime_group(enum dh_group_id group_id) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(safe_prime_groups); ++i) { + if (safe_prime_groups[i].group_id == group_id) + return &safe_prime_groups[i]; + } + + return NULL; +} static inline u8 *dh_pack_data(u8 *dst, u8 *end, const void *src, size_t size) { @@ -28,7 +52,10 @@ static inline const u8 *dh_unpack_data(void *dst, const void *src, size_t size) static inline unsigned int dh_data_size(const struct dh *p) { - return p->key_size + p->p_size + p->g_size; + if (p->group_id == DH_GROUP_ID_UNKNOWN) + return p->key_size + p->p_size + p->g_size; + else + return p->key_size; } unsigned int crypto_dh_key_len(const struct dh *p) @@ -45,18 +72,24 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params) .type = CRYPTO_KPP_SECRET_TYPE_DH, .len = len }; + int group_id; if (unlikely(!len)) return -EINVAL; ptr = dh_pack_data(ptr, end, &secret, sizeof(secret)); + group_id = (int)params->group_id; + ptr = dh_pack_data(ptr, end, &group_id, sizeof(group_id)); ptr = dh_pack_data(ptr, end, ¶ms->key_size, sizeof(params->key_size)); ptr = dh_pack_data(ptr, end, ¶ms->p_size, sizeof(params->p_size)); ptr = dh_pack_data(ptr, end, ¶ms->g_size, sizeof(params->g_size)); ptr = dh_pack_data(ptr, end, params->key, params->key_size); - ptr = dh_pack_data(ptr, end, params->p, params->p_size); - ptr = dh_pack_data(ptr, end, params->g, params->g_size); + if (params->group_id == DH_GROUP_ID_UNKNOWN) { + ptr = dh_pack_data(ptr, end, params->p, params->p_size); + ptr = dh_pack_data(ptr, end, params->g, params->g_size); + } + if (ptr != end) return -EINVAL; return 0; @@ -67,6 +100,7 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh *params) { const u8 *ptr = buf; struct kpp_secret secret; + int group_id; if (unlikely(!buf || len < DH_KPP_SECRET_MIN_SIZE)) return -EINVAL; @@ -75,12 +109,46 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh *params) if (secret.type != CRYPTO_KPP_SECRET_TYPE_DH) return -EINVAL; + ptr = dh_unpack_data(&group_id, ptr, sizeof(group_id)); + params->group_id = (enum dh_group_id)group_id; ptr = dh_unpack_data(¶ms->key_size, ptr, sizeof(params->key_size)); ptr = dh_unpack_data(¶ms->p_size, ptr, sizeof(params->p_size)); ptr = dh_unpack_data(¶ms->g_size, ptr, sizeof(params->g_size)); if (secret.len != crypto_dh_key_len(params)) return -EINVAL; + if (params->group_id == DH_GROUP_ID_UNKNOWN) { + /* Don't allocate memory. Set pointers to data within + * the given buffer + */ + params->key = (void *)ptr; + params->p = (void *)(ptr + params->key_size); + params->g = (void *)(ptr + params->key_size + params->p_size); + + /* + * Don't permit 'p' to be 0. It's not a prime number, + * and it's subject to corner cases such as 'mod 0' + * being undefined or crypto_kpp_maxsize() returning + * 0. + */ + if (memchr_inv(params->p, 0, params->p_size) == NULL) + return -EINVAL; + + } else { + const struct safe_prime_group *g; + + g = get_safe_prime_group(params->group_id); + if (!g) + return -EINVAL; + + params->key = (void *)ptr; + + params->p = g->p; + params->p_size = g->p_size; + params->g = safe_prime_group_g; + params->g_size = sizeof(safe_prime_group_g); + } + /* * Don't permit the buffer for 'key' or 'g' to be larger than 'p', since * some drivers assume otherwise. @@ -89,20 +157,6 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh *params) params->g_size > params->p_size) return -EINVAL; - /* Don't allocate memory. Set pointers to data within - * the given buffer - */ - params->key = (void *)ptr; - params->p = (void *)(ptr + params->key_size); - params->g = (void *)(ptr + params->key_size + params->p_size); - - /* - * Don't permit 'p' to be 0. It's not a prime number, and it's subject - * to corner cases such as 'mod 0' being undefined or - * crypto_kpp_maxsize() returning 0. - */ - if (memchr_inv(params->p, 0, params->p_size) == NULL) - return -EINVAL; return 0; } diff --git a/crypto/testmgr.h b/crypto/testmgr.h index 7f7d5ae48721..637913064c64 100644 --- a/crypto/testmgr.h +++ b/crypto/testmgr.h @@ -1244,13 +1244,15 @@ static const struct kpp_testvec dh_tv_template[] = { .secret = #ifdef __LITTLE_ENDIAN "\x01\x00" /* type */ - "\x11\x02" /* len */ + "\x15\x02" /* len */ + "\x00\x00\x00\x00" /* group_id == DH_GROUP_ID_UNKNOWN */ "\x00\x01\x00\x00" /* key_size */ "\x00\x01\x00\x00" /* p_size */ "\x01\x00\x00\x00" /* g_size */ #else "\x00\x01" /* type */ - "\x02\x11" /* len */ + "\x02\x15" /* len */ + "\x00\x00\x00\x00" /* group_id == DH_GROUP_ID_UNKNOWN */ "\x00\x00\x01\x00" /* key_size */ "\x00\x00\x01\x00" /* p_size */ "\x00\x00\x00\x01" /* g_size */ @@ -1342,7 +1344,7 @@ static const struct kpp_testvec dh_tv_template[] = { "\xd3\x34\x49\xad\x64\xa6\xb1\xc0\x59\x28\x75\x60\xa7\x8a\xb0\x11" "\x56\x89\x42\x74\x11\xf5\xf6\x5e\x6f\x16\x54\x6a\xb1\x76\x4d\x50" "\x8a\x68\xc1\x5b\x82\xb9\x0d\x00\x32\x50\xed\x88\x87\x48\x92\x17", - .secret_size = 529, + .secret_size = 533, .b_public_size = 256, .expected_a_public_size = 256, .expected_ss_size = 256, @@ -1351,13 +1353,15 @@ static const struct kpp_testvec dh_tv_template[] = { .secret = #ifdef __LITTLE_ENDIAN "\x01\x00" /* type */ - "\x11\x02" /* len */ + "\x15\x02" /* len */ + "\x00\x00\x00\x00" /* group_id == DH_GROUP_ID_UNKNOWN */ "\x00\x01\x00\x00" /* key_size */ "\x00\x01\x00\x00" /* p_size */ "\x01\x00\x00\x00" /* g_size */ #else "\x00\x01" /* type */ - "\x02\x11" /* len */ + "\x02\x15" /* len */ + "\x00\x00\x00\x00" /* group_id == DH_GROUP_ID_UNKNOWN */ "\x00\x00\x01\x00" /* key_size */ "\x00\x00\x01\x00" /* p_size */ "\x00\x00\x00\x01" /* g_size */ @@ -1449,7 +1453,7 @@ static const struct kpp_testvec dh_tv_template[] = { "\x5e\x5a\x64\xbd\xf6\x85\x04\xe8\x28\x6a\xac\xef\xce\x19\x8e\x9a" "\xfe\x75\xc0\x27\x69\xe3\xb3\x7b\x21\xa7\xb1\x16\xa4\x85\x23\xee" "\xb0\x1b\x04\x6e\xbd\xab\x16\xde\xfd\x86\x6b\xa9\x95\xd7\x0b\xfd", - .secret_size = 529, + .secret_size = 533, .b_public_size = 256, .expected_a_public_size = 256, .expected_ss_size = 256, diff --git a/include/crypto/dh.h b/include/crypto/dh.h index 67f3f6bca527..f0ed899e2168 100644 --- a/include/crypto/dh.h +++ b/include/crypto/dh.h @@ -19,6 +19,11 @@ * the KPP API function call of crypto_kpp_set_secret. */ +/** enum dh_group_id - identify well-known domain parameter sets */ +enum dh_group_id { + DH_GROUP_ID_UNKNOWN = 0, /* Constants are used in test vectors. */ +}; + /** * struct dh - define a DH private key * @@ -30,6 +35,7 @@ * @g_size: Size of DH generator G */ struct dh { + enum dh_group_id group_id; const void *key; const void *p; const void *g;
DH users are supposed to set a struct dh instance's ->p and ->g domain parameters (as well as the secret ->key), serialize the whole struct dh instance via the crypto_dh_encode_key() helper and pass the encoded blob on to the DH's ->set_secret(). All three currently available DH implementations (generic, drivers/crypto/hisilicon/hpre/ and drivers/crypto/qat/) would then proceed to call the crypto_dh_decode_key() helper for unwrapping the encoded struct dh instance again. Up to now, the only DH user has been the keyctl(KEYCTL_DH_COMPUTE) syscall and thus, all domain parameters have been coming from userspace. The domain parameter encoding scheme for DH's ->set_secret() has been a perfectly reasonable approach in this setting and the potential extra copy of ->p and ->g during the encoding phase didn't harm much. However, recently, the need for working with the well-known safe-prime groups' domain parameters from RFC 3526 and RFC 7919 resp. arose from two independent developments: - The NVME in-band authentication support currently being worked on ([1]) needs to install the RFC 7919 ffdhe groups' domain parameters for DH tfms. - In FIPS mode, there's effectively no sensible way for the DH implementation to conform to SP800-56Arev3 other than rejecting any parameter set not corresponding to some approved safe-prime group specified in either of these two RFCs. As the ->p arrays' lengths are in the range from 256 to 1024 bytes, it would be nice if that extra copy during the crypto_dh_encode_key() step from the NVME in-band authentication code could be avoided. Likewise, it would be great if the DH implementation's FIPS handling code could avoid attempting to match the input ->p and ->g against the individual approved groups' parameters via memcmp() if it's known in advance that the input corresponds to such one, as is the case for NVME. Introduce a enum dh_group_id for referring to any of the safe-prime groups known to the kernel. The introduction of actual such safe-prime groups alongside with their resp. P and G parameters will be deferred to later patches. As of now, the new enum contains only a single member, DH_GROUP_ID_UNKNOWN, which is meant to be associated with parameter sets not corresponding to any of the groups known to the kernel, as is needed to continue to support the current keyctl(KEYCTL_DH_COMPUTE) syscall semantics. Add a new 'group_id' member of type enum group_id to struct dh. Make crypto_dh_encode_key() include it in the serialization and to encode ->p and ->g only if it equals DH_GROUP_ID_UNKNOWN. For all other possible values of the encoded ->group_id, the receiving decoding primitive, crypto_dh_decode_key(), is made to not decode ->p and ->g from the encoded data, but to look them up in a central registry instead. The intended usage pattern is that users like NVME wouldn't set any of the struct dh's ->p or ->g directly, but only the ->group_id for the group they're interested in. They'd then proceed as usual and call crypto_dh_encode_key() on the struct dh instance, pass the encoded result on to DH's ->set_secret() and the latter would then invoke crypto_dh_decode_key(), which would then in turn lookup the parameters associated with the passed ->group_id. Note that this will avoid the extra copy of the ->p and ->g for the groups (to be made) known to the kernel and also, that a future patch can easily introduce a validation of ->group_id if in FIPS mode. As mentioned above, the introduction of actual safe-prime groups will be deferred to later patches, so for now, only introduce an empty placeholder array safe_prime_groups[] to be queried by crypto_dh_decode_key() for domain parameters associated with a given ->group_id as outlined above. Make its elements to be of the new internal struct safe_prime_group type. Among the members ->group_id, ->p and ->p_size with obvious meaning, there will also be a ->max_strength member for storing the maximum security strength supported by the associated group -- its value will be needed for the upcoming private key generation support. Finally, update the encoded secrets provided by the testmgr's DH test vectors in order to account for the additional ->group_id field expected by crypto_dh_decode_key() now. [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de Signed-off-by: Nicolai Stange <nstange@suse.de> --- crypto/dh_helper.c | 90 ++++++++++++++++++++++++++++++++++++--------- crypto/testmgr.h | 16 +++++--- include/crypto/dh.h | 6 +++ 3 files changed, 88 insertions(+), 24 deletions(-)