From 4dc50bc06e8e4d1110048193f18b062c239920a2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:06:48 +0100 Subject: [PATCH 1/6] Fix typo in documentation --- include/mbedtls/ecp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 24017780..065a4cc0 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 ); From 6d9b762ee04e946926df239128d870381707f2cb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:07:58 +0100 Subject: [PATCH 2/6] Add test case for ecdh_calc_secret Add a test case for doing an ECDH calculation by calling mbedtls_ecdh_get_params on both keys, then mbedtls_ecdh_calc_secret. --- tests/suites/test_suite_ecdh.data | 8 +++ tests/suites/test_suite_ecdh.function | 93 +++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/tests/suites/test_suite_ecdh.data b/tests/suites/test_suite_ecdh.data index fe24ed46..f90d88da 100644 --- a/tests/suites/test_suite_ecdh.data +++ b/tests/suites/test_suite_ecdh.data @@ -79,3 +79,11 @@ 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" diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 08a1686e..175735de 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,60 @@ 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 */ From 62a73511f19caecf56be0be664ea0b9701232e74 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:09:29 +0100 Subject: [PATCH 3/6] Add test case for ecdh_get_params with mismatching group Add a test case for doing an ECDH calculation by calling mbedtls_ecdh_get_params on both keys, with keys belonging to different groups. This should fail, but currently passes. --- tests/suites/test_suite_ecdh.data | 8 +++++ tests/suites/test_suite_ecdh.function | 47 +++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/tests/suites/test_suite_ecdh.data b/tests/suites/test_suite_ecdh.data index f90d88da..af25359d 100644 --- a/tests/suites/test_suite_ecdh.data +++ b/tests/suites/test_suite_ecdh.data @@ -87,3 +87,11 @@ ecdh_exchange_calc_secret:MBEDTLS_ECP_DP_SECP256R1:"c6ef9c5d78ae012a011164acb397 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 175735de..9a9cf5f7 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -557,3 +557,50 @@ exit: 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 */ From b47045a18e870d859cbc06b2a26634835d77a0d7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:10:59 +0100 Subject: [PATCH 4/6] Fix ecdh_get_params with mismatching group If mbedtls_ecdh_get_params is called with keys belonging to different groups, make it return an error the second time, rather than silently interpret the first key as being on the second curve. This makes the non-regression test added by the previous commit pass. --- library/ecdh.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/library/ecdh.c b/library/ecdh.c index da95c60d..204a2785 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -442,8 +442,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( ctx->grp.id == 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( ctx->grp.id != key->grp.id ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + } #if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) return( ecdh_get_params_internal( ctx, key, side ) ); From 661610c8e03c657b13ad5d86fa497e57aed5b2a5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:39:16 +0100 Subject: [PATCH 5/6] Add changelog entry for mbedtls_ecdh_get_params robustness --- ChangeLog | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ChangeLog b/ChangeLog index 9b1230e2..4d347b6b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,14 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.x.x branch released xxxx-xx-xx +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. From 05fcf4f3c5c8a0eb22e724bec144de5bc73bc0f7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 22 Feb 2019 12:31:25 +0100 Subject: [PATCH 6/6] Fix mbedtls_ecdh_get_params with new ECDH context The new check for matching groups in mbedtls_ecdh_get_params only worked with legacy ECDH contexts. Make it work with the new context format. --- library/ecdh.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/library/ecdh.c b/library/ecdh.c index 204a2785..c5726877 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,7 +452,7 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, ECDH_VALIDATE_RET( side == MBEDTLS_ECDH_OURS || side == MBEDTLS_ECDH_THEIRS ); - if( ctx->grp.id == MBEDTLS_ECP_DP_NONE ) + 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. */ @@ -454,7 +464,7 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, /* 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( ctx->grp.id != key->grp.id ) + if( mbedtls_ecdh_grp_id( ctx ) != key->grp.id ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); }