Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS #6802

gilles-peskine-arm · 2022-12-16T00:17:23Z

Add metadata tests for two algorithms that were missing them: PSA_ALG_CCM_STAR_NO_TAG and PSA_ALG_TLS12_ECJPAKE_TO_PMS.
PSA_ALG_TLS12_ECJPAKE_TO_PMS is not compatible with key agreements since its input must have a specific format (must begin with 0x04). Adapt the library and automatic test case generation accordingly.
Add PSA_ALG_CCM_STAR_NO_TAG to psa/crypto_config.h where it was missing.
Two key types are newly supported in 3.3.0 for the sake of ECJPAKE: PSA_KEY_TYPE_PASSWORD and PSA_KEY_TYPE_PASSWORD_HASH. These key types are always supported, like PSA_KEY_TYPE_DERIVE.

2.28 has neither CCM* nor ECJPAKE-to-PMS, so this doesn't affect 2.28. Just backport some minor script improvements.

Gatekeeper checklist

changelog provided
backport Backport 2.28: Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS #6844
tests provided

gilles-peskine-arm · 2022-12-16T00:19:58Z

ChangeLog.d/psa_alg_tls12_ecjpake_to_pms-reject_ka.txt

@@ -0,0 +1,4 @@
+Bugfix
+   * The key derivation algorithm PSA_ALG_TLS12_ECJPAKE_TO_PMS cannot be
+     used on a shared secret from a key agreement since its input must be


I find this weird. Why does the secret input of a key derivation need to have one specific format which is meant to be a public key?

Because this "KDF" is actually is really ad-hoc. The PMS is defined as SHA-256(K.X) where K is the shared secret from the EC J-PAKE key exchange, and K.X is it's X coordinate. Since the PSA EC J-PAKE API passes the serialization of K to the KDF, what this KDF needs to do is extract bytes 1-32 included, feed them to SHA-256, and output the result.

So, the KDF as the very least needs to check that it input is at least 33 bytes, but it seemed natural to actually assert that the input is exactly 65 bytes and starts with 0x04, but if that's annoying I guess we can remove it.

Btw, I think the sentence is incorrect: "cannot be used on a shared secret from a key agreement other than EC J-PAKE with secp256r1". It can absolutely be used with that particular key exchange, because that's what we do in the implementation of the ECJPAKE TLS 1.2 key exchange when USE_PSA_CRYPTO is enabled.

ECJPAKE isn't a key agreement as far as the PSA API is concerned (it's not something you can do with psa_raw_key_agreement or psa_key_agreement_key_derivation). The output of an ECDH key agreement on a Weierstrass curve is the X coordinate in big-endian order. Why doesn't PSA_ALG_TLS12_ECJPAKE_TO_PMS take that as its input?

ECJPAKE isn't a key agreement as far as the PSA API is concerned

Ah, of course, good point.

The output of an ECDH key agreement on a Weierstrass curve is the X coordinate in big-endian order. Why doesn't PSA_ALG_TLS12_ECJPAKE_TO_PMS take that as its input?

Because it's designed to be use with psa_pake_get_implicit_key() which according to its documentation "injects the shared secret output of the PAKE into the provided key derivation operation" and for JPAKE that's a group element which for ECC uses "the same [format] as that for public keys in the specific Diffie-Hellman group".

So, PSA_ALG_TLS12_ECJPAKE_TO_PMS take an ECC point in the standard format because that's what psa_pake_get_implicit_key() will give it. We further restricted it to secp256r1 (or any other 256-bit short Weierstrass curve) because that's the only thing that's specified for use in TLS ECJPAKE anyway. But the format (04 | X | Y) is given by the PSA spec.

"injects the shared secret output of the PAKE into the provided key derivation operation" and for JPAKE that's a group element which for ECC uses "the same [format] as that for public keys in the specific Diffie-Hellman group".

But why? Why not use the X coordinate for the shared secret?

