From 390bbd08f77bed6541983f0edbbfefd0b81b7f8e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:07:58 +0100 Subject: [PATCH 1/5] 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 | 103 ++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/tests/suites/test_suite_ecdh.data b/tests/suites/test_suite_ecdh.data index f7119de416..5e8280c425 100644 --- a/tests/suites/test_suite_ecdh.data +++ b/tests/suites/test_suite_ecdh.data @@ -37,3 +37,11 @@ ecdh_exchange:MBEDTLS_ECP_DP_SECP192R1 ECDH exchange #2 depends_on:MBEDTLS_ECP_DP_SECP521R1_ENABLED ecdh_exchange:MBEDTLS_ECP_DP_SECP521R1 + +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 4c6a97baf0..7af0de505f 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -1,5 +1,47 @@ /* BEGIN_HEADER */ #include "mbedtls/ecdh.h" + +static int load_public_key( int grp_id, const char *point_str, + mbedtls_ecp_keypair *ecp ) +{ + int ok = 0; + unsigned char point_buf[MBEDTLS_ECP_MAX_BYTES]; + size_t point_len = unhexify( point_buf, point_str ); + + TEST_ASSERT( mbedtls_ecp_group_load( &ecp->grp, grp_id ) == 0 ); + TEST_ASSERT( mbedtls_ecp_point_read_binary( &ecp->grp, + &ecp->Q, + point_buf, + 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, const char *private_key_str, + mbedtls_ecp_keypair *ecp, + rnd_pseudo_info *rnd_info ) +{ + int ok = 0; + unsigned char private_key_buf[MBEDTLS_ECP_MAX_BYTES]; + size_t private_key_len = unhexify( private_key_buf, private_key_str ); + + TEST_ASSERT( mbedtls_ecp_group_load( &ecp->grp, grp_id ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &ecp->d, + private_key_buf, + 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 @@ -158,3 +200,64 @@ exit: mbedtls_ecdh_free( &cli ); } /* END_CASE */ + +/* BEGIN_CASE */ +void ecdh_exchange_calc_secret( int grp_id, + char *our_private_key, + char *their_point, + int ours_first, + char *expected_str ) +{ + rnd_pseudo_info rnd_info; + unsigned char expected_buf[MBEDTLS_ECP_MAX_BYTES]; + size_t expected_len; + 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 ); + + expected_len = unhexify( expected_buf, expected_str ); + + 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_buf, 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 496c9e053db771d19e38068b5b2382524faf4485 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:09:29 +0100 Subject: [PATCH 2/5] 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 5e8280c425..4ed32219b7 100644 --- a/tests/suites/test_suite_ecdh.data +++ b/tests/suites/test_suite_ecdh.data @@ -45,3 +45,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 7af0de505f..2ce4912ebb 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -261,3 +261,50 @@ exit: mbedtls_ecp_keypair_free( &their_key ); } /* END_CASE */ + +/* BEGIN_CASE */ +void ecdh_exchange_get_params_fail( int our_grp_id, + char *our_private_key, + int their_grp_id, + char *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 f58078c7c5f322f28f09d314d04085f60119ca1c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:10:59 +0100 Subject: [PATCH 3/5] 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 | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/library/ecdh.c b/library/ecdh.c index 61380b6936..75630bd356 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -179,8 +179,20 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, const mbedtls_ecp_keypai { int ret; - if( ( ret = mbedtls_ecp_group_copy( &ctx->grp, &key->grp ) ) != 0 ) - return( ret ); + if( ctx->grp.id == MBEDTLS_ECP_DP_NONE ) + { + /* This is the first call to get_params(). Copy the group information + * into the context. */ + if( ( ret = mbedtls_ecp_group_copy( &ctx->grp, &key->grp ) ) != 0 ) + return( ret ); + } + else + { + /* This is not the first call to get_params(). Check that the group + * is the same as the first time. */ + if( ctx->grp.id != key->grp.id ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + } /* If it's not our key, just import the public part as Qp */ if( side == MBEDTLS_ECDH_THEIRS ) From 0efa8567d80543cce84d6a7f5a9d7075834d70bc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Nov 2018 22:39:16 +0100 Subject: [PATCH 4/5] Add changelog entry for mbedtls_ecdh_get_params robustness --- ChangeLog | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ChangeLog b/ChangeLog index 07cc876923..82f7387743 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,14 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.7.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 * Run the AD too long test only if MBEDTLS_CCM_ALT is not defined. Raised as a comment in #1996. From b46f1bd451e6ec2ffe438dfef1105a07f0211de7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 22 Feb 2019 11:30:14 +0100 Subject: [PATCH 5/5] Fix too small buffer in a test --- tests/suites/test_suite_ecdh.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 2ce4912ebb..0645ce798b 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -5,7 +5,7 @@ static int load_public_key( int grp_id, const char *point_str, mbedtls_ecp_keypair *ecp ) { int ok = 0; - unsigned char point_buf[MBEDTLS_ECP_MAX_BYTES]; + unsigned char point_buf[MBEDTLS_ECP_MAX_PT_LEN]; size_t point_len = unhexify( point_buf, point_str ); TEST_ASSERT( mbedtls_ecp_group_load( &ecp->grp, grp_id ) == 0 );