From 8134d60565d244d019d77c1324e3dc0ac76e1d80 Mon Sep 17 00:00:00 2001 From: Joachim Bauernberger Date: Sun, 25 Oct 2026 14:46:48 +0100 Subject: [PATCH] Reject malformed server_name extensions in ClientHello mbedtls_ssl_parse_server_name_ext() used the first host_name entry and returned without looking at the rest of the extension. It accepted an empty server_name_list, a list that does not fill the extension, an empty host_name, a second host_name entry (forbidden by RFC 6066, section 3) and a malformed entry following the host_name, and it passed host names containing a null byte to the ServerName callback. Validate the whole list first, then record the host name and call the ServerName callback. Structural errors are rejected with a decode_error alert, a null byte in the host name with an unrecognized_name alert. Entries of unknown name types are still skipped, as RFC 6066 allows. Signed-off-by: Joachim Bauernberger --- ChangeLog.d/sni-parse-strict.txt | 9 +++ include/mbedtls/ssl.h | 5 ++ library/ssl_tls.c | 82 +++++++++++++++++-------- tests/suites/test_suite_ssl.data | 45 ++++++++++++++ tests/suites/test_suite_ssl.function | 91 ++++++++++++++++++++++++++++ 5 files changed, 208 insertions(+), 24 deletions(-) create mode 100644 ChangeLog.d/sni-parse-strict.txt diff --git a/ChangeLog.d/sni-parse-strict.txt b/ChangeLog.d/sni-parse-strict.txt new file mode 100644 index 0000000000..f42aed4320 --- /dev/null +++ b/ChangeLog.d/sni-parse-strict.txt @@ -0,0 +1,9 @@ +Bugfix + * The TLS server now rejects a ClientHello whose server_name extension + is malformed: an empty server_name_list, a list that does not fill the + extension, an empty host_name, more than one host_name entry (forbidden + by RFC 6066), or an entry after the host_name that overruns the list. + Previously the first host_name was used and the rest of the extension + was not checked. These are rejected with a decode_error alert. A + host_name containing a null byte is rejected with an unrecognized_name + alert instead of being passed to the ServerName callback. diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 00e28442d6..8b26b2c225 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -4083,6 +4083,11 @@ void mbedtls_ssl_set_hs_authmode(mbedtls_ssl_context *ssl, * then must return 0. If no matching name is found, the * callback may return non-zero to abort the handshake. * + * \note The callback is only called once the whole ServerName + * extension has been validated. \c hostname is not + * null-terminated; it is at least one byte long and does + * not contain any null byte. + * * \param conf SSL configuration * \param f_sni verification function * \param p_sni verification parameter diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 1f44fc71a7..01b136cb9a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8372,6 +8372,13 @@ int mbedtls_ssl_write_sig_alg_ext(mbedtls_ssl_context *ssl, unsigned char *buf, * struct { * ServerName server_name_list<1..2^16-1> * } ServerNameList; + * + * RFC 6066, section 3: "The ServerNameList MUST NOT contain more than one + * name of the same name_type." Entries of unknown name types begin with a + * 16-bit length field and are skipped. + * + * The whole list is validated before the ServerName callback is called, so + * that the callback never acts on a malformed extension. */ MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_parse_server_name_ext(mbedtls_ssl_context *ssl, @@ -8380,8 +8387,9 @@ int mbedtls_ssl_parse_server_name_ext(mbedtls_ssl_context *ssl, { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; const unsigned char *p = buf; - size_t server_name_list_len, hostname_len; - const unsigned char *server_name_list_end; + size_t server_name_list_len, name_len; + const unsigned char *hostname = NULL; + size_t hostname_len = 0; MBEDTLS_SSL_DEBUG_MSG(3, ("parse ServerName extension")); @@ -8389,37 +8397,63 @@ int mbedtls_ssl_parse_server_name_ext(mbedtls_ssl_context *ssl, server_name_list_len = MBEDTLS_GET_UINT16_BE(p, 0); p += 2; - MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, server_name_list_len); - server_name_list_end = p + server_name_list_len; - while (p < server_name_list_end) { - MBEDTLS_SSL_CHK_BUF_READ_PTR(p, server_name_list_end, 3); - hostname_len = MBEDTLS_GET_UINT16_BE(p, 1); - MBEDTLS_SSL_CHK_BUF_READ_PTR(p, server_name_list_end, - hostname_len + 3); + /* The list must not be empty and must fill the extension exactly. */ + if (server_name_list_len == 0 || + server_name_list_len != (size_t) (end - p)) { + MBEDTLS_SSL_DEBUG_MSG(1, ("bad server_name list length")); + MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR); + return MBEDTLS_ERR_SSL_DECODE_ERROR; + } + + while (p < end) { + MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 3); + name_len = MBEDTLS_GET_UINT16_BE(p, 1); + MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, name_len + 3); if (p[0] == MBEDTLS_TLS_EXT_SERVERNAME_HOSTNAME) { - /* sni_name is intended to be used only during the parsing of the - * ClientHello message (it is reset to NULL before the end of - * the message parsing). Thus it is ok to just point to the - * reception buffer and not make a copy of it. - */ - ssl->handshake->sni_name = p + 3; - ssl->handshake->sni_name_len = hostname_len; - if (ssl->conf->f_sni == NULL) { - return 0; + if (hostname != NULL || name_len == 0) { + MBEDTLS_SSL_DEBUG_MSG(1, ("bad server_name host_name entry")); + MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR); + return MBEDTLS_ERR_SSL_DECODE_ERROR; } - ret = ssl->conf->f_sni(ssl->conf->p_sni, - ssl, p + 3, hostname_len); - if (ret != 0) { - MBEDTLS_SSL_DEBUG_RET(1, "ssl_sni_wrapper", ret); + /* A host name never contains a null byte. Reject it rather than + * pass a name to the callback that a C string comparison would + * truncate. */ + if (memchr(p + 3, '\0', name_len) != NULL) { + MBEDTLS_SSL_DEBUG_MSG(1, ("null byte in server_name host_name")); MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_UNRECOGNIZED_NAME, MBEDTLS_ERR_SSL_UNRECOGNIZED_NAME); return MBEDTLS_ERR_SSL_UNRECOGNIZED_NAME; } - return 0; + hostname = p + 3; + hostname_len = name_len; } - p += hostname_len + 3; + p += name_len + 3; + } + + if (hostname == NULL) { + return 0; + } + + /* sni_name is intended to be used only during the parsing of the + * ClientHello message (it is reset to NULL before the end of + * the message parsing). Thus it is ok to just point to the + * reception buffer and not make a copy of it. + */ + ssl->handshake->sni_name = hostname; + ssl->handshake->sni_name_len = hostname_len; + if (ssl->conf->f_sni == NULL) { + return 0; + } + ret = ssl->conf->f_sni(ssl->conf->p_sni, ssl, hostname, hostname_len); + if (ret != 0) { + MBEDTLS_SSL_DEBUG_RET(1, "ssl_sni_wrapper", ret); + MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_UNRECOGNIZED_NAME, + MBEDTLS_ERR_SSL_UNRECOGNIZED_NAME); + return MBEDTLS_ERR_SSL_UNRECOGNIZED_NAME; } return 0; diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 0a9784fcaa..19381d6bb0 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3650,3 +3650,48 @@ tls13_record_boundary_alignment:MBEDTLS_SSL_IS_SERVER:MBEDTLS_SSL_HS_FINISHED TLS 1.3 EndOfEarlyData record boundary alignment enforcement depends_on:MBEDTLS_SSL_EARLY_DATA:MBEDTLS_SSL_SESSION_TICKETS tls13_record_boundary_alignment:MBEDTLS_SSL_IS_SERVER:MBEDTLS_SSL_HS_END_OF_EARLY_DATA + +Parse server_name: single host_name +ssl_parse_server_name_ext:"000e00000b6578616d706c652e636f6d":TEST_SNI_CB_NONE:0:0:"6578616d706c652e636f6d" + +Parse server_name: single host_name, callback accepts +ssl_parse_server_name_ext:"000e00000b6578616d706c652e636f6d":TEST_SNI_CB_ACCEPT:0:0:"6578616d706c652e636f6d" + +Parse server_name: single host_name, callback rejects +ssl_parse_server_name_ext:"000e00000b6578616d706c652e636f6d":TEST_SNI_CB_REJECT:MBEDTLS_ERR_SSL_UNRECOGNIZED_NAME:MBEDTLS_SSL_ALERT_MSG_UNRECOGNIZED_NAME:"6578616d706c652e636f6d" + +Parse server_name: unknown name type skipped before host_name +ssl_parse_server_name_ext:"001401000361626300000b6578616d706c652e636f6d":TEST_SNI_CB_ACCEPT:0:0:"6578616d706c652e636f6d" + +Parse server_name: unknown name type only +ssl_parse_server_name_ext:"0006010003616263":TEST_SNI_CB_ACCEPT:0:0:"" + +Parse server_name: truncated list length +ssl_parse_server_name_ext:"00":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: empty server_name_list +ssl_parse_server_name_ext:"0000":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: trailing data after server_name_list +ssl_parse_server_name_ext:"000e00000b6578616d706c652e636f6dff":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: server_name_list longer than extension +ssl_parse_server_name_ext:"000f00000b6578616d706c652e636f6d":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: truncated entry header +ssl_parse_server_name_ext:"00020000":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: host_name longer than server_name_list +ssl_parse_server_name_ext:"0003000005":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: empty host_name +ssl_parse_server_name_ext:"0003000000":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: two host_name entries +ssl_parse_server_name_ext:"001c00000b6578616d706c652e636f6d00000b6578616d706c652e6e6574":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: malformed entry after host_name +ssl_parse_server_name_ext:"001300000b6578616d706c652e636f6d0100056162":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_DECODE_ERROR:MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR:"" + +Parse server_name: null byte in host_name +ssl_parse_server_name_ext:"000e00000b6578616d706c6500636f6d":TEST_SNI_CB_ACCEPT:MBEDTLS_ERR_SSL_UNRECOGNIZED_NAME:MBEDTLS_SSL_ALERT_MSG_UNRECOGNIZED_NAME:"" diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 622ecd1f73..db4d7812ee 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -607,6 +607,36 @@ static void tls_tweak_in_msglen(void *ctx, int level, } #endif +/* Modes of the ServerName callback in ssl_parse_server_name_ext. These are + * used as expression parameters in test_suite_ssl.data, which the test + * framework's get_expression() evaluates regardless of a test case's + * dependencies, so they must be defined whenever the suite is built, not + * only under SNI && SRV_C. */ +#define TEST_SNI_CB_NONE 0 +#define TEST_SNI_CB_ACCEPT 1 +#define TEST_SNI_CB_REJECT 2 + +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_SRV_C) +typedef struct { + int ret; + int calls; + const unsigned char *name; + size_t name_len; +} test_sni_cb_ctx_t; + +static int test_sni_cb(void *p_ctx, mbedtls_ssl_context *ssl, + const unsigned char *name, size_t name_len) +{ + test_sni_cb_ctx_t *ctx = (test_sni_cb_ctx_t *) p_ctx; + + (void) ssl; + ctx->calls++; + ctx->name = name; + ctx->name_len = name_len; + return ctx->ret; +} +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_SRV_C */ + /* END_HEADER */ @@ -7488,3 +7518,64 @@ exit: PSA_DONE(); } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SERVER_NAME_INDICATION:MBEDTLS_SSL_SRV_C */ +void ssl_parse_server_name_ext(data_t *ext_data, int sni_cb_mode, + int expected_ret, int expected_alert, + data_t *expected_hostname) +{ + mbedtls_ssl_context ssl; + mbedtls_ssl_config conf; + test_sni_cb_ctx_t sni_ctx = { 0, 0, NULL, 0 }; + const unsigned char *sni_name; + size_t sni_name_len; + + mbedtls_ssl_init(&ssl); + mbedtls_ssl_config_init(&conf); + USE_PSA_INIT(); + + TEST_EQUAL(mbedtls_ssl_config_defaults(&conf, MBEDTLS_SSL_IS_SERVER, + MBEDTLS_SSL_TRANSPORT_STREAM, + MBEDTLS_SSL_PRESET_DEFAULT), + 0); + if (sni_cb_mode != TEST_SNI_CB_NONE) { + sni_ctx.ret = (sni_cb_mode == TEST_SNI_CB_REJECT) ? -1 : 0; + mbedtls_ssl_conf_sni(&conf, test_sni_cb, &sni_ctx); + } + TEST_EQUAL(mbedtls_ssl_setup(&ssl, &conf), 0); + + TEST_EQUAL(mbedtls_ssl_parse_server_name_ext(&ssl, ext_data->x, + ext_data->x + ext_data->len), + expected_ret); + + if (expected_alert != 0) { + TEST_EQUAL(ssl.MBEDTLS_PRIVATE(send_alert), 1); + TEST_EQUAL(ssl.MBEDTLS_PRIVATE(alert_type), expected_alert); + TEST_EQUAL(ssl.MBEDTLS_PRIVATE(alert_reason), expected_ret); + } else { + TEST_EQUAL(ssl.MBEDTLS_PRIVATE(send_alert), 0); + } + + /* The host name is recorded, and the callback called with it, only + * once the whole extension has been validated. */ + sni_name = ssl.MBEDTLS_PRIVATE(handshake)->sni_name; + sni_name_len = ssl.MBEDTLS_PRIVATE(handshake)->sni_name_len; + if (expected_hostname->len == 0) { + TEST_ASSERT(sni_name == NULL); + TEST_EQUAL(sni_ctx.calls, 0); + } else { + TEST_MEMORY_COMPARE(sni_name, sni_name_len, + expected_hostname->x, expected_hostname->len); + if (sni_cb_mode != TEST_SNI_CB_NONE) { + TEST_EQUAL(sni_ctx.calls, 1); + TEST_MEMORY_COMPARE(sni_ctx.name, sni_ctx.name_len, + expected_hostname->x, expected_hostname->len); + } + } + +exit: + mbedtls_ssl_free(&ssl); + mbedtls_ssl_config_free(&conf); + USE_PSA_DONE(); +} +/* END_CASE */