From fed825a9aaabf9f66066ce2a71f44404e2f69a39 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 f872e60fc..130f3f98e 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -21,6 +21,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 ) { @@ -3059,7 +3064,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 c0f319674..d20d1faa1 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -65,6 +65,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 @@ -4027,6 +4032,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 bbb22bbd9e42e76f5899fd14d57413a6512fcf55 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 130f3f98e..deecbad3e 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3050,6 +3050,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 ); @@ -3063,18 +3080,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 d20d1faa1..f95c15184 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -4008,10 +4008,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 @@ -4022,16 +4018,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 ) { @@ -4041,6 +4028,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 53de2622f3667bb812de0fe17d8988bfa6da6a27 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 deecbad3e..62bba1380 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -22,7 +22,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 f95c15184..6a4a033aa 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -66,7 +66,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 923d2c136..29e673ae7 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 505712338ea7ac8661026c2acd8f8ccb310c7e01 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 62bba1380..4360fd343 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3049,6 +3049,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 ) @@ -3066,9 +3070,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 d6914e3196909a00fe0ade7af64c0a921ebd0b96 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 4360fd343..01459c08e 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3019,6 +3019,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 ); @@ -3049,10 +3062,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 ) @@ -3071,14 +3080,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 6a4a033aa..6f5d118d6 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -3951,9 +3951,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 ); @@ -3965,6 +3991,11 @@ exit: psa_destroy_key( key_slot2 ); #endif #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++ ) { @@ -3976,17 +4007,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) @@ -4007,16 +4027,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 ) @@ -4034,18 +4044,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