From 9de97e21fe1628512429fd1aeeaf012665d8e7ef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Feb 2021 21:00:11 +0100 Subject: [PATCH 01/28] Make {USE_,}PSA_{INIT,DONE} available in all test suites Make USE_PSA_INIT() and USE_PSA_DONE() available in all test suites in all cases, doing nothing if MBEDTLS_USE_PSA_CRYPTO is disabled. Use those in preference to having explicit defined(MBEDTLS_USE_PSA_CRYPTO) checks (but there may still be places left where using the new macros would be better). Also provide PSA_INIT() by symmetry with PSA_DONE(), functional whenver MBEDTLS_PSA_CRYPTO_C is enabled, but currently unused. Signed-off-by: Gilles Peskine --- tests/include/test/psa_crypto_helpers.h | 34 ++++++++++++++++++++++ tests/suites/helpers.function | 2 -- tests/suites/test_suite_pk.function | 24 ++++----------- tests/suites/test_suite_ssl.function | 4 +-- tests/suites/test_suite_x509parse.function | 24 +++++---------- tests/suites/test_suite_x509write.function | 6 ---- 6 files changed, 49 insertions(+), 45 deletions(-) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index b7dc4b5ea3..30bb20f077 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -22,11 +22,20 @@ #define PSA_CRYPTO_HELPERS_H #include "test/helpers.h" + +#if defined(MBEDTLS_PSA_CRYPTO_C) + #include "test/psa_helpers.h" #include #include +#if defined(MBEDTLS_USE_PSA_CRYPTO) +#include "mbedtls/psa_util.h" +#endif + +#define PSA_INIT( ) PSA_ASSERT( psa_crypto_init( ) ) + /** Check for things that have not been cleaned up properly in the * PSA subsystem. * @@ -185,4 +194,29 @@ psa_status_t mbedtls_test_record_status( psa_status_t status, } \ while( 0 ) +#endif /* MBEDTLS_PSA_CRYPTO_C */ + +/** \def USE_PSA_INIT + * + * Call this macro to initialize the PSA subsystem if #MBEDTLS_USE_PSA_CRYPTO + * is enabled and do nothing otherwise. If the initialization fails, mark + * the test case as failed and jump to the \p exit label. + */ +/** \def USE_PSA_DONE + * + * Call this macro at the end of a test case if you called #USE_PSA_INIT. + * This is like #PSA_DONE, except that it does nothing if + * #MBEDTLS_USE_PSA_CRYPTO is disabled. + */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) +#define USE_PSA_INIT( ) PSA_INIT( ) +#define USE_PSA_DONE( ) PSA_DONE( ) +#else /* MBEDTLS_USE_PSA_CRYPTO */ +/* Define empty macros so that we can use them in the preamble and teardown + * of every test function that uses PSA conditionally based on + * MBEDTLS_USE_PSA_CRYPTO. */ +#define USE_PSA_INIT( ) ( (void) 0 ) +#define USE_PSA_DONE( ) ( (void) 0 ) +#endif /* !MBEDTLS_USE_PSA_CRYPTO */ + #endif /* PSA_CRYPTO_HELPERS_H */ diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index ebe2f06273..91ad925fbf 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -5,9 +5,7 @@ #include #include #include -#if defined(MBEDTLS_PSA_CRYPTO_C) #include -#endif #include diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 577fb474d2..bc469b68db 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -15,18 +15,6 @@ * unconditionally (https://github.com/ARMmbed/mbedtls/issues/2023). */ #include "psa/crypto.h" -#if defined(MBEDTLS_USE_PSA_CRYPTO) -#include "mbedtls/psa_util.h" -#define PSA_INIT( ) PSA_ASSERT( psa_crypto_init( ) ) -#else -/* Define empty macros so that we can use them in the preamble and teardown - * of every test function that uses PSA conditionally based on - * MBEDTLS_USE_PSA_CRYPTO. */ -#define PSA_INIT( ) ( (void) 0 ) -#undef PSA_DONE -#define PSA_DONE( ) ( (void) 0 ) -#endif - #define RSA_KEY_SIZE 512 #define RSA_KEY_LEN 64 @@ -208,7 +196,7 @@ exit: mbedtls_pk_free( &pk ); /* redundant except upon error */ mbedtls_pk_free( &pk2 ); - PSA_DONE( ); + USE_PSA_DONE( ); } /* END_CASE */ @@ -770,7 +758,7 @@ void pk_ec_test_vec( int type, int id, data_t * key, data_t * hash, mbedtls_ecp_keypair *eckey; mbedtls_pk_init( &pk ); - PSA_INIT( ); + USE_PSA_INIT( ); TEST_ASSERT( mbedtls_pk_setup( &pk, mbedtls_pk_info_from_type( type ) ) == 0 ); @@ -787,7 +775,7 @@ void pk_ec_test_vec( int type, int id, data_t * key, data_t * hash, exit: mbedtls_pk_free( &pk ); - PSA_DONE( ); + USE_PSA_DONE( ); } /* END_CASE */ @@ -911,7 +899,7 @@ void pk_sign_verify( int type, int parameter, int sign_ret, int verify_ret ) #endif mbedtls_pk_init( &pk ); - PSA_INIT( ); + USE_PSA_INIT( ); memset( hash, 0x2a, sizeof hash ); memset( sig, 0, sizeof sig ); @@ -973,7 +961,7 @@ exit: mbedtls_pk_restart_free( rs_ctx ); #endif mbedtls_pk_free( &pk ); - PSA_DONE( ); + USE_PSA_DONE( ); } /* END_CASE */ @@ -1302,6 +1290,6 @@ exit: psa_reset_key_attributes( &attributes ); mbedtls_pk_free( &pk ); - PSA_DONE( ); + USE_PSA_DONE( ); } /* END_CASE */ diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 5c97d90e42..93cf50ca07 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -3836,9 +3836,7 @@ void ssl_tls_prf( int type, data_t * secret, data_t * random, if( output == NULL ) goto exit; -#if defined(MBEDTLS_USE_PSA_CRYPTO) - TEST_ASSERT( psa_crypto_init() == 0 ); -#endif + USE_PSA_INIT( ); TEST_ASSERT( mbedtls_ssl_tls_prf( type, secret->x, secret->len, label, random->x, random->len, diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 2bba4e2f79..b09c554600 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -610,14 +610,12 @@ void x509_verify( char *crt_file, char *ca_file, char *crl_file, char * cn_name = NULL; const mbedtls_x509_crt_profile *profile; -#if defined(MBEDTLS_USE_PSA_CRYPTO) - TEST_ASSERT( psa_crypto_init() == 0 ); -#endif - mbedtls_x509_crt_init( &crt ); mbedtls_x509_crt_init( &ca ); mbedtls_x509_crl_init( &crl ); + USE_PSA_INIT( ); + if( strcmp( cn_name_str, "NULL" ) != 0 ) cn_name = cn_name_str; @@ -712,14 +710,12 @@ void x509_verify_callback( char *crt_file, char *ca_file, char *name, uint32_t flags = 0; verify_print_context vrfy_ctx; -#if defined(MBEDTLS_USE_PSA_CRYPTO) - TEST_ASSERT( psa_crypto_init() == 0 ); -#endif - mbedtls_x509_crt_init( &crt ); mbedtls_x509_crt_init( &ca ); verify_print_init( &vrfy_ctx ); + USE_PSA_INIT( ); + TEST_ASSERT( mbedtls_x509_crt_parse_file( &crt, crt_file ) == 0 ); TEST_ASSERT( mbedtls_x509_crt_parse_file( &ca, ca_file ) == 0 ); @@ -1024,10 +1020,6 @@ void mbedtls_x509_crt_verify_max( char *ca_file, char *chain_dir, int nb_int, uint32_t flags; mbedtls_x509_crt trusted, chain; -#if defined(MBEDTLS_USE_PSA_CRYPTO) - TEST_ASSERT( psa_crypto_init() == 0 ); -#endif - /* * We expect chain_dir to contain certificates 00.crt, 01.crt, etc. * with NN.crt signed by NN-1.crt @@ -1036,6 +1028,8 @@ void mbedtls_x509_crt_verify_max( char *ca_file, char *chain_dir, int nb_int, mbedtls_x509_crt_init( &trusted ); mbedtls_x509_crt_init( &chain ); + USE_PSA_INIT( ); + /* Load trusted root */ TEST_ASSERT( mbedtls_x509_crt_parse_file( &trusted, ca_file ) == 0 ); @@ -1069,13 +1063,11 @@ void mbedtls_x509_crt_verify_chain( char *chain_paths, char *trusted_ca, mbedtls_x509_crt trusted, chain; const mbedtls_x509_crt_profile *profile = NULL; -#if defined(MBEDTLS_USE_PSA_CRYPTO) - TEST_ASSERT( psa_crypto_init() == 0 ); -#endif - mbedtls_x509_crt_init( &chain ); mbedtls_x509_crt_init( &trusted ); + USE_PSA_INIT( ); + while( ( act = mystrsep( &chain_paths, " " ) ) != NULL ) TEST_ASSERT( mbedtls_x509_crt_parse_file( &chain, act ) == 0 ); TEST_ASSERT( mbedtls_x509_crt_parse_file( &trusted, trusted_ca ) == 0 ); diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index 9960989ffa..59ea17b2c5 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -6,12 +6,6 @@ #include "mbedtls/oid.h" #include "mbedtls/rsa.h" -#if defined(MBEDTLS_USE_PSA_CRYPTO) -#include "psa/crypto.h" -#include "mbedtls/psa_util.h" -#define PSA_INIT( ) PSA_ASSERT( psa_crypto_init( ) ) -#endif /* MBEDTLS_USE_PSA_CRYPTO */ - #if defined(MBEDTLS_RSA_C) int mbedtls_rsa_decrypt_func( void *ctx, int mode, size_t *olen, const unsigned char *input, unsigned char *output, From 1f186ff330de59140e9afae322b9284f89db6783 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Feb 2021 21:04:06 +0100 Subject: [PATCH 02/28] Add missing calls to USE_PSA_DONE Some functions were not deinitializing the PSA subsystem. This could lead to resource leaks at the level of individual test cases, and possibly at the level of the whole test suite depending on the order and selection of test cases. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ssl.function | 1 + tests/suites/test_suite_x509parse.function | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 93cf50ca07..b1ebf5b97a 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -3850,6 +3850,7 @@ void ssl_tls_prf( int type, data_t * secret, data_t * random, exit: mbedtls_free( output ); + USE_PSA_DONE( ); } /* END_CASE */ diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index b09c554600..66f03768b7 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -667,6 +667,7 @@ exit: mbedtls_x509_crt_free( &crt ); mbedtls_x509_crt_free( &ca ); mbedtls_x509_crl_free( &crl ); + USE_PSA_DONE( ); } /* END_CASE */ @@ -733,6 +734,7 @@ void x509_verify_callback( char *crt_file, char *ca_file, char *name, exit: mbedtls_x509_crt_free( &crt ); mbedtls_x509_crt_free( &ca ); + USE_PSA_DONE( ); } /* END_CASE */ @@ -1049,6 +1051,7 @@ void mbedtls_x509_crt_verify_max( char *ca_file, char *chain_dir, int nb_int, exit: mbedtls_x509_crt_free( &chain ); mbedtls_x509_crt_free( &trusted ); + USE_PSA_DONE( ); } /* END_CASE */ @@ -1092,6 +1095,7 @@ void mbedtls_x509_crt_verify_chain( char *chain_paths, char *trusted_ca, exit: mbedtls_x509_crt_free( &trusted ); mbedtls_x509_crt_free( &chain ); + USE_PSA_DONE( ); } /* END_CASE */ From 1061ec678205ee7e3b4ed4ee3dfa61094e314758 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 21:17:11 +0100 Subject: [PATCH 03/28] 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/include/test/helpers.h | 9 +++++ tests/src/threading_helpers.c | 69 +++++++++++++++++++++++++++++++++ tests/suites/host_test.function | 4 ++ visualc/VS2010/mbedTLS.vcxproj | 1 + 4 files changed, 83 insertions(+) create mode 100644 tests/src/threading_helpers.c diff --git a/tests/include/test/helpers.h b/tests/include/test/helpers.h index 928c636075..aa0370166f 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -260,4 +260,13 @@ void mbedtls_test_param_failed_reset_state( void ); #include "test/fake_external_rng_for_test.h" #endif +#if defined(MBEDTLS_THREADING_C) && defined(MBEDTLS_THREADING_PTHREAD) && \ + defined(MBEDTLS_TEST_HOOKS) +#define MBEDTLS_TEST_MUTEX_USAGE + +/** Permanently activate the mutex usage verification framework. See + * threading_helpers.c for information. */ +void mbedtls_test_mutex_usage_init( void ); +#endif /* MBEDTLS_TEST_MUTEX_USAGE */ + #endif /* TEST_HELPERS_H */ diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c new file mode 100644 index 0000000000..d1ac542f9b --- /dev/null +++ b/tests/src/threading_helpers.c @@ -0,0 +1,69 @@ +/** Mutex usage verification framework. */ + +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + +#include "mbedtls/threading.h" + +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 ) ); +} + +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 3138c33655..f4f4f45bdf 100644 --- a/tests/suites/host_test.function +++ b/tests/suites/host_test.function @@ -536,6 +536,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 diff --git a/visualc/VS2010/mbedTLS.vcxproj b/visualc/VS2010/mbedTLS.vcxproj index e6d6532cca..da026d953a 100644 --- a/visualc/VS2010/mbedTLS.vcxproj +++ b/visualc/VS2010/mbedTLS.vcxproj @@ -358,6 +358,7 @@ + From 2a4c598859c65a729410d0a08fcb91b8a9dcbcee Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 21:18:09 +0100 Subject: [PATCH 04/28] 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/include/test/helpers.h | 17 +++-- tests/src/threading_helpers.c | 118 +++++++++++++++++++++++++++++++- tests/suites/main_test.function | 4 ++ 3 files changed, 134 insertions(+), 5 deletions(-) diff --git a/tests/include/test/helpers.h b/tests/include/test/helpers.h index aa0370166f..c3a844b600 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -31,6 +31,11 @@ #include MBEDTLS_CONFIG_FILE #endif +#if defined(MBEDTLS_THREADING_C) && defined(MBEDTLS_THREADING_PTHREAD) && \ + defined(MBEDTLS_TEST_HOOKS) +#define MBEDTLS_TEST_MUTEX_USAGE +#endif + #if defined(MBEDTLS_PLATFORM_C) #include "mbedtls/platform.h" #else @@ -63,6 +68,9 @@ typedef struct const char *filename; int line_no; unsigned long step; +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + const char *mutex_usage_error; +#endif } mbedtls_test_info_t; extern mbedtls_test_info_t mbedtls_test_info; @@ -260,13 +268,14 @@ void mbedtls_test_param_failed_reset_state( void ); #include "test/fake_external_rng_for_test.h" #endif -#if defined(MBEDTLS_THREADING_C) && defined(MBEDTLS_THREADING_PTHREAD) && \ - defined(MBEDTLS_TEST_HOOKS) -#define MBEDTLS_TEST_MUTEX_USAGE - +#if defined(MBEDTLS_TEST_MUTEX_USAGE) /** Permanently activate the mutex usage verification framework. See * threading_helpers.c for information. */ void mbedtls_test_mutex_usage_init( void ); + +/** Call this function after executing a test case to check for mutex usage + * errors. */ +void mbedtls_test_mutex_usage_check( void ); #endif /* MBEDTLS_TEST_MUTEX_USAGE */ #endif /* TEST_HELPERS_H */ diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index d1ac542f9b..3a58bc731f 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -24,6 +24,48 @@ #include "mbedtls/threading.h" +/** 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 +}; + typedef struct { void (*init)( mbedtls_threading_mutex_t * ); @@ -33,6 +75,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( mbedtls_test_info.mutex_usage_error == NULL ) + mbedtls_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 ); @@ -40,18 +95,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 ); } void mbedtls_test_mutex_usage_init( void ) @@ -66,4 +170,16 @@ void mbedtls_test_mutex_usage_init( void ) mbedtls_mutex_unlock = &mbedtls_test_wrap_mutex_unlock; } +void mbedtls_test_mutex_usage_check( void ) +{ + if( mbedtls_test_info.mutex_usage_error != NULL && + mbedtls_test_info.result != MBEDTLS_TEST_RESULT_FAILED ) + { + /* Functionally, the test passed. But there was a mutex usage error, + * so mark the test as failed after all. */ + mbedtls_test_fail( "Mutex usage error", __LINE__, __FILE__ ); + } + mbedtls_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 57395aebb3..aa408eaafc 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -188,6 +188,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 f96d3d8b204a8fc407534a52a1fe4f3dcf7b3161 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 29 Jan 2021 22:20:32 +0100 Subject: [PATCH 05/28] 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/src/threading_helpers.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 3a58bc731f..8cf95ee336 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -54,6 +54,17 @@ * 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 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 * indicate the exact location of the problematic call. To locate the error, @@ -75,6 +86,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 ) { @@ -91,6 +109,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 ) @@ -111,6 +131,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 ); } @@ -172,6 +194,17 @@ void mbedtls_test_mutex_usage_init( void ) 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( mbedtls_test_info.mutex_usage_error == NULL ) + mbedtls_test_info.mutex_usage_error = "missing free"; + } if( mbedtls_test_info.mutex_usage_error != NULL && mbedtls_test_info.result != MBEDTLS_TEST_RESULT_FAILED ) { From 39a1a26d0b6036832632baf99a240f35bef0b105 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 15:35:29 +0100 Subject: [PATCH 06/28] 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/src/threading_helpers.c | 7 ++++++- 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index 8baf15a75b..05e27c52f1 100644 --- a/include/mbedtls/threading.h +++ b/include/mbedtls/threading.h @@ -46,6 +46,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 2bb932d2d0..2de117f52a 100644 --- a/library/threading.c +++ b/library/threading.c @@ -67,6 +67,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 + * tests/src/threading_helpers for details. */ mutex->is_valid = pthread_mutex_init( &mutex->mutex, NULL ) == 0; } diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 8cf95ee336..ca91b7933a 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -70,8 +70,13 @@ * 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 f4b3429782c66d439d20616229bffe3d226a73f4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 30 Jan 2021 13:05:32 +0100 Subject: [PATCH 07/28] 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 3815dc7ca8..48efada970 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -56,10 +56,6 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ) ctx->reseed_counter = -1; ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; - -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_init( &ctx->mutex ); -#endif } /* @@ -72,15 +68,13 @@ 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; ctx->reseed_counter = -1; -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_init( &ctx->mutex ); -#endif } void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, @@ -464,6 +458,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 da290f9bcd8e1b1373ad11c188652ac631f1ab01 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:44:02 +0100 Subject: [PATCH 08/28] 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 7f1d23253c..550006eefd 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -200,6 +200,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 48efada970..ab52861d57 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -68,6 +68,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 @@ -458,6 +459,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 f305d92480c81c6eb02934a4e1af58152cba28ea Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:44:18 +0100 Subject: [PATCH 09/28] Document thread safety for CTR_DRBG random(), and only this function, is thread-safe. Signed-off-by: Gilles Peskine --- 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 550006eefd..653fd83d54 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -271,6 +271,15 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * make a second call to \p f_entropy. */ #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 */ /** * - The \p custom string. * @@ -297,6 +306,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. @@ -412,6 +423,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. @@ -429,6 +445,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. @@ -452,6 +473,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. @@ -480,8 +506,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 b791dc66cec538dd64cb6ec88e26814ecff18d45 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 31 Jan 2021 00:06:51 +0100 Subject: [PATCH 10/28] 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 25a0225835..d322b657d9 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -54,10 +54,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 } /* @@ -129,6 +125,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 @@ -254,6 +254,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 ); /* @@ -421,14 +425,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 e39b2192e5f9f45542cfe4e889dd482e18777842 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:43:33 +0100 Subject: [PATCH 11/28] 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 91165415fb..43bd62e53a 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -101,6 +101,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 d322b657d9..de9706885c 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -254,6 +254,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 @@ -425,6 +426,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 478847cca3d95bab248e9fa7ce5902c20540eae6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:45:10 +0100 Subject: [PATCH 12/28] Document thread safety for HMAC_DRBG random(), and only this function, is thread-safe. Signed-off-by: Gilles Peskine --- 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 43bd62e53a..fa33611f2d 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -158,7 +158,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 @@ -197,7 +207,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 @@ -260,6 +280,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. @@ -276,6 +301,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 @@ -301,6 +331,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. @@ -330,7 +365,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 71edf749e1e88f47cbfc6467dbc19a2a8199ee12 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:50:03 +0100 Subject: [PATCH 13/28] 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 0000000000..3ac5abfa88 --- /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 7aba0361542ab9fb8319c3833474ce64a2a75ec6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 31 Jan 2021 00:07:11 +0100 Subject: [PATCH 14/28] Add missing cleanup in 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 d9ea441492..2a7b0de135 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -217,6 +217,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 914afe1fdb705fc46a8588e57e52abcd30718da5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Feb 2021 17:55:24 +0100 Subject: [PATCH 15/28] 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 30919f3df3..6f9406ce13 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 6c73e39473..cdbaa13e6e 100644 --- a/tests/suites/test_suite_rsa.function +++ b/tests/suites/test_suite_rsa.function @@ -466,6 +466,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 eb94059eddbc3c5687d61dd8684f15f7f49f490d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Feb 2021 17:57:41 +0100 Subject: [PATCH 16/28] 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 9fe551d51c..8c6a507771 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -490,6 +490,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 } @@ -2481,7 +2484,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 ) ); @@ -2540,7 +2542,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 5e40a7cfa033619c35f583aeb5ebd6a185c3786b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Feb 2021 21:06:10 +0100 Subject: [PATCH 17/28] 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 8c6a507771..68a36f2534 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -540,9 +540,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 @@ -555,6 +552,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 ) @@ -632,7 +635,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 1226ecef01bbfac3b184486eb68f48d25d69c304 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:52:49 +0100 Subject: [PATCH 18/28] 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 0000000000..bafb7b2d5e --- /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 4337a9cb185297050f50c626b0e967ba9a4fceec Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 9 Feb 2021 18:59:42 +0100 Subject: [PATCH 19/28] 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 bafb7b2d5e..49f1a84f28 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 6a315144d3..701fe8bed9 100644 --- a/include/mbedtls/rsa.h +++ b/include/mbedtls/rsa.h @@ -97,7 +97,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. */ @@ -127,6 +130,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 53dea743d516ed16211f2e2fd577d0d94ad9bf17 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Feb 2021 22:55:06 +0100 Subject: [PATCH 20/28] SSL test programs: allow for test hooks init and error reports Create utility functions to set up test hooks and report errors that the test hooks might detect. Call them in ssl_client2 and ssl_server2. Test hooks are potentially enabled by compiling with MBEDTLS_TEST_HOOKS. This commit only sets up the functions. It doesn't make them do anything yet. Signed-off-by: Gilles Peskine --- programs/ssl/ssl_client2.c | 10 ++++++++++ programs/ssl/ssl_server2.c | 10 ++++++++++ programs/ssl/ssl_test_lib.c | 17 +++++++++++++++++ programs/ssl/ssl_test_lib.h | 31 +++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 4875b785c3..25f8ff4223 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -734,6 +734,8 @@ int main( int argc, char *argv[] ) mbedtls_memory_buffer_alloc_init( alloc_buf, sizeof(alloc_buf) ); #endif + test_hooks_init( ); + /* * Make sure memory references are valid. */ @@ -3036,6 +3038,14 @@ exit: mbedtls_free( context_buf ); #endif + if( test_hooks_failure_detected( ) ) + { + if( ret == 0 ) + ret = 1; + mbedtls_printf( "Test hooks detected errors.\n" ); + } + test_hooks_free( ); + #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) #if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_status(); diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 08317b5b03..c04f47a717 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -1369,6 +1369,8 @@ int main( int argc, char *argv[] ) #endif /* MBEDTLS_MEMORY_DEBUG */ #endif /* MBEDTLS_MEMORY_BUFFER_ALLOC_C */ + test_hooks_init( ); + /* * Make sure memory references are valid in case we exit early. */ @@ -3998,6 +4000,14 @@ exit: mbedtls_free( context_buf ); #endif + if( test_hooks_failure_detected( ) ) + { + if( ret == 0 ) + ret = 1; + mbedtls_printf( "Test hooks detected errors.\n" ); + } + test_hooks_free( ); + #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) #if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_status(); diff --git a/programs/ssl/ssl_test_lib.c b/programs/ssl/ssl_test_lib.c index 6636e9e1ad..92d1b97587 100644 --- a/programs/ssl/ssl_test_lib.c +++ b/programs/ssl/ssl_test_lib.c @@ -321,4 +321,21 @@ int idle( mbedtls_net_context *fd, return( 0 ); } +#if defined(MBEDTLS_TEST_HOOKS) + +void test_hooks_init( void ) +{ +} + +int test_hooks_failure_detected( void ) +{ + return( 0 ); +} + +void test_hooks_free( void ) +{ +} + +#endif /* MBEDTLS_TEST_HOOKS */ + #endif /* !defined(MBEDTLS_SSL_TEST_IMPOSSIBLE) */ diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index 04ba15874c..dd6ed0c47d 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -258,5 +258,36 @@ int idle( mbedtls_net_context *fd, #endif int idle_reason ); +#if defined(MBEDTLS_TEST_HOOKS) +/** Initialize whatever test hooks are enabled by the compile-time + * configuration and make sense for the TLS test programs. */ +void test_hooks_init( void ); + +/** Check if any test hooks detected a problem. + * + * If a problem was detected, make sure to print an explanation to stderr, + * either at the time of detection or during the call to this function. + * + * \return Nonzero if a problem was detected. + * \c 0 if no problem was detected. + */ +int test_hooks_failure_detected( void ); + +/** Free any resources allocated for the sake of test hooks. + * + * Call this at the end of the program so that resource leak analyzers + * don't complain. + */ +void test_hooks_free( void ); + +#else /* MBEDTLS_TEST_HOOKS */ + +/* Define macros that do nothing, for convenience. */ +#define test_hooks_init( ) ( (void) 0 ) +#define test_hooks_failure_detected( ) 0 +#define test_hooks_free( ) ( (void) 0 ) + +#endif /* !MBEDTLS_TEST_HOOKS */ + #endif /* MBEDTLS_SSL_TEST_IMPOSSIBLE conditions: else */ #endif /* MBEDTLS_PROGRAMS_SSL_SSL_TEST_LIB_H */ From d0a46e5c7f4d04e2dc3cc5f2ec175249a7c665a4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Feb 2021 00:03:03 +0100 Subject: [PATCH 21/28] ssl_server2: don't check test hooks failure in query_config mode Test hook failure checks may print information to stdout, which messes up the usage of query_config mode. Nothing interesting happens in query_config mode anyway, so that's no loss. Signed-off-by: Gilles Peskine --- programs/ssl/ssl_server2.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index c04f47a717..293c30b10e 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -4000,11 +4000,17 @@ exit: mbedtls_free( context_buf ); #endif - if( test_hooks_failure_detected( ) ) + /* Let test hooks detect errors such as resource leaks. + * Don't do it in query_config mode, because some test code prints + * information to stdout and this gets mixed with the regular output. */ + if( opt.query_config_mode == DFL_QUERY_CONFIG_MODE ) { - if( ret == 0 ) - ret = 1; - mbedtls_printf( "Test hooks detected errors.\n" ); + if( test_hooks_failure_detected( ) ) + { + if( ret == 0 ) + ret = 1; + mbedtls_printf( "Test hooks detected errors.\n" ); + } } test_hooks_free( ); From e374b95fe16019323de4eedc7da6252b8f777058 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Feb 2021 00:05:19 +0100 Subject: [PATCH 22/28] Detect and report mutex usage errors in SSL test programs Signed-off-by: Gilles Peskine --- programs/ssl/ssl_test_lib.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/programs/ssl/ssl_test_lib.c b/programs/ssl/ssl_test_lib.c index 92d1b97587..1bb9d6162f 100644 --- a/programs/ssl/ssl_test_lib.c +++ b/programs/ssl/ssl_test_lib.c @@ -22,6 +22,10 @@ #include "ssl_test_lib.h" +#if defined(MBEDTLS_TEST_HOOKS) +#include "test/helpers.h" +#endif + #if !defined(MBEDTLS_SSL_TEST_IMPOSSIBLE) void my_debug( void *ctx, int level, @@ -325,10 +329,22 @@ int idle( mbedtls_net_context *fd, void test_hooks_init( void ) { + mbedtls_test_info_reset( ); + +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + mbedtls_test_mutex_usage_init( ); +#endif } int test_hooks_failure_detected( void ) { +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + /* Errors are reported via mbedtls_test_info. */ + mbedtls_test_mutex_usage_check( ); +#endif + + if( mbedtls_test_info.result != MBEDTLS_TEST_RESULT_SUCCESS ) + return( 1 ); return( 0 ); } From 414e7170361eba2e6546911128af364fce23596a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Feb 2021 00:04:08 +0100 Subject: [PATCH 23/28] Deinitialize the PSA subsystem The PSA subsystem may consume global resources. It currently doesn't consume any heap when no keys are registered, but it may do so in the future. It does consume mutexes, which are reported as leaks when mutex usage checking is enabled. Signed-off-by: Gilles Peskine --- programs/ssl/ssl_client2.c | 4 ++++ programs/ssl/ssl_server2.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 25f8ff4223..ef804c9648 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3038,6 +3038,10 @@ exit: mbedtls_free( context_buf ); #endif +#if defined(MBEDTLS_USE_PSA_CRYPTO) + mbedtls_psa_crypto_free( ); +#endif + if( test_hooks_failure_detected( ) ) { if( ret == 0 ) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 293c30b10e..1c68e76340 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -4000,6 +4000,10 @@ exit: mbedtls_free( context_buf ); #endif +#if defined(MBEDTLS_USE_PSA_CRYPTO) + mbedtls_psa_crypto_free( ); +#endif + /* Let test hooks detect errors such as resource leaks. * Don't do it in query_config mode, because some test code prints * information to stdout and this gets mixed with the regular output. */ From 76e9c64c3eeed6c10c1a70955a3fa0290dda719e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 15 Feb 2021 10:59:13 +0100 Subject: [PATCH 24/28] Clarify the advice about reporting errors in test hooks Signed-off-by: Gilles Peskine --- programs/ssl/ssl_test_lib.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index dd6ed0c47d..8f37d6341a 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -265,8 +265,9 @@ void test_hooks_init( void ); /** Check if any test hooks detected a problem. * - * If a problem was detected, make sure to print an explanation to stderr, - * either at the time of detection or during the call to this function. + * \note When implementing a test hook, make sure to print a message + * to standard error either at the time the problem is detected + * or during the execution of this function. * * \return Nonzero if a problem was detected. * \c 0 if no problem was detected. From 00d0ad40362e4d74d07d7fec9abccd6d87c2bdee Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 15 Feb 2021 11:02:51 +0100 Subject: [PATCH 25/28] Clarify the advice about reporting errors in test hooks Signed-off-by: Gilles Peskine --- programs/ssl/ssl_test_lib.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index 8f37d6341a..2712da67a2 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -265,9 +265,16 @@ void test_hooks_init( void ); /** Check if any test hooks detected a problem. * - * \note When implementing a test hook, make sure to print a message - * to standard error either at the time the problem is detected - * or during the execution of this function. + * If a problem was detected, it's ok for the calling program to keep going, + * but it should ultimately exit with an error status. + * + * \note When implementing a test hook that detects errors on its own + * (as opposed to e.g. leaving the error for a memory sanitizer to + * report), make sure to print a message to standard error either at + * the time the problem is detected or during the execution of this + * function. This function does not indicate what problem was detected, + * so printing a message is the only way to provide feedback in the + * logs of the calling program. * * \return Nonzero if a problem was detected. * \c 0 if no problem was detected. From 60fe6606bf528cee6e4a859c17f35c6ff7b91b9f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Feb 2021 18:56:42 +0100 Subject: [PATCH 26/28] Only define test_hooks_xxx under MBEDTLS_TEST_HOOKS Signed-off-by: Gilles Peskine --- programs/ssl/ssl_client2.c | 4 ++++ programs/ssl/ssl_server2.c | 4 ++++ programs/ssl/ssl_test_lib.h | 7 ------- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index ef804c9648..d0758bcf46 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -734,7 +734,9 @@ int main( int argc, char *argv[] ) mbedtls_memory_buffer_alloc_init( alloc_buf, sizeof(alloc_buf) ); #endif +#if defined(MBEDTLS_TEST_HOOKS) test_hooks_init( ); +#endif /* MBEDTLS_TEST_HOOKS */ /* * Make sure memory references are valid. @@ -3042,6 +3044,7 @@ exit: mbedtls_psa_crypto_free( ); #endif +#if defined(MBEDTLS_TEST_HOOKS) if( test_hooks_failure_detected( ) ) { if( ret == 0 ) @@ -3049,6 +3052,7 @@ exit: mbedtls_printf( "Test hooks detected errors.\n" ); } test_hooks_free( ); +#endif /* MBEDTLS_TEST_HOOKS */ #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) #if defined(MBEDTLS_MEMORY_DEBUG) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 1c68e76340..bd4dbb64ba 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -1369,7 +1369,9 @@ int main( int argc, char *argv[] ) #endif /* MBEDTLS_MEMORY_DEBUG */ #endif /* MBEDTLS_MEMORY_BUFFER_ALLOC_C */ +#if defined(MBEDTLS_TEST_HOOKS) test_hooks_init( ); +#endif /* MBEDTLS_TEST_HOOKS */ /* * Make sure memory references are valid in case we exit early. @@ -4004,6 +4006,7 @@ exit: mbedtls_psa_crypto_free( ); #endif +#if defined(MBEDTLS_TEST_HOOKS) /* Let test hooks detect errors such as resource leaks. * Don't do it in query_config mode, because some test code prints * information to stdout and this gets mixed with the regular output. */ @@ -4017,6 +4020,7 @@ exit: } } test_hooks_free( ); +#endif /* MBEDTLS_TEST_HOOKS */ #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) #if defined(MBEDTLS_MEMORY_DEBUG) diff --git a/programs/ssl/ssl_test_lib.h b/programs/ssl/ssl_test_lib.h index 2712da67a2..98751a0f03 100644 --- a/programs/ssl/ssl_test_lib.h +++ b/programs/ssl/ssl_test_lib.h @@ -288,13 +288,6 @@ int test_hooks_failure_detected( void ); */ void test_hooks_free( void ); -#else /* MBEDTLS_TEST_HOOKS */ - -/* Define macros that do nothing, for convenience. */ -#define test_hooks_init( ) ( (void) 0 ) -#define test_hooks_failure_detected( ) 0 -#define test_hooks_free( ) ( (void) 0 ) - #endif /* !MBEDTLS_TEST_HOOKS */ #endif /* MBEDTLS_SSL_TEST_IMPOSSIBLE conditions: else */ From 3d979f781e7d49810b6a72f41a18c7347b624361 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 22 Feb 2021 21:24:02 +0100 Subject: [PATCH 27/28] 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 b2d20b4729..2ce01fabda 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 2a7b0de135..ad9b5a45a1 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -134,6 +134,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 b15832160b220124af8d8a29336ff2b5520e5bd6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 22 Feb 2021 21:26:54 +0100 Subject: [PATCH 28/28] 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). 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 49f1a84f28..2a477a9cbb 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 5a9c11c3f2..fa0b24f679 100644 --- a/include/mbedtls/entropy.h +++ b/include/mbedtls/entropy.h @@ -120,13 +120,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 db61f16d84..b9aca86b1e 100644 --- a/library/entropy.c +++ b/library/entropy.c @@ -116,6 +116,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 @@ -132,7 +137,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,