Simplify RSA and EC public key formats #13

Patater · 2019-01-11T11:44:16Z

Out of scope of this PR:

Completely remove dependency on the pk module
Update DH and DSA formats (see PR Simplify or specify public key format for DH and DSA #14)

Previously we weren't initializing the freshly allocated ECP keypair when importing private EC keys. This didn't seem to cause problems, at least according to our current test coverage, but it's better to ensure we don't have a partially initialized object by explicitly initializing the keypair.

Patater · 2019-01-11T11:56:12Z

Force push to update commit descriptions.

Patater · 2019-01-11T15:45:46Z

Rebased to add format specification for DH and DSA keys, and remove the request for crypto know-howerage. No implementation updates required for those, as Mbed Crypto doesn't have a PSA-exposed implementation of DH or DSA yet.

Patater · 2019-01-11T15:54:32Z

Rebase to fix memory leak fix (remember to free the imported "their_key") in ECDH.

Patater · 2019-01-11T16:40:03Z

Rebased and force-pushed an old version of this patchset so @gilles-peskine-arm can post his comments on the patchset he reviewed. Will push back the new version after no more active reviewers.

gilles-peskine-arm

crypto_sizes.h needs some updates: implementation and documentation of some of the PSA_KEY_EXPORT_xxx macros.
I haven't reviewed the changes to test data, but they look superficially ok.

library/psa_crypto.c

include/psa/crypto.h

library/psa_crypto.c

gilles-peskine-arm · 2019-01-11T16:56:29Z

tests/suites/test_suite_psa_crypto.data

@@ -188,65 +188,65 @@ PSA import/export EC secp224r1 key pair: good
 depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP224R1_ENABLED
 import_export:"6849f97d1066f6997759637c7e3899464cee3ec7ac970653a0be0742":PSA_KEY_TYPE_ECC_KEYPAIR(PSA_ECC_CURVE_SECP224R1):PSA_ALG_ECDSA_ANY:PSA_KEY_USAGE_EXPORT:224:0:PSA_SUCCESS:1

-PSA import/export-public EC secp224r1: good
+PSA import/export-public EC secp224r1: good XXX not sure why this was bit string not octet string


This is something I noticed when reading the ASN.1 and reading SEC 1 (http://www.secg.org/sec1-v2.pdf). We previously were using BIT_STRING (0x03) instead of OCTET_STRING (0x04) per the standard.

ECPoint ::= OCTET STRING

This might be worth fling an inter-op bug about unless we know some reason using BIT_STRING is correct. It's not an issue for PSA anymore, since we won't use an ASN.1-formatted key, but is still a potential issue for Mbed TLS.

Oh, I see what you mean. I'm quite confident that there is no interoperability bug since Mbed TLS is able to interact with the other TLS implementations out there. The bug is partly on me for using the name ECPoint for something that is not what the applicable specifications call ECPoint, and I would say partly on SEC1 for having confusing language.

RFC 5280 defines SubjectPublicKeyInfo as containing a subjectPublicKey which is always a BIT STRING. The content of this BIT STRING depends on the key type. RFC 3279 defines the content of a subjectPublicKey for some common key types. For an RSA or DSA or DH key, the content of the BIT STRING is an ASN.1 SEQUENCE. For an EC key, SEC 1 §C.3 says (the same language appears in RFC 3279 §2.3.5):

The elliptic curve public key (an ECPoint which is an OCTET STRING) is mapped to a subjectPublicKey (a BIT STRING) as follows: the most significant bit of the OCTET STRING becomes the most significant bit of the BIT STRING, and the least significant bit of the OCTET STRING becomes the least significant bit of the BIT STRING.

What you do here is to take the content of the ECPoint and stuff it into a BIT STRING. You don't stuff the OCTET STRING (04 + length + content) into the BIT STRING. By coincidence, the content of the BIT STRING happens to start with 04 in all the data we manipulate because that's the prefix for an uncompressed point representation (SEC1 §2.3.3 action 3.3). You can argue that SEC1 is ambiguous, and I'd agree with that, but everybody including the de facto standard openssl stuffs an EC point representation into a BIT STRING and not an ECPoint into a BIT STRING.

I filed a spec bug to go through all uses of “ECPoint” and fix them.

library/psa_crypto.c

Patater · 2019-01-11T17:11:43Z

Rebased to update back to where we were before and additionally improve the Doxygen comments.

Copy the nice and clear documentation from psa_export_key() as to what implementations are allowed to do regarding key export formats, as the same applies to public keys.

gilles-peskine-arm

+ my comments from the previous review

include/psa/crypto.h

Patater · 2019-01-11T19:02:01Z

Rebased to address code review feedback.

Patater · 2019-01-11T19:07:49Z

Rebased to remove XXX observation for ECPoint representation.

dgreen-arm · 2019-01-14T10:30:27Z

include/psa/crypto.h

+ * of the domain parameters written to \p data is specified in the
+ * documentation for psa_set_key_domain_parameters().
+ *
+ * \param[out] data         On success, the key domain parameters.


Handle parameter documentation is missing. Similarly for psa_set_key_domain_parameters.

Patater · 2019-01-14T10:43:22Z

Rebased to remove DH and DSA work, which has been raised in PR #14 instead.

yanesca · 2019-01-18T10:52:18Z

include/psa/crypto.h

+ *   `q` for a curve over `F_q`. The representation consists of:
+ *      - The byte 0x04;
+ *      - `x_P` as a `ceiling(m/8)`-byte string, big-endian;
+ *      - `y_P` as a `ceiling(m/8)`-byte string, big-endian.


We don't specify what implementations should/can do about the y coordinate in the case of PSA_ECC_CURVE_CURVE25519 and PSA_ECC_CURVE_CURVE448. Do we want to? I would assume that implementations simply ignore it, but are they allowed to omit it? Do we want to prescribe what is allowed if there is a parsing error on y?

What would we do with y if we had it? If we don't imagine we or any other implementations would have a use for it, we shouldn't require it.

Some implementations might want to require it to be a specific value for robustness.

But I just realised that Montgomery curves are out of scope for 1.0:
https://github.com/ARMmbed/psa-crypto/issues/101

So we could perhaps make it explicit that Montgomery and Edwards curves are not supported yet.

@gilles-peskine-arm what would be the best way to resolve the ambiguity here?

It's in scope for 1.0, but not for 1.0beta1. I think the 1.0 specification should define the format for Montgomery curves (and the reference implementation should support ECDH) and for Edwards curves (even if the reference implementation doesn't support them yet).

In this case I suggest we define the format according to RFC8032 and RFC7748. (That is x coordinate in little endian byte order in the case of Montgomery and y coordinate in little endian byte order, with the most significant bit of the last byte set to the least significant bit of x.)

Not changing format in this PR. Agreeing on a format is tracked in ARMmbed/psa-crypto#101

yanesca · 2019-01-18T15:42:23Z

include/psa/crypto.h

+ *                          `private_key_type` is the type of `private_key`.
+ *                          For example, for EC keys, this means that peer_key
+ *                          is interpreted as a point on the curve that the
+ *                          private key is on. The standard formats for public


"private key is on." -> "private key is associated with."
(The private key is an integer it is not a point it is not on any curve.)

hanno-becker · 2019-01-21T15:46:40Z

include/psa/crypto.h

+ *   ```
+ * - For elliptic curve public keys (key types for which
+ *   #PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY is true), the format is the uncompressed
+ *   representation defined by SEC1 &sect;2.3.3 as the content of an ECPoint.


Minor: The reading flow is slightly interrupted here because it's not clear that you'll go on explaining the format. I think using a colon : instead of a full stop . after ... of an ECPoint would help.

Added a colon

hanno-becker · 2019-01-21T15:49:01Z

include/psa/crypto.h

+ * \param[in] peer_key      Public key of the peer. The peer key must be in the
+ *                          same format that psa_import_key() accepts for the
+ *                          public key type corresponding to the type of
+ *                          private_key. That is, this function performs the


Minor: Use \p.

hanno-becker · 2019-01-21T15:49:44Z

include/psa/crypto.h

+ *                          `psa_import_key(internal_public_key_handle,
+ *                          PSA_KEY_TYPE_PUBLIC_KEY_OF_KEYPAIR(private_key_type),
+ *                          peer_key, peer_key_length)` where
+ *                          `private_key_type` is the type of `private_key`.


Minor: Use \p private_key.

hanno-becker · 2019-01-21T15:50:44Z

include/psa/crypto.h

+ *                          PSA_KEY_TYPE_PUBLIC_KEY_OF_KEYPAIR(private_key_type),
+ *                          peer_key, peer_key_length)` where
+ *                          `private_key_type` is the type of `private_key`.
+ *                          For example, for EC keys, this means that peer_key


Minor: Use \p peer_key.

hanno-becker · 2019-01-21T15:52:33Z

include/psa/crypto.h

+ *                          keys are documented in the documentation of
+ *                          psa_export_public_key().
+ * \param peer_key_length   Size of \p peer_key in bytes.
+ * \param alg               The key agreement algorithm to compute


(Very) minor: "algorithm to compute" sounds wrong, but that might just be me as a non-native. What about "The key agreement algorithm to use."?

That formulation is from me. It's copied from psa_key_derivation and will move to psa_key_derivation_setup in https://github.com/ARMmbed/mbedtls-psa/pull/244 . I'm not fond of “use” because it is very generic. “Algorithm to perform”? What do you think @Patater ?

Yeah, algorithm to compute sounds like you are computing an algorithm. That's second order. Can't have computers programming themselves...

"Algorithm to follow" sounds OK to me.

hanno-becker · 2019-01-21T15:56:19Z

library/psa_crypto.c

+     * supports non-byte-aligned key sizes, but not well. For example,
+     * mbedtls_rsa_get_len() returns the key size in bytes, not in bits. */
+    bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( rsa ) );
+    if( bits > PSA_VENDOR_RSA_MAX_KEY_BITS )


