From ee6abcedfdd28c797f9e99f1c5c14815013f2523 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 5 Sep 2019 14:47:19 +0100 Subject: [PATCH 01/23] 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 2c5ace6901..3f6cdd1f97 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -594,6 +594,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 d5bde8b2cb..860571fdb4 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1148,6 +1148,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 385d5b8682fc8b825cc0a29c0628ed634e1923e6 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 11 Sep 2019 16:07:14 +0100 Subject: [PATCH 02/23] 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 f8ee09c059..efcb060414 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -175,6 +175,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 eaae1968e0..97fd7b9834 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -587,6 +587,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 a779b4601e0cf706f8b2992e783e6a7edc9700ed Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 16 Sep 2019 14:27:39 +0100 Subject: [PATCH 03/23] 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 c281d84195..596800a675 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2803,6 +2803,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): @@ -2813,6 +2814,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 ) ); @@ -2827,9 +2829,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 b2590790f280ac5599adb16979c07fa916ac2cf6 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 23 Sep 2019 09:19:14 +0100 Subject: [PATCH 04/23] 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 860571fdb4..ff8f8296da 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1182,7 +1182,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 ); @@ -1203,8 +1203,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 d80080c884c7014253e71e9b8d6b626d445b5f45 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 10:22:37 +0100 Subject: [PATCH 05/23] Remove excess vertical space --- library/ecp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/library/ecp.c b/library/ecp.c index 596800a675..b0ef3ca475 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2814,7 +2814,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 1fc97594da2394cc32649eb015df317c35f8513e Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 10:43:40 +0100 Subject: [PATCH 06/23] 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 ff8f8296da..b90404512a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1175,6 +1175,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 */ @@ -1206,7 +1211,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-- ) @@ -1219,8 +1224,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 ) @@ -1230,9 +1235,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 0e5532d6cf98de89b5b13cace783bc67d64cc3bb Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 14:21:53 +0100 Subject: [PATCH 07/23] 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 3f6cdd1f97..d4aedfc390 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -595,23 +595,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 b90404512a..65696470d4 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1148,7 +1148,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; @@ -1175,16 +1176,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; @@ -1197,45 +1193,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 b0ef3ca475..a58e8a6e0f 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2803,7 +2803,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): @@ -2829,13 +2829,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 efcb060414..89aa4d51f6 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -175,38 +175,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 97fd7b9834..617f4615c5 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -588,10 +588,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 ); @@ -601,9 +603,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 a0f732ba06fa85c6935c529bc99f198d2650e8e8 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 08:59:14 +0100 Subject: [PATCH 08/23] 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 65696470d4..55c4624a9b 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1171,7 +1171,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 4abc17236084714e57cf0fdcbfaf43ef50db6c1b Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:01:15 +0100 Subject: [PATCH 09/23] 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 55c4624a9b..cee6662689 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1183,7 +1183,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 ); @@ -1196,9 +1196,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 3f6f0e44ebe755d7a515a830b9768f3e1812d275 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:09:32 +0100 Subject: [PATCH 10/23] 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 cee6662689..d310adbab0 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1148,6 +1148,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 b7e1b494efd095734fc9ad8cc12fa2d993305b64 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:21:49 +0100 Subject: [PATCH 11/23] 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 89aa4d51f6..6ce53dc5f3 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -176,37 +176,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 617f4615c5..63a2509e11 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -588,8 +588,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; @@ -597,8 +597,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 0ac9557c86f75a9594cb84ea04984c994aeb7a25 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 11:33:39 +0100 Subject: [PATCH 12/23] 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 6ce53dc5f3..e97f087b3d 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -208,6 +208,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 1f32b5bea431e951da395e157245ab8741a7d40a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:07:52 +0000 Subject: [PATCH 13/23] 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 d4aedfc390..1d00c560a6 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -185,7 +185,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 73ba9ec9a69f643f12f39fe4d7f098d69fd3a48e Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:12:15 +0000 Subject: [PATCH 14/23] 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 d310adbab0..90704862dc 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1200,12 +1200,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 bb5147f16583b86f90609db818af3ea8f1089e29 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:23:18 +0000 Subject: [PATCH 15/23] 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 90704862dc..71b9163dce 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1190,6 +1190,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 ); @@ -1208,14 +1209,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 5e614cef157346fd647d882923e595a6229e9aad Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:31:34 +0000 Subject: [PATCH 16/23] 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 71b9163dce..d683a5e3cc 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1191,7 +1191,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 ); @@ -1204,16 +1204,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 @@ -1230,7 +1230,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 ); @@ -1241,7 +1241,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 c50e6d5edb4229358d0733de6bb3976cedb4b51d Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:37:21 +0000 Subject: [PATCH 17/23] 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 d683a5e3cc..441e4b5705 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1232,7 +1232,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 @@ -1244,7 +1244,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 f17c8006ae56a9f8540d146aa3e4a5571bc63c1f Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 29 Oct 2019 15:05:12 +0000 Subject: [PATCH 18/23] 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 e97f087b3d..30f1052b9e 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -208,21 +208,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/23] 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 441e4b5705..cdda688deb 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1180,7 +1180,7 @@ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, ret = ret >> ( biL - 1 ); - return ret; + return (unsigned) ret; } /* From 0e4792ef478a35464d40b6edc7387041336c8298 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 11:42:20 +0000 Subject: [PATCH 20/23] 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 30f1052b9e..3058a62f5f 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -209,43 +209,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/23] 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 3058a62f5f..98a194b0e7 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -193,19 +193,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 @@ -238,19 +238,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 0b1ae0e972f84a89960737703181babbe7777ec7 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 12:19:14 +0000 Subject: [PATCH 22/23] 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 98a194b0e7..480768bf39 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -241,9 +241,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 @@ -253,6 +250,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 307024207ab4b7ff75d87a0c214e8172fcb47fc1 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 12:24:52 +0000 Subject: [PATCH 23/23] 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 cdda688deb..fd0e8b2633 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1224,26 +1224,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; }