From b9f6f9bc978ce7111141b64d37aa85f8f1c736b2 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 | 90 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 1c8607264..b57f2a17f 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -559,6 +559,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 an MPI with an integer. * diff --git a/library/bignum.c b/library/bignum.c index 7a700bc1e..c54a1fe4d 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1071,6 +1071,96 @@ 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; + + MPI_VALIDATE_RET( X != NULL ); + MPI_VALIDATE_RET( Y != NULL ); + MPI_VALIDATE_RET( ret != NULL ); + + 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 e9ae6305eaaefe50af3c20157102087aeaa16fe2 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 425e93ad2..f9f51ac27 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -163,6 +163,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 f982385e1..718b165e1 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -537,6 +537,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 4c3408b1406a6605e9f7c514f614d45a8f5b63b2 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 db36191b9..d4bf54310 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2724,6 +2724,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): @@ -2734,6 +2735,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 ) ); @@ -2748,9 +2750,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 4ea231972692ae365b4edc188a5492491bc15d84 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 c54a1fe4d..a60068b34 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1105,7 +1105,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; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -1126,8 +1126,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 3d826456f56322ce74c957678786aa217b8ce562 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 d4bf54310..d317de5d0 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2735,7 +2735,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 4f6cf380164fd4154f64b1af93d29a828dce2f1d 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 a60068b34..784987892 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1098,6 +1098,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 */ @@ -1129,7 +1134,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-- ) @@ -1142,8 +1147,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 ) @@ -1153,9 +1158,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 867a3abff561a8f2d0980f7c31727ff572538c2d 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 b57f2a17f..69dc0b02d 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -560,23 +560,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 an MPI with an integer. diff --git a/library/bignum.c b/library/bignum.c index 784987892..5f1b714ce 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1071,7 +1071,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; @@ -1098,16 +1099,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; @@ -1120,45 +1116,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 d317de5d0..040c20bd3 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2724,7 +2724,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): @@ -2750,13 +2750,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 f9f51ac27..821fd89ad 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -163,38 +163,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 718b165e1..a0aa726a7 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -538,10 +538,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 ); @@ -551,9 +553,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 7a34bcffef371f0f94fe5064ca7564cecd26096a 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 5f1b714ce..e7275333f 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1094,7 +1094,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 b11ce0ec2d6af6d01805567353dc336a5734ad7c 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 e7275333f..8438c1847 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1106,7 +1106,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; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -1119,9 +1119,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 45ec990711b0428b0864514c6cc63361623f05cd 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 8438c1847..daf0cfd9d 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1071,6 +1071,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 27d221a1aa1c515c0b591792ec9b1e85a7af89e3 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 821fd89ad..32eaec07f 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -164,37 +164,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 a0aa726a7..97c338b85 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -538,8 +538,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; @@ -547,8 +547,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 e1bf02ae261f7143f7dee2fd9407aee0a72628bf 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 32eaec07f..553612592 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -196,6 +196,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 aac48d1b3d1ba06dfe9248f6238ba13b4fd40a9c 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 69dc0b02d..22b373113 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -184,7 +184,7 @@ extern "C" { */ typedef struct mbedtls_mpi { - 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 58525180fbda2e8e84e28297193a565c04025ce5 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 daf0cfd9d..39e718dcf 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1123,12 +1123,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 bd87a59007051175751d5036f8df3950f556796f 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 39e718dcf..814a98e24 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1113,6 +1113,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; MPI_VALIDATE_RET( X != NULL ); @@ -1131,14 +1132,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 1f21c1d519089ad8b2d6826f19d9d8536ace520b 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 814a98e24..7b18cf214 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1114,7 +1114,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; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -1127,16 +1127,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 @@ -1153,7 +1153,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 ); @@ -1164,7 +1164,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 fbe4c947cd6254f3221f5fa8a442cad0da4e65bb 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 7b18cf214..6a725976a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1155,7 +1155,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 @@ -1167,7 +1167,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 798e394943b7dd8e47e05a253956a429e1ed18d1 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 553612592..ce16a4fac 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -196,21 +196,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 6a725976a..7025089e6 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1103,7 +1103,7 @@ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, ret = ret >> ( biL - 1 ); - return ret; + return (unsigned) ret; } /* From 14528628c80d1ae4a1e9cdd05139e56c5bb774b6 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 ce16a4fac..0b7aa2e1a 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -197,43 +197,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 0b7aa2e1a..b5183777a 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -181,19 +181,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 @@ -226,19 +226,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 b159ae840951a40430a95aa74c8e6e46eac791f3 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 b5183777a..4d46ca742 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -229,9 +229,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 @@ -241,6 +238,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 e25f1ee44d459567104e5f227fe9fca7de564c18 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 7025089e6..6713bcbf6 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1147,26 +1147,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 d71f31bfb71ae7f95ae573566298721148b16b8a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 11 Nov 2019 14:15:00 +0000 Subject: [PATCH 24/24] Add ChangeLog entry --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index c92e42953..c2dac3e73 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,12 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.16.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. Reported by Jeremy Dubeuf. + Bugfix * Remove redundant line for getting the bitlen of a bignum, since the variable holding the returned value is overwritten a line after.