(That's why as in why the spec was written that way, so “because the spec says so” is not an answer. The spec is beta, we can change it if we notice difficulties during the implementation.)

Then I think that's a question for @yanesca but I'd say that's probably because it's the most generic format and contains the full information. (Using only X loses 1 bit of information, which is not important cryptographically speaking, but if a protocol relies on this particular bit, then that's going to be a problem.)

I blame the author and reviewers of RFC 8236 for being too abstract and not specifying formats. Even in 2017 we knew that's important (I mean, the original Curve25519 paper is from 2005 and does define byte formats).

It would be interesting to know what other protocols based on EC J-PAKE are doing and how what exactly they pass to the KDF. If that's always X, or something that can be derived from X, then we can indeed consider passing only X.

Then I think that's a question for @yanesca but I'd say that's probably because it's the most generic format and contains the full information.

Indeed, I choose that because it seemed the most universal and conceptually the simplest solution. I admit that it is unlikely that the Y coordinate is needed, but if it is, then it is more complicated to derive it than just checking a prefix and discarding trailing bytes.

Note: as noted elsewhere there's an ISO standard about J-PAKE which seems suggest a KDF to use with it. If that's the case, hopefully it also has something to say about what the input to the KDF should be. Unfortunately we don't seem to have access to a copy of that standard atm.

gilles-peskine-arm · 2022-12-16T00:20:43Z

tests/suites/test_suite_psa_crypto.data

@@ -5080,6 +5080,22 @@ PSA key derivation: ECDH on P256 with HKDF-SHA256, missing info
 depends_on:PSA_WANT_ALG_ECDH:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR:MBEDTLS_PK_PARSE_C:PSA_WANT_ECC_SECP_R1_256
 derive_input:PSA_ALG_KEY_AGREEMENT(PSA_ALG_ECDH, PSA_ALG_HKDF(PSA_ALG_SHA_256)):PSA_KEY_DERIVATION_INPUT_SALT:PSA_KEY_TYPE_NONE:"":PSA_SUCCESS:PSA_KEY_DERIVATION_INPUT_SECRET:PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1):"c88f01f510d9ac3f70a292daa2316de544e9aab8afe84049c62a9c57862d1433":PSA_SUCCESS:0:UNUSED:"":UNUSED:PSA_KEY_TYPE_NONE:PSA_ERROR_BAD_STATE

+PSA key derivation: TLS12_ECJPAKE_TO_PMS, good input, output too short


Lots more missing test cases here: positive and negative test cases with correct inputs and varying output lengths, negative test cases with different inputs.

I would assume that this is tested one layer deeper, here: https://github.com/Mbed-TLS/mbedtls/pull/6115/files#diff-cc82e7d511f22a0a29b24a8ad0c62d6c62b5f8c84ad67e55ea71d7fc035101aeR5800-R5840
But sure, I guess that these tests don't take long enough for duplication to be a problem and checking things on a different layer is also fine.

@gilles-peskine-arm What do you want to do about the tests? It might be a bit confusing to have a partial set here and a complete set elsewhere (the place pointed by Andrzej). Probably better to either have only one complete set if possible, or two complete sets if there's a reason to have them in both places.

I initially added those positive cases because I wanted negative test cases when the input comes from a key agreement. But then I realized there was a test case with key agreement chaining in the op-fail test suite. Looking back I'm not completely happy with relying on that because it's a corner case in the test code generator and there's a risk we'll stop generating the test case and not really notice. So I think I should add a manual test here too.

Also, I had not realized that derive_ecjpake_to_pms was testing the same thing. I find it weird that this KDF has its own function. What does it do that's missing in the generic derive functions?

Also, I had not realized that derive_ecjpake_to_pms was testing the same thing. I find it weird that this KDF has its own function. What does it do that's missing in the generic derive functions?

I'm not sure. It's possible that we were just not (sufficiently) aware of the existence of the generic derive functions. If that's the case, then we could just port the existing test cases to the generic function.

mpg · 2022-12-19T09:38:03Z

@gilles-peskine-arm I'm unsure about the status of this: you requested a review from Andrzej and me, but this is not labeled needs-review, so do you recommend I review this now or should I wait?

AndrzejKurek · 2022-12-19T11:06:00Z

scripts/mbedtls_dev/crypto_knowledge.py


+    def is_invalid_key_agreement_with_derivation(self) -> bool:


Minor: This could also return an optional and then is_valid_key_agreement_with_derivation would use it to just flip the condition (or the other way around) instead of having two copies of the same code. The duplication is so small though that it doesn't make much difference.

I found this messy enough as it was, I preferred to have clean interfaces than save maybe one line of code in the implementation.

AndrzejKurek

Only minor points found.

gilles-peskine-arm · 2022-12-19T14:30:12Z

@mpg Needs review if CI passes, which it has. I asked you and Andrzej because you're familiar with that part of the code and participated in the discussion that led to this.

mpg

LGTM

Rename NotSupported to KeyTypeNotSupported, because it's only about testing key management. For algorithms, not-supported is handled by OpFail. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The following shell command lists features that seem to be supported, but are missing from include/psa/crypto_config.h: ``` for x in $(grep -ho -Ew '(PSA_WANT|MBEDTLS_PSA_BUILTIN)_\w+_\w+' library/psa_crypto*.c | sed 's/^MBEDTLS_PSA_BUILTIN/PSA_WANT/' | sort -u); do grep -qw $x include/psa/crypto_config.h || echo $x; done ``` This looks for PSA_WANT_<kind>_<thing> macros that gate a part of the library, as well as their MBEDTLS_PSA_BUILTIN_<kind>_<thing> counterparts. This is not necessarily a complete list of identifiers that must appear in the config file, since a few features are not gated. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

No intended behavior change. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add PSA_WANT_KEY_TYPE_PASSWORD and PSA_WANT_KEY_TYPE_PASSWORD_HASH to psa/crypto_config.h, since the types PSA_KEY_TYPE_PASSWORD and PSA_KEY_TYPE_PASSWORD_HASH are used by ECJPAKE. The two key types are always enabled, like PSA_KEY_TYPE_DERIVE. Add the key types to the metadata test suite as well. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The following shell command (requiring GNU grep) looks for algorithms and key types, as well as IS and GET macros, that lack metadata tests: ``` for x in $(grep -Pho '(?<=^#define )PSA_(ALG|KEY_TYPE)_(?!CATEGORY_|NONE\b|\w+_(BASE|FLAG|MASK|CASE))\w+' include/psa/crypto_values.h include/psa/crypto_extra.h); do grep -qw $x tests/suites/test_suite_psa_crypto_metadata.* || echo $x; done ``` This may have false negatives: it only checks that the constants are mentioned at least once, not that the tests are written correctly. This has false positives: * Types and algorithms that Mbed TLS does not support. * PSA_ALG_ECDSA_IS_DETERMINISTIC, PSA_ALG_DSA_IS_DETERMINISTIC are peculiar auxiliary macros that only apply to very specific algorithms and aren't tested like the other IS macros. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

…_TO_PMS Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Test accordingly. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The key derivation algorithm PSA_ALG_TLS12_ECJPAKE_TO_PMS cannot be used on a shared secret from a key agreement since its input must be an ECC public key. Reject this properly. This is tested by test_suite_psa_crypto_op_fail.generated. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2023-01-19T11:19:47Z

Force-pushed with the new code style.

The backport #6844 is also up for review.

mpg

Re-approving after code style change - ran the script myself and got the same result.

gilles-peskine-arm · 2023-01-26T20:57:05Z

@AndrzejKurek Please re-review for the update after the code style change.

AndrzejKurek · 2023-01-27T09:01:14Z

@AndrzejKurek Please re-review for the update after the code style change.

I did re-review and approve?

mpg · 2023-01-27T09:03:17Z

Perhaps Gilles meant me. Looking at my comment, looks like I meant to approve but probably selected the wrong button as it doesn't show as an approval...

mpg

LGTM

gilles-peskine-arm added bug needs-backports Backports are missing or are pending review and approval. component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) labels Dec 16, 2022

