diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2d7cd18d8..56bdcfeb5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8862,11 +8862,14 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, } /* - * Unserialise session, see mbedtls_ssl_session_save() + * Unserialise session, see mbedtls_ssl_session_save(). + * + * This internal version is wrapped by a public function that cleans up in + * case of error. */ -int mbedtls_ssl_session_load( mbedtls_ssl_session *session, - const unsigned char *buf, - size_t len ) +static int ssl_session_load( mbedtls_ssl_session *session, + const unsigned char *buf, + size_t len ) { const unsigned char *p = buf; const unsigned char * const end = buf + len; @@ -8883,6 +8886,15 @@ int mbedtls_ssl_session_load( mbedtls_ssl_session *session, memcpy( session, p, sizeof( mbedtls_ssl_session ) ); p += sizeof( mbedtls_ssl_session ); + /* Immediately clear invalid pointer values that have been read, in case + * we exit early before we replaced them with valid ones. */ +#if defined(MBEDTLS_X509_CRT_PARSE_C) + session->peer_cert = NULL; +#endif +#if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) + session->ticket = NULL; +#endif + /* * Peer certificate */ @@ -8955,6 +8967,21 @@ int mbedtls_ssl_session_load( mbedtls_ssl_session *session, return( 0 ); } +/* + * Unserialise session: public wrapper for error cleaning + */ +int mbedtls_ssl_session_load( mbedtls_ssl_session *session, + const unsigned char *buf, + size_t len ) +{ + int ret = ssl_session_load( session, buf, len ); + + if( ret != 0 ) + mbedtls_ssl_session_free( session ); + + return( ret ); +} + /* * Perform a single step of the SSL handshake */ diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 45e7d440e..7a39626ea 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -8759,3 +8759,26 @@ ssl_serialise_session_save_buf_size:42:"data_files/server5.crt" Session serialisation, save buffer size: large ticket, cert depends_on:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C:MBEDTLS_X509_USE_C:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C ssl_serialise_session_save_buf_size:1023:"data_files/server5.crt" + +Session serialisation, load buffer size: no ticket, no cert +ssl_serialise_session_load_buf_size:0:"" + +Session serialisation, load buffer size: small ticket, no cert +depends_on:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C +ssl_serialise_session_load_buf_size:42:"" + +Session serialisation, load buffer size: large ticket, no cert +depends_on:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C +ssl_serialise_session_load_buf_size:1023:"" + +Session serialisation, load buffer size: no ticket, cert +depends_on:MBEDTLS_X509_USE_C:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C +ssl_serialise_session_load_buf_size:0:"data_files/server5.crt" + +Session serialisation, load buffer size: small ticket, cert +depends_on:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C:MBEDTLS_X509_USE_C:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C +ssl_serialise_session_load_buf_size:42:"data_files/server5.crt" + +Session serialisation, load buffer size: large ticket, cert +depends_on:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C:MBEDTLS_X509_USE_C:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C +ssl_serialise_session_load_buf_size:1023:"data_files/server5.crt" diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index b08f5dc35..8e8d8bd5d 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -718,3 +718,45 @@ exit: mbedtls_free( buf ); } /* END_CASE */ + +/* BEGIN_CASE */ +void ssl_serialise_session_load_buf_size( int ticket_len, char *crt_file ) +{ + mbedtls_ssl_session session; + unsigned char *good_buf = NULL, *bad_buf = NULL; + size_t good_len, bad_len; + + /* + * Test that session_load() fails cleanly on small buffers + */ + + mbedtls_ssl_session_init( &session ); + + /* Prepare serialised session data */ + ssl_populate_session( &session, ticket_len, crt_file ); + TEST_ASSERT( mbedtls_ssl_session_save( &session, NULL, 0, &good_len ) + == MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); + TEST_ASSERT( ( good_buf = mbedtls_calloc( 1, good_len ) ) != NULL ); + TEST_ASSERT( mbedtls_ssl_session_save( &session, good_buf, good_len, + &good_len ) == 0 ); + mbedtls_ssl_session_free( &session ); + + /* Try all possible bad lengths */ + for( bad_len = 0; bad_len < good_len; bad_len++ ) + { + /* Allocate exact size so that asan/valgrind can detect any overread */ + mbedtls_free( bad_buf ); + bad_buf = mbedtls_calloc( 1, bad_len ? bad_len : 1 ); + TEST_ASSERT( bad_buf != NULL ); + memcpy( bad_buf, good_buf, bad_len ); + + TEST_ASSERT( mbedtls_ssl_session_load( &session, bad_buf, bad_len ) + == MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + +exit: + mbedtls_ssl_session_free( &session ); + mbedtls_free( good_buf ); + mbedtls_free( bad_buf ); +} +/* END_CASE */