From 5325b976b94db9953e3ac6b8b8c2e63a2ddc84eb 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] 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 7cc42766e..70da0fd56 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -258,6 +258,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. @@ -298,7 +337,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 ); @@ -313,7 +351,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) @@ -322,7 +359,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 ); @@ -343,7 +380,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 ); @@ -362,7 +398,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) @@ -373,8 +408,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++ )