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. diff --git a/ChangeLog.d/rsa-mutex.txt b/ChangeLog.d/rsa-mutex.txt new file mode 100644 index 000000000..2a477a9cb --- /dev/null +++ b/ChangeLog.d/rsa-mutex.txt @@ -0,0 +1,13 @@ +Bugfix + * 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. + +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/config.h b/include/mbedtls/config.h index 6b45021d0..fff4240c0 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1746,6 +1746,23 @@ */ //#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. + * + * Uncomment to enable invasive tests. + */ +//#define MBEDTLS_TEST_HOOKS + /** * \def MBEDTLS_THREADING_ALT * diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 278fbbbb7..6c099adf4 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 } @@ -277,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 @@ -286,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. @@ -377,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. @@ -394,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. @@ -417,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. @@ -445,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. 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/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index 970c033c1..5718e187a 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; @@ -177,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 @@ -216,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 @@ -279,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. @@ -295,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 @@ -320,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. @@ -349,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. 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 } 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/ctr_drbg.c b/library/ctr_drbg.c index e92008bbe..90264e844 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,13 @@ void mbedtls_ctr_drbg_free( mbedtls_ctr_drbg_context *ctx ) return; #if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_free( &ctx->mutex ); + /* The mutex is initialized iff f_entropy is set. */ + 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 +417,11 @@ 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 + mbedtls_aes_init( &ctx->aes_ctx ); ctx->f_entropy = f_entropy; 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, diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 10cbd462b..b45d61616 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,11 @@ 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 + md_size = mbedtls_md_get_size( md_info ); /* @@ -451,14 +456,13 @@ void mbedtls_hmac_drbg_free( mbedtls_hmac_drbg_context *ctx ) return; #if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_free( &ctx->mutex ); + /* The mutex is initialized iff the md context is set up. */ + 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) diff --git a/library/rsa.c b/library/rsa.c index 000754649..c8c23dba8 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 } @@ -567,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 @@ -582,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 ) @@ -659,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 ); @@ -2502,7 +2510,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 +2568,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 } 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/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 */ 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 ) { 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 diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 9403d9905..ceac6703f 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 * @@ -371,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; @@ -777,3 +786,202 @@ 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. + * + * 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, + * use a debugger and set a breakpoint on mbedtls_test_mutex_usage_error(). + */ +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 +}; + +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; + +/** 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 ) +{ + (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 ); + if( mutex->is_valid ) + ++live_mutexes; +} + +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; + } + if( mutex->is_valid ) + --live_mutexes; + 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 ) +{ + 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 ) +{ + 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; +} + +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 ) + { + /* 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/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 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 */ } /** 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 889d3bca7..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 ) { @@ -191,6 +213,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 */ 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,