| Message ID | 20211209090358.28231-4-nstange@suse.de (mailing list archive) |
|---|---|
| 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,
Am Mittwoch, 29. Dezember 2021, 03:14:41 CET schrieb Herbert Xu: Hi Herbert, > 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. I fully agree with you. However, there is a small wrinkle we should consider. In FIPS mode, we must only allow DH together with the safe primes provided by the templates of ffdheXX and modpXX. This means in FIPS mode, invoking the algo of "dh" should not be possible. Yet, on the other hand, we cannot mark "dh" as fips_allowed == 0 as the templates would not be able to instantiate them. Therefore, I think we should mark "dh" as CRYPTO_ALG_INTERNAL if in FIPS mode. To do so, I would think that dh_init should contain something like: if (fips_enabled) dh.base.cra_flags |= CRYPTO_ALG_INTERNAL; When encapsulating this small flag setting code into a helper function (just like xts_check_key or xts_verify_key) this helper can be added to other / new DH implementations QAT to make them FIPS-compliant. This would be the same approach as we currently take for XTS where each XTS implementation must have a callback to xts_check_key. Marking "dh" as INTERNAL implies it cannot be allocated in FIPS mode. Some may think this is a small inconsistency as this algo is marked fips_allowed == 1. I personally think there is no inconsistency because DH *is* allowed, however with a small policy caveat (the requirement to use it with safe-primes). So, having "dh" with fips_allowed == 1 but marking it as INTERNAL should be also consistent. Yes, it is a small misuse of the INTERNAL flag. But the alternative would be to create a "__dh" implementation that is wrapped by "dh". In turn this implies a big churn as we would need to touch all drivers implementing "dh". > > Cheers, Ciao Stephan
On Thu, Jan 06, 2022 at 03:30:04PM +0100, Stephan Mueller wrote: > > This means in FIPS mode, invoking the algo of "dh" should not be possible. > Yet, on the other hand, we cannot mark "dh" as fips_allowed == 0 as the > templates would not be able to instantiate them. Right, we have exactly the same problem with sha1 where sha1 per se should be not be allowed in FIPS mode but hmac(sha1) should be. > Therefore, I think we should mark "dh" as CRYPTO_ALG_INTERNAL if in FIPS mode. I think the annotation should be added to testmgr.c. We could mark dh and sha1 as not fips_allowed but allowed as the parameter of a template. This could then be represented in the crypto_alg object by a new flag. This flag could then be set automatically in crypto_grab_* to allow them to be picked up automatically for templates. 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. Cheers,
Herbert Xu <herbert@gondor.apana.org.au> writes: > On Thu, Jan 06, 2022 at 03:30:04PM +0100, Stephan Mueller wrote: >> >> This means in FIPS mode, invoking the algo of "dh" should not be possible. >> Yet, on the other hand, we cannot mark "dh" as fips_allowed == 0 as the >> templates would not be able to instantiate them. > > Right, we have exactly the same problem with sha1 where sha1 > per se should be not be allowed in FIPS mode but hmac(sha1) > should be. > >> Therefore, I think we should mark "dh" as CRYPTO_ALG_INTERNAL if in FIPS mode. > I think the annotation should be added to testmgr.c. We could > mark dh and sha1 as not fips_allowed but allowed as the parameter > of a template. This could then be represented in the crypto_alg > object by a new flag. > > This flag could then be set automatically in crypto_grab_* to > allow them to be picked up automatically for templates. > > 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. Perfect, this will solve my problem with how to handle "dh" vs. fips_enabled quite nicely. Many thanks! Nicolai
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
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(-)