diff --git a/ChangeLog b/ChangeLog index 5d9927ce3..d4e945a86 100644 --- a/ChangeLog +++ b/ChangeLog @@ -50,6 +50,14 @@ API Changes mbedtls_ssl_session structure which otherwise stores the peer's certificate. +Security + * Make mbedtls_ecdh_get_params return an error if the second key + belongs to a different group from the first. Before, if an application + passed keys that belonged to different group, the first key's data was + interpreted according to the second group, which could lead to either + an error or a meaningless output from mbedtls_ecdh_get_params. In the + latter case, this could expose at most 5 bits of the private key. + Bugfix * Fix a compilation issue with mbedtls_ecp_restart_ctx not being defined when MBEDTLS_ECP_ALT is defined. Reported by jwhui. Fixes #2242. diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index c1450dbda..0fa74f061 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -125,6 +125,11 @@ #error "MBEDTLS_ECP_RESTARTABLE defined, but it cannot coexist with an alternative or PSA-based ECP implementation" #endif +#if defined(MBEDTLS_ECP_RESTARTABLE) && \ + ! defined(MBEDTLS_ECDH_LEGACY_CONTEXT) +#error "MBEDTLS_ECP_RESTARTABLE defined, but not MBEDTLS_ECDH_LEGACY_CONTEXT" +#endif + #if defined(MBEDTLS_ECDSA_DETERMINISTIC) && !defined(MBEDTLS_HMAC_DRBG_C) #error "MBEDTLS_ECDSA_DETERMINISTIC defined, but not all prerequisites" #endif diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 31a305923..0fe257460 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -760,10 +760,39 @@ * * \note This option only works with the default software implementation of * elliptic curve functionality. It is incompatible with - * MBEDTLS_ECP_ALT, MBEDTLS_ECDH_XXX_ALT and MBEDTLS_ECDSA_XXX_ALT. + * MBEDTLS_ECP_ALT, MBEDTLS_ECDH_XXX_ALT, MBEDTLS_ECDSA_XXX_ALT + * and MBEDTLS_ECDH_LEGACY_CONTEXT. */ //#define MBEDTLS_ECP_RESTARTABLE +/** + * \def MBEDTLS_ECDH_LEGACY_CONTEXT + * + * Use a backward compatible ECDH context. + * + * Mbed TLS supports two formats for ECDH contexts (#mbedtls_ecdh_context + * defined in `ecdh.h`). For most applications, the choice of format makes + * no difference, since all library functions can work with either format, + * except that the new format is incompatible with MBEDTLS_ECP_RESTARTABLE. + + * The new format used when this option is disabled is smaller + * (56 bytes on a 32-bit platform). In future versions of the library, it + * will support alternative implementations of ECDH operations. + * The new format is incompatible with applications that access + * context fields directly and with restartable ECP operations. + * + * Define this macro if you enable MBEDTLS_ECP_RESTARTABLE or if you + * want to access ECDH context fields directly. Otherwise you should + * comment out this macro definition. + * + * This option has no effect if #MBEDTLS_ECDH_C is not enabled. + * + * \note This configuration option is experimental. Future versions of the + * library may modify the way the ECDH context layout is configured + * and may modify the layout of the new context type. + */ +#define MBEDTLS_ECDH_LEGACY_CONTEXT + /** * \def MBEDTLS_ECDSA_DETERMINISTIC * diff --git a/include/mbedtls/ecdh.h b/include/mbedtls/ecdh.h index 4479a1d46..384c3dc07 100644 --- a/include/mbedtls/ecdh.h +++ b/include/mbedtls/ecdh.h @@ -42,18 +42,6 @@ #include "ecp.h" -/* - * Use a backward compatible ECDH context. - * - * This flag is always enabled for now and future versions might add a - * configuration option that conditionally undefines this flag. - * The configuration option in question may have a different name. - * - * Features undefining this flag, must have a warning in their description in - * config.h stating that the feature breaks backward compatibility. - */ -#define MBEDTLS_ECDH_LEGACY_CONTEXT - #ifdef __cplusplus extern "C" { #endif diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 24017780d..065a4cc0b 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -482,7 +482,7 @@ void mbedtls_ecp_point_init( mbedtls_ecp_point *pt ); * * \note After this function is called, domain parameters * for various ECP groups can be loaded through the - * mbedtls_ecp_load() or mbedtls_ecp_tls_read_group() + * mbedtls_ecp_group_load() or mbedtls_ecp_tls_read_group() * functions. */ void mbedtls_ecp_group_init( mbedtls_ecp_group *grp ); diff --git a/library/ecdh.c b/library/ecdh.c index da95c60da..c5726877d 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -49,6 +49,16 @@ typedef mbedtls_ecdh_context mbedtls_ecdh_context_mbed; #endif +static mbedtls_ecp_group_id mbedtls_ecdh_grp_id( + const mbedtls_ecdh_context *ctx ) +{ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ctx->grp.id ); +#else + return( ctx->grp_id ); +#endif +} + #if !defined(MBEDTLS_ECDH_GEN_PUBLIC_ALT) /* * Generate public key (restartable version) @@ -442,8 +452,21 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, ECDH_VALIDATE_RET( side == MBEDTLS_ECDH_OURS || side == MBEDTLS_ECDH_THEIRS ); - if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) - return( ret ); + if( mbedtls_ecdh_grp_id( ctx ) == MBEDTLS_ECP_DP_NONE ) + { + /* This is the first call to get_params(). Set up the context + * for use with the group. */ + if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) + return( ret ); + } + else + { + /* This is not the first call to get_params(). Check that the + * current key's group is the same as the context's, which was set + * from the first key's group. */ + if( mbedtls_ecdh_grp_id( ctx ) != key->grp.id ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + } #if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) return( ecdh_get_params_internal( ctx, key, side ) ); diff --git a/library/version_features.c b/library/version_features.c index 1be0e0fc9..59eacc47b 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -351,6 +351,9 @@ static const char *features[] = { #if defined(MBEDTLS_ECP_RESTARTABLE) "MBEDTLS_ECP_RESTARTABLE", #endif /* MBEDTLS_ECP_RESTARTABLE */ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + "MBEDTLS_ECDH_LEGACY_CONTEXT", +#endif /* MBEDTLS_ECDH_LEGACY_CONTEXT */ #if defined(MBEDTLS_ECDSA_DETERMINISTIC) "MBEDTLS_ECDSA_DETERMINISTIC", #endif /* MBEDTLS_ECDSA_DETERMINISTIC */ diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index f27267650..6ea901225 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -978,6 +978,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_ECP_RESTARTABLE */ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + if( strcmp( "MBEDTLS_ECDH_LEGACY_CONTEXT", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_ECDH_LEGACY_CONTEXT ); + return( 0 ); + } +#endif /* MBEDTLS_ECDH_LEGACY_CONTEXT */ + #if defined(MBEDTLS_ECDSA_DETERMINISTIC) if( strcmp( "MBEDTLS_ECDSA_DETERMINISTIC", config ) == 0 ) { diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 301dc52ee..2b443b725 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -678,6 +678,23 @@ component_test_rsa_no_crt () { if_build_succeeded tests/compat.sh -t RSA } +component_test_new_ecdh_context () { + msg "build: new ECDH context (ASan build)" # ~ 6 min + scripts/config.pl unset MBEDTLS_ECDH_LEGACY_CONTEXT + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: new ECDH context - main suites (inc. selftests) (ASan build)" # ~ 50s + make test + + msg "test: new ECDH context - ECDH-related part of ssl-opt.sh (ASan build)" # ~ 5s + if_build_succeeded tests/ssl-opt.sh -f ECDH + + msg "test: new ECDH context - compat.sh with some ECDH ciphersuites (ASan build)" # ~ 3 min + # Exclude some symmetric ciphers that are redundant here to gain time. + if_build_succeeded tests/compat.sh -f ECDH -V NO -e 'ARCFOUR\|ARIA\|CAMELLIA\|CHACHA\|DES\|RC4' +} + component_test_small_ssl_out_content_len () { msg "build: small SSL_OUT_CONTENT_LEN (ASan build)" scripts/config.pl set MBEDTLS_SSL_IN_CONTENT_LEN 16384 diff --git a/tests/suites/test_suite_ecdh.data b/tests/suites/test_suite_ecdh.data index fe24ed46a..af25359d3 100644 --- a/tests/suites/test_suite_ecdh.data +++ b/tests/suites/test_suite_ecdh.data @@ -79,3 +79,19 @@ ecdh_restart:MBEDTLS_ECP_DP_SECP256R1:"C88F01F510D9AC3F70A292DAA2316DE544E9AAB8A ECDH exchange legacy context depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED ecdh_exchange_legacy:MBEDTLS_ECP_DP_SECP192R1 + +ECDH calc_secret: ours first, SECP256R1 (RFC 5903) +depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED +ecdh_exchange_calc_secret:MBEDTLS_ECP_DP_SECP256R1:"c6ef9c5d78ae012a011164acb397ce2088685d8f06bf9be0b283ab46476bee53":"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":0:"d6840f6b42f6edafd13116e0e12565202fef8e9ece7dce03812464d04b9442de" + +ECDH calc_secret: theirs first, SECP256R1 (RFC 5903) +depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED +ecdh_exchange_calc_secret:MBEDTLS_ECP_DP_SECP256R1:"c6ef9c5d78ae012a011164acb397ce2088685d8f06bf9be0b283ab46476bee53":"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":1:"d6840f6b42f6edafd13116e0e12565202fef8e9ece7dce03812464d04b9442de" + +ECDH get_params with mismatched groups: our BP256R1, their SECP256R1 +depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_BP256R1_ENABLED +ecdh_exchange_get_params_fail:MBEDTLS_ECP_DP_BP256R1:"1234567812345678123456781234567812345678123456781234567812345678":MBEDTLS_ECP_DP_SECP256R1:"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":0:MBEDTLS_ERR_ECP_BAD_INPUT_DATA + +ECDH get_params with mismatched groups: their SECP256R1, our BP256R1 +depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_BP256R1_ENABLED +ecdh_exchange_get_params_fail:MBEDTLS_ECP_DP_BP256R1:"1234567812345678123456781234567812345678123456781234567812345678":MBEDTLS_ECP_DP_SECP256R1:"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":1:MBEDTLS_ERR_ECP_BAD_INPUT_DATA diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 7db0ed16e..1a33d8175 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -1,5 +1,41 @@ /* BEGIN_HEADER */ #include "mbedtls/ecdh.h" + +static int load_public_key( int grp_id, data_t *point, + mbedtls_ecp_keypair *ecp ) +{ + int ok = 0; + TEST_ASSERT( mbedtls_ecp_group_load( &ecp->grp, grp_id ) == 0 ); + TEST_ASSERT( mbedtls_ecp_point_read_binary( &ecp->grp, + &ecp->Q, + point->x, + point->len ) == 0 ); + TEST_ASSERT( mbedtls_ecp_check_pubkey( &ecp->grp, + &ecp->Q ) == 0 ); + ok = 1; +exit: + return( ok ); +} + +static int load_private_key( int grp_id, data_t *private_key, + mbedtls_ecp_keypair *ecp, + rnd_pseudo_info *rnd_info ) +{ + int ok = 0; + TEST_ASSERT( mbedtls_ecp_group_load( &ecp->grp, grp_id ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &ecp->d, + private_key->x, + private_key->len ) == 0 ); + TEST_ASSERT( mbedtls_ecp_check_privkey( &ecp->grp, &ecp->d ) == 0 ); + /* Calculate the public key from the private key. */ + TEST_ASSERT( mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, + &ecp->grp.G, + &rnd_pseudo_rand, rnd_info ) == 0 ); + ok = 1; +exit: + return( ok ); +} + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -464,3 +500,107 @@ exit: mbedtls_ecdh_free( &cli ); } /* END_CASE */ + +/* BEGIN_CASE */ +void ecdh_exchange_calc_secret( int grp_id, + data_t *our_private_key, + data_t *their_point, + int ours_first, + data_t *expected ) +{ + rnd_pseudo_info rnd_info; + mbedtls_ecp_keypair our_key; + mbedtls_ecp_keypair their_key; + mbedtls_ecdh_context ecdh; + unsigned char shared_secret[MBEDTLS_ECP_MAX_BYTES]; + size_t shared_secret_length = 0; + + memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) ); + mbedtls_ecdh_init( &ecdh ); + mbedtls_ecp_keypair_init( &our_key ); + mbedtls_ecp_keypair_init( &their_key ); + + if( ! load_private_key( grp_id, our_private_key, &our_key, &rnd_info ) ) + goto exit; + if( ! load_public_key( grp_id, their_point, &their_key ) ) + goto exit; + + /* Import the keys to the ECDH calculation. */ + if( ours_first ) + { + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &our_key, MBEDTLS_ECDH_OURS ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) == 0 ); + } + else + { + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &our_key, MBEDTLS_ECDH_OURS ) == 0 ); + } + + /* Perform the ECDH calculation. */ + TEST_ASSERT( mbedtls_ecdh_calc_secret( + &ecdh, + &shared_secret_length, + shared_secret, sizeof( shared_secret ), + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( shared_secret_length == expected->len ); + TEST_ASSERT( memcmp( expected->x, shared_secret, + shared_secret_length ) == 0 ); + +exit: + mbedtls_ecdh_free( &ecdh ); + mbedtls_ecp_keypair_free( &our_key ); + mbedtls_ecp_keypair_free( &their_key ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void ecdh_exchange_get_params_fail( int our_grp_id, + data_t *our_private_key, + int their_grp_id, + data_t *their_point, + int ours_first, + int expected_ret ) +{ + rnd_pseudo_info rnd_info; + mbedtls_ecp_keypair our_key; + mbedtls_ecp_keypair their_key; + mbedtls_ecdh_context ecdh; + + memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) ); + mbedtls_ecdh_init( &ecdh ); + mbedtls_ecp_keypair_init( &our_key ); + mbedtls_ecp_keypair_init( &their_key ); + + if( ! load_private_key( our_grp_id, our_private_key, &our_key, &rnd_info ) ) + goto exit; + if( ! load_public_key( their_grp_id, their_point, &their_key ) ) + goto exit; + + if( ours_first ) + { + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &our_key, MBEDTLS_ECDH_OURS ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) == + expected_ret ); + } + else + { + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_get_params( + &ecdh, &our_key, MBEDTLS_ECDH_OURS ) == + expected_ret ); + } + +exit: + mbedtls_ecdh_free( &ecdh ); + mbedtls_ecp_keypair_free( &our_key ); + mbedtls_ecp_keypair_free( &their_key ); +} +/* END_CASE */