From e9073a6cb225703be540faafd81c2d08f4d7bcb4 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 87ccf42fa..8d2ad9584 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1252,7 +1252,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 bdcb39616d1f036cf9fae6cc1eba596779e09669 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 8d2ad9584..7d1843029 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1892,15 +1892,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; @@ -1928,15 +1925,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; @@ -1944,7 +1939,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 ); } /* @@ -2030,13 +2025,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 ) { @@ -2049,7 +2044,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] @@ -2059,7 +2054,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 ); } } @@ -2096,7 +2091,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; } @@ -2114,12 +2109,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; @@ -2132,18 +2127,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 3ce3ddf1acf1ed184d49f10db5140818f98ed413 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 7d1843029..7f9183f8e 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1250,7 +1250,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, @@ -1889,8 +1890,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 ) @@ -1920,6 +1940,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 @@ -1929,6 +1951,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 c81c5889e9b3cc4474ece038fa62983ca03b358e 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 7f9183f8e..c513525e7 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -242,6 +242,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. @@ -261,10 +277,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 8f6726623a28c761a738601771b6fe1bef61d8f7 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 c513525e7..d1676515a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1956,12 +1956,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 5f5695077d790e7026c9270e0ff43ed4472d62d5 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 6a9433ef34c84359bfbde765bba5069d167b4029 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 d1676515a..e406cc82a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1963,7 +1963,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 46bf7da684643ecaea16635dd3afd38cceeb4b16 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 e406cc82a..7a29eb641 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1269,8 +1269,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; @@ -1325,7 +1325,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: @@ -1959,7 +1959,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 36acd547c524e541d998a5a363e846cc13f082ab 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 7a29eb641..ff6fa4438 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1265,12 +1265,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; @@ -1281,11 +1292,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 ); } /* @@ -1296,6 +1303,7 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi mbedtls_mpi TB; int ret; size_t n; + mbedtls_mpi_uint c, z; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( A != NULL ); MPI_VALIDATE_RET( B != NULL ); @@ -1325,7 +1333,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: @@ -1959,7 +1972,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 635a3749445367aa57c31b4c6628054ae5a03ef5 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 ff6fa4438..cec798324 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1921,8 +1921,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. @@ -1966,18 +1966,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 08fd43c4f62e5307716cae8430e2d228d1858127 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 cec798324..32a03cf19 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1296,20 +1296,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; 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 ); @@ -1333,11 +1333,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 de719d5d6931a6850d8580315fbeed1ad8bd699f 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 32a03cf19..64bf095ce 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1264,7 +1264,7 @@ cleanup: return( ret ); } -/* +/** * Helper for mbedtls_mpi subtraction. * * Calculate d - s where d and s have the same size. @@ -1274,7 +1274,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`. @@ -1308,9 +1308,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 ) @@ -1979,7 +1976,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;