From 7f652adc48b58599f60d32c99098fc10ff4bdd25 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 9ce1377b2..1e56c3e62 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1324,7 +1324,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 44e89c547f75671a6c9fcf4356eb2e6919c4f739 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 6b45021d0..b1541d673 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1746,6 +1746,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 cbf38dc2c..8c8e815e9 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -553,6 +553,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 96a7064754dd83a04df238a50c800787112d1ab2 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 b1541d673..fff4240c0 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1759,9 +1759,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 c07137384218ec858f7699d039ec0ae9832ced71 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 21:17:11 +0100 Subject: [PATCH 04/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 Signed-off-by: Chris Jones --- tests/suites/helpers.function | 55 +++++++++++++++++++++++++++++++++ tests/suites/host_test.function | 4 +++ 2 files changed, 59 insertions(+) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 9403d9905..fa98d802b 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -46,6 +46,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 * @@ -777,3 +783,52 @@ int mbedtls_test_hexcmp( uint8_t * a, uint8_t * b, uint32_t a_len, uint32_t b_le } return ret; } + +/** 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/host_test.function b/tests/suites/host_test.function index 1178135c5..3f8a945ea 100644 --- a/tests/suites/host_test.function +++ b/tests/suites/host_test.function @@ -412,6 +412,10 @@ int execute_tests( 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 cd2e248fdd84f4ec5884676f591cb75b6b18a675 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 21:18:09 +0100 Subject: [PATCH 05/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 Signed-off-by: Chris Jones --- tests/suites/helpers.function | 119 +++++++++++++++++++++++++++++++- tests/suites/main_test.function | 4 ++ 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index fa98d802b..18a96d18a 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -377,6 +377,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; @@ -784,11 +787,49 @@ int mbedtls_test_hexcmp( uint8_t * a, uint8_t * b, uint32_t a_len, uint32_t b_le return ret; } +#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 * ); @@ -798,6 +839,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 ); @@ -805,18 +859,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 ) @@ -831,4 +934,16 @@ 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.result != TEST_RESULT_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 9f366907b..2aecf8142 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -176,6 +176,10 @@ void execute_function_ptr(TestWrapper_t fp, void **params) #else fp( params ); #endif + +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + mbedtls_test_mutex_usage_check( ); +#endif /* MBEDTLS_TEST_MUTEX_USAGE */ } /** From 7252ec394745361ea621a50c663229d25fd99a5c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 22:20:32 +0100 Subject: [PATCH 06/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 Signed-off-by: Chris Jones --- tests/suites/helpers.function | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 18a96d18a..1d48b2023 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -818,6 +818,17 @@ int mbedtls_test_hexcmp( uint8_t * a, uint8_t * b, uint32_t a_len, uint32_t b_le * 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, @@ -839,6 +850,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 ) { @@ -855,6 +873,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 ) @@ -875,6 +895,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 ); } @@ -936,6 +958,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.result != TEST_RESULT_FAILED ) { From 7ba73e575618c8bb8f03b544b322768335db9de7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 15:35:29 +0100 Subject: [PATCH 07/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 a8183a6ef..45161ce46 100644 --- a/include/mbedtls/threading.h +++ b/include/mbedtls/threading.h @@ -73,6 +73,9 @@ extern "C" { typedef struct mbedtls_threading_mutex_t { 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 f4f29cff5..0dc5488c1 100644 --- a/library/threading.c +++ b/library/threading.c @@ -98,6 +98,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 1d48b2023..0bf81895a 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -834,8 +834,13 @@ int mbedtls_test_hexcmp( uint8_t * a, uint8_t * b, uint32_t a_len, uint32_t b_le * 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 89816bc020b91cb6dd5c6df192ea16ebd8975903 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 30 Jan 2021 13:05:32 +0100 Subject: [PATCH 08/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 e92008bbe..5461b7121 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -83,10 +83,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 } /* @@ -99,14 +95,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_platform_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 ) @@ -422,6 +416,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 2ecc0b89f3cfdeaee7416c1df58c585c7788170a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:44:02 +0100 Subject: [PATCH 09/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 278fbbbb7..d8ef5d095 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -214,6 +214,13 @@ typedef struct mbedtls_ctr_drbg_context 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 5461b7121..90264e844 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -95,6 +95,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 @@ -416,6 +417,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 831956980c66b03ab00e2bdc0fbcc55f98bf5f02 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:44:18 +0100 Subject: [PATCH 10/24] Document thread safety for CTR_DRBG random(), and only this function, is thread-safe. Signed-off-by: Gilles Peskine Signed-off-by: Chris Jones --- include/mbedtls/ctr_drbg.h | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index d8ef5d095..6c099adf4 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -284,6 +284,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 @@ -293,6 +302,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. @@ -384,6 +395,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. @@ -401,6 +417,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. @@ -424,6 +445,11 @@ int mbedtls_ctr_drbg_update_ret( 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. @@ -452,8 +478,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 05974893e6fab850ea87716b52f1d571803ced57 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 31 Jan 2021 00:06:51 +0100 Subject: [PATCH 11/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 10cbd462b..330702101 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -84,10 +84,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 } /* @@ -159,6 +155,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 @@ -284,6 +284,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 ); /* @@ -451,14 +455,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_platform_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 b5e295d5c967327d62c6dfd4ebe24cda17267f97 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:43:33 +0100 Subject: [PATCH 12/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 970c033c1..edb3f876a 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -128,6 +128,14 @@ typedef struct mbedtls_hmac_drbg_context 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 330702101..b45d61616 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -284,6 +284,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 @@ -455,6 +456,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 fb6876a111bd67ae9d5e9ce1ca436395caab64bc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:45:10 +0100 Subject: [PATCH 13/24] Document thread safety for HMAC_DRBG random(), and only this function, is thread-safe. Signed-off-by: Gilles Peskine Signed-off-by: Chris Jones --- include/mbedtls/hmac_drbg.h | 50 ++++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index edb3f876a..5718e187a 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. @@ -303,6 +328,11 @@ int mbedtls_hmac_drbg_update_ret( 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 @@ -328,6 +358,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. @@ -357,7 +392,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 0c11622504322df4557dfd7a12348e5fe8087b5e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:50:03 +0100 Subject: [PATCH 14/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 468ef4b3c7b04fd829e5579193e7a0b4ac4c1603 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 31 Jan 2021 00:07:11 +0100 Subject: [PATCH 15/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 889d3bca7..3aee8542c 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -191,6 +191,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 ab5849527dca640b1acb7130fd2af9e1bb176e61 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Feb 2021 17:55:24 +0100 Subject: [PATCH 16/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 Signed-off-by: Chris Jones --- 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 5e4de17e2..4ed833903 100644 --- a/tests/suites/test_suite_rsa.data +++ b/tests/suites/test_suite_rsa.data @@ -1,6 +1,12 @@ RSA parameter validation rsa_invalid_param: +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 f8a2dad0d..89b419fbd 100644 --- a/tests/suites/test_suite_rsa.function +++ b/tests/suites/test_suite_rsa.function @@ -465,6 +465,29 @@ exit: } /* END_CASE */ +/* 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( data_t * message_str, int padding_mode, int digest, int mod, int radix_P, char * input_P, From b9fce3cea111cc0e2239bf3593fff312d6d2e60f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Feb 2021 17:57:41 +0100 Subject: [PATCH 17/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 000754649..0b3011f14 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -520,6 +520,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 } @@ -2502,7 +2505,6 @@ int mbedtls_rsa_copy( mbedtls_rsa_context *dst, const mbedtls_rsa_context *src ) RSA_VALIDATE_RET( dst != NULL ); RSA_VALIDATE_RET( src != NULL ); - dst->ver = src->ver; dst->len = src->len; MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &dst->N, &src->N ) ); @@ -2561,7 +2563,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 718972e94e3a7f7beee2f89548a4f415e5874ad7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Feb 2021 21:06:10 +0100 Subject: [PATCH 18/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 | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 0b3011f14..c8c23dba8 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -570,9 +570,6 @@ int mbedtls_rsa_gen_key( mbedtls_rsa_context *ctx, RSA_VALIDATE_RET( ctx != NULL ); RSA_VALIDATE_RET( f_rng != NULL ); - if( nbits < 128 || exponent < 3 || nbits % 2 != 0 ) - return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); - /* * If the modulus is 1024 bit long or shorter, then the security strength of * the RSA algorithm is less than or equal to 80 bits and therefore an error @@ -585,6 +582,12 @@ int mbedtls_rsa_gen_key( mbedtls_rsa_context *ctx, mbedtls_mpi_init( &G ); mbedtls_mpi_init( &L ); + if( 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: * 1. |P-Q| > 2^( nbits / 2 - 100 ) @@ -662,7 +665,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 3c30a7aedaa2e396c17a66031d059620870636c1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:52:49 +0100 Subject: [PATCH 19/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 ce455ddb3e8c30fc66f8a0c60cc96f82ce9860d9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:59:42 +0100 Subject: [PATCH 20/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 188c37cf3..b2f65334f 100644 --- a/include/mbedtls/rsa.h +++ b/include/mbedtls/rsa.h @@ -124,7 +124,10 @@ extern "C" { */ typedef struct mbedtls_rsa_context { - 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. */ @@ -154,6 +157,7 @@ typedef struct mbedtls_rsa_context 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 ed9f7989f21a130f25e375a8938fd34ed23c3af1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 15 Feb 2021 18:21:55 +0100 Subject: [PATCH 21/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 0bf81895a..ceac6703f 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -827,7 +827,7 @@ int mbedtls_test_hexcmp( uint8_t * a, uint8_t * b, uint32_t a_len, uint32_t b_le * 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 6855d1a4572f3bcffee6dddcfe248d035bd72068 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Wed, 17 Feb 2021 17:20:00 +0000 Subject: [PATCH 22/24] Add MBEDTLS_TEST_HOOKS to query_config.c Signed-off-by: Chris Jones --- programs/ssl/query_config.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index 798c9178f..0a7eb1aec 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -1475,6 +1475,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_SSL_TRUNCATED_HMAC_COMPAT */ +#if defined(MBEDTLS_TEST_HOOKS) + if( strcmp( "MBEDTLS_TEST_HOOKS", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_TEST_HOOKS ); + return( 0 ); + } +#endif /* MBEDTLS_TEST_HOOKS */ + #if defined(MBEDTLS_THREADING_ALT) if( strcmp( "MBEDTLS_THREADING_ALT", config ) == 0 ) { From 210a0168d55c6631a30b1aa914233e187ffaa7ca 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 3aee8542c..f4f9693fb 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -125,6 +125,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 57f8e9116eea0c34bd4051479e7b9d0cf3333973 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 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,