From e0187b95f0d3b861b61455427af66f2d8b8b7e73 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 5 Sep 2019 14:47:19 +0100 Subject: [PATCH 01/24] Add new, constant time mpi comparison --- include/mbedtls/bignum.h | 19 +++++++++ library/bignum.c | 86 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 3bf02a7ee..31776711b 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -484,6 +484,25 @@ int mbedtls_mpi_cmp_abs( const mbedtls_mpi *X, const mbedtls_mpi *Y ); */ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ); +/** + * \brief Compare two MPIs in constant time. + * + * \param X The left-hand MPI. This must point to an initialized MPI + * with the same allocated length as Y. + * \param Y The right-hand MPI. This must point to an initialized MPI + * with the same allocated length as X. + * \param ret The result of the comparison: + * \c 1 if \p X is greater than \p Y. + * \c -1 if \p X is lesser than \p Y. + * \c 0 if \p X is equal to \p Y. + * + * \return 0 on success. + * \return MBEDTLS_ERR_MPI_BAD_INPUT_DATA if the allocated length of + * the two input MPIs is not the same. + */ +int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + int *ret ); + /** * \brief Compare signed values * diff --git a/library/bignum.c b/library/bignum.c index 2b0a14549..dfcb81938 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -919,6 +919,92 @@ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ) return( 0 ); } +static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) +{ + mbedtls_mpi_uint ret; + mbedtls_mpi_uint cond; + + /* + * Check if the most significant bits (MSB) of the operands are different. + */ + cond = ( x ^ y ); + /* + * If the MSB are the same then the difference x-y will be negative (and + * have its MSB set to 1 during conversion to unsigned) if and only if x> ( sizeof( mbedtls_mpi_uint ) * 8 - 1 ); + + return ret; +} + +/* + * Compare signed values in constant time + */ +int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + int *ret ) +{ + size_t i; + unsigned int cond, done; + + if( X->n != Y->n ) + return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; + + /* + * if( X->s > 0 && Y->s < 0 ) + * { + * *ret = 1; + * done = 1; + * } + * else if( Y->s > 0 && X->s < 0 ) + * { + * *ret = -1; + * done = 1; + * } + */ + unsigned int sign_X = X->s; + unsigned int sign_Y = Y->s; + cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); + *ret = cond * X->s; + done = cond; + + for( i = X->n; i > 0; i-- ) + { + /* + * if( ( X->p[i - 1] > Y->p[i - 1] ) && !done ) + * { + * done = 1; + * *ret = X->s; + * } + */ + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); + *ret |= ( cond * ( 1 - done ) ) * X->s; + done |= cond * ( 1 - done ); + + /* + * if( ( X->p[i - 1] < Y->p[i - 1] ) && !done ) + * { + * done = 1; + * *ret = -X->s; + * } + */ + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); + *ret |= ( cond * ( 1 - done ) ) * -X->s; + done |= cond * ( 1 - done ); + + } + + return( 0 ); +} + /* * Compare signed values */ From 883801d3eccc5fba809e592f5104a6808a99689a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 11 Sep 2019 16:07:14 +0100 Subject: [PATCH 02/24] Add tests to constant time mpi comparison --- tests/suites/test_suite_mpi.data | 33 ++++++++++++++++++++++++++++ tests/suites/test_suite_mpi.function | 23 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index b8d7ad14c..2c7e89817 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -157,6 +157,39 @@ mbedtls_mpi_cmp_mpi:10:"2":10:"-3":1 Base test mbedtls_mpi_cmp_mpi (Mixed values) #6 mbedtls_mpi_cmp_mpi:10:"-2":10:"31231231289798":-1 +Base test mbedtls_mpi_cmp_mpi_ct #1 +mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"693":0:0 + +Base test mbedtls_mpi_cmp_mpi_ct #2 +mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"692":1:0 + +Base test mbedtls_mpi_cmp_mpi_ct #3 +mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"694":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #1 +mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-2":0:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #2 +mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-3":1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #3 +mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-1":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #4 +mbedtls_mpi_cmp_mpi_ct:1:10:"-3":1:10:"2":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #5 +mbedtls_mpi_cmp_mpi_ct:1:10:"2":1:10:"-3":1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #6 +mbedtls_mpi_cmp_mpi_ct:2:10:"-2":2:10:"31231231289798":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (X is longer in storage) #7 +mbedtls_mpi_cmp_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA + +Base test mbedtls_mpi_cmp_mpi_ct (Y is longer in storage) #8 +mbedtls_mpi_cmp_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA + Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index aa3c332bb..1a047ccd6 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -332,6 +332,29 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mbedtls_mpi_cmp_mpi_ct( int size_X, int radix_X, char * input_X, int size_Y, + int radix_Y, char * input_Y, int input_ret, int input_err ) +{ + int ret; + mbedtls_mpi X, Y; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); + + TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &Y, radix_Y, input_Y ) == 0 ); + + mbedtls_mpi_grow( &X, size_X ); + mbedtls_mpi_grow( &Y, size_Y ); + + TEST_ASSERT( mbedtls_mpi_cmp_mpi_ct( &X, &Y, &ret ) == input_err ); + if( input_err == 0 ) + TEST_ASSERT( ret == input_ret ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); +} +/* END_CASE */ + /* BEGIN_CASE */ void mbedtls_mpi_cmp_abs( int radix_X, char *input_X, int radix_Y, char *input_Y, int input_A ) From 5f3019b298a9f1e156799fab153b0b541467b603 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 16 Sep 2019 14:27:39 +0100 Subject: [PATCH 03/24] Fix side channel vulnerability in ECDSA --- library/ecp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index cb8e947db..5b8f10396 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1957,6 +1957,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, { /* SEC1 3.2.1: Generate d such that 1 <= n < N */ int count = 0; + int cmp = 0; /* * Match the procedure given in RFC 6979 (deterministic ECDSA): @@ -1967,6 +1968,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, */ 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 ) ); @@ -1981,9 +1983,14 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, */ if( ++count > 30 ) return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); + + ret = mbedtls_mpi_cmp_mpi_ct( d, &grp->N, &cmp ); + if( ret != 0 ) + { + goto cleanup; + } } - while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || - mbedtls_mpi_cmp_mpi( d, &grp->N ) >= 0 ); + while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp >= 0 ); } #endif /* ECP_SHORTWEIERSTRASS */ From c587a32a9c23150b9b113c6b01313d9339d87946 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 23 Sep 2019 09:19:14 +0100 Subject: [PATCH 04/24] Remove declaration after statement Visual Studio 2013 does not like it for some reason. --- library/bignum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index dfcb81938..06445cc11 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -953,7 +953,7 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, int *ret ) { size_t i; - unsigned int cond, done; + unsigned int cond, done, sign_X, sign_Y; if( X->n != Y->n ) return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; @@ -970,8 +970,8 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * done = 1; * } */ - unsigned int sign_X = X->s; - unsigned int sign_Y = Y->s; + sign_X = X->s; + sign_Y = Y->s; cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); *ret = cond * X->s; done = cond; From 8de2d45cd7f257879d305c4361802c45778d994e Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 10:22:37 +0100 Subject: [PATCH 05/24] Remove excess vertical space --- library/ecp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/library/ecp.c b/library/ecp.c index 5b8f10396..ad7563ea5 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1968,7 +1968,6 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, */ 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 ) ); From 8461c0e2a88120a53e97f8d544755855b2feac6a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 10:43:40 +0100 Subject: [PATCH 06/24] mbedtls_mpi_cmp_mpi_ct: remove multiplications Multiplication is known to have measurable timing variations based on the operands. For example it typically is much faster if one of the operands is zero. Remove them from constant time code. --- library/bignum.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 06445cc11..f57b328ad 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -946,6 +946,11 @@ static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) return ret; } +static int ct_bool_get_mask( unsigned int b ) +{ + return ~( b - 1 ); +} + /* * Compare signed values in constant time */ @@ -973,7 +978,7 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, sign_X = X->s; sign_Y = Y->s; cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); - *ret = cond * X->s; + *ret = ct_bool_get_mask( cond ) & X->s; done = cond; for( i = X->n; i > 0; i-- ) @@ -986,8 +991,8 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * } */ cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); - *ret |= ( cond * ( 1 - done ) ) * X->s; - done |= cond * ( 1 - done ); + *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & X->s; + done |= cond & ( 1 - done ); /* * if( ( X->p[i - 1] < Y->p[i - 1] ) && !done ) @@ -997,9 +1002,8 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * } */ cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); - *ret |= ( cond * ( 1 - done ) ) * -X->s; - done |= cond * ( 1 - done ); - + *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & -X->s; + done |= cond & ( 1 - done ); } return( 0 ); From c3b376e2f2164f0201fa41a69f79176138f843c6 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 14:21:53 +0100 Subject: [PATCH 07/24] Change mbedtls_mpi_cmp_mpi_ct to check less than The signature of mbedtls_mpi_cmp_mpi_ct() meant to support using it in place of mbedtls_mpi_cmp_mpi(). This meant full comparison functionality and a signed result. To make the function more universal and friendly to constant time coding, we change the result type to unsigned. Theoretically, we could encode the comparison result in an unsigned value, but it would be less intuitive. Therefore we won't be able to represent the result as unsigned anymore and the functionality will be constrained to checking if the first operand is less than the second. This is sufficient to support the current use case and to check any relationship between MPIs. The only drawback is that we need to call the function twice when checking for equality, but this can be optimised later if an when it is needed. --- include/mbedtls/bignum.h | 11 ++--- library/bignum.c | 68 ++++++++++++++-------------- library/ecp.c | 6 +-- tests/suites/test_suite_mpi.data | 44 +++++++++--------- tests/suites/test_suite_mpi.function | 12 +++-- 5 files changed, 71 insertions(+), 70 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 31776711b..92ba38617 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -485,23 +485,22 @@ int mbedtls_mpi_cmp_abs( const mbedtls_mpi *X, const mbedtls_mpi *Y ); int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ); /** - * \brief Compare two MPIs in constant time. + * \brief Check if an MPI is less than the other in constant time. * * \param X The left-hand MPI. This must point to an initialized MPI * with the same allocated length as Y. * \param Y The right-hand MPI. This must point to an initialized MPI * with the same allocated length as X. * \param ret The result of the comparison: - * \c 1 if \p X is greater than \p Y. - * \c -1 if \p X is lesser than \p Y. - * \c 0 if \p X is equal to \p Y. + * \c 1 if \p X is less than \p Y. + * \c 0 if \p X is greater than or equal to \p Y. * * \return 0 on success. * \return MBEDTLS_ERR_MPI_BAD_INPUT_DATA if the allocated length of * the two input MPIs is not the same. */ -int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, - int *ret ); +int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + unsigned *ret ); /** * \brief Compare signed values diff --git a/library/bignum.c b/library/bignum.c index f57b328ad..317c7cc8e 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -919,7 +919,8 @@ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ) return( 0 ); } -static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) +static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, + const mbedtls_mpi_uint y ) { mbedtls_mpi_uint ret; mbedtls_mpi_uint cond; @@ -946,16 +947,11 @@ static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) return ret; } -static int ct_bool_get_mask( unsigned int b ) -{ - return ~( b - 1 ); -} - /* * Compare signed values in constant time */ -int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, - int *ret ) +int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + unsigned *ret ) { size_t i; unsigned int cond, done, sign_X, sign_Y; @@ -964,45 +960,49 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; /* - * if( X->s > 0 && Y->s < 0 ) - * { - * *ret = 1; - * done = 1; - * } - * else if( Y->s > 0 && X->s < 0 ) - * { - * *ret = -1; - * done = 1; - * } + * Get sign bits of the signs. */ sign_X = X->s; + sign_X = sign_X >> ( sizeof( unsigned int ) * 8 - 1 ); sign_Y = Y->s; - cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); - *ret = ct_bool_get_mask( cond ) & X->s; + sign_Y = sign_Y >> ( sizeof( unsigned int ) * 8 - 1 ); + + /* + * If the signs are different, then the positive operand is the bigger. + * That is if X is negative (sign bit 1), then X < Y is true and it is false + * if X is positive (sign bit 0). + */ + cond = ( sign_X ^ sign_Y ); + *ret = cond & sign_X; + + /* + * This is a constant time function, we might have the result, but we still + * need to go through the loop. Record if we have the result already. + */ done = cond; for( i = X->n; i > 0; i-- ) { /* - * if( ( X->p[i - 1] > Y->p[i - 1] ) && !done ) - * { - * done = 1; - * *ret = X->s; - * } + * If Y->p[i - 1] < X->p[i - 1] and both X and Y are negative, then + * X < Y. + * + * Again even if we can make a decision, we just mark the result and + * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); - *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & X->s; + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & sign_X; + *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); /* - * if( ( X->p[i - 1] < Y->p[i - 1] ) && !done ) - * { - * done = 1; - * *ret = -X->s; - * } + * If X->p[i - 1] < Y->p[i - 1] and both X and Y are positive, then + * X < Y. + * + * Again even if we can make a decision, we just mark the result and + * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); - *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & -X->s; + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) & ( 1 - sign_X ); + *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); } diff --git a/library/ecp.c b/library/ecp.c index ad7563ea5..d1ea7487a 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1957,7 +1957,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, { /* SEC1 3.2.1: Generate d such that 1 <= n < N */ int count = 0; - int cmp = 0; + unsigned cmp = 0; /* * Match the procedure given in RFC 6979 (deterministic ECDSA): @@ -1983,13 +1983,13 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, if( ++count > 30 ) return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); - ret = mbedtls_mpi_cmp_mpi_ct( d, &grp->N, &cmp ); + ret = mbedtls_mpi_lt_mpi_ct( d, &grp->N, &cmp ); if( ret != 0 ) { goto cleanup; } } - while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp >= 0 ); + while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp != 1 ); } #endif /* ECP_SHORTWEIERSTRASS */ diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 2c7e89817..7920304e3 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -157,38 +157,38 @@ mbedtls_mpi_cmp_mpi:10:"2":10:"-3":1 Base test mbedtls_mpi_cmp_mpi (Mixed values) #6 mbedtls_mpi_cmp_mpi:10:"-2":10:"31231231289798":-1 -Base test mbedtls_mpi_cmp_mpi_ct #1 -mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"693":0:0 +Base test mbedtls_mpi_lt_mpi_ct #1 +mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"693":0:0 -Base test mbedtls_mpi_cmp_mpi_ct #2 -mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"692":1:0 +Base test mbedtls_mpi_lt_mpi_ct #2 +mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"692":0:0 -Base test mbedtls_mpi_cmp_mpi_ct #3 -mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"694":-1:0 +Base test mbedtls_mpi_lt_mpi_ct #3 +mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"694":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #1 -mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-2":0:0 +Base test mbedtls_mpi_lt_mpi_ct (Negative values) #1 +mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-2":0:0 -Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #2 -mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-3":1:0 +Base test mbedtls_mpi_lt_mpi_ct (Negative values) #2 +mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-3":0:0 -Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #3 -mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-1":-1:0 +Base test mbedtls_mpi_lt_mpi_ct (Negative values) #3 +mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-1":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #4 -mbedtls_mpi_cmp_mpi_ct:1:10:"-3":1:10:"2":-1:0 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #4 +mbedtls_mpi_lt_mpi_ct:1:10:"-3":1:10:"2":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #5 -mbedtls_mpi_cmp_mpi_ct:1:10:"2":1:10:"-3":1:0 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #5 +mbedtls_mpi_lt_mpi_ct:1:10:"2":1:10:"-3":0:0 -Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #6 -mbedtls_mpi_cmp_mpi_ct:2:10:"-2":2:10:"31231231289798":-1:0 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #6 +mbedtls_mpi_lt_mpi_ct:2:10:"-2":2:10:"31231231289798":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (X is longer in storage) #7 -mbedtls_mpi_cmp_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) #7 +mbedtls_mpi_lt_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA -Base test mbedtls_mpi_cmp_mpi_ct (Y is longer in storage) #8 -mbedtls_mpi_cmp_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 +mbedtls_mpi_lt_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 1a047ccd6..bf3ddfe0e 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -333,10 +333,12 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_cmp_mpi_ct( int size_X, int radix_X, char * input_X, int size_Y, - int radix_Y, char * input_Y, int input_ret, int input_err ) +void mbedtls_mpi_lt_mpi_ct( int size_X, int radix_X, char * input_X, + int size_Y, int radix_Y, char * input_Y, + int input_ret, int input_err ) { - int ret; + unsigned ret; + unsigned input_uret = input_ret; mbedtls_mpi X, Y; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); @@ -346,9 +348,9 @@ void mbedtls_mpi_cmp_mpi_ct( int size_X, int radix_X, char * input_X, int size_Y mbedtls_mpi_grow( &X, size_X ); mbedtls_mpi_grow( &Y, size_Y ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi_ct( &X, &Y, &ret ) == input_err ); + TEST_ASSERT( mbedtls_mpi_lt_mpi_ct( &X, &Y, &ret ) == input_err ); if( input_err == 0 ) - TEST_ASSERT( ret == input_ret ); + TEST_ASSERT( ret == input_uret ); exit: mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); From db9f44940923ecfa69534309271c70309836e5fd Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 08:59:14 +0100 Subject: [PATCH 08/24] ct_lt_mpi_uint: make use of biL --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 317c7cc8e..4b8edd75f 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -942,7 +942,7 @@ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, ret |= y & cond; - ret = ret >> ( sizeof( mbedtls_mpi_uint ) * 8 - 1 ); + ret = ret >> ( biL - 1 ); return ret; } From 782cbe592d6a4a1abc0f81bdcdcc6a10df504733 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:01:15 +0100 Subject: [PATCH 09/24] mpi_lt_mpi_ct: make use of unsigned consistent --- library/bignum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 4b8edd75f..addc92a02 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -954,7 +954,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned *ret ) { size_t i; - unsigned int cond, done, sign_X, sign_Y; + unsigned cond, done, sign_X, sign_Y; if( X->n != Y->n ) return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; @@ -963,9 +963,9 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Get sign bits of the signs. */ sign_X = X->s; - sign_X = sign_X >> ( sizeof( unsigned int ) * 8 - 1 ); + sign_X = sign_X >> ( sizeof( unsigned ) * 8 - 1 ); sign_Y = Y->s; - sign_Y = sign_Y >> ( sizeof( unsigned int ) * 8 - 1 ); + sign_Y = sign_Y >> ( sizeof( unsigned ) * 8 - 1 ); /* * If the signs are different, then the positive operand is the bigger. From 3173a53fe9ba53e5774f2069c80367b92d489a68 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:09:32 +0100 Subject: [PATCH 10/24] Document ct_lt_mpi_uint --- library/bignum.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/bignum.c b/library/bignum.c index addc92a02..2718dedc6 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -919,6 +919,13 @@ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ) return( 0 ); } +/** Decide if an integer is less than the other, without branches. + * + * \param x First integer. + * \param y Second integer. + * + * \return 1 if \p x is less than \p y, 0 otherwise + */ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) { From aaa3f22b767bfa9f9b2a2139605a2855b2da9c2d Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:21:49 +0100 Subject: [PATCH 11/24] mpi_lt_mpi_ct test: hardcode base 16 --- tests/suites/test_suite_mpi.data | 22 +++++++++++----------- tests/suites/test_suite_mpi.function | 8 ++++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 7920304e3..f77ccd74b 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -158,37 +158,37 @@ Base test mbedtls_mpi_cmp_mpi (Mixed values) #6 mbedtls_mpi_cmp_mpi:10:"-2":10:"31231231289798":-1 Base test mbedtls_mpi_lt_mpi_ct #1 -mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"693":0:0 +mbedtls_mpi_lt_mpi_ct:1:"2B5":1:"2B5":0:0 Base test mbedtls_mpi_lt_mpi_ct #2 -mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"692":0:0 +mbedtls_mpi_lt_mpi_ct:1:"2B5":1:"2B4":0:0 Base test mbedtls_mpi_lt_mpi_ct #3 -mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"694":1:0 +mbedtls_mpi_lt_mpi_ct:1:"2B5":1:"2B6":1:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #1 -mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-2":0:0 +mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-2":0:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #2 -mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-3":0:0 +mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-3":0:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #3 -mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-1":1:0 +mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-1":1:0 Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #4 -mbedtls_mpi_lt_mpi_ct:1:10:"-3":1:10:"2":1:0 +mbedtls_mpi_lt_mpi_ct:1:"-3":1:"2":1:0 Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #5 -mbedtls_mpi_lt_mpi_ct:1:10:"2":1:10:"-3":0:0 +mbedtls_mpi_lt_mpi_ct:1:"2":1:"-3":0:0 Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #6 -mbedtls_mpi_lt_mpi_ct:2:10:"-2":2:10:"31231231289798":1:0 +mbedtls_mpi_lt_mpi_ct:2:"-2":2:"1C67967269C6":1:0 Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) #7 -mbedtls_mpi_lt_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 -mbedtls_mpi_lt_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index bf3ddfe0e..820cde271 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -333,8 +333,8 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_lt_mpi_ct( int size_X, int radix_X, char * input_X, - int size_Y, int radix_Y, char * input_Y, +void mbedtls_mpi_lt_mpi_ct( int size_X, char * input_X, + int size_Y, char * input_Y, int input_ret, int input_err ) { unsigned ret; @@ -342,8 +342,8 @@ void mbedtls_mpi_lt_mpi_ct( int size_X, int radix_X, char * input_X, mbedtls_mpi X, Y; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); - TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_string( &Y, radix_Y, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X, 16, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &Y, 16, input_Y ) == 0 ); mbedtls_mpi_grow( &X, size_X ); mbedtls_mpi_grow( &Y, size_Y ); From 9332ecefc8b63e7b8cdd0f6ba2cbf6b9aa50b2c9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 11:33:39 +0100 Subject: [PATCH 12/24] Add more tests for mbedtls_mpi_lt_mpi_ct --- tests/suites/test_suite_mpi.data | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index f77ccd74b..047a90a02 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -190,6 +190,36 @@ mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +Base test mbedtls_mpi_lt_mpi_ct (corner case) #1 +mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFFFFFFFFFF":1:"FF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"7FFFFFFFFFFFFFFF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"1":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"0":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #3 +mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFFFFFFFFFF":1:"FF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (XY, equal MS limbs) #3 +mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFF1":2:"-EEFFFFFFFFFFFFFFFF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (X=Y) #4 +mbedtls_mpi_lt_mpi_ct:2:"EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) #4 +mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":1:0 + Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 From 9741fa6e2be24599f0c11c0dcd5a2eea5debcf83 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:07:52 +0000 Subject: [PATCH 13/24] Bignum: Document assumptions about the sign field --- include/mbedtls/bignum.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 92ba38617..8e44190e7 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -179,7 +179,7 @@ extern "C" { */ typedef struct { - int s; /*!< integer sign */ + int s; /*!< Sign: -1 if the mpi is negative, 1 otherwise */ size_t n; /*!< total # of limbs */ mbedtls_mpi_uint *p; /*!< pointer to limbs */ } From 51ed14e20f528641d69c4722861dd6d32a1ac692 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:12:15 +0000 Subject: [PATCH 14/24] Make mbedtls_mpi_lt_mpi_ct more portable The code relied on the assumptions that CHAR_BIT is 8 and that unsigned does not have padding bits. In the Bignum module we already assume that the sign of an MPI is either -1 or 1. Using this, we eliminate the above mentioned dependency. --- library/bignum.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 2718dedc6..edf3737bb 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -967,12 +967,11 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; /* - * Get sign bits of the signs. + * Set sign_N to 1 if N >= 0, 0 if N < 0. + * We know that N->s == 1 if N >= 0 and N->s == -1 if N < 0. */ - sign_X = X->s; - sign_X = sign_X >> ( sizeof( unsigned ) * 8 - 1 ); - sign_Y = Y->s; - sign_Y = sign_Y >> ( sizeof( unsigned ) * 8 - 1 ); + sign_X = ( X->s & 2 ) >> 1; + sign_Y = ( Y->s & 2 ) >> 1; /* * If the signs are different, then the positive operand is the bigger. From a2b9a96fb871754f27c1482d0f9f83e1dc7fb04f Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:23:18 +0000 Subject: [PATCH 15/24] mbedtls_mpi_lt_mpi_ct: Improve documentation --- library/bignum.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index edf3737bb..b9524a7fc 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -961,6 +961,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned *ret ) { size_t i; + /* The value of any of these variables is either 0 or 1 at all times. */ unsigned cond, done, sign_X, sign_Y; if( X->n != Y->n ) @@ -975,14 +976,14 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, /* * If the signs are different, then the positive operand is the bigger. - * That is if X is negative (sign bit 1), then X < Y is true and it is false - * if X is positive (sign bit 0). + * That is if X is negative (sign_X == 1), then X < Y is true and it is + * false if X is positive (sign_X == 0). */ cond = ( sign_X ^ sign_Y ); *ret = cond & sign_X; /* - * This is a constant time function, we might have the result, but we still + * This is a constant-time function. We might have the result, but we still * need to go through the loop. Record if we have the result already. */ done = cond; From 8ec2a953af981dc90cb05d7176b0277c8c154ffb Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:31:34 +0000 Subject: [PATCH 16/24] Rename variable for better readability --- library/bignum.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index b9524a7fc..6ea2f5a3f 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -962,7 +962,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, { size_t i; /* The value of any of these variables is either 0 or 1 at all times. */ - unsigned cond, done, sign_X, sign_Y; + unsigned cond, done, X_is_negative, Y_is_negative; if( X->n != Y->n ) return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; @@ -971,16 +971,16 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Set sign_N to 1 if N >= 0, 0 if N < 0. * We know that N->s == 1 if N >= 0 and N->s == -1 if N < 0. */ - sign_X = ( X->s & 2 ) >> 1; - sign_Y = ( Y->s & 2 ) >> 1; + X_is_negative = ( X->s & 2 ) >> 1; + Y_is_negative = ( Y->s & 2 ) >> 1; /* * If the signs are different, then the positive operand is the bigger. - * That is if X is negative (sign_X == 1), then X < Y is true and it is - * false if X is positive (sign_X == 0). + * That is if X is negative (X_is_negative == 1), then X < Y is true and it + * is false if X is positive (X_is_negative == 0). */ - cond = ( sign_X ^ sign_Y ); - *ret = cond & sign_X; + cond = ( X_is_negative ^ Y_is_negative ); + *ret = cond & X_is_negative; /* * This is a constant-time function. We might have the result, but we still @@ -997,7 +997,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & sign_X; + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & X_is_negative; *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); @@ -1008,7 +1008,8 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) & ( 1 - sign_X ); + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) + & ( 1 - X_is_negative ); *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); } From cff9e6e03d72ed9852a8492c6fa27cc9005372b0 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:37:21 +0000 Subject: [PATCH 17/24] mbedtls_mpi_lt_mpi_ct: simplify condition In the case of *ret we might need to preserve a 0 value throughout the loop and therefore we need an extra condition to protect it from being overwritten. The value of done is always 1 after *ret has been set and does not need to be protected from overwriting. Therefore in this case the extra condition can be removed. --- library/bignum.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 6ea2f5a3f..f1cf80885 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -999,7 +999,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, */ cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & X_is_negative; *ret |= cond & ( 1 - done ); - done |= cond & ( 1 - done ); + done |= cond; /* * If X->p[i - 1] < Y->p[i - 1] and both X and Y are positive, then @@ -1011,7 +1011,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) & ( 1 - X_is_negative ); *ret |= cond & ( 1 - done ); - done |= cond & ( 1 - done ); + done |= cond; } return( 0 ); From 6adff06e50ae4104e8bc4d682cfedc1f9c78f1ee Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 29 Oct 2019 15:05:12 +0000 Subject: [PATCH 18/24] mbedtls_mpi_lt_mpi_ct: add tests for 32 bit limbs The corner case tests were designed for 64 bit limbs and failed on 32 bit platforms because the numbers in the test ended up being stored in a different number of limbs and the function (correctly) returnd an error upon receiving them. --- tests/suites/test_suite_mpi.data | 35 +++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 047a90a02..9a2e58a70 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -190,21 +190,46 @@ mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA -Base test mbedtls_mpi_lt_mpi_ct (corner case) #1 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #1 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFFFFFFFFFF":1:"FF":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #2 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"7FFFFFFFFFFFFFFF":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #3 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"1":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #4 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"0":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #3 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #5 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFFFFFFFFFF":1:"FF":0:0 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #1 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFF":1:"FF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #2 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"7FFFFFFF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #3 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"1":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #4 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"0":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #5 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 + Multi-limb mbedtls_mpi_lt_mpi_ct (X Date: Tue, 29 Oct 2019 15:08:46 +0000 Subject: [PATCH 19/24] ct_lt_mpi_uint: cast the return value explicitely The return value is always either one or zero and therefore there is no risk of losing precision. Some compilers can't deduce this and complain. --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index f1cf80885..f07f746d4 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -951,7 +951,7 @@ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, ret = ret >> ( biL - 1 ); - return ret; + return (unsigned) ret; } /* From 1b86eeb06b0719cb2fe7162c05d8be6e2a0da750 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 11:42:20 +0000 Subject: [PATCH 20/24] mpi_lt_mpi_ct perform tests for both limb size The corner case tests were designed for 32 and 64 bit limbs independently and performed only on the target platform. On the other platform they are not corner cases anymore, but we can still exercise them. --- tests/suites/test_suite_mpi.data | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 9a2e58a70..2b5f4a4a1 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -191,43 +191,33 @@ Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #1 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFFFFFFFFFF":1:"FF":0:0 +mbedtls_mpi_lt_mpi_ct:2:"7FFFFFFFFFFFFFFF":2:"FF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #2 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"7FFFFFFFFFFFFFFF":0:0 +mbedtls_mpi_lt_mpi_ct:2:"8000000000000000":2:"7FFFFFFFFFFFFFFF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #3 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"1":0:0 +mbedtls_mpi_lt_mpi_ct:2:"8000000000000000":2:"1":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #4 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"0":0:0 +mbedtls_mpi_lt_mpi_ct:2:"8000000000000000":2:"0":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #5 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFFFFFFFFFF":1:"FF":0:0 +mbedtls_mpi_lt_mpi_ct:2:"FFFFFFFFFFFFFFFF":2:"FF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #1 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFF":1:"FF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #2 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"7FFFFFFF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #3 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"1":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #4 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"0":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #5 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 Multi-limb mbedtls_mpi_lt_mpi_ct (X Date: Tue, 5 Nov 2019 11:56:07 +0000 Subject: [PATCH 21/24] mpi_lt_mpi_ct: Fix test numbering --- tests/suites/test_suite_mpi.data | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 2b5f4a4a1..4924bdd56 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -175,19 +175,19 @@ mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-3":0:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #3 mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-1":1:0 -Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #4 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #1 mbedtls_mpi_lt_mpi_ct:1:"-3":1:"2":1:0 -Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #5 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #2 mbedtls_mpi_lt_mpi_ct:1:"2":1:"-3":0:0 -Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #6 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #3 mbedtls_mpi_lt_mpi_ct:2:"-2":2:"1C67967269C6":1:0 -Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) #7 +Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA -Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 +Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #1 @@ -220,19 +220,19 @@ mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"0":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #5 mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 -Multi-limb mbedtls_mpi_lt_mpi_ct (XY, equal MS limbs) #3 +Multi-limb mbedtls_mpi_lt_mpi_ct (X>Y, equal MS limbs) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFF1":2:"-EEFFFFFFFFFFFFFFFF":0:0 -Multi-limb mbedtls_mpi_lt_mpi_ct (X=Y) #4 +Multi-limb mbedtls_mpi_lt_mpi_ct (X=Y) mbedtls_mpi_lt_mpi_ct:2:"EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":0:0 -Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) #4 +Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":1:0 Base test mbedtls_mpi_cmp_abs #1 From f4482aaccc9f966121eec8c350f0fbe1694cc627 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 12:19:14 +0000 Subject: [PATCH 22/24] mpi_lt_mpi_ct: Add further tests The existing tests did not catch a failure that came up at integration testing. Adding the missing test cases to trigger the bug. --- 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 4924bdd56..9f53e2271 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -223,9 +223,6 @@ mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 Multi-limb mbedtls_mpi_lt_mpi_ct (XY, equal MS limbs) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFF1":2:"-EEFFFFFFFFFFFFFFFF":0:0 @@ -235,6 +232,18 @@ mbedtls_mpi_lt_mpi_ct:2:"EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":0:0 Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":1:0 +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #1 +mbedtls_mpi_lt_mpi_ct:2:"11FFFFFFFFFFFFFFFF":2:"FF1111111111111111":1:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #2 +mbedtls_mpi_lt_mpi_ct:2:"FF1111111111111111":2:"11FFFFFFFFFFFFFFFF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #3 +mbedtls_mpi_lt_mpi_ct:2:"-11FFFFFFFFFFFFFFFF":2:"-FF1111111111111111":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #4 +mbedtls_mpi_lt_mpi_ct:2:"-FF1111111111111111":2:"-11FFFFFFFFFFFFFFFF":1:0 + Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 From b4edac561626719ea0af952196024198fd81184d Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 12:24:52 +0000 Subject: [PATCH 23/24] mpi_lt_mpi_ct: fix condition handling The code previously only set the done flag if the return value was one. This led to overriding the correct return value later on. --- library/bignum.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index f07f746d4..9692127b9 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -991,26 +991,25 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, for( i = X->n; i > 0; i-- ) { /* - * If Y->p[i - 1] < X->p[i - 1] and both X and Y are negative, then - * X < Y. + * If Y->p[i - 1] < X->p[i - 1] then X < Y is true if and only if both + * X and Y are negative. * * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & X_is_negative; - *ret |= cond & ( 1 - done ); + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); + *ret |= cond & ( 1 - done ) & X_is_negative; done |= cond; /* - * If X->p[i - 1] < Y->p[i - 1] and both X and Y are positive, then - * X < Y. + * If X->p[i - 1] < Y->p[i - 1] then X < Y is true if and only if both + * X and Y are positive. * * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) - & ( 1 - X_is_negative ); - *ret |= cond & ( 1 - done ); + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); + *ret |= cond & ( 1 - done ) & ( 1 - X_is_negative ); done |= cond; } From dfa4d7187320cb99fb0a27fdae4702af5c33c1e1 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 11 Nov 2019 14:18:18 +0000 Subject: [PATCH 24/24] Add ChangeLog entry --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 16982a0e0..ee020ab04 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,12 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.7.x branch released xxxx-xx-xx +Security + * Fix side channel vulnerability in ECDSA key generation. Obtaining precise + timings on the comparison in the key generation enabled the attacker to + learn leading bits of the ephemeral key used during ECDSA signatures and to + recover the private key. + Changes * Add unit tests for AES-GCM when called through mbedtls_cipher_auth_xxx() from the cipher abstraction layer. Fixes #2198.