From 89ac8c9266184bb8edc2c04111f7b0df220bb440 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 30 Oct 2018 11:24:05 +0000 Subject: [PATCH 01/11] ECP: Add mbedtls_ecp_tls_read_group_id() `mbedtls_ecp_tls_read_group()` both parses the group ID and loads the group into the structure provided. We want to support alternative implementations of ECDH in the future and for that we need to parse the group ID without populating an `mbedtls_ecp_group` structure (because alternative implementations might not use that). This commit moves the part that parses the group ID to a new function. There is no need to test the new function directly, because the tests for `mbedtls_ecp_tls_read_group()` are already implicitly testing it. There is no intended change in behaviour in this commit. --- include/mbedtls/ecp.h | 31 ++++++++++++++++++++++++++----- library/ecp.c | 23 +++++++++++++++++++++-- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 2fb1af49a..0e5b47fc2 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -632,7 +632,7 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, mbedtls_ecp_poi /** * \brief This function imports a point from a TLS ECPoint record. * - * \note On function return, \p buf is updated to point to immediately + * \note On function return, \p *buf is updated to point immediately * after the ECPoint record. * * \param grp The ECP group used. @@ -641,7 +641,8 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, mbedtls_ecp_poi * \param len The length of the buffer. * * \return \c 0 on success. - * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization failure. + * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization + * failure. * \return #MBEDTLS_ERR_ECP_BAD_INPUT_DATA if input is invalid. */ int mbedtls_ecp_tls_read_point( const mbedtls_ecp_group *grp, mbedtls_ecp_point *pt, @@ -687,19 +688,39 @@ int mbedtls_ecp_group_load( mbedtls_ecp_group *grp, mbedtls_ecp_group_id id ); /** * \brief This function sets a group from a TLS ECParameters record. * - * \note \p buf is updated to point right after the ECParameters record - * on exit. + * \note \p buf is updated to point right after the ECParameters + * record on exit. * * \param grp The destination group. * \param buf The address of the pointer to the start of the input buffer. * \param len The length of the buffer. * * \return \c 0 on success. - * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization failure. + * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization + * failure. * \return #MBEDTLS_ERR_ECP_BAD_INPUT_DATA if input is invalid. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if the group is not + * recognised. */ int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, const unsigned char **buf, size_t len ); +/** + * \brief This function reads a group from a TLS ECParameters record. + * + * \note \p buf is updated to point right after the ECParameters + * record on exit. + * + * \param grp Output parameter to hold the group id. + * \param buf The address of the pointer to the start of the input buffer. + * \param len The length of the buffer. + * + * \return \c 0 on success. + * \return #MBEDTLS_ERR_ECP_BAD_INPUT_DATA if input is invalid. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if the group is not + * recognised. + */ +int mbedtls_ecp_tls_read_group_id( mbedtls_ecp_group_id *grp, + const unsigned char **buf, size_t len ); /** * \brief This function writes the TLS ECParameters record for a group. * diff --git a/library/ecp.c b/library/ecp.c index de5725c70..49255b103 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -833,7 +833,24 @@ int mbedtls_ecp_tls_write_point( const mbedtls_ecp_group *grp, const mbedtls_ecp /* * Set a group from an ECParameters record (RFC 4492) */ -int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, const unsigned char **buf, size_t len ) +int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, + const unsigned char **buf, size_t len ) +{ + int ret; + mbedtls_ecp_group_id grp_id; + + if( ( ret = mbedtls_ecp_tls_read_group_id( &grp_id, buf, len ) ) != 0 ) + return( ret ); + + return mbedtls_ecp_group_load( grp, grp_id ); +} + +/* + * Read a group id from an ECParameters record (RFC 4492) and convert it to + * mbedtls_ecp_group_id. + */ +int mbedtls_ecp_tls_read_group_id( mbedtls_ecp_group_id *grp, + const unsigned char **buf, size_t len ) { uint16_t tls_id; const mbedtls_ecp_curve_info *curve_info; @@ -860,7 +877,9 @@ int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, const unsigned char **bu if( ( curve_info = mbedtls_ecp_curve_info_from_tls_id( tls_id ) ) == NULL ) return( MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE ); - return mbedtls_ecp_group_load( grp, curve_info->grp_id ); + *grp = curve_info->grp_id; + + return( 0 ); } /* From f61e486179843ea7b6346066629b1bfa9f37b5fc Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 30 Oct 2018 11:53:25 +0000 Subject: [PATCH 02/11] ECDH: Add mbedtls_ecdh_setup() In the future we want to support alternative ECDH implementations. We can't make assumptions about the structure of the context they might use, and therefore shouldn't access the members of `mbedtls_ecdh_context`. Currently the lifecycle of the context can't be done without direct manipulation. This commit adds `mbedtls_ecdh_setup()` to complete covering the context lifecycle with functions. --- include/mbedtls/ecdh.h | 27 +++++++++++++++++++++++---- library/ecdh.c | 30 ++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ecdh.h b/include/mbedtls/ecdh.h index 27f2ffc6a..68a6989c8 100644 --- a/include/mbedtls/ecdh.h +++ b/include/mbedtls/ecdh.h @@ -134,6 +134,24 @@ int mbedtls_ecdh_compute_shared( mbedtls_ecp_group *grp, mbedtls_mpi *z, */ void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ); +/** + * \brief This function sets up the ECDH context with the information + * given. + * + * This function should be called after mbedtls_ecdh_init() but + * before mbedtls_ecdh_make_params(). There is no need to call + * this function before mbedtls_ecdh_read_params(). + * + * This is the first function used by a TLS server for ECDHE + * ciphersuites. + * + * \param ctx The ECDH context to set up. + * \param grp_id The group id of the group to set up the context for. + * + * \return \c 0 on success. + */ +int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ); + /** * \brief This function frees a context. * @@ -145,8 +163,8 @@ void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ); * \brief This function generates a public key and a TLS * ServerKeyExchange payload. * - * This is the first function used by a TLS server for ECDHE - * ciphersuites. + * This is the second function used by a TLS server for ECDHE + * ciphersuites. (It is called after mbedtls_ecdh_setup().) * * \note This function assumes that the ECP group (grp) of the * \p ctx context has already been properly set, @@ -242,8 +260,9 @@ int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, * \brief This function parses and processes a TLS ClientKeyExchange * payload. * - * This is the second function used by a TLS server for ECDH(E) - * ciphersuites. + * This is the third function used by a TLS server for ECDH(E) + * ciphersuites. (It is called after mbedtls_ecdh_setup() and + * mbedtls_ecdh_make_params().) * * \see ecp.h * diff --git a/library/ecdh.c b/library/ecdh.c index e6ae99994..702ba1a40 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -145,6 +145,23 @@ void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ) #endif } +/* + * Setup context + */ +int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) +{ + int ret; + + ret = mbedtls_ecp_group_load( &ctx->grp, grp_id ); + if( ret != 0 ) + { + mbedtls_ecdh_free( ctx ); + return( MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE ); + } + + return( 0 ); +} + /* * Free context */ @@ -240,12 +257,17 @@ int mbedtls_ecdh_read_params( mbedtls_ecdh_context *ctx, const unsigned char **buf, const unsigned char *end ) { int ret; + mbedtls_ecp_group_id grp_id; - if( ( ret = mbedtls_ecp_tls_read_group( &ctx->grp, buf, end - *buf ) ) != 0 ) + if( ( ret = mbedtls_ecp_tls_read_group_id( &grp_id, buf, end - *buf ) ) + != 0 ) return( ret ); - if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, end - *buf ) ) - != 0 ) + if( ( ret = mbedtls_ecdh_setup( ctx, grp_id ) ) != 0 ) + return( ret ); + + if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, + end - *buf ) ) != 0 ) return( ret ); return( 0 ); @@ -259,7 +281,7 @@ 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 ) + if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) return( ret ); /* If it's not our key, just import the public part as Qp */ From fc03e8dfa9912ff08a5431cf11bcad6da4da754a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 4 Oct 2018 17:17:54 +0100 Subject: [PATCH 03/11] ECDH: Adapt tests for mbedtls_ecdh_setup() The recently added `mbedtls_ecdh_setup()` function is not used in the tests yet. This commit adapts the tests to the new workflow. Having done that, the old lifecycle is not tested anymore, so we add a new test to ensure backward compatibility. --- tests/suites/test_suite_ecdh.data | 4 +++ tests/suites/test_suite_ecdh.function | 44 +++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_ecdh.data b/tests/suites/test_suite_ecdh.data index 0165a7e0a..89e5e3a80 100644 --- a/tests/suites/test_suite_ecdh.data +++ b/tests/suites/test_suite_ecdh.data @@ -69,3 +69,7 @@ ecdh_restart:MBEDTLS_ECP_DP_SECP256R1:"C88F01F510D9AC3F70A292DAA2316DE544E9AAB8A ECDH restartable rfc 5903 p256 restart disabled max_ops=250 depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED ecdh_restart:MBEDTLS_ECP_DP_SECP256R1:"C88F01F510D9AC3F70A292DAA2316DE544E9AAB8AFE84049C62A9C57862D1433":"C6EF9C5D78AE012A011164ACB397CE2088685D8F06BF9BE0B283AB46476BEE53":"D6840F6B42F6EDAFD13116E0E12565202FEF8E9ECE7DCE03812464D04B9442DE":0:250:0:0 + +ECDH exchange legacy context +depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED +ecdh_exchange_legacy:MBEDTLS_ECP_DP_SECP192R1 diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 965230885..52fecf350 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -134,16 +134,16 @@ void ecdh_exchange( int id ) mbedtls_ecdh_init( &cli ); memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) ); - TEST_ASSERT( mbedtls_ecp_group_load( &srv.grp, id ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_setup( &srv, id ) == 0 ); memset( buf, 0x00, sizeof( buf ) ); vbuf = buf; TEST_ASSERT( mbedtls_ecdh_make_params( &srv, &len, buf, 1000, - &rnd_pseudo_rand, &rnd_info ) == 0 ); + &rnd_pseudo_rand, &rnd_info ) == 0 ); TEST_ASSERT( mbedtls_ecdh_read_params( &cli, &vbuf, buf + len ) == 0 ); memset( buf, 0x00, sizeof( buf ) ); TEST_ASSERT( mbedtls_ecdh_make_public( &cli, &len, buf, 1000, - &rnd_pseudo_rand, &rnd_info ) == 0 ); + &rnd_pseudo_rand, &rnd_info ) == 0 ); TEST_ASSERT( mbedtls_ecdh_read_public( &srv, buf, len ) == 0 ); TEST_ASSERT( mbedtls_ecdh_calc_secret( &srv, &len, buf, 1000, @@ -273,3 +273,41 @@ exit: mbedtls_ecdh_free( &cli ); } /* END_CASE */ + +/* BEGIN_CASE */ +void ecdh_exchange_legacy( int id ) +{ + mbedtls_ecdh_context srv, cli; + unsigned char buf[1000]; + const unsigned char *vbuf; + size_t len; + + rnd_pseudo_info rnd_info; + + mbedtls_ecdh_init( &srv ); + mbedtls_ecdh_init( &cli ); + memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) ); + + TEST_ASSERT( mbedtls_ecp_group_load( &srv.grp, id ) == 0 ); + + memset( buf, 0x00, sizeof( buf ) ); vbuf = buf; + TEST_ASSERT( mbedtls_ecdh_make_params( &srv, &len, buf, 1000, + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_read_params( &cli, &vbuf, buf + len ) == 0 ); + + memset( buf, 0x00, sizeof( buf ) ); + TEST_ASSERT( mbedtls_ecdh_make_public( &cli, &len, buf, 1000, + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_read_public( &srv, buf, len ) == 0 ); + + TEST_ASSERT( mbedtls_ecdh_calc_secret( &srv, &len, buf, 1000, + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_calc_secret( &cli, &len, buf, 1000, NULL, + NULL ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &srv.z, &cli.z ) == 0 ); + +exit: + mbedtls_ecdh_free( &srv ); + mbedtls_ecdh_free( &cli ); +} +/* END_CASE */ From c9c32f3f639bb64153c3130110f38797392cb615 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 13 Aug 2018 15:52:45 +0100 Subject: [PATCH 04/11] ECDH: Add flexible context and legacy flag We want to support alternative software implementations and we extend the ECDH context to enable this. The actual functional change that makes use of the new context is out of scope for this commit. Changing the context breaks the API and therefore it has to be excluded from the default configuration by a compile time flag. We add the compile time flag to the module header instead of `config.h`, because this is not a standalone feature, it only enables adding new implementations in the future. The new context features a union of the individual implementations and a selector that chooses the implementation in use. An alternative is to use an opaque context and function pointers, like for example the PK module does it, but it is more dangerous, error prone and tedious to implement. We leave the group ID and the point format at the top level of the structure, because they are very simple and adding an abstraction layer around them away does not come with any obvious benefit. Other alternatives considered: - Using the module level replacement mechanism in the ECP module. This would have made the use of the replacement feature more difficult and the benefit limited. - Replacing our Montgomery implementations with a new one directly. This would have prevented using Montgomery curves across implementations. (For example use implementation A for Curve448 and implementation B for Curve22519.) Also it would have been inflexible and limited to Montgomery curves. - Encoding the implementation selector and the alternative context in `mbedtls_ecp_point` somehow and rewriting `mbedtls_ecp_mul()` to dispatch between implementations. This would have been a dangerous and ugly hack, and very likely to break legacy applications. - Same as above just with hardcoding the selector and using a compile time option to make the selection. Rejected for the same reasons as above. - Using the PK module to provide to provide an entry point for alternative implementations. Like most of the above options this wouldn't have come with a new compile time option, but conceptually would have been very out of place and would have meant much more work to complete the abstraction around the context. In retrospect: - We could have used the group ID as the selector, but this would have made the code less flexible and only marginally simpler. On the other hand it would have allowed to get rid of the compile time option if a tight integration of the alternative is possible. (It does not seem possible at this point.) - We could have used the same approach we do in this commit to the `mbedtls_ecp_point` structure. Completing the abstraction around this structure would have been a much bigger and much riskier code change with increase in memory footprint, potential decrease in performance and no immediate benefit. --- include/mbedtls/ecdh.h | 67 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ecdh.h b/include/mbedtls/ecdh.h index 68a6989c8..cbd48414a 100644 --- a/include/mbedtls/ecdh.h +++ b/include/mbedtls/ecdh.h @@ -36,6 +36,18 @@ #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 @@ -49,6 +61,39 @@ typedef enum MBEDTLS_ECDH_THEIRS, /**< The key of the peer. */ } mbedtls_ecdh_side; +#if !defined(MBEDTLS_ECDH_LEGACY_CONTEXT) +/** + * Defines the ECDH implementation used. + * + * Later versions of the library may add new variants, therefore users should + * not make any assumptions about them. + */ +typedef enum +{ + MBEDTLS_ECDH_VARIANT_NONE = 0, /*!< Implementation not defined. */ + MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0,/*!< The default Mbed TLS implementation */ +} mbedtls_ecdh_variant; + +/** + * The context used by the default ECDH implementation. + * + * Later versions might change the structure of this context, therefore users + * should not make any assumptions about the structure of + * mbedtls_ecdh_context_mbed. + */ +typedef struct mbedtls_ecdh_context_mbed +{ + mbedtls_ecp_group grp; /*!< The elliptic curve used. */ + mbedtls_mpi d; /*!< The private key. */ + mbedtls_ecp_point Q; /*!< The public key. */ + mbedtls_ecp_point Qp; /*!< The value of the public key of the peer. */ + mbedtls_mpi z; /*!< The shared secret. */ +#if defined(MBEDTLS_ECP_RESTARTABLE) + mbedtls_ecp_restart_ctx rs; /*!< The restart context for EC computations. */ +#endif +} mbedtls_ecdh_context_mbed; +#endif + /** * * \warning Performing multiple operations concurrently on the same @@ -58,6 +103,7 @@ typedef enum */ typedef struct mbedtls_ecdh_context { +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) mbedtls_ecp_group grp; /*!< The elliptic curve used. */ mbedtls_mpi d; /*!< The private key. */ mbedtls_ecp_point Q; /*!< The public key. */ @@ -70,7 +116,26 @@ typedef struct mbedtls_ecdh_context #if defined(MBEDTLS_ECP_RESTARTABLE) int restart_enabled; /*!< The flag for restartable mode. */ mbedtls_ecp_restart_ctx rs; /*!< The restart context for EC computations. */ -#endif +#endif /* MBEDTLS_ECP_RESTARTABLE */ +#else + uint8_t point_format; /*!< The format of point export in TLS messages + as defined in RFC 4492. */ + mbedtls_ecp_group_id grp_id;/*!< The elliptic curve used. */ + mbedtls_ecdh_variant var; /*!< The ECDH implementation/structure used. */ + union + { + mbedtls_ecdh_context_mbed mbed_ecdh; + } ctx; /*!< Implementation-specific context. The + context in use is specified by the \c var + field. */ +#if defined(MBEDTLS_ECP_RESTARTABLE) + uint8_t restart_enabled; /*!< The flag for restartable mode. Functions of + an alternative implementation not supporting + restartable mode must return + MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED error + if this flag is set. */ +#endif /* MBEDTLS_ECP_RESTARTABLE */ +#endif /* MBEDTLS_ECDH_LEGACY_CONTEXT */ } mbedtls_ecdh_context; From 52735ef2fec4502870877f5dfcd06cd7a36774aa Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 15 Aug 2018 10:19:16 +0100 Subject: [PATCH 05/11] ECDH: Prevent direct access in non-legacy mode Some sample programs access structure fields directly. Making these work is desirable in the long term, but these are not essential for the core functionality in non-legacy mode. --- programs/pkey/ecdh_curve25519.c | 4 ++-- programs/test/benchmark.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/programs/pkey/ecdh_curve25519.c b/programs/pkey/ecdh_curve25519.c index 5db04088f..7fbf1678f 100644 --- a/programs/pkey/ecdh_curve25519.c +++ b/programs/pkey/ecdh_curve25519.c @@ -35,12 +35,12 @@ #define MBEDTLS_EXIT_FAILURE EXIT_FAILURE #endif /* MBEDTLS_PLATFORM_C */ -#if !defined(MBEDTLS_ECDH_C) || \ +#if !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_ECDH_LEGACY_CONTEXT) || \ !defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ !defined(MBEDTLS_ENTROPY_C) || !defined(MBEDTLS_CTR_DRBG_C) int main( void ) { - mbedtls_printf( "MBEDTLS_ECDH_C and/or " + mbedtls_printf( "MBEDTLS_ECDH_C and/or MBEDTLS_ECDH_LEGACY_CONTEXT and/or " "MBEDTLS_ECP_DP_CURVE25519_ENABLED and/or " "MBEDTLS_ENTROPY_C and/or MBEDTLS_CTR_DRBG_C " "not defined\n" ); diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index e7d29c396..dd4303b89 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -862,7 +862,7 @@ int main( int argc, char *argv[] ) } #endif -#if defined(MBEDTLS_ECDH_C) +#if defined(MBEDTLS_ECDH_C) && defined(MBEDTLS_ECDH_LEGACY_CONTEXT) if( todo.ecdh ) { mbedtls_ecdh_context ecdh; From 5a3e1bfda0ef95f18b01a14eac21a68255c21451 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 13 Aug 2018 15:54:22 +0100 Subject: [PATCH 06/11] ECDH: Make the implementation use the new context The functionality from public API functions are moved to `xxx_internal()` functions. The public API functions are modified to do basic parameter validation and dispatch the call to the right implementation. There is no intended change in behaviour when `MBEDTLS_ECDH_LEGACY_CONTEXT` is enabled. --- library/ecdh.c | 374 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 304 insertions(+), 70 deletions(-) diff --git a/library/ecdh.c b/library/ecdh.c index 702ba1a40..d68db8ac7 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -38,6 +38,10 @@ #include +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) +typedef mbedtls_ecdh_context mbedtls_ecdh_context_mbed; +#endif + #if !defined(MBEDTLS_ECDH_GEN_PUBLIC_ALT) /* * Generate public key (restartable version) @@ -124,38 +128,48 @@ int mbedtls_ecdh_compute_shared( mbedtls_ecp_group *grp, mbedtls_mpi *z, } #endif /* !MBEDTLS_ECDH_COMPUTE_SHARED_ALT */ -/* - * Initialize context - */ -void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ) +static void ecdh_init_internal( mbedtls_ecdh_context_mbed *ctx ) { mbedtls_ecp_group_init( &ctx->grp ); mbedtls_mpi_init( &ctx->d ); mbedtls_ecp_point_init( &ctx->Q ); mbedtls_ecp_point_init( &ctx->Qp ); mbedtls_mpi_init( &ctx->z ); - ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; - mbedtls_ecp_point_init( &ctx->Vi ); - mbedtls_ecp_point_init( &ctx->Vf ); - mbedtls_mpi_init( &ctx->_d ); #if defined(MBEDTLS_ECP_RESTARTABLE) - ctx->restart_enabled = 0; mbedtls_ecp_restart_init( &ctx->rs ); #endif } /* - * Setup context + * Initialize context */ -int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) +void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ) +{ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + ecdh_init_internal( ctx ); + mbedtls_ecp_point_init( &ctx->Vi ); + mbedtls_ecp_point_init( &ctx->Vf ); + mbedtls_mpi_init( &ctx->_d ); +#else + memset( ctx, 0, sizeof( mbedtls_ecdh_context ) ); + + ctx->var = MBEDTLS_ECDH_VARIANT_NONE; +#endif + ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; +#if defined(MBEDTLS_ECP_RESTARTABLE) + ctx->restart_enabled = 0; +#endif +} + +static int ecdh_setup_internal( mbedtls_ecdh_context_mbed *ctx, + mbedtls_ecp_group_id grp_id ) { int ret; ret = mbedtls_ecp_group_load( &ctx->grp, grp_id ); if( ret != 0 ) { - mbedtls_ecdh_free( ctx ); return( MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE ); } @@ -163,21 +177,35 @@ int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) } /* - * Free context + * Setup context */ -void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ) +int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) { if( ctx == NULL ) - return; + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_setup_internal( ctx, grp_id ) ); +#else + switch( grp_id ) + { + default: + ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; + ctx->var = MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0; + ctx->grp_id = grp_id; + ecdh_init_internal( &ctx->ctx.mbed_ecdh ); + return( ecdh_setup_internal( &ctx->ctx.mbed_ecdh, grp_id ) ); + } +#endif +} + +static void ecdh_free_internal( mbedtls_ecdh_context_mbed *ctx ) +{ mbedtls_ecp_group_free( &ctx->grp ); mbedtls_mpi_free( &ctx->d ); mbedtls_ecp_point_free( &ctx->Q ); mbedtls_ecp_point_free( &ctx->Qp ); mbedtls_mpi_free( &ctx->z ); - mbedtls_ecp_point_free( &ctx->Vi ); - mbedtls_ecp_point_free( &ctx->Vf ); - mbedtls_mpi_free( &ctx->_d ); #if defined(MBEDTLS_ECP_RESTARTABLE) mbedtls_ecp_restart_free( &ctx->rs ); @@ -190,21 +218,50 @@ void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ) */ void mbedtls_ecdh_enable_restart( mbedtls_ecdh_context *ctx ) { + if( ctx == NULL ) + return; + ctx->restart_enabled = 1; } #endif /* - * Setup and write the ServerKeyExhange parameters (RFC 4492) - * struct { - * ECParameters curve_params; - * ECPoint public; - * } ServerECDHParams; + * Free context */ -int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, - unsigned char *buf, size_t blen, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ) +{ + if( ctx == NULL ) + return; + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + mbedtls_ecp_point_free( &ctx->Vi ); + mbedtls_ecp_point_free( &ctx->Vf ); + mbedtls_mpi_free( &ctx->_d ); + ecdh_free_internal( ctx ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + ecdh_free_internal( &ctx->ctx.mbed_ecdh ); + break; + default: + break; + } + + ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; + ctx->var = MBEDTLS_ECDH_VARIANT_NONE; + ctx->grp_id = MBEDTLS_ECP_DP_NONE; +#endif +} + +static int ecdh_make_params_internal( mbedtls_ecdh_context_mbed *ctx, + size_t *olen, int point_format, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, + unsigned char *, + size_t), + void *p_rng, + int restart_enabled ) { int ret; size_t grp_len, pt_len; @@ -212,12 +269,14 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, mbedtls_ecp_restart_ctx *rs_ctx = NULL; #endif - if( ctx == NULL || ctx->grp.pbits == 0 ) + if( ctx->grp.pbits == 0 ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ctx->restart_enabled ) + if( restart_enabled ) rs_ctx = &ctx->rs; +#else + (void) restart_enabled; #endif @@ -231,14 +290,14 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, return( ret ); #endif /* MBEDTLS_ECP_RESTARTABLE */ - if( ( ret = mbedtls_ecp_tls_write_group( &ctx->grp, &grp_len, buf, blen ) ) - != 0 ) + if( ( ret = mbedtls_ecp_tls_write_group( &ctx->grp, &grp_len, buf, + blen ) ) != 0 ) return( ret ); buf += grp_len; blen -= grp_len; - if( ( ret = mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, ctx->point_format, + if( ( ret = mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, point_format, &pt_len, buf, blen ) ) != 0 ) return( ret ); @@ -246,6 +305,54 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, return( 0 ); } +/* + * Setup and write the ServerKeyExhange parameters (RFC 4492) + * struct { + * ECParameters curve_params; + * ECPoint public; + * } ServerECDHParams; + */ +int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + int restart_enabled = 0; + + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + +#if defined(MBEDTLS_ECP_RESTARTABLE) + restart_enabled = ctx->restart_enabled; +#else + (void) restart_enabled; +#endif + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_make_params_internal( ctx, olen, ctx->point_format, buf, blen, + f_rng, p_rng, restart_enabled ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_make_params_internal( &ctx->ctx.mbed_ecdh, olen, + ctx->point_format, buf, blen, + f_rng, p_rng, + restart_enabled ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_read_params_internal( mbedtls_ecdh_context_mbed *ctx, + const unsigned char **buf, + const unsigned char *end ) +{ + return( mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, + end - *buf ) ); +} + /* * Read the ServerKeyExhange parameters (RFC 4492) * struct { @@ -254,11 +361,15 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, * } ServerECDHParams; */ int mbedtls_ecdh_read_params( mbedtls_ecdh_context *ctx, - const unsigned char **buf, const unsigned char *end ) + const unsigned char **buf, + const unsigned char *end ) { int ret; mbedtls_ecp_group_id grp_id; + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + if( ( ret = mbedtls_ecp_tls_read_group_id( &grp_id, buf, end - *buf ) ) != 0 ) return( ret ); @@ -266,24 +377,26 @@ int mbedtls_ecdh_read_params( mbedtls_ecdh_context *ctx, if( ( ret = mbedtls_ecdh_setup( ctx, grp_id ) ) != 0 ) return( ret ); - if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, - end - *buf ) ) != 0 ) - return( ret ); - - return( 0 ); +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_read_params_internal( ctx, buf, end ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_read_params_internal( &ctx->ctx.mbed_ecdh, + buf, end ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif } -/* - * Get parameters from a keypair - */ -int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, const mbedtls_ecp_keypair *key, - mbedtls_ecdh_side side ) +static int ecdh_get_params_internal( mbedtls_ecdh_context_mbed *ctx, + const mbedtls_ecp_keypair *key, + mbedtls_ecdh_side side ) { int ret; - if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) - return( ret ); - /* If it's not our key, just import the public part as Qp */ if( side == MBEDTLS_ECDH_THEIRS ) return( mbedtls_ecp_copy( &ctx->Qp, &key->Q ) ); @@ -300,29 +413,61 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, const mbedtls_ecp_keypai } /* - * Setup and export the client public value + * Get parameters from a keypair */ -int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, - unsigned char *buf, size_t blen, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, + const mbedtls_ecp_keypair *key, + mbedtls_ecdh_side side ) +{ + int ret; + + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + + if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) + return( ret ); + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_get_params_internal( ctx, key, side ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_get_params_internal( &ctx->ctx.mbed_ecdh, + key, side ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_make_public_internal( mbedtls_ecdh_context_mbed *ctx, + size_t *olen, int point_format, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, + unsigned char *, + size_t), + void *p_rng, + int restart_enabled ) { int ret; #if defined(MBEDTLS_ECP_RESTARTABLE) mbedtls_ecp_restart_ctx *rs_ctx = NULL; #endif - if( ctx == NULL || ctx->grp.pbits == 0 ) + if( ctx->grp.pbits == 0 ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ctx->restart_enabled ) + if( restart_enabled ) rs_ctx = &ctx->rs; +#else + (void) restart_enabled; #endif #if defined(MBEDTLS_ECP_RESTARTABLE) if( ( ret = ecdh_gen_public_restartable( &ctx->grp, &ctx->d, &ctx->Q, - f_rng, p_rng, rs_ctx ) ) != 0 ) + f_rng, p_rng, rs_ctx ) ) != 0 ) return( ret ); #else if( ( ret = mbedtls_ecdh_gen_public( &ctx->grp, &ctx->d, &ctx->Q, @@ -330,23 +475,52 @@ int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, return( ret ); #endif /* MBEDTLS_ECP_RESTARTABLE */ - return mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, ctx->point_format, - olen, buf, blen ); + return mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, point_format, olen, + buf, blen ); } /* - * Parse and import the client's public value + * Setup and export the client public value */ -int mbedtls_ecdh_read_public( mbedtls_ecdh_context *ctx, - const unsigned char *buf, size_t blen ) +int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) { - int ret; - const unsigned char *p = buf; + int restart_enabled = 0; if( ctx == NULL ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); - if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, &p, blen ) ) != 0 ) +#if defined(MBEDTLS_ECP_RESTARTABLE) + restart_enabled = ctx->restart_enabled; +#endif + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_make_public_internal( ctx, olen, ctx->point_format, buf, blen, + f_rng, p_rng, restart_enabled ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_make_public_internal( &ctx->ctx.mbed_ecdh, olen, + ctx->point_format, buf, blen, + f_rng, p_rng, + restart_enabled ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_read_public_internal( mbedtls_ecdh_context_mbed *ctx, + const unsigned char *buf, size_t blen ) +{ + int ret; + const unsigned char *p = buf; + + if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, &p, + blen ) ) != 0 ) return( ret ); if( (size_t)( p - buf ) != blen ) @@ -356,12 +530,36 @@ int mbedtls_ecdh_read_public( mbedtls_ecdh_context *ctx, } /* - * Derive and export the shared secret + * Parse and import the client's public value */ -int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, - unsigned char *buf, size_t blen, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +int mbedtls_ecdh_read_public( mbedtls_ecdh_context *ctx, + const unsigned char *buf, size_t blen ) +{ + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_read_public_internal( ctx, buf, blen ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_read_public_internal( &ctx->ctx.mbed_ecdh, + buf, blen ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_calc_secret_internal( mbedtls_ecdh_context_mbed *ctx, + size_t *olen, unsigned char *buf, + size_t blen, + int (*f_rng)(void *, + unsigned char *, + size_t), + void *p_rng, + int restart_enabled ) { int ret; #if defined(MBEDTLS_ECP_RESTARTABLE) @@ -372,13 +570,16 @@ int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ctx->restart_enabled ) + if( restart_enabled ) rs_ctx = &ctx->rs; +#else + (void) restart_enabled; #endif #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ( ret = ecdh_compute_shared_restartable( &ctx->grp, - &ctx->z, &ctx->Qp, &ctx->d, f_rng, p_rng, rs_ctx ) ) != 0 ) + if( ( ret = ecdh_compute_shared_restartable( &ctx->grp, &ctx->z, &ctx->Qp, + &ctx->d, f_rng, p_rng, + rs_ctx ) ) != 0 ) { return( ret ); } @@ -397,4 +598,37 @@ int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, return mbedtls_mpi_write_binary( &ctx->z, buf, *olen ); } +/* + * Derive and export the shared secret + */ +int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + int restart_enabled = 0; + + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + +#if defined(MBEDTLS_ECP_RESTARTABLE) + restart_enabled = ctx->restart_enabled; +#endif + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_calc_secret_internal( ctx, olen, buf, blen, f_rng, p_rng, + restart_enabled ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_calc_secret_internal( &ctx->ctx.mbed_ecdh, olen, buf, + blen, f_rng, p_rng, + restart_enabled ) ); + default: + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + } +#endif +} + #endif /* MBEDTLS_ECDH_C */ From 948f4bedcceacd44dac08797f42019f3dad7cb13 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 22 Aug 2018 01:37:55 +0100 Subject: [PATCH 07/11] Debug: Add functions for ECDH contexts The SSL module accesses ECDH context members directly to print debug information. This can't work with the new context, where we can't make assumptions about the implementation of the context. This commit adds new debug functions to complete the encapsulation of the ECDH context and work around the problem. --- include/mbedtls/debug.h | 36 +++++++++++++++++++++++++++++ library/debug.c | 50 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/include/mbedtls/debug.h b/include/mbedtls/debug.h index ef8db67ff..736444bb7 100644 --- a/include/mbedtls/debug.h +++ b/include/mbedtls/debug.h @@ -65,6 +65,11 @@ mbedtls_debug_print_crt( ssl, level, __FILE__, __LINE__, text, crt ) #endif +#if defined(MBEDTLS_ECDH_C) +#define MBEDTLS_SSL_DEBUG_ECDH( level, ecdh, attr ) \ + mbedtls_debug_printf_ecdh( ssl, level, __FILE__, __LINE__, ecdh, attr ) +#endif + #else /* MBEDTLS_DEBUG_C */ #define MBEDTLS_SSL_DEBUG_MSG( level, args ) do { } while( 0 ) @@ -73,6 +78,7 @@ #define MBEDTLS_SSL_DEBUG_MPI( level, text, X ) do { } while( 0 ) #define MBEDTLS_SSL_DEBUG_ECP( level, text, X ) do { } while( 0 ) #define MBEDTLS_SSL_DEBUG_CRT( level, text, crt ) do { } while( 0 ) +#define MBEDTLS_SSL_DEBUG_ECDH( level, ecdh, attr ) do { } while( 0 ) #endif /* MBEDTLS_DEBUG_C */ @@ -221,6 +227,36 @@ void mbedtls_debug_print_crt( const mbedtls_ssl_context *ssl, int level, const char *text, const mbedtls_x509_crt *crt ); #endif +#if defined(MBEDTLS_ECDH_C) +typedef enum +{ + MBEDTLS_DEBUG_ECDH_Q, + MBEDTLS_DEBUG_ECDH_QP, + MBEDTLS_DEBUG_ECDH_Z, +} mbedtls_debug_ecdh_attr; + +/** + * \brief Print a field of the ECDH structure in the SSL context to the debug + * output. This function is always used through the + * MBEDTLS_SSL_DEBUG_ECDH() macro, which supplies the ssl context, file + * and line number parameters. + * + * \param ssl SSL context + * \param level error level of the debug message + * \param file file the error has occurred in + * \param line line number the error has occurred in + * \param ecdh the ECDH context + * \param attr the identifier of the attribute being output + * + * \attention This function is intended for INTERNAL usage within the + * library only. + */ +void mbedtls_debug_printf_ecdh( const mbedtls_ssl_context *ssl, int level, + const char *file, int line, + const mbedtls_ecdh_context *ecdh, + mbedtls_debug_ecdh_attr attr ); +#endif + #ifdef __cplusplus } #endif diff --git a/library/debug.c b/library/debug.c index db3924ac5..824cd0236 100644 --- a/library/debug.c +++ b/library/debug.c @@ -365,4 +365,54 @@ void mbedtls_debug_print_crt( const mbedtls_ssl_context *ssl, int level, } #endif /* MBEDTLS_X509_CRT_PARSE_C */ +#if defined(MBEDTLS_ECDH_C) +static void mbedtls_debug_printf_ecdh_internal( const mbedtls_ssl_context *ssl, + int level, const char *file, + int line, + const mbedtls_ecdh_context *ecdh, + mbedtls_debug_ecdh_attr attr ) +{ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + const mbedtls_ecdh_context* ctx = ecdh; +#else + const mbedtls_ecdh_context_mbed* ctx = &ecdh->ctx.mbed_ecdh; +#endif + + switch( attr ) + { + case MBEDTLS_DEBUG_ECDH_Q: + mbedtls_debug_print_ecp( ssl, level, file, line, "ECDH: Q", + &ctx->Q ); + break; + case MBEDTLS_DEBUG_ECDH_QP: + mbedtls_debug_print_ecp( ssl, level, file, line, "ECDH: Qp", + &ctx->Qp ); + break; + case MBEDTLS_DEBUG_ECDH_Z: + mbedtls_debug_print_mpi( ssl, level, file, line, "ECDH: z", + &ctx->z ); + break; + default: + break; + } +} + +void mbedtls_debug_printf_ecdh( const mbedtls_ssl_context *ssl, int level, + const char *file, int line, + const mbedtls_ecdh_context *ecdh, + mbedtls_debug_ecdh_attr attr ) +{ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + mbedtls_debug_printf_ecdh_internal( ssl, level, file, line, ecdh, attr ); +#else + switch( ecdh->var ) + { + default: + mbedtls_debug_printf_ecdh_internal( ssl, level, file, line, ecdh, + attr ); + } +#endif +} +#endif /* MBEDTLS_ECDH_C */ + #endif /* MBEDTLS_DEBUG_C */ From 3fbdadad7b9d3db16936bbb275f5915224a2baba Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 15 Aug 2018 10:26:53 +0100 Subject: [PATCH 08/11] SSL: Make use of the new ECDH interface The SSL module accesses ECDH context members directly. This can't work with the new context, where we can't make any assumption about the implementation of the context. This commit makes use of the new functions to avoid accessing ECDH members directly. The only members that are still accessed directly are the group ID and the point format and they are independent from the implementation. --- library/ssl_cli.c | 22 ++++++++++++++++------ library/ssl_srv.c | 16 ++++++++++------ library/ssl_tls.c | 3 ++- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index ff576f3a8..afced7a99 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2027,8 +2027,14 @@ static int ssl_parse_server_dh_params( mbedtls_ssl_context *ssl, unsigned char * static int ssl_check_server_ecdh_params( const mbedtls_ssl_context *ssl ) { const mbedtls_ecp_curve_info *curve_info; + mbedtls_ecp_group_id grp_id; +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + grp_id = ssl->handshake->ecdh_ctx.grp.id; +#else + grp_id = ssl->handshake->ecdh_ctx.grp_id; +#endif - curve_info = mbedtls_ecp_curve_info_from_grp_id( ssl->handshake->ecdh_ctx.grp.id ); + curve_info = mbedtls_ecp_curve_info_from_grp_id( grp_id ); if( curve_info == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -2038,14 +2044,15 @@ 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, ssl->handshake->ecdh_ctx.grp.id ) != 0 ) + 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_ECP( 3, "ECDH: Qp", &ssl->handshake->ecdh_ctx.Qp ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_QP ); return( 0 ); } @@ -2967,7 +2974,8 @@ static int ssl_write_client_key_exchange( mbedtls_ssl_context *ssl ) return( ret ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Q", &ssl->handshake->ecdh_ctx.Q ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Q ); #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled ) @@ -2994,7 +3002,8 @@ ecdh_calc_secret: return( ret ); } - MBEDTLS_SSL_DEBUG_MPI( 3, "ECDH: z", &ssl->handshake->ecdh_ctx.z ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Z ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED || @@ -3089,7 +3098,8 @@ ecdh_calc_secret: return( ret ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Q", &ssl->handshake->ecdh_ctx.Q ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Q ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED */ diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 36ca0d69f..bc77f8020 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -3048,8 +3048,8 @@ curve_matching_done: MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDHE curve: %s", (*curve)->name ) ); - if( ( ret = mbedtls_ecp_group_load( &ssl->handshake->ecdh_ctx.grp, - (*curve)->grp_id ) ) != 0 ) + if( ( ret = mbedtls_ecdh_setup( &ssl->handshake->ecdh_ctx, + (*curve)->grp_id ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ecp_group_load", ret ); return( ret ); @@ -3071,7 +3071,8 @@ curve_matching_done: ssl->out_msglen += len; - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Q ", &ssl->handshake->ecdh_ctx.Q ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Q ); } #endif /* MBEDTLS_KEY_EXCHANGE__SOME__ECDHE_ENABLED */ @@ -3794,7 +3795,8 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_RP ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Qp ", &ssl->handshake->ecdh_ctx.Qp ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_QP ); if( ( ret = mbedtls_ecdh_calc_secret( &ssl->handshake->ecdh_ctx, &ssl->handshake->pmslen, @@ -3806,7 +3808,8 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_CS ); } - MBEDTLS_SSL_DEBUG_MPI( 3, "ECDH: z ", &ssl->handshake->ecdh_ctx.z ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Z ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED || @@ -3919,7 +3922,8 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_RP ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Qp ", &ssl->handshake->ecdh_ctx.Qp ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_QP ); if( ( ret = mbedtls_ssl_psk_derive_premaster( ssl, ciphersuite_info->key_exchange ) ) != 0 ) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 82e65251f..c64036c0c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1333,7 +1333,8 @@ int mbedtls_ssl_psk_derive_premaster( mbedtls_ssl_context *ssl, mbedtls_key_exch *(p++) = (unsigned char)( zlen ); p += zlen; - MBEDTLS_SSL_DEBUG_MPI( 3, "ECDH: z", &ssl->handshake->ecdh_ctx.z ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Z ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED */ From 36c5f7fe9be9b0f7250cda63fb6a975c4d5a96f1 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 30 Oct 2018 14:08:52 +0000 Subject: [PATCH 09/11] ECDH: Hide context from tests The tests for the ECDH key exchange that use the context accessed it directly. This can't work with the new context, where we can't make any assumptions about the implementation of the context. This commit works around this problem and comes with the cost of allocating an extra structures on the stack when executing the test. One of the tests is testing an older interface for the sake of backward compatibility. The new ECDH context is not backward compatible and this test doesn't make any sense for it, therefore we skip this test in non-legacy mode. --- tests/suites/test_suite_ecdh.function | 29 ++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 52fecf350..7144763a2 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -129,6 +129,8 @@ void ecdh_exchange( int id ) const unsigned char *vbuf; size_t len; rnd_pseudo_info rnd_info; + unsigned char res_buf[1000]; + size_t res_len; mbedtls_ecdh_init( &srv ); mbedtls_ecdh_init( &cli ); @@ -147,9 +149,11 @@ void ecdh_exchange( int id ) TEST_ASSERT( mbedtls_ecdh_read_public( &srv, buf, len ) == 0 ); TEST_ASSERT( mbedtls_ecdh_calc_secret( &srv, &len, buf, 1000, - &rnd_pseudo_rand, &rnd_info ) == 0 ); - TEST_ASSERT( mbedtls_ecdh_calc_secret( &cli, &len, buf, 1000, NULL, NULL ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &srv.z, &cli.z ) == 0 ); + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_calc_secret( &cli, &res_len, res_buf, 1000, + NULL, NULL ) == 0 ); + TEST_ASSERT( len == res_len ); + TEST_ASSERT( memcmp( buf, res_buf, len ) == 0 ); exit: mbedtls_ecdh_free( &srv ); @@ -172,7 +176,9 @@ void ecdh_restart( int id, char *dA_str, char *dB_str, char *z_str, unsigned char rnd_buf_B[MBEDTLS_ECP_MAX_BYTES]; rnd_buf_info rnd_info_A, rnd_info_B; int cnt_restart; + mbedtls_ecp_group grp; + mbedtls_ecp_group_init( &grp ); mbedtls_ecdh_init( &srv ); mbedtls_ecdh_init( &cli ); @@ -184,16 +190,20 @@ void ecdh_restart( int id, char *dA_str, char *dB_str, char *z_str, rnd_info_B.buf = rnd_buf_B; rnd_info_B.length = unhexify( rnd_buf_B, dB_str ); - TEST_ASSERT( mbedtls_ecp_group_load( &srv.grp, id ) == 0 ); + /* The ECDH context is not guaranteed ot have an mbedtls_ecp_group structure + * in every configuration, therefore we load it separately. */ + TEST_ASSERT( mbedtls_ecp_group_load( &grp, id ) == 0 ); - /* otherwise we would have to fix the random buffer, - * as in ecdh_primitive_test_vec */ - TEST_ASSERT( srv.grp.nbits % 8 == 0 ); + /* Otherwise we would have to fix the random buffer, + * as in ecdh_primitive_testvec. */ + TEST_ASSERT( grp.nbits % 8 == 0 ); + + TEST_ASSERT( mbedtls_ecdh_setup( &srv, id ) == 0 ); /* set up restart parameters */ mbedtls_ecp_set_max_ops( max_ops ); - if( enable) + if( enable ) { mbedtls_ecdh_enable_restart( &srv ); mbedtls_ecdh_enable_restart( &cli ); @@ -269,12 +279,13 @@ void ecdh_restart( int id, char *dA_str, char *dB_str, char *z_str, TEST_ASSERT( memcmp( buf, z, len ) == 0 ); exit: + mbedtls_ecp_group_free( &grp ); mbedtls_ecdh_free( &srv ); mbedtls_ecdh_free( &cli ); } /* END_CASE */ -/* BEGIN_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_ECDH_LEGACY_CONTEXT */ void ecdh_exchange_legacy( int id ) { mbedtls_ecdh_context srv, cli; From b8f27060e0e05da8fe318acb041ec17a12c0e698 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 16 Aug 2018 16:32:43 +0100 Subject: [PATCH 10/11] Add Changelog entry for the new ECDH context --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 8f0e8c1c7..f3dca4072 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.x.x branch released xxxx-xx-xx + +Changes + * Extend ECDH interface to enable alternative implementations. + = mbed TLS 2.14.0 branch released 2018-11-19 Security From c3b680b028495615733c5272ae3a74c6498f1484 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 5 Dec 2018 16:01:13 +0000 Subject: [PATCH 11/11] Clarify requirements on handling ECP group IDs --- include/mbedtls/ecp.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 0e5b47fc2..1c372980e 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -159,6 +159,10 @@ mbedtls_ecp_point; * additions or subtractions. Therefore, it is only an approximative modular * reduction. It must return 0 on success and non-zero on failure. * + * \note Alternative implementations must keep the group IDs distinct. If + * two group structures have the same ID, then they must be + * identical. + * */ typedef struct mbedtls_ecp_group {