Merge pull request #939 from ronald-cron-arm/tls13-add-missing-overread-check
TLS 1.3: Add missing overread check
diff --git a/ChangeLog.d/tls13-add-missing-overread-check.txt b/ChangeLog.d/tls13-add-missing-overread-check.txt
new file mode 100644
index 0000000..4552cd7
--- /dev/null
+++ b/ChangeLog.d/tls13-add-missing-overread-check.txt
@@ -0,0 +1,8 @@
+Security
+ * Fix a buffer overread in TLS 1.3 Certificate parsing. An unauthenticated
+ client or server could cause an MbedTLS server or client to overread up
+ to 64 kBytes of data and potentially overread the input buffer by that
+ amount minus the size of the input buffer. As overread data undergoes
+ various checks, the likelihood of reaching the boundary of the input
+ buffer is rather small but increases as its size
+ MBEDTLS_SSL_IN_CONTENT_LEN decreases.
diff --git a/library/ssl_misc.h b/library/ssl_misc.h
index 1280241..c17fa20 100644
--- a/library/ssl_misc.h
+++ b/library/ssl_misc.h
@@ -381,11 +381,36 @@
* \return Zero if the needed space is available in the buffer, non-zero
* otherwise.
*/
+#if ! defined(MBEDTLS_TEST_HOOKS)
static inline int mbedtls_ssl_chk_buf_ptr( const uint8_t *cur,
const uint8_t *end, size_t need )
{
return( ( cur > end ) || ( need > (size_t)( end - cur ) ) );
}
+#else
+typedef struct
+{
+ const uint8_t *cur;
+ const uint8_t *end;
+ size_t need;
+} mbedtls_ssl_chk_buf_ptr_args;
+
+void mbedtls_ssl_set_chk_buf_ptr_fail_args(
+ const uint8_t *cur, const uint8_t *end, size_t need );
+void mbedtls_ssl_reset_chk_buf_ptr_fail_args( void );
+int mbedtls_ssl_cmp_chk_buf_ptr_fail_args( mbedtls_ssl_chk_buf_ptr_args *args );
+
+static inline int mbedtls_ssl_chk_buf_ptr( const uint8_t *cur,
+ const uint8_t *end, size_t need )
+{
+ if( ( cur > end ) || ( need > (size_t)( end - cur ) ) )
+ {
+ mbedtls_ssl_set_chk_buf_ptr_fail_args( cur, end, need );
+ return( 1 );
+ }
+ return( 0 );
+}
+#endif /* MBEDTLS_TEST_HOOKS */
/**
* \brief This macro checks if the remaining size in a buffer is
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 668b5ec..55b7f85 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -58,6 +58,30 @@
#include "mbedtls/oid.h"
#endif
+#if defined(MBEDTLS_TEST_HOOKS)
+static mbedtls_ssl_chk_buf_ptr_args chk_buf_ptr_fail_args;
+
+void mbedtls_ssl_set_chk_buf_ptr_fail_args(
+ const uint8_t *cur, const uint8_t *end, size_t need )
+{
+ chk_buf_ptr_fail_args.cur = cur;
+ chk_buf_ptr_fail_args.end = end;
+ chk_buf_ptr_fail_args.need = need;
+}
+
+void mbedtls_ssl_reset_chk_buf_ptr_fail_args( void )
+{
+ memset( &chk_buf_ptr_fail_args, 0, sizeof( chk_buf_ptr_fail_args ) );
+}
+
+int mbedtls_ssl_cmp_chk_buf_ptr_fail_args( mbedtls_ssl_chk_buf_ptr_args *args )
+{
+ return( ( chk_buf_ptr_fail_args.cur != args->cur ) ||
+ ( chk_buf_ptr_fail_args.end != args->end ) ||
+ ( chk_buf_ptr_fail_args.need != args->need ) );
+}
+#endif /* MBEDTLS_TEST_HOOKS */
+
#if defined(MBEDTLS_SSL_PROTO_DTLS)
#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID)
@@ -1103,6 +1127,8 @@
memset( ssl->in_buf, 0, in_buf_len );
}
+ ssl->send_alert = 0;
+
/* Reset outgoing message writing */
ssl->out_msgtype = 0;
ssl->out_msglen = 0;
diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c
index c7e00a9..7d0559b 100644
--- a/library/ssl_tls13_generic.c
+++ b/library/ssl_tls13_generic.c
@@ -31,6 +31,7 @@
#include <string.h>
#include "ssl_misc.h"
+#include "ssl_tls13_invasive.h"
#include "ssl_tls13_keys.h"
#include "ssl_debug_helpers.h"
@@ -391,9 +392,10 @@
/* Parse certificate chain send by the server. */
MBEDTLS_CHECK_RETURN_CRITICAL
-static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl,
- const unsigned char *buf,
- const unsigned char *end )
+MBEDTLS_STATIC_TESTABLE
+int mbedtls_ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl,
+ const unsigned char *buf,
+ const unsigned char *end )
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
size_t certificate_request_context_len = 0;
@@ -444,6 +446,7 @@
mbedtls_x509_crt_init( ssl->session_negotiate->peer_cert );
+ MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, certificate_list_len );
certificate_list_end = p + certificate_list_len;
while( p < certificate_list_end )
{
@@ -524,9 +527,10 @@
}
#else
MBEDTLS_CHECK_RETURN_CRITICAL
-static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl,
- const unsigned char *buf,
- const unsigned char *end )
+MBEDTLS_STATIC_TESTABLE
+int mbedtls_ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl,
+ const unsigned char *buf,
+ const unsigned char *end )
{
((void) ssl);
((void) buf);
@@ -657,7 +661,8 @@
* with details encoded in the verification flags. All other kinds
* of error codes, including those from the user provided f_vrfy
* functions, are treated as fatal and lead to a failure of
- * ssl_tls13_parse_certificate even if verification was optional. */
+ * mbedtls_ssl_tls13_parse_certificate even if verification was optional.
+ */
if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL &&
( ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED ||
ret == MBEDTLS_ERR_SSL_BAD_CERTIFICATE ) )
@@ -735,8 +740,8 @@
&buf, &buf_len ) );
/* Parse the certificate chain sent by the peer. */
- MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_certificate( ssl, buf,
- buf + buf_len ) );
+ MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_parse_certificate( ssl, buf,
+ buf + buf_len ) );
/* Validate the certificate chain and set the verification results. */
MBEDTLS_SSL_PROC_CHK( ssl_tls13_validate_certificate( ssl ) );
diff --git a/library/ssl_tls13_invasive.h b/library/ssl_tls13_invasive.h
index 4e39f90..55fc958 100644
--- a/library/ssl_tls13_invasive.h
+++ b/library/ssl_tls13_invasive.h
@@ -80,6 +80,10 @@
const unsigned char *info, size_t info_len,
unsigned char *okm, size_t okm_len );
+MBEDTLS_CHECK_RETURN_CRITICAL
+int mbedtls_ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl,
+ const unsigned char *buf,
+ const unsigned char *end );
#endif /* MBEDTLS_TEST_HOOKS */
#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */
diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh
index 6144c2f..27c211f 100755
--- a/tests/scripts/all.sh
+++ b/tests/scripts/all.sh
@@ -2904,6 +2904,18 @@
if_build_succeeded tests/ssl-opt.sh
}
+component_test_tls13_only_with_hooks () {
+ msg "build: default config with MBEDTLS_SSL_PROTO_TLS1_3 and MBEDTLS_TEST_HOOKS, without MBEDTLS_SSL_PROTO_TLS1_2"
+ scripts/config.py set MBEDTLS_TEST_HOOKS
+ make CFLAGS="'-DMBEDTLS_USER_CONFIG_FILE=\"../tests/configs/tls13-only.h\"'"
+
+ msg "test: default config with MBEDTLS_SSL_PROTO_TLS1_3 enabled, without MBEDTLS_SSL_PROTO_TLS1_2"
+ if_build_succeeded make test
+
+ msg "ssl-opt.sh (TLS 1.3)"
+ if_build_succeeded tests/ssl-opt.sh
+}
+
component_test_tls13 () {
msg "build: default config with MBEDTLS_SSL_PROTO_TLS1_3 enabled, without padding"
scripts/config.py set MBEDTLS_SSL_PROTO_TLS1_3
diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data
index e82c66e..8ce81d0 100644
--- a/tests/suites/test_suite_ssl.data
+++ b/tests/suites/test_suite_ssl.data
@@ -3381,3 +3381,6 @@
Cookie parsing: one byte overread
cookie_parsing:"16fefd0000000000000000002F010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d0001":MBEDTLS_ERR_SSL_DECODE_ERROR
+
+TLS 1.3 srv Certificate msg - wrong vector lengths
+tls13_server_certificate_msg_invalid_vector_len
diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function
index 2ab2aaa..59c69c6 100644
--- a/tests/suites/test_suite_ssl.function
+++ b/tests/suites/test_suite_ssl.function
@@ -105,6 +105,23 @@
opts->cli_log_fun = NULL;
opts->resize_buffers = 1;
}
+
+#if defined(MBEDTLS_TEST_HOOKS)
+static void set_chk_buf_ptr_args(
+ mbedtls_ssl_chk_buf_ptr_args *args,
+ unsigned char *cur, unsigned char *end, size_t need )
+{
+ args->cur = cur;
+ args->end = end;
+ args->need = need;
+}
+
+static void reset_chk_buf_ptr_args( mbedtls_ssl_chk_buf_ptr_args *args )
+{
+ memset( args, 0, sizeof( *args ) );
+}
+#endif /* MBEDTLS_TEST_HOOKS */
+
/*
* Buffer structure for custom I/O callbacks.
*/
@@ -2307,6 +2324,119 @@
}
#endif /* MBEDTLS_X509_CRT_PARSE_C && MBEDTLS_ENTROPY_C && MBEDTLS_CTR_DRBG_C */
+#if defined(MBEDTLS_TEST_HOOKS)
+/*
+ * Tweak vector lengths in a TLS 1.3 Certificate message
+ *
+ * \param[in] buf Buffer containing the Certificate message to tweak
+ * \param[in]]out] end End of the buffer to parse
+ * \param tweak Tweak identifier (from 1 to the number of tweaks).
+ * \param[out] expected_result Error code expected from the parsing function
+ * \param[out] args Arguments of the MBEDTLS_SSL_CHK_BUF_READ_PTR call that
+ * is expected to fail. All zeroes if no
+ * MBEDTLS_SSL_CHK_BUF_READ_PTR failure is expected.
+ */
+int tweak_tls13_certificate_msg_vector_len(
+ unsigned char *buf, unsigned char **end, int tweak,
+ int *expected_result, mbedtls_ssl_chk_buf_ptr_args *args )
+{
+/*
+ * The definition of the tweaks assume that the certificate list contains only
+ * one certificate.
+ */
+
+/*
+ * struct {
+ * opaque cert_data<1..2^24-1>;
+ * Extension extensions<0..2^16-1>;
+ * } CertificateEntry;
+ *
+ * struct {
+ * opaque certificate_request_context<0..2^8-1>;
+ * CertificateEntry certificate_list<0..2^24-1>;
+ * } Certificate;
+ */
+ unsigned char *p_certificate_request_context_len = buf;
+ size_t certificate_request_context_len = buf[0];
+
+ unsigned char *p_certificate_list_len = buf + 1 + certificate_request_context_len;
+ unsigned char *certificate_list = p_certificate_list_len + 3;
+ size_t certificate_list_len = MBEDTLS_GET_UINT24_BE( p_certificate_list_len, 0 );
+
+ unsigned char *p_cert_data_len = certificate_list;
+ unsigned char *cert_data = p_cert_data_len + 3;
+ size_t cert_data_len = MBEDTLS_GET_UINT24_BE( p_cert_data_len, 0 );
+
+ unsigned char *p_extensions_len = cert_data + cert_data_len;
+ unsigned char *extensions = p_extensions_len + 2;
+ size_t extensions_len = MBEDTLS_GET_UINT16_BE( p_extensions_len, 0 );
+
+ *expected_result = MBEDTLS_ERR_SSL_DECODE_ERROR;
+
+ switch( tweak )
+ {
+ case 1:
+ /* Failure when checking if the certificate request context length and
+ * certificate list length can be read
+ */
+ *end = buf + 3;
+ set_chk_buf_ptr_args( args, buf, *end, 4 );
+ break;
+
+ case 2:
+ /* Invalid certificate request context length.
+ */
+ *p_certificate_request_context_len =
+ certificate_request_context_len + 1;
+ reset_chk_buf_ptr_args( args );
+ break;
+
+ case 3:
+ /* Failure when checking if certificate_list data can be read. */
+ MBEDTLS_PUT_UINT24_BE( certificate_list_len + 1,
+ p_certificate_list_len, 0 );
+ set_chk_buf_ptr_args( args, certificate_list, *end,
+ certificate_list_len + 1 );
+ break;
+
+ case 4:
+ /* Failure when checking if the cert_data length can be read. */
+ MBEDTLS_PUT_UINT24_BE( 2, p_certificate_list_len, 0 );
+ set_chk_buf_ptr_args( args, p_cert_data_len, certificate_list + 2, 3 );
+ break;
+
+ case 5:
+ /* Failure when checking if cert_data data can be read. */
+ MBEDTLS_PUT_UINT24_BE( certificate_list_len - 3 + 1,
+ p_cert_data_len, 0 );
+ set_chk_buf_ptr_args( args, cert_data,
+ certificate_list + certificate_list_len,
+ certificate_list_len - 3 + 1 );
+ break;
+
+ case 6:
+ /* Failure when checking if the extensions length can be read. */
+ MBEDTLS_PUT_UINT24_BE( certificate_list_len - extensions_len - 1,
+ p_certificate_list_len, 0 );
+ set_chk_buf_ptr_args( args, p_extensions_len,
+ certificate_list + certificate_list_len - extensions_len - 1, 2 );
+ break;
+
+ case 7:
+ /* Failure when checking if extensions data can be read. */
+ MBEDTLS_PUT_UINT16_BE( extensions_len + 1, p_extensions_len, 0 );
+
+ set_chk_buf_ptr_args( args, extensions,
+ certificate_list + certificate_list_len, extensions_len + 1 );
+ break;
+
+ default:
+ return( -1 );
+ }
+
+ return( 0 );
+}
+#endif /* MBEDTLS_TEST_HOOKS */
/* END_HEADER */
/* BEGIN_DEPENDENCIES
@@ -5708,3 +5838,87 @@
USE_PSA_DONE( );
}
/* END_CASE */
+/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS:MBEDTLS_SSL_PROTO_TLS1_3:!MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_X509_CRT_PARSE_C:MBEDTLS_ECP_DP_SECP384R1_ENABLED */
+void tls13_server_certificate_msg_invalid_vector_len( )
+{
+ int ret = -1;
+ mbedtls_endpoint client_ep, server_ep;
+ unsigned char *buf, *end;
+ size_t buf_len;
+ int step = 0;
+ int expected_result;
+ mbedtls_ssl_chk_buf_ptr_args expected_chk_buf_ptr_args;
+
+ /*
+ * Test set-up
+ */
+ USE_PSA_INIT( );
+
+ ret = mbedtls_endpoint_init( &client_ep, MBEDTLS_SSL_IS_CLIENT,
+ MBEDTLS_PK_ECDSA, NULL, NULL, NULL, NULL );
+ TEST_EQUAL( ret, 0 );
+
+ ret = mbedtls_endpoint_init( &server_ep, MBEDTLS_SSL_IS_SERVER,
+ MBEDTLS_PK_ECDSA, NULL, NULL, NULL, NULL );
+ TEST_EQUAL( ret, 0 );
+
+ ret = mbedtls_mock_socket_connect( &(client_ep.socket),
+ &(server_ep.socket), 1024 );
+ TEST_EQUAL( ret, 0 );
+
+ while( 1 )
+ {
+ mbedtls_test_set_step( ++step );
+
+ ret = mbedtls_move_handshake_to_state( &(server_ep.ssl),
+ &(client_ep.ssl),
+ MBEDTLS_SSL_CERTIFICATE_VERIFY );
+ TEST_EQUAL( ret, 0 );
+
+ ret = mbedtls_ssl_flush_output( &(server_ep.ssl) );
+ TEST_EQUAL( ret, 0 );
+
+ ret = mbedtls_move_handshake_to_state( &(client_ep.ssl),
+ &(server_ep.ssl),
+ MBEDTLS_SSL_SERVER_CERTIFICATE );
+ TEST_EQUAL( ret, 0 );
+
+ ret = mbedtls_ssl_tls13_fetch_handshake_msg( &(client_ep.ssl),
+ MBEDTLS_SSL_HS_CERTIFICATE,
+ &buf, &buf_len );
+ TEST_EQUAL( ret, 0 );
+
+ end = buf + buf_len;
+
+ /*
+ * Tweak server Certificate message and parse it.
+ */
+
+ ret = tweak_tls13_certificate_msg_vector_len(
+ buf, &end, step, &expected_result, &expected_chk_buf_ptr_args );
+
+ if( ret != 0 )
+ break;
+
+ ret = mbedtls_ssl_tls13_parse_certificate( &(client_ep.ssl), buf, end );
+ TEST_EQUAL( ret, expected_result );
+
+ TEST_ASSERT( mbedtls_ssl_cmp_chk_buf_ptr_fail_args(
+ &expected_chk_buf_ptr_args ) == 0 );
+
+ mbedtls_ssl_reset_chk_buf_ptr_fail_args( );
+
+ ret = mbedtls_ssl_session_reset( &(client_ep.ssl) );
+ TEST_EQUAL( ret, 0 );
+
+ ret = mbedtls_ssl_session_reset( &(server_ep.ssl) );
+ TEST_EQUAL( ret, 0 );
+ }
+
+exit:
+ mbedtls_ssl_reset_chk_buf_ptr_fail_args( );
+ mbedtls_endpoint_free( &client_ep, NULL );
+ mbedtls_endpoint_free( &server_ep, NULL );
+ USE_PSA_DONE( );
+}
+/* END_CASE */
