From cce0601485dfe1330635d821e2161131b6ae3509 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 23 Mar 2021 21:58:14 +0100 Subject: [PATCH 01/48] mbedtls_ecp_gen_privkey: minor refactoring Prepare to isolate the Montgomery and short Weierstrass implementations of mbedtls_ecp_gen_privkey into their own function. Signed-off-by: Gilles Peskine --- library/ecp.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 1ad5b6748..858dd5ed8 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3066,36 +3066,39 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, void *p_rng ) { int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; - size_t n_size; + size_t n_bits; + const mbedtls_mpi *N = NULL; ECP_VALIDATE_RET( grp != NULL ); ECP_VALIDATE_RET( d != NULL ); ECP_VALIDATE_RET( f_rng != NULL ); - n_size = ( grp->nbits + 7 ) / 8; + N = &grp->N; + n_bits = grp->nbits; #if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { - /* [M225] page 5 */ size_t b; + size_t n_bytes = ( n_bits + 7 ) / 8; + /* [Curve25519] page 5 */ do { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_size, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); } while( mbedtls_mpi_bitlen( d ) == 0); - /* Make sure the most significant bit is nbits */ + /* Make sure the most significant bit is n_bits */ b = mbedtls_mpi_bitlen( d ) - 1; /* mbedtls_mpi_bitlen is one-based */ - if( b > grp->nbits ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - grp->nbits ) ); + if( b > n_bits ) + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - n_bits ) ); else - MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, grp->nbits, 1 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, n_bits, 1 ) ); /* Make sure the last two bits are unset for Curve448, three bits for Curve25519 */ MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 0, 0 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 1, 0 ) ); - if( grp->nbits == 254 ) + if( n_bits == 254 ) { MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 2, 0 ) ); } @@ -3108,18 +3111,20 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, /* SEC1 3.2.1: Generate d such that 1 <= n < N */ int count = 0; unsigned cmp = 0; + size_t n_bytes = ( n_bits + 7 ) / 8; /* - * Match the procedure given in RFC 6979 (deterministic ECDSA): + * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) + * when f_rng is a suitably parametrized instance of HMAC_DRBG: * - use the same byte ordering; - * - keep the leftmost nbits bits of the generated octet string; + * - keep the leftmost n_bits bits of the generated octet string; * - try until result is in the desired range. - * This also avoids any biais, which is especially important for ECDSA. + * This also avoids any bias, which is especially important for ECDSA. */ do { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_size, f_rng, p_rng ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_size - grp->nbits ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_bytes - n_bits ) ); /* * Each try has at worst a probability 1/2 of failing (the msb has @@ -3136,7 +3141,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, goto cleanup; } - ret = mbedtls_mpi_lt_mpi_ct( d, &grp->N, &cmp ); + ret = mbedtls_mpi_lt_mpi_ct( d, N, &cmp ); if( ret != 0 ) { goto cleanup; From 72fcc98d2361330411b8c6ccce346d5cc3c9e962 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 23 Mar 2021 22:31:31 +0100 Subject: [PATCH 02/48] mbedtls_ecp_gen_privkey: create subfunctions for each curve type Put the Montgomery and short Weierstrass implementations of mbedtls_ecp_gen_privkey into their own function which can be tested independently, but will not be part of the public ABI/API. Signed-off-by: Gilles Peskine --- library/ecp.c | 171 +++++++++++++++++++++++------------------ library/ecp_invasive.h | 52 +++++++++++++ 2 files changed, 147 insertions(+), 76 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 858dd5ed8..506f0cbaf 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3057,6 +3057,97 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); } +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) +MBEDTLS_STATIC_TESTABLE +int mbedtls_ecp_gen_privkey_mx( size_t n_bits, + mbedtls_mpi *d, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + size_t b; + size_t n_bytes = ( n_bits + 7 ) / 8; + + /* [Curve25519] page 5 */ + do { + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); + } while( mbedtls_mpi_bitlen( d ) == 0); + + /* Make sure the most significant bit is n_bits */ + b = mbedtls_mpi_bitlen( d ) - 1; /* mbedtls_mpi_bitlen is one-based */ + if( b > n_bits ) + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - n_bits ) ); + else + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, n_bits, 1 ) ); + + /* Make sure the last two bits are unset for Curve448, three bits for + Curve25519 */ + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 0, 0 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 1, 0 ) ); + if( n_bits == 254 ) + { + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 2, 0 ) ); + } + +cleanup: + return( ret ); +} +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ + +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) +MBEDTLS_STATIC_TESTABLE +int mbedtls_ecp_gen_privkey_sw( const mbedtls_mpi *N, size_t n_bits, + mbedtls_mpi *d, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + /* SEC1 3.2.1: Generate d such that 1 <= n < N */ + int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + int count = 0; + unsigned cmp = 0; + size_t n_bytes = ( n_bits + 7 ) / 8; + + /* + * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) + * when f_rng is a suitably parametrized instance of HMAC_DRBG: + * - use the same byte ordering; + * - keep the leftmost n_bits bits of the generated octet string; + * - try until result is in the desired range. + * This also avoids any bias, which is especially important for ECDSA. + */ + do + { + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_bytes - n_bits ) ); + + /* + * Each try has at worst a probability 1/2 of failing (the msb has + * a probability 1/2 of being 0, and then the result will be < N), + * so after 30 tries failure probability is a most 2**(-30). + * + * For most curves, 1 try is enough with overwhelming probability, + * since N starts with a lot of 1s in binary, but some curves + * such as secp224k1 are actually very close to the worst case. + */ + if( ++count > 30 ) + { + ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; + goto cleanup; + } + + ret = mbedtls_mpi_lt_mpi_ct( d, N, &cmp ); + if( ret != 0 ) + { + goto cleanup; + } + } + while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp != 1 ); + +cleanup: + return( ret ); +} +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ + /* * Generate a private key */ @@ -3065,94 +3156,22 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; - size_t n_bits; - const mbedtls_mpi *N = NULL; - ECP_VALIDATE_RET( grp != NULL ); ECP_VALIDATE_RET( d != NULL ); ECP_VALIDATE_RET( f_rng != NULL ); - N = &grp->N; - n_bits = grp->nbits; - #if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) - { - size_t b; - size_t n_bytes = ( n_bits + 7 ) / 8; - - /* [Curve25519] page 5 */ - do { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); - } while( mbedtls_mpi_bitlen( d ) == 0); - - /* Make sure the most significant bit is n_bits */ - b = mbedtls_mpi_bitlen( d ) - 1; /* mbedtls_mpi_bitlen is one-based */ - if( b > n_bits ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - n_bits ) ); - else - MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, n_bits, 1 ) ); - - /* Make sure the last two bits are unset for Curve448, three bits for - Curve25519 */ - MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 0, 0 ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 1, 0 ) ); - if( n_bits == 254 ) - { - MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 2, 0 ) ); - } - } + return( mbedtls_ecp_gen_privkey_mx( grp->nbits, d, f_rng, p_rng ) ); #endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ #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 */ - int count = 0; - unsigned cmp = 0; - size_t n_bytes = ( n_bits + 7 ) / 8; - - /* - * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) - * when f_rng is a suitably parametrized instance of HMAC_DRBG: - * - use the same byte ordering; - * - keep the leftmost n_bits bits of the generated octet string; - * - try until result is in the desired range. - * This also avoids any bias, which is especially important for ECDSA. - */ - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_bytes - n_bits ) ); - - /* - * Each try has at worst a probability 1/2 of failing (the msb has - * a probability 1/2 of being 0, and then the result will be < N), - * so after 30 tries failure probability is a most 2**(-30). - * - * For most curves, 1 try is enough with overwhelming probability, - * since N starts with a lot of 1s in binary, but some curves - * such as secp224k1 are actually very close to the worst case. - */ - if( ++count > 30 ) - { - ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; - goto cleanup; - } - - ret = mbedtls_mpi_lt_mpi_ct( d, N, &cmp ); - if( ret != 0 ) - { - goto cleanup; - } - } - while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp != 1 ); - } + return( mbedtls_ecp_gen_privkey_sw( &grp->N, grp->nbits, d, + f_rng, p_rng ) ); #endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -cleanup: - return( ret ); + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); } /* diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index b5239676f..2895b19e1 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -27,6 +27,7 @@ #define MBEDTLS_ECP_INVASIVE_H #include "common.h" +#include "mbedtls/bignum.h" #include "mbedtls/ecp.h" #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_ECP_C) @@ -46,6 +47,57 @@ void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ); #endif +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) +/** Generate a private key on a Montgomery curve (Curve25519 or Curve448). + * + * This function implements key generation for the set of secret keys + * specified in [Curve25519] p. 5 and in [Curve448]. The resulting value + * has the lower bits masked but is not necessarily canonical. + * + * \note - [Curve25519] http://cr.yp.to/ecdh/curve25519-20060209.pdf + * - [RFC7748] https://tools.ietf.org/html/rfc7748 + * + * \p n_bits The position of the high-order bit of the key to generate. + * This is the bit-size of the key minus 1: + * 254 for Curve25519 or 447 for Curve448. + * \param d The randomly generated key. This is a number of size + * exactly \p n_bits + 1 bits, with the least significant bits + * masked as specified in [Curve25519] and in [RFC7748] §5. + * \param f_rng The RNG function. + * \param p_rng The RNG context to be passed to \p f_rng. + * + * \return \c 0 on success. + * \return \c MBEDTLS_ERR_ECP_xxx or MBEDTLS_ERR_MPI_xxx on failure. + */ +int mbedtls_ecp_gen_privkey_mx( size_t n_bits, + mbedtls_mpi *d, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ); + +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ + +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) +/** Generate a private key on a short Weierstrass curve. + * + * The procedure complies with RFC 6979 §3.3 (deterministic ECDSA) + * when the RNG is a suitably parametrized instance of HMAC_DRBG. + * + * \p N The upper bound of the range. + * \p n_bits The size of \p N in bits. This value must be correct, + * otherwise the result is unpredictable. + * \param d A random number, uniformly generated in the range [1, N-1]. + * \param f_rng The RNG function. + * \param p_rng The RNG context to be passed to \p f_rng. + * + * \return \c 0 on success. + * \return \c MBEDTLS_ERR_ECP_xxx or MBEDTLS_ERR_MPI_xxx on failure. + */ +int mbedtls_ecp_gen_privkey_sw( const mbedtls_mpi *N, size_t n_bits, + mbedtls_mpi *d, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ); +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ + #endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_ECP_C */ #endif /* MBEDTLS_ECP_INVASIVE_H */ From 55c46040f6e7b1f0b9bd63f5dbc832a6c96a3451 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Mar 2021 12:34:40 +0100 Subject: [PATCH 03/48] mbedtls_ecp_gen_privkey_mx: rename n_bits to high_bit For Montgomery keys, n_bits is actually the position of the highest bit and not the number of bits, which would be 1 more (fence vs posts). Rename the variable accordingly to lessen the confusion. No semantic change. Signed-off-by: Gilles Peskine --- library/ecp.c | 16 ++++++++-------- library/ecp_invasive.h | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 506f0cbaf..584e0242c 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3059,32 +3059,32 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, #if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) MBEDTLS_STATIC_TESTABLE -int mbedtls_ecp_gen_privkey_mx( size_t n_bits, +int mbedtls_ecp_gen_privkey_mx( size_t high_bit, mbedtls_mpi *d, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; size_t b; - size_t n_bytes = ( n_bits + 7 ) / 8; + size_t n_bytes = ( high_bit + 7 ) / 8; /* [Curve25519] page 5 */ do { MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); } while( mbedtls_mpi_bitlen( d ) == 0); - /* Make sure the most significant bit is n_bits */ - b = mbedtls_mpi_bitlen( d ) - 1; /* mbedtls_mpi_bitlen is one-based */ - if( b > n_bits ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - n_bits ) ); + /* Make sure the most significant bit is high_bit */ + b = mbedtls_mpi_bitlen( d ) - 1; /* position of the highest bit in d */ + if( b > high_bit ) + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - high_bit ) ); else - MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, n_bits, 1 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, high_bit, 1 ) ); /* Make sure the last two bits are unset for Curve448, three bits for Curve25519 */ MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 0, 0 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 1, 0 ) ); - if( n_bits == 254 ) + if( high_bit == 254 ) { MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 2, 0 ) ); } diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index 2895b19e1..eeb430511 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -57,7 +57,7 @@ void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ); * \note - [Curve25519] http://cr.yp.to/ecdh/curve25519-20060209.pdf * - [RFC7748] https://tools.ietf.org/html/rfc7748 * - * \p n_bits The position of the high-order bit of the key to generate. + * \p high_bit The position of the high-order bit of the key to generate. * This is the bit-size of the key minus 1: * 254 for Curve25519 or 447 for Curve448. * \param d The randomly generated key. This is a number of size From 0b1b0abe33d6c9717176e7a8254a2d16ad418d5d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Mar 2021 00:14:53 +0100 Subject: [PATCH 04/48] Update references in some test function documentation Signed-off-by: Gilles Peskine --- tests/include/test/random.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/include/test/random.h b/tests/include/test/random.h index 5e7e4e6ef..22b9b5c9f 100644 --- a/tests/include/test/random.h +++ b/tests/include/test/random.h @@ -67,24 +67,24 @@ int mbedtls_test_rnd_std_rand( void *rng_state, size_t len ); /** - * This function only returns zeros + * This function only returns zeros. * - * rng_state shall be NULL. + * \p rng_state shall be \c NULL. */ int mbedtls_test_rnd_zero_rand( void *rng_state, unsigned char *output, size_t len ); /** - * This function returns random based on a buffer it receives. + * This function returns random data based on a buffer it receives. * - * rng_state shall be a pointer to a rnd_buf_info structure. + * \p rng_state shall be a pointer to a #mbedtls_test_rnd_buf_info structure. * * The number of bytes released from the buffer on each call to * the random function is specified by per_call. (Can be between * 1 and 4) * - * After the buffer is empty it will return rand(); + * After the buffer is empty it will return mbedtls_test_rnd_std_rand(). */ int mbedtls_test_rnd_buffer_rand( void *rng_state, unsigned char *output, @@ -96,7 +96,7 @@ int mbedtls_test_rnd_buffer_rand( void *rng_state, * Pseudo random is based on the XTEA encryption algorithm to * generate pseudorandom. * - * rng_state shall be a pointer to a rnd_pseudo_info structure. + * \p rng_state shall be a pointer to a #mbedtls_test_rnd_pseudo_info structure. */ int mbedtls_test_rnd_pseudo_rand( void *rng_state, unsigned char *output, From ecacc3c9d21b1e0dc264777aa69969fbefc6aa7f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Mar 2021 00:48:57 +0100 Subject: [PATCH 05/48] Make the fallback behavior of mbedtls_test_rnd_buffer_rand optional If a fallback is not explicitly configured in the mbedtls_test_rnd_buf_info structure, fail after the buffer is exhausted. There is no intended behavior change in this commit: all existing uses of mbedtls_test_rnd_buffer_rand() have been updated to set mbedtls_test_rnd_std_rand as the fallback. Signed-off-by: Gilles Peskine --- tests/include/test/random.h | 9 +++++++-- tests/src/random.c | 14 ++++++++++++-- tests/suites/test_suite_ecdh.function | 8 ++++++++ tests/suites/test_suite_ecdsa.function | 2 ++ tests/suites/test_suite_pkcs1_v15.function | 4 ++++ tests/suites/test_suite_pkcs1_v21.function | 4 ++++ 6 files changed, 37 insertions(+), 4 deletions(-) diff --git a/tests/include/test/random.h b/tests/include/test/random.h index 22b9b5c9f..b64a072b3 100644 --- a/tests/include/test/random.h +++ b/tests/include/test/random.h @@ -36,8 +36,11 @@ typedef struct { - unsigned char *buf; + unsigned char *buf; /* Pointer to a buffer of length bytes. */ size_t length; + /* If fallback_f_rng is NULL, fail after delivering length bytes. */ + int ( *fallback_f_rng )( void*, unsigned char *, size_t ); + void *fallback_p_rng; } mbedtls_test_rnd_buf_info; /** @@ -84,7 +87,9 @@ int mbedtls_test_rnd_zero_rand( void *rng_state, * the random function is specified by per_call. (Can be between * 1 and 4) * - * After the buffer is empty it will return mbedtls_test_rnd_std_rand(). + * After the buffer is empty, this function will call the fallback RNG in the + * #mbedtls_test_rnd_buf_info structure if there is one, and + * will return #MBEDTLS_ERR_ENTROPY_SOURCE_FAILED otherwise. */ int mbedtls_test_rnd_buffer_rand( void *rng_state, unsigned char *output, diff --git a/tests/src/random.c b/tests/src/random.c index e01bd4d2d..7f3f40166 100644 --- a/tests/src/random.c +++ b/tests/src/random.c @@ -35,6 +35,8 @@ #include #include +#include + int mbedtls_test_rnd_std_rand( void *rng_state, unsigned char *output, size_t len ) @@ -91,8 +93,16 @@ int mbedtls_test_rnd_buffer_rand( void *rng_state, } if( len - use_len > 0 ) - return( mbedtls_test_rnd_std_rand( NULL, output + use_len, - len - use_len ) ); + { + if( info->fallback_f_rng != NULL ) + { + return( info->fallback_f_rng( info->fallback_p_rng, + output + use_len, + len - use_len ) ); + } + else + return( MBEDTLS_ERR_ENTROPY_SOURCE_FAILED ); + } return( 0 ); } diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 3ab96fa11..da0531b45 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -240,6 +240,8 @@ void ecdh_primitive_testvec( int id, data_t * rnd_buf_A, char * xA_str, rnd_info_A.buf = rnd_buf_A->x; rnd_info_A.length = rnd_buf_A->len; + rnd_info_A.fallback_f_rng = mbedtls_test_rnd_std_rand; + rnd_info_A.fallback_p_rng = NULL; /* Fix rnd_buf_A->x by shifting it left if necessary */ if( grp.nbits % 8 != 0 ) @@ -256,6 +258,8 @@ void ecdh_primitive_testvec( int id, data_t * rnd_buf_A, char * xA_str, rnd_info_B.buf = rnd_buf_B->x; rnd_info_B.length = rnd_buf_B->len; + rnd_info_B.fallback_f_rng = mbedtls_test_rnd_std_rand; + rnd_info_B.fallback_p_rng = NULL; /* Fix rnd_buf_B->x by shifting it left if necessary */ if( grp.nbits % 8 != 0 ) @@ -362,9 +366,13 @@ void ecdh_restart( int id, data_t *dA, data_t *dB, data_t *z, mbedtls_ecdh_init( &srv ); mbedtls_ecdh_init( &cli ); + rnd_info_A.fallback_f_rng = mbedtls_test_rnd_std_rand; + rnd_info_A.fallback_p_rng = NULL; rnd_info_A.buf = dA->x; rnd_info_A.length = dA->len; + rnd_info_B.fallback_f_rng = mbedtls_test_rnd_std_rand; + rnd_info_B.fallback_p_rng = NULL; rnd_info_B.buf = dB->x; rnd_info_B.length = dB->len; diff --git a/tests/suites/test_suite_ecdsa.function b/tests/suites/test_suite_ecdsa.function index 58cedc13c..2cb892591 100644 --- a/tests/suites/test_suite_ecdsa.function +++ b/tests/suites/test_suite_ecdsa.function @@ -292,6 +292,8 @@ void ecdsa_prim_test_vectors( int id, char * d_str, char * xQ_str, TEST_ASSERT( mbedtls_mpi_read_string( &d, 16, d_str ) == 0 ); TEST_ASSERT( mbedtls_mpi_read_string( &r_check, 16, r_str ) == 0 ); TEST_ASSERT( mbedtls_mpi_read_string( &s_check, 16, s_str ) == 0 ); + rnd_info.fallback_f_rng = mbedtls_test_rnd_std_rand; + rnd_info.fallback_p_rng = NULL; rnd_info.buf = rnd_buf->x; rnd_info.length = rnd_buf->len; diff --git a/tests/suites/test_suite_pkcs1_v15.function b/tests/suites/test_suite_pkcs1_v15.function index d1c0fc129..a7fb2a5ff 100644 --- a/tests/suites/test_suite_pkcs1_v15.function +++ b/tests/suites/test_suite_pkcs1_v15.function @@ -19,6 +19,8 @@ void pkcs1_rsaes_v15_encrypt( int mod, int radix_N, char * input_N, mbedtls_test_rnd_buf_info info; mbedtls_mpi N, E; + info.fallback_f_rng = mbedtls_test_rnd_std_rand; + info.fallback_p_rng = NULL; info.buf = rnd_buf->x; info.length = rnd_buf->len; @@ -268,6 +270,8 @@ void pkcs1_rsassa_v15_sign( int mod, int radix_P, char * input_P, int radix_Q, mbedtls_mpi N, P, Q, E; mbedtls_test_rnd_buf_info info; + info.fallback_f_rng = mbedtls_test_rnd_std_rand; + info.fallback_p_rng = NULL; info.buf = rnd_buf->x; info.length = rnd_buf->len; diff --git a/tests/suites/test_suite_pkcs1_v21.function b/tests/suites/test_suite_pkcs1_v21.function index 8f22f2094..f7e1e24ac 100644 --- a/tests/suites/test_suite_pkcs1_v21.function +++ b/tests/suites/test_suite_pkcs1_v21.function @@ -18,6 +18,8 @@ void pkcs1_rsaes_oaep_encrypt( int mod, data_t * input_N, data_t * input_E, mbedtls_test_rnd_buf_info info; mbedtls_mpi N, E; + info.fallback_f_rng = mbedtls_test_rnd_std_rand; + info.fallback_p_rng = NULL; info.buf = rnd_buf->x; info.length = rnd_buf->len; @@ -122,6 +124,8 @@ void pkcs1_rsassa_pss_sign( int mod, data_t * input_P, data_t * input_Q, mbedtls_test_rnd_buf_info info; mbedtls_mpi N, P, Q, E; + info.fallback_f_rng = mbedtls_test_rnd_std_rand; + info.fallback_p_rng = NULL; info.buf = rnd_buf->x; info.length = rnd_buf->len; From 6ff8a01a57f27f1795aa36e23fc0c24a11b70450 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Mar 2021 12:01:02 +0100 Subject: [PATCH 06/48] Add unit tests for mbedtls_ecp_gen_privkey_mx Test the exact output from known RNG input. This is overly constraining, but ensures that the code has good properties. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ecp.data | 42 ++++++++++++++++++++++++ tests/suites/test_suite_ecp.function | 49 ++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index 106791cb8..f66522a62 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -276,6 +276,48 @@ ECP gen keypair wrapper depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED mbedtls_ecp_gen_key:MBEDTLS_ECP_DP_SECP192R1 +ECP generate Montgomery key: Curve25519, random in range +genkey_mx_known_answer:254:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" + +ECP generate Montgomery key: Curve25519, set high bit +genkey_mx_known_answer:254:"0f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" + +ECP generate Montgomery key: Curve25519, clear higher bit +## If the bit 255 is set, the library shifts the random number right. +genkey_mx_known_answer:254:"ff0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8":"7f808101820283038404850586068707880889098a0a8b0b8c0c8d0d8e0e8f78" + +ECP generate Montgomery key: Curve25519, clear low bits +genkey_mx_known_answer:254:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1eff":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" + +# ECP generate Montgomery key: Curve25519, random = all-bits-zero +## Currently explicitly rejected in the library, but the specification +## says it shouldn't be. +# genkey_mx_known_answer:254:"0000000000000000000000000000000000000000000000000000000000000000":"4000000000000000000000000000000000000000000000000000000000000000" + +ECP generate Montgomery key: Curve25519, random = all-bits-one +genkey_mx_known_answer:254:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":"7ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff8" + +ECP generate Montgomery key: Curve25519, not enough entropy +genkey_mx_known_answer:254:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e":"" + +ECP generate Montgomery key: Curve448, random in range +genkey_mx_known_answer:447:"cf0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536fc":"cf0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536fc" + +ECP generate Montgomery key: Curve448, set high bit +genkey_mx_known_answer:447:"0f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536fc":"8f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536fc" + +ECP generate Montgomery key: Curve448, clear low bits +genkey_mx_known_answer:447:"cf0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536ff":"cf0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536fc" + +# ECP generate Montgomery key: Curve448, random = all-bits-zero +# genkey_mx_known_answer:447:"0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000":"8000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + +ECP generate Montgomery key: Curve448, random = all-bits-one +genkey_mx_known_answer:447:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":"fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffc" + +ECP generate Montgomery key: Curve448, not enough entropy +genkey_mx_known_answer:447:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536":"" + ECP read key #1 (short weierstrass, too small) depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED mbedtls_ecp_read_key:MBEDTLS_ECP_DP_SECP192R1:"00":MBEDTLS_ERR_ECP_INVALID_KEY:0 diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 6d23377f3..1492b9531 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -1237,6 +1237,55 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS:MBEDTLS_ECP_MONTGOMERY_ENABLED */ +void genkey_mx_known_answer( int bits, data_t *seed, data_t *expected ) +{ + mbedtls_test_rnd_buf_info rnd_info; + mbedtls_mpi d; + int ret; + uint8_t *actual = NULL; + + mbedtls_mpi_init( &d ); + rnd_info.buf = seed->x; + rnd_info.length = seed->len; + rnd_info.fallback_f_rng = NULL; + rnd_info.fallback_p_rng = NULL; + + ASSERT_ALLOC( actual, expected->len ); + + ret = mbedtls_ecp_gen_privkey_mx( bits, &d, + mbedtls_test_rnd_buffer_rand, &rnd_info ); + + if( expected->len == 0 ) + { + /* Expecting an error (happens if there isn't enough randomness) */ + TEST_ASSERT( ret != 0 ); + } + else + { + TEST_EQUAL( ret, 0 ); + TEST_EQUAL( (size_t) bits + 1, mbedtls_mpi_bitlen( &d ) ); + TEST_EQUAL( 0, mbedtls_mpi_write_binary( &d, actual, expected->len ) ); + /* Test the exact result. This assumes that the output of the + * RNG is used in a specific way, which is overly constraining. + * The advantage is that it's easier to test the expected properties + * of the generated key: + * - The most significant bit must be at a specific positions + * (can be enforced by checking the bit-length). + * - The least significant bits must have specific values + * (can be enforced by checking these bits). + * - Other bits must be random (by testing with different RNG outputs, + * we validate that those bits are indeed influenced by the RNG). */ + ASSERT_COMPARE( expected->x, expected->len, + actual, expected->len ); + } + +exit: + mbedtls_free( actual ); + mbedtls_mpi_free( &d ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void ecp_selftest( ) { From 96449ceebeb3024123ff41bc88a6f275bb602eac Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Mar 2021 12:04:43 +0100 Subject: [PATCH 07/48] mbedtls_ecp_gen_privkey_mx: remove the exception for all-zero The library rejected an RNG input of all-bits-zero, which led to the key 2^{254} (for Curve25519) having a 31/32 chance of being generated compared to other keys. This had no practical impact because the probability of non-compliance was 2^{-256}, but needlessly complicated the code. The exception was added in 98e28a74e33f32bcb855e16f8d5d2016b2102129 to avoid the case where b - 1 wraps because b is 0. Instead, change the comparison code to avoid calculating b - 1. Signed-off-by: Gilles Peskine --- library/ecp.c | 10 ++++------ tests/suites/test_suite_ecp.data | 10 ++++------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 584e0242c..1592e87de 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3069,14 +3069,12 @@ int mbedtls_ecp_gen_privkey_mx( size_t high_bit, size_t n_bytes = ( high_bit + 7 ) / 8; /* [Curve25519] page 5 */ - do { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); - } while( mbedtls_mpi_bitlen( d ) == 0); + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); /* Make sure the most significant bit is high_bit */ - b = mbedtls_mpi_bitlen( d ) - 1; /* position of the highest bit in d */ - if( b > high_bit ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - high_bit ) ); + b = mbedtls_mpi_bitlen( d ); /* mbedtls_mpi_bitlen is one-based */ + if( b > high_bit + 1 ) + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - 1 - high_bit ) ); else MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, high_bit, 1 ) ); diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index f66522a62..21a71922e 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -289,10 +289,8 @@ genkey_mx_known_answer:254:"ff0102030405060708090a0b0c0d0e0f10111213141516171819 ECP generate Montgomery key: Curve25519, clear low bits genkey_mx_known_answer:254:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1eff":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" -# ECP generate Montgomery key: Curve25519, random = all-bits-zero -## Currently explicitly rejected in the library, but the specification -## says it shouldn't be. -# genkey_mx_known_answer:254:"0000000000000000000000000000000000000000000000000000000000000000":"4000000000000000000000000000000000000000000000000000000000000000" +ECP generate Montgomery key: Curve25519, random = all-bits-zero +genkey_mx_known_answer:254:"0000000000000000000000000000000000000000000000000000000000000000":"4000000000000000000000000000000000000000000000000000000000000000" ECP generate Montgomery key: Curve25519, random = all-bits-one genkey_mx_known_answer:254:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":"7ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff8" @@ -309,8 +307,8 @@ genkey_mx_known_answer:447:"0f0102030405060708090a0b0c0d0e0f10111213141516171819 ECP generate Montgomery key: Curve448, clear low bits genkey_mx_known_answer:447:"cf0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536ff":"cf0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536fc" -# ECP generate Montgomery key: Curve448, random = all-bits-zero -# genkey_mx_known_answer:447:"0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000":"8000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" +ECP generate Montgomery key: Curve448, random = all-bits-zero +genkey_mx_known_answer:447:"0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000":"8000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" ECP generate Montgomery key: Curve448, random = all-bits-one genkey_mx_known_answer:447:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":"fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffc" From 67986d06135c2edcb911123461ee84b2ee639347 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Mar 2021 12:25:59 +0100 Subject: [PATCH 08/48] mbedtls_ecp_gen_privkey_mx: make bit manipulations unconditional Don't calculate the bit-size of the initially generated random number. This is not necessary to reach the desired distribution of private keys, and creates a (tiny) side channel opportunity. This changes the way the result is derived from the random number, but does not affect the resulting distribution. Signed-off-by: Gilles Peskine --- library/ecp.c | 10 +++------- tests/suites/test_suite_ecp.data | 8 ++------ 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 1592e87de..81ba93306 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3065,18 +3065,14 @@ int mbedtls_ecp_gen_privkey_mx( size_t high_bit, void *p_rng ) { int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; - size_t b; size_t n_bytes = ( high_bit + 7 ) / 8; /* [Curve25519] page 5 */ MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); - /* Make sure the most significant bit is high_bit */ - b = mbedtls_mpi_bitlen( d ); /* mbedtls_mpi_bitlen is one-based */ - if( b > high_bit + 1 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, b - 1 - high_bit ) ); - else - MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, high_bit, 1 ) ); + /* Make sure the most significant bit is exactly at high_bit */ + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_bytes - high_bit - 1 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, high_bit, 1 ) ); /* Make sure the last two bits are unset for Curve448, three bits for Curve25519 */ diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index 21a71922e..5f92ca459 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -277,17 +277,13 @@ depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED mbedtls_ecp_gen_key:MBEDTLS_ECP_DP_SECP192R1 ECP generate Montgomery key: Curve25519, random in range -genkey_mx_known_answer:254:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" - -ECP generate Montgomery key: Curve25519, set high bit -genkey_mx_known_answer:254:"0f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" +genkey_mx_known_answer:254:"9e020406080a0c0e10121416181a1c1e20222426282a2c2e30323436383a3df0":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" ECP generate Montgomery key: Curve25519, clear higher bit -## If the bit 255 is set, the library shifts the random number right. genkey_mx_known_answer:254:"ff0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8":"7f808101820283038404850586068707880889098a0a8b0b8c0c8d0d8e0e8f78" ECP generate Montgomery key: Curve25519, clear low bits -genkey_mx_known_answer:254:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1eff":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" +genkey_mx_known_answer:254:"9e020406080a0c0e10121416181a1c1e20222426282a2c2e30323436383a3dff":"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1ef8" ECP generate Montgomery key: Curve25519, random = all-bits-zero genkey_mx_known_answer:254:"0000000000000000000000000000000000000000000000000000000000000000":"4000000000000000000000000000000000000000000000000000000000000000" From 61f1f5febf119f8661ec21acfbe70f2279c484f1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Mar 2021 12:46:46 +0100 Subject: [PATCH 09/48] mbedtls_ecp_gen_privkey_mx: simplify the size calculation logic mbedtls_ecp_gen_privkey_mx generates a random number with a certain top bit set. Depending on the size, it would either generate a number with that top bit being random, then forcibly set the top bit to 1 (when high_bit is not a multiple of 8); or generate a number with that top bit being 0, then set the top bit to 1 (when high_bit is a multiple of 8). Change it to always generate the top bit randomly first. This doesn't make any difference in practice: the probability distribution is the same either way, and no supported or plausible curve has a size of the form 8n+1 anyway. But it slightly simplifies reasoning about the behavior of this function. Signed-off-by: Gilles Peskine --- library/ecp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 81ba93306..cb7c2ae1c 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3065,13 +3065,16 @@ int mbedtls_ecp_gen_privkey_mx( size_t high_bit, void *p_rng ) { int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; - size_t n_bytes = ( high_bit + 7 ) / 8; + size_t n_random_bytes = high_bit / 8 + 1; /* [Curve25519] page 5 */ - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); + /* Generate a (high_bit+1)-bit random number by generating just enough + * random bytes, then shifting out extra bits from the top (necessary + * when (high_bit+1) is not a multiple of 8). */ + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_random_bytes, + f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_random_bytes - high_bit - 1 ) ); - /* Make sure the most significant bit is exactly at high_bit */ - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_bytes - high_bit - 1 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, high_bit, 1 ) ); /* Make sure the last two bits are unset for Curve448, three bits for From 7888073147d69ea70febf51bae55b06c181fd977 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 29 Mar 2021 21:32:16 +0200 Subject: [PATCH 10/48] mbedtls_ecp_gen_privkey_sw: range and coverage tests Add unit tests for private key generation on short Weierstrass curves. These tests validate that the result is within the desired range. Additionally, they validate that after performing many iterations, the range is covered to an acceptable extent: for tiny ranges, all values must be reached; for larger ranges, all value bits must reach both 0 and 1. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ecp.data | 90 +++++++++++++++++ tests/suites/test_suite_ecp.function | 145 +++++++++++++++++++++++++++ 2 files changed, 235 insertions(+) diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index 5f92ca459..0c344561b 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -312,6 +312,96 @@ genkey_mx_known_answer:447:"ffffffffffffffffffffffffffffffffffffffffffffffffffff ECP generate Montgomery key: Curve448, not enough entropy genkey_mx_known_answer:447:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536":"" +ECP generate in range: 4 +genkey_sw_many:"04":1000 + +ECP generate in range: 5 +genkey_sw_many:"05":1000 + +ECP generate in range: 6 +genkey_sw_many:"06":1000 + +ECP generate in range: 7 +genkey_sw_many:"07":1000 + +ECP generate in range: 8 +genkey_sw_many:"08":1000 + +ECP generate in range: 9 +genkey_sw_many:"09":1000 + +ECP generate in range: 10 +genkey_sw_many:"0a":1000 + +ECP generate in range: 11 +genkey_sw_many:"0b":1000 + +ECP generate in range: 12 +genkey_sw_many:"0c":1000 + +ECP generate in range: 255 +genkey_sw_many:"ff":100 + +ECP generate in range: 256 +genkey_sw_many:"0100":100 + +ECP generate in range: 257 +genkey_sw_many:"0101":100 + +ECP generate in range: 272 +genkey_sw_many:"0110":100 + +ECP generate in range: 2^64-1 +genkey_sw_many:"ffffffffffffffff":100 + +ECP generate in range: 2^64 +genkey_sw_many:"010000000000000000":100 + +ECP generate in range: 2^64+1 +genkey_sw_many:"010000000000000001":100 + +ECP generate in range: 2^64+2^63 +genkey_sw_many:"018000000000000000":100 + +ECP generate in range: 2^65-1 +genkey_sw_many:"01ffffffffffffffff":100 + +ECP generate in range: 2^65 +genkey_sw_many:"020000000000000000":100 + +ECP generate in range: 2^65+1 +genkey_sw_many:"020000000000000001":100 + +ECP generate in range: 2^65+2^64 +genkey_sw_many:"030000000000000000":100 + +ECP generate in range: 2^66+2^65 +genkey_sw_many:"060000000000000000":100 + +ECP generate in range: 2^71-1 +genkey_sw_many:"7fffffffffffffffff":100 + +ECP generate in range: 2^71 +genkey_sw_many:"800000000000000000":100 + +ECP generate in range: 2^71+1 +genkey_sw_many:"800000000000000001":100 + +ECP generate in range: 2^71+2^63 +genkey_sw_many:"c00000000000000000":100 + +ECP generate in range: 2^72-1 +genkey_sw_many:"ffffffffffffffffff":100 + +ECP generate in range: 2^72 +genkey_sw_many:"01000000000000000000":100 + +ECP generate in range: 2^72+1 +genkey_sw_many:"01000000000000000001":100 + +ECP generate in range: 2^72+2^63 +genkey_sw_many:"01800000000000000000":100 + ECP read key #1 (short weierstrass, too small) depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED mbedtls_ecp_read_key:MBEDTLS_ECP_DP_SECP192R1:"00":MBEDTLS_ERR_ECP_INVALID_KEY:0 diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 1492b9531..1049c96a1 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -15,6 +15,43 @@ #define ECP_PT_RESET( x ) \ mbedtls_ecp_point_free( x ); \ mbedtls_ecp_point_init( x ); + +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) +/* Test whether bytes represents (in big-endian base 256) a number B that + * is "significantly" above a power of 2, which is defined as follows. + * Let n be the integer such that 2^n <= B < 2^{n+1}. B is significantly + * above a power of 2 if (B - 2^n) / 2^n is not negligible. "Negligible" + * is defined as having a negligible chance that if you draw an integer + * in the range [1, B-1] K times, the number will always be less than 2^n, + * where K is the iteration count passed to genkey_sw_many. + */ +static int is_significantly_above_a_power_of_2( data_t *bytes ) +{ + const uint8_t *p = bytes->x; + size_t len = bytes->len; + unsigned x; + while( len > 0 && p[0] == 0 ) + { + ++p; + --len; + } + if( len == 0 ) + return( 0 ); + else if( len == 1 ) + x = p[0]; + else + x = ( p[0] << 8 ) | p[1]; + + if( x <= 4 ) + return( 0 ); + + while( ( x & 0x8000 ) == 0 ) + x <<= 1; + x &= 0x7fff; + return( x >= 0x1000 ); +} +#endif + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -1286,6 +1323,114 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS:MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ +void genkey_sw_many( data_t *bound_bytes, int iterations ) +{ + /* Generate numbers in the range 1..bound-1. Do it iterations times. + * This function assumes that the value of bound is at least 2 and + * that iterations is large enough that a one-in-2^iterations chance + * effectively never occurs. + */ + + mbedtls_mpi bound; + size_t n_bits; + mbedtls_mpi result; + size_t b; + /* If bound is small, stats[b] is the number of times the value b + * has been generated. Otherwise stats[b] is the number of times a + * value with bit b set has been generated. */ + size_t *stats = NULL; + size_t stats_len; + int full_stats; + size_t i; + + mbedtls_mpi_init( &bound ); + mbedtls_mpi_init( &result ); + + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &bound, + bound_bytes->x, bound_bytes->len ) ); + n_bits = mbedtls_mpi_bitlen( &bound ); + /* Consider a bound "small" if it's less than 2^5. This value is chosen + * to be small enough that the probability of missing one value is + * negligible given the number of iterations. It must be less than + * 256 because some of the code below assumes that "small" values + * fit in a byte. */ + if( n_bits <= 5 ) + { + full_stats = 1; + stats_len = bound_bytes->x[bound_bytes->len - 1]; + } + else + { + full_stats = 0; + stats_len = n_bits; + } + ASSERT_ALLOC( stats, stats_len ); + + for( i = 0; i < (size_t) iterations; i++ ) + { + mbedtls_test_set_step( i ); + TEST_EQUAL( 0, mbedtls_ecp_gen_privkey_sw( + &bound, n_bits, &result, + mbedtls_test_rnd_std_rand, NULL ) ); + + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &result, &bound ) < 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &result, 1 ) >= 0 ); + if( full_stats ) + { + uint8_t value; + TEST_EQUAL( 0, mbedtls_mpi_write_binary( &result, &value, 1 ) ); + TEST_ASSERT( value < stats_len ); + ++stats[value]; + } + else + { + for( b = 0; b < n_bits; b++ ) + stats[b] += mbedtls_mpi_get_bit( &result, b ); + } + } + + if( full_stats ) + { + for( b = 1; b < stats_len; b++ ) + { + mbedtls_test_set_step( 1000000 + b ); + /* Assert that each value has been reached at least once. + * This is almost guaranteed if the iteration count is large + * enough. This is a very crude way of checking the distribution. + */ + TEST_ASSERT( stats[b] > 0 ); + } + } + else + { + for( b = 0; b < n_bits; b++ ) + { + mbedtls_test_set_step( 1000000 + b ); + /* Assert that each bit has been set in at least one result and + * clear in at least one result. Provided that iterations is not + * too small, it would be extremely unlikely for this not to be + * the case if the results are uniformly distributed. + * + * As an exception, the top bit may legitimately never be set + * if bound is a power of 2 or only slightly above. + */ + if( b != n_bits - 1 || + is_significantly_above_a_power_of_2( bound_bytes ) ) + { + TEST_ASSERT( stats[b] > 0 ); + } + TEST_ASSERT( stats[b] < (size_t) iterations ); + } + } + +exit: + mbedtls_mpi_free( &bound ); + mbedtls_mpi_free( &result ); + mbedtls_free( stats ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void ecp_selftest( ) { From 8cfffb30b3343d8d31c225e9382ea8d72a10df8c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 29 Mar 2021 21:53:58 +0200 Subject: [PATCH 11/48] mbedtls_ecp_gen_privkey_sw: generalize to mbedtls_mpi_random Rename mbedtls_ecp_gen_privkey_sw to mbedtls_mpi_random since it has no particular connection to elliptic curves beyond the fact that its operation is defined by the deterministic ECDSA specification. This is a generic function that generates a random MPI between 1 inclusive and N exclusive. Slightly generalize the function to accept a different lower bound, which adds a negligible amount of complexity. Signed-off-by: Gilles Peskine --- library/ecp.c | 27 +++--- library/ecp_invasive.h | 39 +++++--- tests/suites/test_suite_ecp.data | 129 ++++++++++++++------------- tests/suites/test_suite_ecp.function | 23 +++-- 4 files changed, 119 insertions(+), 99 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index cb7c2ae1c..98ada8283 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3093,15 +3093,17 @@ cleanup: #if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) MBEDTLS_STATIC_TESTABLE -int mbedtls_ecp_gen_privkey_sw( const mbedtls_mpi *N, size_t n_bits, - mbedtls_mpi *d, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +int mbedtls_mpi_random( mbedtls_mpi *X, + mbedtls_mpi_sint min, + const mbedtls_mpi *N, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) { - /* SEC1 3.2.1: Generate d such that 1 <= n < N */ - int ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + /* SEC1 3.2.1: Generate X such that 1 <= n < N */ + int ret = MBEDTLS_ERR_MPI_BAD_INPUT_DATA; int count = 0; unsigned cmp = 0; + size_t n_bits = mbedtls_mpi_bitlen( N ); size_t n_bytes = ( n_bits + 7 ) / 8; /* @@ -3114,8 +3116,8 @@ int mbedtls_ecp_gen_privkey_sw( const mbedtls_mpi *N, size_t n_bits, */ do { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_bytes, f_rng, p_rng ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_bytes - n_bits ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( X, n_bytes, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( X, 8 * n_bytes - n_bits ) ); /* * Each try has at worst a probability 1/2 of failing (the msb has @@ -3128,17 +3130,17 @@ int mbedtls_ecp_gen_privkey_sw( const mbedtls_mpi *N, size_t n_bits, */ if( ++count > 30 ) { - ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; + ret = MBEDTLS_ERR_MPI_NOT_ACCEPTABLE; goto cleanup; } - ret = mbedtls_mpi_lt_mpi_ct( d, N, &cmp ); + ret = mbedtls_mpi_lt_mpi_ct( X, N, &cmp ); if( ret != 0 ) { goto cleanup; } } - while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp != 1 ); + while( mbedtls_mpi_cmp_int( X, min ) < 0 || cmp != 1 ); cleanup: return( ret ); @@ -3164,8 +3166,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, #if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) - return( mbedtls_ecp_gen_privkey_sw( &grp->N, grp->nbits, d, - f_rng, p_rng ) ); + return( mbedtls_mpi_random( d, 1, &grp->N, f_rng, p_rng ) ); #endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index eeb430511..1ca65fd78 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -77,25 +77,36 @@ int mbedtls_ecp_gen_privkey_mx( size_t n_bits, #endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ #if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) -/** Generate a private key on a short Weierstrass curve. +/** Generate a random number uniformly in a range. + * + * This function generates a random number between \p min inclusive and + * \p N exclusive. * * The procedure complies with RFC 6979 §3.3 (deterministic ECDSA) - * when the RNG is a suitably parametrized instance of HMAC_DRBG. + * when the RNG is a suitably parametrized instance of HMAC_DRBG + * and \p min is \c 1. * - * \p N The upper bound of the range. - * \p n_bits The size of \p N in bits. This value must be correct, - * otherwise the result is unpredictable. - * \param d A random number, uniformly generated in the range [1, N-1]. - * \param f_rng The RNG function. - * \param p_rng The RNG context to be passed to \p f_rng. + * \note There are `N - min` possible outputs. The lower bound + * \p min can be reached, but the upper bound \p N cannot. * - * \return \c 0 on success. - * \return \c MBEDTLS_ERR_ECP_xxx or MBEDTLS_ERR_MPI_xxx on failure. + * \param X The destination MPI. This must point to an initialized MPI. + * \param min The minimum value to return. + * It must be nonnegative. + * \param N The upper bound of the range, exclusive. + * In other words, this is one plus the maximum value to return. + * \p N must be strictly larger than \p min. + * \param f_rng The RNG function to use. This must not be \c NULL. + * \param p_rng The RNG parameter to be passed to \p f_rng. + * + * \return \c 0 if successful. + * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. + * \return Another negative error code on failure. */ -int mbedtls_ecp_gen_privkey_sw( const mbedtls_mpi *N, size_t n_bits, - mbedtls_mpi *d, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ); +int mbedtls_mpi_random( mbedtls_mpi *X, + mbedtls_mpi_sint min, + const mbedtls_mpi *N, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ); #endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ #endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_ECP_C */ diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index 0c344561b..c007cad38 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -312,95 +312,104 @@ genkey_mx_known_answer:447:"ffffffffffffffffffffffffffffffffffffffffffffffffffff ECP generate Montgomery key: Curve448, not enough entropy genkey_mx_known_answer:447:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536":"" -ECP generate in range: 4 -genkey_sw_many:"04":1000 +MPI random in range: 1..4 +mpi_random_many:1:"04":1000 -ECP generate in range: 5 -genkey_sw_many:"05":1000 +MPI random in range: 1..5 +mpi_random_many:1:"05":1000 -ECP generate in range: 6 -genkey_sw_many:"06":1000 +MPI random in range: 1..6 +mpi_random_many:1:"06":1000 -ECP generate in range: 7 -genkey_sw_many:"07":1000 +MPI random in range: 1..7 +mpi_random_many:1:"07":1000 -ECP generate in range: 8 -genkey_sw_many:"08":1000 +MPI random in range: 1..8 +mpi_random_many:1:"08":1000 -ECP generate in range: 9 -genkey_sw_many:"09":1000 +MPI random in range: 1..9 +mpi_random_many:1:"09":1000 -ECP generate in range: 10 -genkey_sw_many:"0a":1000 +MPI random in range: 1..10 +mpi_random_many:1:"0a":1000 -ECP generate in range: 11 -genkey_sw_many:"0b":1000 +MPI random in range: 1..11 +mpi_random_many:1:"0b":1000 -ECP generate in range: 12 -genkey_sw_many:"0c":1000 +MPI random in range: 1..12 +mpi_random_many:1:"0c":1000 -ECP generate in range: 255 -genkey_sw_many:"ff":100 +MPI random in range: 1..255 +mpi_random_many:1:"ff":100 -ECP generate in range: 256 -genkey_sw_many:"0100":100 +MPI random in range: 1..256 +mpi_random_many:1:"0100":100 -ECP generate in range: 257 -genkey_sw_many:"0101":100 +MPI random in range: 1..257 +mpi_random_many:1:"0101":100 -ECP generate in range: 272 -genkey_sw_many:"0110":100 +MPI random in range: 1..272 +mpi_random_many:1:"0110":100 -ECP generate in range: 2^64-1 -genkey_sw_many:"ffffffffffffffff":100 +MPI random in range: 1..2^64-1 +mpi_random_many:1:"ffffffffffffffff":100 -ECP generate in range: 2^64 -genkey_sw_many:"010000000000000000":100 +MPI random in range: 1..2^64 +mpi_random_many:1:"010000000000000000":100 -ECP generate in range: 2^64+1 -genkey_sw_many:"010000000000000001":100 +MPI random in range: 1..2^64+1 +mpi_random_many:1:"010000000000000001":100 -ECP generate in range: 2^64+2^63 -genkey_sw_many:"018000000000000000":100 +MPI random in range: 1..2^64+2^63 +mpi_random_many:1:"018000000000000000":100 -ECP generate in range: 2^65-1 -genkey_sw_many:"01ffffffffffffffff":100 +MPI random in range: 1..2^65-1 +mpi_random_many:1:"01ffffffffffffffff":100 -ECP generate in range: 2^65 -genkey_sw_many:"020000000000000000":100 +MPI random in range: 1..2^65 +mpi_random_many:1:"020000000000000000":100 -ECP generate in range: 2^65+1 -genkey_sw_many:"020000000000000001":100 +MPI random in range: 1..2^65+1 +mpi_random_many:1:"020000000000000001":100 -ECP generate in range: 2^65+2^64 -genkey_sw_many:"030000000000000000":100 +MPI random in range: 1..2^65+2^64 +mpi_random_many:1:"030000000000000000":100 -ECP generate in range: 2^66+2^65 -genkey_sw_many:"060000000000000000":100 +MPI random in range: 1..2^66+2^65 +mpi_random_many:1:"060000000000000000":100 -ECP generate in range: 2^71-1 -genkey_sw_many:"7fffffffffffffffff":100 +MPI random in range: 1..2^71-1 +mpi_random_many:1:"7fffffffffffffffff":100 -ECP generate in range: 2^71 -genkey_sw_many:"800000000000000000":100 +MPI random in range: 1..2^71 +mpi_random_many:1:"800000000000000000":100 -ECP generate in range: 2^71+1 -genkey_sw_many:"800000000000000001":100 +MPI random in range: 1..2^71+1 +mpi_random_many:1:"800000000000000001":100 -ECP generate in range: 2^71+2^63 -genkey_sw_many:"c00000000000000000":100 +MPI random in range: 1..2^71+2^63 +mpi_random_many:1:"c00000000000000000":100 -ECP generate in range: 2^72-1 -genkey_sw_many:"ffffffffffffffffff":100 +MPI random in range: 1..2^72-1 +mpi_random_many:1:"ffffffffffffffffff":100 -ECP generate in range: 2^72 -genkey_sw_many:"01000000000000000000":100 +MPI random in range: 1..2^72 +mpi_random_many:1:"01000000000000000000":100 -ECP generate in range: 2^72+1 -genkey_sw_many:"01000000000000000001":100 +MPI random in range: 1..2^72+1 +mpi_random_many:1:"01000000000000000001":100 -ECP generate in range: 2^72+2^63 -genkey_sw_many:"01800000000000000000":100 +MPI random in range: 1..2^72+2^63 +mpi_random_many:1:"01800000000000000000":100 + +MPI random in range: 0..4 +mpi_random_many:0:"04":1000 + +MPI random in range: 2..4 +mpi_random_many:1:"04":1000 + +MPI random in range: 3..4 +mpi_random_many:1:"04":1000 ECP read key #1 (short weierstrass, too small) depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 1049c96a1..fadc3001c 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -1324,7 +1324,7 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS:MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -void genkey_sw_many( data_t *bound_bytes, int iterations ) +void mpi_random_many( int min, data_t *bound_bytes, int iterations ) { /* Generate numbers in the range 1..bound-1. Do it iterations times. * This function assumes that the value of bound is at least 2 and @@ -1332,11 +1332,11 @@ void genkey_sw_many( data_t *bound_bytes, int iterations ) * effectively never occurs. */ - mbedtls_mpi bound; + mbedtls_mpi upper_bound; size_t n_bits; mbedtls_mpi result; size_t b; - /* If bound is small, stats[b] is the number of times the value b + /* If upper_bound is small, stats[b] is the number of times the value b * has been generated. Otherwise stats[b] is the number of times a * value with bit b set has been generated. */ size_t *stats = NULL; @@ -1344,12 +1344,12 @@ void genkey_sw_many( data_t *bound_bytes, int iterations ) int full_stats; size_t i; - mbedtls_mpi_init( &bound ); + mbedtls_mpi_init( &upper_bound ); mbedtls_mpi_init( &result ); - TEST_EQUAL( 0, mbedtls_mpi_read_binary( &bound, + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &upper_bound, bound_bytes->x, bound_bytes->len ) ); - n_bits = mbedtls_mpi_bitlen( &bound ); + n_bits = mbedtls_mpi_bitlen( &upper_bound ); /* Consider a bound "small" if it's less than 2^5. This value is chosen * to be small enough that the probability of missing one value is * negligible given the number of iterations. It must be less than @@ -1370,12 +1370,11 @@ void genkey_sw_many( data_t *bound_bytes, int iterations ) for( i = 0; i < (size_t) iterations; i++ ) { mbedtls_test_set_step( i ); - TEST_EQUAL( 0, mbedtls_ecp_gen_privkey_sw( - &bound, n_bits, &result, - mbedtls_test_rnd_std_rand, NULL ) ); + TEST_EQUAL( 0, mbedtls_mpi_random( &result, min, &upper_bound, + mbedtls_test_rnd_std_rand, NULL ) ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &result, &bound ) < 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_int( &result, 1 ) >= 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &result, &upper_bound ) < 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &result, min ) >= 0 ); if( full_stats ) { uint8_t value; @@ -1425,7 +1424,7 @@ void genkey_sw_many( data_t *bound_bytes, int iterations ) } exit: - mbedtls_mpi_free( &bound ); + mbedtls_mpi_free( &upper_bound ); mbedtls_mpi_free( &result ); mbedtls_free( stats ); } From 02ac93a1a3bd7fe4de82866b3da2df68b9d0c622 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 29 Mar 2021 22:02:55 +0200 Subject: [PATCH 12/48] Move mbedtls_mpi_random to the bignum module Since mbedtls_mpi_random() is not specific to ECC code, move it from the ECP module to the bignum module. This increases the code size in builds without short Weierstrass curves (including builds without ECC at all) that do not optimize out unused functions. Signed-off-by: Gilles Peskine --- include/mbedtls/bignum.h | 31 ++++++ library/bignum.c | 53 ++++++++++ library/ecp.c | 56 ----------- library/ecp_invasive.h | 33 ------- tests/suites/test_suite_ecp.data | 99 ------------------- tests/suites/test_suite_ecp.function | 143 --------------------------- tests/suites/test_suite_mpi.data | 99 +++++++++++++++++++ tests/suites/test_suite_mpi.function | 141 ++++++++++++++++++++++++++ 8 files changed, 324 insertions(+), 331 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 073b4a40c..e938232ad 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -871,6 +871,37 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ); +/** Generate a random number uniformly in a range. + * + * This function generates a random number between \p min inclusive and + * \p N exclusive. + * + * The procedure complies with RFC 6979 §3.3 (deterministic ECDSA) + * when the RNG is a suitably parametrized instance of HMAC_DRBG + * and \p min is \c 1. + * + * \note There are `N - min` possible outputs. The lower bound + * \p min can be reached, but the upper bound \p N cannot. + * + * \param X The destination MPI. This must point to an initialized MPI. + * \param min The minimum value to return. + * It must be nonnegative. + * \param N The upper bound of the range, exclusive. + * In other words, this is one plus the maximum value to return. + * \p N must be strictly larger than \p min. + * \param f_rng The RNG function to use. This must not be \c NULL. + * \param p_rng The RNG parameter to be passed to \p f_rng. + * + * \return \c 0 if successful. + * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. + * \return Another negative error code on failure. + */ +int mbedtls_mpi_random( mbedtls_mpi *X, + mbedtls_mpi_sint min, + const mbedtls_mpi *N, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ); + /** * \brief Compute the greatest common divisor: G = gcd(A, B) * diff --git a/library/bignum.c b/library/bignum.c index c20c6b77b..aa4b1ce2d 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2432,6 +2432,59 @@ cleanup: return( ret ); } +int mbedtls_mpi_random( mbedtls_mpi *X, + mbedtls_mpi_sint min, + const mbedtls_mpi *N, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + /* SEC1 3.2.1: Generate X such that 1 <= n < N */ + int ret = MBEDTLS_ERR_MPI_BAD_INPUT_DATA; + int count = 0; + unsigned cmp = 0; + size_t n_bits = mbedtls_mpi_bitlen( N ); + size_t n_bytes = ( n_bits + 7 ) / 8; + + /* + * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) + * when f_rng is a suitably parametrized instance of HMAC_DRBG: + * - use the same byte ordering; + * - keep the leftmost n_bits bits of the generated octet string; + * - try until result is in the desired range. + * This also avoids any bias, which is especially important for ECDSA. + */ + do + { + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( X, n_bytes, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( X, 8 * n_bytes - n_bits ) ); + + /* + * Each try has at worst a probability 1/2 of failing (the msb has + * a probability 1/2 of being 0, and then the result will be < N), + * so after 30 tries failure probability is a most 2**(-30). + * + * For most curves, 1 try is enough with overwhelming probability, + * since N starts with a lot of 1s in binary, but some curves + * such as secp224k1 are actually very close to the worst case. + */ + if( ++count > 30 ) + { + ret = MBEDTLS_ERR_MPI_NOT_ACCEPTABLE; + goto cleanup; + } + + ret = mbedtls_mpi_lt_mpi_ct( X, N, &cmp ); + if( ret != 0 ) + { + goto cleanup; + } + } + while( mbedtls_mpi_cmp_int( X, min ) < 0 || cmp != 1 ); + +cleanup: + return( ret ); +} + /* * Modular inverse: X = A^-1 mod N (HAC 14.61 / 14.64) */ diff --git a/library/ecp.c b/library/ecp.c index 98ada8283..3e5cebc29 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3091,62 +3091,6 @@ cleanup: } #endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ -#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) -MBEDTLS_STATIC_TESTABLE -int mbedtls_mpi_random( mbedtls_mpi *X, - mbedtls_mpi_sint min, - const mbedtls_mpi *N, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) -{ - /* SEC1 3.2.1: Generate X such that 1 <= n < N */ - int ret = MBEDTLS_ERR_MPI_BAD_INPUT_DATA; - int count = 0; - unsigned cmp = 0; - size_t n_bits = mbedtls_mpi_bitlen( N ); - size_t n_bytes = ( n_bits + 7 ) / 8; - - /* - * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) - * when f_rng is a suitably parametrized instance of HMAC_DRBG: - * - use the same byte ordering; - * - keep the leftmost n_bits bits of the generated octet string; - * - try until result is in the desired range. - * This also avoids any bias, which is especially important for ECDSA. - */ - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( X, n_bytes, f_rng, p_rng ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( X, 8 * n_bytes - n_bits ) ); - - /* - * Each try has at worst a probability 1/2 of failing (the msb has - * a probability 1/2 of being 0, and then the result will be < N), - * so after 30 tries failure probability is a most 2**(-30). - * - * For most curves, 1 try is enough with overwhelming probability, - * since N starts with a lot of 1s in binary, but some curves - * such as secp224k1 are actually very close to the worst case. - */ - if( ++count > 30 ) - { - ret = MBEDTLS_ERR_MPI_NOT_ACCEPTABLE; - goto cleanup; - } - - ret = mbedtls_mpi_lt_mpi_ct( X, N, &cmp ); - if( ret != 0 ) - { - goto cleanup; - } - } - while( mbedtls_mpi_cmp_int( X, min ) < 0 || cmp != 1 ); - -cleanup: - return( ret ); -} -#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ - /* * Generate a private key */ diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index 1ca65fd78..71c770275 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -76,39 +76,6 @@ int mbedtls_ecp_gen_privkey_mx( size_t n_bits, #endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ -#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) -/** Generate a random number uniformly in a range. - * - * This function generates a random number between \p min inclusive and - * \p N exclusive. - * - * The procedure complies with RFC 6979 §3.3 (deterministic ECDSA) - * when the RNG is a suitably parametrized instance of HMAC_DRBG - * and \p min is \c 1. - * - * \note There are `N - min` possible outputs. The lower bound - * \p min can be reached, but the upper bound \p N cannot. - * - * \param X The destination MPI. This must point to an initialized MPI. - * \param min The minimum value to return. - * It must be nonnegative. - * \param N The upper bound of the range, exclusive. - * In other words, this is one plus the maximum value to return. - * \p N must be strictly larger than \p min. - * \param f_rng The RNG function to use. This must not be \c NULL. - * \param p_rng The RNG parameter to be passed to \p f_rng. - * - * \return \c 0 if successful. - * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. - * \return Another negative error code on failure. - */ -int mbedtls_mpi_random( mbedtls_mpi *X, - mbedtls_mpi_sint min, - const mbedtls_mpi *N, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ); -#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ - #endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_ECP_C */ #endif /* MBEDTLS_ECP_INVASIVE_H */ diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index c007cad38..5f92ca459 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -312,105 +312,6 @@ genkey_mx_known_answer:447:"ffffffffffffffffffffffffffffffffffffffffffffffffffff ECP generate Montgomery key: Curve448, not enough entropy genkey_mx_known_answer:447:"4f0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536":"" -MPI random in range: 1..4 -mpi_random_many:1:"04":1000 - -MPI random in range: 1..5 -mpi_random_many:1:"05":1000 - -MPI random in range: 1..6 -mpi_random_many:1:"06":1000 - -MPI random in range: 1..7 -mpi_random_many:1:"07":1000 - -MPI random in range: 1..8 -mpi_random_many:1:"08":1000 - -MPI random in range: 1..9 -mpi_random_many:1:"09":1000 - -MPI random in range: 1..10 -mpi_random_many:1:"0a":1000 - -MPI random in range: 1..11 -mpi_random_many:1:"0b":1000 - -MPI random in range: 1..12 -mpi_random_many:1:"0c":1000 - -MPI random in range: 1..255 -mpi_random_many:1:"ff":100 - -MPI random in range: 1..256 -mpi_random_many:1:"0100":100 - -MPI random in range: 1..257 -mpi_random_many:1:"0101":100 - -MPI random in range: 1..272 -mpi_random_many:1:"0110":100 - -MPI random in range: 1..2^64-1 -mpi_random_many:1:"ffffffffffffffff":100 - -MPI random in range: 1..2^64 -mpi_random_many:1:"010000000000000000":100 - -MPI random in range: 1..2^64+1 -mpi_random_many:1:"010000000000000001":100 - -MPI random in range: 1..2^64+2^63 -mpi_random_many:1:"018000000000000000":100 - -MPI random in range: 1..2^65-1 -mpi_random_many:1:"01ffffffffffffffff":100 - -MPI random in range: 1..2^65 -mpi_random_many:1:"020000000000000000":100 - -MPI random in range: 1..2^65+1 -mpi_random_many:1:"020000000000000001":100 - -MPI random in range: 1..2^65+2^64 -mpi_random_many:1:"030000000000000000":100 - -MPI random in range: 1..2^66+2^65 -mpi_random_many:1:"060000000000000000":100 - -MPI random in range: 1..2^71-1 -mpi_random_many:1:"7fffffffffffffffff":100 - -MPI random in range: 1..2^71 -mpi_random_many:1:"800000000000000000":100 - -MPI random in range: 1..2^71+1 -mpi_random_many:1:"800000000000000001":100 - -MPI random in range: 1..2^71+2^63 -mpi_random_many:1:"c00000000000000000":100 - -MPI random in range: 1..2^72-1 -mpi_random_many:1:"ffffffffffffffffff":100 - -MPI random in range: 1..2^72 -mpi_random_many:1:"01000000000000000000":100 - -MPI random in range: 1..2^72+1 -mpi_random_many:1:"01000000000000000001":100 - -MPI random in range: 1..2^72+2^63 -mpi_random_many:1:"01800000000000000000":100 - -MPI random in range: 0..4 -mpi_random_many:0:"04":1000 - -MPI random in range: 2..4 -mpi_random_many:1:"04":1000 - -MPI random in range: 3..4 -mpi_random_many:1:"04":1000 - ECP read key #1 (short weierstrass, too small) depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED mbedtls_ecp_read_key:MBEDTLS_ECP_DP_SECP192R1:"00":MBEDTLS_ERR_ECP_INVALID_KEY:0 diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index fadc3001c..934598dd3 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -16,42 +16,6 @@ mbedtls_ecp_point_free( x ); \ mbedtls_ecp_point_init( x ); -#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) -/* Test whether bytes represents (in big-endian base 256) a number B that - * is "significantly" above a power of 2, which is defined as follows. - * Let n be the integer such that 2^n <= B < 2^{n+1}. B is significantly - * above a power of 2 if (B - 2^n) / 2^n is not negligible. "Negligible" - * is defined as having a negligible chance that if you draw an integer - * in the range [1, B-1] K times, the number will always be less than 2^n, - * where K is the iteration count passed to genkey_sw_many. - */ -static int is_significantly_above_a_power_of_2( data_t *bytes ) -{ - const uint8_t *p = bytes->x; - size_t len = bytes->len; - unsigned x; - while( len > 0 && p[0] == 0 ) - { - ++p; - --len; - } - if( len == 0 ) - return( 0 ); - else if( len == 1 ) - x = p[0]; - else - x = ( p[0] << 8 ) | p[1]; - - if( x <= 4 ) - return( 0 ); - - while( ( x & 0x8000 ) == 0 ) - x <<= 1; - x &= 0x7fff; - return( x >= 0x1000 ); -} -#endif - /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -1323,113 +1287,6 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS:MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -void mpi_random_many( int min, data_t *bound_bytes, int iterations ) -{ - /* Generate numbers in the range 1..bound-1. Do it iterations times. - * This function assumes that the value of bound is at least 2 and - * that iterations is large enough that a one-in-2^iterations chance - * effectively never occurs. - */ - - mbedtls_mpi upper_bound; - size_t n_bits; - mbedtls_mpi result; - size_t b; - /* If upper_bound is small, stats[b] is the number of times the value b - * has been generated. Otherwise stats[b] is the number of times a - * value with bit b set has been generated. */ - size_t *stats = NULL; - size_t stats_len; - int full_stats; - size_t i; - - mbedtls_mpi_init( &upper_bound ); - mbedtls_mpi_init( &result ); - - TEST_EQUAL( 0, mbedtls_mpi_read_binary( &upper_bound, - bound_bytes->x, bound_bytes->len ) ); - n_bits = mbedtls_mpi_bitlen( &upper_bound ); - /* Consider a bound "small" if it's less than 2^5. This value is chosen - * to be small enough that the probability of missing one value is - * negligible given the number of iterations. It must be less than - * 256 because some of the code below assumes that "small" values - * fit in a byte. */ - if( n_bits <= 5 ) - { - full_stats = 1; - stats_len = bound_bytes->x[bound_bytes->len - 1]; - } - else - { - full_stats = 0; - stats_len = n_bits; - } - ASSERT_ALLOC( stats, stats_len ); - - for( i = 0; i < (size_t) iterations; i++ ) - { - mbedtls_test_set_step( i ); - TEST_EQUAL( 0, mbedtls_mpi_random( &result, min, &upper_bound, - mbedtls_test_rnd_std_rand, NULL ) ); - - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &result, &upper_bound ) < 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_int( &result, min ) >= 0 ); - if( full_stats ) - { - uint8_t value; - TEST_EQUAL( 0, mbedtls_mpi_write_binary( &result, &value, 1 ) ); - TEST_ASSERT( value < stats_len ); - ++stats[value]; - } - else - { - for( b = 0; b < n_bits; b++ ) - stats[b] += mbedtls_mpi_get_bit( &result, b ); - } - } - - if( full_stats ) - { - for( b = 1; b < stats_len; b++ ) - { - mbedtls_test_set_step( 1000000 + b ); - /* Assert that each value has been reached at least once. - * This is almost guaranteed if the iteration count is large - * enough. This is a very crude way of checking the distribution. - */ - TEST_ASSERT( stats[b] > 0 ); - } - } - else - { - for( b = 0; b < n_bits; b++ ) - { - mbedtls_test_set_step( 1000000 + b ); - /* Assert that each bit has been set in at least one result and - * clear in at least one result. Provided that iterations is not - * too small, it would be extremely unlikely for this not to be - * the case if the results are uniformly distributed. - * - * As an exception, the top bit may legitimately never be set - * if bound is a power of 2 or only slightly above. - */ - if( b != n_bits - 1 || - is_significantly_above_a_power_of_2( bound_bytes ) ) - { - TEST_ASSERT( stats[b] > 0 ); - } - TEST_ASSERT( stats[b] < (size_t) iterations ); - } - } - -exit: - mbedtls_mpi_free( &upper_bound ); - mbedtls_mpi_free( &result ); - mbedtls_free( stats ); -} -/* END_CASE */ - /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void ecp_selftest( ) { diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 59fd7824b..a057dcd88 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1033,6 +1033,105 @@ mpi_fill_random:16:15:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: MAX_SIZE bytes, RNG failure after MAX_SIZE-1 bytes mpi_fill_random:MBEDTLS_MPI_MAX_SIZE:MBEDTLS_MPI_MAX_SIZE-1:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +MPI random in range: 1..4 +mpi_random_many:1:"04":1000 + +MPI random in range: 1..5 +mpi_random_many:1:"05":1000 + +MPI random in range: 1..6 +mpi_random_many:1:"06":1000 + +MPI random in range: 1..7 +mpi_random_many:1:"07":1000 + +MPI random in range: 1..8 +mpi_random_many:1:"08":1000 + +MPI random in range: 1..9 +mpi_random_many:1:"09":1000 + +MPI random in range: 1..10 +mpi_random_many:1:"0a":1000 + +MPI random in range: 1..11 +mpi_random_many:1:"0b":1000 + +MPI random in range: 1..12 +mpi_random_many:1:"0c":1000 + +MPI random in range: 1..255 +mpi_random_many:1:"ff":100 + +MPI random in range: 1..256 +mpi_random_many:1:"0100":100 + +MPI random in range: 1..257 +mpi_random_many:1:"0101":100 + +MPI random in range: 1..272 +mpi_random_many:1:"0110":100 + +MPI random in range: 1..2^64-1 +mpi_random_many:1:"ffffffffffffffff":100 + +MPI random in range: 1..2^64 +mpi_random_many:1:"010000000000000000":100 + +MPI random in range: 1..2^64+1 +mpi_random_many:1:"010000000000000001":100 + +MPI random in range: 1..2^64+2^63 +mpi_random_many:1:"018000000000000000":100 + +MPI random in range: 1..2^65-1 +mpi_random_many:1:"01ffffffffffffffff":100 + +MPI random in range: 1..2^65 +mpi_random_many:1:"020000000000000000":100 + +MPI random in range: 1..2^65+1 +mpi_random_many:1:"020000000000000001":100 + +MPI random in range: 1..2^65+2^64 +mpi_random_many:1:"030000000000000000":100 + +MPI random in range: 1..2^66+2^65 +mpi_random_many:1:"060000000000000000":100 + +MPI random in range: 1..2^71-1 +mpi_random_many:1:"7fffffffffffffffff":100 + +MPI random in range: 1..2^71 +mpi_random_many:1:"800000000000000000":100 + +MPI random in range: 1..2^71+1 +mpi_random_many:1:"800000000000000001":100 + +MPI random in range: 1..2^71+2^63 +mpi_random_many:1:"c00000000000000000":100 + +MPI random in range: 1..2^72-1 +mpi_random_many:1:"ffffffffffffffffff":100 + +MPI random in range: 1..2^72 +mpi_random_many:1:"01000000000000000000":100 + +MPI random in range: 1..2^72+1 +mpi_random_many:1:"01000000000000000001":100 + +MPI random in range: 1..2^72+2^63 +mpi_random_many:1:"01800000000000000000":100 + +MPI random in range: 0..4 +mpi_random_many:0:"04":1000 + +MPI random in range: 2..4 +mpi_random_many:1:"04":1000 + +MPI random in range: 3..4 +mpi_random_many:1:"04":1000 + MPI Selftest depends_on:MBEDTLS_SELF_TEST mpi_selftest: diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index c5bb5a678..8cf53b302 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -64,6 +64,40 @@ static int f_rng_bytes_left( void *state, unsigned char *buf, size_t len ) return( 0 ); } +/* Test whether bytes represents (in big-endian base 256) a number B that + * is "significantly" above a power of 2, which is defined as follows. + * Let n be the integer such that 2^n <= B < 2^{n+1}. B is significantly + * above a power of 2 if (B - 2^n) / 2^n is not negligible. "Negligible" + * is defined as having a negligible chance that if you draw an integer + * in the range [1, B-1] K times, the number will always be less than 2^n, + * where K is the iteration count passed to genkey_sw_many. + */ +static int is_significantly_above_a_power_of_2( data_t *bytes ) +{ + const uint8_t *p = bytes->x; + size_t len = bytes->len; + unsigned x; + while( len > 0 && p[0] == 0 ) + { + ++p; + --len; + } + if( len == 0 ) + return( 0 ); + else if( len == 1 ) + x = p[0]; + else + x = ( p[0] << 8 ) | p[1]; + + if( x <= 4 ) + return( 0 ); + + while( ( x & 0x8000 ) == 0 ) + x <<= 1; + x &= 0x7fff; + return( x >= 0x1000 ); +} + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -1396,6 +1430,113 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mpi_random_many( int min, data_t *bound_bytes, int iterations ) +{ + /* Generate numbers in the range 1..bound-1. Do it iterations times. + * This function assumes that the value of bound is at least 2 and + * that iterations is large enough that a one-in-2^iterations chance + * effectively never occurs. + */ + + mbedtls_mpi upper_bound; + size_t n_bits; + mbedtls_mpi result; + size_t b; + /* If upper_bound is small, stats[b] is the number of times the value b + * has been generated. Otherwise stats[b] is the number of times a + * value with bit b set has been generated. */ + size_t *stats = NULL; + size_t stats_len; + int full_stats; + size_t i; + + mbedtls_mpi_init( &upper_bound ); + mbedtls_mpi_init( &result ); + + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &upper_bound, + bound_bytes->x, bound_bytes->len ) ); + n_bits = mbedtls_mpi_bitlen( &upper_bound ); + /* Consider a bound "small" if it's less than 2^5. This value is chosen + * to be small enough that the probability of missing one value is + * negligible given the number of iterations. It must be less than + * 256 because some of the code below assumes that "small" values + * fit in a byte. */ + if( n_bits <= 5 ) + { + full_stats = 1; + stats_len = bound_bytes->x[bound_bytes->len - 1]; + } + else + { + full_stats = 0; + stats_len = n_bits; + } + ASSERT_ALLOC( stats, stats_len ); + + for( i = 0; i < (size_t) iterations; i++ ) + { + mbedtls_test_set_step( i ); + TEST_EQUAL( 0, mbedtls_mpi_random( &result, min, &upper_bound, + mbedtls_test_rnd_std_rand, NULL ) ); + + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &result, &upper_bound ) < 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &result, min ) >= 0 ); + if( full_stats ) + { + uint8_t value; + TEST_EQUAL( 0, mbedtls_mpi_write_binary( &result, &value, 1 ) ); + TEST_ASSERT( value < stats_len ); + ++stats[value]; + } + else + { + for( b = 0; b < n_bits; b++ ) + stats[b] += mbedtls_mpi_get_bit( &result, b ); + } + } + + if( full_stats ) + { + for( b = 1; b < stats_len; b++ ) + { + mbedtls_test_set_step( 1000000 + b ); + /* Assert that each value has been reached at least once. + * This is almost guaranteed if the iteration count is large + * enough. This is a very crude way of checking the distribution. + */ + TEST_ASSERT( stats[b] > 0 ); + } + } + else + { + for( b = 0; b < n_bits; b++ ) + { + mbedtls_test_set_step( 1000000 + b ); + /* Assert that each bit has been set in at least one result and + * clear in at least one result. Provided that iterations is not + * too small, it would be extremely unlikely for this not to be + * the case if the results are uniformly distributed. + * + * As an exception, the top bit may legitimately never be set + * if bound is a power of 2 or only slightly above. + */ + if( b != n_bits - 1 || + is_significantly_above_a_power_of_2( bound_bytes ) ) + { + TEST_ASSERT( stats[b] > 0 ); + } + TEST_ASSERT( stats[b] < (size_t) iterations ); + } + } + +exit: + mbedtls_mpi_free( &upper_bound ); + mbedtls_mpi_free( &result ); + mbedtls_free( stats ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void mpi_selftest( ) { From 1e918f44c98bc0f820e174d1faf40c8382c93bee Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 29 Mar 2021 22:14:51 +0200 Subject: [PATCH 13/48] mbedtls_mpi_random: check for invalid arguments Signed-off-by: Gilles Peskine --- include/mbedtls/bignum.h | 2 ++ library/bignum.c | 5 +++++ tests/suites/test_suite_mpi.data | 9 +++++++++ tests/suites/test_suite_mpi.function | 22 ++++++++++++++++++++++ 4 files changed, 38 insertions(+) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index e938232ad..327442214 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -894,6 +894,8 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, * * \return \c 0 if successful. * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. + * \return #MBEDTLS_ERR_MPI_BAD_INPUT_DATA if \p min or \p N is invalid + * or if they are incompatible. * \return Another negative error code on failure. */ int mbedtls_mpi_random( mbedtls_mpi *X, diff --git a/library/bignum.c b/library/bignum.c index aa4b1ce2d..fba9acc5b 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2445,6 +2445,11 @@ int mbedtls_mpi_random( mbedtls_mpi *X, size_t n_bits = mbedtls_mpi_bitlen( N ); size_t n_bytes = ( n_bits + 7 ) / 8; + if( min < 0 ) + return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + if( mbedtls_mpi_cmp_int( N, min ) <= 0 ) + return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + /* * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) * when f_rng is a suitably parametrized instance of HMAC_DRBG: diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index a057dcd88..fb2c553d0 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1132,6 +1132,15 @@ mpi_random_many:1:"04":1000 MPI random in range: 3..4 mpi_random_many:1:"04":1000 +MPI random bad arguments: min < 0 +mpi_random_fail:-1:"04":MBEDTLS_ERR_MPI_BAD_INPUT_DATA + +MPI random bad arguments: min = N = 0 +mpi_random_fail:0:"00":MBEDTLS_ERR_MPI_BAD_INPUT_DATA + +MPI random bad arguments: min = N = 1 +mpi_random_fail:1:"01":MBEDTLS_ERR_MPI_BAD_INPUT_DATA + MPI Selftest depends_on:MBEDTLS_SELF_TEST mpi_selftest: diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 8cf53b302..0ca6b6439 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -1537,6 +1537,28 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mpi_random_fail( int min, data_t *bound_bytes, int expected_ret ) +{ + mbedtls_mpi upper_bound; + mbedtls_mpi result; + int actual_ret; + + mbedtls_mpi_init( &upper_bound ); + mbedtls_mpi_init( &result ); + + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &upper_bound, + bound_bytes->x, bound_bytes->len ) ); + actual_ret = mbedtls_mpi_random( &result, min, &upper_bound, + mbedtls_test_rnd_std_rand, NULL ); + TEST_EQUAL( expected_ret, actual_ret ); + +exit: + mbedtls_mpi_free( &upper_bound ); + mbedtls_mpi_free( &result ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void mpi_selftest( ) { From fdc58c1e8b54455f48a7e6d0f374f009205a8686 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 23:15:00 +0200 Subject: [PATCH 14/48] Changelog entry for adding mbedtls_mpi_random() Signed-off-by: Gilles Peskine --- ChangeLog.d/mpi_random.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 ChangeLog.d/mpi_random.txt diff --git a/ChangeLog.d/mpi_random.txt b/ChangeLog.d/mpi_random.txt new file mode 100644 index 000000000..9e6a41623 --- /dev/null +++ b/ChangeLog.d/mpi_random.txt @@ -0,0 +1,3 @@ +Features + * The new function mbedtls_mpi_random() generates a random value in a + given range uniformly. From 60d8b98d4844475884b4a585cf38ac8a26184d91 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 29 Mar 2021 22:28:21 +0200 Subject: [PATCH 15/48] Preserve MBEDTLS_ERR_ECP_RANDOM_FAILED in case of a hostile RNG Signed-off-by: Gilles Peskine --- library/ecp.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/library/ecp.c b/library/ecp.c index 3e5cebc29..30d4d1923 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3091,6 +3091,22 @@ cleanup: } #endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) +static int mbedtls_ecp_gen_privkey_sw( + const mbedtls_mpi *N, mbedtls_mpi *d, + int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) +{ + int ret = mbedtls_mpi_random( d, 1, N, f_rng, p_rng ); + switch( ret ) + { + case MBEDTLS_ERR_MPI_NOT_ACCEPTABLE: + return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); + default: + return( ret ); + } +} +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ + /* * Generate a private key */ @@ -3110,7 +3126,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, #if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) - return( mbedtls_mpi_random( d, 1, &grp->N, f_rng, p_rng ) ); + return( mbedtls_ecp_gen_privkey_sw( &grp->N, d, f_rng, p_rng ) ); #endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); From 5921517126fcd87837c320075bf0dc2b108239c3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 29 Mar 2021 22:28:50 +0200 Subject: [PATCH 16/48] ECP: use mbedtls_mpi_random for blinding Instead of generating blinding values in a not-quite-uniform way (https://github.com/ARMmbed/mbedtls/issues/4245) with copy-pasted code, use mbedtls_mpi_random(). Signed-off-by: Gilles Peskine --- library/ecp.c | 38 ++++++-------------------------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 30d4d1923..f450056c0 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1713,26 +1713,11 @@ static int ecp_randomize_jac( const mbedtls_ecp_group *grp, mbedtls_ecp_point *p #else int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi l, ll; - int count = 0; - size_t p_size = ( grp->pbits + 7 ) / 8; mbedtls_mpi_init( &l ); mbedtls_mpi_init( &ll ); /* Generate l such that 1 < l < p */ - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &l, p_size, f_rng, p_rng ) ); - - while( mbedtls_mpi_cmp_mpi( &l, &grp->P ) >= 0 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, 1 ) ); - - if( count++ > 10 ) - { - ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; - goto cleanup; - } - } - while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 ); + MBEDTLS_MPI_CHK( mbedtls_mpi_random( &l, 2, &grp->P, f_rng, p_rng ) ); /* Z = l * Z */ MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mod( grp, &pt->Z, &pt->Z, &l ) ); @@ -1748,6 +1733,8 @@ static int ecp_randomize_jac( const mbedtls_ecp_group *grp, mbedtls_ecp_point *p cleanup: mbedtls_mpi_free( &l ); mbedtls_mpi_free( &ll ); + if( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) + ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; return( ret ); #endif /* !defined(MBEDTLS_ECP_NO_FALLBACK) || !defined(MBEDTLS_ECP_RANDOMIZE_JAC_ALT) */ } @@ -2502,25 +2489,10 @@ static int ecp_randomize_mxz( const mbedtls_ecp_group *grp, mbedtls_ecp_point *P #else int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi l; - int count = 0; - size_t p_size = ( grp->pbits + 7 ) / 8; mbedtls_mpi_init( &l ); /* Generate l such that 1 < l < p */ - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &l, p_size, f_rng, p_rng ) ); - - while( mbedtls_mpi_cmp_mpi( &l, &grp->P ) >= 0 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, 1 ) ); - - if( count++ > 10 ) - { - ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; - goto cleanup; - } - } - while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 ); + MBEDTLS_MPI_CHK( mbedtls_mpi_random( &l, 2, &grp->P, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mod( grp, &P->X, &P->X, &l ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mod( grp, &P->Z, &P->Z, &l ) ); @@ -2528,6 +2500,8 @@ static int ecp_randomize_mxz( const mbedtls_ecp_group *grp, mbedtls_ecp_point *P cleanup: mbedtls_mpi_free( &l ); + if( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) + ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; return( ret ); #endif /* !defined(MBEDTLS_ECP_NO_FALLBACK) || !defined(MBEDTLS_ECP_RANDOMIZE_MXZ_ALT) */ } From 02db8f4cf76932e83823b4726c8133d25c21b768 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 30 Mar 2021 23:28:51 +0200 Subject: [PATCH 17/48] Test range and format of dhm_make_params output Improve the validation of the output from mbedtls_dhm_make_params: * Test that the output in the byte buffer matches the value in the context structure. * Test that the calculated values are in the desired range. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_dhm.function | 63 ++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/suites/test_suite_dhm.function b/tests/suites/test_suite_dhm.function index 1726b9eb7..b9dcde580 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -1,5 +1,61 @@ /* BEGIN_HEADER */ #include "mbedtls/dhm.h" + +static int check_dhm_param_output( const mbedtls_mpi *expected, + const unsigned char *buffer, + size_t size, + size_t *offset ) +{ + size_t n; + mbedtls_mpi actual; + int ok = 0; + mbedtls_mpi_init( &actual ); + + ++mbedtls_test_info.step; + + TEST_ASSERT( size >= *offset + 2 ); + n = ( buffer[*offset] << 8 ) | buffer[*offset + 1]; + *offset += 2; + TEST_EQUAL( n, mbedtls_mpi_size( expected ) ); + TEST_ASSERT( size >= *offset + n ); + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &actual, buffer + *offset, n ) ); + TEST_EQUAL( 0, mbedtls_mpi_cmp_mpi( expected, &actual ) ); + *offset += n; + + ok = 1; +exit: + mbedtls_mpi_free( &actual ); + return( ok ); +} + +static int check_dhm_params( const mbedtls_dhm_context *ctx, + size_t x_size, + const unsigned char *ske, size_t ske_len ) +{ + size_t offset = 0; + + /* Check that ctx->X and ctx->GX are within range. */ + TEST_ASSERT( mbedtls_mpi_cmp_int( &ctx->X, 1 ) > 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &ctx->X, &ctx->P ) < 0 ); + TEST_ASSERT( mbedtls_mpi_size( &ctx->X ) <= x_size ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &ctx->GX, 1 ) > 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &ctx->GX, &ctx->P ) < 0 ); + + /* Check ske: it must contain P, G and G^X, each prefixed with a + * 2-byte size. */ + if( !check_dhm_param_output( &ctx->P, ske, ske_len, &offset ) ) + goto exit; + if( !check_dhm_param_output( &ctx->G, ske, ske_len, &offset ) ) + goto exit; + if( !check_dhm_param_output( &ctx->GX, ske, ske_len, &offset ) ) + goto exit; + TEST_EQUAL( offset, ske_len ); + + return( 1 ); +exit: + return( 0 ); +} + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -151,11 +207,14 @@ void dhm_do_dhm( int radix_P, char *input_P, /* * First key exchange */ + mbedtls_test_set_step( 10 ); TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &mbedtls_test_rnd_pseudo_rand, &rnd_info ) == result ); if ( result != 0 ) goto exit; + if( !check_dhm_params( &ctx_srv, x_size, ske, ske_len ) ) + goto exit; ske[ske_len++] = 0; ske[ske_len++] = 0; @@ -179,6 +238,7 @@ void dhm_do_dhm( int radix_P, char *input_P, /* Re-do calc_secret on server a few times to test update of blinding values */ for( i = 0; i < 3; i++ ) { + mbedtls_test_set_step( 20 + i ); sec_srv_len = 1000; TEST_ASSERT( mbedtls_dhm_calc_secret( &ctx_srv, sec_srv, sizeof( sec_srv ), &sec_srv_len, @@ -195,9 +255,12 @@ void dhm_do_dhm( int radix_P, char *input_P, */ p = ske; + mbedtls_test_set_step( 30 ); TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &mbedtls_test_rnd_pseudo_rand, &rnd_info ) == 0 ); + if( !check_dhm_params( &ctx_srv, x_size, ske, ske_len ) ) + goto exit; ske[ske_len++] = 0; ske[ske_len++] = 0; TEST_ASSERT( mbedtls_dhm_read_params( &ctx_cli, &p, ske + ske_len ) == 0 ); From b27db0acff5a7e1199772821840f409ae3d03d01 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 30 Mar 2021 23:33:49 +0200 Subject: [PATCH 18/48] Repeat a few DH tests Repeat a few tests that use random data. This way the code is exercised with a few different random values. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_dhm.data | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_dhm.data b/tests/suites/test_suite_dhm.data index c4795b6d3..063c1a601 100644 --- a/tests/suites/test_suite_dhm.data +++ b/tests/suites/test_suite_dhm.data @@ -1,13 +1,37 @@ Diffie-Hellman parameter validation dhm_invalid_params: -Diffie-Hellman full exchange #1 +Diffie-Hellman full exchange: 5-bit #1 dhm_do_dhm:10:"23":10:"5":0 -Diffie-Hellman full exchange #2 +Diffie-Hellman full exchange: 5-bit #2 +dhm_do_dhm:10:"23":10:"5":0 + +Diffie-Hellman full exchange: 5-bit #3 +dhm_do_dhm:10:"23":10:"5":0 + +Diffie-Hellman full exchange: 5-bit #4 +dhm_do_dhm:10:"23":10:"5":0 + +Diffie-Hellman full exchange: 5-bit #5 +dhm_do_dhm:10:"23":10:"5":0 + +Diffie-Hellman full exchange: 97-bit #1 dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 -Diffie-Hellman full exchange #3 +Diffie-Hellman full exchange: 97-bit #2 +dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit #3 +dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit #4 +dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit #5 +dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 286-bit dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":10:"9345098309485093845098340962223981329819812792137312973297123912791271":0 Diffie-Hellman trivial subgroup #1 From 2baf2b0532bcbabcabbb9fffd996ebfad1fbd893 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 30 Mar 2021 23:44:22 +0200 Subject: [PATCH 19/48] Test mbedtls_dhm_make_params with different x_size mbedtls_dhm_make_params() with x_size != size of P is not likely to be useful, but it's supported, so test it. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_dhm.data | 63 +++++++++++++++++++++------- tests/suites/test_suite_dhm.function | 7 ++-- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/tests/suites/test_suite_dhm.data b/tests/suites/test_suite_dhm.data index 063c1a601..fcfc5cf7a 100644 --- a/tests/suites/test_suite_dhm.data +++ b/tests/suites/test_suite_dhm.data @@ -1,50 +1,83 @@ +Diffie-Hellman full exchange: tiny x_size +dhm_do_dhm:10:"93450983094850938450983409623":1:10:"9345098304850938450983409622":0 + Diffie-Hellman parameter validation dhm_invalid_params: +Diffie-Hellman full exchange: 5-bit, x_size=3 +dhm_do_dhm:10:"23":3:10:"5":0 + +Diffie-Hellman full exchange: 5-bit, x_size=2 +dhm_do_dhm:10:"23":2:10:"5":0 + Diffie-Hellman full exchange: 5-bit #1 -dhm_do_dhm:10:"23":10:"5":0 +dhm_do_dhm:10:"23":1:10:"5":0 Diffie-Hellman full exchange: 5-bit #2 -dhm_do_dhm:10:"23":10:"5":0 +dhm_do_dhm:10:"23":1:10:"5":0 Diffie-Hellman full exchange: 5-bit #3 -dhm_do_dhm:10:"23":10:"5":0 +dhm_do_dhm:10:"23":1:10:"5":0 Diffie-Hellman full exchange: 5-bit #4 -dhm_do_dhm:10:"23":10:"5":0 +dhm_do_dhm:10:"23":1:10:"5":0 Diffie-Hellman full exchange: 5-bit #5 -dhm_do_dhm:10:"23":10:"5":0 +dhm_do_dhm:10:"23":1:10:"5":0 + +Diffie-Hellman full exchange: 97-bit, x_size=14 +dhm_do_dhm:10:"93450983094850938450983409623":14:10:"9345098304850938450983409622":0 Diffie-Hellman full exchange: 97-bit #1 -dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 +dhm_do_dhm:10:"93450983094850938450983409623":13:10:"9345098304850938450983409622":0 Diffie-Hellman full exchange: 97-bit #2 -dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 +dhm_do_dhm:10:"93450983094850938450983409623":13:10:"9345098304850938450983409622":0 Diffie-Hellman full exchange: 97-bit #3 -dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 +dhm_do_dhm:10:"93450983094850938450983409623":13:10:"9345098304850938450983409622":0 Diffie-Hellman full exchange: 97-bit #4 -dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 +dhm_do_dhm:10:"93450983094850938450983409623":13:10:"9345098304850938450983409622":0 Diffie-Hellman full exchange: 97-bit #5 -dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 +dhm_do_dhm:10:"93450983094850938450983409623":13:10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit, x_size=12 +dhm_do_dhm:10:"93450983094850938450983409623":12:10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit, x_size=11 +dhm_do_dhm:10:"93450983094850938450983409623":11:10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit, x_size=1 #1 +dhm_do_dhm:10:"93450983094850938450983409623":1:10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit, x_size=1 #2 +dhm_do_dhm:10:"93450983094850938450983409623":1:10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit, x_size=1 #3 +dhm_do_dhm:10:"93450983094850938450983409623":1:10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit, x_size=1 #4 +dhm_do_dhm:10:"93450983094850938450983409623":1:10:"9345098304850938450983409622":0 + +Diffie-Hellman full exchange: 97-bit, x_size=1 #5 +dhm_do_dhm:10:"93450983094850938450983409623":1:10:"9345098304850938450983409622":0 Diffie-Hellman full exchange: 286-bit -dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":10:"9345098309485093845098340962223981329819812792137312973297123912791271":0 +dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":36:10:"9345098309485093845098340962223981329819812792137312973297123912791271":0 Diffie-Hellman trivial subgroup #1 -dhm_do_dhm:10:"23":10:"1":MBEDTLS_ERR_DHM_BAD_INPUT_DATA +dhm_do_dhm:10:"23":1:10:"1":MBEDTLS_ERR_DHM_BAD_INPUT_DATA Diffie-Hellman trivial subgroup #2 -dhm_do_dhm:10:"23":10:"-1":MBEDTLS_ERR_DHM_BAD_INPUT_DATA +dhm_do_dhm:10:"23":1:10:"-1":MBEDTLS_ERR_DHM_BAD_INPUT_DATA Diffie-Hellman small modulus -dhm_do_dhm:10:"3":10:"5":MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED +dhm_do_dhm:10:"3":1:10:"5":MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED Diffie-Hellman zero modulus -dhm_do_dhm:10:"0":10:"5":MBEDTLS_ERR_DHM_BAD_INPUT_DATA +dhm_do_dhm:10:"0":1:10:"5":MBEDTLS_ERR_DHM_BAD_INPUT_DATA Diffie-Hellman MPI_MAX_SIZE modulus dhm_make_public:MBEDTLS_MPI_MAX_SIZE:10:"5":0 diff --git a/tests/suites/test_suite_dhm.function b/tests/suites/test_suite_dhm.function index b9dcde580..edd96989a 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -171,7 +171,7 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void dhm_do_dhm( int radix_P, char *input_P, +void dhm_do_dhm( int radix_P, char *input_P, int x_size, int radix_G, char *input_G, int result ) { mbedtls_dhm_context ctx_srv; @@ -185,7 +185,7 @@ void dhm_do_dhm( int radix_P, char *input_P, size_t pub_cli_len = 0; size_t sec_srv_len; size_t sec_cli_len; - int x_size, i; + int i; mbedtls_test_rnd_pseudo_info rnd_info; mbedtls_dhm_init( &ctx_srv ); @@ -201,8 +201,7 @@ void dhm_do_dhm( int radix_P, char *input_P, */ TEST_ASSERT( mbedtls_mpi_read_string( &ctx_srv.P, radix_P, input_P ) == 0 ); TEST_ASSERT( mbedtls_mpi_read_string( &ctx_srv.G, radix_G, input_G ) == 0 ); - x_size = mbedtls_mpi_size( &ctx_srv.P ); - pub_cli_len = x_size; + pub_cli_len = mbedtls_mpi_size( &ctx_srv.P ); /* * First key exchange From cb660f2bdaf8572243462bcd89f8d778f91d9ea4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:35:13 +0200 Subject: [PATCH 20/48] DHM refactoring: unify mbedtls_dhm_make_{params,public} Unify the common parts of mbedtls_dhm_make_params and mbedtls_dhm_make_public. No intended behavior change, except that the exact error code may change in some corner cases which are too exotic for the existing unit tests. Signed-off-by: Gilles Peskine --- library/dhm.c | 86 +++++++++++++++++++++++---------------------------- 1 file changed, 39 insertions(+), 47 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index 9758af787..f2ad551ce 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -150,21 +150,11 @@ int mbedtls_dhm_read_params( mbedtls_dhm_context *ctx, return( 0 ); } -/* - * Setup and write the ServerKeyExchange parameters - */ -int mbedtls_dhm_make_params( mbedtls_dhm_context *ctx, int x_size, - unsigned char *output, size_t *olen, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +static int dhm_make_common( mbedtls_dhm_context *ctx, int x_size, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) { int ret, count = 0; - size_t n1, n2, n3; - unsigned char *p; - DHM_VALIDATE_RET( ctx != NULL ); - DHM_VALIDATE_RET( output != NULL ); - DHM_VALIDATE_RET( olen != NULL ); - DHM_VALIDATE_RET( f_rng != NULL ); if( mbedtls_mpi_cmp_int( &ctx->P, 0 ) == 0 ) return( MBEDTLS_ERR_DHM_BAD_INPUT_DATA ); @@ -193,6 +183,30 @@ int mbedtls_dhm_make_params( mbedtls_dhm_context *ctx, int x_size, if( ( ret = dhm_check_range( &ctx->GX, &ctx->P ) ) != 0 ) return( ret ); +cleanup: + return( ret ); +} + +/* + * Setup and write the ServerKeyExchange parameters + */ +int mbedtls_dhm_make_params( mbedtls_dhm_context *ctx, int x_size, + unsigned char *output, size_t *olen, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + int ret; + size_t n1, n2, n3; + unsigned char *p; + DHM_VALIDATE_RET( ctx != NULL ); + DHM_VALIDATE_RET( output != NULL ); + DHM_VALIDATE_RET( olen != NULL ); + DHM_VALIDATE_RET( f_rng != NULL ); + + ret = dhm_make_common( ctx, x_size, f_rng, p_rng ); + if( ret != 0 ) + goto cleanup; + /* * export P, G, GX */ @@ -220,11 +234,9 @@ int mbedtls_dhm_make_params( mbedtls_dhm_context *ctx, int x_size, ctx->len = n1; cleanup: - - if( ret != 0 ) - return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED, ret ) ); - - return( 0 ); + if( ret != 0 && ret > -128 ) + ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED, ret ); + return( ret ); } /* @@ -276,7 +288,7 @@ int mbedtls_dhm_make_public( mbedtls_dhm_context *ctx, int x_size, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - int ret, count = 0; + int ret; DHM_VALIDATE_RET( ctx != NULL ); DHM_VALIDATE_RET( output != NULL ); DHM_VALIDATE_RET( f_rng != NULL ); @@ -284,38 +296,18 @@ int mbedtls_dhm_make_public( mbedtls_dhm_context *ctx, int x_size, if( olen < 1 || olen > ctx->len ) return( MBEDTLS_ERR_DHM_BAD_INPUT_DATA ); - if( mbedtls_mpi_cmp_int( &ctx->P, 0 ) == 0 ) - return( MBEDTLS_ERR_DHM_BAD_INPUT_DATA ); - - /* - * generate X and calculate GX = G^X mod P - */ - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &ctx->X, x_size, f_rng, p_rng ) ); - - while( mbedtls_mpi_cmp_mpi( &ctx->X, &ctx->P ) >= 0 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &ctx->X, 1 ) ); - - if( count++ > 10 ) - return( MBEDTLS_ERR_DHM_MAKE_PUBLIC_FAILED ); - } - while( dhm_check_range( &ctx->X, &ctx->P ) != 0 ); - - MBEDTLS_MPI_CHK( mbedtls_mpi_exp_mod( &ctx->GX, &ctx->G, &ctx->X, - &ctx->P , &ctx->RP ) ); - - if( ( ret = dhm_check_range( &ctx->GX, &ctx->P ) ) != 0 ) - return( ret ); + ret = dhm_make_common( ctx, x_size, f_rng, p_rng ); + if( ret == MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED ) + return( MBEDTLS_ERR_DHM_MAKE_PUBLIC_FAILED ); + if( ret != 0 ) + goto cleanup; MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( &ctx->GX, output, olen ) ); cleanup: - - if( ret != 0 ) - return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_DHM_MAKE_PUBLIC_FAILED, ret ) ); - - return( 0 ); + if( ret != 0 && ret > -128 ) + ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_DHM_MAKE_PUBLIC_FAILED, ret ); + return( ret ); } /* From 7b2b66e3f3cbe8b56288d741ac5909f86d1994b8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:50:57 +0200 Subject: [PATCH 21/48] DHM blinding: don't accept P-1 as a blinding value P-1 is as bad as 1 as a blinding value. Don't accept it. The chance that P-1 would be randomly generated is infinitesimal, so this is not a practical issue, but it makes the code cleaner. It was inconsistent to accept P-1 as a blinding value but not as a private key. Signed-off-by: Gilles Peskine --- library/dhm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index f2ad551ce..5e0864b32 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -329,7 +329,7 @@ static int dhm_random_below( mbedtls_mpi *R, const mbedtls_mpi *M, if( count++ > 10 ) return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ); } - while( mbedtls_mpi_cmp_int( R, 1 ) <= 0 ); + while( dhm_check_range( R, M ) != 0 ); cleanup: return( ret ); @@ -382,7 +382,7 @@ static int dhm_update_blinding( mbedtls_dhm_context *ctx, * We need to generate blinding values from scratch */ - /* Vi = random( 2, P-1 ) */ + /* Vi = random( 2, P-2 ) */ MBEDTLS_MPI_CHK( dhm_random_below( &ctx->Vi, &ctx->P, f_rng, p_rng ) ); /* Vf = Vi^-X mod P From 17f1a265933b8090a38d16f247ac5234679456a1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:48:14 +0200 Subject: [PATCH 22/48] DHM refactoring: use dhm_random_below in dhm_make_common dhm_make_common includes a piece of code that is identical to dhm_random_below except for returning a different error code in one case. Call dhm_random_below instead of repeating the code. Signed-off-by: Gilles Peskine --- library/dhm.c | 75 ++++++++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index 5e0864b32..7168f26c2 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -150,29 +150,55 @@ int mbedtls_dhm_read_params( mbedtls_dhm_context *ctx, return( 0 ); } +/* + * Pick a random R in the range [2, M) for blinding or key generation. + */ +static int dhm_random_below( mbedtls_mpi *R, const mbedtls_mpi *M, + int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) +{ + int ret, count; + + count = 0; + do + { + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( R, mbedtls_mpi_size( M ), f_rng, p_rng ) ); + + while( mbedtls_mpi_cmp_mpi( R, M ) >= 0 ) + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( R, 1 ) ); + + if( count++ > 10 ) + return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ); + } + while( dhm_check_range( R, M ) != 0 ); + +cleanup: + return( ret ); +} + static int dhm_make_common( mbedtls_dhm_context *ctx, int x_size, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - int ret, count = 0; + int ret = 0; if( mbedtls_mpi_cmp_int( &ctx->P, 0 ) == 0 ) return( MBEDTLS_ERR_DHM_BAD_INPUT_DATA ); + if( x_size < 0 ) + return( MBEDTLS_ERR_DHM_BAD_INPUT_DATA ); - /* - * Generate X as large as possible ( < P ) - */ - do + if( (unsigned) x_size < mbedtls_mpi_size( &ctx->P ) ) { MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &ctx->X, x_size, f_rng, p_rng ) ); - - while( mbedtls_mpi_cmp_mpi( &ctx->X, &ctx->P ) >= 0 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &ctx->X, 1 ) ); - - if( count++ > 10 ) - return( MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED ); } - while( dhm_check_range( &ctx->X, &ctx->P ) != 0 ); + else + { + /* Generate X as large as possible ( <= P - 2 ) */ + ret = dhm_random_below( &ctx->X, &ctx->P, f_rng, p_rng ); + if( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) + return( MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED ); + if( ret != 0 ) + return( ret ); + } /* * Calculate GX = G^X mod P @@ -310,31 +336,6 @@ cleanup: return( ret ); } -/* - * Pick a random R in the range [2, M) for blinding purposes - */ -static int dhm_random_below( mbedtls_mpi *R, const mbedtls_mpi *M, - int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) -{ - int ret, count; - - count = 0; - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( R, mbedtls_mpi_size( M ), f_rng, p_rng ) ); - - while( mbedtls_mpi_cmp_mpi( R, M ) >= 0 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( R, 1 ) ); - - if( count++ > 10 ) - return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ); - } - while( dhm_check_range( R, M ) != 0 ); - -cleanup: - return( ret ); -} - /* * Use the blinding method and optimisation suggested in section 10 of: From 8e38acc9a551a6d061ed409c993ab54020e60671 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:56:43 +0200 Subject: [PATCH 23/48] dhm_check_range: microoptimization No need to build a bignum for the value 2. Signed-off-by: Gilles Peskine --- library/dhm.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index 7168f26c2..befa99850 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -100,22 +100,21 @@ static int dhm_read_bignum( mbedtls_mpi *X, */ static int dhm_check_range( const mbedtls_mpi *param, const mbedtls_mpi *P ) { - mbedtls_mpi L, U; + mbedtls_mpi U; int ret = 0; - mbedtls_mpi_init( &L ); mbedtls_mpi_init( &U ); + mbedtls_mpi_init( &U ); - MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &L, 2 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_sub_int( &U, P, 2 ) ); - if( mbedtls_mpi_cmp_mpi( param, &L ) < 0 || + if( mbedtls_mpi_cmp_int( param, 2 ) < 0 || mbedtls_mpi_cmp_mpi( param, &U ) > 0 ) { ret = MBEDTLS_ERR_DHM_BAD_INPUT_DATA; } cleanup: - mbedtls_mpi_free( &L ); mbedtls_mpi_free( &U ); + mbedtls_mpi_free( &U ); return( ret ); } From da7ee01589aff9d4dd432de7206f843f7175ff8b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 23:04:50 +0200 Subject: [PATCH 24/48] DHM: use mbedtls_mpi_random for blinding and key generation Instead of generating blinding values and keys in a not-quite-uniform way (https://github.com/ARMmbed/mbedtls/issues/4245) with copy-pasted code, use mbedtls_mpi_random(). Signed-off-by: Gilles Peskine --- library/dhm.c | 18 ++++-------------- tests/suites/test_suite_dhm.data | 2 +- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index befa99850..e8055be71 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -150,25 +150,15 @@ int mbedtls_dhm_read_params( mbedtls_dhm_context *ctx, } /* - * Pick a random R in the range [2, M) for blinding or key generation. + * Pick a random R in the range [2, M-2] for blinding or key generation. */ static int dhm_random_below( mbedtls_mpi *R, const mbedtls_mpi *M, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - int ret, count; + int ret; - count = 0; - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( R, mbedtls_mpi_size( M ), f_rng, p_rng ) ); - - while( mbedtls_mpi_cmp_mpi( R, M ) >= 0 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( R, 1 ) ); - - if( count++ > 10 ) - return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ); - } - while( dhm_check_range( R, M ) != 0 ); + MBEDTLS_MPI_CHK( mbedtls_mpi_random( R, 3, M, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_sub_int( R, R, 1 ) ); cleanup: return( ret ); diff --git a/tests/suites/test_suite_dhm.data b/tests/suites/test_suite_dhm.data index fcfc5cf7a..773fe3b9c 100644 --- a/tests/suites/test_suite_dhm.data +++ b/tests/suites/test_suite_dhm.data @@ -74,7 +74,7 @@ Diffie-Hellman trivial subgroup #2 dhm_do_dhm:10:"23":1:10:"-1":MBEDTLS_ERR_DHM_BAD_INPUT_DATA Diffie-Hellman small modulus -dhm_do_dhm:10:"3":1:10:"5":MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED +dhm_do_dhm:10:"3":1:10:"5":MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED+MBEDTLS_ERR_MPI_BAD_INPUT_DATA Diffie-Hellman zero modulus dhm_do_dhm:10:"0":1:10:"5":MBEDTLS_ERR_DHM_BAD_INPUT_DATA From 9367f4b1d9645bc1fc6b8ce070d9d5dd57e10f16 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 23:12:35 +0200 Subject: [PATCH 25/48] Add changelog entry for non-uniform MPI random generation Fix #4245. Signed-off-by: Gilles Peskine --- ChangeLog.d/random-range.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/random-range.txt diff --git a/ChangeLog.d/random-range.txt b/ChangeLog.d/random-range.txt new file mode 100644 index 000000000..dc35ec6c6 --- /dev/null +++ b/ChangeLog.d/random-range.txt @@ -0,0 +1,4 @@ +Security +* Fix a bias in the generation of finite-field Diffie-Hellman-Merkle (DHM) + private keys and of blinding values for DHM and elliptic curves (ECP) + computations. Reported by FlorianF89 in #4245. From 1a7df4eda092a1b200c0151051f954f66dad7059 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 1 Apr 2021 15:57:18 +0200 Subject: [PATCH 26/48] Fix mbedtls_mpi_random when N has leading zeros mbedtls_mpi_random() uses mbedtls_mpi_cmp_mpi_ct(), which requires its two arguments to have the same storage size. This was not the case when the upper bound passed to mbedtls_mpi_random() had leading zero limbs. Fix this by forcing the result MPI to the desired size. Since this is not what mbedtls_mpi_fill_random() does, don't call it from mbedtls_mpi_random(), but instead call a new auxiliary function. Add tests to cover this and other conditions with varying sizes for the two arguments. Signed-off-by: Gilles Peskine --- library/bignum.c | 46 +++++++++++++++++++++++----- tests/suites/test_suite_mpi.data | 27 ++++++++++++++++ tests/suites/test_suite_mpi.function | 23 ++++++++++++++ 3 files changed, 88 insertions(+), 8 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index fba9acc5b..25fb9ae54 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2395,6 +2395,30 @@ cleanup: return( ret ); } +/* Fill X with n_bytes random bytes. + * X must already have room for those bytes. + * n_bytes must not be 0. + */ +static int mpi_fill_random_internal( + mbedtls_mpi *X, size_t n_bytes, + int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + const size_t limbs = CHARS_TO_LIMBS( n_bytes ); + const size_t overhead = ( limbs * ciL ) - n_bytes; + + if( X->n < limbs ) + return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); + + MBEDTLS_MPI_CHK( f_rng( p_rng, (unsigned char *) X->p + overhead, n_bytes ) ); + mpi_bigendian_to_host( X->p, limbs ); + +cleanup: + return( ret ); +} + /* * Fill X with size bytes of random. * @@ -2408,8 +2432,6 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t const limbs = CHARS_TO_LIMBS( size ); - size_t const overhead = ( limbs * ciL ) - size; - unsigned char *Xp; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( f_rng != NULL ); @@ -2421,12 +2443,10 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, mbedtls_mpi_init( X ); MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); } - MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); + if( size == 0 ) + return( 0 ); - Xp = (unsigned char*) X->p; - MBEDTLS_MPI_CHK( f_rng( p_rng, Xp + overhead, size ) ); - - mpi_bigendian_to_host( X->p, limbs ); + ret = mpi_fill_random_internal( X, size, f_rng, p_rng ); cleanup: return( ret ); @@ -2450,6 +2470,16 @@ int mbedtls_mpi_random( mbedtls_mpi *X, if( mbedtls_mpi_cmp_int( N, min ) <= 0 ) return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + /* Ensure that target MPI has exactly the same number of limbs + * as the upper bound, even if the upper bound has leading zeros. + * This is necessary for the mbedtls_mpi_lt_mpi_ct() check. */ + if( X->n != N->n ) + { + mbedtls_mpi_free( X ); + mbedtls_mpi_init( X ); + MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, N->n ) ); + } + /* * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) * when f_rng is a suitably parametrized instance of HMAC_DRBG: @@ -2460,7 +2490,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X, */ do { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( X, n_bytes, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mpi_fill_random_internal( X, n_bytes, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( X, 8 * n_bytes - n_bits ) ); /* diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index fb2c553d0..3488854e2 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1132,6 +1132,33 @@ mpi_random_many:1:"04":1000 MPI random in range: 3..4 mpi_random_many:1:"04":1000 +MPI random in range: smaller result +mpi_random_grown:1:"aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbb":1 + +MPI random in range: same size result (32-bit limbs) +mpi_random_grown:1:"aaaaaaaaaaaaaaaa":2 + +MPI random in range: same size result (64-bit limbs) +mpi_random_grown:1:"aaaaaaaaaaaaaaaa":1 + +MPI random in range: larger result +mpi_random_grown:1:"aaaaaaaaaaaaaaaa":3 + +MPI random in range: leading 0 limb in upper bound #0 +mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":0 + +MPI random in range: leading 0 limb in upper bound #1 +mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":1 + +MPI random in range: leading 0 limb in upper bound #2 +mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":2 + +MPI random in range: leading 0 limb in upper bound #3 +mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":3 + +MPI random in range: leading 0 limb in upper bound #4 +mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":4 + MPI random bad arguments: min < 0 mpi_random_fail:-1:"04":MBEDTLS_ERR_MPI_BAD_INPUT_DATA diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 0ca6b6439..f3c9107c3 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -1537,6 +1537,29 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mpi_random_grown( int min, data_t *bound_bytes, int nlimbs ) +{ + mbedtls_mpi upper_bound; + mbedtls_mpi result; + + mbedtls_mpi_init( &upper_bound ); + mbedtls_mpi_init( &result ); + + TEST_EQUAL( 0, mbedtls_mpi_grow( &result, nlimbs ) ); + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &upper_bound, + bound_bytes->x, bound_bytes->len ) ); + TEST_EQUAL( 0, mbedtls_mpi_random( &result, min, &upper_bound, + mbedtls_test_rnd_std_rand, NULL ) ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &result, &upper_bound ) < 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &result, min ) >= 0 ); + +exit: + mbedtls_mpi_free( &upper_bound ); + mbedtls_mpi_free( &result ); +} +/* END_CASE */ + /* BEGIN_CASE */ void mpi_random_fail( int min, data_t *bound_bytes, int expected_ret ) { From 422e867acb43a9ca537d0cb20c6c656e4e1167e8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 2 Apr 2021 00:02:27 +0200 Subject: [PATCH 27/48] MPI random: add unit tests with a previously nonzero value Add unit tests for mbedtls_mpi_fill_random() and mbedtls_mpi_random() when the resulting MPI object previously had a nonzero value. I wrote those to catch a bug that I introduced during the development of mbedtls_mpi_random() (but does not appear in a committed version). Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 88 ++++++++++++++++++++-------- tests/suites/test_suite_mpi.function | 23 +++++++- 2 files changed, 86 insertions(+), 25 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 3488854e2..6cd62f10c 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -992,46 +992,76 @@ Test bit set (Invalid bit value) mbedtls_mpi_set_bit:16:"00":5:2:16:"00":MBEDTLS_ERR_MPI_BAD_INPUT_DATA Fill random: 0 bytes -mpi_fill_random:0:0:0 +mpi_fill_random:0:0:0:0 Fill random: 1 byte, good -mpi_fill_random:1:1:0 +mpi_fill_random:1:1:0:0 Fill random: 2 bytes, good, no leading zero -mpi_fill_random:2:2:0 +mpi_fill_random:2:2:0:0 Fill random: 2 bytes, good, 1 leading zero -mpi_fill_random:2:256:0 +mpi_fill_random:2:256:0:0 Fill random: MAX_SIZE - 7, good -mpi_fill_random:MBEDTLS_MPI_MAX_SIZE - 7:MBEDTLS_MPI_MAX_SIZE - 7:0 +mpi_fill_random:MBEDTLS_MPI_MAX_SIZE - 7:MBEDTLS_MPI_MAX_SIZE - 7:0:0 Fill random: MAX_SIZE, good -mpi_fill_random:MBEDTLS_MPI_MAX_SIZE:MBEDTLS_MPI_MAX_SIZE:0 +mpi_fill_random:MBEDTLS_MPI_MAX_SIZE:MBEDTLS_MPI_MAX_SIZE:0:0 + +Fill random: 0 bytes, previously small >0 +mpi_fill_random:0:0:1:0 + +Fill random: 0 bytes, previously small <0 +mpi_fill_random:0:0:-1:0 + +Fill random: 0 bytes, previously large >0 +mpi_fill_random:0:0:65:0 + +Fill random: 0 bytes, previously large <0 +mpi_fill_random:0:0:-65:0 + +Fill random: 1 byte, previously small >0 +mpi_fill_random:1:1:1:0 + +Fill random: 1 byte, previously small <0 +mpi_fill_random:1:1:-1:0 + +Fill random: 1 byte, previously large >0 +mpi_fill_random:1:1:65:0 + +Fill random: 1 byte, previously large <0 +mpi_fill_random:1:1:-65:0 + +Fill random: 9 bytes, previously small >0 +mpi_fill_random:1:1:1:0 + +Fill random: 9 bytes, previously small <0 +mpi_fill_random:1:1:-1:0 Fill random: 1 byte, RNG failure -mpi_fill_random:1:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:1:0:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: 2 bytes, RNG failure after 1 byte -mpi_fill_random:2:1:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:2:1:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: 4 bytes, RNG failure after 3 bytes -mpi_fill_random:4:3:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:4:3:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: 8 bytes, RNG failure after 7 bytes -mpi_fill_random:8:7:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:8:7:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: 16 bytes, RNG failure after 1 bytes -mpi_fill_random:16:1:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:16:1:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: 16 bytes, RNG failure after 8 bytes -mpi_fill_random:16:8:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:16:8:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: 16 bytes, RNG failure after 15 bytes -mpi_fill_random:16:15:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:16:15:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: MAX_SIZE bytes, RNG failure after MAX_SIZE-1 bytes -mpi_fill_random:MBEDTLS_MPI_MAX_SIZE:MBEDTLS_MPI_MAX_SIZE-1:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +mpi_fill_random:MBEDTLS_MPI_MAX_SIZE:MBEDTLS_MPI_MAX_SIZE-1:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED MPI random in range: 1..4 mpi_random_many:1:"04":1000 @@ -1133,31 +1163,43 @@ MPI random in range: 3..4 mpi_random_many:1:"04":1000 MPI random in range: smaller result -mpi_random_grown:1:"aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbb":1 +mpi_random_sizes:1:"aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbb":1:0 MPI random in range: same size result (32-bit limbs) -mpi_random_grown:1:"aaaaaaaaaaaaaaaa":2 +mpi_random_sizes:1:"aaaaaaaaaaaaaaaa":2:0 MPI random in range: same size result (64-bit limbs) -mpi_random_grown:1:"aaaaaaaaaaaaaaaa":1 +mpi_random_sizes:1:"aaaaaaaaaaaaaaaa":1:0 MPI random in range: larger result -mpi_random_grown:1:"aaaaaaaaaaaaaaaa":3 +mpi_random_sizes:1:"aaaaaaaaaaaaaaaa":3:0 MPI random in range: leading 0 limb in upper bound #0 -mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":0 +mpi_random_sizes:1:"00aaaaaaaaaaaaaaaa":0:0 MPI random in range: leading 0 limb in upper bound #1 -mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":1 +mpi_random_sizes:1:"00aaaaaaaaaaaaaaaa":1:0 MPI random in range: leading 0 limb in upper bound #2 -mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":2 +mpi_random_sizes:1:"00aaaaaaaaaaaaaaaa":2:0 MPI random in range: leading 0 limb in upper bound #3 -mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":3 +mpi_random_sizes:1:"00aaaaaaaaaaaaaaaa":3:0 MPI random in range: leading 0 limb in upper bound #4 -mpi_random_grown:1:"00aaaaaaaaaaaaaaaa":4 +mpi_random_sizes:1:"00aaaaaaaaaaaaaaaa":4:0 + +MPI random in range: previously small >0 +mpi_random_sizes:1:"1234567890":4:1 + +MPI random in range: previously small <0 +mpi_random_sizes:1:"1234567890":4:-1 + +MPI random in range: previously large >0 +mpi_random_sizes:1:"1234":4:65 + +MPI random in range: previously large <0 +mpi_random_sizes:1:"1234":4:-65 MPI random bad arguments: min < 0 mpi_random_fail:-1:"04":MBEDTLS_ERR_MPI_BAD_INPUT_DATA diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index f3c9107c3..e82fe99b5 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -1400,13 +1400,23 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mpi_fill_random( int wanted_bytes, int rng_bytes, int expected_ret ) +void mpi_fill_random( int wanted_bytes, int rng_bytes, + int before, int expected_ret ) { mbedtls_mpi X; int ret; size_t bytes_left = rng_bytes; mbedtls_mpi_init( &X ); + if( before != 0 ) + { + /* Set X to sign(before) * 2^(|before|-1) */ + TEST_ASSERT( mbedtls_mpi_lset( &X, before > 0 ? 1 : -1 ) == 0 ); + if( before < 0 ) + before = - before; + TEST_ASSERT( mbedtls_mpi_shift_l( &X, before - 1 ) == 0 ); + } + ret = mbedtls_mpi_fill_random( &X, wanted_bytes, f_rng_bytes_left, &bytes_left ); TEST_ASSERT( ret == expected_ret ); @@ -1538,7 +1548,7 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mpi_random_grown( int min, data_t *bound_bytes, int nlimbs ) +void mpi_random_sizes( int min, data_t *bound_bytes, int nlimbs, int before ) { mbedtls_mpi upper_bound; mbedtls_mpi result; @@ -1546,6 +1556,15 @@ void mpi_random_grown( int min, data_t *bound_bytes, int nlimbs ) mbedtls_mpi_init( &upper_bound ); mbedtls_mpi_init( &result ); + if( before != 0 ) + { + /* Set result to sign(before) * 2^(|before|-1) */ + TEST_ASSERT( mbedtls_mpi_lset( &result, before > 0 ? 1 : -1 ) == 0 ); + if( before < 0 ) + before = - before; + TEST_ASSERT( mbedtls_mpi_shift_l( &result, before - 1 ) == 0 ); + } + TEST_EQUAL( 0, mbedtls_mpi_grow( &result, nlimbs ) ); TEST_EQUAL( 0, mbedtls_mpi_read_binary( &upper_bound, bound_bytes->x, bound_bytes->len ) ); From eedefa5627fb331830b613d8141da77beea5008a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 19:50:04 +0200 Subject: [PATCH 28/48] Better document and slightly simplify >>2^n heuristic Slightly simplify is_significantly_above_a_power_of_2() to make it easier to understand: * Remove the explicit negative answer for x <= 4. The only functional difference this makes is that is_significantly_above_a_power_of_2(3) is now true. * Shift the most significant bit of x to position 8 rather than 15. This makes the final comparison easier to explain. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.function | 38 ++++++++++++++++++---------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index e82fe99b5..170967752 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -64,38 +64,48 @@ static int f_rng_bytes_left( void *state, unsigned char *buf, size_t len ) return( 0 ); } -/* Test whether bytes represents (in big-endian base 256) a number B that - * is "significantly" above a power of 2, which is defined as follows. - * Let n be the integer such that 2^n <= B < 2^{n+1}. B is significantly - * above a power of 2 if (B - 2^n) / 2^n is not negligible. "Negligible" - * is defined as having a negligible chance that if you draw an integer - * in the range [1, B-1] K times, the number will always be less than 2^n, - * where K is the iteration count passed to genkey_sw_many. +/* Test whether bytes represents (in big-endian base 256) a number b that + * is significantly above a power of 2. That is, b must not have a long run + * of unset bits after the most significant bit. + * + * Let n be the bit-size of b, i.e. the integer such that 2^n <= b < 2^{n+1}. + * This function returns 1 if, when drawing a number between 0 and b, + * the probability that this number is at least 2^n is not negligible. + * This probability is (b - 2^n) / b and this function checks that this + * number is above some threshold A. The threshold value is heuristic and + * based on the needs of mpi_random_many(). */ static int is_significantly_above_a_power_of_2( data_t *bytes ) { const uint8_t *p = bytes->x; size_t len = bytes->len; unsigned x; + + /* Skip leading null bytes */ while( len > 0 && p[0] == 0 ) { ++p; --len; } + /* 0 is not significantly above a power of 2 */ if( len == 0 ) return( 0 ); - else if( len == 1 ) + /* Extract the (up to) 2 most significant bytes */ + if( len == 1 ) x = p[0]; else x = ( p[0] << 8 ) | p[1]; - if( x <= 4 ) - return( 0 ); + /* Shift the most significant bit of x to position 8 and mask it out */ + while( ( x & 0xfe00 ) != 0 ) + x >>= 1; + x &= 0x00ff; - while( ( x & 0x8000 ) == 0 ) - x <<= 1; - x &= 0x7fff; - return( x >= 0x1000 ); + /* At this point, x = floor((b - 2^n) / 2^(n-8)). b is significantly above + * a power of 2 iff x is significantly above 0 compared to 2^8. + * Testing x >= 2^4 amounts to picking A = 1/16 in the function + * description above. */ + return( x >= 0x10 ); } /* END_HEADER */ From ee966c4ae4afea6ce1e5b107001d1205cad7acae Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 19:59:21 +0200 Subject: [PATCH 29/48] Contextualize comment about mbedtls_mpi_random retries This comment is no longer in the specific context of generating a random point on an elliptic curve. Signed-off-by: Gilles Peskine --- library/bignum.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 25fb9ae54..b504752e6 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2498,9 +2498,11 @@ int mbedtls_mpi_random( mbedtls_mpi *X, * a probability 1/2 of being 0, and then the result will be < N), * so after 30 tries failure probability is a most 2**(-30). * - * For most curves, 1 try is enough with overwhelming probability, - * since N starts with a lot of 1s in binary, but some curves - * such as secp224k1 are actually very close to the worst case. + * When N is just below a power of 2, as is the case when generating + * a random point on most elliptic curves, 1 try is enough with + * overwhelming probability. When N is just above a power of 2, + * as when generating a random point on secp224k1, each try has + * a probability of failing that is almost 1/2. */ if( ++count > 30 ) { From fbb90098e8d0ec5573fdf3c469afa2c52337376a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 20:00:57 +0200 Subject: [PATCH 30/48] Fix copypasta in test case description Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 6cd62f10c..aa58c94cd 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1157,10 +1157,10 @@ MPI random in range: 0..4 mpi_random_many:0:"04":1000 MPI random in range: 2..4 -mpi_random_many:1:"04":1000 +mpi_random_many:2:"04":1000 MPI random in range: 3..4 -mpi_random_many:1:"04":1000 +mpi_random_many:3:"04":1000 MPI random in range: smaller result mpi_random_sizes:1:"aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbb":1:0 From 951b5695e3a4303ab3b3a8e63e46d549f0f1311f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 20:44:04 +0200 Subject: [PATCH 31/48] MPI random test: Add a few more small-range tests Do more iterations with small values. This makes it more likely that a mistake on bounds will be detected. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index aa58c94cd..cf25e273d 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1063,6 +1063,12 @@ mpi_fill_random:16:15:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED Fill random: MAX_SIZE bytes, RNG failure after MAX_SIZE-1 bytes mpi_fill_random:MBEDTLS_MPI_MAX_SIZE:MBEDTLS_MPI_MAX_SIZE-1:0:MBEDTLS_ERR_ENTROPY_SOURCE_FAILED +MPI random in range: 1..2 +mpi_random_many:1:"02":1000 + +MPI random in range: 1..3 +mpi_random_many:1:"03":1000 + MPI random in range: 1..4 mpi_random_many:1:"04":1000 @@ -1153,14 +1159,17 @@ mpi_random_many:1:"01000000000000000001":100 MPI random in range: 1..2^72+2^63 mpi_random_many:1:"01800000000000000000":100 +MPI random in range: 0..1 +mpi_random_many:0:"04":10000 + MPI random in range: 0..4 -mpi_random_many:0:"04":1000 +mpi_random_many:0:"04":10000 MPI random in range: 2..4 -mpi_random_many:2:"04":1000 +mpi_random_many:2:"04":10000 MPI random in range: 3..4 -mpi_random_many:3:"04":1000 +mpi_random_many:3:"04":10000 MPI random in range: smaller result mpi_random_sizes:1:"aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbb":1:0 From d463edf8c5345e72315bfcb266af7fae11c5d275 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 20:45:05 +0200 Subject: [PATCH 32/48] MPI random test: fix small-range test stats check when min > 1 Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 170967752..a2d312f6a 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -1518,7 +1518,7 @@ void mpi_random_many( int min, data_t *bound_bytes, int iterations ) if( full_stats ) { - for( b = 1; b < stats_len; b++ ) + for( b = min; b < stats_len; b++ ) { mbedtls_test_set_step( 1000000 + b ); /* Assert that each value has been reached at least once. From 0ad640ab8344cc30987a219c2fa7fd66f100937d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 20:47:07 +0200 Subject: [PATCH 33/48] MPI random test: Add test cases with lower_bound > upper_bound Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index cf25e273d..66339c9da 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1219,6 +1219,15 @@ mpi_random_fail:0:"00":MBEDTLS_ERR_MPI_BAD_INPUT_DATA MPI random bad arguments: min = N = 1 mpi_random_fail:1:"01":MBEDTLS_ERR_MPI_BAD_INPUT_DATA +MPI random bad arguments: min > N = 0 +mpi_random_fail:1:"00":MBEDTLS_ERR_MPI_BAD_INPUT_DATA + +MPI random bad arguments: min > N = 1 +mpi_random_fail:2:"01":MBEDTLS_ERR_MPI_BAD_INPUT_DATA + +MPI random bad arguments: min > N = 001 +mpi_random_fail:2:"000000000000000001":MBEDTLS_ERR_MPI_BAD_INPUT_DATA + MPI Selftest depends_on:MBEDTLS_SELF_TEST mpi_selftest: From e5381686ef4cc089b4209a7fcd2258bea6805570 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 21:23:25 +0200 Subject: [PATCH 34/48] MPI random test: use more iterations for small numbers In real life, min << N and the probability that mbedtls_mpi_random() fails to find a suitable value after 30 iterations is less than one in a billion. But at least for testing purposes, it's useful to not outright reject "silly" small values of N, and for such values, 30 iterations is not enough to have a good probability of success. Pick 250 iterations, which is enough for cases like (min=3, N=4), but not for cases like (min=255, N=256). Signed-off-by: Gilles Peskine --- library/bignum.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index b504752e6..c77d188ee 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2460,7 +2460,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X, { /* SEC1 3.2.1: Generate X such that 1 <= n < N */ int ret = MBEDTLS_ERR_MPI_BAD_INPUT_DATA; - int count = 0; + int count; unsigned cmp = 0; size_t n_bits = mbedtls_mpi_bitlen( N ); size_t n_bytes = ( n_bits + 7 ) / 8; @@ -2470,6 +2470,28 @@ int mbedtls_mpi_random( mbedtls_mpi *X, if( mbedtls_mpi_cmp_int( N, min ) <= 0 ) return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + /* + * When min == 0, each try has at worst a probability 1/2 of failing + * (the msb has a probability 1/2 of being 0, and then the result will + * be < N), so after 30 tries failure probability is a most 2**(-30). + * + * When N is just below a power of 2, as is the case when generating + * a random point on most elliptic curves, 1 try is enough with + * overwhelming probability. When N is just above a power of 2, + * as when generating a random point on secp224k1, each try has + * a probability of failing that is almost 1/2. + * + * The probabilities are almost the same if min is nonzero but negligible + * compared to N. This is always the case when N is crypto-sized, but + * it's convenient to support small N for testing purposes. When N + * is small, use a higher repeat count, otherwise the probability of + * failure is macroscopic. + */ + if( n_bytes <= 4 ) + count = 250; + else + count = 30; + /* Ensure that target MPI has exactly the same number of limbs * as the upper bound, even if the upper bound has leading zeros. * This is necessary for the mbedtls_mpi_lt_mpi_ct() check. */ @@ -2493,18 +2515,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X, MBEDTLS_MPI_CHK( mpi_fill_random_internal( X, n_bytes, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( X, 8 * n_bytes - n_bits ) ); - /* - * Each try has at worst a probability 1/2 of failing (the msb has - * a probability 1/2 of being 0, and then the result will be < N), - * so after 30 tries failure probability is a most 2**(-30). - * - * When N is just below a power of 2, as is the case when generating - * a random point on most elliptic curves, 1 try is enough with - * overwhelming probability. When N is just above a power of 2, - * as when generating a random point on secp224k1, each try has - * a probability of failing that is almost 1/2. - */ - if( ++count > 30 ) + if( --count == 0 ) { ret = MBEDTLS_ERR_MPI_NOT_ACCEPTABLE; goto cleanup; From 7ed7c5a37d60cf441c86c7bb398e4551feb531b1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 21:28:38 +0200 Subject: [PATCH 35/48] mbedtls_mpi_random: document MBEDTLS_ERR_MPI_NOT_ACCEPTABLE Note that this error has a negligible probability with a "crypto-sized" bound, but macroscopic probability with a small bound. Signed-off-by: Gilles Peskine --- include/mbedtls/bignum.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 327442214..6a790243d 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -896,6 +896,11 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED if a memory allocation failed. * \return #MBEDTLS_ERR_MPI_BAD_INPUT_DATA if \p min or \p N is invalid * or if they are incompatible. + * \return #MBEDTLS_ERR_MPI_NOT_ACCEPTABLE if the implementation was + * unable to find a suitable value within a limited number + * of attempts. This has a negligible probability if \p N + * is significantly larger than \p min, which is the case + * for all usual cryptographic applications. * \return Another negative error code on failure. */ int mbedtls_mpi_random( mbedtls_mpi *X, From 0cb493d2397f71011ceabd1238133ee5c3cc079f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 21:32:24 +0200 Subject: [PATCH 36/48] Note that the "0 limb in ..." tests rely on undocumented behavior Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 66339c9da..24bbc6ad1 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1183,6 +1183,11 @@ mpi_random_sizes:1:"aaaaaaaaaaaaaaaa":1:0 MPI random in range: larger result mpi_random_sizes:1:"aaaaaaaaaaaaaaaa":3:0 +## The "0 limb in upper bound" tests rely on the fact that +## mbedtls_mpi_read_binary() bases the size of the MPI on the size of +## the input, without first checking for leading zeros. If this was +## not the case, the tests would still pass, but would not exercise +## the advertised behavior. MPI random in range: leading 0 limb in upper bound #0 mpi_random_sizes:1:"00aaaaaaaaaaaaaaaa":0:0 @@ -1225,7 +1230,7 @@ mpi_random_fail:1:"00":MBEDTLS_ERR_MPI_BAD_INPUT_DATA MPI random bad arguments: min > N = 1 mpi_random_fail:2:"01":MBEDTLS_ERR_MPI_BAD_INPUT_DATA -MPI random bad arguments: min > N = 001 +MPI random bad arguments: min > N = 1, 0 limb in upper bound mpi_random_fail:2:"000000000000000001":MBEDTLS_ERR_MPI_BAD_INPUT_DATA MPI Selftest From ebe9b6a51da1fa4f9ece249b80a2b47e826a6e83 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 21:55:35 +0200 Subject: [PATCH 37/48] mpi_fill_random_internal: remove spurious grow() call Since the internal function mpi_fill_random_internal() assumes that X has the right size, there is no need to call grow(). To further simplify the function, set the sign outside, and zero out the non-randomized part directly. Signed-off-by: Gilles Peskine --- library/bignum.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index c77d188ee..ac9bda81e 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2397,6 +2397,7 @@ cleanup: /* Fill X with n_bytes random bytes. * X must already have room for those bytes. + * The size and sign of X are unchanged. * n_bytes must not be 0. */ static int mpi_fill_random_internal( @@ -2409,9 +2410,9 @@ static int mpi_fill_random_internal( if( X->n < limbs ) return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); - MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); + memset( X->p, 0, overhead ); + memset( (unsigned char *) X->p + limbs * ciL, 0, ( X->n - limbs ) * ciL ); MBEDTLS_MPI_CHK( f_rng( p_rng, (unsigned char *) X->p + overhead, n_bytes ) ); mpi_bigendian_to_host( X->p, limbs ); @@ -2443,6 +2444,7 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, mbedtls_mpi_init( X ); MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); } + X->s = 1; if( size == 0 ) return( 0 ); @@ -2501,6 +2503,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X, mbedtls_mpi_init( X ); MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, N->n ) ); } + X->s = 1; /* * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) From 03299dcf5b8d58b4878acc2d16fd1bc0177fc3d8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 22:10:24 +0200 Subject: [PATCH 38/48] DHM: add notes about leading zeros Signed-off-by: Gilles Peskine --- library/dhm.c | 6 ++++-- tests/suites/test_suite_dhm.function | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index e8055be71..accd5a85c 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -223,7 +223,8 @@ int mbedtls_dhm_make_params( mbedtls_dhm_context *ctx, int x_size, goto cleanup; /* - * export P, G, GX + * Export P, G, GX. RFC 5246 §4.4 states that "leading zero octets are + * not required". We omit leading zeros for compactness. */ #define DHM_MPI_EXPORT( X, n ) \ do { \ @@ -436,8 +437,9 @@ int mbedtls_dhm_calc_secret( mbedtls_dhm_context *ctx, MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->K, &ctx->K, &ctx->P ) ); } + /* Output the secret without any leading zero byte. This is mandatory + * for TLS per RFC 5246 §8.1.2. */ *olen = mbedtls_mpi_size( &ctx->K ); - MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( &ctx->K, output, *olen ) ); cleanup: diff --git a/tests/suites/test_suite_dhm.function b/tests/suites/test_suite_dhm.function index edd96989a..c51ec946d 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -16,6 +16,8 @@ static int check_dhm_param_output( const mbedtls_mpi *expected, TEST_ASSERT( size >= *offset + 2 ); n = ( buffer[*offset] << 8 ) | buffer[*offset + 1]; *offset += 2; + /* The DHM param output from Mbed TLS has leading zeros stripped, as + * permitted but not required by RFC 5246 \S4.4. */ TEST_EQUAL( n, mbedtls_mpi_size( expected ) ); TEST_ASSERT( size >= *offset + n ); TEST_EQUAL( 0, mbedtls_mpi_read_binary( &actual, buffer + *offset, n ) ); From 19e36207ba79da7a7ffd8b152cb39f0381150a22 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 22:16:45 +0200 Subject: [PATCH 39/48] DHM tests: add some explanations Signed-off-by: Gilles Peskine --- tests/suites/test_suite_dhm.data | 2 ++ tests/suites/test_suite_dhm.function | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/tests/suites/test_suite_dhm.data b/tests/suites/test_suite_dhm.data index 773fe3b9c..1def7fca9 100644 --- a/tests/suites/test_suite_dhm.data +++ b/tests/suites/test_suite_dhm.data @@ -10,6 +10,8 @@ dhm_do_dhm:10:"23":3:10:"5":0 Diffie-Hellman full exchange: 5-bit, x_size=2 dhm_do_dhm:10:"23":2:10:"5":0 +## Repeat this test case and a few similar ones several times. The RNG state +## changes, so we get to exercise the code with a few different values. Diffie-Hellman full exchange: 5-bit #1 dhm_do_dhm:10:"23":1:10:"5":0 diff --git a/tests/suites/test_suite_dhm.function b/tests/suites/test_suite_dhm.function index c51ec946d..6d3743f94 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -1,6 +1,9 @@ /* BEGIN_HEADER */ #include "mbedtls/dhm.h" +/* Sanity checks on a Diffie-Hellman parameter: check the length-value + * syntax and check that the value is the expected one (taken from the + * DHM context by the caller). */ static int check_dhm_param_output( const mbedtls_mpi *expected, const unsigned char *buffer, size_t size, @@ -30,6 +33,8 @@ exit: return( ok ); } +/* Sanity checks on Diffie-Hellman parameters: syntax, range, and comparison + * against the context. */ static int check_dhm_params( const mbedtls_dhm_context *ctx, size_t x_size, const unsigned char *ske, size_t ske_len ) From 3270b14d4bedd948134f31b9dc4f4a44997a7cde Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 22:26:27 +0200 Subject: [PATCH 40/48] DHM: add test case with x_size < 0 Signed-off-by: Gilles Peskine --- tests/suites/test_suite_dhm.data | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/suites/test_suite_dhm.data b/tests/suites/test_suite_dhm.data index 1def7fca9..33c721659 100644 --- a/tests/suites/test_suite_dhm.data +++ b/tests/suites/test_suite_dhm.data @@ -27,6 +27,10 @@ dhm_do_dhm:10:"23":1:10:"5":0 Diffie-Hellman full exchange: 5-bit #5 dhm_do_dhm:10:"23":1:10:"5":0 +## This is x_size = P_size + 1. Arguably x_size > P_size makes no sense, +## but it's the current undocumented behavior to treat it the same as when +## x_size = P_size. If this behavior changes in the future, change the expected +## return status from 0 to MBEDTLS_ERR_DHM_BAD_INPUT_DATA. Diffie-Hellman full exchange: 97-bit, x_size=14 dhm_do_dhm:10:"93450983094850938450983409623":14:10:"9345098304850938450983409622":0 @@ -81,6 +85,9 @@ dhm_do_dhm:10:"3":1:10:"5":MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED+MBEDTLS_ERR_MPI_BA Diffie-Hellman zero modulus dhm_do_dhm:10:"0":1:10:"5":MBEDTLS_ERR_DHM_BAD_INPUT_DATA +Diffie-Hellman: x_size < 0 +dhm_do_dhm:10:"93450983094850938450983409623":-1:10:"9345098304850938450983409622":MBEDTLS_ERR_DHM_BAD_INPUT_DATA + Diffie-Hellman MPI_MAX_SIZE modulus dhm_make_public:MBEDTLS_MPI_MAX_SIZE:10:"5":0 From e842e58f61da961b57b9a462c6d094d20f122a7b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 15 Apr 2021 11:45:19 +0200 Subject: [PATCH 41/48] Correct some comments about ECC in mbedtls_mpi_random Signed-off-by: Gilles Peskine --- library/bignum.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index ac9bda81e..5f5728d67 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2460,7 +2460,6 @@ int mbedtls_mpi_random( mbedtls_mpi *X, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - /* SEC1 3.2.1: Generate X such that 1 <= n < N */ int ret = MBEDTLS_ERR_MPI_BAD_INPUT_DATA; int count; unsigned cmp = 0; @@ -2478,9 +2477,9 @@ int mbedtls_mpi_random( mbedtls_mpi *X, * be < N), so after 30 tries failure probability is a most 2**(-30). * * When N is just below a power of 2, as is the case when generating - * a random point on most elliptic curves, 1 try is enough with + * a random scalar on most elliptic curves, 1 try is enough with * overwhelming probability. When N is just above a power of 2, - * as when generating a random point on secp224k1, each try has + * as when generating a random scalar on secp224k1, each try has * a probability of failing that is almost 1/2. * * The probabilities are almost the same if min is nonzero but negligible From c7eeeb1e8dbb220bae3768df773c2dea0a5a61d5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Jun 2021 21:17:36 +0200 Subject: [PATCH 42/48] Fix long-standing obsolete comment Signed-off-by: Gilles Peskine --- tests/include/test/random.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/include/test/random.h b/tests/include/test/random.h index b64a072b3..642828078 100644 --- a/tests/include/test/random.h +++ b/tests/include/test/random.h @@ -84,8 +84,7 @@ int mbedtls_test_rnd_zero_rand( void *rng_state, * \p rng_state shall be a pointer to a #mbedtls_test_rnd_buf_info structure. * * The number of bytes released from the buffer on each call to - * the random function is specified by per_call. (Can be between - * 1 and 4) + * the random function is specified by \p len. * * After the buffer is empty, this function will call the fallback RNG in the * #mbedtls_test_rnd_buf_info structure if there is one, and From 87823d791355e21693c9b22248d3f07924cc2dfe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Jun 2021 21:18:59 +0200 Subject: [PATCH 43/48] Use ternary operator with the most common case first Signed-off-by: Gilles Peskine --- library/bignum.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 5f5728d67..1c20e3cbf 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2488,10 +2488,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X, * is small, use a higher repeat count, otherwise the probability of * failure is macroscopic. */ - if( n_bytes <= 4 ) - count = 250; - else - count = 30; + count = ( n_bytes > 4 ? 30 : 250 ); /* Ensure that target MPI has exactly the same number of limbs * as the upper bound, even if the upper bound has leading zeros. From 9077e435c6e702745e6289f8499c016c53c530d9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Jun 2021 21:22:25 +0200 Subject: [PATCH 44/48] Fix mistakes in test case descriptions Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 24bbc6ad1..1671d6e1e 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -1144,7 +1144,7 @@ mpi_random_many:1:"800000000000000000":100 MPI random in range: 1..2^71+1 mpi_random_many:1:"800000000000000001":100 -MPI random in range: 1..2^71+2^63 +MPI random in range: 1..2^71+2^70 mpi_random_many:1:"c00000000000000000":100 MPI random in range: 1..2^72-1 @@ -1156,7 +1156,7 @@ mpi_random_many:1:"01000000000000000000":100 MPI random in range: 1..2^72+1 mpi_random_many:1:"01000000000000000001":100 -MPI random in range: 1..2^72+2^63 +MPI random in range: 1..2^72+2^71 mpi_random_many:1:"01800000000000000000":100 MPI random in range: 0..1 From ceefe5d2696877842c1325749c2dc8b8b16a5354 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Jun 2021 21:24:04 +0200 Subject: [PATCH 45/48] Lift function call out of inner loop Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.function | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index a2d312f6a..58079841c 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -1530,6 +1530,8 @@ void mpi_random_many( int min, data_t *bound_bytes, int iterations ) } else { + int statistically_safe_all_the_way = + is_significantly_above_a_power_of_2( bound_bytes ); for( b = 0; b < n_bits; b++ ) { mbedtls_test_set_step( 1000000 + b ); @@ -1541,8 +1543,7 @@ void mpi_random_many( int min, data_t *bound_bytes, int iterations ) * As an exception, the top bit may legitimately never be set * if bound is a power of 2 or only slightly above. */ - if( b != n_bits - 1 || - is_significantly_above_a_power_of_2( bound_bytes ) ) + if( statistically_safe_all_the_way || b != n_bits - 1 ) { TEST_ASSERT( stats[b] > 0 ); } From ed32b576a4ebfa2213236f53abb307375c651af6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Jun 2021 22:17:52 +0200 Subject: [PATCH 46/48] New internal function mbedtls_mpi_resize_clear The idiom "resize an mpi to a given size" appeared 4 times. Unify it in a single function. Guarantee that the value is set to 0, which is required by some of the callers and not a significant expense where not required. Signed-off-by: Gilles Peskine --- library/bignum.c | 58 ++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 1c20e3cbf..45310fe15 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -181,6 +181,27 @@ int mbedtls_mpi_shrink( mbedtls_mpi *X, size_t nblimbs ) return( 0 ); } +/* Resize X to have exactly n limbs and set it to 0. */ +static int mbedtls_mpi_resize_clear( mbedtls_mpi *X, size_t limbs ) +{ + if( limbs == 0 ) + { + mbedtls_mpi_free( X ); + return( 0 ); + } + else if( X->n == limbs ) + { + memset( X->p, 0, limbs * ciL ); + X->s = 1; + return( 0 ); + } + else + { + mbedtls_mpi_free( X ); + return( mbedtls_mpi_grow( X, limbs ) ); + } +} + /* * Copy the contents of Y into X */ @@ -838,14 +859,7 @@ int mbedtls_mpi_read_binary_le( mbedtls_mpi *X, size_t const limbs = CHARS_TO_LIMBS( buflen ); /* Ensure that target MPI has exactly the necessary number of limbs */ - if( X->n != limbs ) - { - mbedtls_mpi_free( X ); - mbedtls_mpi_init( X ); - MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); - } - - MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_resize_clear( X, limbs ) ); for( i = 0; i < buflen; i++ ) X->p[i / ciL] |= ((mbedtls_mpi_uint) buf[i]) << ((i % ciL) << 3); @@ -874,17 +888,11 @@ int mbedtls_mpi_read_binary( mbedtls_mpi *X, const unsigned char *buf, size_t bu MPI_VALIDATE_RET( buflen == 0 || buf != NULL ); /* Ensure that target MPI has exactly the necessary number of limbs */ - if( X->n != limbs ) - { - mbedtls_mpi_free( X ); - mbedtls_mpi_init( X ); - MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); - } - MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_resize_clear( X, limbs ) ); - /* Avoid calling `memcpy` with NULL source argument, + /* Avoid calling `memcpy` with NULL source or destination argument, * even if buflen is 0. */ - if( buf != NULL ) + if( buflen != 0 ) { Xp = (unsigned char*) X->p; memcpy( Xp + overhead, buf, buflen ); @@ -2438,13 +2446,7 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, MPI_VALIDATE_RET( f_rng != NULL ); /* Ensure that target MPI has exactly the necessary number of limbs */ - if( X->n != limbs ) - { - mbedtls_mpi_free( X ); - mbedtls_mpi_init( X ); - MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); - } - X->s = 1; + MBEDTLS_MPI_CHK( mbedtls_mpi_resize_clear( X, limbs ) ); if( size == 0 ) return( 0 ); @@ -2493,13 +2495,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X, /* Ensure that target MPI has exactly the same number of limbs * as the upper bound, even if the upper bound has leading zeros. * This is necessary for the mbedtls_mpi_lt_mpi_ct() check. */ - if( X->n != N->n ) - { - mbedtls_mpi_free( X ); - mbedtls_mpi_init( X ); - MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, N->n ) ); - } - X->s = 1; + MBEDTLS_MPI_CHK( mbedtls_mpi_resize_clear( X, N->n ) ); /* * Match the procedure given in RFC 6979 §3.3 (deterministic ECDSA) From 405b091d9e3aefc5a9b2a4e51aabbb2bf002e51f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Jun 2021 11:38:26 +0200 Subject: [PATCH 47/48] Use MBEDTLS_MPI_CHK where warranted Signed-off-by: Gilles Peskine --- library/bignum.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 45310fe15..670d6141c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2516,11 +2516,7 @@ int mbedtls_mpi_random( mbedtls_mpi *X, goto cleanup; } - ret = mbedtls_mpi_lt_mpi_ct( X, N, &cmp ); - if( ret != 0 ) - { - goto cleanup; - } + MBEDTLS_MPI_CHK( mbedtls_mpi_lt_mpi_ct( X, N, &cmp ) ); } while( mbedtls_mpi_cmp_int( X, min ) < 0 || cmp != 1 ); From afb2bd2f22dfa87414fb8e566853098241b5be63 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Jun 2021 11:51:09 +0200 Subject: [PATCH 48/48] Note that the byte order in mpi_fill_random_internal() is deliberate Signed-off-by: Gilles Peskine --- library/bignum.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/bignum.c b/library/bignum.c index 670d6141c..c49d63e03 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2405,6 +2405,8 @@ cleanup: /* Fill X with n_bytes random bytes. * X must already have room for those bytes. + * The ordering of the bytes returned from the RNG is suitable for + * deterministic ECDSA (see RFC 6979 §3.3 and mbedtls_mpi_random()). * The size and sign of X are unchanged. * n_bytes must not be 0. */