From 2ddec4306fc99bd0785791c3905b87d6c3a64d66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 24 Aug 2020 12:49:23 +0200 Subject: [PATCH 1/5] Use bit operations for constant-flow padding check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous code used comparison operators >= and == that are quite likely to be compiled to branches by some compilers on some architectures (with some optimisation levels). For example, take the following function: void old_update( size_t data_len, size_t *padlen ) { *padlen *= ( data_len >= *padlen + 1 ); } With Clang 3.8, let's compile it for the Arm v6-M architecture: % clang --target=arm-none-eabi -march=armv6-m -Os foo.c -S -o - | sed -n '/^old_update:$/,/\.size/p' old_update: .fnstart @ BB#0: .save {r4, lr} push {r4, lr} ldr r2, [r1] adds r4, r2, #1 movs r3, #0 cmp r4, r0 bls .LBB0_2 @ BB#1: mov r2, r3 .LBB0_2: str r2, [r1] pop {r4, pc} .Lfunc_end0: .size old_update, .Lfunc_end0-old_update We can see an unbalanced secret-dependant branch, resulting in a total execution time depends on the value of the secret (here padlen) in a straightforward way. The new version, based on bit operations, doesn't have this issue: new_update: .fnstart @ BB#0: ldr r2, [r1] subs r0, r0, #1 subs r0, r0, r2 asrs r0, r0, #31 bics r2, r0 str r2, [r1] bx lr .Lfunc_end1: .size new_update, .Lfunc_end1-new_update (As a bonus, it's smaller and uses less stack.) While there's no formal guarantee that the version based on bit operations in C won't be translated using branches by the compiler, experiments tend to show that's the case [1], and it is commonly accepted knowledge in the practical crypto community that if we want to sick to C, bit operations are the safest bet [2]. [1] https://github.com/mpg/ct/blob/master/results [2] https://github.com/veorq/cryptocoding Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 107 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 98 insertions(+), 9 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 2ea35808ad5a..b4e4aea3d44d 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1044,6 +1044,82 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, } #if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) +/* + * Constant-flow mask generation for "less than" comparison: + * - if x < y, return all bits 1, that is (size_t) -1 + * - otherwise, return all bits 0, that is 0 + * + * Use only bit operations to avoid branches that could be used by some + * compilers on some platforms to translate comparison operators. + */ +static size_t mbedtls_ssl_cf_mask_lt(size_t x, size_t y) +{ + /* This has the msb set if and only if x < y */ + const size_t sub = x - y; + + /* sub1 = (x < y) in {0, 1} */ + const size_t sub1 = sub >> ( sizeof( sub ) * 8 - 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 + /* mask = (x < y) ? 0xff... : 0x00... */ + const size_t mask = -sub1; +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + return( mask ); +} + +/* + * Constant-flow mask generation for "greater or equal" comparison: + * - if x >= y, return all bits 1, that is (size_t) -1 + * - otherwise, return all bits 0, that is 0 + * + * Use only bit operations to avoid branches that could be used by some + * compilers on some platforms to translate comparison operators. + */ +static size_t mbedtls_ssl_cf_mask_ge(size_t x, size_t y) +{ + return( ~mbedtls_ssl_cf_mask_lt(x, y) ); +} + +/* + * Constant-flow boolean "equal" comparison: + * return x == y + * + * Use only bit operations to avoid branches that could be used by some + * compilers on some platforms to translate comparison operators. + */ +static size_t mbedtls_ssl_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) in {0, 1} */ + const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); + + return( 1 ^ diff1 ); +} + /* * Constant-flow conditional memcpy: * - if c1 == c2, equivalent to memcpy(dst, src, len), @@ -1071,7 +1147,7 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, /* diff_msb's most significant bit is equal to c1 != c2 */ const size_t diff_msb = ( diff | -diff ); - /* diff1 = c1 != c2 */ + /* diff1 = (c1 != c2) in {0, 1} */ const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); /* mask = c1 != c2 ? 0xff : 0x00 */ @@ -1528,8 +1604,11 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, if( auth_done == 1 ) { - correct *= ( rec->data_len >= padlen + 1 ); - padlen *= ( rec->data_len >= padlen + 1 ); + const size_t mask = mbedtls_ssl_cf_mask_ge( + rec->data_len, + padlen + 1 ); + correct &= mask; + padlen &= mask; } else { @@ -1543,8 +1622,11 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, } #endif - correct *= ( rec->data_len >= transform->maclen + padlen + 1 ); - padlen *= ( rec->data_len >= transform->maclen + padlen + 1 ); + const size_t mask = mbedtls_ssl_cf_mask_ge( + rec->data_len, + transform->maclen + padlen + 1 ); + correct &= mask; + padlen &= mask; } padlen++; @@ -1555,6 +1637,9 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, #if defined(MBEDTLS_SSL_PROTO_SSL3) if( transform->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) { + /* This is the SSL 3.0 path, we don't have to worry about Lucky + * 13, because there's a strictly worse padding attack built in + * the protocol (known as part of POODLE), so branches are OK. */ if( padlen > transform->ivlen ) { #if defined(MBEDTLS_SSL_DEBUG_ALL) @@ -1578,7 +1663,6 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, * `min(256,plaintext_len)` reads (but take into account * only the last `padlen` bytes for the padding check). */ size_t pad_count = 0; - size_t real_count = 0; volatile unsigned char* const check = data; /* Index of first padding byte; it has been ensured above @@ -1590,10 +1674,15 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, for( idx = start_idx; idx < rec->data_len; idx++ ) { - real_count |= ( idx >= padding_idx ); - pad_count += real_count * ( check[idx] == padlen - 1 ); + /* pad_count += (idx >= padding_idx) && + * (chech[idx] == padlen - 1); + */ + const size_t mask = mbedtls_ssl_cf_mask_ge( idx, padding_idx ); + const size_t equal = mbedtls_ssl_cf_bool_eq( check[idx], + padlen - 1 ); + pad_count += mask & equal; } - correct &= ( pad_count == padlen ); + correct &= mbedtls_ssl_cf_bool_eq( pad_count, padlen ); #if defined(MBEDTLS_SSL_DEBUG_ALL) if( padlen > 0 && correct == 0 ) From 6e2a9a7faaebdb1011152eb41042f6434d6f1f71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 10:01:00 +0200 Subject: [PATCH 2/5] Factor repeated code in ssl_cf functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 65 ++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 37 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index b4e4aea3d44d..075345d36f7a 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1044,6 +1044,25 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, } #if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) +/* + * Turn a bit into a mask: + * - if bit == 1, return the all-bits 1 mask, aka (size_t) -1 + * - if bit == 0, return the all-bits 0 mask, aka 0 + */ +static size_t mbedtls_ssl_cf_mask_from_bit( size_t bit ) +{ + /* 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 + return -bit; +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif +} + /* * Constant-flow mask generation for "less than" comparison: * - if x < y, return all bits 1, that is (size_t) -1 @@ -1052,7 +1071,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, * Use only bit operations to avoid branches that could be used by some * compilers on some platforms to translate comparison operators. */ -static size_t mbedtls_ssl_cf_mask_lt(size_t x, size_t y) +static size_t mbedtls_ssl_cf_mask_lt( size_t x, size_t y ) { /* This has the msb set if and only if x < y */ const size_t sub = x - y; @@ -1060,17 +1079,8 @@ static size_t mbedtls_ssl_cf_mask_lt(size_t x, size_t y) /* sub1 = (x < y) in {0, 1} */ const size_t sub1 = sub >> ( sizeof( sub ) * 8 - 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 /* mask = (x < y) ? 0xff... : 0x00... */ - const size_t mask = -sub1; -#if defined(_MSC_VER) -#pragma warning( pop ) -#endif + const size_t mask = mbedtls_ssl_cf_mask_from_bit( sub1 ); return( mask ); } @@ -1083,9 +1093,9 @@ static size_t mbedtls_ssl_cf_mask_lt(size_t x, size_t y) * Use only bit operations to avoid branches that could be used by some * compilers on some platforms to translate comparison operators. */ -static size_t mbedtls_ssl_cf_mask_ge(size_t x, size_t y) +static size_t mbedtls_ssl_cf_mask_ge( size_t x, size_t y ) { - return( ~mbedtls_ssl_cf_mask_lt(x, y) ); + return( ~mbedtls_ssl_cf_mask_lt( x, y ) ); } /* @@ -1095,7 +1105,7 @@ static size_t mbedtls_ssl_cf_mask_ge(size_t x, size_t y) * Use only bit operations to avoid branches that could be used by some * compilers on some platforms to translate comparison operators. */ -static size_t mbedtls_ssl_cf_bool_eq(size_t x, size_t y) +static size_t mbedtls_ssl_cf_bool_eq( size_t x, size_t y ) { /* diff = 0 if x == y, non-zero otherwise */ const size_t diff = x ^ y; @@ -1134,32 +1144,13 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, size_t len, size_t c1, size_t c2 ) { - /* diff = 0 if c1 == c2, non-zero otherwise */ - const size_t diff = c1 ^ c2; - - /* 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 c1 != c2 */ - const size_t diff_msb = ( diff | -diff ); - - /* diff1 = (c1 != c2) in {0, 1} */ - const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); - - /* mask = c1 != c2 ? 0xff : 0x00 */ - const unsigned char mask = (unsigned char) -diff1; - -#if defined(_MSC_VER) -#pragma warning( pop ) -#endif + /* mask = c1 == c2 ? 0xff : 0x00 */ + const size_t equal = mbedtls_ssl_cf_bool_eq( c1, c2 ); + const unsigned char mask = mbedtls_ssl_cf_mask_from_bit( equal ); /* dst[i] = c1 != c2 ? dst[i] : src[i] */ for( size_t i = 0; i < len; i++ ) - dst[i] = ( dst[i] & mask ) | ( src[i] & ~mask ); + dst[i] = ( dst[i] & ~mask ) | ( src[i] & mask ); } /* From 2a59fb45b545bd8bbe86a4e8de966bb54233a0ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:51:46 +0200 Subject: [PATCH 3/5] Add explicit cast when truncating values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MSVC complains about it otherwise. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 075345d36f7a..6091834b6643 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1146,7 +1146,7 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, { /* mask = c1 == c2 ? 0xff : 0x00 */ const size_t equal = mbedtls_ssl_cf_bool_eq( c1, c2 ); - const unsigned char mask = mbedtls_ssl_cf_mask_from_bit( equal ); + const unsigned char mask = (unsigned char) mbedtls_ssl_cf_mask_from_bit( equal ); /* dst[i] = c1 != c2 ? dst[i] : src[i] */ for( size_t i = 0; i < len; i++ ) From 822b3729e74580e19d8979e7026f8aefade93e1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 18 Sep 2020 09:54:01 +0200 Subject: [PATCH 4/5] Remove last use of non-bit operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit According to https://www.bearssl.org/ctmul.html even single-precision multiplication is not constant-time on some older platforms. An added benefit of the new code is that it removes the somewhat mysterious constant 0x1ff - which was selected because at that point the maximum value of padlen was 256. The new code is perhaps a bit more readable for that reason. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 6091834b6643..e5def644f96e 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1679,7 +1679,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, if( padlen > 0 && correct == 0 ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad padding byte detected" ) ); #endif - padlen &= correct * 0x1FF; + padlen &= mbedtls_ssl_cf_mask_from_bit( correct ); } else #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ From 6d6f8a4b97b945e443a02d87645487bb876f45f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 25 Sep 2020 09:56:53 +0200 Subject: [PATCH 5/5] Clarify descriptions of constant-flow helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 47 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index e5def644f96e..981d94e1681c 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1048,6 +1048,12 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, * Turn a bit into a mask: * - if bit == 1, return the all-bits 1 mask, aka (size_t) -1 * - if bit == 0, return the all-bits 0 mask, aka 0 + * + * This function can be used to write constant-time code by replacing branches + * with bit operations using masks. + * + * 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_ssl_cf_mask_from_bit( size_t bit ) { @@ -1068,15 +1074,18 @@ static size_t mbedtls_ssl_cf_mask_from_bit( size_t bit ) * - if x < y, return all bits 1, that is (size_t) -1 * - otherwise, return all bits 0, that is 0 * - * Use only bit operations to avoid branches that could be used by some - * compilers on some platforms to translate comparison operators. + * This function can be used to write constant-time code by replacing branches + * with bit operations using masks. + * + * 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_ssl_cf_mask_lt( size_t x, size_t y ) { - /* This has the msb set if and only if x < y */ + /* This has the most significant bit set if and only if x < y */ const size_t sub = x - y; - /* sub1 = (x < y) in {0, 1} */ + /* sub1 = (x < y) ? 1 : 0 */ const size_t sub1 = sub >> ( sizeof( sub ) * 8 - 1 ); /* mask = (x < y) ? 0xff... : 0x00... */ @@ -1090,8 +1099,11 @@ static size_t mbedtls_ssl_cf_mask_lt( size_t x, size_t y ) * - if x >= y, return all bits 1, that is (size_t) -1 * - otherwise, return all bits 0, that is 0 * - * Use only bit operations to avoid branches that could be used by some - * compilers on some platforms to translate comparison operators. + * This function can be used to write constant-time code by replacing branches + * with bit operations using masks. + * + * 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_ssl_cf_mask_ge( size_t x, size_t y ) { @@ -1102,8 +1114,12 @@ static size_t mbedtls_ssl_cf_mask_ge( size_t x, size_t y ) * Constant-flow boolean "equal" comparison: * return x == y * - * Use only bit operations to avoid branches that could be used by some - * compilers on some platforms to translate comparison operators. + * 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_ssl_cf_bool_eq( size_t x, size_t y ) { @@ -1124,7 +1140,7 @@ static size_t mbedtls_ssl_cf_bool_eq( size_t x, size_t y ) #pragma warning( pop ) #endif - /* diff1 = (x != y) in {0, 1} */ + /* diff1 = (x != y) ? 1 : 0 */ const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); return( 1 ^ diff1 ); @@ -1136,8 +1152,8 @@ static size_t mbedtls_ssl_cf_bool_eq( size_t x, size_t y ) * - otherwise, a no-op, * but with execution flow independent of the values of c1 and c2. * - * Use only bit operations to avoid branches that could be used by some - * compilers on some platforms to translate comparison operators. + * This function is implemented without using comparison operators, as those + * might be translated to branches by some compilers on some platforms. */ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, const unsigned char *src, @@ -1148,9 +1164,9 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, const size_t equal = mbedtls_ssl_cf_bool_eq( c1, c2 ); const unsigned char mask = (unsigned char) mbedtls_ssl_cf_mask_from_bit( equal ); - /* dst[i] = c1 != c2 ? dst[i] : src[i] */ + /* dst[i] = c1 == c2 ? src[i] : dst[i] */ for( size_t i = 0; i < len; i++ ) - dst[i] = ( dst[i] & ~mask ) | ( src[i] & mask ); + dst[i] = ( src[i] & mask ) | ( dst[i] & ~mask ); } /* @@ -1630,7 +1646,8 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, { /* This is the SSL 3.0 path, we don't have to worry about Lucky * 13, because there's a strictly worse padding attack built in - * the protocol (known as part of POODLE), so branches are OK. */ + * the protocol (known as part of POODLE), so we don't care if the + * code is not constant-time, in particular branches are OK. */ if( padlen > transform->ivlen ) { #if defined(MBEDTLS_SSL_DEBUG_ALL) @@ -1666,7 +1683,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, for( idx = start_idx; idx < rec->data_len; idx++ ) { /* pad_count += (idx >= padding_idx) && - * (chech[idx] == padlen - 1); + * (check[idx] == padlen - 1); */ const size_t mask = mbedtls_ssl_cf_mask_ge( idx, padding_idx ); const size_t equal = mbedtls_ssl_cf_bool_eq( check[idx],