From dd046ab8d8596ac604fd01aac5b7b79d099a94d2 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Wed, 12 Feb 2025 17:31:50 +0900 Subject: [PATCH] handshake: only shuffle extensions in the first Client Hello Related: RHEL-58246 Signed-off-by: Daiki Ueno --- gnutls-3.8.9-limit-shuffle-extensions.patch | 213 ++++++++++++++++++++ gnutls.spec | 2 + 2 files changed, 215 insertions(+) create mode 100644 gnutls-3.8.9-limit-shuffle-extensions.patch diff --git a/gnutls-3.8.9-limit-shuffle-extensions.patch b/gnutls-3.8.9-limit-shuffle-extensions.patch new file mode 100644 index 0000000..8526252 --- /dev/null +++ b/gnutls-3.8.9-limit-shuffle-extensions.patch @@ -0,0 +1,213 @@ +From dc5ee80c3a28577e9de0f82fb08164e4c02b96af Mon Sep 17 00:00:00 2001 +From: Daiki Ueno +Date: Sun, 9 Feb 2025 10:31:20 +0900 +Subject: [PATCH] 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 +--- + 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 d10a028b59..572de5aba3 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 40af8c2b10..d677addd75 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 9d3ece7570..43e961cfa2 100644 +--- a/lib/state.c ++++ b/lib/state.c +@@ -516,6 +516,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 f407b64234..6c5f698f01 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); + +-- +2.48.1 + diff --git a/gnutls.spec b/gnutls.spec index 4f2d474..b644b84 100644 --- a/gnutls.spec +++ b/gnutls.spec @@ -30,6 +30,8 @@ Patch: gnutls-3.7.6-fips-sha1-sigver.patch Patch: gnutls-3.8.8-tests-ktls-skip-tls12-chachapoly.patch # not upstreamed: https://gitlab.com/gnutls/gnutls/-/merge_requests/1932 Patch: gnutls-3.8.9-allow-rsa-pkcs1-encrypt.patch +# upstreamed: https://gitlab.com/gnutls/gnutls/-/merge_requests/1930 +Patch: gnutls-3.8.9-limit-shuffle-extensions.patch %bcond_without bootstrap %bcond_without dane