From da1655a48e197230d7507881a74d1e38e54b8137 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 18 Oct 2017 14:21:44 +0100 Subject: [PATCH 1/8] Remove temporary stack-buffer from mbedtls_mpi_fill_random() Context: The function `mbedtls_mpi_fill_random()` uses a temporary stack buffer to hold the random data before reading it into the target MPI. Problem: This is inefficient both computationally and memory-wise. Memory-wise, it may lead to a stack overflow on constrained devices with limited stack. Fix: This commit introduces the following changes to get rid of the temporary stack buffer entirely: 1. It modifies the call to the PRNG to output the random data directly into the target MPI's data buffer. This alone, however, constitutes a change of observable behaviour: The previous implementation guaranteed to interpret the bytes emitted by the PRNG in a big-endian fashion, while rerouting the PRNG output into the target MPI's limb array leads to an interpretation that depends on the endianness of the host machine. As a remedy, the following change is applied, too: 2. Reorder the bytes emitted from the PRNG within the target MPI's data buffer to ensure big-endian semantics. Luckily, the byte reordering was already implemented as part of `mbedtls_mpi_read_binary()`, so: 3. Extract bigendian-to-host byte reordering from `mbedtls_mpi_read_binary()` to a separate internal function `mpi_bigendian_to_host()` to be used by `mbedtls_mpi_read_binary()` and `mbedtls_mpi_fill_random()`. --- library/bignum.c | 89 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 78 insertions(+), 11 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index f968a0ad7..d141a17e4 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -715,14 +715,70 @@ cleanup: } #endif /* MBEDTLS_FS_IO */ + +/* Convert a big-endian byte array aligned to the size of mbedtls_mpi_uint + * into the storage form used by mbedtls_mpi. */ +static int mpi_bigendian_to_host( unsigned char * const buf, size_t size ) +{ + mbedtls_mpi_uint * const p = (mbedtls_mpi_uint *) buf; + size_t const limbs = size / ciL; + size_t i; + + unsigned char *cur_byte_left; + unsigned char *cur_byte_right; + + mbedtls_mpi_uint *cur_limb_left; + mbedtls_mpi_uint *cur_limb_right; + + mbedtls_mpi_uint tmp_left, tmp_right; + + if( size % ciL != 0 || limbs == 0 ) + return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + + /* + * Traverse limbs and + * - adapt byte-order in each limb + * - swap the limbs themselves. + * For that, simultaneously traverse the limbs from left to right + * and from right to left, as long as the left index is not bigger + * than the right index (it's not a problem if limbs is odd and the + * indices coincide in the last iteration). + */ + + for( cur_limb_left = p, cur_limb_right = p + ( limbs - 1 ); + cur_limb_left <= cur_limb_right; + cur_limb_left++, cur_limb_right-- ) + { + cur_byte_left = (unsigned char*) cur_limb_left; + cur_byte_right = (unsigned char*) cur_limb_right; + + tmp_left = 0; + tmp_right = 0; + + for( i = 0; i < ciL; i++ ) + { + tmp_left |= ( (mbedtls_mpi_uint) *cur_byte_left++ ) + << ( ( ciL - 1 - i ) << 3 ); + tmp_right |= ( (mbedtls_mpi_uint) *cur_byte_right++ ) + << ( ( ciL - 1 - i ) << 3 ); + } + + *cur_limb_right = tmp_left; + *cur_limb_left = tmp_right; + } + + return( 0 ); +} + /* * Import X from unsigned binary data, big endian */ int mbedtls_mpi_read_binary( mbedtls_mpi *X, const unsigned char *buf, size_t buflen ) { int ret; - size_t i, j; - size_t const limbs = CHARS_TO_LIMBS( buflen ); + size_t const limbs = CHARS_TO_LIMBS( buflen ); + size_t const overhead = ( limbs * ciL ) - buflen; + unsigned char *Xp; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( buflen == 0 || buf != NULL ); @@ -734,11 +790,12 @@ int mbedtls_mpi_read_binary( mbedtls_mpi *X, const unsigned char *buf, size_t bu mbedtls_mpi_init( X ); MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); } - MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); - for( i = buflen, j = 0; i > 0; i--, j++ ) - X->p[j / ciL] |= ((mbedtls_mpi_uint) buf[i - 1]) << ((j % ciL) << 3); + Xp = (unsigned char*) X->p; + memcpy( Xp + overhead, buf, buflen ); + + MBEDTLS_MPI_CHK( mpi_bigendian_to_host( Xp, limbs * ciL ) ); cleanup: @@ -2008,18 +2065,28 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, void *p_rng ) { int ret; - unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; + size_t const limbs CHARS_TO_LIMBS( size ); + size_t const overhead = ( limbs * ciL ) - size; + unsigned char *Xp; + MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( f_rng != NULL ); - if( size > MBEDTLS_MPI_MAX_SIZE ) - return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + /* Ensure that target MPI has exactly the necessary number of limbs */ + if( X->n != limbs ) + { + mbedtls_mpi_free( X ); + mbedtls_mpi_init( X ); + MBEDTLS_MPI_CHK( mbedtls_mpi_grow( X, limbs ) ); + } + MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); - MBEDTLS_MPI_CHK( f_rng( p_rng, buf, size ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( X, buf, size ) ); + Xp = (unsigned char*) X->p; + f_rng( p_rng, Xp + overhead, size ); + + MBEDTLS_MPI_CHK( mpi_bigendian_to_host( Xp, limbs * ciL ) ); cleanup: - mbedtls_platform_zeroize( buf, sizeof( buf ) ); return( ret ); } From 2be8a55f724489af502401ac6cac5e15ea0180ac Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 25 Oct 2018 12:40:09 +0100 Subject: [PATCH 2/8] Change signature of mpi_bigendian_to_host() to reflect usage --- library/bignum.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index d141a17e4..402a3d5c1 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -718,10 +718,8 @@ cleanup: /* Convert a big-endian byte array aligned to the size of mbedtls_mpi_uint * into the storage form used by mbedtls_mpi. */ -static int mpi_bigendian_to_host( unsigned char * const buf, size_t size ) +static void mpi_bigendian_to_host( mbedtls_mpi_uint * const p, size_t limbs ) { - mbedtls_mpi_uint * const p = (mbedtls_mpi_uint *) buf; - size_t const limbs = size / ciL; size_t i; unsigned char *cur_byte_left; @@ -732,8 +730,8 @@ static int mpi_bigendian_to_host( unsigned char * const buf, size_t size ) mbedtls_mpi_uint tmp_left, tmp_right; - if( size % ciL != 0 || limbs == 0 ) - return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + if( limbs == 0 ) + return; /* * Traverse limbs and @@ -767,7 +765,7 @@ static int mpi_bigendian_to_host( unsigned char * const buf, size_t size ) *cur_limb_left = tmp_right; } - return( 0 ); + return; } /* @@ -795,7 +793,7 @@ int mbedtls_mpi_read_binary( mbedtls_mpi *X, const unsigned char *buf, size_t bu Xp = (unsigned char*) X->p; memcpy( Xp + overhead, buf, buflen ); - MBEDTLS_MPI_CHK( mpi_bigendian_to_host( Xp, limbs * ciL ) ); + mpi_bigendian_to_host( X->p, limbs ); cleanup: @@ -2084,7 +2082,7 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, Xp = (unsigned char*) X->p; f_rng( p_rng, Xp + overhead, size ); - MBEDTLS_MPI_CHK( mpi_bigendian_to_host( Xp, limbs * ciL ) ); + mpi_bigendian_to_host( X->p, limbs ); cleanup: return( ret ); From 8116ef7c81e8258fd8530f6ee726f65304df2756 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 25 Oct 2018 12:42:08 +0100 Subject: [PATCH 3/8] Adapt ChangeLog --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index be28645d6..33d4147db 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.xx.x branch released xxxx-xx-xx + +Bugfix + * Reduce the stack consumption of mbedtls_mpi_fill_random() which could + previously lead to a stack overflow on constrained targets. + = mbed TLS 2.16.0 branch released 2018-12-21 Features From f872007782fd166e2016fdbd0ba5d48232d55e35 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Nov 2018 11:53:49 +0000 Subject: [PATCH 4/8] Optimize mpi_bigendian_to_host() for speed and size Use GCC / Clang builtins for byte swapping. --- library/bignum.c | 82 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 27 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 402a3d5c1..c83f06d32 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -718,18 +718,59 @@ cleanup: /* Convert a big-endian byte array aligned to the size of mbedtls_mpi_uint * into the storage form used by mbedtls_mpi. */ + +static mbedtls_mpi_uint mpi_uint_bigendian_to_host_c( mbedtls_mpi_uint x ) +{ + uint8_t i; + mbedtls_mpi_uint tmp = 0; + /* This works regardless of the endianness. */ + for( i = 0; i < ciL; i++, x >>= 8 ) + tmp |= ( x & 0xFF ) << ( ( ciL - 1 - i ) << 3 ); + return( tmp ); +} + +static mbedtls_mpi_uint mpi_uint_bigendian_to_host( mbedtls_mpi_uint x ) +{ +#if defined(__BYTE_ORDER__) + +/* Nothing to do on bigendian systems. */ +#if ( __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ ) + return( x ); +#endif /* __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ */ + +#if ( __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ ) + +/* For GCC and Clang, have builtins for byte swapping. */ +#if( defined(__GNUC__) && __GNUC_PREREQ(4,3) ) +#define have_bswap +#elif defined(__clang__) && \ + defined(__has_builtin) && \ + __has_builtin(__builtin_bswap32) && \ + __has_builtin(__builtin_bswap64) +#define have_bswap +#endif +#if defined(have_bswap) + /* The compiler is hopefully able to statically evaluate this! */ + switch( sizeof(mbedtls_mpi_uint) ) + { + case 4: + return( __builtin_bswap32(x) ); + case 8: + return( __builtin_bswap64(x) ); + } +#endif +#endif /* __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ */ +#endif /* __BYTE_ORDER__ */ + + /* Fall back to C-based reordering if we don't know the byte order + * or we couldn't use a compiler-specific builtin. */ + return( mpi_uint_bigendian_to_host_c( x ) ); +} + static void mpi_bigendian_to_host( mbedtls_mpi_uint * const p, size_t limbs ) { - size_t i; - - unsigned char *cur_byte_left; - unsigned char *cur_byte_right; - mbedtls_mpi_uint *cur_limb_left; mbedtls_mpi_uint *cur_limb_right; - - mbedtls_mpi_uint tmp_left, tmp_right; - if( limbs == 0 ) return; @@ -742,30 +783,17 @@ static void mpi_bigendian_to_host( mbedtls_mpi_uint * const p, size_t limbs ) * than the right index (it's not a problem if limbs is odd and the * indices coincide in the last iteration). */ - for( cur_limb_left = p, cur_limb_right = p + ( limbs - 1 ); cur_limb_left <= cur_limb_right; cur_limb_left++, cur_limb_right-- ) { - cur_byte_left = (unsigned char*) cur_limb_left; - cur_byte_right = (unsigned char*) cur_limb_right; - - tmp_left = 0; - tmp_right = 0; - - for( i = 0; i < ciL; i++ ) - { - tmp_left |= ( (mbedtls_mpi_uint) *cur_byte_left++ ) - << ( ( ciL - 1 - i ) << 3 ); - tmp_right |= ( (mbedtls_mpi_uint) *cur_byte_right++ ) - << ( ( ciL - 1 - i ) << 3 ); - } - - *cur_limb_right = tmp_left; - *cur_limb_left = tmp_right; + mbedtls_mpi_uint tmp; + /* Note that if cur_limb_left == cur_limb_right, + * this code effectively swaps the bytes only once. */ + tmp = mpi_uint_bigendian_to_host( *cur_limb_left ); + *cur_limb_left = mpi_uint_bigendian_to_host( *cur_limb_right ); + *cur_limb_right = tmp; } - - return; } /* From 5d91c0bbee6270c0c524afc111f265c198088105 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 2 Jan 2019 11:24:30 +0000 Subject: [PATCH 5/8] Add missing macro existence check in byte swapping code in bignum.c --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index c83f06d32..91c82323b 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -741,7 +741,7 @@ static mbedtls_mpi_uint mpi_uint_bigendian_to_host( mbedtls_mpi_uint x ) #if ( __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ ) /* For GCC and Clang, have builtins for byte swapping. */ -#if( defined(__GNUC__) && __GNUC_PREREQ(4,3) ) +#if( defined(__GNUC__) && defined(__GNUC_PREREQ) && __GNUC_PREREQ(4,3) ) #define have_bswap #elif defined(__clang__) && \ defined(__has_builtin) && \ From 6dab6200c678386bdf8951f57b9b769d80810bb3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 2 Jan 2019 16:42:29 +0000 Subject: [PATCH 6/8] Fix typo after rebase --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 91c82323b..503ec537d 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2091,7 +2091,7 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, void *p_rng ) { int ret; - size_t const limbs CHARS_TO_LIMBS( size ); + size_t const limbs = CHARS_TO_LIMBS( size ); size_t const overhead = ( limbs * ciL ) - size; unsigned char *Xp; From 9f6d16ad791e5468ecdb33509cf830577d7aa97a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 2 Jan 2019 17:15:06 +0000 Subject: [PATCH 7/8] Fix preprocessor macro existence check in bignum.c --- library/bignum.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 503ec537d..bdd6af85c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -741,14 +741,19 @@ static mbedtls_mpi_uint mpi_uint_bigendian_to_host( mbedtls_mpi_uint x ) #if ( __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ ) /* For GCC and Clang, have builtins for byte swapping. */ -#if( defined(__GNUC__) && defined(__GNUC_PREREQ) && __GNUC_PREREQ(4,3) ) -#define have_bswap -#elif defined(__clang__) && \ - defined(__has_builtin) && \ - __has_builtin(__builtin_bswap32) && \ - __has_builtin(__builtin_bswap64) +#if defined(__GNUC__) && defined(__GNUC_PREREQ) +#if __GNUC_PREREQ(4,3) #define have_bswap #endif +#endif + +#if defined(__clang__) && defined(__has_builtin) +#if __has_builtin(__builtin_bswap32) && \ + __has_builtin(__builtin_bswap64) +#define have_bswap +#endif +#endif + #if defined(have_bswap) /* The compiler is hopefully able to statically evaluate this! */ switch( sizeof(mbedtls_mpi_uint) ) From 0e810b9648c3bc240d08ecfd01564f725e35ff2d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 3 Jan 2019 17:13:11 +0000 Subject: [PATCH 8/8] Don't call memcpy with NULL pointer in mbedtls_mpi_read_binary() mbedtls_mpi_read_binary() calls memcpy() with the source pointer being the source pointer passed to mbedtls_mpi_read_binary(), the latter may be NULL if the buffer length is 0 (and this happens e.g. in the ECJPAKE test suite). The behavior of memcpy(), in contrast, is undefined when called with NULL source buffer, even if the length of the copy operation is 0. This commit fixes this by explicitly checking that the source pointer is not NULL before calling memcpy(), and skipping the call otherwise. --- library/bignum.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index bdd6af85c..d3d02b1a0 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -823,10 +823,15 @@ int mbedtls_mpi_read_binary( mbedtls_mpi *X, const unsigned char *buf, size_t bu } MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); - Xp = (unsigned char*) X->p; - memcpy( Xp + overhead, buf, buflen ); + /* Avoid calling `memcpy` with NULL source argument, + * even if buflen is 0. */ + if( buf != NULL ) + { + Xp = (unsigned char*) X->p; + memcpy( Xp + overhead, buf, buflen ); - mpi_bigendian_to_host( X->p, limbs ); + mpi_bigendian_to_host( X->p, limbs ); + } cleanup: