From 6497b5a1d10e59caa45c77bdcfacfeada1337ac0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Jun 2022 17:01:40 +0200 Subject: [PATCH 1/8] Add setbuf platform function Add a platform function mbedtls_setbuf(), defaulting to setbuf(). The intent is to allow disabling stdio buffering when reading or writing files with sensitive data, because this exposes the sensitive data to a subsequent memory disclosure vulnerability. Signed-off-by: Gilles Peskine --- include/mbedtls/check_config.h | 14 ++++++++ include/mbedtls/mbedtls_config.h | 3 ++ include/mbedtls/platform.h | 55 ++++++++++++++++++++++++++++++++ library/platform.c | 28 ++++++++++++++++ tests/scripts/all.sh | 1 + 5 files changed, 101 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 1ced6e578..0ac9ccac4 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -368,6 +368,20 @@ #error "MBEDTLS_PLATFORM_EXIT_MACRO and MBEDTLS_PLATFORM_STD_EXIT/MBEDTLS_PLATFORM_EXIT_ALT cannot be defined simultaneously" #endif +#if defined(MBEDTLS_PLATFORM_SETBUF_ALT) && !defined(MBEDTLS_PLATFORM_C) +#error "MBEDTLS_PLATFORM_SETBUF_ALT defined, but not all prerequisites" +#endif + +#if defined(MBEDTLS_PLATFORM_SETBUF_MACRO) && !defined(MBEDTLS_PLATFORM_C) +#error "MBEDTLS_PLATFORM_SETBUF_MACRO defined, but not all prerequisites" +#endif + +#if defined(MBEDTLS_PLATFORM_SETBUF_MACRO) &&\ + ( defined(MBEDTLS_PLATFORM_STD_SETBUF) ||\ + defined(MBEDTLS_PLATFORM_SETBUF_ALT) ) +#error "MBEDTLS_PLATFORM_SETBUF_MACRO and MBEDTLS_PLATFORM_STD_SETBUF/MBEDTLS_PLATFORM_SETBUF_ALT cannot be defined simultaneously" +#endif + #if defined(MBEDTLS_PLATFORM_TIME_ALT) &&\ ( !defined(MBEDTLS_PLATFORM_C) ||\ !defined(MBEDTLS_HAVE_TIME) ) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 2d32f67cc..ad9696135 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -225,6 +225,7 @@ * Uncomment a macro to enable alternate implementation of specific base * platform function */ +//#define MBEDTLS_PLATFORM_SETBUF_ALT //#define MBEDTLS_PLATFORM_EXIT_ALT //#define MBEDTLS_PLATFORM_TIME_ALT //#define MBEDTLS_PLATFORM_FPRINTF_ALT @@ -3288,6 +3289,7 @@ //#define MBEDTLS_PLATFORM_STD_MEM_HDR /**< Header to include if MBEDTLS_PLATFORM_NO_STD_FUNCTIONS is defined. Don't define if no header is needed. */ //#define MBEDTLS_PLATFORM_STD_CALLOC calloc /**< Default allocator to use, can be undefined */ //#define MBEDTLS_PLATFORM_STD_FREE free /**< Default free to use, can be undefined */ +//#define MBEDTLS_PLATFORM_STD_SETBUF setbuf /**< Default setbuf to use, can be undefined */ //#define MBEDTLS_PLATFORM_STD_EXIT exit /**< Default exit to use, can be undefined */ //#define MBEDTLS_PLATFORM_STD_TIME time /**< Default time to use, can be undefined. MBEDTLS_HAVE_TIME must be enabled */ //#define MBEDTLS_PLATFORM_STD_FPRINTF fprintf /**< Default fprintf to use, can be undefined */ @@ -3305,6 +3307,7 @@ //#define MBEDTLS_PLATFORM_CALLOC_MACRO calloc /**< Default allocator macro to use, can be undefined */ //#define MBEDTLS_PLATFORM_FREE_MACRO free /**< Default free macro to use, can be undefined */ //#define MBEDTLS_PLATFORM_EXIT_MACRO exit /**< Default exit macro to use, can be undefined */ +//#define MBEDTLS_PLATFORM_SETBUF_MACRO setbuf /**< Default setbuf macro to use, can be undefined */ //#define MBEDTLS_PLATFORM_TIME_MACRO time /**< Default time macro to use, can be undefined. MBEDTLS_HAVE_TIME must be enabled */ //#define MBEDTLS_PLATFORM_TIME_TYPE_MACRO time_t /**< Default time macro to use, can be undefined. MBEDTLS_HAVE_TIME must be enabled */ //#define MBEDTLS_PLATFORM_FPRINTF_MACRO fprintf /**< Default fprintf macro to use, can be undefined */ diff --git a/include/mbedtls/platform.h b/include/mbedtls/platform.h index a5984345b..bad442e72 100644 --- a/include/mbedtls/platform.h +++ b/include/mbedtls/platform.h @@ -91,6 +91,9 @@ extern "C" { #if !defined(MBEDTLS_PLATFORM_STD_FREE) #define MBEDTLS_PLATFORM_STD_FREE free /**< The default \c free function to use. */ #endif +#if !defined(MBEDTLS_PLATFORM_STD_SETBUF) +#define MBEDTLS_PLATFORM_STD_SETBUF setbuf /**< The default \c setbuf function to use. */ +#endif #if !defined(MBEDTLS_PLATFORM_STD_EXIT) #define MBEDTLS_PLATFORM_STD_EXIT exit /**< The default \c exit function to use. */ #endif @@ -276,6 +279,58 @@ int mbedtls_platform_set_vsnprintf( int (*vsnprintf_func)( char * s, size_t n, #endif /* MBEDTLS_PLATFORM_VSNPRINTF_MACRO */ #endif /* MBEDTLS_PLATFORM_VSNPRINTF_ALT */ +/* + * The function pointers for setbuf + */ +#if defined(MBEDTLS_PLATFORM_SETBUF_ALT) +#include +/** + * \brief Function pointer to call for `setbuf()` functionality + * (changing the internal buffering on stdio calls). + * + * \note The library calls this function to disable + * buffering when reading or writing sensitive data, + * to avoid having extra copies of sensitive data + * remaining in stdio buffers after the file is + * closed. If this is not a concern, for example if + * your platform's stdio doesn't have any buffering, + * you can set mbedtls_setbuf to a function that + * does nothing. + * + * \param setbuf_func The \c setbuf function implementation. + * It is always called with `buf` equal to `NULL`. + * + * \return \c 0 on success, negative on error. + */ +extern void (*mbedtls_setbuf)( FILE *stream, char *buf ); + +/** + * \brief Dynamically configure the function that is called + * when the mbedtls_setbuf() function is called by the + * library. + * + * \param setbuf_func The \c setbuf function implementation + * + * \return \c 0 + */ +int mbedtls_platform_set_setbuf( void (*setbuf_func)( + FILE *stream, char *buf ) ); +#elif defined(MBEDTLS_PLATFORM_SETBUF_MACRO) +/** + * \brief Macro defining the function for the library to + * call for `setbuf` functionality (changing the + * internal buffering on stdio calls). + * + * \note See extra comments on the mbedtls_setbuf() function + * pointer above. + * + * \return \c 0 on success, negative on error. + */ +#define mbedtls_setbuf MBEDTLS_PLATFORM_SETBUF_MACRO +#else +#define mbedtls_setbuf setbuf +#endif /* MBEDTLS_PLATFORM_SETBUF_ALT / MBEDTLS_PLATFORM_SETBUF_MACRO */ + /* * The function pointers for exit */ diff --git a/library/platform.c b/library/platform.c index e742fde7c..f3a1f9847 100644 --- a/library/platform.c +++ b/library/platform.c @@ -226,6 +226,28 @@ int mbedtls_platform_set_fprintf( int (*fprintf_func)( FILE *, const char *, ... } #endif /* MBEDTLS_PLATFORM_FPRINTF_ALT */ +#if defined(MBEDTLS_PLATFORM_SETBUF_ALT) +#if !defined(MBEDTLS_PLATFORM_STD_SETBUF) +/* + * Make dummy function to prevent NULL pointer dereferences + */ +static void platform_setbuf_uninit( FILE *stream, char *buf ) +{ + (( void ) stream); + (( void ) buf); +} + +#define MBEDTLS_PLATFORM_STD_SETBUF platform_setbuf_uninit +#endif /* !MBEDTLS_PLATFORM_STD_SETBUF */ +void (*mbedtls_setbuf)( FILE *stream, char *buf ) = MBEDTLS_PLATFORM_STD_SETBUF; + +int mbedtls_platform_set_setbuf( void (*setbuf_func)( FILE *stream, char *buf ) ) +{ + mbedtls_setbuf = setbuf_func; + return( 0 ); +} +#endif /* MBEDTLS_PLATFORM_SETBUF_ALT */ + #if defined(MBEDTLS_PLATFORM_EXIT_ALT) #if !defined(MBEDTLS_PLATFORM_STD_EXIT) /* @@ -288,6 +310,9 @@ int mbedtls_platform_std_nv_seed_read( unsigned char *buf, size_t buf_len ) if( ( file = fopen( MBEDTLS_PLATFORM_STD_NV_SEED_FILE, "rb" ) ) == NULL ) return( -1 ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( file, NULL ); + if( ( n = fread( buf, 1, buf_len, file ) ) != buf_len ) { fclose( file ); @@ -307,6 +332,9 @@ int mbedtls_platform_std_nv_seed_write( unsigned char *buf, size_t buf_len ) if( ( file = fopen( MBEDTLS_PLATFORM_STD_NV_SEED_FILE, "w" ) ) == NULL ) return -1; + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( file, NULL ); + if( ( n = fwrite( buf, 1, buf_len, file ) ) != buf_len ) { fclose( file ); diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 1e5cd65ee..1a3a04b82 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2178,6 +2178,7 @@ component_test_no_platform () { scripts/config.py unset MBEDTLS_PLATFORM_SNPRINTF_ALT scripts/config.py unset MBEDTLS_PLATFORM_TIME_ALT scripts/config.py unset MBEDTLS_PLATFORM_EXIT_ALT + scripts/config.py unset MBEDTLS_PLATFORM_SETBUF_ALT scripts/config.py unset MBEDTLS_PLATFORM_NV_SEED_ALT scripts/config.py unset MBEDTLS_ENTROPY_NV_SEED scripts/config.py unset MBEDTLS_FS_IO From da0913ba6b3ddd5708189df8115d85bbd0ff2be3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Jun 2022 17:03:40 +0200 Subject: [PATCH 2/8] Call setbuf when reading or writing files: library After opening a file containing sensitive data, call mbedtls_setbuf() to disable buffering. This way, we don't expose sensitive data to a memory disclosure vulnerability in a buffer outside our control. This commit adds a call to mbedtls_setbuf() after each call to fopen(), except: * In ctr_drbg.c, in load_file(), because this is only used for DH parameters and they are not confidential data. * In psa_its_file.c, in psa_its_remove(), because the file is only opened to check its existence, we don't read data from it. Signed-off-by: Gilles Peskine --- library/ctr_drbg.c | 6 ++++++ library/dhm.c | 1 + library/entropy.c | 6 ++++++ library/entropy_poll.c | 5 ++++- library/hmac_drbg.c | 6 ++++++ library/md.c | 3 +++ library/pkparse.c | 3 +++ library/psa_its_file.c | 7 +++++++ 8 files changed, 36 insertions(+), 1 deletion(-) diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 93a7cdcd1..a2f55cc59 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -607,6 +607,9 @@ int mbedtls_ctr_drbg_write_seed_file( mbedtls_ctr_drbg_context *ctx, if( ( f = fopen( path, "wb" ) ) == NULL ) return( MBEDTLS_ERR_CTR_DRBG_FILE_IO_ERROR ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + if( ( ret = mbedtls_ctr_drbg_random( ctx, buf, MBEDTLS_CTR_DRBG_MAX_INPUT ) ) != 0 ) goto exit; @@ -640,6 +643,9 @@ int mbedtls_ctr_drbg_update_seed_file( mbedtls_ctr_drbg_context *ctx, if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_CTR_DRBG_FILE_IO_ERROR ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + n = fread( buf, 1, sizeof( buf ), f ); if( fread( &c, 1, 1, f ) != 0 ) { diff --git a/library/dhm.c b/library/dhm.c index 2ce0ed4fd..1e95bdab0 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -620,6 +620,7 @@ static int load_file( const char *path, unsigned char **buf, size_t *n ) if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_DHM_FILE_IO_ERROR ); + /* The data loaded here is public, so don't bother disabling buffering. */ fseek( f, 0, SEEK_END ); if( ( size = ftell( f ) ) == -1 ) diff --git a/library/entropy.c b/library/entropy.c index 9e31f8491..08c5bd7d1 100644 --- a/library/entropy.c +++ b/library/entropy.c @@ -457,6 +457,9 @@ int mbedtls_entropy_write_seed_file( mbedtls_entropy_context *ctx, const char *p goto exit; } + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + if( fwrite( buf, 1, MBEDTLS_ENTROPY_BLOCK_SIZE, f ) != MBEDTLS_ENTROPY_BLOCK_SIZE ) { ret = MBEDTLS_ERR_ENTROPY_FILE_IO_ERROR; @@ -484,6 +487,9 @@ int mbedtls_entropy_update_seed_file( mbedtls_entropy_context *ctx, const char * if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_ENTROPY_FILE_IO_ERROR ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + fseek( f, 0, SEEK_END ); n = (size_t) ftell( f ); fseek( f, 0, SEEK_SET ); diff --git a/library/entropy_poll.c b/library/entropy_poll.c index 058c307df..2ae57fdc0 100644 --- a/library/entropy_poll.c +++ b/library/entropy_poll.c @@ -35,7 +35,7 @@ #if defined(MBEDTLS_TIMING_C) #include "mbedtls/timing.h" #endif -#if defined(MBEDTLS_ENTROPY_NV_SEED) +#if defined(MBEDTLS_ENTROPY_NV_SEED) || !defined(HAVE_SYSCTL_ARND) #include "mbedtls/platform.h" #endif @@ -195,6 +195,9 @@ int mbedtls_platform_entropy_poll( void *data, if( file == NULL ) return( MBEDTLS_ERR_ENTROPY_SOURCE_FAILED ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( file, NULL ); + read_len = fread( output, 1, len, file ); if( read_len != len ) { diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index ab353bfd5..8b13a860f 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -436,6 +436,9 @@ int mbedtls_hmac_drbg_write_seed_file( mbedtls_hmac_drbg_context *ctx, const cha if( ( f = fopen( path, "wb" ) ) == NULL ) return( MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + if( ( ret = mbedtls_hmac_drbg_random( ctx, buf, sizeof( buf ) ) ) != 0 ) goto exit; @@ -465,6 +468,9 @@ int mbedtls_hmac_drbg_update_seed_file( mbedtls_hmac_drbg_context *ctx, const ch if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + n = fread( buf, 1, sizeof( buf ), f ); if( fread( &c, 1, 1, f ) != 0 ) { diff --git a/library/md.c b/library/md.c index f2c1a90f8..a387da50a 100644 --- a/library/md.c +++ b/library/md.c @@ -605,6 +605,9 @@ int mbedtls_md_file( const mbedtls_md_info_t *md_info, const char *path, unsigne if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_MD_FILE_IO_ERROR ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + mbedtls_md_init( &ctx ); if( ( ret = mbedtls_md_setup( &ctx, md_info, 0 ) ) != 0 ) diff --git a/library/pkparse.c b/library/pkparse.c index 22dab3ad7..0e5dd5546 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -82,6 +82,9 @@ int mbedtls_pk_load_file( const char *path, unsigned char **buf, size_t *n ) if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_PK_FILE_IO_ERROR ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( f, NULL ); + fseek( f, 0, SEEK_END ); if( ( size = ftell( f ) ) == -1 ) { diff --git a/library/psa_its_file.c b/library/psa_its_file.c index f05872095..b7c2e6b04 100644 --- a/library/psa_its_file.c +++ b/library/psa_its_file.c @@ -102,6 +102,9 @@ static psa_status_t psa_its_read_file( psa_storage_uid_t uid, if( *p_stream == NULL ) return( PSA_ERROR_DOES_NOT_EXIST ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( *p_stream, NULL ); + n = fread( &header, 1, sizeof( header ), *p_stream ); if( n != sizeof( header ) ) return( PSA_ERROR_DATA_CORRUPT ); @@ -201,9 +204,13 @@ psa_status_t psa_its_set( psa_storage_uid_t uid, psa_its_fill_filename( uid, filename ); stream = fopen( PSA_ITS_STORAGE_TEMP, "wb" ); + if( stream == NULL ) goto exit; + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( stream, NULL ); + status = PSA_ERROR_INSUFFICIENT_STORAGE; n = fwrite( &header, 1, sizeof( header ), stream ); if( n != sizeof( header ) ) From 6d576c9646fec898d938232d03aaad7c048e6aa1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Jun 2022 17:06:11 +0200 Subject: [PATCH 3/8] Call setbuf when reading or writing files: programs After opening a file containing sensitive data, call mbedtls_setbuf() to disable buffering. This way, we don't expose sensitive data to a memory disclosure vulnerability in a buffer outside our control. This commit adds a call to mbedtls_setbuf() after each call to fopen(), but only in sample programs that were calling mbedtls_platform_zeroize(). Don't bother protecting stdio buffers in programs where application buffers weren't protected. Signed-off-by: Gilles Peskine --- programs/aes/crypt_and_hash.c | 4 ++++ programs/psa/key_ladder_demo.c | 13 +++++++++++++ programs/ssl/ssl_test_common_source.c | 4 ++++ programs/ssl/ssl_test_lib.h | 1 + 4 files changed, 22 insertions(+) diff --git a/programs/aes/crypt_and_hash.c b/programs/aes/crypt_and_hash.c index 74ea88c3c..fa874b240 100644 --- a/programs/aes/crypt_and_hash.c +++ b/programs/aes/crypt_and_hash.c @@ -171,6 +171,10 @@ int main( int argc, char *argv[] ) goto exit; } + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( fin, NULL ); + mbedtls_setbuf( fout, NULL ); + /* * Read the Cipher and MD from the command line */ diff --git a/programs/psa/key_ladder_demo.c b/programs/psa/key_ladder_demo.c index cad875e01..13037190d 100644 --- a/programs/psa/key_ladder_demo.c +++ b/programs/psa/key_ladder_demo.c @@ -56,6 +56,7 @@ #include #include +#include "mbedtls/platform.h" // for mbedtls_setbuf #include "mbedtls/platform_util.h" // for mbedtls_platform_zeroize #include @@ -177,6 +178,8 @@ static psa_status_t save_key( psa_key_id_t key, key_data, sizeof( key_data ), &key_size ) ); SYS_CHECK( ( key_file = fopen( output_file_name, "wb" ) ) != NULL ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( key_file, NULL ); SYS_CHECK( fwrite( key_data, 1, key_size, key_file ) == key_size ); SYS_CHECK( fclose( key_file ) == 0 ); key_file = NULL; @@ -231,6 +234,8 @@ static psa_status_t import_key_from_file( psa_key_usage_t usage, unsigned char extra_byte; SYS_CHECK( ( key_file = fopen( key_file_name, "rb" ) ) != NULL ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( key_file, NULL ); SYS_CHECK( ( key_size = fread( key_data, 1, sizeof( key_data ), key_file ) ) != 0 ); if( fread( &extra_byte, 1, 1, key_file ) != 0 ) @@ -372,6 +377,8 @@ static psa_status_t wrap_data( const char *input_file_name, /* Find the size of the data to wrap. */ SYS_CHECK( ( input_file = fopen( input_file_name, "rb" ) ) != NULL ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( input_file, NULL ); SYS_CHECK( fseek( input_file, 0, SEEK_END ) == 0 ); SYS_CHECK( ( input_position = ftell( input_file ) ) != -1 ); #if LONG_MAX > SIZE_MAX @@ -418,6 +425,8 @@ static psa_status_t wrap_data( const char *input_file_name, /* Write the output. */ SYS_CHECK( ( output_file = fopen( output_file_name, "wb" ) ) != NULL ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( output_file, NULL ); SYS_CHECK( fwrite( &header, 1, sizeof( header ), output_file ) == sizeof( header ) ); SYS_CHECK( fwrite( buffer, 1, ciphertext_size, @@ -453,6 +462,8 @@ static psa_status_t unwrap_data( const char *input_file_name, /* Load and validate the header. */ SYS_CHECK( ( input_file = fopen( input_file_name, "rb" ) ) != NULL ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( input_file, NULL ); SYS_CHECK( fread( &header, 1, sizeof( header ), input_file ) == sizeof( header ) ); if( memcmp( &header.magic, WRAPPED_DATA_MAGIC, @@ -509,6 +520,8 @@ static psa_status_t unwrap_data( const char *input_file_name, /* Write the output. */ SYS_CHECK( ( output_file = fopen( output_file_name, "wb" ) ) != NULL ); + /* Ensure no stdio buffering of secrets, as such buffers cannot be wiped. */ + mbedtls_setbuf( output_file, NULL ); SYS_CHECK( fwrite( buffer, 1, plaintext_size, output_file ) == plaintext_size ); SYS_CHECK( fclose( output_file ) == 0 ); diff --git a/programs/ssl/ssl_test_common_source.c b/programs/ssl/ssl_test_common_source.c index 0e66895db..23237d1a2 100644 --- a/programs/ssl/ssl_test_common_source.c +++ b/programs/ssl/ssl_test_common_source.c @@ -101,6 +101,10 @@ void nss_keylog_export( void *p_expkey, goto exit; } + /* Ensure no stdio buffering of secrets, as such buffers cannot be + * wiped. */ + mbedtls_setbuf( f, NULL ); + if( fwrite( nss_keylog_line, 1, len, f ) != len ) { fclose( f ); diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index f0d0c3b89..d2354bbd4 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -35,6 +35,7 @@ #define mbedtls_fprintf fprintf #define mbedtls_snprintf snprintf #define mbedtls_exit exit +#define mbedtls_setbuf setbuf #define MBEDTLS_EXIT_SUCCESS EXIT_SUCCESS #define MBEDTLS_EXIT_FAILURE EXIT_FAILURE #endif From cf4d9f98c79b45d7b5f38880d0bb7535d5b408b0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Jun 2022 17:07:47 +0200 Subject: [PATCH 4/8] Changelog entry for mbedtls_setbuf() * Security: we're improving a countermeasure. * Requirement change: the library will no longer compile on a platform without setbuf(). Signed-off-by: Gilles Peskine --- ChangeLog.d/add_mbedtls_setbuf.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 ChangeLog.d/add_mbedtls_setbuf.txt diff --git a/ChangeLog.d/add_mbedtls_setbuf.txt b/ChangeLog.d/add_mbedtls_setbuf.txt new file mode 100644 index 000000000..6152d60df --- /dev/null +++ b/ChangeLog.d/add_mbedtls_setbuf.txt @@ -0,0 +1,10 @@ +Security + * Add the platform function mbedtls_setbuf() to allow buffering to be + disabled on stdio files, to stop secrets loaded from said files being + potentially left in memory after file operations. Reported by + Glenn Strauss. +Requirement changes + * The library will no longer compile out of the box on a platform without + setbuf() if MBEDTLS_FS_IO is enabled. If your platform does not have + setbuf(), you can configure an alternative function by enabling + MBEDTLS_PLATFORM_SETBUF_ALT or MBEDTLS_PLATFORM_SETBUF_MACRO. From 0bd76ee2eda38c4813477eea05a86e8c76283488 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Jun 2022 19:32:02 +0200 Subject: [PATCH 5/8] Fix Doxygen documentation attached to non-existent elements Signed-off-by: Gilles Peskine --- include/mbedtls/platform.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/platform.h b/include/mbedtls/platform.h index bad442e72..a5a43ac6d 100644 --- a/include/mbedtls/platform.h +++ b/include/mbedtls/platform.h @@ -297,10 +297,8 @@ int mbedtls_platform_set_vsnprintf( int (*vsnprintf_func)( char * s, size_t n, * you can set mbedtls_setbuf to a function that * does nothing. * - * \param setbuf_func The \c setbuf function implementation. - * It is always called with `buf` equal to `NULL`. - * - * \return \c 0 on success, negative on error. + * The library always calls this function with + * `buf` equal to `NULL`. */ extern void (*mbedtls_setbuf)( FILE *stream, char *buf ); From ff15dbab4cfb308a8c6bf2036d1b192c8f185f7a Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 1 Jul 2022 16:30:08 +0100 Subject: [PATCH 6/8] Make definition order a bit neater Signed-off-by: Paul Elliott --- programs/ssl/ssl_test_lib.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index d2354bbd4..e37a9d8f3 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -34,8 +34,8 @@ #define mbedtls_printf printf #define mbedtls_fprintf fprintf #define mbedtls_snprintf snprintf -#define mbedtls_exit exit #define mbedtls_setbuf setbuf +#define mbedtls_exit exit #define MBEDTLS_EXIT_SUCCESS EXIT_SUCCESS #define MBEDTLS_EXIT_FAILURE EXIT_FAILURE #endif From dfb5da2a99f8c3118914f86a1b295bf622ca73af Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 1 Jul 2022 16:32:14 +0100 Subject: [PATCH 7/8] Fix changelog requirements section. Setbuf() is currently not guarded by MBEDTLS_FS_IO, nor can it easily be so. Raised a seperate issue to cover this, and removed the mention of MBEDTLS_FS_IO from the Changelog. Signed-off-by: Paul Elliott --- ChangeLog.d/add_mbedtls_setbuf.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ChangeLog.d/add_mbedtls_setbuf.txt b/ChangeLog.d/add_mbedtls_setbuf.txt index 6152d60df..d14cd18aa 100644 --- a/ChangeLog.d/add_mbedtls_setbuf.txt +++ b/ChangeLog.d/add_mbedtls_setbuf.txt @@ -5,6 +5,7 @@ Security Glenn Strauss. Requirement changes * The library will no longer compile out of the box on a platform without - setbuf() if MBEDTLS_FS_IO is enabled. If your platform does not have - setbuf(), you can configure an alternative function by enabling - MBEDTLS_PLATFORM_SETBUF_ALT or MBEDTLS_PLATFORM_SETBUF_MACRO. + setbuf(). If your platform does not have setbuf(), you can configure an + alternative function by enabling MBEDTLS_PLATFORM_SETBUF_ALT or + MBEDTLS_PLATFORM_SETBUF_MACRO. + From c466ec2e73a8ce808f99fbee0411b8fb98a66e9c Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 1 Jul 2022 16:43:25 +0100 Subject: [PATCH 8/8] Fix code formatting Signed-off-by: Paul Elliott --- library/platform.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/platform.c b/library/platform.c index f3a1f9847..6151e6c49 100644 --- a/library/platform.c +++ b/library/platform.c @@ -233,8 +233,8 @@ int mbedtls_platform_set_fprintf( int (*fprintf_func)( FILE *, const char *, ... */ static void platform_setbuf_uninit( FILE *stream, char *buf ) { - (( void ) stream); - (( void ) buf); + ((void) stream); + ((void) buf); } #define MBEDTLS_PLATFORM_STD_SETBUF platform_setbuf_uninit