Streamline PSA key type and curve encodings #330

gilles-peskine-arm · 2019-12-04T20:10:17Z

Change psa_key_type_t to 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:

USE_PSA_CRYPTO: update elliptic curve encoding Mbed-TLS/mbedtls#2964 — change the code of ssl_cli so that it works with the new curve encoding.
[upcoming PR to https://github.com/ARMmbed/mbed-os] — change the code of the attestation service so that it works with the new curve encoding.

athoelke · 2019-12-04T20:55:14Z

include/psa/crypto_values.h

-        (type) == PSA_KEY_TYPE_ARC4 ? 1 :            \
-        (type) == PSA_KEY_TYPE_CHACHA20 ? 1 :            \
-        0)
+    (((type) & PSA_KEY_TYPE_CATEGORY_MASK) == PSA_KEY_TYPE_CATEGORY_SYMMETRIC ? \


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.

athoelke · 2019-12-04T21:10:10Z

include/psa/crypto_values.h

+ * 2048, 3072, 4096, 6144, 8192. A given implementation may support
+ * all of these sizes or only a subet.
+ */
+#define PSA_DH_GROUP_RFC7919            ((psa_dh_group_t) 0x02)


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...

I amended the old commits.

athoelke

From a design perspective, this looks pretty sound to me. It deals with some primary concerns:

improves code size and coding logic for applications, by not encoding the key size in different ways in two places
increases room for vendor defined keys in the ECC and DH spaces
Provides some mitigation against bit-flip fault attacks.

athoelke · 2019-12-06T16:49:12Z

include/psa/crypto_values.h

+ *
+ * This family includes groups with the following key sizes (in bits):
+ * 2048, 3072, 4096. A given implementation may support
+ * all of these sizes or only a subet.


"subet" -> "subset"

athoelke · 2019-12-06T16:49:32Z

include/psa/crypto_values.h

@@ -568,14 +581,22 @@
                       ((type) & PSA_KEY_TYPE_DH_GROUP_MASK) :  \
                       0))