Do you also want to apply a lower bound on the number of bits in the RSA modulus?

At the moment we're applying an upper bound in PSA to limit the amount of memory and CPU time that an operation might use. A lower bound would come from a security policy, and we don't have a minimum-key-size security policy. I'd prefer to do nothing in the PSA code for now and only follow Mbed TLS, but this isn't workable here since Mbed TLS allows unrealistiaclly large RSA key sizes.

No change planned here for this PR

hanno-becker · 2019-01-21T15:57:48Z

library/psa_crypto.c

    }
+
+    /* On success, store the content of the object in the RSA context. */
+    *p_rsa = rsa;


Note: Strictly speaking, this violates the PK API because it is assumed that apart from the RSA context within the PK context, there is no other structure worth freeing.

mbedtls_pk_context is defined in pk.h, not pk_internal.h, and its fields are documented. So this assumption is validate by the documentation of pk.h, even if we might wish that the type was more opaque.

hanno-becker · 2019-01-21T16:07:24Z

library/psa_crypto.c

+        goto exit;
+    /* Load the public value. */
+    status = mbedtls_to_psa_error(
+        mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q,


mbedtls_ecp_point_read_binary() supports the 1-Byte representation { 0x00 } of the point of infinity, but as far as I see it's not documented above. If we don't want to accept the point at infinity here, we should rule it out by e.g. checking data_length.

Won't this later get rejected during mbedtls_ecp_check_pubkey?

hanno-becker · 2019-01-21T16:08:12Z

library/psa_crypto.c

+
+    /* Check that the point is on the curve. */
+    status = mbedtls_to_psa_error(
+        mbedtls_ecp_check_pubkey( &ecp->grp, &ecp->Q ) );


Oh ok, this would fail for the point at infinity.

Remove pkcs-1 and rsaEncryption front matter from RSA public keys. Move code that was shared between RSA and other key types (like EC keys) to be used only with non-RSA keys.

Use the PSA-native status type in psa_key_agreement_ecdh() in preparation for us calling PSA functions (and not just Mbed TLS functions) and still being able to return a psa_status_t (without having to translate it to a Mbed TLS error and then back again).

Patater · 2019-01-23T17:39:12Z

Rebased to update macros and documentation in crypto_sizes.h. Also addressed review comments from @hanno-arm

Remove front matter from our EC key format, to make it just the contents of an ECPoint as defined by SEC1 section 2.3.3. As a consequence of the simplification, remove the restriction on not being able to use an ECDH key with ECDSA. There is no longer any OID specified when importing a key, so we can't reject importing of an ECDH key for the purpose of ECDSA based on the OID.

Move pk-using code to inside psa_import_rsa_key(). This aligns the shape of psa_import_rsa_key() to match that of psa_import_ec_private_key() and psa_import_ec_public_key().

Remove extra status handling code from psa_import_key_into_slot(). This helps save a tiny amount of code space, but mainly serves to improve the readability of the code.

Document `peer_key` parameter requirements, including an explanation of how the peer key is used and an example for EC keys.

Patater · 2019-01-23T17:40:42Z

Rebased to remove stale reference to ECPoint in crypto_sizes.h.

Patater added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. needs: ci Needs a passing full CI run labels Jan 11, 2019

Patater requested review from gilles-peskine-arm and dgreen-arm January 11, 2019 11:44

Patater force-pushed the pubkey-format branch from 0ca50fb to 990d025 Compare January 11, 2019 11:55

Patater force-pushed the pubkey-format branch from 990d025 to 693b4c4 Compare January 11, 2019 15:43

Patater force-pushed the pubkey-format branch from 693b4c4 to 1c3c49e Compare January 11, 2019 15:53

Patater force-pushed the pubkey-format branch from 1c3c49e to 990d025 Compare January 11, 2019 16:38

gilles-peskine-arm requested changes Jan 11, 2019

View reviewed changes

Patater force-pushed the pubkey-format branch from 990d025 to 2920c1b Compare January 11, 2019 17:10

psa: Document requirements for psa_export_public_key()

d3a0c2c

Copy the nice and clear documentation from psa_export_key() as to what implementations are allowed to do regarding key export formats, as the same applies to public keys.

gilles-peskine-arm requested changes Jan 11, 2019

View reviewed changes

Patater force-pushed the pubkey-format branch from 2920c1b to 936f278 Compare January 11, 2019 18:59

Patater changed the title ~~Simplify RSA and EC public key formats~~ Simplify or specify public key formats Jan 11, 2019

Patater force-pushed the pubkey-format branch from 936f278 to 7bc988f Compare January 11, 2019 19:07

dgreen-arm reviewed Jan 14, 2019

View reviewed changes

Patater force-pushed the pubkey-format branch from 7bc988f to b0ce828 Compare January 14, 2019 10:37

Patater mentioned this pull request Jan 14, 2019

Simplify or specify public key format for DH and DSA #14

Closed

Patater changed the title ~~Simplify or specify public key formats~~ Simplify public key formats for RSA and ECC Jan 14, 2019

Patater changed the title ~~Simplify public key formats for RSA and ECC~~ Simplify RSA and EC public key formats Jan 14, 2019

gilles-peskine-arm removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Jan 16, 2019

yanesca reviewed Jan 18, 2019

View reviewed changes

hanno-becker reviewed Jan 21, 2019

View reviewed changes

Patater added 2 commits January 23, 2019 17:31

psa: Simplify RSA public key format

25384a2

Remove pkcs-1 and rsaEncryption front matter from RSA public keys. Move code that was shared between RSA and other key types (like EC keys) to be used only with non-RSA keys.

Patater dismissed dgreen-arm’s stale review via bc1759c January 23, 2019 17:36

Patater force-pushed the pubkey-format branch from 8afbff8 to bc1759c Compare January 23, 2019 17:36

Patater and others added 4 commits January 23, 2019 17:39

psa: Refactor psa_import_rsa_key() pk-using code

cd09d8c

Move pk-using code to inside psa_import_rsa_key(). This aligns the shape of psa_import_rsa_key() to match that of psa_import_ec_private_key() and psa_import_ec_public_key().

psa: Remove extra status handling from import

c67200d

Remove extra status handling code from psa_import_key_into_slot(). This helps save a tiny amount of code space, but mainly serves to improve the readability of the code.

psa: Expand documentation for psa_key_agreement()

21fec0c

Document `peer_key` parameter requirements, including an explanation of how the peer key is used and an example for EC keys.

Patater force-pushed the pubkey-format branch from bc1759c to 21fec0c Compare January 23, 2019 17:40

Patater added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Jan 23, 2019

dgreen-arm approved these changes Jan 25, 2019

View reviewed changes

gilles-peskine-arm approved these changes Jan 25, 2019

View reviewed changes

Patater merged commit 4d69cf1 into ARMmbed:development Jan 25, 2019

This was referenced Jan 25, 2019

Adapt ECDSA wrapper to new EC public key format #28

Merged

PSA Integration: Add support for client-side PSA-based ECDHE Mbed-TLS/mbedtls#2338

Merged

Simplify RSA and EC public key formats #13

Simplify RSA and EC public key formats #13

Uh oh!

Conversation

Patater commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patater commented Jan 11, 2019

Uh oh!

Patater commented Jan 11, 2019

Uh oh!

Patater commented Jan 11, 2019

Uh oh!

Patater commented Jan 11, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Patater commented Jan 11, 2019

Uh oh!

gilles-peskine-arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Patater commented Jan 11, 2019

Uh oh!

Patater commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater commented Jan 14, 2019

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Patater commented Jan 11, 2019 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading