From 9a1a151a1a0597e6a8f458304ef742640950c61b 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 c727f936e..9e98f1df0 100644 --- a/programs/aes/aescrypt2.c +++ b/programs/aes/aescrypt2.c @@ -72,6 +72,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 = 0; @@ -445,13 +451,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 < (unsigned int) 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 99d30c9a9..5024f4a6f 100644 --- a/programs/aes/crypt_and_hash.c +++ b/programs/aes/crypt_and_hash.c @@ -74,6 +74,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; @@ -542,13 +548,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 d82e0c0235242fd5829ff2da89e649d98fa0d366 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 da55801f6..c6b17ae27 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3235,6 +3235,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 74a1c4b1780023c8d9139dd9782aa280d2e6775c 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 0a9dc4f8d..7855a400a 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.