gilles-peskine-arm requested review from mpg and AndrzejKurek December 16, 2022 00:17

gilles-peskine-arm mentioned this pull request Dec 16, 2022

Check list for implementing a new mechanism in PSA crypto #5904

Merged

3 tasks

gilles-peskine-arm commented Dec 16, 2022

View reviewed changes

gilles-peskine-arm mentioned this pull request Dec 16, 2022

Improve reliability of PSA mechanism collection #6805

Open

mpg removed the needs-ci Needs to pass CI tests label Dec 19, 2022

AndrzejKurek reviewed Dec 19, 2022

View reviewed changes

AndrzejKurek previously approved these changes Dec 19, 2022

View reviewed changes

gilles-peskine-arm added the needs-review Every commit must be reviewed by at least two team members, label Dec 19, 2022

mpg previously approved these changes Dec 20, 2022

View reviewed changes

mpg removed the needs-review Every commit must be reviewed by at least two team members, label Dec 20, 2022

gilles-peskine-arm mentioned this pull request Dec 24, 2022

Backport 2.28: Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS #6844

Merged

3 tasks

gilles-peskine-arm added 8 commits January 19, 2023 12:11

NotSupported is specifically about key types

0e9e442

Rename NotSupported to KeyTypeNotSupported, because it's only about testing key management. For algorithms, not-supported is handled by OpFail. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Refactoring: new method Algorithm.is_valid_for_operation

