From e5119898e4df73cd52372043f8910c6299a5a1a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 9 Dec 2021 11:45:03 +0100 Subject: [PATCH 01/11] Improve a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_cli.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e411b70490..1233676afc 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2366,9 +2366,16 @@ static int ssl_parse_server_ecdh_params_psa( mbedtls_ssl_context *ssl, mbedtls_ssl_handshake_params *handshake = ssl->handshake; /* - * Parse ECC group + * struct { + * ECParameters curve_params; + * ECPoint public; + * } ServerECDHParams; + * + * 1 curve_type (must be named curve) + * 2..3 NamedCurve + * 4 ECPoint.len + * 5+ ECPoint contents */ - if( end - *p < 4 ) return( MBEDTLS_ERR_SSL_DECODE_ERROR ); From bc4069596b6d444c66c9cc1d476beda215753ff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Jan 2022 11:18:20 +0100 Subject: [PATCH 02/11] Group related functions together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We had ECC then PK then ECC, move PK to the end, now all ECC things are together. (The comments suggest that was the intention all along.) Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/psa_util.h | 58 ++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/include/mbedtls/psa_util.h b/include/mbedtls/psa_util.h index c54c035c37..892d609391 100644 --- a/include/mbedtls/psa_util.h +++ b/include/mbedtls/psa_util.h @@ -338,36 +338,6 @@ static inline int mbedtls_psa_get_ecc_oid_from_id( #endif /* MBEDTLS_ECP_DP_BP512R1_ENABLED */ -/* Translations for PK layer */ - -static inline int mbedtls_psa_err_translate_pk( psa_status_t status ) -{ - switch( status ) - { - case PSA_SUCCESS: - return( 0 ); - case PSA_ERROR_NOT_SUPPORTED: - return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); - case PSA_ERROR_INSUFFICIENT_MEMORY: - return( MBEDTLS_ERR_PK_ALLOC_FAILED ); - case PSA_ERROR_INSUFFICIENT_ENTROPY: - return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); - case PSA_ERROR_BAD_STATE: - return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); - /* All other failures */ - case PSA_ERROR_COMMUNICATION_FAILURE: - case PSA_ERROR_HARDWARE_FAILURE: - case PSA_ERROR_CORRUPTION_DETECTED: - return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED ); - default: /* We return the same as for the 'other failures', - * but list them separately nonetheless to indicate - * which failure conditions we have considered. */ - return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED ); - } -} - -/* Translations for ECC */ - /* This function transforms an ECC group identifier from * https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 * into a PSA ECC group identifier. */ @@ -423,6 +393,34 @@ static inline int mbedtls_psa_tls_ecpoint_to_psa_ec( unsigned char const *src, return( 0 ); } +/* Translations for PK layer */ + +static inline int mbedtls_psa_err_translate_pk( psa_status_t status ) +{ + switch( status ) + { + case PSA_SUCCESS: + return( 0 ); + case PSA_ERROR_NOT_SUPPORTED: + return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); + case PSA_ERROR_INSUFFICIENT_MEMORY: + return( MBEDTLS_ERR_PK_ALLOC_FAILED ); + case PSA_ERROR_INSUFFICIENT_ENTROPY: + return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); + case PSA_ERROR_BAD_STATE: + return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + /* All other failures */ + case PSA_ERROR_COMMUNICATION_FAILURE: + case PSA_ERROR_HARDWARE_FAILURE: + case PSA_ERROR_CORRUPTION_DETECTED: + return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED ); + default: /* We return the same as for the 'other failures', + * but list them separately nonetheless to indicate + * which failure conditions we have considered. */ + return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED ); + } +} + #endif /* MBEDTLS_USE_PSA_CRYPTO */ /* Expose whatever RNG the PSA subsystem uses to applications using the From 59753768f0d321f657f11f2809b33f6306c54e13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Jan 2022 11:52:11 +0100 Subject: [PATCH 03/11] Simplify the definition of a macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relying on a PSA_VENDOR macro is not ideal, since the standard doesn't guarantee this macro exists, but OTOH relying on MBEDTLS_ECP_DP_xxx_ENABLED was even less ideal, so I believe this is still an improvement. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/psa_util.h | 81 +------------------------------------- 1 file changed, 2 insertions(+), 79 deletions(-) diff --git a/include/mbedtls/psa_util.h b/include/mbedtls/psa_util.h index 892d609391..917799c12b 100644 --- a/include/mbedtls/psa_util.h +++ b/include/mbedtls/psa_util.h @@ -258,85 +258,8 @@ static inline int mbedtls_psa_get_ecc_oid_from_id( return( -1 ); } -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH 1 - -#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 192 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 192 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP192R1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 224 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 224 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP224R1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 256 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 256 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP256R1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 384 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 384 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP384R1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 521 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 521 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP521R1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 192 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 192 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP192K1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 224 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 224 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP224K1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 256 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 256 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_SECP256K1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 256 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 256 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_BP256R1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 384 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 384 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_BP384R1_ENABLED */ - -#if defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) -#if MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH < ( 2 * ( ( 512 + 7 ) / 8 ) + 1 ) -#undef MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH -#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH ( 2 * ( ( 512 + 7 ) / 8 ) + 1 ) -#endif -#endif /* MBEDTLS_ECP_DP_BP512R1_ENABLED */ - +#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH \ + PSA_KEY_EXPORT_ECC_PUBLIC_KEY_MAX_SIZE( PSA_VENDOR_ECC_MAX_CURVE_BITS ) /* This function transforms an ECC group identifier from * https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 From 58d2383ef458e61eddeb06cbccd5a16c5d9cec6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Jan 2022 12:17:15 +0100 Subject: [PATCH 04/11] Remove mbedtls_psa_tls_psa_ec_to_ecpoint() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Initially this function was doing something because the output format of psa_export_public() didn't match the ECPoint format that TLS wants. Then it became a no-op then the output format of psa_export_public() changed, but it made sense to still keep the function in case the format changed again. Now that the PSA Crypto API has reached 1.0 status, this is unlikely to happen, so the no-op function is no longer useful. Removing it de-clutters the code a bit; while at it we can remove a temporary stack buffer (that was up to 133 bytes). It's OK to remove this function even if it was declared in a public header, as there's a warning at the top of the file saying it's not part of the public API. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/psa_util.h | 20 -------------------- library/ssl_cli.c | 32 +++++++++++--------------------- 2 files changed, 11 insertions(+), 41 deletions(-) diff --git a/include/mbedtls/psa_util.h b/include/mbedtls/psa_util.h index 917799c12b..603be8970e 100644 --- a/include/mbedtls/psa_util.h +++ b/include/mbedtls/psa_util.h @@ -277,26 +277,6 @@ static inline psa_key_type_t mbedtls_psa_parse_tls_ecc_group( } #endif /* MBEDTLS_ECP_C */ -/* This function takes a buffer holding an EC public key - * exported through psa_export_public_key(), and converts - * it into an ECPoint structure to be put into a ClientKeyExchange - * message in an ECDHE exchange. - * - * Both the present and the foreseeable future format of EC public keys - * used by PSA have the ECPoint structure contained in the exported key - * as a subbuffer, and the function merely selects this subbuffer instead - * of making a copy. - */ -static inline int mbedtls_psa_tls_psa_ec_to_ecpoint( unsigned char *src, - size_t srclen, - unsigned char **dst, - size_t *dstlen ) -{ - *dst = src; - *dstlen = srclen; - return( 0 ); -} - /* This function takes a buffer holding an ECPoint structure * (as contained in a TLS ServerKeyExchange message for ECDHE * exchanges) and converts it into a format that the PSA key diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 1233676afc..40b87dda69 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3379,11 +3379,6 @@ static int ssl_write_client_key_exchange( mbedtls_ssl_context *ssl ) mbedtls_ssl_handshake_params *handshake = ssl->handshake; - unsigned char own_pubkey[MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH]; - size_t own_pubkey_len; - unsigned char *own_pubkey_ecpoint; - size_t own_pubkey_ecpoint_len; - header_len = 4; MBEDTLS_SSL_DEBUG_MSG( 1, ( "Perform PSA-based ECDH computation." ) ); @@ -3411,27 +3406,22 @@ static int ssl_write_client_key_exchange( mbedtls_ssl_context *ssl ) if( status != PSA_SUCCESS ) return( MBEDTLS_ERR_SSL_HW_ACCEL_FAILED ); - /* Export the public part of the ECDH private key from PSA - * and convert it to ECPoint format used in ClientKeyExchange. */ + /* Export the public part of the ECDH private key from PSA. + * The export format is an ECPoint structre as expected by TLS, + * but we just need to add a length byte before that. */ + unsigned char *own_pubkey = ssl->out_msg + header_len + 1; + unsigned char *end = ssl->out_msg + MBEDTLS_SSL_OUT_CONTENT_LEN; + size_t own_pubkey_max_len = (size_t)( end - own_pubkey ); + size_t own_pubkey_len; + status = psa_export_public_key( handshake->ecdh_psa_privkey, - own_pubkey, sizeof( own_pubkey ), + own_pubkey, own_pubkey_max_len, &own_pubkey_len ); if( status != PSA_SUCCESS ) return( MBEDTLS_ERR_SSL_HW_ACCEL_FAILED ); - if( mbedtls_psa_tls_psa_ec_to_ecpoint( own_pubkey, - own_pubkey_len, - &own_pubkey_ecpoint, - &own_pubkey_ecpoint_len ) != 0 ) - { - return( MBEDTLS_ERR_SSL_HW_ACCEL_FAILED ); - } - - /* Copy ECPoint structure to outgoing message buffer. */ - ssl->out_msg[header_len] = (unsigned char) own_pubkey_ecpoint_len; - memcpy( ssl->out_msg + header_len + 1, - own_pubkey_ecpoint, own_pubkey_ecpoint_len ); - content_len = own_pubkey_ecpoint_len + 1; + ssl->out_msg[header_len] = (unsigned char) own_pubkey_len; + content_len = own_pubkey_len + 1; /* The ECDH secret is the premaster secret used for key derivation. */ From 4a0ac1f160f126a2f76073b3accd1f5b4c72e4f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Jan 2022 12:30:40 +0100 Subject: [PATCH 05/11] Remove mbedtls_psa_tls_ecpoint_to_psa_ec() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same reasons as for the previous commit. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/psa_util.h | 19 ------------------- library/ssl_cli.c | 18 ++++++------------ 2 files changed, 6 insertions(+), 31 deletions(-) diff --git a/include/mbedtls/psa_util.h b/include/mbedtls/psa_util.h index 603be8970e..e38e2e3469 100644 --- a/include/mbedtls/psa_util.h +++ b/include/mbedtls/psa_util.h @@ -277,25 +277,6 @@ static inline psa_key_type_t mbedtls_psa_parse_tls_ecc_group( } #endif /* MBEDTLS_ECP_C */ -/* This function takes a buffer holding an ECPoint structure - * (as contained in a TLS ServerKeyExchange message for ECDHE - * exchanges) and converts it into a format that the PSA key - * agreement API understands. - */ -static inline int mbedtls_psa_tls_ecpoint_to_psa_ec( unsigned char const *src, - size_t srclen, - unsigned char *dst, - size_t dstlen, - size_t *olen ) -{ - if( srclen > dstlen ) - return( MBEDTLS_ERR_ECP_BUFFER_TOO_SMALL ); - - memcpy( dst, src, srclen ); - *olen = srclen; - return( 0 ); -} - /* Translations for PK layer */ static inline int mbedtls_psa_err_translate_pk( psa_status_t status ) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 40b87dda69..1ce9183765 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2398,24 +2398,18 @@ static int ssl_parse_server_ecdh_params_psa( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); handshake->ecdh_bits = (uint16_t) ecdh_bits; - /* - * Put peer's ECDH public key in the format understood by PSA. - */ - + /* Keep a copy of the peer's public key */ ecpoint_len = *(*p)++; if( (size_t)( end - *p ) < ecpoint_len ) return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - if( mbedtls_psa_tls_ecpoint_to_psa_ec( - *p, ecpoint_len, - handshake->ecdh_psa_peerkey, - sizeof( handshake->ecdh_psa_peerkey ), - &handshake->ecdh_psa_peerkey_len ) != 0 ) - { - return( MBEDTLS_ERR_SSL_HW_ACCEL_FAILED ); - } + if( ecpoint_len > sizeof( handshake->ecdh_psa_peerkey ) ) + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + memcpy( handshake->ecdh_psa_peerkey, *p, ecpoint_len ); + handshake->ecdh_psa_peerkey_len = ecpoint_len; *p += ecpoint_len; + return( 0 ); } #endif /* MBEDTLS_USE_PSA_CRYPTO && From 3caa0edb9ba026d41dd4f323767af175d44d3307 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Jan 2022 12:41:48 +0100 Subject: [PATCH 06/11] Remove dead preprocessor code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's no way currently (see below regarding the future) that ECC-based key exchanges are enabled without ECP_C being defined. So, the #if was fully redundant with the checks surrounding the function, as it always evaluated to true. The situation arose as, in the old days (before Mbed TLS 2.0), mbedtls_ssl_conf_curves() (or ssl_set_curves() as it was called back then) was optional, controlled by its own compile-time option POLARSSL_SSL_SET_CURVES. So, in turn mbedtls_ssl_check_curve() depended on POLARSSL_SSL_SET_CURVES too, and all calls to it were guarded by that. When it was made non-optional, a blind s/POLARSSL_SSL_SET_CURVES/MBEDTLS_ECP_C/ was done, which resulted in stupid situations like this with redundant checks for ECP_C. Note regarding the future: at some point it will be possible to compile with ECC-based key exchanges but without ECP_C. This doesn't change anything to the reasoning above: mbedtls_ssl_check_curve() will be available in all builds where ECC is used; it will just need a new definition (with new guards), but that doesn't change anything for its callers. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_cli.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 1ce9183765..a9691bf962 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2334,12 +2334,7 @@ static int ssl_check_server_ecdh_params( const mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDH curve: %s", curve_info->name ) ); -#if defined(MBEDTLS_ECP_C) if( mbedtls_ssl_check_curve( ssl, grp_id ) != 0 ) -#else - if( ssl->handshake->ecdh_ctx.grp.nbits < 163 || - ssl->handshake->ecdh_ctx.grp.nbits > 521 ) -#endif return( -1 ); MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, From 0d63b84fa49ecb758dbec4fd7a94df59fe8367ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Jan 2022 13:10:56 +0100 Subject: [PATCH 07/11] Add mbedtls_ssl_check_curve_tls_id() (internal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This can be used to validate the server's choice of group in the PSA case (this will be done in the next commit). Note that new function doesn't depend on ECP_C, as it only requires mbedtls_ssl_get_groups(), which is always available. As a general rule, functions for defining and enforcing policy in the TLS module should not depend on low-level modules but work with TLS-level identifiers are much as possible, and this new function follows that principle. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_misc.h | 1 + library/ssl_tls.c | 14 +++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index ad358b3693..f86e3c6c05 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1288,6 +1288,7 @@ mbedtls_md_type_t mbedtls_ssl_md_alg_from_hash( unsigned char hash ); unsigned char mbedtls_ssl_hash_from_md_alg( int md ); int mbedtls_ssl_set_calc_verify_md( mbedtls_ssl_context *ssl, int md ); +int mbedtls_ssl_check_curve_tls_id( const mbedtls_ssl_context *ssl, uint16_t tls_id ); #if defined(MBEDTLS_ECP_C) int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id ); #endif diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f261a6a89a..a4737becc1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7054,18 +7054,16 @@ unsigned char mbedtls_ssl_hash_from_md_alg( int md ) } } -#if defined(MBEDTLS_ECP_C) /* * Check if a curve proposed by the peer is in our list. * Return 0 if we're willing to use it, -1 otherwise. */ -int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id ) +int mbedtls_ssl_check_curve_tls_id( const mbedtls_ssl_context *ssl, uint16_t tls_id ) { const uint16_t *group_list = mbedtls_ssl_get_groups( ssl ); if( group_list == NULL ) return( -1 ); - uint16_t tls_id = mbedtls_ecp_curve_info_from_grp_id(grp_id)->tls_id; for( ; *group_list != 0; group_list++ ) { @@ -7075,6 +7073,16 @@ int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_i return( -1 ); } + +#if defined(MBEDTLS_ECP_C) +/* + * Same as mbedtls_ssl_check_curve_tls_id() but with a mbedtls_ecp_group_id. + */ +int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id ) +{ + uint16_t tls_id = mbedtls_ecp_curve_info_from_grp_id(grp_id)->tls_id; + return mbedtls_ssl_check_curve_tls_id( ssl, tls_id ); +} #endif /* MBEDTLS_ECP_C */ #if defined(MBEDTLS_X509_CRT_PARSE_C) From 141be6cc7faeb68296625670b851670542481ab6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Jan 2022 11:46:19 +0100 Subject: [PATCH 08/11] Fix missing check on server-chosen curve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We had this check in the non-PSA case, but it was missing in the PSA case. Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/use-psa-ecdhe-curve.txt | 7 +++++++ library/ssl_cli.c | 4 ++++ 2 files changed, 11 insertions(+) create mode 100644 ChangeLog.d/use-psa-ecdhe-curve.txt diff --git a/ChangeLog.d/use-psa-ecdhe-curve.txt b/ChangeLog.d/use-psa-ecdhe-curve.txt new file mode 100644 index 0000000000..cc432bdaee --- /dev/null +++ b/ChangeLog.d/use-psa-ecdhe-curve.txt @@ -0,0 +1,7 @@ +Bugfix + * Fix a bug in (D)TLS curve negotiation: when MBEDTLS_USE_PSA_CRYPTO was + enabled and an ECDHE-ECDSA or ECDHE-RSA key exchange was used, the + client would fail to check that the curve selected by the server for + ECDHE was indeed one that was offered. As a result, the client would + accept any curve that it supported, even if that curve was not allowed + according to its configuration. diff --git a/library/ssl_cli.c b/library/ssl_cli.c index a9691bf962..59b22435ee 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2383,6 +2383,10 @@ static int ssl_parse_server_ecdh_params_psa( mbedtls_ssl_context *ssl, tls_id <<= 8; tls_id |= *(*p)++; + /* Check it's a curve we offered */ + if( mbedtls_ssl_check_curve_tls_id( ssl, tls_id ) != 0 ) + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + /* Convert EC group to PSA key type. */ if( ( handshake->ecdh_psa_type = mbedtls_psa_parse_tls_ecc_group( tls_id, &ecdh_bits ) ) == 0 ) From 422370d6333c5e09558f9564c853ad7abec773b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Feb 2022 11:55:21 +0100 Subject: [PATCH 09/11] Improve a comment and fix some whitespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_cli.c | 2 +- library/ssl_tls.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 59b22435ee..75f2256522 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2366,7 +2366,7 @@ static int ssl_parse_server_ecdh_params_psa( mbedtls_ssl_context *ssl, * ECPoint public; * } ServerECDHParams; * - * 1 curve_type (must be named curve) + * 1 curve_type (must be "named_curve") * 2..3 NamedCurve * 4 ECPoint.len * 5+ ECPoint contents diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a4737becc1..58d16e8ba8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7080,7 +7080,7 @@ int mbedtls_ssl_check_curve_tls_id( const mbedtls_ssl_context *ssl, uint16_t tls */ int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id ) { - uint16_t tls_id = mbedtls_ecp_curve_info_from_grp_id(grp_id)->tls_id; + uint16_t tls_id = mbedtls_ecp_curve_info_from_grp_id( grp_id )->tls_id; return mbedtls_ssl_check_curve_tls_id( ssl, tls_id ); } #endif /* MBEDTLS_ECP_C */ From ff229cf639c28b0f1af66bb5b1353bddd32d2e26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Feb 2022 12:00:32 +0100 Subject: [PATCH 10/11] Add debug message for wrong curve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The non-PSA path has a debug message here, so let's have a similar one in the PSA case - just add the curve ID to be a bit more informative. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_cli.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 75f2256522..1a5a25f9bc 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2385,7 +2385,12 @@ static int ssl_parse_server_ecdh_params_psa( mbedtls_ssl_context *ssl, /* Check it's a curve we offered */ if( mbedtls_ssl_check_curve_tls_id( ssl, tls_id ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, + ( "bad server key exchange message (ECDHE curve): %u", + (unsigned) tls_id ) ); return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } /* Convert EC group to PSA key type. */ if( ( handshake->ecdh_psa_type = From 5d6053f54894c65bccf7ea04ccab9171b8aa7e54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Feb 2022 10:26:19 +0100 Subject: [PATCH 11/11] Fix a typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_cli.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 1a5a25f9bc..4f1bf51e01 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3405,7 +3405,7 @@ static int ssl_write_client_key_exchange( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_HW_ACCEL_FAILED ); /* Export the public part of the ECDH private key from PSA. - * The export format is an ECPoint structre as expected by TLS, + * The export format is an ECPoint structure as expected by TLS, * but we just need to add a length byte before that. */ unsigned char *own_pubkey = ssl->out_msg + header_len + 1; unsigned char *end = ssl->out_msg + MBEDTLS_SSL_OUT_CONTENT_LEN;