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 <ueno@gnu.org>

diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h
index d10a028b59c3a41a57c040d650ebab4882e0184a..572de5aba32d3326e4e20a885b590ec93f20e5a8 100644 (file)
--- 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 40af8c2b102e58e46b2ccdfba6c39a3cdd13d0ca..d677addd756d0910c5b5399709c9519826d758bb 100644 (file)
--- 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 9d3ece757066460d00c641cda4416d329ce033cc..43e961cfa2207f2255cca5e674c93140479e5cf3 100644 (file)
--- 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 f407b642348c2ed416c0dfcc553f0712da2a0986..6c5f698f010e9f3ca07fdadbf74a94552fb3202c 100644 (file)
--- 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);