-/** Diffie-Hellman groups defined in RFC 7919 Appendix A.
+/** Diffie-Hellman FFDHE groups defined in RFC 7919 Appendix A.
 *
 * This family includes groups with the following key sizes (in bits):
 * 2048, 3072, 4096, 6144, 8192. A given implementation may support
 * all of these sizes or only a subet.


"subet" -> "subset"

athoelke · 2019-12-13T16:30:44Z

include/psa/crypto_sizes.h

-     (curve) == PSA_ECC_CURVE_CURVE25519       ? 255 : \
-     (curve) == PSA_ECC_CURVE_CURVE448         ? 448 : \
-     0)
+#define PSA_ECC_CURVE_BITS(curve) ((curve) & 0xffff)


This isn't going to work anymore after the removal of the curve size from the key type.

athoelke · 2019-12-16T10:41:48Z

There are a few more places which refer to the ECC curves using wildcard references (PSA_ECC_CURVE_XXX):

PSA_ALG_ECDH
psa_export_key()

Internally, use the corresponding function from psa_crypto.c instead. Externally, this function is not used in Mbed TLS and is documented as "may change at any time".

Don't assume that the PSA encoding of elliptic curves is identical to the TLS encoding. This is currently true but about to change. The new implementation only works when MBEDTLS_ECP_C is defined. This is ok because the function is only used with MBEDTLS_ECP_C defined.

Change the representation of psa_ecc_curve_t and psa_dh_group_t from the IETF 16-bit encoding to a custom 24-bit encoding where the upper 8 bits represent a curve family and the lower 16 bits are the key size in bits. Families are based on naming and mathematical similarity, with sufficiently precise families that no two curves in a family have the same bit size (for example SECP-R1 and SECP-R2 are two different families). As a consequence, the lower 16 bits of a key type value are always either the key size or 0.

Define constants for ECC curve families and DH group families. These constants have 0x0000 in the lower 16 bits of the key type. Support these constants in the implementation and in the PSA metadata tests. Switch the slot management and secure element driver HAL tests to the new curve encodings. This requires SE driver code to become slightly more clever when figuring out the bit-size of an imported EC key since it now needs to take the data size into account. Switch some documentation to the new encodings. Remove the macro PSA_ECC_CURVE_BITS which can no longer be implemented.

Use the same translation function that the PSA crypto implementation uses.

This is a change to an internal API that is exposed only for the sake of Mbed TLS.

Remove the values of curve encodings that are based on the TLS registry and include the curve size, keeping only the new encoding that merely encodes a curve family in 8 bits. Keep the old constant names as aliases for the new values and deprecate the old names.

All key types now have an encoding on 32 bits where the bottom 16 bits are zero. Change to using 16 bits only. Keep 32 bits for key types in storage, but move the significant half-word from the top to the bottom. Likewise, change EC curve and DH group families from 32 bits out of which the top 8 and bottom 16 bits are zero, to 8 bits only. Reorder psa_core_key_attributes_t to avoid padding.

Change the encoding of key types, EC curve families and DH group families to make the low-order bit a parity bit (with even parity). This ensures that distinct key type values always have a Hamming distance of at least 2, which makes it easier for implementations to resist single bit flips.

If psa_key_agreement_ecdh fails, there may be output that leaks sensitive information in the output buffer. Zeroize it. If this is due to an underlying failure in the ECDH implementation, it is currently not an issue since both the traditional Mbed TLS/Crypto implementation and Everest only write to the output buffer once every intermediate step has succeeded, but zeroizing is more robust. If this is because the recently added key size check fails, a leak could be a serious issue.

gilles-peskine-arm · 2020-01-31T10:12:48Z

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. Mbed-TLS/mbedtls#2964 updates the crypto submodule.

Patater

Only subjective clarity improvements recommended, non-blocking.

gilles-peskine-arm · 2020-01-31T13:34:18Z

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 Mbed-TLS/mbedtls#2964 is merged, we should merge this when we're ready to move on with Mbed-TLS/mbedtls#2964.

Previously in d875285: * ARMmbed#333: Streamline PSA key type encodings: prepare * ARMmbed#323: Initialise return values to an error Previously in dbcb442: * ARMmbed#291: Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY * ARMmbed#334: Fix some pylint warnings Previously in ceceedb: * ARMmbed#348: Bump version to Mbed TLS 2.20.0 and crypto SO version to 4 * ARMmbed#354: Fix incrementing pointer instead of value In this commit: * ARMmbed#349: Fix minor defects found by Coverity * ARMmbed#179: Add option to build SHA-512 without SHA-384 * ARMmbed#327: Implement psa_hash_compute and psa_hash_compare * ARMmbed#330: Streamline PSA key type and curve encodings

gilles-peskine-arm added needs: preceding PR Requires another PR to be merged first needs: design review needs: ci Needs a passing full CI run labels Dec 4, 2019

gilles-peskine-arm changed the title ~~Streamline key and curve encodings~~ Streamline PSA key type and curve encodings Dec 4, 2019

athoelke reviewed Dec 4, 2019

View reviewed changes

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: ci Needs a passing full CI run labels Dec 4, 2019

athoelke reviewed Dec 4, 2019

View reviewed changes

athoelke previously approved these changes Dec 4, 2019

View reviewed changes

gilles-peskine-arm dismissed athoelke’s stale review via 450546a December 6, 2019 16:44

gilles-peskine-arm force-pushed the psa-streamline_encodings-types_and_curves branch from cc2dbc6 to 450546a Compare December 6, 2019 16:44

gilles-peskine-arm added needs: ci Needs a passing full CI run and removed needs: preceding PR Requires another PR to be merged first needs: design review needs: work The pull request needs rework before it can be merged. labels Dec 6, 2019

athoelke reviewed Dec 6, 2019

View reviewed changes

gilles-peskine-arm force-pushed the psa-streamline_encodings-types_and_curves branch 2 times, most recently from c58e7df to f8255b0 Compare December 8, 2019 17:40

gilles-peskine-arm mentioned this pull request Dec 11, 2019

Streamline PSA key type encodings: prepare #333

Merged

gilles-peskine-arm force-pushed the psa-streamline_encodings-types_and_curves branch from f8255b0 to 7442f9e Compare December 12, 2019 23:49

gilles-peskine-arm added needs: preceding PR Requires another PR to be merged first needs: work The pull request needs rework before it can be merged. labels Dec 13, 2019

gilles-peskine-arm force-pushed the psa-streamline_encodings-types_and_curves branch from 7442f9e to 7066307 Compare December 13, 2019 16:16

athoelke reviewed Dec 13, 2019

View reviewed changes

gilles-peskine-arm force-pushed the psa-streamline_encodings-types_and_curves branch 3 times, most recently from a680910 to 48d4763 Compare December 19, 2019 11:07

gilles-peskine-arm mentioned this pull request Dec 19, 2019

USE_PSA_CRYPTO: update elliptic curve encoding Mbed-TLS/mbedtls#2964

Merged

4 tasks

gilles-peskine-arm added 17 commits January 31, 2020 10:15

Remove mbedtls_psa_translate_ecc_group

fc2459d

Internally, use the corresponding function from psa_crypto.c instead. Externally, this function is not used in Mbed TLS and is documented as "may change at any time".

Move size-specific curve/group constants to crypto_compat.h

45c29ce

pk tests: USE_PSA_CRYPTO: test attributes of the PSA key

85f47c9

pk tests: USE_PSA_CRYPTO: test several curves

33b1c69

Convert USE_PSA_CRYPTO pk interface to the new PSA EC curve encoding

89177e8

pk: USE_PSA_CRYPTO: don't translate via the TLS ID

4080c91

Use the same translation function that the PSA crypto implementation uses.

Switch psa_crypto tests to the new curve encoding

80d26fb

Change auxiliary functions for TLS to the new PSA EC curve encoding

d1959dc

This is a change to an internal API that is exposed only for the sake of Mbed TLS.

Document the vendor range for EC curve and DH group families

46e6f9d

Test parsing invalid key type in storage

7d7c8dc

gilles-peskine-arm force-pushed the psa-streamline_encodings-types_and_curves branch from 79ab5ce to 3e819b7 Compare January 31, 2020 09:24

Patater approved these changes Jan 31, 2020

View reviewed changes

AndrzejKurek approved these changes Jan 31, 2020

View reviewed changes

gilles-peskine-arm merged commit 819799c into ARMmbed:development Jan 31, 2020

gilles-peskine-arm mentioned this pull request Mar 2, 2020

Fix useless calls in PSA crypto metadata tests #378

Merged

yanesca mentioned this pull request Mar 16, 2020

Incorrect sizes for secp224k1 keys through the PSA API #336

Closed

gilles-peskine-arm mentioned this pull request Aug 26, 2020

Minor fixes in PSA code and tests Mbed-TLS/mbedtls#3611

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Streamline PSA key type and curve encodings #330

Streamline PSA key type and curve encodings #330

Uh oh!

gilles-peskine-arm commented Dec 4, 2019 •

edited

Loading

Uh oh!

athoelke Dec 4, 2019

Uh oh!

gilles-peskine-arm Dec 4, 2019

Uh oh!

athoelke Dec 4, 2019

Uh oh!

athoelke Dec 4, 2019

Uh oh!

gilles-peskine-arm Jan 30, 2020

Uh oh!

athoelke left a comment

Uh oh!

athoelke Dec 6, 2019

Uh oh!

athoelke Dec 6, 2019

Uh oh!

athoelke Dec 13, 2019

Uh oh!

athoelke commented Dec 16, 2019

Uh oh!

gilles-peskine-arm commented Jan 31, 2020

Uh oh!

Patater left a comment

Uh oh!

gilles-peskine-arm commented Jan 31, 2020

Uh oh!

Uh oh!

Streamline PSA key type and curve encodings #330

Streamline PSA key type and curve encodings #330

Uh oh!

Conversation

gilles-peskine-arm commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athoelke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athoelke commented Dec 16, 2019

Uh oh!

gilles-peskine-arm commented Jan 31, 2020

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Jan 31, 2020

Uh oh!

Uh oh!

gilles-peskine-arm commented Dec 4, 2019 •

edited

Loading