Merge pull request #5765 from leorosen/fix-some-resource-leaks

Fix resource leaks

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.

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 );

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 );
 }

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

tests/suites/test_suite_ssl.function

@@ -9,6 +9,10 @@
 #include <ssl_tls13_invasive.h>
 #include "test/certs.h"
 
+#if defined(MBEDTLS_SSL_CACHE_C)
+#include "mbedtls/ssl_cache.h"
+#endif
+
 #include <psa/crypto.h>
 
 #include <constant_time_internal.h>
@@ -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->cipher = "";
-  opts->client_min_version = MBEDTLS_SSL_VERSION_UNKNOWN;
+    opts->client_min_version = MBEDTLS_SSL_VERSION_UNKNOWN;
-  opts->client_max_version = MBEDTLS_SSL_VERSION_UNKNOWN;
+    opts->client_max_version = MBEDTLS_SSL_VERSION_UNKNOWN;
-  opts->server_min_version = MBEDTLS_SSL_VERSION_UNKNOWN;
+    opts->server_min_version = MBEDTLS_SSL_VERSION_UNKNOWN;
-  opts->server_max_version = MBEDTLS_SSL_VERSION_UNKNOWN;
+    opts->server_max_version = MBEDTLS_SSL_VERSION_UNKNOWN;
-  opts->expected_negotiated_version = MBEDTLS_SSL_VERSION_TLS1_2;
+    opts->expected_negotiated_version = MBEDTLS_SSL_VERSION_TLS1_2;
-  opts->expected_handshake_result = 0;
+    opts->expected_handshake_result = 0;
-  opts->expected_ciphersuite = 0;
+    opts->expected_ciphersuite = 0;
-  opts->pk_alg = MBEDTLS_PK_RSA;
+    opts->pk_alg = MBEDTLS_PK_RSA;
-  opts->opaque_alg = 0;
+    opts->opaque_alg = 0;
-  opts->opaque_alg2 = 0;
+    opts->opaque_alg2 = 0;
-  opts->opaque_usage = 0;
+    opts->opaque_usage = 0;
-  opts->psk_str = NULL;
+    opts->psk_str = NULL;
-  opts->dtls = 0;
+    opts->dtls = 0;
-  opts->srv_auth_mode = MBEDTLS_SSL_VERIFY_NONE;
+    opts->srv_auth_mode = MBEDTLS_SSL_VERIFY_NONE;
-  opts->serialize = 0;
+    opts->serialize = 0;
-  opts->mfl = MBEDTLS_SSL_MAX_FRAG_LEN_NONE;
+    opts->mfl = MBEDTLS_SSL_MAX_FRAG_LEN_NONE;
-  opts->cli_msg_len = 100;
+    opts->cli_msg_len = 100;
-  opts->srv_msg_len = 100;
+    opts->srv_msg_len = 100;
-  opts->expected_cli_fragments = 1;
+    opts->expected_cli_fragments = 1;
-  opts->expected_srv_fragments = 1;
+    opts->expected_srv_fragments = 1;
-  opts->renegotiate = 0;
+    opts->renegotiate = 0;
-  opts->legacy_renegotiation = MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION;
+    opts->legacy_renegotiation = MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION;
-  opts->srv_log_obj = NULL;
+    opts->srv_log_obj = NULL;
-  opts->srv_log_obj = NULL;
+    opts->srv_log_obj = NULL;
-  opts->srv_log_fun = NULL;
+    opts->srv_log_fun = NULL;
-  opts->cli_log_fun = NULL;
+    opts->cli_log_fun = NULL;
-  opts->resize_buffers = 1;
+    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 mbedtls_endpoint_init( mbedtls_endpoint *ep, int endpoint_type, int pk_alg,
+                           handshake_test_options *options,
-                           int opaque_alg, int opaque_alg2, int opaque_usage,
                            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,
+    ret = mbedtls_endpoint_certificate_init( ep, options->pk_alg,
-                                             opaque_alg2, opaque_usage );
+                                             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, &client_context,
-                                            options->opaque_alg,
-                                            options->opaque_alg2,
-                                            options->opaque_usage,
-                                            &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, NULL, NULL,
-                                            options->opaque_alg,
-                                            options->opaque_alg2,
-                                            options->opaque_usage,
-                                            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, &server_context,
-                                            options->opaque_alg,
-                                            options->opaque_alg2,
-                                            options->opaque_usage,
-                                            &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, NULL, NULL, NULL,
-                                            options->opaque_alg,
+                                            NULL ) == 0 );
-                                            options->opaque_alg2,
-                                            options->opaque_usage,
-                                            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,
+    ret = mbedtls_endpoint_init( NULL, endpoint_type, &options,
-                                 0, 0, 0, NULL, NULL, NULL, NULL );
+                                 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,
+    ret = mbedtls_endpoint_init( &base_ep, endpoint_type, &options,
-                                 0, 0, 0, NULL, NULL, NULL, NULL );
+                                 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,
+                                 &options, NULL, NULL, NULL, NULL );
-                                 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,
+                                        &options, NULL, NULL,
-                                        NULL, NULL, NULL, NULL ), 0 );
+                                        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( );
 }