From: Daiki Ueno Date: Sun, 9 Feb 2025 10:31:20 +0900 Subject: handshake: only shuffle extensions in the first Client Hello RFC 8446 section 4.1.2 states that the second Client Hello after HRR should preserve the same content as the first Client Hello with limited exceptions. Since GnuTLS 3.8.5, however, the library started shuffling the order of extensions for privacy reasons and that didn't comply with the RFC, leading to a connectivity issue against the server configuration with a stricter check on that. Signed-off-by: Daiki Ueno Origin: upstream, 3.8.10, commit:dc5ee80c3a28577e9de0f82fb08164e4c02b96af Bug: https://gitlab.com/gnutls/gnutls/-/work_items/1660 Bug-Debian: https://bugs.debian.org/1130152 Bug-SteamRT: steamrt/tasks#938 --- lib/gnutls_int.h | 4 +++ lib/hello_ext.c | 41 +++++++++++++++++++------------ lib/state.c | 2 ++ tests/tls13/hello_retry_request.c | 51 ++++++++++++++++++++++++++++++++++++--- 4 files changed, 79 insertions(+), 19 deletions(-) diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h index d10a028..572de5a 100644 --- a/lib/gnutls_int.h +++ b/lib/gnutls_int.h @@ -1666,6 +1666,10 @@ typedef struct { /* Compression method for certificate compression */ gnutls_compression_method_t compress_certificate_method; + /* To shuffle extension sending order */ + extensions_t client_hello_exts[MAX_EXT_TYPES]; + bool client_hello_exts_set; + /* If you add anything here, check _gnutls_handshake_internal_state_clear(). */ } internals_st; diff --git a/lib/hello_ext.c b/lib/hello_ext.c index 45237b8..94e6d86 100644 --- a/lib/hello_ext.c +++ b/lib/hello_ext.c @@ -438,8 +438,6 @@ int _gnutls_gen_hello_extensions(gnutls_session_t session, int pos, ret; size_t i; hello_ext_ctx_st ctx; - /* To shuffle extension sending order */ - extensions_t indices[MAX_EXT_TYPES]; msg &= GNUTLS_EXT_FLAG_SET_ONLY_FLAGS_MASK; @@ -469,26 +467,39 @@ int _gnutls_gen_hello_extensions(gnutls_session_t session, ret - 4); } - /* Initializing extensions array */ - for (i = 0; i < MAX_EXT_TYPES; i++) { - indices[i] = i; - } + if (msg & GNUTLS_EXT_FLAG_CLIENT_HELLO && + !session->internals.client_hello_exts_set) { + /* Initializing extensions array */ + for (i = 0; i < MAX_EXT_TYPES; i++) { + session->internals.client_hello_exts[i] = i; + } - if (!session->internals.priorities->no_shuffle_extensions) { - /* Ordering padding and pre_shared_key as last extensions */ - swap_exts(indices, MAX_EXT_TYPES - 2, GNUTLS_EXTENSION_DUMBFW); - swap_exts(indices, MAX_EXT_TYPES - 1, - GNUTLS_EXTENSION_PRE_SHARED_KEY); + if (!session->internals.priorities->no_shuffle_extensions) { + /* Ordering padding and pre_shared_key as last extensions */ + swap_exts(session->internals.client_hello_exts, + MAX_EXT_TYPES - 2, GNUTLS_EXTENSION_DUMBFW); + swap_exts(session->internals.client_hello_exts, + MAX_EXT_TYPES - 1, + GNUTLS_EXTENSION_PRE_SHARED_KEY); - ret = shuffle_exts(indices, MAX_EXT_TYPES - 2); - if (ret < 0) - return gnutls_assert_val(ret); + ret = shuffle_exts(session->internals.client_hello_exts, + MAX_EXT_TYPES - 2); + if (ret < 0) + return gnutls_assert_val(ret); + } + session->internals.client_hello_exts_set = true; } /* hello_ext_send() ensures we don't send duplicates, in case * of overridden extensions */ for (i = 0; i < MAX_EXT_TYPES; i++) { - size_t ii = indices[i]; + size_t ii; + + if (msg & GNUTLS_EXT_FLAG_CLIENT_HELLO) + ii = session->internals.client_hello_exts[i]; + else + ii = i; + if (!extfunc[ii]) continue; diff --git a/lib/state.c b/lib/state.c index 020f212..8090686 100644 --- a/lib/state.c +++ b/lib/state.c @@ -518,6 +518,8 @@ static void handshake_internal_state_clear1(gnutls_session_t session) session->internals.hrr_cs[0] = CS_INVALID_MAJOR; session->internals.hrr_cs[1] = CS_INVALID_MINOR; + + session->internals.client_hello_exts_set = false; } /* This function will clear all the variables in internals diff --git a/tests/tls13/hello_retry_request.c b/tests/tls13/hello_retry_request.c index f407b64..6c5f698 100644 --- a/tests/tls13/hello_retry_request.c +++ b/tests/tls13/hello_retry_request.c @@ -51,14 +51,37 @@ static void tls_log_func(int level, const char *str) } #define HANDSHAKE_SESSION_ID_POS 34 +#define MAX_EXT_TYPES 64 struct ctx_st { unsigned hrr_seen; unsigned hello_counter; uint8_t session_id[32]; size_t session_id_len; + unsigned extensions[MAX_EXT_TYPES]; + size_t extensions_size1; + size_t extensions_size2; }; +static int ext_callback(void *_ctx, unsigned tls_id, const unsigned char *data, + unsigned size) +{ + struct ctx_st *ctx = _ctx; + if (ctx->hello_counter == 0) { + assert(ctx->extensions_size1 < MAX_EXT_TYPES); + ctx->extensions[ctx->extensions_size1++] = tls_id; + } else { + assert(ctx->extensions_size2 < MAX_EXT_TYPES); + if (tls_id != ctx->extensions[ctx->extensions_size2]) { + fail("extension doesn't match at position %zu, %u != %u\n", + ctx->extensions_size2, tls_id, + ctx->extensions[ctx->extensions_size2]); + } + ctx->extensions_size2++; + } + return 0; +} + static int hello_callback(gnutls_session_t session, unsigned int htype, unsigned post, unsigned int incoming, const gnutls_datum_t *msg) @@ -73,15 +96,25 @@ static int hello_callback(gnutls_session_t session, unsigned int htype, post == GNUTLS_HOOK_POST) { size_t session_id_len; uint8_t *session_id; + unsigned pos = HANDSHAKE_SESSION_ID_POS; + gnutls_datum_t mmsg; + int ret; - assert(msg->size > HANDSHAKE_SESSION_ID_POS + 1); - session_id_len = msg->data[HANDSHAKE_SESSION_ID_POS]; - session_id = &msg->data[HANDSHAKE_SESSION_ID_POS + 1]; + assert(msg->size > pos + 1); + session_id_len = msg->data[pos]; + session_id = &msg->data[pos + 1]; + + SKIP8(pos, msg->size); + SKIP16(pos, msg->size); + SKIP8(pos, msg->size); + + mmsg.data = &msg->data[pos]; + mmsg.size = msg->size - pos; if (ctx->hello_counter > 0) { assert(msg->size > 4); if (msg->data[0] != 0x03 || msg->data[1] != 0x03) { - fail("version is %d.%d expected 3,3\n", + fail("version is %d.%d expected 3.3\n", (int)msg->data[0], (int)msg->data[1]); } @@ -95,6 +128,12 @@ static int hello_callback(gnutls_session_t session, unsigned int htype, ctx->session_id_len = session_id_len; memcpy(ctx->session_id, session_id, session_id_len); + ret = gnutls_ext_raw_parse(ctx, ext_callback, &mmsg, 0); + if (ret < 0) { + fail("unable to parse extensions: %s\n", + gnutls_strerror(ret)); + } + ctx->hello_counter++; } @@ -164,6 +203,10 @@ void doit(void) myfail("group doesn't match the expected: %s\n", gnutls_group_get_name(gnutls_group_get(server))); + if (ctx.extensions_size1 != ctx.extensions_size2) + myfail("the number of extensions don't match in second Client Hello: %zu != %zu\n", + ctx.extensions_size1, ctx.extensions_size2); + gnutls_bye(client, GNUTLS_SHUT_WR); gnutls_bye(server, GNUTLS_SHUT_WR);