Streamline PSA key type and curve encodings #330
No reviewers
Labels
No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-ARMmbed/mbed-crypto!330
Loading…
Reference in a new issue
No description provided.
Delete branch "psa-streamline_encodings-types_and_curves"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Change
psa_key_type_tto a more compact 16-bit type.Change the encoding of elliptic curves (and FFDH groups) drastically: now there is an 8-bit curve family in the lower 8 bits of the key type. A curve family can include several curves as long as they all have different octet lengths. The precise curve is determined by the curve family and the key size, which is fully known when generating or deriving a key and known up to byte granularity when importing a key. Rationale: it makes the encoding more compact, and makes it easier for applications to work with multiple curves without having to embed a conversion between the curve/type encoding and the size.
Distinct key types have a Hamming distance of at least 2. This helps implementations that protect against a single bit flip. This is done by making the least significant bit a parity bit.
Elliptic curves no longer use the encoding from the TLS registry, which was restrictive because it has little room reserved for curves that aren't in the registry, but the world outside TLS uses quite a few curves that aren't used in TLS.
Private discussion about the design: https://github.com/ARMmbed/psa-crypto/issues/122#issuecomment-561096308
This breaks the build of the attestation service in Mbed OS due to the change in how elliptic curves are encoded.
Needs preceding PRs:
Technically redundant test as the specification of the macro says "unspecified if not a supported cipher key type".
That's the Mbed Crypto implementation which currently tends to favor robustness over minimal code size.
Presume this should bx
0x020000? - to align with the definition of the ECC group families?At least - when first introduced, until the commit that changes to 16-bit key types...
@ -582,2 +578,4 @@#define PSA_GET_KEY_TYPE_BLOCK_SIZE_EXPONENT(type) \(((type) >> 8) & 7)/** The block size of a block cipher."subet" -> "subset"
@ -582,2 +578,4 @@#define PSA_GET_KEY_TYPE_BLOCK_SIZE_EXPONENT(type) \(((type) >> 8) & 7)/** The block size of a block cipher."subet" -> "subset"
This isn't going to work anymore after the removal of the curve size from the key type.
There are a few more places which refer to the ECC curves using wildcard references (PSA_ECC_CURVE_XXX):
PSA_ALG_ECDHpsa_export_key()@ -160,81 +160,96 @@ static inline psa_algorithm_t mbedtls_psa_translate_md( mbedtls_md_type_t md_alg/* Translations for ECC. */RFC 5639 seems to indicate that brainpoolP512r1 is 512 bits, not 521 bits.
We very recently added these GREASE values. It's intentional to remove them now? Where is it documented how vendors can use GREASE now after this change?
@ -375,71 +375,118 @@ static inline int psa_key_slot_is_external( const psa_key_slot_t *slot )#endif /* MBEDTLS_PSA_CRYPTO_SE_C */Isn't Curve25519 255 bits not 519 bits?
Is it guaranteed that
shared_secretwill not contain secret data ifmbedtls_ecdh_calc_secret()fails? If not, we should zeroize it before returning. We should also check thatmbedtls_ecdh_free()will remove secrets fromecdheven ifmbedtls_ecdh_calc_secrethas erred.@ -276,0 +275,4 @@depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLEDpk_psa_sign:MBEDTLS_ECP_DP_SECP256R1:PSA_ECC_CURVE_SECP_R1:256PSA wrapped sign: SECP384R1Can we test with RSA as well? I'd guess we could if we'd have implemented RSA via PK via PSA...
@ -276,0 +300,4 @@depends_on:MBEDTLS_ECP_DP_BP256R1_ENABLEDpk_psa_sign:MBEDTLS_ECP_DP_BP256R1:PSA_ECC_CURVE_BRAINPOOL_P_R1:256PSA wrapped sign: BP384R1Consider adding test for BP512R1 (and perhaps a different test for Curve25519) as well. They could have caught some issues.
Consider renaming to pk_psa_ecdsa_sign() to emphasize that this function is for ECDSA testing and isn't generic enough to support e.g. RSA.
@ -59,0 +65,4 @@value ^= value >> 4;return( 0x9669 & 1 << ( value & 0xf ) );}#define TEST_PARITY( value ) \Suggestion:
TEST_ODD_PARITY()to make it clear what about parity you are checking for.@ -160,81 +160,96 @@ static inline psa_algorithm_t mbedtls_psa_translate_md( mbedtls_md_type_t md_alg/* Translations for ECC. */Typo. Fixed by amending the commit.
@ -375,71 +375,118 @@ static inline int psa_key_slot_is_external( const psa_key_slot_t *slot )#endif /* MBEDTLS_PSA_CRYPTO_SE_C */My search-and-replace wasn't quite right. Fixed by amending the commit.
GREASE is a TLS feature. It's irrelevant here. Using values reserved for GREASE was a hack to get more room for values not defined by the TLS registry. Since we no longer leverage the TLS registry, GREASE is not relevant anymore.
@ -276,0 +300,4 @@depends_on:MBEDTLS_ECP_DP_BP256R1_ENABLEDpk_psa_sign:MBEDTLS_ECP_DP_BP256R1:PSA_ECC_CURVE_BRAINPOOL_P_R1:256PSA wrapped sign: BP384R1I added a test for all supported curves in Mbed Crypto except secp224k1. There's no test for Curve25519 because it can't be used for ECDSA and that's the only pk-over-psa interface that's implemented.
Good point.
mbedtls_ecdh_calc_secretis implemented safely (it only writes its output as the very last, non-fallible step), but robustness here is good. More importantly, if the size check fails, it's possible that the leaked output would be sensitive (I haven't analyzed to check).@ -276,0 +275,4 @@depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLEDpk_psa_sign:MBEDTLS_ECP_DP_SECP256R1:PSA_ECC_CURVE_SECP_R1:256PSA wrapped sign: SECP384R1No, pk-over-psa is not implemented for RSA.
This is a preexisting function. Renaming it would be out of scope.
@ -59,0 +65,4 @@value ^= value >> 4;return( 0x9669 & 1 << ( value & 0xf ) );}#define TEST_PARITY( value ) \It's actually even parity (so that 0 has the correct parity). And I think the macro name should not indicate which parity it is. The job of the macro is to test for the expected parity, not for even parity.
@ -50,6 +50,8 @@ extern "C" {typedef MBEDTLS_PSA_DEPRECATED size_t mbedtls_deprecated_size_t;Minor: typo in commit
27f8944-Remove the values of curve encodings that is based- encodings/values are plural, so I'd assume that it should bethat **are** based.Also,
andinclude->and includeI assume that key bits can be inferred from key data, right?
@ -59,0 +65,4 @@value ^= value >> 4;return( 0x9669 & 1 << ( value & 0xf ) );}#define TEST_PARITY( value ) \I'll explain where I'm coming from. If a hardware datasheet says "odd parity bit", I would expect a value of 1 there to mean odd parity and 0 to mean even parity. If a datasheet says "even parity bit", then I expect a value of 1 there to mean even parity, and 0 to mean odd parity. If the datasheet says "parity bit", I am unclear what the hardware will place there.
In this case, I read "value" as the value you wanted to check the parity of, not the expected parity. Consider renaming "value" to "expected_parity" if the job of the macro is to test for arbitrary expected parity. Consider renaming to
TEST_EVEN_PARITYif value is the value you are checking the parity of. The comment you've added totest_parityis good, as it says what sort of parity you are checking for, but the names could still be more clear.@ -3502,10 +3502,10 @@ psa_status_t psa_key_derivation_output_bytes(* length is determined by the curve, and sets the mandatory bitsWe stopped calling these curves "XED" in the latest spec. We may want to avoid adding more uses of "XED" that we'll need to remove later by using the new label.
@ -458,70 +458,82 @@((type) & PSA_KEY_TYPE_ECC_CURVE_MASK) : \Consider using
PSA_ECC_FAMILY_MONTGOMERYinstead, to be more up to date with the latest PSA spec.Could easily be added in a new commit within the same PR. Next opportunity for rename is when we add a new function for testing RSA.
@ -59,0 +65,4 @@value ^= value >> 4;return( 0x9669 & 1 << ( value & 0xf ) );}#define TEST_PARITY( value ) \The name of the macro deliberately doesn't say which way the parity should go because its job is to test for whatever parity the API design intends. The fact that it's even parity is an implementation detail.
I don't understand how you come to name it odd parity. A value is correct iff the number of bits that is set is even.
I don't understand why you want to rename the argument. The argument is a value whose parity needs to be checked. There is no argument indicating what parity is expected: that's built into the code, since all values that are checked for parity must obey the same parity requirement.
Yes, in most cases. The exception would be for EC or DH keys if there were two curves in the same family with sizes that are within the same range [8k, 8k+7], but there are no such families and I don't expect there to ever be any such family.
Or we might modify this function to support RSA, in which case renaming it now would have been the wrong thing. This PR is big enough as it is; I'm not renaming this function.
@ -458,70 +458,82 @@((type) & PSA_KEY_TYPE_ECC_CURVE_MASK) : \Since I was rewriting history anyway, I made it
MONTGOMERYfrom the get go.I amended the old commits.
@ -50,6 +50,8 @@ extern "C" {typedef MBEDTLS_PSA_DEPRECATED size_t mbedtls_deprecated_size_t;I fixed the commit message.
I rebased yet again.
developmentso that there are no merge conflicts.PSA_ECC_CURVE_MONTGOMERYfrom the start and not_XED.Making the function do even more and be even larger would be dumb. Test functions should be small and focused on what they are testing.
The role of the function is to test signing through pk-over-psa. The current implementation is a monolithic sequence of actions to generate a key, encapsulate it, perform the signature and verify the result. Good, textbook design would be to make each step into an auxiliary function that receives the key type as an argument (if necessary) and does its job given the key type. Having separate functions with the same structure for different key types would violate DRY. Now, these are unit tests, and some amount of repetition is excusable. Still, I would prefer to have more auxiliary functions and less repetition in our test suites, not more.
The Crypto CI passed except for Mbed OS which is already broken before this PR and will require a further update after this PR.
The TLS CI is not expected to pass here because an update of the crypto submodule is required. https://github.com/ARMmbed/mbedtls/pull/2964 updates the crypto submodule.
Another day, another merge conflict. I'll rebase.
The Crypto CI passed except for Mbed OS which is already broken before this PR and will require a further update after this PR.
The TLS CI is not expected to pass here because an update of the crypto submodule is required. https://github.com/ARMmbed/mbedtls/pull/2964 updates the crypto submodule.
Only subjective clarity improvements recommended, non-blocking.
With two approvals and with acceptable CI (crypto and TLS pass except for known unrelated failures), this is ready for merge. Since it will disrupt TLS testing until the corresponding TLS PR https://github.com/ARMmbed/mbedtls/pull/2964 is merged, we should merge this when we're ready to move on with https://github.com/ARMmbed/mbedtls/pull/2964.