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.
diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function
index 61ff416..8a184d0 100644
--- a/tests/suites/test_suite_ssl.function
+++ b/tests/suites/test_suite_ssl.function
@@ -748,3 +748,45 @@
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 */