From 799e57612abd3f587206f2034ba04b9dc7c57ba2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 17:34:00 +0200 Subject: [PATCH 01/16] ECDSA requires a short Weierstrass curve Document in config.h, and enforce in check_config.h, that MBEDTLS_ECDSA_C requires at least one short Weierstrass curve to be enabled. A Montgomery curve is not enough. Signed-off-by: Gilles Peskine --- include/mbedtls/check_config.h | 11 +++++++++++ include/mbedtls/config.h | 6 +++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index f2148a8b5e..3596597900 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -103,6 +103,17 @@ #if defined(MBEDTLS_ECDSA_C) && \ ( !defined(MBEDTLS_ECP_C) || \ + !( defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) ) || \ !defined(MBEDTLS_ASN1_PARSE_C) || \ !defined(MBEDTLS_ASN1_WRITE_C) ) #error "MBEDTLS_ECDSA_C defined, but not all prerequisites" diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index e00c546e5a..0bab0c0cbe 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -756,6 +756,7 @@ * * Comment macros to disable the curve and functions for it */ +/* Short Weierstrass curves (supporting ECP, ECDH, ECDSA) */ #define MBEDTLS_ECP_DP_SECP192R1_ENABLED #define MBEDTLS_ECP_DP_SECP224R1_ENABLED #define MBEDTLS_ECP_DP_SECP256R1_ENABLED @@ -767,6 +768,7 @@ #define MBEDTLS_ECP_DP_BP256R1_ENABLED #define MBEDTLS_ECP_DP_BP384R1_ENABLED #define MBEDTLS_ECP_DP_BP512R1_ENABLED +/* Montgomery curves (supporting ECP) */ #define MBEDTLS_ECP_DP_CURVE25519_ENABLED #define MBEDTLS_ECP_DP_CURVE448_ENABLED @@ -2571,7 +2573,9 @@ * This module is used by the following key exchanges: * ECDHE-ECDSA * - * Requires: MBEDTLS_ECP_C, MBEDTLS_ASN1_WRITE_C, MBEDTLS_ASN1_PARSE_C + * Requires: MBEDTLS_ECP_C, MBEDTLS_ASN1_WRITE_C, MBEDTLS_ASN1_PARSE_C, + * and at least one MBEDTLS_ECP_DP_XXX_ENABLED for a + * short Weierstrass curve. */ #define MBEDTLS_ECDSA_C From 9b99a8942f4afd63e5653482d20a2be1330c8c6a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 18:32:19 +0200 Subject: [PATCH 02/16] mbedtls_ecp_muladd is only for short Weierstrass curves Document that mbedtls_ecp_muladd and mbedtls_ecp_muladd_restartable are only implemented on short Weierstrass curves. Exclude these functions at build time if no short Weierstrass curve is included in the build. Before, these functions failed to compile in such a configuration. Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 34 ++++++++++++++++++++++++++++++++++ library/ecp.c | 2 ++ 2 files changed, 36 insertions(+) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 450e354926..8185224a9f 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -61,6 +61,26 @@ #define MBEDTLS_ERR_ECP_IN_PROGRESS -0x4B00 /**< Operation in progress, call again with the same parameters to continue. */ +/* Flags indicating whether to include code that is specific to certain + * types of curves. These flags are for internal library use only. */ +#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) +#define MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED +#endif +#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ + defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) +#define MBEDTLS_ECP_MONTGOMERY_ENABLED +#endif + #ifdef __cplusplus extern "C" { #endif @@ -906,6 +926,7 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng, mbedtls_ecp_restart_ctx *rs_ctx ); +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /** * \brief This function performs multiplication and addition of two * points by integers: \p R = \p m * \p P + \p n * \p Q @@ -915,6 +936,10 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * \note In contrast to mbedtls_ecp_mul(), this function does not * guarantee a constant execution flow and timing. * + * \note This function is only defined for short Weierstrass curves. + * It may not be included in builds without any short + * Weierstrass curve. + * * \param grp The ECP group to use. * This must be initialized and have group parameters * set, for example through mbedtls_ecp_group_load(). @@ -933,6 +958,8 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * valid private keys, or \p P or \p Q are not valid public * keys. * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED on memory-allocation failure. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if \p grp does not + * designate a short Weierstrass curve. * \return Another negative error code on other kinds of failure. */ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, @@ -950,6 +977,10 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * but it can return early and restart according to the limit * set with \c mbedtls_ecp_set_max_ops() to reduce blocking. * + * \note This function is only defined for short Weierstrass curves. + * It may not be included in builds without any short + * Weierstrass curve. + * * \param grp The ECP group to use. * This must be initialized and have group parameters * set, for example through mbedtls_ecp_group_load(). @@ -969,6 +1000,8 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * valid private keys, or \p P or \p Q are not valid public * keys. * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED on memory-allocation failure. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if \p grp does not + * designate a short Weierstrass curve. * \return #MBEDTLS_ERR_ECP_IN_PROGRESS if maximum number of * operations was reached: see \c mbedtls_ecp_set_max_ops(). * \return Another negative error code on other kinds of failure. @@ -978,6 +1011,7 @@ int mbedtls_ecp_muladd_restartable( const mbedtls_mpi *m, const mbedtls_ecp_point *P, const mbedtls_mpi *n, const mbedtls_ecp_point *Q, mbedtls_ecp_restart_ctx *rs_ctx ); +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ /** * \brief This function checks that a point is a valid public key diff --git a/library/ecp.c b/library/ecp.c index 2f69d68692..1b289f67a6 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2781,6 +2781,7 @@ cleanup: } #endif /* ECP_SHORTWEIERSTRASS */ +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* * R = m * P with shortcuts for m == 1 and m == -1 * NOT constant-time - ONLY for short Weierstrass! @@ -2926,6 +2927,7 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, ECP_VALIDATE_RET( Q != NULL ); return( mbedtls_ecp_muladd_restartable( grp, R, m, P, n, Q, NULL ) ); } +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ #if defined(ECP_MONTGOMERY) /* From e8c04fed5129a2fdf89d0aad8746420b5bceb634 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 17:44:21 +0200 Subject: [PATCH 03/16] Replace ECP_xxx by MBEDTLS_ECP__xxx_ENABLED Replace the now-redundant internal curve type macros ECP_xxx by the macros MBEDTLS_ECP__xxx_ENABLED which are declared in ecp.h. Signed-off-by: Gilles Peskine --- include/mbedtls/ecp_internal.h | 8 ++-- library/ecp.c | 75 +++++++++++++--------------------- 2 files changed, 32 insertions(+), 51 deletions(-) diff --git a/include/mbedtls/ecp_internal.h b/include/mbedtls/ecp_internal.h index 3b6fbf1121..92fee42d4b 100644 --- a/include/mbedtls/ecp_internal.h +++ b/include/mbedtls/ecp_internal.h @@ -105,7 +105,7 @@ int mbedtls_internal_ecp_init( const mbedtls_ecp_group *grp ); */ void mbedtls_internal_ecp_free( const mbedtls_ecp_group *grp ); -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) #if defined(MBEDTLS_ECP_RANDOMIZE_JAC_ALT) /** @@ -245,9 +245,9 @@ int mbedtls_internal_ecp_normalize_jac( const mbedtls_ecp_group *grp, mbedtls_ecp_point *pt ); #endif -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) #if defined(MBEDTLS_ECP_DOUBLE_ADD_MXZ_ALT) int mbedtls_internal_ecp_double_add_mxz( const mbedtls_ecp_group *grp, @@ -291,7 +291,7 @@ int mbedtls_internal_ecp_normalize_mxz( const mbedtls_ecp_group *grp, mbedtls_ecp_point *P ); #endif -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ #endif /* MBEDTLS_ECP_INTERNAL_ALT */ diff --git a/library/ecp.c b/library/ecp.c index 1b289f67a6..ab5ab9585c 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -501,25 +501,6 @@ int mbedtls_ecp_check_budget( const mbedtls_ecp_group *grp, #endif /* MBEDTLS_ECP_RESTARTABLE */ -#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) -#define ECP_SHORTWEIERSTRASS -#endif - -#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ - defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) -#define ECP_MONTGOMERY -#endif - /* * List of supported curves: * - internal ID @@ -897,7 +878,7 @@ int mbedtls_ecp_point_write_binary( const mbedtls_ecp_group *grp, plen = mbedtls_mpi_size( &grp->P ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { *olen = plen; @@ -907,7 +888,7 @@ int mbedtls_ecp_point_write_binary( const mbedtls_ecp_group *grp, MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary_le( &P->X, buf, plen ) ); } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { /* @@ -970,7 +951,7 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, plen = mbedtls_mpi_size( &grp->P ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { if( plen != ilen ) @@ -986,7 +967,7 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &pt->Z, 1 ) ); } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { if( buf[0] == 0x00 ) @@ -1304,7 +1285,7 @@ cleanup: return( ret ); } -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* * For curves in short Weierstrass form, we do all the internal operations in * Jacobian coordinates. @@ -2413,9 +2394,9 @@ cleanup: return( ret ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) /* * For Montgomery curves, we do all the internal arithmetic in projective * coordinates. Import/export of points uses only the x coordinates, which is @@ -2649,7 +2630,7 @@ cleanup: return( ret ); } -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ /* * Restartable multiplication R = m * P @@ -2693,11 +2674,11 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, } ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) MBEDTLS_MPI_CHK( ecp_mul_mxz( grp, R, m, P, f_rng, p_rng ) ); #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) MBEDTLS_MPI_CHK( ecp_mul_comb( grp, R, m, P, f_rng, p_rng, rs_ctx ) ); #endif @@ -2731,7 +2712,7 @@ int mbedtls_ecp_mul( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, return( mbedtls_ecp_mul_restartable( grp, R, m, P, f_rng, p_rng, NULL ) ); } -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* * Check that an affine point is valid as a public key, * short weierstrass curves (SEC1 3.2.3.1) @@ -2779,7 +2760,7 @@ cleanup: return( ret ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ #if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* @@ -2929,7 +2910,7 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, } #endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) /* * Check validity of a public key for Montgomery curves with x-only schemes */ @@ -2943,7 +2924,7 @@ static int ecp_check_pubkey_mx( const mbedtls_ecp_group *grp, const mbedtls_ecp_ return( 0 ); } -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ /* * Check that a point is valid as a public key @@ -2958,11 +2939,11 @@ int mbedtls_ecp_check_pubkey( const mbedtls_ecp_group *grp, if( mbedtls_mpi_cmp_int( &pt->Z, 1 ) != 0 ) return( MBEDTLS_ERR_ECP_INVALID_KEY ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) return( ecp_check_pubkey_mx( grp, pt ) ); #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) return( ecp_check_pubkey_sw( grp, pt ) ); #endif @@ -2978,7 +2959,7 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, ECP_VALIDATE_RET( grp != NULL ); ECP_VALIDATE_RET( d != NULL ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { /* see RFC 7748 sec. 5 para. 5 */ @@ -2993,8 +2974,8 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, return( 0 ); } -#endif /* ECP_MONTGOMERY */ -#if defined(ECP_SHORTWEIERSTRASS) +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { /* see SEC1 3.2 */ @@ -3004,7 +2985,7 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, else return( 0 ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); } @@ -3026,7 +3007,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, n_size = ( grp->nbits + 7 ) / 8; -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { /* [M225] page 5 */ @@ -3052,9 +3033,9 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 2, 0 ) ); } } -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { /* SEC1 3.2.1: Generate d such that 1 <= n < N */ @@ -3096,7 +3077,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, } while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp != 1 ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ cleanup: return( ret ); @@ -3174,7 +3155,7 @@ int mbedtls_ecp_read_key( mbedtls_ecp_group_id grp_id, mbedtls_ecp_keypair *key, ret = MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE; -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { /* @@ -3209,7 +3190,7 @@ int mbedtls_ecp_read_key( mbedtls_ecp_group_id grp_id, mbedtls_ecp_keypair *key, } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( &key->d, buf, buflen ) ); @@ -3237,7 +3218,7 @@ int mbedtls_ecp_write_key( mbedtls_ecp_keypair *key, ECP_VALIDATE_RET( key != NULL ); ECP_VALIDATE_RET( buf != NULL ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { if( key->grp.id == MBEDTLS_ECP_DP_CURVE25519 ) @@ -3252,7 +3233,7 @@ int mbedtls_ecp_write_key( mbedtls_ecp_keypair *key, } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( &key->d, buf, buflen ) ); From aa9493a4111e88ec1918dca5b1abb281faa405b6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Sep 2018 14:44:03 +0200 Subject: [PATCH 04/16] Add guards around code that is specific to dynamically-loaded groups For some curves (semi-coincidentally, short Weierstrass curves), the ECP module calculates some group parameters dynamically. Build the code to calculate the parameters only if a relevant curve is enabled. This fixes an unused function warning when building with only Montgomery curves. Signed-off-by: Gilles Peskine --- library/ecp_curves.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index 92bbb896a5..137ef1ede8 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -553,6 +553,22 @@ static const mbedtls_mpi_uint brainpoolP512r1_n[] = { }; #endif /* MBEDTLS_ECP_DP_BP512R1_ENABLED */ +#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) +/* For these curves, we build the group parameters dynamically. */ +#define ECP_LOAD_GROUP +#endif + +#if defined(ECP_LOAD_GROUP) /* * Create an MPI from embedded constants * (assumes len is an exact multiple of sizeof mbedtls_mpi_uint) @@ -603,6 +619,7 @@ static int ecp_group_load( mbedtls_ecp_group *grp, return( 0 ); } +#endif /* ECP_LOAD_GROUP */ #if defined(MBEDTLS_ECP_NIST_OPTIM) /* Forward declarations */ @@ -644,6 +661,7 @@ static int ecp_mod_p224k1( mbedtls_mpi * ); static int ecp_mod_p256k1( mbedtls_mpi * ); #endif +#if defined(ECP_LOAD_GROUP) #define LOAD_GROUP_A( G ) ecp_group_load( grp, \ G ## _p, sizeof( G ## _p ), \ G ## _a, sizeof( G ## _a ), \ @@ -659,6 +677,7 @@ static int ecp_mod_p256k1( mbedtls_mpi * ); G ## _gx, sizeof( G ## _gx ), \ G ## _gy, sizeof( G ## _gy ), \ G ## _n, sizeof( G ## _n ) ) +#endif /* ECP_LOAD_GROUP */ #if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) /* From 963a207678ff0b83ee39641af1b452d78049632a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 18:31:30 +0200 Subject: [PATCH 05/16] Document what needs to be done when adding a new curve Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 8185224a9f..7fe7473022 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -94,6 +94,20 @@ extern "C" { * parameters. Therefore, only standardized domain parameters from trusted * sources should be used. See mbedtls_ecp_group_load(). */ +/* Note: when adding a new curve: + * - Add it at the end of this enum, otherwise you'll break the ABI by + * changing the numerical value for existing curves. + * - Increment MBEDTLS_ECP_DP_MAX below. + * - Add the corresponding MBEDTLS_ECP_DP_xxx_ENABLED macro definition to + * config.h. + * - List the curve as a dependency of MBEDTLS_ECP_C and + * MBEDTLS_ECDSA_C if supported in check_config.h. + * - Add the curve to the appropriate curve type macro + * MBEDTLS_ECP_yyy_ENABLED above. + * - Add the necessary definitions to ecp_curves.c. + * - Add the curve to the ecp_supported_curves array in ecp.c. + * - Add the curve to applicable profiles in x509_crt.c if applicable. + */ typedef enum { MBEDTLS_ECP_DP_NONE = 0, /*!< Curve not defined. */ From 7ab66a6bf166891978ac1beec53843451f7dfc4c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 17:47:41 +0200 Subject: [PATCH 06/16] Add missing dependencies for ECDH_xxx key exchanges ECDH_ECDSA requires ECDSA and ECDH_RSA requires RSA. Signed-off-by: Gilles Peskine --- include/mbedtls/check_config.h | 6 ++++-- include/mbedtls/config.h | 4 ++-- tests/scripts/depends-pkalgs.pl | 4 +++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 3596597900..4f6c6328d9 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -257,12 +257,14 @@ #endif #if defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED) && \ - ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_X509_CRT_PARSE_C) ) + ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_ECDSA_C) || \ + !defined(MBEDTLS_X509_CRT_PARSE_C) ) #error "MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED defined, but not all prerequisites" #endif #if defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) && \ - ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_X509_CRT_PARSE_C) ) + ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_RSA_C) || \ + !defined(MBEDTLS_X509_CRT_PARSE_C) ) #error "MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED defined, but not all prerequisites" #endif diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 0bab0c0cbe..24ba789925 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1085,7 +1085,7 @@ * * Enable the ECDH-ECDSA based ciphersuite modes in SSL / TLS. * - * Requires: MBEDTLS_ECDH_C, MBEDTLS_X509_CRT_PARSE_C + * Requires: MBEDTLS_ECDH_C, MBEDTLS_ECDSA_C, MBEDTLS_X509_CRT_PARSE_C * * This enables the following ciphersuites (if other requisites are * enabled as well): @@ -1109,7 +1109,7 @@ * * Enable the ECDH-RSA based ciphersuite modes in SSL / TLS. * - * Requires: MBEDTLS_ECDH_C, MBEDTLS_X509_CRT_PARSE_C + * Requires: MBEDTLS_ECDH_C, MBEDTLS_RSA_C, MBEDTLS_X509_CRT_PARSE_C * * This enables the following ciphersuites (if other requisites are * enabled as well): diff --git a/tests/scripts/depends-pkalgs.pl b/tests/scripts/depends-pkalgs.pl index 1577fee384..0d5d297416 100755 --- a/tests/scripts/depends-pkalgs.pl +++ b/tests/scripts/depends-pkalgs.pl @@ -50,7 +50,8 @@ my $config_h = 'include/mbedtls/config.h'; # Some algorithms can't be disabled on their own as others depend on them, so # we list those reverse-dependencies here to keep check_config.h happy. my %algs = ( - 'MBEDTLS_ECDSA_C' => ['MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED'], + 'MBEDTLS_ECDSA_C' => ['MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED', + 'MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED'], 'MBEDTLS_ECP_C' => ['MBEDTLS_ECDSA_C', 'MBEDTLS_ECDH_C', 'MBEDTLS_ECJPAKE_C', @@ -68,6 +69,7 @@ my %algs = ( 'MBEDTLS_RSA_C' => ['MBEDTLS_X509_RSASSA_PSS_SUPPORT', 'MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED', 'MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED', + 'MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED', 'MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED', 'MBEDTLS_KEY_EXCHANGE_RSA_ENABLED'], ); From d9767a579945e47b5667e336c6dec2f40dc95f72 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 19:29:47 +0200 Subject: [PATCH 07/16] Tweak ECP self-test to work with secp192k1 The constants used in the test worked with every supported curve except secp192k1. For secp192k1, the "N-1" exponent was too large. Signed-off-by: Gilles Peskine --- library/ecp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index ab5ab9585c..00917e8420 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3303,11 +3303,13 @@ int mbedtls_ecp_self_test( int verbose ) mbedtls_ecp_point R, P; mbedtls_mpi m; unsigned long add_c_prev, dbl_c_prev, mul_c_prev; - /* exponents especially adapted for secp192r1 */ + /* Exponents especially adapted for secp192k1, which has the lowest + * order n of all supported curves (secp192r1 is in a slightly larger + * field but the order of its base point is slightly smaller). */ const char *exponents[] = { "000000000000000000000000000000000000000000000001", /* one */ - "FFFFFFFFFFFFFFFFFFFFFFFF99DEF836146BC9B1B4D22830", /* N - 1 */ + "FFFFFFFFFFFFFFFFFFFFFFFE26F2FC170F69466A74DEFD8C", /* n - 1 */ "5EA6F389A38B8BC81E767753B15AA5569E1782E30ABE7D25", /* random */ "400000000000000000000000000000000000000000000000", /* one and zeros */ "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF", /* all ones */ From c95696fec46f8ee126acb398332e3d7c1d94b644 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 15:59:01 +0200 Subject: [PATCH 08/16] Factor common code in mbedtls_ecp_self_test No intended behavior change. Signed-off-by: Gilles Peskine --- library/ecp.c | 128 +++++++++++++++++++++++--------------------------- 1 file changed, 59 insertions(+), 69 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 00917e8420..1f7943aa53 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3292,17 +3292,64 @@ cleanup: #if defined(MBEDTLS_SELF_TEST) +static int self_test_point( int verbose, + mbedtls_ecp_group *grp, + mbedtls_ecp_point *R, + mbedtls_mpi *m, + mbedtls_ecp_point *P, + const char *const *exponents, + size_t n_exponents ) +{ + int ret = 0; + size_t i; + unsigned long add_c_prev, dbl_c_prev, mul_c_prev; + add_count = 0; + dbl_count = 0; + mul_count = 0; + MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[0] ) ); + MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); + + for( i = 1; i < n_exponents; i++ ) + { + add_c_prev = add_count; + dbl_c_prev = dbl_count; + mul_c_prev = mul_count; + add_count = 0; + dbl_count = 0; + mul_count = 0; + + MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[i] ) ); + MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); + + if( add_count != add_c_prev || + dbl_count != dbl_c_prev || + mul_count != mul_c_prev ) + { + ret = 1; + break; + } + } + +cleanup: + if( verbose != 0 ) + { + if( ret != 0 ) + mbedtls_printf( "failed (%u)\n", (unsigned int) i ); + else + mbedtls_printf( "passed\n" ); + } + return( ret ); +} + /* * Checkup routine */ int mbedtls_ecp_self_test( int verbose ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - size_t i; mbedtls_ecp_group grp; mbedtls_ecp_point R, P; mbedtls_mpi m; - unsigned long add_c_prev, dbl_c_prev, mul_c_prev; /* Exponents especially adapted for secp192k1, which has the lowest * order n of all supported curves (secp192r1 is in a slightly larger * field but the order of its base point is slightly smaller). */ @@ -3330,80 +3377,23 @@ int mbedtls_ecp_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( " ECP test #1 (constant op_count, base point G): " ); - /* Do a dummy multiplication first to trigger precomputation */ MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &m, 2 ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &P, &m, &grp.G, NULL, NULL ) ); - - add_count = 0; - dbl_count = 0; - mul_count = 0; - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[0] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &grp.G, NULL, NULL ) ); - - for( i = 1; i < sizeof( exponents ) / sizeof( exponents[0] ); i++ ) - { - add_c_prev = add_count; - dbl_c_prev = dbl_count; - mul_c_prev = mul_count; - add_count = 0; - dbl_count = 0; - mul_count = 0; - - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[i] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &grp.G, NULL, NULL ) ); - - if( add_count != add_c_prev || - dbl_count != dbl_c_prev || - mul_count != mul_c_prev ) - { - if( verbose != 0 ) - mbedtls_printf( "failed (%u)\n", (unsigned int) i ); - - ret = 1; - goto cleanup; - } - } - - if( verbose != 0 ) - mbedtls_printf( "passed\n" ); + ret = self_test_point( verbose, + &grp, &R, &m, &grp.G, + exponents, + sizeof( exponents ) / sizeof( exponents[0] )); + if( ret != 0 ) + goto cleanup; if( verbose != 0 ) mbedtls_printf( " ECP test #2 (constant op_count, other point): " ); /* We computed P = 2G last time, use it */ - - add_count = 0; - dbl_count = 0; - mul_count = 0; - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[0] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &P, NULL, NULL ) ); - - for( i = 1; i < sizeof( exponents ) / sizeof( exponents[0] ); i++ ) - { - add_c_prev = add_count; - dbl_c_prev = dbl_count; - mul_c_prev = mul_count; - add_count = 0; - dbl_count = 0; - mul_count = 0; - - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[i] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &P, NULL, NULL ) ); - - if( add_count != add_c_prev || - dbl_count != dbl_c_prev || - mul_count != mul_c_prev ) - { - if( verbose != 0 ) - mbedtls_printf( "failed (%u)\n", (unsigned int) i ); - - ret = 1; - goto cleanup; - } - } - - if( verbose != 0 ) - mbedtls_printf( "passed\n" ); + ret = self_test_point( verbose, + &grp, &R, &m, &P, + exponents, + sizeof( exponents ) / sizeof( exponents[0] )); cleanup: From 24666795e4a079001961881d260438d7e8c65926 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 18:29:49 +0200 Subject: [PATCH 09/16] ECP self test: add self-test step for Montgomery curves Run some self-test both for a short Weierstrass curve and for a Montgomery curve, if the build-time configuration includes a curve of both types. Run both because there are significant differences in the implementation. The test data is suitable for Curve25519. Signed-off-by: Gilles Peskine --- library/ecp.c | 55 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 1f7943aa53..519c50adbe 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3301,7 +3301,7 @@ static int self_test_point( int verbose, size_t n_exponents ) { int ret = 0; - size_t i; + size_t i = 0; unsigned long add_c_prev, dbl_c_prev, mul_c_prev; add_count = 0; dbl_count = 0; @@ -3350,10 +3350,12 @@ int mbedtls_ecp_self_test( int verbose ) mbedtls_ecp_group grp; mbedtls_ecp_point R, P; mbedtls_mpi m; + +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* Exponents especially adapted for secp192k1, which has the lowest * order n of all supported curves (secp192r1 is in a slightly larger * field but the order of its base point is slightly smaller). */ - const char *exponents[] = + const char *sw_exponents[] = { "000000000000000000000000000000000000000000000001", /* one */ "FFFFFFFFFFFFFFFFFFFFFFFE26F2FC170F69466A74DEFD8C", /* n - 1 */ @@ -3362,12 +3364,25 @@ int mbedtls_ecp_self_test( int verbose ) "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF", /* all ones */ "555555555555555555555555555555555555555555555555", /* 101010... */ }; +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) + const char *m_exponents[] = + { + "4000000000000000000000000000000000000000000000000000000000000000", + "5C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C30", + "5715ECCE24583F7A7023C24164390586842E816D7280A49EF6DF4EAE6B280BF8", + "41A2B017516F6D254E1F002BCCBADD54BE30F8CEC737A0E912B4963B6BA74460", + "5555555555555555555555555555555555555555555555555555555555555550", + "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF8", + }; +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ mbedtls_ecp_group_init( &grp ); mbedtls_ecp_point_init( &R ); mbedtls_ecp_point_init( &P ); mbedtls_mpi_init( &m ); +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* Use secp192r1 if available, or any available curve */ #if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) MBEDTLS_MPI_CHK( mbedtls_ecp_group_load( &grp, MBEDTLS_ECP_DP_SECP192R1 ) ); @@ -3376,24 +3391,48 @@ int mbedtls_ecp_self_test( int verbose ) #endif if( verbose != 0 ) - mbedtls_printf( " ECP test #1 (constant op_count, base point G): " ); + mbedtls_printf( " ECP SW test #1 (constant op_count, base point G): " ); /* Do a dummy multiplication first to trigger precomputation */ MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &m, 2 ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &P, &m, &grp.G, NULL, NULL ) ); ret = self_test_point( verbose, &grp, &R, &m, &grp.G, - exponents, - sizeof( exponents ) / sizeof( exponents[0] )); + sw_exponents, + sizeof( sw_exponents ) / sizeof( sw_exponents[0] )); if( ret != 0 ) goto cleanup; if( verbose != 0 ) - mbedtls_printf( " ECP test #2 (constant op_count, other point): " ); + mbedtls_printf( " ECP SW test #2 (constant op_count, other point): " ); /* We computed P = 2G last time, use it */ ret = self_test_point( verbose, &grp, &R, &m, &P, - exponents, - sizeof( exponents ) / sizeof( exponents[0] )); + sw_exponents, + sizeof( sw_exponents ) / sizeof( sw_exponents[0] )); + if( ret != 0 ) + goto cleanup; + + mbedtls_ecp_group_free( &grp ); + mbedtls_ecp_point_free( &R ); +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ + +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) + if( verbose != 0 ) + mbedtls_printf( " ECP Montgomery test (constant op_count): " ); +#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) + MBEDTLS_MPI_CHK( mbedtls_ecp_group_load( &grp, MBEDTLS_ECP_DP_CURVE25519 ) ); +#elif defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) + MBEDTLS_MPI_CHK( mbedtls_ecp_group_load( &grp, MBEDTLS_ECP_DP_CURVE448 ) ); +#else +#error "MBEDTLS_ECP_MONTGOMERY_ENABLED is defined, but no curve is supported for self-test" +#endif + ret = self_test_point( verbose, + &grp, &R, &m, &grp.G, + m_exponents, + sizeof( m_exponents ) / sizeof( m_exponents[0] )); + if( ret != 0 ) + goto cleanup; +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ cleanup: From a088c81fcba5357d0cf44b5b107c09582e20dc81 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 18:31:15 +0200 Subject: [PATCH 10/16] Adjust ECP self-test to support Curve448 Adjust the Montgomery self-test to use Curve448 in builds without Curve25519. Signed-off-by: Gilles Peskine --- library/ecp.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/library/ecp.c b/library/ecp.c index 519c50adbe..f44298ab80 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3292,6 +3292,39 @@ cleanup: #if defined(MBEDTLS_SELF_TEST) +static int self_test_adjust_exponent( const mbedtls_ecp_group *grp, + mbedtls_mpi *m ) +{ + int ret = 0; + switch( grp->id ) + { + /* If Curve25519 is available, then that's what we use for the + * Montgomery test, so we don't need the adjustment code. */ +#if ! defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) +#if defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) + case MBEDTLS_ECP_DP_CURVE448: + /* Move highest bit from 254 to N-1. Setting bit N-1 is + * necessary to enforce the highest-bit-set constraint. */ + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( m, 254, 0 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( m, grp->nbits, 1 ) ); + /* Copy second-highest bit from 253 to N-2. This is not + * necessary but improves the test variety a bit. */ + MBEDTLS_MPI_CHK( + mbedtls_mpi_set_bit( m, grp->nbits - 1, + mbedtls_mpi_get_bit( m, 253 ) ) ); + break; +#endif +#endif /* ! defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) */ + default: + /* Non-Montgomery curves and Curve25519 need no adjustment. */ + (void) grp; + (void) m; + goto cleanup; + } +cleanup: + return( ret ); +} + static int self_test_point( int verbose, mbedtls_ecp_group *grp, mbedtls_ecp_point *R, @@ -3306,7 +3339,9 @@ static int self_test_point( int verbose, add_count = 0; dbl_count = 0; mul_count = 0; + MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[0] ) ); + MBEDTLS_MPI_CHK( self_test_adjust_exponent( grp, m ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); for( i = 1; i < n_exponents; i++ ) @@ -3319,6 +3354,7 @@ static int self_test_point( int verbose, mul_count = 0; MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[i] ) ); + MBEDTLS_MPI_CHK( self_test_adjust_exponent( grp, m ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); if( add_count != add_c_prev || From a2611604d4dda9629e2659b77b6310de9be53b79 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 18:40:33 +0200 Subject: [PATCH 11/16] curves.pl: test with each elliptic curve enabled Previously curves.pl tested with all elliptic curves enabled except one, for each curve. This catches tests that are missing dependencies on one of the curve that they use, but does not catch misplaced conditional directives around parts of the library. Now, we additionally test with a single curve, for each curve. This catches missing or extraneous guards around code that is specific to one particular curve or to a class of curves. Signed-off-by: Gilles Peskine --- tests/scripts/curves.pl | 91 ++++++++++++++++++++++++++++++++++------- 1 file changed, 77 insertions(+), 14 deletions(-) diff --git a/tests/scripts/curves.pl b/tests/scripts/curves.pl index cd6ea0d9a5..8db44303cf 100755 --- a/tests/scripts/curves.pl +++ b/tests/scripts/curves.pl @@ -2,7 +2,7 @@ # curves.pl # -# Copyright (c) 2014-2016, ARM Limited, All Rights Reserved +# Copyright (c) 2014-2020, ARM Limited, All Rights Reserved # SPDX-License-Identifier: Apache-2.0 # # Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -21,21 +21,25 @@ # # Purpose # -# To test the code dependencies on individual curves in each test suite. This -# is a verification step to ensure we don't ship test suites that do not work -# for some build options. +# The purpose of this test script is to validate that the library works +# with any combination of elliptic curves. To this effect, build the library +# and run the test suite with each tested combination of elliptic curves. # -# The process is: -# for each possible curve -# build the library and test suites with the curve disabled -# execute the test suites -# -# And any test suite with the wrong dependencies will fail. +# Testing all 2^n combinations would be too much, so we only test 2*n: # +# 1. Test with a single curve, for each curve. This validates that the +# library works with any curve, and in particular that curve-specific +# code is guarded by the proper preprocessor conditionals. +# 2. Test with all curves except one, for each curve. This validates that +# the test cases have correct dependencies. Testing with a single curve +# doesn't validate this for tests that require more than one curve. + # Usage: tests/scripts/curves.pl # # This script should be executed from the root of the project directory. # +# Only curves that are enabled in config.h will be tested. +# # For best effect, run either with cmake disabled, or cmake enabled in a mode # that includes -Werror. @@ -48,6 +52,25 @@ my $sed_cmd = 's/^#define \(MBEDTLS_ECP_DP.*_ENABLED\)/\1/p'; my $config_h = 'include/mbedtls/config.h'; my @curves = split( /\s+/, `sed -n -e '$sed_cmd' $config_h` ); +# Determine which curves support ECDSA by checking the dependencies of +# ECDSA in check_config.h. +my %curve_supports_ecdsa = (); +{ + local $/ = ""; + local *CHECK_CONFIG; + open(CHECK_CONFIG, '<', 'include/mbedtls/check_config.h') + or die "open include/mbedtls/check_config.h: $!"; + while (my $stanza = ) { + if ($stanza =~ /\A#if defined\(MBEDTLS_ECDSA_C\)/) { + for my $curve ($stanza =~ /(?<=\()MBEDTLS_ECP_DP_\w+_ENABLED(?=\))/g) { + $curve_supports_ecdsa{$curve} = 1; + } + last; + } + } + close(CHECK_CONFIG); +} + system( "cp $config_h $config_h.bak" ) and die; sub abort { system( "mv $config_h.bak $config_h" ) and warn "$config_h not restored\n"; @@ -56,6 +79,46 @@ sub abort { exit 1; } +# Disable all the curves. We'll then re-enable them one by one. +for my $curve (@curves) { + system( "scripts/config.pl unset $curve" ) + and abort "Failed to disable $curve\n"; +} +# Depends on a specific curve. Also, ignore error if it wasn't enabled. +system( "scripts/config.pl unset MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED" ); + +# Test with only $curve enabled, for each $curve. +for my $curve (@curves) { + system( "make clean" ) and die; + + print "\n******************************************\n"; + print "* Testing with only curve: $curve\n"; + print "******************************************\n"; + $ENV{MBEDTLS_TEST_CONFIGURATION} = "$curve"; + + system( "scripts/config.pl set $curve" ) + and abort "Failed to enable $curve\n"; + + my $ecdsa = $curve_supports_ecdsa{$curve} ? "set" : "unset"; + for my $dep (qw(MBEDTLS_ECDSA_C + MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED + MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED)) { + system( "scripts/config.pl $ecdsa $dep" ) + and abort "Failed to $ecdsa $dep\n"; + } + + system( "CFLAGS='-Werror -Wall -Wextra' make" ) + and abort "Failed to build: only $curve\n"; + system( "make test" ) + and abort "Failed test suite: only $curve\n"; + + system( "scripts/config.pl unset $curve" ) + and abort "Failed to disable $curve\n"; +} + +system( "cp $config_h.bak $config_h" ) and die "$config_h not restored\n"; + +# Test with $curve disabled but the others enabled, for each $curve. for my $curve (@curves) { system( "cp $config_h.bak $config_h" ) and die "$config_h not restored\n"; system( "make clean" ) and die; @@ -71,10 +134,10 @@ for my $curve (@curves) { system( "scripts/config.py unset $curve" ) and abort "Failed to disable $curve\n"; - system( "CFLAGS='-Werror -Wall -Wextra' make lib" ) - and abort "Failed to build lib: $curve\n"; - system( "make" ) and abort "Failed to build tests: $curve\n"; - system( "make test" ) and abort "Failed test suite: $curve\n"; + system( "CFLAGS='-Werror -Wall -Wextra' make" ) + and abort "Failed to build: all but $curve\n"; + system( "make test" ) + and abort "Failed test suite: all but $curve\n"; } From 599700561191916265d0c826ad6c047511dbe3d6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 28 Feb 2019 13:12:06 +0100 Subject: [PATCH 12/16] Fix unused variables in Montgomery-only configuration Signed-off-by: Gilles Peskine --- library/ecp.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/ecp.c b/library/ecp.c index f44298ab80..0c901b0ac8 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -879,6 +879,7 @@ int mbedtls_ecp_point_write_binary( const mbedtls_ecp_group *grp, plen = mbedtls_mpi_size( &grp->P ); #if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) + (void) format; /* Montgomery curves always use the same point format */ if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { *olen = plen; @@ -2653,6 +2654,8 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, /* reset ops count for this call if top-level */ if( rs_ctx != NULL && rs_ctx->depth++ == 0 ) rs_ctx->ops_done = 0; +#else + (void) rs_ctx; #endif #if defined(MBEDTLS_ECP_INTERNAL_ALT) From 0478c2f77e6606d01b0547446fade696da1f9a36 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 21:21:28 +0200 Subject: [PATCH 13/16] Add ChangeLog entry for single-curve build fixes Fix #941, #1412, #1147, #2017 Signed-off-by: Gilles Peskine --- ChangeLog.d/build_with_only_montgomery_curves.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/build_with_only_montgomery_curves.txt diff --git a/ChangeLog.d/build_with_only_montgomery_curves.txt b/ChangeLog.d/build_with_only_montgomery_curves.txt new file mode 100644 index 0000000000..d4ec7c56c4 --- /dev/null +++ b/ChangeLog.d/build_with_only_montgomery_curves.txt @@ -0,0 +1,6 @@ +Bugfix + * Fix build errors when the only enabled elliptic curves are Montgomery + curves. Raised by signpainter in #941 and by Taiki-San in #1412. This + also fixes missing declarations reported by Steven Cooreman in #1147. + * Fix self-test failure when the only enabled short Weierstrass elliptic + curve is secp192k1. Fixes #2017. From d3beca9e38637a6b3f395c476789eaa60997fd4d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 3 Jul 2020 00:15:37 +0200 Subject: [PATCH 14/16] Test Everest with only Curve25519 enabled tests/scripts/curves.pl tests the library with a single curve enabled. This uses the legacy ECDH context and the default ECDH implementation. For Curve25519, there is an alternative implementation, which is Everest. Test this. This also tests the new ECDH context, which Everest requires. Signed-off-by: Gilles Peskine --- tests/scripts/all.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index ec61d19621..558016d040 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1001,6 +1001,25 @@ component_test_everest () { if_build_succeeded tests/compat.sh -f ECDH -V NO -e 'ARCFOUR\|ARIA\|CAMELLIA\|CHACHA\|DES\|RC4' } +component_test_everest_curve25519_only () { + msg "build: Everest ECDH context, only Curve25519" # ~ 6 min + scripts/config.py unset MBEDTLS_ECDH_LEGACY_CONTEXT + scripts/config.py set MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED + scripts/config.py unset MBEDTLS_ECDSA_C + scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED + scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED + # Disable all curves + for c in $(sed -n 's/#define \(MBEDTLS_ECP_DP_[0-9A-Z_a-z]*_ENABLED\).*/\1/p' <"$CONFIG_H"); do + scripts/config.py unset "$c" + done + scripts/config.py set MBEDTLS_ECP_DP_CURVE25519_ENABLED + + make CFLAGS="$ASAN_CFLAGS -O2" LDFLAGS="$ASAN_CFLAGS" + + msg "test: Everest ECDH context, only Curve25519" # ~ 50s + make test +} + component_test_small_ssl_out_content_len () { msg "build: small SSL_OUT_CONTENT_LEN (ASan build)" scripts/config.py set MBEDTLS_SSL_IN_CONTENT_LEN 16384 From a3de08d0b53dbaee2aff957c630ceab7db83e650 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jul 2020 01:23:37 +0200 Subject: [PATCH 15/16] Reorder curve enumeration like mbedtls_ecp_group_id No semantic change. Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 7fe7473022..b317d70213 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -68,12 +68,12 @@ defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) || \ defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) #define MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED #endif #if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ From 6d9c8d7b2db393b9b51d92577c077eeaff13fb73 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jul 2020 01:26:25 +0200 Subject: [PATCH 16/16] Minor documentation improvements Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 2 +- library/ecp.c | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index b317d70213..980ec5e66e 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -97,7 +97,7 @@ extern "C" { /* Note: when adding a new curve: * - Add it at the end of this enum, otherwise you'll break the ABI by * changing the numerical value for existing curves. - * - Increment MBEDTLS_ECP_DP_MAX below. + * - Increment MBEDTLS_ECP_DP_MAX below if needed. * - Add the corresponding MBEDTLS_ECP_DP_xxx_ENABLED macro definition to * config.h. * - List the curve as a dependency of MBEDTLS_ECP_C and diff --git a/library/ecp.c b/library/ecp.c index 0c901b0ac8..d6ef5edc4c 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3295,6 +3295,9 @@ cleanup: #if defined(MBEDTLS_SELF_TEST) +/* Adjust the exponent to be a valid private point for the specified curve. + * This is sometimes necessary because we use a single set of exponents + * for all curves but the validity of values depends on the curve. */ static int self_test_adjust_exponent( const mbedtls_ecp_group *grp, mbedtls_mpi *m ) { @@ -3328,11 +3331,13 @@ cleanup: return( ret ); } +/* Calculate R = m.P for each m in exponents. Check that the number of + * basic operations doesn't depend on the value of m. */ static int self_test_point( int verbose, mbedtls_ecp_group *grp, mbedtls_ecp_point *R, mbedtls_mpi *m, - mbedtls_ecp_point *P, + const mbedtls_ecp_point *P, const char *const *exponents, size_t n_exponents ) { @@ -3407,6 +3412,9 @@ int mbedtls_ecp_self_test( int verbose ) #if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) const char *m_exponents[] = { + /* Valid private values for Curve25519. In a build with Curve448 + * but not Curve25519, they will be adjusted in + * self_test_adjust_exponent(). */ "4000000000000000000000000000000000000000000000000000000000000000", "5C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C30", "5715ECCE24583F7A7023C24164390586842E816D7280A49EF6DF4EAE6B280BF8",