mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

Ad-hoc KDF for EC J-PAKE in TLS 1.2

Open AndrzejKurek opened this issue 3 years ago • 3 comments

Fixes https://github.com/Mbed-TLS/mbedtls/issues/5978. This PR adds an algorithm to prepare SHA256(K.X) for the use of TLS 1.2. Open questions:

  • Should the input type be restricted to only accept PSA_KEY_DERIVATION_INPUT_SECRET?
  • Should the operation capacity restriction be added to psa_key_derivation_setup_kdf?

Requires backport

NO - new feature

Requires ChangeLog entry

[mpg] Unsure - on one hand, that's a new feature, on the other hand, it's really only meant to be used in the library: the only use case is implementing TLS 1.2's ECJPAKE key exchange.

AndrzejKurek avatar Jul 20 '22 17:07 AndrzejKurek

Note: the ABI-API check outputs information that's expected and harmless (new field added in a union), so CI is as good as green.

mpg avatar Sep 12 '22 08:09 mpg

@mpg this solution requires a bit of changes in psa_key_derivation_setup_kdf to account for the lack of passed hash algorithm. Let me know if this approach is fine, and if yes - I'll proceed with minor things to add around - like a check in check_crypto_config.h, or a changelog entry.

AndrzejKurek avatar Sep 14 '22 12:09 AndrzejKurek

Added all requested changes apart from two: removal of one newline and a proposal of restricting the reading to one chunk of 32 bytes.

AndrzejKurek avatar Sep 16 '22 11:09 AndrzejKurek

Btw we also need to make a decision regarding the ChangeLog entry. I previously wrote:

Unsure - on one hand, that's a new feature, on the other hand, it's really only meant to be used in the library: the only use case is implementing TLS 1.2's ECJPAKE key exchange.

But now I'm thinking, if we consider this important enough to be included in the official PSA API, then surely this is also important enough for our ChangeLog. So, please add a ChangeLog entry unless someone disagrees.

mpg avatar Sep 27 '22 07:09 mpg

we should allow piecewise output

It doesn't have to gate this PR though. Only supporting a single output could be an acceptable temporary limitation, and then we could build a generic implementation of piecewise output from fixed-size-output KDF without the pressure of delivering ECJPAKE.

gilles-peskine-arm avatar Sep 27 '22 08:09 gilles-peskine-arm

It doesn't have to gate this PR though. Only supporting a single output could be an acceptable temporary limitation, and then we could build a generic implementation of piecewise output from fixed-size-output KDF without the pressure of delivering ECJPAKE.

Ok, good point, let's do this (unless @AndrzejKurek has already implemented it in the meantime). I believe you already created an issue to track that yesterday, so that's covered. I'll change my review to "approved" as soon as the ChangeLog entry is there.

mpg avatar Sep 27 '22 08:09 mpg

It doesn't have to gate this PR though. Only supporting a single output could be an acceptable temporary limitation, and then we could build a generic implementation of piecewise output from fixed-size-output KDF without the pressure of delivering ECJPAKE.

Ok, good point, let's do this (unless @AndrzejKurek has already implemented it in the meantime). I believe you already created an issue to track that yesterday, so that's covered. I'll change my review to "approved" as soon as the ChangeLog entry is there.

I was in the progress of doing it, but I'll just add a changelog entry instead. Thanks!

AndrzejKurek avatar Sep 27 '22 08:09 AndrzejKurek

The only failure here is the API/ABI check.

AndrzejKurek avatar Sep 28 '22 07:09 AndrzejKurek

I should probably have thought about it before hitting merge, but this needs side-porting to the PSA Crypto spec. @gilles-peskine-arm @athoelke can you remind me what the procedure is these days?

mpg avatar Oct 17 '22 08:10 mpg

@mpg The procedure is to file an issue either in https://github.com/Mbed-TLS/mbedtls or https://github.com/ARM-software/psa-crypto-api, or to directly make a PR to psa-crypto-api. In this case Andrew has already filed an issue: https://github.com/ARM-software/psa-crypto-api/issues/564

gilles-peskine-arm avatar Oct 17 '22 08:10 gilles-peskine-arm

Ok, great, thanks!

mpg avatar Oct 17 '22 08:10 mpg