diff --git a/ChangeLog.d/fix_some_resource_leaks.txt b/ChangeLog.d/fix_some_resource_leaks.txt new file mode 100644 index 000000000..9761537d8 --- /dev/null +++ b/ChangeLog.d/fix_some_resource_leaks.txt @@ -0,0 +1,6 @@ +Bugfix + * Fix resource leaks in mbedtls_pk_parse_public_key() in low + memory conditions. +Security + * Fix potential memory leak inside mbedtls_ssl_cache_set() with + an invalid session id length. diff --git a/library/pkparse.c b/library/pkparse.c index 68727ec7e..6f409689f 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -1454,10 +1454,16 @@ int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx, { p = pem.buf; if( ( pk_info = mbedtls_pk_info_from_type( MBEDTLS_PK_RSA ) ) == NULL ) + { + mbedtls_pem_free( &pem ); return( MBEDTLS_ERR_PK_UNKNOWN_PK_ALG ); + } if( ( ret = mbedtls_pk_setup( ctx, pk_info ) ) != 0 ) + { + mbedtls_pem_free( &pem ); return( ret ); + } if ( ( ret = pk_get_rsapubkey( &p, p + pem.buflen, mbedtls_pk_rsa( *ctx ) ) ) != 0 ) mbedtls_pk_free( ctx ); diff --git a/library/ssl_cache.c b/library/ssl_cache.c index fe4f30cf8..d19847428 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -312,7 +312,11 @@ exit: #endif if( session_serialized != NULL ) + { mbedtls_platform_zeroize( session_serialized, session_serialized_len ); + mbedtls_free( session_serialized ); + session_serialized = NULL; + } return( ret ); } diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 1b36d324e..a851d5c74 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3426,3 +3426,6 @@ raw_key_agreement_fail:0 Raw key agreement: bad server key raw_key_agreement_fail:1 + +Force a bad session id length +force_bad_session_id_len diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index efec5ea46..c65b56554 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -9,6 +9,10 @@ #include #include "test/certs.h" +#if defined(MBEDTLS_SSL_CACHE_C) +#include "mbedtls/ssl_cache.h" +#endif + #include #include @@ -82,38 +86,58 @@ typedef struct handshake_test_options void (*srv_log_fun)(void *, int, const char *, int, const char *); void (*cli_log_fun)(void *, int, const char *, int, const char *); int resize_buffers; +#if defined(MBEDTLS_SSL_CACHE_C) + mbedtls_ssl_cache_context *cache; +#endif } handshake_test_options; void init_handshake_options( handshake_test_options *opts ) { - opts->cipher = ""; - opts->client_min_version = MBEDTLS_SSL_VERSION_UNKNOWN; - opts->client_max_version = MBEDTLS_SSL_VERSION_UNKNOWN; - opts->server_min_version = MBEDTLS_SSL_VERSION_UNKNOWN; - opts->server_max_version = MBEDTLS_SSL_VERSION_UNKNOWN; - opts->expected_negotiated_version = MBEDTLS_SSL_VERSION_TLS1_2; - opts->expected_handshake_result = 0; - opts->expected_ciphersuite = 0; - opts->pk_alg = MBEDTLS_PK_RSA; - opts->opaque_alg = 0; - opts->opaque_alg2 = 0; - opts->opaque_usage = 0; - opts->psk_str = NULL; - opts->dtls = 0; - opts->srv_auth_mode = MBEDTLS_SSL_VERIFY_NONE; - opts->serialize = 0; - opts->mfl = MBEDTLS_SSL_MAX_FRAG_LEN_NONE; - opts->cli_msg_len = 100; - opts->srv_msg_len = 100; - opts->expected_cli_fragments = 1; - opts->expected_srv_fragments = 1; - opts->renegotiate = 0; - opts->legacy_renegotiation = MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION; - opts->srv_log_obj = NULL; - opts->srv_log_obj = NULL; - opts->srv_log_fun = NULL; - opts->cli_log_fun = NULL; - opts->resize_buffers = 1; + opts->cipher = ""; + opts->client_min_version = MBEDTLS_SSL_VERSION_UNKNOWN; + opts->client_max_version = MBEDTLS_SSL_VERSION_UNKNOWN; + opts->server_min_version = MBEDTLS_SSL_VERSION_UNKNOWN; + opts->server_max_version = MBEDTLS_SSL_VERSION_UNKNOWN; + opts->expected_negotiated_version = MBEDTLS_SSL_VERSION_TLS1_2; + opts->expected_handshake_result = 0; + opts->expected_ciphersuite = 0; + opts->pk_alg = MBEDTLS_PK_RSA; + opts->opaque_alg = 0; + opts->opaque_alg2 = 0; + opts->opaque_usage = 0; + opts->psk_str = NULL; + opts->dtls = 0; + opts->srv_auth_mode = MBEDTLS_SSL_VERIFY_NONE; + opts->serialize = 0; + opts->mfl = MBEDTLS_SSL_MAX_FRAG_LEN_NONE; + opts->cli_msg_len = 100; + opts->srv_msg_len = 100; + opts->expected_cli_fragments = 1; + opts->expected_srv_fragments = 1; + opts->renegotiate = 0; + opts->legacy_renegotiation = MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION; + opts->srv_log_obj = NULL; + opts->srv_log_obj = NULL; + opts->srv_log_fun = NULL; + opts->cli_log_fun = NULL; + opts->resize_buffers = 1; +#if defined(MBEDTLS_SSL_CACHE_C) + opts->cache = NULL; + ASSERT_ALLOC( opts->cache, sizeof( mbedtls_ssl_cache_context ) ); + mbedtls_ssl_cache_init( opts->cache ); +exit: + return; +#endif +} + +void free_handshake_options( handshake_test_options *opts ) +{ +#if defined(MBEDTLS_SSL_CACHE_C) + mbedtls_ssl_cache_free( opts->cache ); + mbedtls_free( opts->cache ); +#else + (void) opts; +#endif } /* * Buffer structure for custom I/O callbacks. @@ -918,9 +942,8 @@ exit: * * \retval 0 on success, otherwise error code. */ - -int mbedtls_endpoint_init( mbedtls_endpoint *ep, int endpoint_type, int pk_alg, - int opaque_alg, int opaque_alg2, int opaque_usage, +int mbedtls_endpoint_init( mbedtls_endpoint *ep, int endpoint_type, + handshake_test_options *options, mbedtls_test_message_socket_context *dtls_context, mbedtls_test_message_queue *input_queue, mbedtls_test_message_queue *output_queue, @@ -1002,6 +1025,15 @@ int mbedtls_endpoint_init( mbedtls_endpoint *ep, int endpoint_type, int pk_alg, mbedtls_ssl_conf_authmode( &( ep->conf ), MBEDTLS_SSL_VERIFY_REQUIRED ); +#if defined(MBEDTLS_SSL_CACHE_C) && defined(MBEDTLS_SSL_SRV_C) + if( endpoint_type == MBEDTLS_SSL_IS_SERVER && options->cache != NULL ) + { + mbedtls_ssl_conf_session_cache( &( ep->conf ), options->cache, + mbedtls_ssl_cache_get, + mbedtls_ssl_cache_set ); + } +#endif + ret = mbedtls_ssl_setup( &( ep->ssl ), &( ep->conf ) ); TEST_ASSERT( ret == 0 ); @@ -1010,8 +1042,10 @@ int mbedtls_endpoint_init( mbedtls_endpoint *ep, int endpoint_type, int pk_alg, mbedtls_ssl_conf_dtls_cookies( &( ep->conf ), NULL, NULL, NULL ); #endif - ret = mbedtls_endpoint_certificate_init( ep, pk_alg, opaque_alg, - opaque_alg2, opaque_usage ); + ret = mbedtls_endpoint_certificate_init( ep, options->pk_alg, + options->opaque_alg, + options->opaque_alg2, + options->opaque_usage ); TEST_ASSERT( ret == 0 ); TEST_EQUAL( mbedtls_ssl_conf_get_user_data_n( &ep->conf ), user_data_n ); @@ -1952,7 +1986,7 @@ exit: #if defined(MBEDTLS_X509_CRT_PARSE_C) && \ defined(MBEDTLS_ENTROPY_C) && \ defined(MBEDTLS_CTR_DRBG_C) -void perform_handshake( handshake_test_options* options ) +void perform_handshake( handshake_test_options *options ) { /* forced_ciphersuite needs to last until the end of the handshake */ int forced_ciphersuite[2]; @@ -1984,11 +2018,7 @@ void perform_handshake( handshake_test_options* options ) if( options->dtls != 0 ) { TEST_ASSERT( mbedtls_endpoint_init( &client, MBEDTLS_SSL_IS_CLIENT, - options->pk_alg, - options->opaque_alg, - options->opaque_alg2, - options->opaque_usage, - &client_context, + options, &client_context, &client_queue, &server_queue, NULL ) == 0 ); #if defined(MBEDTLS_TIMING_C) @@ -2000,11 +2030,7 @@ void perform_handshake( handshake_test_options* options ) else { TEST_ASSERT( mbedtls_endpoint_init( &client, MBEDTLS_SSL_IS_CLIENT, - options->pk_alg, - options->opaque_alg, - options->opaque_alg2, - options->opaque_usage, - NULL, NULL, + options, NULL, NULL, NULL, NULL ) == 0 ); } @@ -2038,11 +2064,7 @@ void perform_handshake( handshake_test_options* options ) if( options->dtls != 0 ) { TEST_ASSERT( mbedtls_endpoint_init( &server, MBEDTLS_SSL_IS_SERVER, - options->pk_alg, - options->opaque_alg, - options->opaque_alg2, - options->opaque_usage, - &server_context, + options, &server_context, &server_queue, &client_queue, NULL ) == 0 ); #if defined(MBEDTLS_TIMING_C) @@ -2054,12 +2076,8 @@ void perform_handshake( handshake_test_options* options ) else { TEST_ASSERT( mbedtls_endpoint_init( &server, MBEDTLS_SSL_IS_SERVER, - options->pk_alg, - options->opaque_alg, - options->opaque_alg2, - options->opaque_usage, - NULL, NULL, - NULL, NULL ) == 0 ); + options, NULL, NULL, NULL, + NULL ) == 0 ); } mbedtls_ssl_conf_authmode( &server.conf, options->srv_auth_mode ); @@ -4771,20 +4789,24 @@ void mbedtls_endpoint_sanity( int endpoint_type ) enum { BUFFSIZE = 1024 }; mbedtls_endpoint ep; int ret = -1; + handshake_test_options options; + init_handshake_options( &options ); + options.pk_alg = MBEDTLS_PK_RSA; - ret = mbedtls_endpoint_init( NULL, endpoint_type, MBEDTLS_PK_RSA, - 0, 0, 0, NULL, NULL, NULL, NULL ); + ret = mbedtls_endpoint_init( NULL, endpoint_type, &options, + NULL, NULL, NULL, NULL ); TEST_ASSERT( MBEDTLS_ERR_SSL_BAD_INPUT_DATA == ret ); - ret = mbedtls_endpoint_certificate_init( NULL, MBEDTLS_PK_RSA, 0, 0, 0 ); + ret = mbedtls_endpoint_certificate_init( NULL, options.pk_alg, 0, 0, 0 ); TEST_ASSERT( MBEDTLS_ERR_SSL_BAD_INPUT_DATA == ret ); - ret = mbedtls_endpoint_init( &ep, endpoint_type, MBEDTLS_PK_RSA, 0, 0, 0, + ret = mbedtls_endpoint_init( &ep, endpoint_type, &options, NULL, NULL, NULL, NULL ); TEST_ASSERT( ret == 0 ); exit: mbedtls_endpoint_free( &ep, NULL ); + free_handshake_options( &options ); } /* END_CASE */ @@ -4794,18 +4816,21 @@ void move_handshake_to_state(int endpoint_type, int state, int need_pass) enum { BUFFSIZE = 1024 }; mbedtls_endpoint base_ep, second_ep; int ret = -1; + handshake_test_options options; + init_handshake_options( &options ); + options.pk_alg = MBEDTLS_PK_RSA; USE_PSA_INIT( ); - ret = mbedtls_endpoint_init( &base_ep, endpoint_type, MBEDTLS_PK_RSA, - 0, 0, 0, NULL, NULL, NULL, NULL ); + ret = mbedtls_endpoint_init( &base_ep, endpoint_type, &options, + NULL, NULL, NULL, NULL ); TEST_ASSERT( ret == 0 ); ret = mbedtls_endpoint_init( &second_ep, ( endpoint_type == MBEDTLS_SSL_IS_SERVER ) ? MBEDTLS_SSL_IS_CLIENT : MBEDTLS_SSL_IS_SERVER, - MBEDTLS_PK_RSA, 0, 0, 0, - NULL, NULL, NULL, NULL ); + &options, NULL, NULL, NULL, NULL ); + TEST_ASSERT( ret == 0 ); ret = mbedtls_mock_socket_connect( &(base_ep.socket), @@ -4832,6 +4857,7 @@ void move_handshake_to_state(int endpoint_type, int state, int need_pass) } exit: + free_handshake_options( &options ); mbedtls_endpoint_free( &base_ep, NULL ); mbedtls_endpoint_free( &second_ep, NULL ); USE_PSA_DONE( ); @@ -4857,6 +4883,9 @@ void handshake_version( int dtls, int client_min_version, int client_max_version /* The goto below is used to avoid an "unused label" warning.*/ goto exit; + +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -4875,6 +4904,9 @@ void handshake_psk_cipher( char* cipher, int pk_alg, data_t *psk_str, int dtls ) /* The goto below is used to avoid an "unused label" warning.*/ goto exit; + +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -4909,6 +4941,9 @@ void handshake_ciphersuite_select( char* cipher, int pk_alg, data_t *psk_str, /* The goto below is used to avoid an "unused label" warning.*/ goto exit; + +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -4931,8 +4966,12 @@ void app_data( int mfl, int cli_msg_len, int srv_msg_len, #endif perform_handshake( &options ); + /* The goto below is used to avoid an "unused label" warning.*/ goto exit; + +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -4971,6 +5010,8 @@ void handshake_serialization( ) perform_handshake( &options ); /* The goto below is used to avoid an "unused label" warning.*/ goto exit; +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -5007,6 +5048,9 @@ void handshake_fragmentation( int mfl, int expected_srv_hs_fragmentation, int ex { TEST_ASSERT( cli_pattern.counter >= 1 ); } + +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -5021,8 +5065,11 @@ void renegotiation( int legacy_renegotiation ) options.dtls = 1; perform_handshake( &options ); + /* The goto below is used to avoid an "unused label" warning.*/ goto exit; +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -5042,8 +5089,11 @@ void resize_buffers( int mfl, int renegotiation, int legacy_renegotiation, options.resize_buffers = 1; perform_handshake( &options ); + /* The goto below is used to avoid an "unused label" warning.*/ goto exit; +exit: + free_handshake_options( &options ); } /* END_CASE */ @@ -5459,6 +5509,71 @@ void conf_group() } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_CACHE_C:!MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_DEBUG_C:MBEDTLS_X509_CRT_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_ECP_DP_SECP384R1_ENABLED:MBEDTLS_PKCS1_V15:MBEDTLS_ENTROPY_C:MBEDTLS_CTR_DRBG_C */ +void force_bad_session_id_len( ) +{ + enum { BUFFSIZE = 1024 }; + handshake_test_options options; + mbedtls_endpoint client, server; + log_pattern srv_pattern, cli_pattern; + mbedtls_test_message_socket_context server_context, client_context; + + srv_pattern.pattern = cli_pattern.pattern = "cache did not store session"; + srv_pattern.counter = 0; + init_handshake_options( &options ); + + options.srv_log_obj = &srv_pattern; + options.srv_log_fun = log_analyzer; + + USE_PSA_INIT( ); + + mbedtls_message_socket_init( &server_context ); + mbedtls_message_socket_init( &client_context ); + + TEST_ASSERT( mbedtls_endpoint_init( &client, MBEDTLS_SSL_IS_CLIENT, + &options, NULL, NULL, + NULL, NULL ) == 0 ); + + TEST_ASSERT( mbedtls_endpoint_init( &server, MBEDTLS_SSL_IS_SERVER, + &options, NULL, NULL, NULL, + NULL ) == 0 ); + + mbedtls_debug_set_threshold( 1 ); + mbedtls_ssl_conf_dbg( &server.conf, options.srv_log_fun, + options.srv_log_obj ); + + TEST_ASSERT( mbedtls_mock_socket_connect( &(client.socket), + &(server.socket), + BUFFSIZE ) == 0 ); + + TEST_ASSERT( mbedtls_move_handshake_to_state( &(client.ssl), + &(server.ssl), + MBEDTLS_SSL_HANDSHAKE_WRAPUP ) + == 0 ); + /* Force a bad session_id_len that will be read by the server in + * mbedtls_ssl_cache_set. */ + server.ssl.session_negotiate->id_len = 33; + if( options.cli_msg_len != 0 || options.srv_msg_len != 0 ) + { + /* Start data exchanging test */ + TEST_ASSERT( mbedtls_exchange_data( &(client.ssl), options.cli_msg_len, + options.expected_cli_fragments, + &(server.ssl), options.srv_msg_len, + options.expected_srv_fragments ) + == 0 ); + } + + /* Make sure that the cache did not store the session */ + TEST_EQUAL( srv_pattern.counter, 1 ); +exit: + mbedtls_endpoint_free( &client, NULL ); + mbedtls_endpoint_free( &server, NULL ); + free_handshake_options( &options ); + mbedtls_debug_set_threshold( 0 ); + USE_PSA_DONE( ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_TIMING_C:MBEDTLS_HAVE_TIME */ void timing_final_delay_accessor( ) { @@ -5558,22 +5673,26 @@ void raw_key_agreement_fail( int bad_server_ecdhe_key ) mbedtls_endpoint client, server; mbedtls_psa_stats_t stats; size_t free_slots_before = -1; + handshake_test_options options; uint16_t iana_tls_group_list[] = { MBEDTLS_SSL_IANA_TLS_GROUP_SECP256R1, MBEDTLS_SSL_IANA_TLS_GROUP_NONE }; USE_PSA_INIT( ); + init_handshake_options( &options ); + options.pk_alg = MBEDTLS_PK_ECDSA; + /* Client side, force SECP256R1 to make one key bitflip fail * the raw key agreement. Flipping the first byte makes the * required 0x04 identifier invalid. */ TEST_EQUAL( mbedtls_endpoint_init( &client, MBEDTLS_SSL_IS_CLIENT, - MBEDTLS_PK_ECDSA, 0, 0, 0, NULL, NULL, + &options, NULL, NULL, NULL, iana_tls_group_list ), 0 ); /* Server side */ TEST_EQUAL( mbedtls_endpoint_init( &server, MBEDTLS_SSL_IS_SERVER, - MBEDTLS_PK_ECDSA, 0, 0, 0, - NULL, NULL, NULL, NULL ), 0 ); + &options, NULL, NULL, + NULL, NULL ), 0 ); TEST_EQUAL( mbedtls_mock_socket_connect( &(client.socket), &(server.socket), @@ -5611,6 +5730,7 @@ void raw_key_agreement_fail( int bad_server_ecdhe_key ) exit: mbedtls_endpoint_free( &client, NULL ); mbedtls_endpoint_free( &server, NULL ); + free_handshake_options( &options ); USE_PSA_DONE( ); }