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

Patater · 2019-01-14T10:42:10Z

This PR builds on top of #13 (starting with 63455af "psa: Add get/set domain parameters") to simplify the DSA key format and to specify the DH key format.

TODO:

Determine what we want the API to look like for setting domain parameters and update the PR to reflect that choice.
Rebase on top of Simplify RSA and EC public key formats #13 after that's merged.

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.

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.

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

Remove front matter from our EC key format, to make it just 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().

dgreen-arm · 2019-01-14T11:55:06Z

include/psa/crypto.h

+ *
+ * The format for the required domain parameters varies by the key type.
+ *
+ * \param[in] data    Buffer containing the key domain parameters. The content


Documentation for handle parameter is missing.

dgreen-arm · 2019-01-14T11:55:34Z

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.


Documentation for handle parameter is missing.

DSA and static DH need extra domain parameters. Instead of passing these in with the keys themselves, add get and set functions to set and retrieve this information about keys.

Remove front matter and DSS parameters from our DSA key formats, both keypair and public key, to make it just a representation of the integer private key, `x`, or the public key, `y`, respectively.

Add the ability to specify Diffie-Hellman key exchange keys. Specify the import/export format as well, even though importing and exporting isn't implemented yet.

Patater · 2019-01-14T13:21:09Z

Rebased to add Doxygen documentation for handle parameters.

gilles-peskine-arm · 2019-01-14T18:10:18Z

include/psa/crypto.h

@@ -428,6 +509,10 @@ psa_status_t psa_get_key_information(psa_key_handle_t handle,
 *   and `PSA_ECC_CURVE_BRAINPOOL_PXXX`).
 *   This is the content of the `privateKey` field of the `ECPrivateKey`
 *   format defined by RFC 5915.
