From d3b2fcb7c6e63c9b4e33babc110510a50e773c88 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 17 Jun 2019 14:30:05 +0100 Subject: [PATCH] Don't store client-supported ECs in heap-allocated buffer So far, the client-proposed list of elliptic curves was stored for the duration of the entire handshake in a heap-allocated buffer referenced from mbedtls_ssl_handshake_params::curves. It is used in the following places: 1) When the server chooses a suitable ciphersuite, it checks that it has a certificate matching the ciphersuite; in particular, if the ciphersuite involves ECDHE, the server needs an EC certificate with a curve suitable for the client. 2) When performing the ECDHE key exchange, the server choose one curve among those proposed by the client which matches the server's own supported curve configuration. This commit removes the hold back the entire client-side curve list during the handshake, by performing (1) and (2) on during ClientHello parsing, and in case of (2) only remembering the curve chosen for ECDHE within mbedtls_ssl_handshake_params. --- include/mbedtls/ssl_internal.h | 2 +- library/ssl_srv.c | 122 +++++++++++++++++---------------- library/ssl_tls.c | 6 -- 3 files changed, 65 insertions(+), 65 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 74c9f1a94..e9f3409c6 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -383,7 +383,7 @@ struct mbedtls_ssl_handshake_params #endif /* MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED */ #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) - const mbedtls_ecp_curve_info **curves; /*!< Supported elliptic curves */ + mbedtls_ecp_curve_info const *curve_info; /*!< Info for EC for ECDHE. */ #endif #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED) unsigned char *psk; /*!< PSK from the callback */ diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 66f25ead9..ccc32d73a 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -279,12 +279,11 @@ static int ssl_parse_signature_algorithms_ext( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) static int ssl_parse_supported_elliptic_curves( mbedtls_ssl_context *ssl, - const unsigned char *buf, - size_t len ) + const unsigned char *buf, size_t len, + mbedtls_ecp_group_id curve_ids[ MBEDTLS_ECP_DP_MAX ] ) { size_t list_size, our_size; const unsigned char *p; - const mbedtls_ecp_curve_info *curve_info, **curves; if ( len < 2 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); @@ -302,44 +301,38 @@ static int ssl_parse_supported_elliptic_curves( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } - /* Should never happen unless client duplicates the extension */ - if( ssl->handshake->curves != NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); - } - - /* Don't allow our peer to make us allocate too much memory, - * and leave room for a final 0 */ - our_size = list_size / 2 + 1; - if( our_size > MBEDTLS_ECP_DP_MAX ) - our_size = MBEDTLS_ECP_DP_MAX; - - if( ( curves = mbedtls_calloc( our_size, sizeof( *curves ) ) ) == NULL ) - { - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - } - - ssl->handshake->curves = curves; - p = buf + 2; + our_size = MBEDTLS_ECP_DP_MAX; + + /* Leave room for final 0-entry */ while( list_size > 0 && our_size > 1 ) { - curve_info = mbedtls_ecp_curve_info_from_tls_id( ( p[0] << 8 ) | p[1] ); + uint16_t const tls_id = ( p[0] << 8 ) | p[1]; + mbedtls_ecp_curve_info const * const info = + mbedtls_ecp_curve_info_from_tls_id( tls_id ); - if( curve_info != NULL ) + if( info != NULL ) { - *curves++ = curve_info; + mbedtls_ecp_group_id const *gid; + /* Remember the first curve that we also support. */ + for( gid = ssl->conf->curve_list; + *gid != MBEDTLS_ECP_DP_NONE; gid++ ) + { + if( info->grp_id != *gid ) + continue; + + if( ssl->handshake->curve_info == NULL ) + ssl->handshake->curve_info = info; + } + + *curve_ids++ = info->grp_id; our_size--; } list_size -= 2; p += 2; } + *curve_ids = MBEDTLS_ECP_DP_NONE; return( 0 ); } @@ -743,16 +736,18 @@ static int ssl_parse_alpn_ext( mbedtls_ssl_context *ssl, */ #if defined(MBEDTLS_ECDSA_C) static int ssl_check_key_curve( mbedtls_pk_context *pk, - const mbedtls_ecp_curve_info **curves ) + mbedtls_ecp_group_id const *acceptable_ec_grp_ids ) { - const mbedtls_ecp_curve_info **crv = curves; mbedtls_ecp_group_id grp_id = mbedtls_pk_ec( *pk )->grp.id; - while( *crv != NULL ) + if( acceptable_ec_grp_ids == NULL ) + return( -1 ); + + while( *acceptable_ec_grp_ids != MBEDTLS_ECP_DP_NONE ) { - if( (*crv)->grp_id == grp_id ) + if( *acceptable_ec_grp_ids == grp_id ) return( 0 ); - crv++; + acceptable_ec_grp_ids++; } return( -1 ); @@ -764,7 +759,9 @@ static int ssl_check_key_curve( mbedtls_pk_context *pk, * return 0 on success and -1 on failure. */ static int ssl_pick_cert( mbedtls_ssl_context *ssl, - const mbedtls_ssl_ciphersuite_t * ciphersuite_info ) + const mbedtls_ssl_ciphersuite_t * ciphersuite_info, + mbedtls_ecp_group_id const *acceptable_ec_grp_ids ) + { mbedtls_ssl_key_cert *cur, *list, *fallback = NULL; mbedtls_pk_type_t pk_alg = @@ -829,11 +826,13 @@ static int ssl_pick_cert( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_ECDSA_C) if( pk_alg == MBEDTLS_PK_ECDSA && - ssl_check_key_curve( pk, ssl->handshake->curves ) != 0 ) + ssl_check_key_curve( pk, acceptable_ec_grp_ids ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "certificate mismatch: elliptic curve" ) ); match = 0; } +#else + ((void) acceptable_ec_grp_ids); #endif #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) @@ -921,7 +920,8 @@ static int ssl_pick_cert( mbedtls_ssl_context *ssl, * Sets ciphersuite_info only if the suite matches. */ static int ssl_ciphersuite_match( mbedtls_ssl_context *ssl, int suite_id, - const mbedtls_ssl_ciphersuite_t **ciphersuite_info ) + const mbedtls_ssl_ciphersuite_t **ciphersuite_info, + mbedtls_ecp_group_id const *acceptable_ec_grp_ids ) { const mbedtls_ssl_ciphersuite_t *suite_info; @@ -971,11 +971,9 @@ static int ssl_ciphersuite_match( mbedtls_ssl_context *ssl, int suite_id, } #endif - #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) if( mbedtls_ssl_ciphersuite_uses_ec( suite_info ) && - ( ssl->handshake->curves == NULL || - ssl->handshake->curves[0] == NULL ) ) + ssl->handshake->curve_info == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "ciphersuite mismatch: " "no common elliptic curve" ) ); @@ -1023,12 +1021,14 @@ static int ssl_ciphersuite_match( mbedtls_ssl_context *ssl, int suite_id, * - try the next ciphersuite if we don't * This must be done last since we modify the key_cert list. */ - if( ssl_pick_cert( ssl, suite_info ) != 0 ) + if( ssl_pick_cert( ssl, suite_info, acceptable_ec_grp_ids ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "ciphersuite mismatch: " "no suitable certificate" ) ); return( 0 ); } +#else + ((void) acceptable_ec_grp_ids); #endif *ciphersuite_info = suite_info; @@ -1252,8 +1252,11 @@ static int ssl_parse_client_hello_v2( mbedtls_ssl_context *ssl ) got_common_suite = 1; if( ( ret = ssl_ciphersuite_match( ssl, ciphersuites[i], - &ciphersuite_info ) ) != 0 ) + &ciphersuite_info, + NULL ) ) ) + { return( ret ); + } if( ciphersuite_info != NULL ) goto have_ciphersuite_v2; @@ -1323,6 +1326,13 @@ static int ssl_parse_client_hello( mbedtls_ssl_context *ssl ) const mbedtls_ssl_ciphersuite_t *ciphersuite_info; int major, minor; +#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ + defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) + mbedtls_ecp_group_id acceptable_ec_grp_ids[ MBEDTLS_ECP_DP_MAX ]; +#else + mbedtls_ecp_group_id * acceptable_ec_grp_ids = NULL; +#endif + /* If there is no signature-algorithm extension present, * we need to fall back to the default values for allowed * signature-hash pairs. */ @@ -1862,7 +1872,9 @@ read_record_header: case MBEDTLS_TLS_EXT_SUPPORTED_ELLIPTIC_CURVES: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported elliptic curves extension" ) ); - ret = ssl_parse_supported_elliptic_curves( ssl, ext + 4, ext_size ); + ret = ssl_parse_supported_elliptic_curves( ssl, ext + 4, + ext_size, + acceptable_ec_grp_ids ); if( ret != 0 ) return( ret ); break; @@ -2133,8 +2145,11 @@ read_record_header: got_common_suite = 1; if( ( ret = ssl_ciphersuite_match( ssl, ciphersuites[i], - &ciphersuite_info ) ) != 0 ) + &ciphersuite_info, + acceptable_ec_grp_ids ) ) != 0 ) + { return( ret ); + } if( ciphersuite_info != NULL ) goto have_ciphersuite; @@ -3241,28 +3256,19 @@ static int ssl_prepare_server_key_exchange( mbedtls_ssl_context *ssl, * ECPoint public; * } ServerECDHParams; */ - const mbedtls_ecp_curve_info **curve = NULL; - const mbedtls_ecp_group_id *gid; + const mbedtls_ecp_curve_info *curve = ssl->handshake->curve_info; int ret; size_t len = 0; - /* Match our preference list against the offered curves */ - for( gid = ssl->conf->curve_list; *gid != MBEDTLS_ECP_DP_NONE; gid++ ) - for( curve = ssl->handshake->curves; *curve != NULL; curve++ ) - if( (*curve)->grp_id == *gid ) - goto curve_matching_done; - -curve_matching_done: - if( curve == NULL || *curve == NULL ) + if( curve == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "no matching curve for ECDHE" ) ); return( MBEDTLS_ERR_SSL_NO_CIPHER_CHOSEN ); } - - MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDHE curve: %s", (*curve)->name ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDHE curve: %s", curve->name ) ); if( ( ret = mbedtls_ecdh_setup( &ssl->handshake->ecdh_ctx, - (*curve)->grp_id ) ) != 0 ) + curve->grp_id ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ecp_group_load", ret ); return( ret ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 91b944cc4..950d03d05 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -10546,12 +10546,6 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) #endif #endif -#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ - defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) - /* explicit void pointer cast for buggy MS compiler */ - mbedtls_free( (void *) handshake->curves ); -#endif - #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED) if( handshake->psk != NULL ) {