From a0d5d77b2ded3a63787b9c36b5f5de2a63c3afdd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 13 Jan 2021 22:24:51 +0100 Subject: [PATCH 01/24] Use $ASAN_FLAGS instead of repeating its contents Signed-off-by: Gilles Peskine --- tests/scripts/all.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index bd95a21aa..abb6668ae 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1193,7 +1193,7 @@ component_test_malloc_0_null () { msg "build: malloc(0) returns NULL (ASan+UBSan build)" scripts/config.pl full scripts/config.pl unset MBEDTLS_MEMORY_BUFFER_ALLOC_C - make CC=gcc CFLAGS="'-DMBEDTLS_CONFIG_FILE=\"$PWD/tests/configs/config-wrapper-malloc-0-null.h\"' -O -Werror -Wall -Wextra -fsanitize=address,undefined" LDFLAGS='-fsanitize=address,undefined' + make CC=gcc CFLAGS="'-DMBEDTLS_CONFIG_FILE=\"$PWD/tests/configs/config-wrapper-malloc-0-null.h\"' -O $ASAN_CFLAGS" LDFLAGS="$ASAN_CFLAGS" msg "test: malloc(0) returns NULL (ASan+UBSan build)" make test From 04ea1064a61e5913bcade25598729173d4e076b7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 11 Sep 2019 13:27:48 +0200 Subject: [PATCH 02/24] Declare MBEDTLS_TEST_HOOKS in config.h When this option is enabled, the product includes additional interfaces that enable additional tests. This option should not be enabled in production, but is included in the "full" build to enable the extra tests. Signed-off-by: Gilles Peskine --- include/mbedtls/config.h | 20 ++++++++++++++++++++ library/version_features.c | 3 +++ 2 files changed, 23 insertions(+) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index d23a7efcc..9fac9452b 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1549,6 +1549,26 @@ */ //#define MBEDTLS_SSL_TRUNCATED_HMAC_COMPAT +/** + * \def MBEDTLS_TEST_HOOKS + * + * Enable features for invasive testing such as introspection functions and + * hooks for fault injection. This enables additional unit tests. + * + * Merely enabling this feature should not change the behavior of the product. + * It only adds new code, and new branching points where the default behavior + * is the same as when this feature is disabled. + * However, this feature increases the attack surface: there is an added + * risk of vulnerabilities, and more gadgets that can make exploits easier. + * Therefore this feature must never be enabled in production. + * + * See `docs/architecture/testing/mbed-crypto-invasive-testing.md` for more + * information. + * + * Uncomment to enable invasive tests. + */ +//#define MBEDTLS_TEST_HOOKS + /** * \def MBEDTLS_THREADING_ALT * diff --git a/library/version_features.c b/library/version_features.c index a3e3df4f9..ee13f7898 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -508,6 +508,9 @@ static const char *features[] = { #if defined(MBEDTLS_SSL_TRUNCATED_HMAC_COMPAT) "MBEDTLS_SSL_TRUNCATED_HMAC_COMPAT", #endif /* MBEDTLS_SSL_TRUNCATED_HMAC_COMPAT */ +#if defined(MBEDTLS_TEST_HOOKS) + "MBEDTLS_TEST_HOOKS", +#endif /* MBEDTLS_TEST_HOOKS */ #if defined(MBEDTLS_THREADING_ALT) "MBEDTLS_THREADING_ALT", #endif /* MBEDTLS_THREADING_ALT */ From da174241b8da63d787c9f41ae397efeb96d67094 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 10 Feb 2021 13:15:08 +0100 Subject: [PATCH 03/24] Remove reference to a document that doesn't exist in this branch Don't reference the architecture document. This is an LTS branch and new invasive tests are only going to be introduced as (perhaps partial or adapted) backports of invasive tests from development anyway. Signed-off-by: Gilles Peskine --- include/mbedtls/config.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 9fac9452b..7243cae44 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1562,9 +1562,6 @@ * risk of vulnerabilities, and more gadgets that can make exploits easier. * Therefore this feature must never be enabled in production. * - * See `docs/architecture/testing/mbed-crypto-invasive-testing.md` for more - * information. - * * Uncomment to enable invasive tests. */ //#define MBEDTLS_TEST_HOOKS From e137ebce7fac270b2511391b45d09e92672d71c9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 21:12:52 +0100 Subject: [PATCH 04/24] Fix off-by-one error in #line directives The line number is the number of the next line. Signed-off-by: Gilles Peskine --- tests/suites/helpers.function | 2 +- tests/suites/main_test.function | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 7aa614ab8..d611e7217 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -1,4 +1,4 @@ -#line 1 "helpers.function" +#line 2 "helpers.function" /*----------------------------------------------------------------------------*/ /* Headers */ diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index 1c9804f71..8c4ea0127 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -1,4 +1,4 @@ -#line 1 "main_test.function" +#line 2 "main_test.function" SUITE_PRE_DEP #define TEST_SUITE_ACTIVE From 44498ff9eb9423c6c54f12f5246c409c47c9c094 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 21:17:11 +0100 Subject: [PATCH 05/24] Mutex usage testing: set up wrapper functions When using pthread mutexes (MBEDTLS_THREADING_C and MBEDTLS_THREADING_PTHREAD enabled), and when test hooks are enabled (MBEDTLS_TEST_HOOKS), set up wrappers around the mbedtls_mutex_xxx abstraction. In this commit, the wrapper functions don't do anything yet. Signed-off-by: Gilles Peskine --- tests/suites/helpers.function | 54 +++++++++++++++++++++++++++++++++ tests/suites/main_test.function | 4 +++ 2 files changed, 58 insertions(+) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index d611e7217..8c9e3bd09 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -38,6 +38,12 @@ typedef UINT32 uint32_t; #include #endif +#if defined(MBEDTLS_THREADING_C) && defined(MBEDTLS_THREADING_PTHREAD) && \ + defined(MBEDTLS_TEST_HOOKS) +#include "mbedtls/threading.h" +#define MBEDTLS_TEST_MUTEX_USAGE +#endif + /* * Define the two macros * @@ -519,3 +525,51 @@ int rnd_pseudo_rand( void *rng_state, unsigned char *output, size_t len ) return( 0 ); } +/** Mutex usage verification framework. + * + */ + +#if defined(MBEDTLS_TEST_MUTEX_USAGE) +typedef struct +{ + void (*init)( mbedtls_threading_mutex_t * ); + void (*free)( mbedtls_threading_mutex_t * ); + int (*lock)( mbedtls_threading_mutex_t * ); + int (*unlock)( mbedtls_threading_mutex_t * ); +} mutex_functions_t; +static mutex_functions_t mutex_functions; + +static void mbedtls_test_wrap_mutex_init( mbedtls_threading_mutex_t *mutex ) +{ + mutex_functions.init( mutex ); +} + +static void mbedtls_test_wrap_mutex_free( mbedtls_threading_mutex_t *mutex ) +{ + mutex_functions.free( mutex ); +} + +static int mbedtls_test_wrap_mutex_lock( mbedtls_threading_mutex_t *mutex ) +{ + int ret = mutex_functions.lock( mutex ); + return( ret ); +} + +static int mbedtls_test_wrap_mutex_unlock( mbedtls_threading_mutex_t *mutex ) +{ + return( mutex_functions.unlock( mutex ) ); +} + +static void mbedtls_test_mutex_usage_init( void ) +{ + mutex_functions.init = mbedtls_mutex_init; + mutex_functions.free = mbedtls_mutex_free; + mutex_functions.lock = mbedtls_mutex_lock; + mutex_functions.unlock = mbedtls_mutex_unlock; + mbedtls_mutex_init = &mbedtls_test_wrap_mutex_init; + mbedtls_mutex_free = &mbedtls_test_wrap_mutex_free; + mbedtls_mutex_lock = &mbedtls_test_wrap_mutex_lock; + mbedtls_mutex_unlock = &mbedtls_test_wrap_mutex_unlock; +} + +#endif /* MBEDTLS_TEST_MUTEX_USAGE */ diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index 8c4ea0127..63e91e861 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -285,6 +285,10 @@ int main(int argc, const char *argv[]) mbedtls_memory_buffer_alloc_init( alloc_buf, sizeof(alloc_buf) ); #endif +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + mbedtls_test_mutex_usage_init( ); +#endif + /* * The C standard doesn't guarantee that all-bits-0 is the representation * of a NULL pointer. We do however use that in our code for initializing From 0abb8e4bd84ea1141622609cc075789b8d7728f7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 21:18:09 +0100 Subject: [PATCH 06/24] Detect and report mutex usage errors If the mutex usage verification framework is enabled and it detects a mutex usage error, report this error and mark the test as failed. This detects most usage errors, but not all cases of using uninitialized memory (which is impossible in full generality) and not leaks due to missing free (which will be handled in a subsequent commit). Signed-off-by: Gilles Peskine --- tests/suites/helpers.function | 118 +++++++++++++++++++++++++++++++- tests/suites/main_test.function | 5 ++ 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 8c9e3bd09..fa3a5514d 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -193,6 +193,9 @@ static struct const char *test; const char *filename; int line_no; +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + const char *mutex_usage_error; +#endif } test_info; @@ -525,11 +528,49 @@ int rnd_pseudo_rand( void *rng_state, unsigned char *output, size_t len ) return( 0 ); } +#if defined(MBEDTLS_TEST_MUTEX_USAGE) /** Mutex usage verification framework. * + * The mutex usage verification code below aims to detect bad usage of + * Mbed TLS's mutex abstraction layer at runtime. Note that this is solely + * about the use of the mutex itself, not about checking whether the mutex + * correctly protects whatever it is supposed to protect. + * + * The normal usage of a mutex is: + * ``` + * digraph mutex_states { + * "UNINITIALIZED"; // the initial state + * "IDLE"; + * "FREED"; + * "LOCKED"; + * "UNINITIALIZED" -> "IDLE" [label="init"]; + * "FREED" -> "IDLE" [label="init"]; + * "IDLE" -> "LOCKED" [label="lock"]; + * "LOCKED" -> "IDLE" [label="unlock"]; + * "IDLE" -> "FREED" [label="free"]; + * } + * ``` + * + * All bad transitions that can be unambiguously detected are reported. + * An attempt to use an uninitialized mutex cannot be detected in general + * since the memory content may happen to denote a valid state. For the same + * reason, a double init cannot be detected. + * All-bits-zero is the state of a freed mutex, which is distinct from an + * initialized mutex, so attempting to use zero-initialized memory as a mutex + * without calling the init function is detected. + * + * If an error is detected, this framework will report what happened and the + * test case will be marked as failed. Unfortunately, the error report cannot + * indicate the exact location of the problematic call. To locate the error, + * use a debugger and set a breakpoint on mbedtls_test_mutex_usage_error(). */ +enum value_of_mutex_is_valid +{ + MUTEX_FREED = 0, //!< Set by threading_mutex_free_pthread + MUTEX_IDLE = 1, //!< Set by threading_mutex_init_pthread and by our unlock + MUTEX_LOCKED = 2, //!< Set by our lock +}; -#if defined(MBEDTLS_TEST_MUTEX_USAGE) typedef struct { void (*init)( mbedtls_threading_mutex_t * ); @@ -539,6 +580,19 @@ typedef struct } mutex_functions_t; static mutex_functions_t mutex_functions; +static void mbedtls_test_mutex_usage_error( mbedtls_threading_mutex_t *mutex, + const char *msg ) +{ + (void) mutex; + if( test_info.mutex_usage_error == NULL ) + test_info.mutex_usage_error = msg; + mbedtls_fprintf( stdout, "[mutex: %s] ", msg ); + /* Don't mark the test as failed yet. This way, if the test fails later + * for a functional reason, the test framework will report the message + * and location for this functional reason. If the test passes, + * mbedtls_test_mutex_usage_check() will mark it as failed. */ +} + static void mbedtls_test_wrap_mutex_init( mbedtls_threading_mutex_t *mutex ) { mutex_functions.init( mutex ); @@ -546,18 +600,67 @@ static void mbedtls_test_wrap_mutex_init( mbedtls_threading_mutex_t *mutex ) static void mbedtls_test_wrap_mutex_free( mbedtls_threading_mutex_t *mutex ) { + switch( mutex->is_valid ) + { + case MUTEX_FREED: + mbedtls_test_mutex_usage_error( mutex, "free without init or double free" ); + break; + case MUTEX_IDLE: + /* Do nothing. The underlying free function will reset is_valid + * to 0. */ + break; + case MUTEX_LOCKED: + mbedtls_test_mutex_usage_error( mutex, "free without unlock" ); + break; + default: + mbedtls_test_mutex_usage_error( mutex, "corrupted state" ); + break; + } mutex_functions.free( mutex ); } static int mbedtls_test_wrap_mutex_lock( mbedtls_threading_mutex_t *mutex ) { int ret = mutex_functions.lock( mutex ); + switch( mutex->is_valid ) + { + case MUTEX_FREED: + mbedtls_test_mutex_usage_error( mutex, "lock without init" ); + break; + case MUTEX_IDLE: + if( ret == 0 ) + mutex->is_valid = 2; + break; + case MUTEX_LOCKED: + mbedtls_test_mutex_usage_error( mutex, "double lock" ); + break; + default: + mbedtls_test_mutex_usage_error( mutex, "corrupted state" ); + break; + } return( ret ); } static int mbedtls_test_wrap_mutex_unlock( mbedtls_threading_mutex_t *mutex ) { - return( mutex_functions.unlock( mutex ) ); + int ret = mutex_functions.unlock( mutex ); + switch( mutex->is_valid ) + { + case MUTEX_FREED: + mbedtls_test_mutex_usage_error( mutex, "unlock without init" ); + break; + case MUTEX_IDLE: + mbedtls_test_mutex_usage_error( mutex, "unlock without lock" ); + break; + case MUTEX_LOCKED: + if( ret == 0 ) + mutex->is_valid = MUTEX_IDLE; + break; + default: + mbedtls_test_mutex_usage_error( mutex, "corrupted state" ); + break; + } + return( ret ); } static void mbedtls_test_mutex_usage_init( void ) @@ -572,4 +675,15 @@ static void mbedtls_test_mutex_usage_init( void ) mbedtls_mutex_unlock = &mbedtls_test_wrap_mutex_unlock; } +static void mbedtls_test_mutex_usage_check( void ) +{ + if( test_info.mutex_usage_error != NULL && ! test_info.failed ) + { + /* Functionally, the test passed. But there was a mutex usage error, + * so mark the test as failed after all. */ + test_fail( "Mutex usage error", __LINE__, __FILE__ ); + } + test_info.mutex_usage_error = NULL; +} + #endif /* MBEDTLS_TEST_MUTEX_USAGE */ diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index 63e91e861..4d4f32f69 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -111,6 +111,7 @@ DISPATCH_FUNCTION fflush( stdout ); ret = DISPATCH_TEST_FN_NOT_FOUND; } + #else ret = DISPATCH_UNSUPPORTED_SUITE; #endif @@ -457,6 +458,10 @@ int main(int argc, const char *argv[]) } #endif /* __unix__ || __APPLE__ __MACH__ */ +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + if( ret == DISPATCH_TEST_SUCCESS ) + mbedtls_test_mutex_usage_check( ); +#endif /* MBEDTLS_TEST_MUTEX_USAGE */ } if( unmet_dep_count > 0 || ret == DISPATCH_UNSUPPORTED_SUITE ) From df8db9ace2be98f8a09772d470180f9a4f96b2ac Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 22:20:32 +0100 Subject: [PATCH 07/24] Count and report non-freed mutexes Subtract the number of calls to mbedtls_mutex_free() from the number of calls to mbedtls_mutex_init(). A mutex leak will manifest as a positive result at the end of the test case. Signed-off-by: Gilles Peskine --- tests/suites/helpers.function | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index fa3a5514d..19f0af5a7 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -559,6 +559,17 @@ int rnd_pseudo_rand( void *rng_state, unsigned char *output, size_t len ) * initialized mutex, so attempting to use zero-initialized memory as a mutex * without calling the init function is detected. * + * The framework attempts to detect missing calls to init and free by counting + * calls to init and free. If there are more calls to init than free, this + * means that a mutex is not being freed somewhere, which is a memory leak + * on platforms where a mutex consumes resources other than the + * mbedtls_threading_mutex_t object itself. If there are more calls to free + * than init, this indicates a missing init, which is likely to be detected + * by an attempt to lock the mutex as well. A limitation of this framework is + * that it cannot detect scenarios where there is exactly the same number of + * calls to init and free but the calls don't match. A bug like this is + * unlikely happen uniformly throughout the whole test suite though. + * * If an error is detected, this framework will report what happened and the * test case will be marked as failed. Unfortunately, the error report cannot * indicate the exact location of the problematic call. To locate the error, @@ -580,6 +591,13 @@ typedef struct } mutex_functions_t; static mutex_functions_t mutex_functions; +/** The total number of calls to mbedtls_mutex_init(), minus the total number + * of calls to mbedtls_mutex_free(). + * + * Reset to 0 after each test case. + */ +static int live_mutexes; + static void mbedtls_test_mutex_usage_error( mbedtls_threading_mutex_t *mutex, const char *msg ) { @@ -596,6 +614,8 @@ static void mbedtls_test_mutex_usage_error( mbedtls_threading_mutex_t *mutex, static void mbedtls_test_wrap_mutex_init( mbedtls_threading_mutex_t *mutex ) { mutex_functions.init( mutex ); + if( mutex->is_valid ) + ++live_mutexes; } static void mbedtls_test_wrap_mutex_free( mbedtls_threading_mutex_t *mutex ) @@ -616,6 +636,8 @@ static void mbedtls_test_wrap_mutex_free( mbedtls_threading_mutex_t *mutex ) mbedtls_test_mutex_usage_error( mutex, "corrupted state" ); break; } + if( mutex->is_valid ) + --live_mutexes; mutex_functions.free( mutex ); } @@ -677,6 +699,17 @@ static void mbedtls_test_mutex_usage_init( void ) static void mbedtls_test_mutex_usage_check( void ) { + if( live_mutexes != 0 ) + { + /* A positive number (more init than free) means that a mutex resource + * is leaking (on platforms where a mutex consumes more than the + * mbedtls_threading_mutex_t object itself). The rare case of a + * negative number means a missing init somewhere. */ + mbedtls_fprintf( stdout, "[mutex: %d leaked] ", live_mutexes ); + live_mutexes = 0; + if( test_info.mutex_usage_error == NULL ) + test_info.mutex_usage_error = "missing free"; + } if( test_info.mutex_usage_error != NULL && ! test_info.failed ) { /* Functionally, the test passed. But there was a mutex usage error, From 57107321452eeb829847938db364a27744be0909 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 15:35:29 +0100 Subject: [PATCH 08/24] Explain the usage of is_valid in pthread mutexes Document the usage inside the library, and relate it with how it's additionally used in the test code. Signed-off-by: Gilles Peskine --- include/mbedtls/threading.h | 3 +++ library/threading.c | 6 ++++++ tests/suites/helpers.function | 7 ++++++- 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index a5d0a34a6..f1d53bdb1 100644 --- a/include/mbedtls/threading.h +++ b/include/mbedtls/threading.h @@ -70,6 +70,9 @@ extern "C" { typedef struct { pthread_mutex_t mutex; + /* is_valid is 0 after a failed init or a free, and nonzero after a + * successful init. This field is not considered part of the public + * API of Mbed TLS and may change without notice. */ char is_valid; } mbedtls_threading_mutex_t; #endif diff --git a/library/threading.c b/library/threading.c index 838cd74d9..df16fe72f 100644 --- a/library/threading.c +++ b/library/threading.c @@ -60,6 +60,12 @@ static void threading_mutex_init_pthread( mbedtls_threading_mutex_t *mutex ) if( mutex == NULL ) return; + /* A nonzero value of is_valid indicates a successfully initialized + * mutex. This is a workaround for not being able to return an error + * code for this function. The lock/unlock functions return an error + * if is_valid is nonzero. The Mbed TLS unit test code uses this field + * to distinguish more states of the mutex; see helpers.function for + * details. */ mutex->is_valid = pthread_mutex_init( &mutex->mutex, NULL ) == 0; } diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 19f0af5a7..65bb2ffce 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -575,8 +575,13 @@ int rnd_pseudo_rand( void *rng_state, unsigned char *output, size_t len ) * indicate the exact location of the problematic call. To locate the error, * use a debugger and set a breakpoint on mbedtls_test_mutex_usage_error(). */ -enum value_of_mutex_is_valid +enum value_of_mutex_is_valid_field { + /* Potential values for the is_valid field of mbedtls_threading_mutex_t. + * Note that MUTEX_FREED must be 0 and MUTEX_IDLE must be 1 for + * compatibility with threading_mutex_init_pthread() and + * threading_mutex_free_pthread(). MUTEX_LOCKED could be any nonzero + * value. */ MUTEX_FREED = 0, //!< Set by threading_mutex_free_pthread MUTEX_IDLE = 1, //!< Set by threading_mutex_init_pthread and by our unlock MUTEX_LOCKED = 2, //!< Set by our lock From 085b69f8fd2493d665fcc32d874cb63516cf878d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 30 Jan 2021 13:05:32 +0100 Subject: [PATCH 09/24] Fix mutex leak in CTR_DRBG mbedtls_ctr_drbg_free() left a mutex in the initialized state. This caused a resource leak on platforms where mbedtls_mutex_init() allocates resources. To fix this, mbedtls_ctr_drbg_free() no longer reinitializes the mutex. To preserve the property that mbedtls_ctr_drbg_free() leaves the object in an initialized state, which is generally true throughout the library except regarding mutex objects on some platforms, no longer initialize the mutex in mbedtls_ctr_drbg_init(). Since the mutex is only used after seeding, and seeding is only permitted once, call mbedtls_mutex_init() as part of the seeding process. Signed-off-by: Gilles Peskine --- library/ctr_drbg.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index bc5cc8f63..64b536aaa 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -87,10 +87,6 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ) memset( ctx, 0, sizeof( mbedtls_ctr_drbg_context ) ); ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; - -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_init( &ctx->mutex ); -#endif } /* @@ -103,14 +99,12 @@ void mbedtls_ctr_drbg_free( mbedtls_ctr_drbg_context *ctx ) return; #if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_free( &ctx->mutex ); + if( ctx->f_entropy != NULL ) + mbedtls_mutex_free( &ctx->mutex ); #endif mbedtls_aes_free( &ctx->aes_ctx ); mbedtls_zeroize( ctx, sizeof( mbedtls_ctr_drbg_context ) ); ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_init( &ctx->mutex ); -#endif } void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, int resistance ) @@ -382,6 +376,10 @@ int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, memset( key, 0, MBEDTLS_CTR_DRBG_KEYSIZE ); +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_init( &ctx->mutex ); +#endif + mbedtls_aes_init( &ctx->aes_ctx ); ctx->f_entropy = f_entropy; From 6e2cf25639285b6b913df6314ea89f2695876acc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:44:02 +0100 Subject: [PATCH 10/24] Document mutex invariant for CTR_DRBG Signed-off-by: Gilles Peskine --- include/mbedtls/ctr_drbg.h | 7 +++++++ library/ctr_drbg.c | 2 ++ 2 files changed, 9 insertions(+) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 24d9870be..e1f2c7172 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -190,6 +190,13 @@ typedef struct void *p_entropy; /*!< The context for the entropy function. */ #if defined(MBEDTLS_THREADING_C) + /* Invariant: the mutex is initialized if and only if f_entropy != NULL. + * This means that the mutex is initialized during the initial seeding + * in mbedtls_ctr_drbg_seed() and freed in mbedtls_ctr_drbg_free(). + * + * Note that this invariant may change without notice. Do not rely on it + * and do not access the mutex directly in application code. + */ mbedtls_threading_mutex_t mutex; #endif } diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 64b536aaa..e275f1a72 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -99,6 +99,7 @@ void mbedtls_ctr_drbg_free( mbedtls_ctr_drbg_context *ctx ) return; #if defined(MBEDTLS_THREADING_C) + /* The mutex is initialized iff f_entropy is set. */ if( ctx->f_entropy != NULL ) mbedtls_mutex_free( &ctx->mutex ); #endif @@ -376,6 +377,7 @@ int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, memset( key, 0, MBEDTLS_CTR_DRBG_KEYSIZE ); + /* The mutex is initialized iff f_entropy is set. */ #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_init( &ctx->mutex ); #endif From 275598d3fa00a3adfd4408a1a660520b7e8c032e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:44:18 +0100 Subject: [PATCH 11/24] Document thread safety for CTR_DRBG random(), and only this function, is thread-safe. Signed-off-by: Gilles Peskine --- include/mbedtls/ctr_drbg.h | 43 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index e1f2c7172..33fe63f48 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -259,6 +259,15 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * device. */ #endif +#if defined(MBEDTLS_THREADING_C) +/** + * \note When Mbed TLS is built with threading support, + * after this function returns successfully, + * it is safe to call mbedtls_ctr_drbg_random() + * from multiple threads. Other operations, including + * reseeding, are not thread-safe. + */ +#endif /* MBEDTLS_THREADING_C */ /** * \param ctx The CTR_DRBG context to seed. * It must have been initialized with @@ -268,6 +277,8 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * the same context unless you call * mbedtls_ctr_drbg_free() and mbedtls_ctr_drbg_init() * again first. + * After a failed call to mbedtls_ctr_drbg_seed(), + * you must call mbedtls_ctr_drbg_free(). * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the * length of the buffer. @@ -351,6 +362,11 @@ void mbedtls_ctr_drbg_set_reseed_interval( mbedtls_ctr_drbg_context *ctx, * \brief This function reseeds the CTR_DRBG context, that is * extracts data from the entropy source. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param ctx The CTR_DRBG context. * \param additional Additional data to add to the state. Can be \c NULL. * \param len The length of the additional data. @@ -368,6 +384,11 @@ int mbedtls_ctr_drbg_reseed( mbedtls_ctr_drbg_context *ctx, /** * \brief This function updates the state of the CTR_DRBG context. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param ctx The CTR_DRBG context. * \param additional The data to update the state with. This must not be * \c NULL unless \p add_len is \c 0. @@ -395,6 +416,11 @@ int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT Bytes are used. * The remaining Bytes are silently discarded. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param ctx The CTR_DRBG context. * \param additional The data to update the state with. This must not be * \c NULL unless \p add_len is \c 0. @@ -412,6 +438,11 @@ void mbedtls_ctr_drbg_update( mbedtls_ctr_drbg_context *ctx, * This function automatically reseeds if the reseed counter is exceeded * or prediction resistance is enabled. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param p_rng The CTR_DRBG context. This must be a pointer to a * #mbedtls_ctr_drbg_context structure. * \param output The buffer to fill. @@ -440,8 +471,16 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, * * This function automatically reseeds if the reseed counter is exceeded * or prediction resistance is enabled. - * - * + */ +#if defined(MBEDTLS_THREADING_C) +/** + * \note When Mbed TLS is built with threading support, + * it is safe to call mbedtls_ctr_drbg_random() + * from multiple threads. Other operations, including + * reseeding, are not thread-safe. + */ +#endif /* MBEDTLS_THREADING_C */ +/** * \param p_rng The CTR_DRBG context. This must be a pointer to a * #mbedtls_ctr_drbg_context structure. * \param output The buffer to fill. From a9857af16ab974b121689623dfbab9a6ab4d3608 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 31 Jan 2021 00:06:51 +0100 Subject: [PATCH 12/24] Fix mutex leak in HMAC_DRBG mbedtls_hmac_drbg_free() left a mutex in the initialized state. This caused a resource leak on platforms where mbedtls_mutex_init() allocates resources. To fix this, mbedtls_hmac_drbg_free() no longer reinitializes the mutex. To preserve the property that mbedtls_hmac_drbg_free() leaves the object in an initialized state, which is generally true throughout the library except regarding mutex objects on some platforms, no longer initialize the mutex in mbedtls_hmac_drbg_init(). Since the mutex is only used after seeding, and seeding is only permitted once, call mbedtls_mutex_init() as part of the seeding process. Signed-off-by: Gilles Peskine --- library/hmac_drbg.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 26b15e952..8bd8a05d5 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -88,10 +88,6 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ) memset( ctx, 0, sizeof( mbedtls_hmac_drbg_context ) ); ctx->reseed_interval = MBEDTLS_HMAC_DRBG_RESEED_INTERVAL; - -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_init( &ctx->mutex ); -#endif } /* @@ -161,6 +157,10 @@ int mbedtls_hmac_drbg_seed_buf( mbedtls_hmac_drbg_context *ctx, if( ( ret = mbedtls_md_setup( &ctx->md_ctx, md_info, 1 ) ) != 0 ) return( ret ); +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_init( &ctx->mutex ); +#endif + /* * Set initial working state. * Use the V memory location, which is currently all 0, to initialize the @@ -286,6 +286,10 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, if( ( ret = mbedtls_md_setup( &ctx->md_ctx, md_info, 1 ) ) != 0 ) return( ret ); +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_init( &ctx->mutex ); +#endif + md_size = mbedtls_md_get_size( md_info ); /* @@ -453,14 +457,12 @@ void mbedtls_hmac_drbg_free( mbedtls_hmac_drbg_context *ctx ) return; #if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_free( &ctx->mutex ); + if( ctx->md_ctx.md_info != NULL ) + mbedtls_mutex_free( &ctx->mutex ); #endif mbedtls_md_free( &ctx->md_ctx ); mbedtls_zeroize( ctx, sizeof( mbedtls_hmac_drbg_context ) ); ctx->reseed_interval = MBEDTLS_HMAC_DRBG_RESEED_INTERVAL; -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_init( &ctx->mutex ); -#endif } #if defined(MBEDTLS_FS_IO) From 3ec7f57482d8336109960e92b334f2ad523106da Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:43:33 +0100 Subject: [PATCH 13/24] Document mutex invariant for HMAC_DRBG Signed-off-by: Gilles Peskine --- include/mbedtls/hmac_drbg.h | 8 ++++++++ library/hmac_drbg.c | 2 ++ 2 files changed, 10 insertions(+) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index cd23a16af..c9818a723 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -128,6 +128,14 @@ typedef struct void *p_entropy; /*!< context for the entropy function */ #if defined(MBEDTLS_THREADING_C) + /* Invariant: the mutex is initialized if and only if + * md_ctx->md_info != NULL. This means that the mutex is initialized + * during the initial seeding in mbedtls_hmac_drbg_seed() or + * mbedtls_hmac_drbg_seed_buf() and freed in mbedtls_ctr_drbg_free(). + * + * Note that this invariant may change without notice. Do not rely on it + * and do not access the mutex directly in application code. + */ mbedtls_threading_mutex_t mutex; #endif } mbedtls_hmac_drbg_context; diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 8bd8a05d5..876fa574a 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -286,6 +286,7 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, if( ( ret = mbedtls_md_setup( &ctx->md_ctx, md_info, 1 ) ) != 0 ) return( ret ); + /* The mutex is initialized iff the md context is set up. */ #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_init( &ctx->mutex ); #endif @@ -457,6 +458,7 @@ void mbedtls_hmac_drbg_free( mbedtls_hmac_drbg_context *ctx ) return; #if defined(MBEDTLS_THREADING_C) + /* The mutex is initialized iff the md context is set up. */ if( ctx->md_ctx.md_info != NULL ) mbedtls_mutex_free( &ctx->mutex ); #endif From 94bf03330b567382507de9f484db729d57d0ecb8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:45:10 +0100 Subject: [PATCH 14/24] Document thread safety for HMAC_DRBG random(), and only this function, is thread-safe. Signed-off-by: Gilles Peskine --- include/mbedtls/hmac_drbg.h | 55 +++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index c9818a723..4a797d472 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -185,7 +185,17 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * \note During the initial seeding, this function calls * the entropy source to obtain a nonce * whose length is half the entropy length. - * + */ +#if defined(MBEDTLS_THREADING_C) +/** + * \note When Mbed TLS is built with threading support, + * after this function returns successfully, + * it is safe to call mbedtls_hmac_drbg_random() + * from multiple threads. Other operations, including + * reseeding, are not thread-safe. + */ +#endif /* MBEDTLS_THREADING_C */ +/** * \param ctx HMAC_DRBG context to be seeded. * \param md_info MD algorithm to use for HMAC_DRBG. * \param f_entropy The entropy callback, taking as arguments the @@ -224,7 +234,17 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, * * This function is meant for use in algorithms that need a pseudorandom * input such as deterministic ECDSA. - * + */ +#if defined(MBEDTLS_THREADING_C) +/** + * \note When Mbed TLS is built with threading support, + * after this function returns successfully, + * it is safe to call mbedtls_hmac_drbg_random() + * from multiple threads. Other operations, including + * reseeding, are not thread-safe. + */ +#endif /* MBEDTLS_THREADING_C */ +/** * \param ctx HMAC_DRBG context to be initialised. * \param md_info MD algorithm to use for HMAC_DRBG. * \param data Concatenation of the initial entropy string and @@ -287,6 +307,11 @@ void mbedtls_hmac_drbg_set_reseed_interval( mbedtls_hmac_drbg_context *ctx, /** * \brief This function updates the state of the HMAC_DRBG context. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param ctx The HMAC_DRBG context. * \param additional The data to update the state with. * If this is \c NULL, there is no additional data. @@ -305,6 +330,11 @@ int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, * \warning This function cannot report errors. You should use * mbedtls_hmac_drbg_update_ret() instead. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param ctx HMAC_DRBG context * \param additional Additional data to update state with, or NULL * \param add_len Length of additional data, or 0 @@ -320,6 +350,11 @@ void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, * \brief This function reseeds the HMAC_DRBG context, that is * extracts data from the entropy source. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param ctx The HMAC_DRBG context. * \param additional Additional data to add to the state. * If this is \c NULL, there is no additional data @@ -345,6 +380,11 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, * This function automatically reseeds if the reseed counter is exceeded * or prediction resistance is enabled. * + * \note This function is not thread-safe. It is not safe + * to call this function if another thread might be + * concurrently obtaining random numbers from the same + * context or updating or reseeding the same context. + * * \param p_rng The HMAC_DRBG context. This must be a pointer to a * #mbedtls_hmac_drbg_context structure. * \param output The buffer to fill. @@ -374,7 +414,16 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, * * This function automatically reseeds if the reseed counter is exceeded * or prediction resistance is enabled. - * + */ +#if defined(MBEDTLS_THREADING_C) +/** + * \note When Mbed TLS is built with threading support, + * it is safe to call mbedtls_ctr_drbg_random() + * from multiple threads. Other operations, including + * reseeding, are not thread-safe. + */ +#endif /* MBEDTLS_THREADING_C */ +/** * \param p_rng The HMAC_DRBG context. This must be a pointer to a * #mbedtls_hmac_drbg_context structure. * \param output The buffer to fill. From e525bc830f5ab0d4a3fa1fe0021f3ee0be7f3f47 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:50:03 +0100 Subject: [PATCH 15/24] Changelog entry for DRBG mutex usage fix Signed-off-by: Gilles Peskine --- ChangeLog.d/drbg-mutex.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/drbg-mutex.txt diff --git a/ChangeLog.d/drbg-mutex.txt b/ChangeLog.d/drbg-mutex.txt new file mode 100644 index 000000000..3ac5abfa8 --- /dev/null +++ b/ChangeLog.d/drbg-mutex.txt @@ -0,0 +1,5 @@ +Bugfix + * Fix a resource leak in CTR_DRBG and HMAC_DRBG when MBEDTLS_THREADING_C + is enabled, on platforms where initializing a mutex allocates resources. + This was a regression introduced in the previous release. Reported in + #4017, #4045 and #4071. From ff754e67aedfbc8a8a68731c2802e93172090849 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 31 Jan 2021 00:07:11 +0100 Subject: [PATCH 16/24] Add missing cleanup in a test function Signed-off-by: Gilles Peskine --- tests/suites/test_suite_entropy.function | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suites/test_suite_entropy.function b/tests/suites/test_suite_entropy.function index cb5e20462..76aaad71d 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -190,6 +190,9 @@ void entropy_func_len( int len, int ret ) for( j = len; j < sizeof( buf ); j++ ) TEST_ASSERT( acc[j] == 0 ); + +exit: + mbedtls_entropy_free( &ctx ); } /* END_CASE */ From 54e7e2bdc79cefce0bc06127584b2092c0c34d64 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Feb 2021 17:55:24 +0100 Subject: [PATCH 17/24] Add init-free tests for RSA These tests are trivial except when compiling with MBEDTLS_THREADING_C and a mutex implementation that are picky about matching each mbedtls_mutex_init() with exactly one mbedtls_mutex_free(). Signed-off-by: Gilles Peskine --- tests/suites/test_suite_rsa.data | 6 ++++++ tests/suites/test_suite_rsa.function | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/tests/suites/test_suite_rsa.data b/tests/suites/test_suite_rsa.data index d6ba66d84..f15006fde 100644 --- a/tests/suites/test_suite_rsa.data +++ b/tests/suites/test_suite_rsa.data @@ -1,3 +1,9 @@ +RSA init-free-free +rsa_init_free:0 + +RSA init-free-init-free +rsa_init_free:1 + RSA PKCS1 Verify v1.5 CAVS #1 depends_on:MBEDTLS_SHA1_C:MBEDTLS_PKCS1_V15 # Good padding but wrong hash diff --git a/tests/suites/test_suite_rsa.function b/tests/suites/test_suite_rsa.function index e895302c9..b421c6713 100644 --- a/tests/suites/test_suite_rsa.function +++ b/tests/suites/test_suite_rsa.function @@ -17,6 +17,29 @@ * END_DEPENDENCIES */ +/* BEGIN_CASE */ +void rsa_init_free( int reinit ) +{ + mbedtls_rsa_context ctx; + + /* Double free is not explicitly documented to work, but we rely on it + * even inside the library so that you can call mbedtls_rsa_free() + * unconditionally on an error path without checking whether it has + * already been called in the success path. */ + + mbedtls_rsa_init( &ctx, 0, 0 ); + mbedtls_rsa_free( &ctx ); + + if( reinit ) + mbedtls_rsa_init( &ctx, 0, 0 ); + mbedtls_rsa_free( &ctx ); + + /* This test case always succeeds, functionally speaking. A plausible + * bug might trigger an invalid pointer dereference or a memory leak. */ + goto exit; +} +/* END_CASE */ + /* BEGIN_CASE */ void mbedtls_rsa_pkcs1_sign( char *message_hex_string, int padding_mode, int digest, int mod, int radix_P, char *input_P, int radix_Q, From d7e82ad9bf96f2bca5f9468e6f0dd2cc60071823 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Feb 2021 17:57:41 +0100 Subject: [PATCH 18/24] Fix mutex double-free in RSA When MBEDTLS_THREADING_C is enabled, RSA code protects the use of the key with a mutex. mbedtls_rsa_free() frees this mutex by calling mbedtls_mutex_free(). This does not match the usage of mbedtls_mutex_free(), which in general can only be done once. Signed-off-by: Gilles Peskine --- library/rsa.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 7e853b152..65b75d60f 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -499,6 +499,9 @@ void mbedtls_rsa_init( mbedtls_rsa_context *ctx, mbedtls_rsa_set_padding( ctx, padding, hash_id ); #if defined(MBEDTLS_THREADING_C) + /* Set ctx->ver to nonzero to indicate that the mutex has been + * initialized and will need to be freed. */ + ctx->ver = 1; mbedtls_mutex_init( &ctx->mutex ); #endif } @@ -2325,7 +2328,6 @@ int mbedtls_rsa_copy( mbedtls_rsa_context *dst, const mbedtls_rsa_context *src ) { int ret; - dst->ver = src->ver; dst->len = src->len; MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &dst->N, &src->N ) ); @@ -2375,7 +2377,12 @@ void mbedtls_rsa_free( mbedtls_rsa_context *ctx ) #endif /* MBEDTLS_RSA_NO_CRT */ #if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_free( &ctx->mutex ); + /* Free the mutex, but only if it hasn't been freed already. */ + if( ctx->ver != 0 ) + { + mbedtls_mutex_free( &ctx->mutex ); + ctx->ver = 0; + } #endif } From 22dc2e7a9b5dde165b1af00f7a118cc85c00484d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Feb 2021 21:06:10 +0100 Subject: [PATCH 19/24] Fix mutex leak in RSA mbedtls_rsa_gen_key() was not freeing the RSA object, and specifically not freeing the mutex, in some error cases. Signed-off-by: Gilles Peskine --- library/rsa.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 65b75d60f..0fa19e5b7 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -538,15 +538,15 @@ int mbedtls_rsa_gen_key( mbedtls_rsa_context *ctx, int ret; mbedtls_mpi H, G; - if( f_rng == NULL || nbits < 128 || exponent < 3 ) - return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); - - if( nbits % 2 ) - return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); - mbedtls_mpi_init( &H ); mbedtls_mpi_init( &G ); + if( f_rng == NULL || nbits < 128 || exponent < 3 || nbits % 2 != 0 ) + { + ret = MBEDTLS_ERR_RSA_BAD_INPUT_DATA; + goto cleanup; + } + /* * find primes P and Q with Q < P so that: * GCD( E, (P-1)*(Q-1) ) == 1 @@ -610,7 +610,9 @@ cleanup: if( ret != 0 ) { mbedtls_rsa_free( ctx ); - return( MBEDTLS_ERR_RSA_KEY_GEN_FAILED + ret ); + if( ( -ret & ~0x7f ) == 0 ) + ret = MBEDTLS_ERR_RSA_KEY_GEN_FAILED + ret; + return( ret ); } return( 0 ); From 3be78f318ca95104bb48c28dac70ab0dff24fda7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:52:49 +0100 Subject: [PATCH 20/24] Changelog entry for RSA mutex usage fix Signed-off-by: Gilles Peskine --- ChangeLog.d/rsa-mutex.txt | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 ChangeLog.d/rsa-mutex.txt diff --git a/ChangeLog.d/rsa-mutex.txt b/ChangeLog.d/rsa-mutex.txt new file mode 100644 index 000000000..bafb7b2d5 --- /dev/null +++ b/ChangeLog.d/rsa-mutex.txt @@ -0,0 +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. + * 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. From e8505e37df2e9a0054747da107a8dcfeac81307e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:59:42 +0100 Subject: [PATCH 21/24] Document mutex usage for RSA The mutex is now initialized iff ver != 0. Signed-off-by: Gilles Peskine --- ChangeLog.d/rsa-mutex.txt | 5 +++++ include/mbedtls/rsa.h | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/ChangeLog.d/rsa-mutex.txt b/ChangeLog.d/rsa-mutex.txt index bafb7b2d5..49f1a84f2 100644 --- a/ChangeLog.d/rsa-mutex.txt +++ b/ChangeLog.d/rsa-mutex.txt @@ -6,3 +6,8 @@ Bugfix * 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. + +Default behavior changes + * In mbedtls_rsa_context objects, the ver field was formerly documented + as always 0. It is now reserved for internal purposes and may take + different values. diff --git a/include/mbedtls/rsa.h b/include/mbedtls/rsa.h index 7b508c5dc..cfb66dd8e 100644 --- a/include/mbedtls/rsa.h +++ b/include/mbedtls/rsa.h @@ -118,7 +118,10 @@ extern "C" { */ typedef struct { - int ver; /*!< Always 0.*/ + int ver; /*!< Reserved for internal purposes. + * Do not set this field in application + * code. Its meaning might change without + * notice. */ size_t len; /*!< The size of \p N in Bytes. */ mbedtls_mpi N; /*!< The public modulus. */ @@ -148,6 +151,7 @@ typedef struct mask generating function used in the EME-OAEP and EMSA-PSS encodings. */ #if defined(MBEDTLS_THREADING_C) + /* Invariant: the mutex is initialized iff ver != 0. */ mbedtls_threading_mutex_t mutex; /*!< Thread-safety mutex. */ #endif } From 58a39e02da2f61b46014804d71fd432ad06b1b86 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 15 Feb 2021 18:21:55 +0100 Subject: [PATCH 22/24] Fix typo in documentation Signed-off-by: Gilles Peskine --- tests/suites/helpers.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 65bb2ffce..5542462f9 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -568,7 +568,7 @@ int rnd_pseudo_rand( void *rng_state, unsigned char *output, size_t len ) * by an attempt to lock the mutex as well. A limitation of this framework is * that it cannot detect scenarios where there is exactly the same number of * calls to init and free but the calls don't match. A bug like this is - * unlikely happen uniformly throughout the whole test suite though. + * unlikely to happen uniformly throughout the whole test suite though. * * If an error is detected, this framework will report what happened and the * test case will be marked as failed. Unfortunately, the error report cannot From 9f97f9522555f581e48c3dd8fdf4f04696fddefb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 22 Feb 2021 21:24:02 +0100 Subject: [PATCH 23/24] Add init-free tests for entropy These tests validate that an entropy object can be reused and that calling mbedtls_entropy_free() twice is ok. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_entropy.data | 6 ++++++ tests/suites/test_suite_entropy.function | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/tests/suites/test_suite_entropy.data b/tests/suites/test_suite_entropy.data index 5cff39984..8ad8760e2 100644 --- a/tests/suites/test_suite_entropy.data +++ b/tests/suites/test_suite_entropy.data @@ -1,3 +1,9 @@ +Entropy init-free-free +entropy_init_free:0 + +Entropy init-free-init-free +entropy_init_free:1 + Create NV seed_file nv_seed_file_create: diff --git a/tests/suites/test_suite_entropy.function b/tests/suites/test_suite_entropy.function index 76aaad71d..df9ca2efa 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -124,6 +124,28 @@ int read_nv_seed( unsigned char *buf, size_t buf_len ) * END_DEPENDENCIES */ +/* BEGIN_CASE */ +void entropy_init_free( int reinit ) +{ + mbedtls_entropy_context ctx; + + /* Double free is not explicitly documented to work, but it is convenient + * to call mbedtls_entropy_free() unconditionally on an error path without + * checking whether it has already been called in the success path. */ + + mbedtls_entropy_init( &ctx ); + mbedtls_entropy_free( &ctx ); + + if( reinit ) + mbedtls_entropy_init( &ctx ); + mbedtls_entropy_free( &ctx ); + + /* This test case always succeeds, functionally speaking. A plausible + * bug might trigger an invalid pointer dereference or a memory leak. */ + goto exit; +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_ENTROPY_NV_SEED:MBEDTLS_FS_IO */ void entropy_seed_file( char *path, int ret ) { From 2de4691bb02cd88c71b6038827b557f06e7f8fb0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 22 Feb 2021 21:26:54 +0100 Subject: [PATCH 24/24] 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 cee6ab5a7..41648aaf2 100644 --- a/include/mbedtls/entropy.h +++ b/include/mbedtls/entropy.h @@ -147,13 +147,15 @@ mbedtls_entropy_source_state; */ typedef struct { - 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 ee98e4cdb..fb5e0eca6 100644 --- a/library/entropy.c +++ b/library/entropy.c @@ -150,6 +150,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 @@ -166,7 +171,7 @@ void mbedtls_entropy_free( mbedtls_entropy_context *ctx ) #endif ctx->source_count = 0; mbedtls_zeroize( ctx->source, sizeof( ctx->source ) ); - ctx->accumulator_started = 0; + ctx->accumulator_started = -1; } int mbedtls_entropy_add_source( mbedtls_entropy_context *ctx,