+ * - For Diffie-Hellman key exchange key pairs (#PSA_KEY_TYPE_DH_KEYPAIR), the


“key pairs” → “private keys” for consistency

gilles-peskine-arm · 2019-01-14T18:14:14Z

include/psa/crypto.h

+ *
+ * The format for the required domain parameters varies by the key type.
+ * - For DSA public keys (#PSA_KEY_TYPE_DSA_PUBLIC_KEY),
+ *   the `Dss-Parms` format as defined by RFC 3279 &sect;2.3.2.


“the DER representation of the Dss-Parms format”

gilles-peskine-arm · 2019-01-14T18:14:28Z

include/psa/crypto.h

+ *   }
+ *   ```
+ * - For Diffie-Hellman key exchange keys (#PSA_KEY_TYPE_DH_PUBLIC_KEY), the
+ *   `DomainParameters` format as defined by RFC 3279 &sect;2.3.3.


“the DER representation of the DomainParameters format”

gilles-peskine-arm · 2019-01-14T18:17:04Z

include/psa/crypto.h

+ *      g               INTEGER,                    -- generator, g
+ *      q               INTEGER,                    -- factor of p-1
+ *      j               INTEGER OPTIONAL,           -- subgroup factor
+ *      validationParms ValidationParms OPTIONAL


We try to avoid optional stuff in PSA. Do we allow, mandate or reject j and validationParms? If it's allowed, do we guarantee that the implementation will check it?

My gut feeling:

Allow them because it's convenient to accept whatever has been package, and it's very easy to skip.

Allow an implementation to ignore them altogether, because verifying them can be costly and the API is meant to be suitable for microcontrollers.

That sounds very good for usability.

Considering it from the security perspective the checks a specific implementation makes have significant impact and I think that we should be very explicit about it. How about allowing the validationParams to be present and allowing the implementations to ignore them, but adding a new function for parameter validation?

By the way can't we just drop custom parameter FFDH from 1.0 and use named groups instead. We could reintroduce custom parameters if and when customers ask for it.
(I know I have asked this before, but I don't remember what was the conclusion.)

I dislike custom parameter FFDH, because it can be backdoored and no matter how hard you validate your parameters, you won't notice:
https://tls.mbed.org/tech-updates/blog/dh-backdoors

(Of course you can argue that named groups have to be bigger because of logjam, but I wouldn't advise to use keys that short with custom parameters either.)

gilles-peskine-arm · 2019-01-14T18:17:43Z

include/psa/crypto.h

+ *
+ * \param handle      Handle to the key to set domain parameters for.
+ * \param[in] data    Buffer containing the key domain parameters. The content
+ *                    of this buffer is interpreted according to \p type. of


Missing \p type parameter in the documentation and in the prototype.

gilles-peskine-arm · 2019-01-14T18:22:30Z

include/psa/crypto.h

+ * \retval #PSA_ERROR_INVALID_HANDLE
+ * \retval #PSA_ERROR_EMPTY_SLOT
+ *         There is no key in the specified slot.
+ * \retval #PSA_ERROR_INVALID_ARGUMENT


+ NOT_SUPPORTED if the implementation doesn't have code to format domain parameters

gilles-peskine-arm · 2019-01-14T18:24:32Z

include/psa/crypto.h

@@ -2253,6 +2319,18 @@ typedef struct {
 *                            specifying the public exponent. The
 *                            default public exponent used when \p extra
 *                            is \c NULL is 65537.
+ *                          - For an DSA key (\p type is
+ *                            #PSA_KEY_TYPE_DSA_KEYPAIR), \p extra is an
+ *                            optional structure specifying the key domain


It isn't a structure, it's a buffer that will be parsed.

gilles-peskine-arm · 2019-01-14T18:28:11Z

include/psa/crypto.h

@@ -2253,6 +2319,18 @@ typedef struct {
 *                            specifying the public exponent. The
 *                            default public exponent used when \p extra
 *                            is \c NULL is 65537.
+ *                          - For an DSA key (\p type is
+ *                            #PSA_KEY_TYPE_DSA_KEYPAIR), \p extra is an
+ *                            optional structure specifying the key domain


I find this description unclear: it looks like you can pass extra, use set_key_domain_parameters, or do neither. In fact you need to do at least one, and the last one applies.

For a DSA key (\p type is #PSA_KEY_TYPE_DSA_KEYPAIR) or a finite-field Diffie-Hellman key (p type is #PSA_KEY_TYPE_DH_KEYPAIR), extra is a pointer to a buffer containing domain parameters in DER representation. The format of buffer is the same as the input to psa_set_key_domain_parameters(). If the application has previously called psa_set_key_domain_parameters() on \p handle, it may pass an empty buffer (`extra_length = 0`), in which case the key generation will use the previously set domain parameters.

gilles-peskine-arm · 2019-01-14T18:28:28Z

include/psa/crypto.h

+ *                            parameters. The key domain parameters can also be
+ *                            provided by psa_set_key_domain_parameters(),
+ *                            which documents the format of the structure.
+ *                          - For a DH key (\p type is


The logic is exactly the same for DSA and DH, so you can combine their documentation.

gilles-peskine-arm · 2019-01-14T18:33:25Z

include/psa/crypto.h

+ *         The library has not been previously initialized by psa_crypto_init().
+ *         It is implementation-dependent whether a failure to initialize
+ *         results in this error code.
+ */


Needs a macro in crypto_sizes.h to determine the buffer size from the key type and bit-size.

gilles-peskine-arm · 2019-01-18T16:13:31Z

For the beta1 API release, I'm adding a type parameter to psa_set_key_domain_parameters. https://github.com/gilles-peskine-arm/mbed-crypto/tree/pubkey-format-dh-dsa-pass_type_to_set_domain_parameters

Patater · 2019-09-04T07:52:17Z

Closing as very out of date. Needs to be redone in a PSA Crypto API 1.0b3 way.

Patater and others added 6 commits January 10, 2019 20:25

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.

psa: Simplify RSA public key format

6b19600

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.

psa: Refactor psa_import_rsa_key() pk-using code

b0ce828

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

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: work The pull request needs rework before it can be merged. needs: design review labels Jan 14, 2019

Patater requested review from gilles-peskine-arm and dgreen-arm January 14, 2019 10:42

Patater mentioned this pull request Jan 14, 2019

Simplify RSA and EC public key formats #13

Merged

dgreen-arm suggested changes Jan 14, 2019

View reviewed changes

Patater added 3 commits January 14, 2019 13:20

psa: Add get/set domain parameters

b4019bc

DSA and static DH need extra domain parameters. Instead of passing these in with the keys themselves, add get and set functions to set and retrieve this information about keys.

psa: Simplify DSA key formats

08bbd13

Remove front matter and DSS parameters from our DSA key formats, both keypair and public key, to make it just a representation of the integer private key, `x`, or the public key, `y`, respectively.

psa: Add DH key exchange keys

dfe6582

Add the ability to specify Diffie-Hellman key exchange keys. Specify the import/export format as well, even though importing and exporting isn't implemented yet.

Patater force-pushed the pubkey-format-dh-dsa branch from 0a07b34 to dfe6582 Compare January 14, 2019 13:20

gilles-peskine-arm requested changes Jan 14, 2019

View reviewed changes

Patater closed this Sep 4, 2019

Patater added the stale This PR is not being actively worked on. It can be considered abandoned work. label Sep 4, 2019

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

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

Uh oh!

Conversation

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!

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!

gilles-peskine-arm commented Jan 18, 2019

Uh oh!

Patater commented Sep 4, 2019

Uh oh!

Uh oh!