From 742f1a45281a41cb1a690593aa2e4ed359ed8ff7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jun 2020 15:01:32 +0200 Subject: [PATCH 01/12] Add a const annotation to the non-changing argument of mpi_sub_mul Signed-off-by: Gilles Peskine --- library/bignum.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index d56a16e76..8c9e9f46a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1330,7 +1330,9 @@ cleanup: /* * Helper for mbedtls_mpi subtraction */ -static void mpi_sub_hlp( size_t n, mbedtls_mpi_uint *s, mbedtls_mpi_uint *d ) +static void mpi_sub_hlp( size_t n, + const mbedtls_mpi_uint *s, + mbedtls_mpi_uint *d ) { size_t i; mbedtls_mpi_uint c, z; From 4e91d473c30471adc196e47456f85607ddefe8ec Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jun 2020 20:55:15 +0200 Subject: [PATCH 02/12] Revert "Shut up a clang-analyzer warning" This reverts commit 2cc69fffcf431085f18f4e59c3c5188297f97b87. A check was added in mpi_montmul because clang-analyzer warned about a possibly null pointer. However this was a false positive. Recent versions of clang-analyzer no longer emit a warning (3.6 does, 6 doesn't). Incidentally, the size check was wrong: mpi_montmul needs T->n >= 2 * (N->n + 1), not just T->n >= N->n + 1. Given that this is an internal function which is only used from one public function and in a tightly controlled way, remove both the null check (which is of low value to begin with) and the size check (which would be slightly more valuable, but was wrong anyway). This allows the function not to need to return an error, which makes the source code a little easier to read and makes the object code a little smaller. Signed-off-by: Gilles Peskine --- library/bignum.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 8c9e9f46a..4869d60cc 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1980,15 +1980,12 @@ static void mpi_montg_init( mbedtls_mpi_uint *mm, const mbedtls_mpi *N ) /* * Montgomery multiplication: A = A * B * R^-1 mod N (HAC 14.36) */ -static int mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi *N, mbedtls_mpi_uint mm, +static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi *N, mbedtls_mpi_uint mm, const mbedtls_mpi *T ) { size_t i, n, m; mbedtls_mpi_uint u0, u1, *d; - if( T->n < N->n + 1 || T->p == NULL ) - return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); - memset( T->p, 0, T->n * ciL ); d = T->p; @@ -2016,15 +2013,13 @@ static int mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi else /* prevent timing attacks */ mpi_sub_hlp( n, A->p, T->p ); - - return( 0 ); } /* * Montgomery reduction: A = A * R^-1 mod N */ -static int mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, - mbedtls_mpi_uint mm, const mbedtls_mpi *T ) +static void mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, + mbedtls_mpi_uint mm, const mbedtls_mpi *T ) { mbedtls_mpi_uint z = 1; mbedtls_mpi U; @@ -2032,7 +2027,7 @@ static int mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, U.n = U.s = (int) z; U.p = &z; - return( mpi_montmul( A, &U, N, mm, T ) ); + mpi_montmul( A, &U, N, mm, T ); } /* @@ -2118,13 +2113,13 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, else MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &W[1], A ) ); - MBEDTLS_MPI_CHK( mpi_montmul( &W[1], &RR, N, mm, &T ) ); + mpi_montmul( &W[1], &RR, N, mm, &T ); /* * X = R^2 * R^-1 mod N = R mod N */ MBEDTLS_MPI_CHK( mbedtls_mpi_copy( X, &RR ) ); - MBEDTLS_MPI_CHK( mpi_montred( X, N, mm, &T ) ); + mpi_montred( X, N, mm, &T ); if( wsize > 1 ) { @@ -2137,7 +2132,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &W[j], &W[1] ) ); for( i = 0; i < wsize - 1; i++ ) - MBEDTLS_MPI_CHK( mpi_montmul( &W[j], &W[j], N, mm, &T ) ); + mpi_montmul( &W[j], &W[j], N, mm, &T ); /* * W[i] = W[i - 1] * W[1] @@ -2147,7 +2142,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, MBEDTLS_MPI_CHK( mbedtls_mpi_grow( &W[i], N->n + 1 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &W[i], &W[i - 1] ) ); - MBEDTLS_MPI_CHK( mpi_montmul( &W[i], &W[1], N, mm, &T ) ); + mpi_montmul( &W[i], &W[1], N, mm, &T ); } } @@ -2184,7 +2179,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, /* * out of window, square X */ - MBEDTLS_MPI_CHK( mpi_montmul( X, X, N, mm, &T ) ); + mpi_montmul( X, X, N, mm, &T ); continue; } @@ -2202,12 +2197,12 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, * X = X^wsize R^-1 mod N */ for( i = 0; i < wsize; i++ ) - MBEDTLS_MPI_CHK( mpi_montmul( X, X, N, mm, &T ) ); + mpi_montmul( X, X, N, mm, &T ); /* * X = X * W[wbits] R^-1 mod N */ - MBEDTLS_MPI_CHK( mpi_montmul( X, &W[wbits], N, mm, &T ) ); + mpi_montmul( X, &W[wbits], N, mm, &T ); state--; nbits = 0; @@ -2220,18 +2215,18 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, */ for( i = 0; i < nbits; i++ ) { - MBEDTLS_MPI_CHK( mpi_montmul( X, X, N, mm, &T ) ); + mpi_montmul( X, X, N, mm, &T ); wbits <<= 1; if( ( wbits & ( one << wsize ) ) != 0 ) - MBEDTLS_MPI_CHK( mpi_montmul( X, &W[1], N, mm, &T ) ); + mpi_montmul( X, &W[1], N, mm, &T ); } /* * X = A^E * R * R^-1 mod N = A^E mod N */ - MBEDTLS_MPI_CHK( mpi_montred( X, N, mm, &T ) ); + mpi_montred( X, N, mm, &T ); if( neg && E->n != 0 && ( E->p[0] & 1 ) != 0 ) { From 2a82f72703c3cbb90697c13bdf57184c0547b653 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jun 2020 15:00:49 +0200 Subject: [PATCH 03/12] Document some internal bignum functions Signed-off-by: Gilles Peskine --- library/bignum.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 4869d60cc..24c492633 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1328,7 +1328,8 @@ cleanup: } /* - * Helper for mbedtls_mpi subtraction + * Helper for mbedtls_mpi subtraction: + * d -= s where d and s have the same size and d >= s. */ static void mpi_sub_hlp( size_t n, const mbedtls_mpi_uint *s, @@ -1977,8 +1978,27 @@ static void mpi_montg_init( mbedtls_mpi_uint *mm, const mbedtls_mpi *N ) *mm = ~x + 1; } -/* - * Montgomery multiplication: A = A * B * R^-1 mod N (HAC 14.36) +/** Montgomery multiplication: A = A * B * R^-1 mod N (HAC 14.36) + * + * \param[in,out] A One of the numbers to multiply. + * It must have at least one more limb than N + * (A->n >= N->n + 1). + * On successful completion, A contains the result of + * the multiplication A * B * R^-1 mod N where + * R = (2^ciL)^n. + * \param[in] B One of the numbers to multiply. + * It must be nonzero and must not have more limbs than N + * (B->n <= N->n). + * \param[in] N The modulo. N must be odd. + * \param mm The value calculated by `mpi_montg_init(&mm, N)`. + * This is -N^-1 mod 2^ciL. + * \param[in,out] T A bignum for temporary storage. + * It must be at least twice the limb size of N plus 2 + * (T->n >= 2 * (N->n + 1)). + * Its initial content is unused and + * its final content is indeterminate. + * Note that unlike the usual convention in the library + * for `const mbedtls_mpi*`, the content of T can change. */ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi *N, mbedtls_mpi_uint mm, const mbedtls_mpi *T ) @@ -2008,6 +2028,8 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi memcpy( A->p, d, ( n + 1 ) * ciL ); + /* If A >= N then A -= N. Do the subtraction unconditionally to prevent + * timing attacks. Modify T as a side effect. */ if( mbedtls_mpi_cmp_abs( A, N ) >= 0 ) mpi_sub_hlp( n, N->p, A->p ); else @@ -2017,6 +2039,8 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi /* * Montgomery reduction: A = A * R^-1 mod N + * + * See mpi_montmul() regarding constraints and guarantees on the parameters. */ static void mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, mbedtls_mpi_uint mm, const mbedtls_mpi *T ) From f04d11e8b27d399c5f0ad079a0409320dcb646d6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jun 2020 19:14:58 +0200 Subject: [PATCH 04/12] Separate out low-level mpi_safe_cond_assign Separate out a version of mpi_safe_cond_assign that works on equal-sized limb arrays, without worrying about allocation sizes or signs. Signed-off-by: Gilles Peskine --- library/bignum.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 24c492633..33534874f 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -243,6 +243,22 @@ void mbedtls_mpi_swap( mbedtls_mpi *X, mbedtls_mpi *Y ) memcpy( Y, &T, sizeof( mbedtls_mpi ) ); } +/* + * Conditionally assign dest = src, without leaking information + * about whether the assignment was made or not. + * dest and src must be arrays of limbs of size n. + * assign must be 0 or 1. + */ +static void mpi_safe_cond_assign( size_t n, + mbedtls_mpi_uint *dest, + const mbedtls_mpi_uint *src, + unsigned char assign ) +{ + size_t i; + for( i = 0; i < n; i++ ) + dest[i] = dest[i] * ( 1 - assign ) + src[i] * assign; +} + /* * Conditionally assign X = Y, without leaking information * about whether the assignment was made or not. @@ -262,10 +278,9 @@ int mbedtls_mpi_safe_cond_assign( mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned X->s = X->s * ( 1 - assign ) + Y->s * assign; - for( i = 0; i < Y->n; i++ ) - X->p[i] = X->p[i] * ( 1 - assign ) + Y->p[i] * assign; + mpi_safe_cond_assign( Y->n, X->p, Y->p, assign ); - for( ; i < X->n; i++ ) + for( i = Y->n; i < X->n; i++ ) X->p[i] *= ( 1 - assign ); cleanup: From 132c0976e9867eb316bbdd27b8343a25532cbdc3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jun 2020 21:05:24 +0200 Subject: [PATCH 05/12] Remove a secret-dependent branch in Montgomery multiplication In mpi_montmul, an auxiliary function for modular exponentiation (mbedtls_mpi_mod_exp) that performs Montgomery multiplication, the last step is a conditional subtraction to force the result into the correct range. The current implementation uses a branch and therefore may leak information about secret data to an adversary who can observe what branch is taken through a side channel. Avoid this potential leak by always doing the same subtraction and doing a contant-trace conditional assignment to set the result. Signed-off-by: Gilles Peskine --- library/bignum.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 33534874f..aecd461b2 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2044,12 +2044,15 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi memcpy( A->p, d, ( n + 1 ) * ciL ); /* If A >= N then A -= N. Do the subtraction unconditionally to prevent - * timing attacks. Modify T as a side effect. */ - if( mbedtls_mpi_cmp_abs( A, N ) >= 0 ) - mpi_sub_hlp( n, N->p, A->p ); - else - /* prevent timing attacks */ - mpi_sub_hlp( n, A->p, T->p ); + * timing attacks. */ + /* Set d to A + (2^biL)^n - N. */ + d[n] += 1; + mpi_sub_hlp( n, N->p, d ); + /* Now d - (2^biL)^n = A - N so d >= (2^biL)^n iff A >= N. + * So we want to copy the result of the subtraction iff d->p[n] != 0. + * Note that d->p[n] is either 0 or 1 since A - N <= N <= (2^biL)^n. */ + mpi_safe_cond_assign( n + 1, A->p, d, d[n] ); + A->p[n] = 0; } /* From d55bfe962a57a1c12297471f6c623412d7566269 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Jun 2020 21:38:26 +0200 Subject: [PATCH 06/12] Add changelog entry: fix #3394 Signed-off-by: Gilles Peskine --- ChangeLog.d/montmul-cmp-branch.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/montmul-cmp-branch.txt diff --git a/ChangeLog.d/montmul-cmp-branch.txt b/ChangeLog.d/montmul-cmp-branch.txt new file mode 100644 index 000000000..59945188a --- /dev/null +++ b/ChangeLog.d/montmul-cmp-branch.txt @@ -0,0 +1,6 @@ +Security + * Fix a side channel vulnerability in modular exponentiation that could + reveal an RSA private key used in a secure enclave. Noticed by Sangho Lee, + Ming-Wei Shih, Prasun Gera, Taesoo Kim and Hyesoon Kim (Georgia Institute + of Technology); and Marcus Peinado (Microsoft Research). Reported by Raoul + Strackx (Fortanix) in #3394. From 026f555df39824d9382fc3a43f200709f6ade108 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Jun 2020 10:48:25 +0200 Subject: [PATCH 07/12] Explicitly cast down from mbedtls_mpi_uint to unsigned char Let code analyzers know that this is deliberate. For example MSVC warns about the conversion if it's implicit. Signed-off-by: Gilles Peskine --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index aecd461b2..487f1ef9c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2051,7 +2051,7 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi /* Now d - (2^biL)^n = A - N so d >= (2^biL)^n iff A >= N. * So we want to copy the result of the subtraction iff d->p[n] != 0. * Note that d->p[n] is either 0 or 1 since A - N <= N <= (2^biL)^n. */ - mpi_safe_cond_assign( n + 1, A->p, d, d[n] ); + mpi_safe_cond_assign( n + 1, A->p, d, (unsigned char) d[n] ); A->p[n] = 0; } From 37ecc61836da2b5be50112479828e7c0ca147ee1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 8 Jun 2020 22:05:13 +0200 Subject: [PATCH 08/12] More logical parameter order for mpi_sub_hlp mpi_sub_hlp performs a subtraction A - B, but took parameters in the order (B, A). Swap the parameters so that they match the usual mathematical syntax. This has the additional benefit of putting the output parameter (A) first, which is the normal convention in this module. Signed-off-by: Gilles Peskine --- library/bignum.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 487f1ef9c..64ea872e0 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1347,8 +1347,8 @@ cleanup: * d -= s where d and s have the same size and d >= s. */ static void mpi_sub_hlp( size_t n, - const mbedtls_mpi_uint *s, - mbedtls_mpi_uint *d ) + mbedtls_mpi_uint *d, + const mbedtls_mpi_uint *s ) { size_t i; mbedtls_mpi_uint c, z; @@ -1403,7 +1403,7 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi if( B->p[n - 1] != 0 ) break; - mpi_sub_hlp( n, B->p, X->p ); + mpi_sub_hlp( n, X->p, B->p ); cleanup: @@ -2047,7 +2047,7 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi * timing attacks. */ /* Set d to A + (2^biL)^n - N. */ d[n] += 1; - mpi_sub_hlp( n, N->p, d ); + mpi_sub_hlp( n, d, N->p ); /* Now d - (2^biL)^n = A - N so d >= (2^biL)^n iff A >= N. * So we want to copy the result of the subtraction iff d->p[n] != 0. * Note that d->p[n] is either 0 or 1 since A - N <= N <= (2^biL)^n. */ From c097e9ea45f12bd8cb2f2ffa40a9181dba898b91 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 8 Jun 2020 21:58:22 +0200 Subject: [PATCH 09/12] Move carry propagation out of mpi_sub_hlp The function mpi_sub_hlp had confusing semantics: although it took a size parameter, it accessed the limb array d beyond this size, to propagate the carry. This made the function difficult to understand and analyze, with a potential buffer overflow if misused (not enough room to propagate the carry). Change the function so that it only performs the subtraction within the specified number of limbs, and returns the carry. Move the carry propagation out of mpi_sub_hlp and into its caller mbedtls_mpi_sub_abs. This makes the code of subtraction very slightly less neat, but not significantly different. In the one other place where mpi_sub_hlp is used, namely mpi_montmul, this is a net win because the carry is potentially sensitive data and the function carefully arranges to not have to propagate it. Signed-off-by: Gilles Peskine --- library/bignum.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 64ea872e0..0fddef2ac 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1343,12 +1343,23 @@ cleanup: } /* - * Helper for mbedtls_mpi subtraction: - * d -= s where d and s have the same size and d >= s. + * Helper for mbedtls_mpi subtraction. + * + * Calculate d - s where d and s have the same size. + * This function operates modulo (2^ciL)^n and returns the carry + * (1 if there was a wraparound, i.e. if `d < s`, and 0 otherwise). + * + * \param n Number of limbs of \p d and \p s. + * \param[in,out] d On input, the left operand. + * On output, the result of the subtraction: + * \param[s] The right operand. + * + * \return 1 if `d < s`. + * 0 if `d >= s`. */ -static void mpi_sub_hlp( size_t n, - mbedtls_mpi_uint *d, - const mbedtls_mpi_uint *s ) +static mbedtls_mpi_uint mpi_sub_hlp( size_t n, + mbedtls_mpi_uint *d, + const mbedtls_mpi_uint *s ) { size_t i; mbedtls_mpi_uint c, z; @@ -1359,11 +1370,7 @@ static void mpi_sub_hlp( size_t n, c = ( *d < *s ) + z; *d -= *s; } - while( c != 0 ) - { - z = ( *d < c ); *d -= c; - c = z; d++; - } + return( c ); } /* @@ -1374,6 +1381,7 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi mbedtls_mpi TB; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t n; + mbedtls_mpi_uint c, z; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( A != NULL ); MPI_VALIDATE_RET( B != NULL ); @@ -1403,7 +1411,12 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi if( B->p[n - 1] != 0 ) break; - mpi_sub_hlp( n, X->p, B->p ); + c = mpi_sub_hlp( n, X->p, B->p ); + while( c != 0 ) + { + z = ( X->p[n] < c ); X->p[n] -= c; + c = z; n++; + } cleanup: @@ -2047,7 +2060,7 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi * timing attacks. */ /* Set d to A + (2^biL)^n - N. */ d[n] += 1; - mpi_sub_hlp( n, d, N->p ); + d[n] -= mpi_sub_hlp( n, d, N->p ); /* Now d - (2^biL)^n = A - N so d >= (2^biL)^n iff A >= N. * So we want to copy the result of the subtraction iff d->p[n] != 0. * Note that d->p[n] is either 0 or 1 since A - N <= N <= (2^biL)^n. */ From 221626f2d3fb8712a4e171240a909a1eb464ce53 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 8 Jun 2020 22:37:50 +0200 Subject: [PATCH 10/12] Simplify the final reduction in mpi_montmul There was some confusion during review about when A->p[n] could be nonzero. In fact, there is no need to set A->p[n]: only the intermediate result d might need to extend to n+1 limbs, not the final result A. So never access A->p[n]. Rework the explanation of the calculation in a way that should be easier to follow. Signed-off-by: Gilles Peskine --- library/bignum.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 0fddef2ac..7a81f33b1 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2009,8 +2009,8 @@ static void mpi_montg_init( mbedtls_mpi_uint *mm, const mbedtls_mpi *N ) /** Montgomery multiplication: A = A * B * R^-1 mod N (HAC 14.36) * * \param[in,out] A One of the numbers to multiply. - * It must have at least one more limb than N - * (A->n >= N->n + 1). + * It must have at least as many limbs as N + * (A->n >= N->n), and any limbs beyond n are ignored. * On successful completion, A contains the result of * the multiplication A * B * R^-1 mod N where * R = (2^ciL)^n. @@ -2054,18 +2054,25 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi *d++ = u0; d[n + 1] = 0; } - memcpy( A->p, d, ( n + 1 ) * ciL ); + /* At this point, d is either the desired result or the desired result + * plus N. We now potentially subtract N, avoiding leaking whether the + * subtraction is performed through side channels. */ - /* If A >= N then A -= N. Do the subtraction unconditionally to prevent - * timing attacks. */ - /* Set d to A + (2^biL)^n - N. */ + /* Copy the n least significant limbs of d to A, so that + * A = d if d < N (recall that N has n limbs). */ + memcpy( A->p, d, n * ciL ); + /* If d >= N then we want to set A to N - d. To prevent timing attacks, + * do the calculation without using conditional tests. */ + /* Set d to d0 + (2^biL)^n - N where d0 is the current value of d. */ d[n] += 1; d[n] -= mpi_sub_hlp( n, d, N->p ); - /* Now d - (2^biL)^n = A - N so d >= (2^biL)^n iff A >= N. - * So we want to copy the result of the subtraction iff d->p[n] != 0. - * Note that d->p[n] is either 0 or 1 since A - N <= N <= (2^biL)^n. */ - mpi_safe_cond_assign( n + 1, A->p, d, (unsigned char) d[n] ); - A->p[n] = 0; + /* If d0 < N then d < (2^biL)^n + * so d[n] == 0 and we want to keep A as it is. + * If d0 >= N then d >= (2^biL)^n, and d <= (2^biL)^n + N < 2 * (2^biL)^n + * so d[n] == 1 and we want to set A to the result of the subtraction + * which is d - (2^biL)^n, i.e. the n least significant limbs of d. + * This exactly corresponds to a conditional assignment. */ + mpi_safe_cond_assign( n, A->p, d, (unsigned char) d[n] ); } /* From 0e5faf64074b987284ed3305f3bb4326662a8ad5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 8 Jun 2020 22:50:35 +0200 Subject: [PATCH 11/12] mbedtls_mpi_sub_abs: check the range of the result when it happens The function mbedtls_mpi_sub_abs first checked that A >= B and then performed the subtraction, relying on the fact that A >= B to guarantee that the carry propagation would stop, and not taking advantage of the fact that the carry when subtracting two numbers can only be 0 or 1. This made the carry propagation code a little hard to follow. Write an ad hoc loop for the carry propagation, checking the size of the result. This makes termination obvious. The initial check that A >= B is no longer needed, since the function now checks that the carry propagation terminates, which is equivalent. This is a slight performance gain. Signed-off-by: Gilles Peskine --- library/bignum.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 7a81f33b1..3825820ea 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1374,20 +1374,20 @@ static mbedtls_mpi_uint mpi_sub_hlp( size_t n, } /* - * Unsigned subtraction: X = |A| - |B| (HAC 14.9) + * Unsigned subtraction: X = |A| - |B| (HAC 14.9, 14.10) */ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi *B ) { mbedtls_mpi TB; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t n; - mbedtls_mpi_uint c, z; + mbedtls_mpi_uint carry; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( A != NULL ); MPI_VALIDATE_RET( B != NULL ); - if( mbedtls_mpi_cmp_abs( A, B ) < 0 ) - return( MBEDTLS_ERR_MPI_NEGATIVE_VALUE ); + /* if( mbedtls_mpi_cmp_abs( A, B ) < 0 ) */ + /* return( MBEDTLS_ERR_MPI_NEGATIVE_VALUE ); */ mbedtls_mpi_init( &TB ); @@ -1411,11 +1411,17 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi if( B->p[n - 1] != 0 ) break; - c = mpi_sub_hlp( n, X->p, B->p ); - while( c != 0 ) + carry = mpi_sub_hlp( n, X->p, B->p ); + if( carry != 0 ) { - z = ( X->p[n] < c ); X->p[n] -= c; - c = z; n++; + /* Propagate the carry to the first nonzero limb of X. */ + for( ; n < X->n && X->p[n] == 0; n++ ) + --X->p[n]; + /* If we ran out of space for the carry, it means that the result + * is negative. */ + if( n == X->n ) + return( MBEDTLS_ERR_MPI_NEGATIVE_VALUE ); + --X->p[n]; } cleanup: From 09ec10a32e56c11d358659ba3a2e547049520298 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Jun 2020 10:39:38 +0200 Subject: [PATCH 12/12] Clean up some comments Signed-off-by: Gilles Peskine --- library/bignum.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 3825820ea..d9ab6f68b 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1342,7 +1342,7 @@ cleanup: return( ret ); } -/* +/** * Helper for mbedtls_mpi subtraction. * * Calculate d - s where d and s have the same size. @@ -1352,7 +1352,7 @@ cleanup: * \param n Number of limbs of \p d and \p s. * \param[in,out] d On input, the left operand. * On output, the result of the subtraction: - * \param[s] The right operand. + * \param[in] s The right operand. * * \return 1 if `d < s`. * 0 if `d >= s`. @@ -1386,9 +1386,6 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi MPI_VALIDATE_RET( A != NULL ); MPI_VALIDATE_RET( B != NULL ); - /* if( mbedtls_mpi_cmp_abs( A, B ) < 0 ) */ - /* return( MBEDTLS_ERR_MPI_NEGATIVE_VALUE ); */ - mbedtls_mpi_init( &TB ); if( X == B ) @@ -2067,7 +2064,7 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi /* Copy the n least significant limbs of d to A, so that * A = d if d < N (recall that N has n limbs). */ memcpy( A->p, d, n * ciL ); - /* If d >= N then we want to set A to N - d. To prevent timing attacks, + /* If d >= N then we want to set A to d - N. To prevent timing attacks, * do the calculation without using conditional tests. */ /* Set d to d0 + (2^biL)^n - N where d0 is the current value of d. */ d[n] += 1;