72f4156

No intended behavior change. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix documentation

cafda87

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add SECRET input validation test cases for PSA_WANT_ALG_TLS12_ECJPAKE…

4db02f2

…_TO_PMS Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add metadata test case for PSA_ALG_TLS12_ECJPAKE_TO_PMS

2566679

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added 2 commits January 19, 2023 12:11

A key agreement cannot be chained with PSA_ALG_TLS12_ECJPAKE_TO_PMS

f6c6b64

Test accordingly. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm dismissed stale reviews from mpg and AndrzejKurek via bb3814c January 19, 2023 11:18

gilles-peskine-arm force-pushed the test_suite_psa_crypto_metadata-20221215 branch from f09341e to bb3814c Compare January 19, 2023 11:18

gilles-peskine-arm requested review from mpg and AndrzejKurek January 19, 2023 11:19

mpg reviewed Jan 24, 2023

View reviewed changes

mpg added the needs-review Every commit must be reviewed by at least two team members, label Jan 24, 2023

AndrzejKurek approved these changes Jan 24, 2023

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 24, 2023

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests labels Jan 26, 2023

mpg approved these changes Jan 27, 2023

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels Jan 27, 2023

mpg merged commit 169d9e6 into Mbed-TLS:development Jan 27, 2023

athoelke mentioned this pull request Apr 27, 2023

New algorithm: Ad-hoc KDF for EC J-PAKE in TLS 1.2 ARM-software/psa-api#11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS #6802

Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS #6802

gilles-peskine-arm commented Dec 16, 2022 •

edited

Loading

gilles-peskine-arm Dec 16, 2022

mpg Dec 16, 2022

gilles-peskine-arm Dec 16, 2022

mpg Dec 16, 2022 •

edited

Loading

gilles-peskine-arm Dec 16, 2022

mpg Dec 16, 2022

mpg Dec 16, 2022

yanesca Dec 16, 2022

mpg Dec 19, 2022

gilles-peskine-arm Dec 16, 2022

AndrzejKurek Dec 19, 2022

mpg Dec 20, 2022

gilles-peskine-arm Dec 20, 2022

mpg Dec 20, 2022

mpg commented Dec 19, 2022

AndrzejKurek Dec 19, 2022 •

edited

Loading

gilles-peskine-arm Dec 19, 2022

AndrzejKurek left a comment

gilles-peskine-arm commented Dec 19, 2022 •

edited

Loading

mpg left a comment

gilles-peskine-arm commented Jan 19, 2023

mpg left a comment

gilles-peskine-arm commented Jan 26, 2023

AndrzejKurek commented Jan 27, 2023

mpg commented Jan 27, 2023

mpg left a comment

Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS #6802

Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS #6802

Conversation

gilles-peskine-arm commented Dec 16, 2022 • edited Loading

Gatekeeper checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg commented Dec 19, 2022

AndrzejKurek Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Dec 19, 2022 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jan 19, 2023

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jan 26, 2023

AndrzejKurek commented Jan 27, 2023

mpg commented Jan 27, 2023

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Dec 16, 2022 •

edited

Loading

mpg Dec 16, 2022 •

edited

Loading

AndrzejKurek Dec 19, 2022 •

edited

Loading

gilles-peskine-arm commented Dec 19, 2022 •

edited

Loading