From 8850e2e36712c98ca8364f44dcb975df55d8a919 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Jun 2019 15:01:08 +0200 Subject: [PATCH 1/3] Fix misuse of signed ints in the HAVEGE module The elements of the HAVEGE state are manipulated with bitwise operations, with the expectations that the elements are 32-bit unsigned integers (or larger). But they are declared as int, and so the code has undefined behavior. Clang with Asan correctly points out some shifts that reach the sign bit. Use unsigned int internally. This is technically an aliasing violation since we're accessing an array of `int` via a pointer to `unsigned int`, but since we don't access the array directly inside the same function, it's very unlikely to be compiled in an unintended manner. --- library/havege.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/library/havege.c b/library/havege.c index 2b75ef7bd..61383d486 100644 --- a/library/havege.c +++ b/library/havege.c @@ -58,7 +58,7 @@ static void mbedtls_zeroize( void *v, size_t n ) { * ------------------------------------------------------------------------ */ -#define SWAP(X,Y) { int *T = X; X = Y; Y = T; } +#define SWAP(X,Y) { unsigned *T = (X); (X) = (Y); (Y) = T; } #define TST1_ENTER if( PTEST & 1 ) { PTEST ^= 3; PTEST >>= 1; #define TST2_ENTER if( PTEST & 1 ) { PTEST ^= 3; PTEST >>= 1; @@ -81,7 +81,7 @@ static void mbedtls_zeroize( void *v, size_t n ) { PTX = (PT1 >> 18) & 7; \ PT1 &= 0x1FFF; \ PT2 &= 0x1FFF; \ - CLK = (int) mbedtls_timing_hardclock(); \ + CLK = (unsigned) mbedtls_timing_hardclock(); \ \ i = 0; \ A = &WALK[PT1 ]; RES[i++] ^= *A; \ @@ -104,7 +104,7 @@ static void mbedtls_zeroize( void *v, size_t n ) { \ IN = (*A >> (5)) ^ (*A << (27)) ^ CLK; \ *A = (*B >> (6)) ^ (*B << (26)) ^ CLK; \ - *B = IN; CLK = (int) mbedtls_timing_hardclock(); \ + *B = IN; CLK = (unsigned) mbedtls_timing_hardclock(); \ *C = (*C >> (7)) ^ (*C << (25)) ^ CLK; \ *D = (*D >> (8)) ^ (*D << (24)) ^ CLK; \ \ @@ -155,19 +155,20 @@ static void mbedtls_zeroize( void *v, size_t n ) { PT1 ^= (PT2 ^ 0x10) & 0x10; \ \ for( n++, i = 0; i < 16; i++ ) \ - hs->pool[n % MBEDTLS_HAVEGE_COLLECT_SIZE] ^= RES[i]; + POOL[n % MBEDTLS_HAVEGE_COLLECT_SIZE] ^= RES[i]; /* * Entropy gathering function */ static void havege_fill( mbedtls_havege_state *hs ) { - int i, n = 0; - int U1, U2, *A, *B, *C, *D; - int PT1, PT2, *WALK, RES[16]; - int PTX, PTY, CLK, PTEST, IN; + unsigned i, n = 0; + unsigned U1, U2, *A, *B, *C, *D; + unsigned PT1, PT2, *WALK, *POOL, RES[16]; + unsigned PTX, PTY, CLK, PTEST, IN; - WALK = hs->WALK; + WALK = (unsigned *) hs->WALK; + POOL = (unsigned *) hs->pool; PT1 = hs->PT1; PT2 = hs->PT2; From d1800a76a93d7fe568a1082d863f677b2bb00ebe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Jun 2019 15:12:51 +0200 Subject: [PATCH 2/3] Prevent building the HAVEGE module on platforms where it doesn't work If int is not capable of storing as many values as unsigned, the code may generate a trap value. If signed int and unsigned int aren't 32-bit types, the code may calculate meaningless values. --- library/havege.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/library/havege.c b/library/havege.c index 61383d486..69e5c3a41 100644 --- a/library/havege.c +++ b/library/havege.c @@ -37,8 +37,19 @@ #include "mbedtls/havege.h" #include "mbedtls/timing.h" +#include #include +/* If int isn't capable of storing 2^32 distinct values, the code of this + * module may cause a processor trap or a miscalculation. If int is more + * than 32 bits, the code may not calculate the intended values. */ +#if INT_MIN + 1 != -0x7fffffff +#error "The HAVEGE module requires int to be exactly 32 bits, with INT_MIN = -2^31." +#endif +#if UINT_MAX != 0xffffffff +#error "The HAVEGE module requires unsigned to be exactly 32 bits." +#endif + /* Implementation that should never be optimized out by the compiler */ static void mbedtls_zeroize( void *v, size_t n ) { volatile unsigned char *p = v; while( n-- ) *p++ = 0; From 990ea3da50f61403dc9608b7a035b3a157d88bb9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Jun 2019 15:15:40 +0200 Subject: [PATCH 3/3] Changelog entry for HAVEGE fix --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index 3c8a8cd4a..28502a371 100644 --- a/ChangeLog +++ b/ChangeLog @@ -12,6 +12,7 @@ Bugfix irwir. * Enable Suite B with subset of ECP curves. Make sure the code compiles even if some curves are not defined. Fixes #1591 reported by dbedev. + * Fix misuse of signed arithmetic in the HAVEGE module. #2598 Changes * Make `make clean` clean all programs always. Fixes #1862.