From 92f4714e7ee682a23e2a8185fb6ca28944b7fe10 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 30 Mar 2021 23:28:51 +0200 Subject: [PATCH 01/13] 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. Cherry-picked dc0b6e44b0147809f4541ec2a0760eeeabdea485. Changed mbedtls_test_rnd_pseudo_rand to rnd_pseudo_rand. Removed test step code. Signed-off-by: Gilles Peskine Signed-off-by: Ronald Cron --- tests/suites/test_suite_dhm.function | 66 +++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_dhm.function b/tests/suites/test_suite_dhm.function index 6ec1e7e882d9..d964f2818ca8 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -1,5 +1,59 @@ /* 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 ); + + 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,9 +205,13 @@ void dhm_do_dhm( int radix_P, char *input_P, /* * First key exchange */ - TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &rnd_pseudo_rand, &rnd_info ) == result ); + TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, + &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; @@ -185,7 +243,11 @@ void dhm_do_dhm( int radix_P, char *input_P, */ p = ske; - TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, + &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 863b83b666cea70ccedee4d1524eab25db34df9e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 30 Mar 2021 23:33:49 +0200 Subject: [PATCH 02/13] 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 c4795b6d345a..063c1a601684 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: 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 #3 +Diffie-Hellman full exchange: 286-bit dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":10:"9345098309485093845098340962223981329819812792137312973297123912791271":0 Diffie-Hellman trivial subgroup #1 From d1eb14ae8a1f87bdf90db1e20784942d220f0b0d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 30 Mar 2021 23:44:22 +0200 Subject: [PATCH 03/13] 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. Cherry-picked 33ec863570e2448319ae8b44e8e4b52183450e73 Changed mbedtls_test_rnd_pseudo_info type to rnd_pseudo_info Signed-off-by: Gilles Peskine Signed-off-by: Ronald Cron --- 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 063c1a601684..fcfc5cf7a8ca 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 d964f2818ca8..7dd9e970224d 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -169,7 +169,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; @@ -183,7 +183,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; rnd_pseudo_info rnd_info; mbedtls_dhm_init( &ctx_srv ); @@ -199,8 +199,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 e75bb6308a061e6d84683b07cf6fc9acdd6a641c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:35:13 +0200 Subject: [PATCH 04/13] 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. Removed usage of MBEDTLS_ERROR_ADD which does not exist in 2.16. Signed-off-by: Gilles Peskine --- library/dhm.c | 81 +++++++++++++++++++++++---------------------------- 1 file changed, 37 insertions(+), 44 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index d652cf0ac978..85840c31d884 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -180,21 +180,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 ); @@ -223,6 +213,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 */ @@ -250,11 +264,9 @@ int mbedtls_dhm_make_params( mbedtls_dhm_context *ctx, int x_size, ctx->len = n1; cleanup: - - if( ret != 0 ) + if( ret != 0 && ret > -128 ) return( MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED + ret ); - - return( 0 ); + return( ret ); } /* @@ -306,7 +318,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 ); @@ -314,38 +326,19 @@ 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 ) + if( ret != 0 && ret > -128 ) return( MBEDTLS_ERR_DHM_MAKE_PUBLIC_FAILED + ret ); - return( 0 ); + return( ret ); } /* From b2fbda3867c0cf159f28d0b0ac5daa2d5d92bd23 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:50:57 +0200 Subject: [PATCH 05/13] 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 85840c31d884..c010d9ffffc9 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -360,7 +360,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 ); @@ -413,7 +413,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 c53560ea0054a2441e57bb353fd83d886869edf8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:48:14 +0200 Subject: [PATCH 06/13] 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 | 73 ++++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 36 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index c010d9ffffc9..4624659bfecc 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -180,29 +180,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 ) + } + 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 ); } - while( dhm_check_range( &ctx->X, &ctx->P ) != 0 ); /* * Calculate GX = G^X mod P @@ -341,31 +367,6 @@ int mbedtls_dhm_make_public( mbedtls_dhm_context *ctx, int x_size, 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 260be63e7dd06056ea05e8e7511d304e1ad7faff Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 22:56:43 +0200 Subject: [PATCH 07/13] 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 4624659bfecc..7eb52b0833b8 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -130,22 +130,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 b4367a358f6f53e135d82c804173623ab46d58a4 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Tue, 8 Jun 2021 15:48:28 +0200 Subject: [PATCH 08/13] dhm: Fix bias in private key generation and blinding Signed-off-by: Ronald Cron --- library/dhm.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index 7eb52b0833b8..6fa5435afe39 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -180,23 +180,23 @@ 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; + size_t m_size = mbedtls_mpi_size( M ); + size_t m_bitlen = mbedtls_mpi_bitlen( M ); 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 ) + if( count++ > 30 ) return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ); + + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( R, m_size, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( R, ( m_size * 8 ) - m_bitlen ) ); } while( dhm_check_range( R, M ) != 0 ); From 39b1a51cc353cf58abc70014533068185ce92c74 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 22:10:24 +0200 Subject: [PATCH 09/13] 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 6fa5435afe39..535b698ce645 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -263,7 +263,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 { \ @@ -477,8 +478,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 7dd9e970224d..43dcd56a578f 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -14,6 +14,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 60c4fec07f4db05e90823856d9946dc4ca1fb3bf Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 22:16:45 +0200 Subject: [PATCH 10/13] 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 fcfc5cf7a8ca..005784d2ac16 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 43dcd56a578f..2e8644d6be8e 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, @@ -28,6 +31,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 424d13ce7d5ed0eec5e7e15334778bb740f1c25e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Apr 2021 22:26:27 +0200 Subject: [PATCH 11/13] 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 005784d2ac16..6802f548cba8 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 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 2e0969abce48d3a350142d6011e218c9f0271e56 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Thu, 10 Jun 2021 17:24:02 +0200 Subject: [PATCH 12/13] ecp: Fix bias in the generation of blinding values Signed-off-by: Ronald Cron --- library/ecp.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 2168981afbae..d3c6ccc8de46 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1738,18 +1738,17 @@ static int ecp_randomize_jac( const mbedtls_ecp_group *grp, mbedtls_ecp_point *p /* 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 ) + if( count++ > 30 ) { ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; goto cleanup; } + + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &l, p_size, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, ( p_size * 8 ) - grp->pbits ) ); } - while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 ); + while( ( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 ) || + ( mbedtls_mpi_cmp_mpi( &l, &grp->P ) >= 0 ) ); /* Z = l * Z */ MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &pt->Z, &pt->Z, &l ) ); MOD_MUL( pt->Z ); @@ -2514,18 +2513,17 @@ static int ecp_randomize_mxz( const mbedtls_ecp_group *grp, mbedtls_ecp_point *P /* 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 ) + if( count++ > 30 ) { ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; goto cleanup; } + + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &l, p_size, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &l, ( p_size * 8 ) - grp->pbits ) ); } - while( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 ); + while( ( mbedtls_mpi_cmp_int( &l, 1 ) <= 0 ) || + ( mbedtls_mpi_cmp_mpi( &l, &grp->P ) >= 0 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &P->X, &P->X, &l ) ); MOD_MUL( P->X ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &P->Z, &P->Z, &l ) ); MOD_MUL( P->Z ); From ebe9ec448bb8e3a945e733ed75511bb585fab7c9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 31 Mar 2021 23:12:35 +0200 Subject: [PATCH 13/13] 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 000000000000..dc35ec6c6634 --- /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.