From 1cc67a0d0ecc46e4916a2a42a567c1f010ae3a37 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 17 Jan 2018 17:38:28 +0000 Subject: [PATCH 1/4] Add missing calls to sha{256/512}_{init/free} in entropy module The entropy context contains a SHA-256 or SHA-512 context for entropy mixing, but doesn't initialize / free this context properly in the initialization and freeing functions `mbedtls_entropy_init` and `mbedtls_entropy_free` through a call to `mbedtls_sha{256/512}_init` resp. `mbedtls_sha{256/512}_free`. Instead, only a zeroization of the entire entropy structure is performed. This doesn't lead to problems for the current software implementations of SHA-256 and SHA-512 because zeroization is proper initialization for them, but it may (and does) cause problems for alternative implementations of SHA-256 and SHA-512 that use context structures that cannot be properly initialized through zeroization. This commit fixes this. Found and fix suggested by ccli8. --- library/entropy.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/library/entropy.c b/library/entropy.c index b45384dbe..d3c132719 100644 --- a/library/entropy.c +++ b/library/entropy.c @@ -68,8 +68,10 @@ void mbedtls_entropy_init( mbedtls_entropy_context *ctx ) #endif #if defined(MBEDTLS_ENTROPY_SHA512_ACCUMULATOR) + mbedtls_sha512_init( &ctx->accumulator ); mbedtls_sha512_starts( &ctx->accumulator, 0 ); #else + mbedtls_sha256_init( &ctx->accumulator ); mbedtls_sha256_starts( &ctx->accumulator, 0 ); #endif #if defined(MBEDTLS_HAVEGE_C) @@ -105,6 +107,13 @@ void mbedtls_entropy_free( mbedtls_entropy_context *ctx ) #if defined(MBEDTLS_HAVEGE_C) mbedtls_havege_free( &ctx->havege_data ); #endif + +#if defined(MBEDTLS_ENTROPY_SHA512_ACCUMULATOR) + mbedtls_sha512_free( &ctx->accumulator ); +#else + mbedtls_sha256_free( &ctx->accumulator ); +#endif + #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_free( &ctx->mutex ); #endif From 4ecd34f86c9c829b2b48f71b9536955875509428 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 17 Jan 2018 17:45:31 +0000 Subject: [PATCH 2/4] Adapt ChangeLog --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index ed7818e30..d853b226c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -20,6 +20,11 @@ Features heavily-loaded machine. Bugfix + * Properly initialize and free SHA-256 / SHA-512 context in entropy module + instead of performing zeroization only. This could lead to failure for + alternative implementations of SHA-256 / SHA-512 for which zeroization + of contexts is not a proper way of initialization. + Found and fix suggested by ccli8. * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7. * Fix memory leak in mbedtls_ssl_set_hostname() when called multiple times. From 31b37f6edd54233cdf67665d10205d9b94b9cc2d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 17 Jan 2018 23:09:20 +0000 Subject: [PATCH 3/4] Use free + init to reset accumulator in entropy module The SHA-256 / SHA-512 context used for entropy mixing in entropy.c was previously reset by zeroization. The commit replaces this by a pair of calls to `mbedtls_shaxxx_init` and `mbedtls_shaxxx_free` which is safe also for alternative implementations of SHA-256 or SHA-512 for which zeroization might not be a proper reset. --- library/entropy.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/entropy.c b/library/entropy.c index d3c132719..8125f644a 100644 --- a/library/entropy.c +++ b/library/entropy.c @@ -318,7 +318,8 @@ int mbedtls_entropy_func( void *data, unsigned char *output, size_t len ) /* * Reset accumulator and counters and recycle existing entropy */ - memset( &ctx->accumulator, 0, sizeof( mbedtls_sha512_context ) ); + mbedtls_sha512_free( &ctx->accumulator ); + mbedtls_sha512_init( &ctx->accumulator ); mbedtls_sha512_starts( &ctx->accumulator, 0 ); mbedtls_sha512_update( &ctx->accumulator, buf, MBEDTLS_ENTROPY_BLOCK_SIZE ); @@ -332,7 +333,8 @@ int mbedtls_entropy_func( void *data, unsigned char *output, size_t len ) /* * Reset accumulator and counters and recycle existing entropy */ - memset( &ctx->accumulator, 0, sizeof( mbedtls_sha256_context ) ); + mbedtls_sha256_free( &ctx->accumulator ); + mbedtls_sha256_init( &ctx->accumulator ); mbedtls_sha256_starts( &ctx->accumulator, 0 ); mbedtls_sha256_update( &ctx->accumulator, buf, MBEDTLS_ENTROPY_BLOCK_SIZE ); From 681f5aacfee113f135ef7c51ed779d1aa17da80a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 12 Mar 2018 23:50:18 +0100 Subject: [PATCH 4/4] Align ChangeLog entry with 2.7 --- ChangeLog | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index d853b226c..e76f646e7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -20,11 +20,6 @@ Features heavily-loaded machine. Bugfix - * Properly initialize and free SHA-256 / SHA-512 context in entropy module - instead of performing zeroization only. This could lead to failure for - alternative implementations of SHA-256 / SHA-512 for which zeroization - of contexts is not a proper way of initialization. - Found and fix suggested by ccli8. * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7. * Fix memory leak in mbedtls_ssl_set_hostname() when called multiple times. @@ -67,6 +62,11 @@ Bugfix * Fix issue in RSA key generation program programs/x509/rsa_genkey where the failure of CTR DRBG initialization lead to freeing an RSA context without proper initialization beforehand. + * Fix the entropy.c module to ensure that mbedtls_sha256_init() or + mbedtls_sha512_init() is called before operating on the relevant context + structure. Do not assume that zeroizing a context is a correct way to + reset it. Found independently by ccli8 on Github. + * In mbedtls_entropy_free(), properly free the message digest context. Changes * Extend cert_write example program by options to set the CRT version