From a3d831b9e67cb8521073dfe7f15b07c53f9294c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 23 May 2019 12:28:45 +0200 Subject: [PATCH] Add test for session_load() from small buffers This uncovered a bug that led to a double-free (in practice, in general could be free() on any invalid value): initially the session structure is loaded with `memcpy()` which copies the previous values of pointers peer_cert and ticket to heap-allocated buffers (or any other value if the input is attacker-controlled). Now if we exit before we got a chance to replace those invalid values with valid ones (for example because the input buffer is too small, or because the second malloc() failed), then the next call to session_free() is going to call free() on invalid pointers. This bug is fixed in this commit by always setting the pointers to NULL right after they've been read from the serialised state, so that the invalid values can never be used. (An alternative would be to NULL-ify them when writing, which was rejected mostly because we need to do it when reading anyway (as the consequences of free(invalid) are too severe to take any risk), so doing it when writing as well is redundant and a waste of code size.) Also, while thinking about what happens in case of errors, it became apparent to me that it was bad practice to leave the session structure in an half-initialised state and rely on the caller to call session_free(), so this commit also ensures we always clear the structure when loading failed. --- library/ssl_tls.c | 26 ++++++++++++++--- tests/suites/test_suite_ssl.data | 23 +++++++++++++++ tests/suites/test_suite_ssl.function | 42 ++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6f9203daa4..cc70510cb8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -9959,11 +9959,14 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, } /* - * Unserialise session, see ssl_save_session() + * 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; @@ -10091,6 +10094,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 b166a9a93a..d41fcd01dc 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -8798,3 +8798,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 61ff416081..8a184d0f87 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -748,3 +748,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 */