From 6a74b2f687c53e9f2c43f044511f0d87281914a3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Oct 2018 16:46:37 +0100 Subject: [PATCH 1/3] Zeroize sensitive data in aescrypt2 and crypt_and_hash examples This commit replaces multiple `memset()` calls in the example programs aes/aescrypt2.c and aes/crypt_and_hash.c by calls to the reliable zeroization function `mbedtls_zeroize()`. While not a security issue because the code is in the example programs, it's bad practice and should be fixed. --- programs/aes/aescrypt2.c | 18 ++++++++++++------ programs/aes/crypt_and_hash.c | 18 ++++++++++++------ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/programs/aes/aescrypt2.c b/programs/aes/aescrypt2.c index de9d422cd7..939c927a9e 100644 --- a/programs/aes/aescrypt2.c +++ b/programs/aes/aescrypt2.c @@ -69,6 +69,12 @@ int main( void ) return( 0 ); } #else + +/* 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; +} + int main( int argc, char *argv[] ) { int ret = 1; @@ -441,13 +447,13 @@ exit: the case when the user has missed or reordered some, in which case the key might not be in argv[4]. */ for( i = 0; i < argc; i++ ) - memset( argv[i], 0, strlen( argv[i] ) ); + mbedtls_zeroize( argv[i], strlen( argv[i] ) ); - memset( IV, 0, sizeof( IV ) ); - memset( key, 0, sizeof( key ) ); - memset( tmp, 0, sizeof( tmp ) ); - memset( buffer, 0, sizeof( buffer ) ); - memset( digest, 0, sizeof( digest ) ); + mbedtls_zeroize( IV, sizeof( IV ) ); + mbedtls_zeroize( key, sizeof( key ) ); + mbedtls_zeroize( tmp, sizeof( tmp ) ); + mbedtls_zeroize( buffer, sizeof( buffer ) ); + mbedtls_zeroize( digest, sizeof( digest ) ); mbedtls_aes_free( &aes_ctx ); mbedtls_md_free( &sha_ctx ); diff --git a/programs/aes/crypt_and_hash.c b/programs/aes/crypt_and_hash.c index 5885bdb5a8..caad604a8f 100644 --- a/programs/aes/crypt_and_hash.c +++ b/programs/aes/crypt_and_hash.c @@ -71,6 +71,12 @@ int main( void ) return( 0 ); } #else + +/* 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; +} + int main( int argc, char *argv[] ) { int ret = 1, i, n; @@ -533,13 +539,13 @@ exit: the case when the user has missed or reordered some, in which case the key might not be in argv[6]. */ for( i = 0; i < argc; i++ ) - memset( argv[i], 0, strlen( argv[i] ) ); + mbedtls_zeroize( argv[i], strlen( argv[i] ) ); - memset( IV, 0, sizeof( IV ) ); - memset( key, 0, sizeof( key ) ); - memset( buffer, 0, sizeof( buffer ) ); - memset( output, 0, sizeof( output ) ); - memset( digest, 0, sizeof( digest ) ); + mbedtls_zeroize( IV, sizeof( IV ) ); + mbedtls_zeroize( key, sizeof( key ) ); + mbedtls_zeroize( buffer, sizeof( buffer ) ); + mbedtls_zeroize( output, sizeof( output ) ); + mbedtls_zeroize( digest, sizeof( digest ) ); mbedtls_cipher_free( &cipher_ctx ); mbedtls_md_free( &md_ctx ); From 728d6cdcef3cd327ba5ac03ee0975017b0efd5b2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 15 Oct 2018 13:22:22 +0100 Subject: [PATCH 2/3] Add missing zeroization of reassembled handshake messages This commit ensures that buffers holding fragmented or handshake messages get zeroized before they are freed when the respective handshake message is no longer needed. Previously, the handshake message content would leak on the heap. --- library/ssl_tls.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 80917956e3..00ae9fcc36 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3212,6 +3212,7 @@ static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) memcpy( ssl->in_msg, ssl->handshake->hs_msg, ssl->in_hslen ); + mbedtls_zeroize( ssl->handshake->hs_msg, ssl->in_hslen ); mbedtls_free( ssl->handshake->hs_msg ); ssl->handshake->hs_msg = NULL; From f5b094fc723ded00c239e8d9389319ad6c48a22d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 15 Oct 2018 13:20:28 +0100 Subject: [PATCH 3/3] Adapt ChangeLog --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 8c82d08d1f..31380c9644 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Bugfix MBEDTLS_THREADING_C is defined. Found by TrinityTonic, #1095 * Fix a bug in the update function for SSL ticket keys which previously invalidated keys of a lifetime of less than a 1s. Fixes #1968. + * Zeroize memory used for reassembling handshake messages after use. + * Use `mbedtls_zeroize()` instead of `memset()` for zeroization of + sensitive data in the example programs aescrypt2 and crypt_and_hash. Changes * Add tests for session resumption in DTLS.