From 1297ef3762880320864cb6d7b92981e1db710dde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 9 Mar 2021 11:22:20 +0100 Subject: [PATCH 1/9] Use constant-time look-up for modular exponentiation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index e74a1ade04..343658124e 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2098,6 +2098,32 @@ static void mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, mpi_montmul( A, &U, N, mm, T ); } +/** + * Select an MPI from a table without leaking the index. + * + * This is functionally equivalent to mbedtls_mpi_copy(R, T[idx]) except it + * reads the entire table in order to avoid leaking the value of idx to an + * attacker able to observe memory access patterns. + * + * \param[out] R Where to write the selected MPI. + * \param[in] T The table to read from. + * \param[in] T_size The number of elements in the table. + * \param[in] idx The index of the element to select; + * this must satisfy 0 <= idx < T_size. + * + * \return \c 0 on success, or a negative error code. + */ +static int mpi_select( mbedtls_mpi *R, const mbedtls_mpi *T, size_t T_size, size_t idx ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + + for( size_t i = 0; i < T_size; i++ ) + MBEDTLS_MPI_CHK( mbedtls_mpi_safe_cond_assign( R, &T[i], i == idx ) ); + +cleanup: + return( ret ); +} + /* * Sliding-window exponentiation: X = A^E mod N (HAC 14.85) */ @@ -2110,7 +2136,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, size_t i, j, nblimbs; size_t bufsize, nbits; mbedtls_mpi_uint ei, mm, state; - mbedtls_mpi RR, T, W[ 1 << MBEDTLS_MPI_WINDOW_SIZE ], Apos; + mbedtls_mpi RR, T, W[ 1 << MBEDTLS_MPI_WINDOW_SIZE ], WW, Apos; int neg; MPI_VALIDATE_RET( X != NULL ); @@ -2134,6 +2160,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, mpi_montg_init( &mm, N ); mbedtls_mpi_init( &RR ); mbedtls_mpi_init( &T ); mbedtls_mpi_init( &Apos ); + mbedtls_mpi_init( &WW ); memset( W, 0, sizeof( W ) ); i = mbedtls_mpi_bitlen( E ); @@ -2274,7 +2301,8 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, /* * X = X * W[wbits] R^-1 mod N */ - mpi_montmul( X, &W[wbits], N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_select( &WW, W, 1 << wsize, wbits ) ); + mpi_montmul( X, &WW, N, mm, &T ); state--; nbits = 0; @@ -2312,6 +2340,7 @@ cleanup: mbedtls_mpi_free( &W[i] ); mbedtls_mpi_free( &W[1] ); mbedtls_mpi_free( &T ); mbedtls_mpi_free( &Apos ); + mbedtls_mpi_free( &WW ); if( _RR == NULL || _RR->p == NULL ) mbedtls_mpi_free( &RR ); From 92413ef5bcb5ca47bd132ae7bdb55c8cf7223d2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 3 Jun 2021 10:42:46 +0200 Subject: [PATCH 2/9] Avoid using == for sensitive comparisons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mbedtls_mpi_cf_bool_eq() is a verbatim copy of mbedtls_ssl_cf_bool_eq() Deduplication will be part of a future task. Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 343658124e..808de176ea 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2098,6 +2098,42 @@ static void mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, mpi_montmul( A, &U, N, mm, T ); } +/* + * Constant-flow boolean "equal" comparison: + * return x == y + * + * This function can be used to write constant-time code by replacing branches + * with bit operations - it can be used in conjunction with + * mbedtls_ssl_cf_mask_from_bit(). + * + * This function is implemented without using comparison operators, as those + * might be translated to branches by some compilers on some platforms. + */ +static size_t mbedtls_mpi_cf_bool_eq( size_t x, size_t y ) +{ + /* diff = 0 if x == y, non-zero otherwise */ + const size_t diff = x ^ y; + + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + + /* diff_msb's most significant bit is equal to x != y */ + const size_t diff_msb = ( diff | -diff ); + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + /* diff1 = (x != y) ? 1 : 0 */ + const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); + + return( 1 ^ diff1 ); +} + /** * Select an MPI from a table without leaking the index. * @@ -2118,7 +2154,10 @@ static int mpi_select( mbedtls_mpi *R, const mbedtls_mpi *T, size_t T_size, size int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; for( size_t i = 0; i < T_size; i++ ) - MBEDTLS_MPI_CHK( mbedtls_mpi_safe_cond_assign( R, &T[i], i == idx ) ); + { + MBEDTLS_MPI_CHK( mbedtls_mpi_safe_cond_assign( R, &T[i], + mbedtls_mpi_cf_bool_eq( i, idx ) ) ); + } cleanup: return( ret ); From 5ada7a80c3c482f0362d0eaa235b7d2efc8487d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 31 May 2021 11:48:45 +0200 Subject: [PATCH 3/9] Use bit operations for mpi_safe_cond_assign() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - copied limbs - sign - cleared limbs Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 808de176ea..343ce48dfd 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -249,8 +249,23 @@ static void mpi_safe_cond_assign( size_t n, unsigned char assign ) { size_t i; + + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + + /* all-bits 1 if assign is 1, all-bits 0 if assign is 0 */ + const mbedtls_mpi_uint mask = -assign; + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + for( i = 0; i < n; i++ ) - dest[i] = dest[i] * ( 1 - assign ) + src[i] * assign; + dest[i] = ( src[i] & mask ) | ( dest[i] & ~mask ); } /* @@ -262,20 +277,36 @@ int mbedtls_mpi_safe_cond_assign( mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned { int ret = 0; size_t i; + unsigned int mask; + mbedtls_mpi_uint limb_mask; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + /* make sure assign is 0 or 1 in a time-constant manner */ assign = (assign | (unsigned char)-assign) >> 7; + /* all-bits 1 if assign is 1, all-bits 0 if assign is 0 */ + mask = -assign; + limb_mask = -assign; + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, Y->n ) ); - X->s = X->s * ( 1 - assign ) + Y->s * assign; + X->s = ( X->s & ~mask ) | ( Y->s & mask ); mpi_safe_cond_assign( Y->n, X->p, Y->p, assign ); for( i = Y->n; i < X->n; i++ ) - X->p[i] *= ( 1 - assign ); + X->p[i] &= ~limb_mask; cleanup: return( ret ); From 448f13584df898ddda8e97fc7afe3034cfaeae07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 3 Jun 2021 10:54:01 +0200 Subject: [PATCH 4/9] Use bit operations for mpi_safe_cond_swap() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unrelated to RSA (only used in ECP), but while improving one mbedtls_safe_cond_xxx function, let's improve the other as well. Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 343ce48dfd..d934cb2c9e 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -322,6 +322,8 @@ int mbedtls_mpi_safe_cond_swap( mbedtls_mpi *X, mbedtls_mpi *Y, unsigned char sw { int ret, s; size_t i; + unsigned int sign_mask; + mbedtls_mpi_uint limb_mask; mbedtls_mpi_uint tmp; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -329,22 +331,36 @@ int mbedtls_mpi_safe_cond_swap( mbedtls_mpi *X, mbedtls_mpi *Y, unsigned char sw if( X == Y ) return( 0 ); + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + /* make sure swap is 0 or 1 in a time-constant manner */ swap = (swap | (unsigned char)-swap) >> 7; + /* all-bits 1 if swap is 1, all-bits 0 if swap is 0 */ + sign_mask = -swap; + limb_mask = -swap; + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, Y->n ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_grow( Y, X->n ) ); s = X->s; - X->s = X->s * ( 1 - swap ) + Y->s * swap; - Y->s = Y->s * ( 1 - swap ) + s * swap; + X->s = ( X->s & ~sign_mask ) | ( Y->s & sign_mask ); + Y->s = ( Y->s & ~sign_mask ) | ( s & sign_mask ); for( i = 0; i < X->n; i++ ) { tmp = X->p[i]; - X->p[i] = X->p[i] * ( 1 - swap ) + Y->p[i] * swap; - Y->p[i] = Y->p[i] * ( 1 - swap ) + tmp * swap; + X->p[i] = ( X->p[i] & ~limb_mask ) | ( Y->p[i] & limb_mask ); + Y->p[i] = ( Y->p[i] & ~limb_mask ) | ( tmp & limb_mask ); } cleanup: From 3ae4ae445fb58ba7d2b71b0e5531dc170fbf4c33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Jun 2021 09:51:00 +0200 Subject: [PATCH 5/9] Avoid UB caused by conversion to int MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 49 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index d934cb2c9e..641049ba78 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -237,6 +237,45 @@ void mbedtls_mpi_swap( mbedtls_mpi *X, mbedtls_mpi *Y ) memcpy( Y, &T, sizeof( mbedtls_mpi ) ); } +/** + * Select between two sign values in constant-time. + * + * This is functionally equivalent to second ? a : b but uses only bit + * operations in order to avoid branches. + * + * \param[in] a The first sign; must be either +1 or -1. + * \param[in] b The second sign; must be either +1 or -1. + * \param[in] second Must be either 1 (return b) or 0 (return a). + * + * \return The selected sign value. + */ +static int mpi_safe_cond_select_sign( int a, int b, unsigned char second ) +{ + /* In order to avoid questions about what we can reasonnably assume about + * the representations of signed integers, move everything to unsigned + * by taking advantage of the fact that a and b are either +1 or -1. */ + unsigned ua = a + 1; + unsigned ub = b + 1; + + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + /* all-bits 1 if assign is 1, all-bits 0 if assign is 0 */ + const unsigned mask = -second; +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + /* select ua or ub */ + unsigned ur = ( ua & ~mask ) | ( ub & mask ); + + /* ur is now 0 or 2, convert back to -1 or +1 */ + return( (int) ur - 1 ); +} + /* * Conditionally assign dest = src, without leaking information * about whether the assignment was made or not. @@ -277,7 +316,6 @@ int mbedtls_mpi_safe_cond_assign( mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned { int ret = 0; size_t i; - unsigned int mask; mbedtls_mpi_uint limb_mask; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -292,7 +330,6 @@ int mbedtls_mpi_safe_cond_assign( mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned /* make sure assign is 0 or 1 in a time-constant manner */ assign = (assign | (unsigned char)-assign) >> 7; /* all-bits 1 if assign is 1, all-bits 0 if assign is 0 */ - mask = -assign; limb_mask = -assign; #if defined(_MSC_VER) @@ -301,7 +338,7 @@ int mbedtls_mpi_safe_cond_assign( mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, Y->n ) ); - X->s = ( X->s & ~mask ) | ( Y->s & mask ); + X->s = mpi_safe_cond_select_sign( X->s, Y->s, assign ); mpi_safe_cond_assign( Y->n, X->p, Y->p, assign ); @@ -322,7 +359,6 @@ int mbedtls_mpi_safe_cond_swap( mbedtls_mpi *X, mbedtls_mpi *Y, unsigned char sw { int ret, s; size_t i; - unsigned int sign_mask; mbedtls_mpi_uint limb_mask; mbedtls_mpi_uint tmp; MPI_VALIDATE_RET( X != NULL ); @@ -341,7 +377,6 @@ int mbedtls_mpi_safe_cond_swap( mbedtls_mpi *X, mbedtls_mpi *Y, unsigned char sw /* make sure swap is 0 or 1 in a time-constant manner */ swap = (swap | (unsigned char)-swap) >> 7; /* all-bits 1 if swap is 1, all-bits 0 if swap is 0 */ - sign_mask = -swap; limb_mask = -swap; #if defined(_MSC_VER) @@ -352,8 +387,8 @@ int mbedtls_mpi_safe_cond_swap( mbedtls_mpi *X, mbedtls_mpi *Y, unsigned char sw MBEDTLS_MPI_CHK( mbedtls_mpi_grow( Y, X->n ) ); s = X->s; - X->s = ( X->s & ~sign_mask ) | ( Y->s & sign_mask ); - Y->s = ( Y->s & ~sign_mask ) | ( s & sign_mask ); + X->s = mpi_safe_cond_select_sign( X->s, Y->s, swap ); + Y->s = mpi_safe_cond_select_sign( Y->s, s, swap ); for( i = 0; i < X->n; i++ ) From 31ec1d7319ffb891bda09af34480006d08e16303 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 10 Jun 2021 09:36:41 +0200 Subject: [PATCH 6/9] Simplify sign selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 641049ba78..b1f9063d39 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -257,17 +257,8 @@ static int mpi_safe_cond_select_sign( int a, int b, unsigned char second ) unsigned ua = a + 1; unsigned ub = b + 1; - /* MSVC has a warning about unary minus on unsigned integer types, - * but this is well-defined and precisely what we want to do here. */ -#if defined(_MSC_VER) -#pragma warning( push ) -#pragma warning( disable : 4146 ) -#endif - /* all-bits 1 if assign is 1, all-bits 0 if assign is 0 */ - const unsigned mask = -second; -#if defined(_MSC_VER) -#pragma warning( pop ) -#endif + /* second was 0 or 1, mask is 0 or 2 as are ua and ub */ + const unsigned mask = second << 1; /* select ua or ub */ unsigned ur = ( ua & ~mask ) | ( ub & mask ); From e22176e7e07f3df029edc0a9d71f6493b839b963 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 10 Jun 2021 09:34:00 +0200 Subject: [PATCH 7/9] Silence MSVC type conversion warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index b1f9063d39..f4de16b918 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2229,7 +2229,7 @@ static int mpi_select( mbedtls_mpi *R, const mbedtls_mpi *T, size_t T_size, size for( size_t i = 0; i < T_size; i++ ) { MBEDTLS_MPI_CHK( mbedtls_mpi_safe_cond_assign( R, &T[i], - mbedtls_mpi_cf_bool_eq( i, idx ) ) ); + (unsigned char) mbedtls_mpi_cf_bool_eq( i, idx ) ) ); } cleanup: @@ -2413,7 +2413,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, /* * X = X * W[wbits] R^-1 mod N */ - MBEDTLS_MPI_CHK( mpi_select( &WW, W, 1 << wsize, wbits ) ); + MBEDTLS_MPI_CHK( mpi_select( &WW, W, (size_t) 1 << wsize, wbits ) ); mpi_montmul( X, &WW, N, mm, &T ); state--; From 8323244ca3749337154ee3e8a8acdfe591192658 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 31 May 2021 12:01:34 +0200 Subject: [PATCH 8/9] Add ChangeLog entry about RSA side channel. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/fix-rsa-leak.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/fix-rsa-leak.txt diff --git a/ChangeLog.d/fix-rsa-leak.txt b/ChangeLog.d/fix-rsa-leak.txt new file mode 100644 index 0000000000..b7d3e3e758 --- /dev/null +++ b/ChangeLog.d/fix-rsa-leak.txt @@ -0,0 +1,6 @@ +Security + * An adversary with access to precise enough information about memory + accesses (typically, an untrusted operating system attacking a secure + enclave) could recover an RSA private key after observing the victim + performing a single private-key operation. Found and reported by + Zili KOU, Wenjian HE, Sharad Sinha, and Wei ZHANG. From a48b16a449053ecddcdb84672c5a12fd87709c86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 17 Jun 2021 13:25:03 +0200 Subject: [PATCH 9/9] Homogenize coding patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/bignum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index f4de16b918..b7df3f047e 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -319,7 +319,7 @@ int mbedtls_mpi_safe_cond_assign( mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned #endif /* make sure assign is 0 or 1 in a time-constant manner */ - assign = (assign | (unsigned char)-assign) >> 7; + assign = (assign | (unsigned char)-assign) >> (sizeof( assign ) * 8 - 1); /* all-bits 1 if assign is 1, all-bits 0 if assign is 0 */ limb_mask = -assign; @@ -366,7 +366,7 @@ int mbedtls_mpi_safe_cond_swap( mbedtls_mpi *X, mbedtls_mpi *Y, unsigned char sw #endif /* make sure swap is 0 or 1 in a time-constant manner */ - swap = (swap | (unsigned char)-swap) >> 7; + swap = (swap | (unsigned char)-swap) >> (sizeof( swap ) * 8 - 1); /* all-bits 1 if swap is 1, all-bits 0 if swap is 0 */ limb_mask = -swap; @@ -2195,7 +2195,7 @@ static size_t mbedtls_mpi_cf_bool_eq( size_t x, size_t y ) #endif /* diff_msb's most significant bit is equal to x != y */ - const size_t diff_msb = ( diff | -diff ); + const size_t diff_msb = ( diff | (size_t) -diff ); #if defined(_MSC_VER) #pragma warning( pop )