From cb2ba29c49be581a267e0a86c7a854ddad65067c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 17 Oct 2017 15:17:05 +0100 Subject: [PATCH 1/6] Mention that mpi_fill_random interprets PRNG output as big-endian --- include/mbedtls/bignum.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 1a5b4b675..030982d86 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -648,6 +648,10 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi * * \return 0 if successful, * MBEDTLS_ERR_MPI_ALLOC_FAILED if memory allocation failed + * + * \note The bytes obtained from the PRNG are interpreted + * as a big-endian representation of an MPI; this can + * be relevant in applications like deterministic ECDSA. */ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, int (*f_rng)(void *, unsigned char *, size_t), From 7d80688e5316663a4cfeaad59e7b29cf89d4634b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 17 Oct 2017 15:17:27 +0100 Subject: [PATCH 2/6] Make mpi_read_binary time constant This commit modifies mpi_read_binary to always allocate the minimum number of limbs required to hold the entire buffer provided to the function, regardless of its content. Previously, leading zero bytes in the input data were detected and used to reduce memory footprint and time, but this non-constant behavior turned out to be non-tolerable for the cryptographic applications this function is used for. --- library/bignum.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 52edd3def..886429206 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -672,16 +672,20 @@ cleanup: int mbedtls_mpi_read_binary( mbedtls_mpi *X, const unsigned char *buf, size_t buflen ) { int ret; - size_t i, j, n; + size_t i, j; + size_t const limbs = CHARS_TO_LIMBS( buflen ); - for( n = 0; n < buflen; n++ ) - if( buf[n] != 0 ) - break; + /* 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_grow( X, CHARS_TO_LIMBS( buflen - n ) ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_lset( X, 0 ) ); - for( i = buflen, j = 0; i > n; i--, j++ ) + for( i = buflen, j = 0; i > 0; i--, j++ ) X->p[j / ciL] |= ((mbedtls_mpi_uint) buf[i - 1]) << ((j % ciL) << 3); cleanup: From b3088b4b378914a058853be2835476255639e4ad Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 17 Oct 2017 15:19:38 +0100 Subject: [PATCH 3/6] Fix information leak in ecp_gen_keypair_base The function mbedtls_ecp_gen_keypair_base did not wipe the stack buffer used to hold the private exponent before returning. This commit fixes this by not using a stack buffer in the first place but instead calling mpi_fill_random directly to acquire the necessary random MPI. --- library/ecp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 1d4f5cba2..5787b9b2c 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1830,7 +1830,6 @@ int mbedtls_ecp_gen_keypair( mbedtls_ecp_group *grp, mbedtls_mpi *d, mbedtls_ecp { /* SEC1 3.2.1: Generate d such that 1 <= n < N */ int count = 0; - unsigned char rnd[MBEDTLS_ECP_MAX_BYTES]; /* * Match the procedure given in RFC 6979 (deterministic ECDSA): @@ -1841,8 +1840,7 @@ int mbedtls_ecp_gen_keypair( mbedtls_ecp_group *grp, mbedtls_mpi *d, mbedtls_ecp */ do { - MBEDTLS_MPI_CHK( f_rng( p_rng, rnd, n_size ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( d, rnd, n_size ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_size, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_size - grp->nbits ) ); /* From 0f49bbc1fc63feafed4b295c1bb0880385faffed Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 18 Oct 2017 12:41:30 +0100 Subject: [PATCH 4/6] Zeroize stack before returning from mpi_fill_random --- library/bignum.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/bignum.c b/library/bignum.c index 886429206..25fe8beeb 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1881,6 +1881,7 @@ int mbedtls_mpi_fill_random( mbedtls_mpi *X, size_t size, MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( X, buf, size ) ); cleanup: + mbedtls_zeroize( buf, sizeof( buf ) ); return( ret ); } From cf873f74d40690e1c76ade0b6dadf228bc71d398 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 19 Oct 2017 09:13:35 +0100 Subject: [PATCH 5/6] Adapt ChangeLog --- ChangeLog | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ChangeLog b/ChangeLog index 8f7843dc6..55e8cf16a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,13 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.1.x released xxxx-xx-xx +Security + * Make mbedtls_mpi_read_binary constant-time with respect to + the input data. Previously, trailing zero bytes were detected + and omitted for the sake of saving memory, but potentially + leading to slight timing differences. + Reported by Marco Macchetti, Kudelski Group. + Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7. From 25e39d38bd295eef1c5cac5976afee2dcf265517 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 19 Oct 2017 10:10:18 +0100 Subject: [PATCH 6/6] Add ChangeLog message for EC private exponent information leak --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 55e8cf16a..4e919c98a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,6 +8,8 @@ Security and omitted for the sake of saving memory, but potentially leading to slight timing differences. Reported by Marco Macchetti, Kudelski Group. + * Wipe stack buffer temporarily holding EC private exponent + after keypair generation. Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records