From f958c05fa7de6dc1f29c8a563ee153c8b7440c1b Mon Sep 17 00:00:00 2001 From: Joachim Bauernberger Date: Sun, 25 Oct 2026 18:26:34 +0100 Subject: [PATCH] Validate the TLS 1.3 server supported_groups extension The TLS 1.3 server accepted an empty named_group_list, and data trailing the list, in a ClientHello supported_groups extension and continued the handshake. RFC 8446 section 4.2.7 defines the list as NamedGroup<2..2^16-1>, so it must hold at least one group and be the whole content of the extension. Reject an empty list and trailing data in ssl_tls13_parse_supported_groups_ext() with a decode_error alert, which is what OpenSSL does. Expose the parser with MBEDTLS_STATIC_TESTABLE and add unit tests. Fixes #10897. Fixes #10733. Signed-off-by: Joachim Bauernberger --- .../tls13-supported-groups-validation.txt | 4 +++ library/ssl_tls13_invasive.h | 7 ++++ library/ssl_tls13_server.c | 27 +++++++++++--- tests/suites/test_suite_ssl.data | 11 ++++++ tests/suites/test_suite_ssl.function | 36 +++++++++++++++++++ 5 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 ChangeLog.d/tls13-supported-groups-validation.txt diff --git a/ChangeLog.d/tls13-supported-groups-validation.txt b/ChangeLog.d/tls13-supported-groups-validation.txt new file mode 100644 index 0000000000..23f42a4270 --- /dev/null +++ b/ChangeLog.d/tls13-supported-groups-validation.txt @@ -0,0 +1,4 @@ +Bugfix + * On the TLS 1.3 server, reject an empty supported_groups list, and + trailing data after the list, in the ClientHello with a decode_error + alert, as required by RFC 8446. Fixes #10897. Fixes #10733. diff --git a/library/ssl_tls13_invasive.h b/library/ssl_tls13_invasive.h index c98902db1d..d042c516f0 100644 --- a/library/ssl_tls13_invasive.h +++ b/library/ssl_tls13_invasive.h @@ -17,6 +17,13 @@ int mbedtls_ssl_tls13_parse_certificate(mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end); +#if defined(MBEDTLS_SSL_SRV_C) && \ + (defined(PSA_WANT_ALG_ECDH) || defined(PSA_WANT_ALG_FFDH)) +int mbedtls_ssl_tls13_parse_supported_groups_ext(mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end); +#endif + #if defined(MBEDTLS_SSL_SRV_C) && \ defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_EPHEMERAL_ENABLED) int mbedtls_ssl_tls13_parse_key_shares_ext(mbedtls_ssl_context *ssl, diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 1f79ab6216..d9f7f2b28b 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -830,9 +830,10 @@ static int ssl_tls13_parse_supported_versions_ext(mbedtls_ssl_context *ssl, * } NamedGroupList; */ MBEDTLS_CHECK_RETURN_CRITICAL -static int ssl_tls13_parse_supported_groups_ext(mbedtls_ssl_context *ssl, - const unsigned char *buf, - const unsigned char *end) +MBEDTLS_STATIC_TESTABLE +int mbedtls_ssl_tls13_parse_supported_groups_ext(mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end) { const unsigned char *p = buf; size_t named_group_list_len; @@ -842,8 +843,24 @@ static int ssl_tls13_parse_supported_groups_ext(mbedtls_ssl_context *ssl, MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 2); named_group_list_len = MBEDTLS_GET_UINT16_BE(p, 0); p += 2; + + /* named_group_list is NamedGroup<2..2^16-1>: at least one group (RFC 8446 4.2.7). */ + if (named_group_list_len < 2) { + MBEDTLS_SSL_DEBUG_MSG(1, ("supported_groups list is empty")); + MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR); + return MBEDTLS_ERR_SSL_DECODE_ERROR; + } MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, named_group_list_len); named_group_list_end = p + named_group_list_len; + + /* named_group_list is the whole extension; no trailing data. */ + if (named_group_list_end != end) { + MBEDTLS_SSL_DEBUG_MSG(1, ("supported_groups extension has trailing data")); + MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR); + return MBEDTLS_ERR_SSL_DECODE_ERROR; + } ssl->handshake->hrr_selected_group = 0; while (p < named_group_list_end) { @@ -1551,11 +1568,11 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, * indicates the named groups which the client supports, * ordered from most preferred to least preferred. */ - ret = ssl_tls13_parse_supported_groups_ext( + ret = mbedtls_ssl_tls13_parse_supported_groups_ext( ssl, p, extension_data_end); if (ret != 0) { MBEDTLS_SSL_DEBUG_RET( - 1, "ssl_tls13_parse_supported_groups_ext", ret); + 1, "mbedtls_ssl_tls13_parse_supported_groups_ext", ret); return ret; } diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 10ef8a8fb5..f56e4ab0ee 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3673,3 +3673,14 @@ tls13_server_parse_key_shares_ext:"0103010000ff111111111111111111111111111111111 TLS 1.3 srv parse key_shares: trailing data after client_shares tls13_server_parse_key_shares_ext:"0024001d00201111111111111111111111111111111111111111111111111111111111111111ff":MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR +TLS 1.3 srv parse supported_groups: valid +tls13_server_parse_supported_groups_ext:"0004001d0017":0:-1 + +TLS 1.3 srv parse supported_groups: empty list +tls13_server_parse_supported_groups_ext:"0000":MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR + +TLS 1.3 srv parse supported_groups: trailing data +tls13_server_parse_supported_groups_ext:"0004001d0017ff":MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR + +TLS 1.3 srv parse supported_groups: odd length +tls13_server_parse_supported_groups_ext:"0003001d00":MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 6dbeb9e9c2..51174cc04f 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4608,6 +4608,42 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_PROTO_TLS1_3 */ +void tls13_server_parse_supported_groups_ext(data_t *ext, int expected_result, + int expected_alert) +{ + mbedtls_ssl_context ssl; + mbedtls_ssl_config conf; + int ret; + + mbedtls_ssl_init(&ssl); + mbedtls_ssl_config_init(&conf); + MD_OR_USE_PSA_INIT(); + + TEST_EQUAL(mbedtls_ssl_config_defaults(&conf, MBEDTLS_SSL_IS_SERVER, + MBEDTLS_SSL_TRANSPORT_STREAM, + MBEDTLS_SSL_PRESET_DEFAULT), 0); + TEST_EQUAL(mbedtls_ssl_setup(&ssl, &conf), 0); + + ret = mbedtls_ssl_tls13_parse_supported_groups_ext(&ssl, ext->x, + ext->x + ext->len); + TEST_EQUAL(ret, expected_result); + + if (expected_alert >= 0) { + TEST_EQUAL(ssl.MBEDTLS_PRIVATE(send_alert), 1); + TEST_EQUAL(ssl.MBEDTLS_PRIVATE(alert_type), + (unsigned char) expected_alert); + } else { + TEST_EQUAL(ssl.MBEDTLS_PRIVATE(send_alert), 0); + } + +exit: + mbedtls_ssl_free(&ssl); + mbedtls_ssl_config_free(&conf); + MD_OR_USE_PSA_DONE(); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_TEST_AT_LEAST_ONE_TLS1_3_CIPHERSUITE:MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED:PSA_WANT_ALG_SHA_256:PSA_WANT_ALG_ECDH:PSA_WANT_ALG_ECDSA_ANY:PSA_WANT_ECC_SECP_R1_256:PSA_WANT_ECC_SECP_R1_384 */ void reject_hrr_selecting_unoffered_group(void) {