From 7c7fb877c655d3ee1655a03e46a434ab5a0db42d Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Tue, 26 Oct 2021 14:32:10 +0200 Subject: [PATCH 1/5] ssl_client2, ssl_server2: add check for psa memory leaks Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_client2.c | 13 +++++++++++++ programs/ssl/ssl_server2.c | 12 ++++++++++++ tests/include/test/psa_crypto_helpers.h | 2 ++ 3 files changed, 27 insertions(+) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 393798138..3cad338b3 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -19,6 +19,11 @@ #include "ssl_test_lib.h" +#if defined(MBEDTLS_USE_PSA_CRYPTO) +#define SKIP_LIBRARY_HEADERS +#include "test/psa_crypto_helpers.h" +#endif + #if defined(MBEDTLS_SSL_TEST_IMPOSSIBLE) int main( void ) { @@ -3041,7 +3046,15 @@ exit: #endif #if defined(MBEDTLS_USE_PSA_CRYPTO) + mbedtls_psa_crypto_free( ); + const char* message = mbedtls_test_helper_is_psa_leaking(); + if( message ) + { + if( ret == 0 ) + ret = 1; + mbedtls_printf( "PSA memory leak detected: %s\n", message); + } #endif #if defined(MBEDTLS_TEST_HOOKS) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index bd4dbb64b..e62a61051 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -63,6 +63,11 @@ int main( void ) #include #endif +#if defined(MBEDTLS_USE_PSA_CRYPTO) +#define SKIP_LIBRARY_HEADERS +#include "test/psa_crypto_helpers.h" +#endif + /* Size of memory to be allocated for the heap, when using the library's memory * management and MBEDTLS_MEMORY_BUFFER_ALLOC_C is enabled. */ #define MEMORY_HEAP_SIZE 120000 @@ -4004,6 +4009,13 @@ exit: #if defined(MBEDTLS_USE_PSA_CRYPTO) mbedtls_psa_crypto_free( ); + const char* message = mbedtls_test_helper_is_psa_leaking(); + if( message ) + { + if( ret == 0 ) + ret = 1; + mbedtls_printf( "PSA memory leak detected: %s\n", message); + } #endif #if defined(MBEDTLS_TEST_HOOKS) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 8a8c37e00..8e7d425a9 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -28,7 +28,9 @@ #include "test/psa_helpers.h" #include +#if !defined(SKIP_LIBRARY_HEADERS) #include +#endif #if defined(MBEDTLS_USE_PSA_CRYPTO) #include "mbedtls/psa_util.h" From d6e0a5824af1a5825d1cccb3e924c4c1fb043e32 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Wed, 3 Nov 2021 09:06:09 +0100 Subject: [PATCH 2/5] ssl_client2/ssl_server2: Move is_psa_leaking() before mbedtls_psa_crypto_free() (and rng_free()) Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_client2.c | 29 +++++++++++++++++------------ programs/ssl/ssl_server2.c | 32 +++++++++++++++++++------------- 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 3cad338b3..251489b8b 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3032,6 +3032,23 @@ exit: #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED && MBEDTLS_USE_PSA_CRYPTO */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) + const char* message = mbedtls_test_helper_is_psa_leaking(); + if( message ) + { + if( ret == 0 ) + ret = 1; + mbedtls_printf( "PSA memory leak detected: %s\n", message); + } +#endif /* MBEDTLS_USE_PSA_CRYPTO */ + + /* For builds with MBEDTLS_TEST_USE_PSA_CRYPTO_RNG psa crypto + * resources are freed by rng_free(). */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) && \ + !defined(MBEDTLS_TEST_USE_PSA_CRYPTO_RNG) + mbedtls_psa_crypto_free( ); +#endif + mbedtls_ssl_session_free( &saved_session ); mbedtls_ssl_free( &ssl ); mbedtls_ssl_config_free( &conf ); @@ -3045,18 +3062,6 @@ exit: mbedtls_free( context_buf ); #endif -#if defined(MBEDTLS_USE_PSA_CRYPTO) - - mbedtls_psa_crypto_free( ); - const char* message = mbedtls_test_helper_is_psa_leaking(); - if( message ) - { - if( ret == 0 ) - ret = 1; - mbedtls_printf( "PSA memory leak detected: %s\n", message); - } -#endif - #if defined(MBEDTLS_TEST_HOOKS) if( test_hooks_failure_detected( ) ) { diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index e62a61051..0787250d7 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -3985,10 +3985,6 @@ exit: #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED && MBEDTLS_USE_PSA_CRYPTO */ - mbedtls_ssl_free( &ssl ); - mbedtls_ssl_config_free( &conf ); - rng_free( &rng ); - #if defined(MBEDTLS_SSL_CACHE_C) mbedtls_ssl_cache_free( &cache ); #endif @@ -3999,16 +3995,7 @@ exit: mbedtls_ssl_cookie_free( &cookie_ctx ); #endif - mbedtls_free( buf ); - -#if defined(MBEDTLS_SSL_CONTEXT_SERIALIZATION) - if( context_buf != NULL ) - mbedtls_platform_zeroize( context_buf, context_buf_len ); - mbedtls_free( context_buf ); -#endif - #if defined(MBEDTLS_USE_PSA_CRYPTO) - mbedtls_psa_crypto_free( ); const char* message = mbedtls_test_helper_is_psa_leaking(); if( message ) { @@ -4018,6 +4005,25 @@ exit: } #endif + /* For builds with MBEDTLS_TEST_USE_PSA_CRYPTO_RNG psa crypto + * resources are freed by rng_free(). */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) && \ + !defined(MBEDTLS_TEST_USE_PSA_CRYPTO_RNG) + mbedtls_psa_crypto_free( ); +#endif + + mbedtls_ssl_free( &ssl ); + mbedtls_ssl_config_free( &conf ); + rng_free( &rng ); + + mbedtls_free( buf ); + +#if defined(MBEDTLS_SSL_CONTEXT_SERIALIZATION) + if( context_buf != NULL ) + mbedtls_platform_zeroize( context_buf, context_buf_len ); + mbedtls_free( context_buf ); +#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 From b66bc0ad4a5d0087674c9e37b3c3dfe0e564acf5 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Wed, 3 Nov 2021 09:35:35 +0100 Subject: [PATCH 3/5] Move psa_crypto_slot_management.h out from psa_crypto_helpers.h Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_client2.c | 1 - programs/ssl/ssl_server2.c | 1 - tests/include/test/psa_crypto_helpers.h | 3 --- tests/src/psa_crypto_helpers.c | 1 + tests/src/psa_exercise_key.c | 1 + 5 files changed, 2 insertions(+), 5 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 251489b8b..d8408d80e 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -20,7 +20,6 @@ #include "ssl_test_lib.h" #if defined(MBEDTLS_USE_PSA_CRYPTO) -#define SKIP_LIBRARY_HEADERS #include "test/psa_crypto_helpers.h" #endif diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 0787250d7..8864955d4 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -64,7 +64,6 @@ int main( void ) #endif #if defined(MBEDTLS_USE_PSA_CRYPTO) -#define SKIP_LIBRARY_HEADERS #include "test/psa_crypto_helpers.h" #endif diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 8e7d425a9..f5622e2d2 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -28,9 +28,6 @@ #include "test/psa_helpers.h" #include -#if !defined(SKIP_LIBRARY_HEADERS) -#include -#endif #if defined(MBEDTLS_USE_PSA_CRYPTO) #include "mbedtls/psa_util.h" diff --git a/tests/src/psa_crypto_helpers.c b/tests/src/psa_crypto_helpers.c index d9d841abd..299b6d125 100644 --- a/tests/src/psa_crypto_helpers.c +++ b/tests/src/psa_crypto_helpers.c @@ -22,6 +22,7 @@ #include #include +#include #include #if defined(MBEDTLS_PSA_CRYPTO_C) diff --git a/tests/src/psa_exercise_key.c b/tests/src/psa_exercise_key.c index 91bac678e..fc58fbd48 100644 --- a/tests/src/psa_exercise_key.c +++ b/tests/src/psa_exercise_key.c @@ -29,6 +29,7 @@ #include #include +#include #include #if defined(MBEDTLS_PSA_CRYPTO_SE_C) From e9dea7c3b020cff88b481811c9e27cd493b2a031 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Wed, 3 Nov 2021 14:19:52 +0100 Subject: [PATCH 4/5] ssl_client2: move memory leak check before rng_free() Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_client2.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index d8408d80e..91e231c76 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3031,6 +3031,10 @@ exit: #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED && MBEDTLS_USE_PSA_CRYPTO */ + mbedtls_ssl_session_free( &saved_session ); + mbedtls_ssl_free( &ssl ); + mbedtls_ssl_config_free( &conf ); + #if defined(MBEDTLS_USE_PSA_CRYPTO) const char* message = mbedtls_test_helper_is_psa_leaking(); if( message ) @@ -3048,9 +3052,6 @@ exit: mbedtls_psa_crypto_free( ); #endif - mbedtls_ssl_session_free( &saved_session ); - mbedtls_ssl_free( &ssl ); - mbedtls_ssl_config_free( &conf ); rng_free( &rng ); if( session_data != NULL ) mbedtls_platform_zeroize( session_data, session_data_len ); From a226ac9738a9b9630b0acdc0844540036daa2e97 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Wed, 10 Nov 2021 10:46:11 +0100 Subject: [PATCH 5/5] ssl_client2/ssl_server2: Rework ordering of cleanup Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_client2.c | 25 ++++++++------- programs/ssl/ssl_server2.c | 64 ++++++++++++++++++++------------------ 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 91e231c76..574caa611 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3001,6 +3001,19 @@ exit: mbedtls_net_free( &server_fd ); + mbedtls_ssl_free( &ssl ); + mbedtls_ssl_config_free( &conf ); + mbedtls_ssl_session_free( &saved_session ); + + if( session_data != NULL ) + mbedtls_platform_zeroize( session_data, session_data_len ); + mbedtls_free( session_data ); +#if defined(MBEDTLS_SSL_CONTEXT_SERIALIZATION) + if( context_buf != NULL ) + mbedtls_platform_zeroize( context_buf, context_buf_len ); + mbedtls_free( context_buf ); +#endif + #if defined(MBEDTLS_X509_CRT_PARSE_C) mbedtls_x509_crt_free( &clicert ); mbedtls_x509_crt_free( &cacert ); @@ -3031,10 +3044,6 @@ exit: #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED && MBEDTLS_USE_PSA_CRYPTO */ - mbedtls_ssl_session_free( &saved_session ); - mbedtls_ssl_free( &ssl ); - mbedtls_ssl_config_free( &conf ); - #if defined(MBEDTLS_USE_PSA_CRYPTO) const char* message = mbedtls_test_helper_is_psa_leaking(); if( message ) @@ -3053,14 +3062,6 @@ exit: #endif rng_free( &rng ); - if( session_data != NULL ) - mbedtls_platform_zeroize( session_data, session_data_len ); - mbedtls_free( session_data ); -#if defined(MBEDTLS_SSL_CONTEXT_SERIALIZATION) - if( context_buf != NULL ) - mbedtls_platform_zeroize( context_buf, context_buf_len ); - mbedtls_free( context_buf ); -#endif #if defined(MBEDTLS_TEST_HOOKS) if( test_hooks_failure_detected( ) ) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 8864955d4..329305ea1 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -3932,9 +3932,35 @@ exit: mbedtls_net_free( &client_fd ); mbedtls_net_free( &listen_fd ); -#if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_FS_IO) - mbedtls_dhm_free( &dhm ); + mbedtls_ssl_free( &ssl ); + mbedtls_ssl_config_free( &conf ); + +#if defined(MBEDTLS_SSL_CACHE_C) + mbedtls_ssl_cache_free( &cache ); #endif +#if defined(MBEDTLS_SSL_SESSION_TICKETS) + mbedtls_ssl_ticket_free( &ticket_ctx ); +#endif +#if defined(MBEDTLS_SSL_COOKIE_C) + mbedtls_ssl_cookie_free( &cookie_ctx ); +#endif + +#if defined(MBEDTLS_SSL_CONTEXT_SERIALIZATION) + if( context_buf != NULL ) + mbedtls_platform_zeroize( context_buf, context_buf_len ); + mbedtls_free( context_buf ); +#endif + +#if defined(SNI_OPTION) + sni_free( sni_info ); +#endif + +#if defined(MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED) + ret = psk_free( psk_info ); + if( ( ret != 0 ) && ( opt.query_config_mode == DFL_QUERY_CONFIG_MODE ) ) + mbedtls_printf( "Failed to list of opaque PSKs - error was %d\n", ret ); +#endif + #if defined(MBEDTLS_X509_CRT_PARSE_C) mbedtls_x509_crt_free( &cacert ); mbedtls_x509_crt_free( &srvcert ); @@ -3942,6 +3968,11 @@ exit: mbedtls_x509_crt_free( &srvcert2 ); mbedtls_pk_free( &pkey2 ); #endif + +#if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_FS_IO) + mbedtls_dhm_free( &dhm ); +#endif + #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) for( i = 0; (size_t) i < ssl_async_keys.slots_used; i++ ) { @@ -3953,17 +3984,6 @@ exit: } } #endif -#if defined(SNI_OPTION) - sni_free( sni_info ); -#endif -#if defined(MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED) - ret = psk_free( psk_info ); - if( ( ret != 0 ) && ( opt.query_config_mode == DFL_QUERY_CONFIG_MODE ) ) - mbedtls_printf( "Failed to list of opaque PSKs - error was %d\n", ret ); -#endif -#if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_FS_IO) - mbedtls_dhm_free( &dhm ); -#endif #if defined(MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED) && \ defined(MBEDTLS_USE_PSA_CRYPTO) @@ -3984,16 +4004,6 @@ exit: #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED && MBEDTLS_USE_PSA_CRYPTO */ -#if defined(MBEDTLS_SSL_CACHE_C) - mbedtls_ssl_cache_free( &cache ); -#endif -#if defined(MBEDTLS_SSL_SESSION_TICKETS) - mbedtls_ssl_ticket_free( &ticket_ctx ); -#endif -#if defined(MBEDTLS_SSL_COOKIE_C) - mbedtls_ssl_cookie_free( &cookie_ctx ); -#endif - #if defined(MBEDTLS_USE_PSA_CRYPTO) const char* message = mbedtls_test_helper_is_psa_leaking(); if( message ) @@ -4011,18 +4021,10 @@ exit: mbedtls_psa_crypto_free( ); #endif - mbedtls_ssl_free( &ssl ); - mbedtls_ssl_config_free( &conf ); rng_free( &rng ); mbedtls_free( buf ); -#if defined(MBEDTLS_SSL_CONTEXT_SERIALIZATION) - if( context_buf != NULL ) - mbedtls_platform_zeroize( context_buf, context_buf_len ); - mbedtls_free( context_buf ); -#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