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

2019-01-14T04:42:10-06:00

Patater commented

2019-01-14 04:42:10 -06:00

(Migrated from github.com)

This PR builds on top of #13 (starting with 63455af9dcf7 "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 #13 after that's merged.

This PR builds on top of #13 (starting with 63455af9dcf7 "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 #13 after that's merged.

dgreen-arm (Migrated from github.com) requested changes

2019-01-14 05:57:23 -06:00

include/psa/crypto.h

					
				@ -372,0 +404,4 @@

				 *   ```

				 *

				 * \param handle      Handle to the key to set domain parameters for.

				 * \param[in] data    Buffer containing the key domain parameters. The content

Documentation for handle parameter is missing.

include/psa/crypto.h

					
				@ -372,0 +435,4 @@

				 * documentation for psa_set_key_domain_parameters().

				 *

				 * \param handle            Handle to the key to get domain parameters from.

				 * \param[out] data         On success, the key domain parameters.

Documentation for handle parameter is missing.

Patater commented

2019-01-14 07:21:09 -06:00

(Migrated from github.com)

Rebased to add Doxygen documentation for handle parameters.

gilles-peskine-arm (Migrated from github.com) requested changes

2019-01-14 12:33:52 -06:00

include/psa/crypto.h

					
				@ -372,0 +379,4 @@

				 *

				 * 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”

“the DER representation of the `Dss-Parms` format”

include/psa/crypto.h

					
				@ -372,0 +388,4 @@

				 *   }

				 *   ```

				 * - 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”

“the DER representation of the `DomainParameters` format”

include/psa/crypto.h

					
				@ -372,0 +395,4 @@

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

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.

include/psa/crypto.h

					
				@ -372,0 +405,4 @@

				 *

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

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

Something's missing before “of psa_export_key() or …”

Something's missing before “of `psa_export_key()` or …”

include/psa/crypto.h

					
				@ -372,0 +419,4 @@

				 * \retval #PSA_ERROR_HARDWARE_FAILURE

				 * \retval #PSA_ERROR_TAMPERING_DETECTED

				 * \retval #PSA_ERROR_BAD_STATE

				 *         The library has not been previously initialized by psa_crypto_init().

Not relevant for this function (you won't get a handle in the first place)

include/psa/crypto.h

					
				@ -372,0 +443,4 @@

				 * \retval #PSA_SUCCESS

				 * \retval #PSA_ERROR_INVALID_HANDLE

				 * \retval #PSA_ERROR_EMPTY_SLOT

				 *         There is no key in the specified slot.

That's not an error: you can read domain parameters that have been set even before importing a key. The possible errors here are that no domain parameters have been set, or that the slot contains a key which does not have any domain parameters.

include/psa/crypto.h

					
				@ -372,0 +444,4 @@

				 * \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

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

include/psa/crypto.h

					
				@ -372,0 +450,4 @@

				 * \retval #PSA_ERROR_HARDWARE_FAILURE

				 * \retval #PSA_ERROR_TAMPERING_DETECTED

				 * \retval #PSA_ERROR_BAD_STATE

				 *         The library has not been previously initialized by psa_crypto_init().

Not relevant for this function (you won't get a handle in the first place)

include/psa/crypto.h

					
				@ -372,0 +453,4 @@

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

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

include/psa/crypto.h

“key pairs” → “private keys” for consistency

include/psa/crypto.h

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

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.

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.

include/psa/crypto.h

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

yanesca (Migrated from github.com) reviewed

2019-01-18 09:21:33 -06:00

include/psa/crypto.h

					
				@ -372,0 +395,4 @@

				 *      g               INTEGER,                    -- generator, g

				 *      q               INTEGER,                    -- factor of p-1

				 *      j               INTEGER OPTIONAL,           -- subgroup factor

				 *      validationParms ValidationParms OPTIONAL

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

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 commented

2019-01-18 10:13:31 -06:00

(Migrated from github.com)

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

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 commented

2019-09-04 02:52:17 -05:00

(Migrated from github.com)