From 57f8e9116eea0c34bd4051479e7b9d0cf3333973 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 22 Feb 2021 21:26:54 +0100 Subject: [PATCH] Make entropy double-free work Although the library documentation does not guarantee that calling mbedtls_entropy_free() twice works, it's a plausible assumption and it's natural to write code that frees an object twice. While this is uncommon for an entropy context, which is usually a global variable, it came up in our own unit tests (random_twice tests in test_suite_random in the development branch). Announce this in the same changelog entry as for RSA because it's the same bug in the two modules. Signed-off-by: Gilles Peskine --- ChangeLog.d/rsa-mutex.txt | 8 ++++---- include/mbedtls/entropy.h | 6 ++++-- library/entropy.c | 7 ++++++- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/ChangeLog.d/rsa-mutex.txt b/ChangeLog.d/rsa-mutex.txt index 49f1a84f2..2a477a9cb 100644 --- a/ChangeLog.d/rsa-mutex.txt +++ b/ChangeLog.d/rsa-mutex.txt @@ -1,8 +1,8 @@ Bugfix - * Ensure that calling mbedtls_rsa_free() twice is safe. This happens - when some Mbed TLS library functions fail. Such a double-free was - not safe when MBEDTLS_THREADING_C was enabled on platforms where - freeing a mutex twice is not safe. + * Ensure that calling mbedtls_rsa_free() or mbedtls_entropy_free() + twice is safe. This happens for RSA when some Mbed TLS library functions + fail. Such a double-free was not safe when MBEDTLS_THREADING_C was + enabled on platforms where freeing a mutex twice is not safe. * Fix a resource leak in a bad-arguments case of mbedtls_rsa_gen_key() when MBEDTLS_THREADING_C is enabled on platforms where initializing a mutex allocates resources. diff --git a/include/mbedtls/entropy.h b/include/mbedtls/entropy.h index 1e1d3f56e..1d6e9b821 100644 --- a/include/mbedtls/entropy.h +++ b/include/mbedtls/entropy.h @@ -147,13 +147,15 @@ mbedtls_entropy_source_state; */ typedef struct mbedtls_entropy_context { - int accumulator_started; + int accumulator_started; /* 0 after init. + * 1 after the first update. + * -1 after free. */ #if defined(MBEDTLS_ENTROPY_SHA512_ACCUMULATOR) mbedtls_sha512_context accumulator; #else mbedtls_sha256_context accumulator; #endif - int source_count; + int source_count; /* Number of entries used in source. */ mbedtls_entropy_source_state source[MBEDTLS_ENTROPY_MAX_SOURCES]; #if defined(MBEDTLS_HAVEGE_C) mbedtls_havege_state havege_data; diff --git a/library/entropy.c b/library/entropy.c index 666c55654..c5f414a01 100644 --- a/library/entropy.c +++ b/library/entropy.c @@ -146,6 +146,11 @@ void mbedtls_entropy_init( mbedtls_entropy_context *ctx ) void mbedtls_entropy_free( mbedtls_entropy_context *ctx ) { + /* If the context was already free, don't call free() again. + * This is important for mutexes which don't allow double-free. */ + if( ctx->accumulator_started == -1 ) + return; + #if defined(MBEDTLS_HAVEGE_C) mbedtls_havege_free( &ctx->havege_data ); #endif @@ -162,7 +167,7 @@ void mbedtls_entropy_free( mbedtls_entropy_context *ctx ) #endif ctx->source_count = 0; mbedtls_platform_zeroize( ctx->source, sizeof( ctx->source ) ); - ctx->accumulator_started = 0; + ctx->accumulator_started = -1; } int mbedtls_entropy_add_source( mbedtls_entropy_context *ctx,