Merge branch 'wip/dueno/shuffle-exts-followup' into 'master'

doc: mention ClientHello extensions shuffling

See merge request gnutls/gnutls!1738

9 files changed, 67 insertions, 46 deletions

diff --git a/NEWS b/NEWS
index 331c8074c8..5f6e45a814 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,15 @@ Copyright (C) 2000-2016 Free Software Foundation, Inc.
 Copyright (C) 2013-2019 Nikos Mavrogiannopoulos
 See the end for copying conditions.
 
+* Version 3.8.1 (unreleased)
+
+** libgnutls: ClientHello extensions are randomized by default
+   To make fingerprinting harder, TLS extensions in ClientHello
+   messages are shuffled. As this behavior may cause compatibility
+   issue with legacy applications that do not accept the last
+   extension without payload, the behavior can be reverted with the
+   %NO_SHUFFLE_EXTENSIONS priority keyword.
+
 * Version 3.8.0 (released 2023-02-09)
 
 ** libgnutls: Fix a Bleichenbacher oracle in the TLS RSA key exchange.
diff --git a/doc/cha-gtls-app.texi b/doc/cha-gtls-app.texi
index 77727a44ec..8755a6a026 100644
--- a/doc/cha-gtls-app.texi
+++ b/doc/cha-gtls-app.texi
@@ -1552,6 +1552,10 @@ that TLS 1.2 requires extensions to be used, as well as safe
 renegotiation thus this option must be used with care. When this option
 is set no versions later than TLS1.2 can be negotiated.
 
+@item %NO_SHUFFLE_EXTENSIONS @tab
+will prevent randomizing the order of ClientHello extensions. By
+default, those extensions are randomized to make fingerprinting harder.
+
 @item %NO_STATUS_REQUEST @tab
 will prevent sending of the TLS status_request extension in client side.
 
@@ -1608,8 +1612,8 @@ negotiate CBC ciphersuites only when both sides of the connection support
 encrypt-then-mac TLS extension (RFC7366).
 
 @item %DISABLE_SAFE_RENEGOTIATION @tab
-will completely disable safe renegotiation
-completely.  Do not use unless you know what you are doing.
+will completely disable safe renegotiation.  Do not use unless you
+know what you are doing.
 
 @item %UNSAFE_RENEGOTIATION @tab
 will allow handshakes and re-handshakes
diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h
index a3ee5e6f20..0d0b9a8e5e 100644
--- a/lib/gnutls_int.h
+++ b/lib/gnutls_int.h
@@ -356,6 +356,11 @@ verify(GNUTLS_EXTENSION_MAX < MAX_EXT_TYPES);
  */
 verify(GNUTLS_EXTENSION_MAX_VALUE - GNUTLS_EXTENSION_MAX >= 16);
 
+/* MAX_EXT_TYPES + 1 must fit in a single byte, to generate random
+ * permutation at once.
+ */
+verify(MAX_EXT_TYPES <= UINT8_MAX);
+
 /* The 'verify' symbol from <verify.h> is used extensively in the
  * code; undef it to avoid clash
  */
@@ -949,7 +954,7 @@ struct gnutls_priority_st {
 	bool no_extensions;
 
 	/* to disable extensions shuffling */
-	bool no_exts_shuffle;
+	bool no_shuffle_extensions;
 
 	safe_renegotiation_t sr;
 	bool min_record_version;
diff --git a/lib/hello_ext.c b/lib/hello_ext.c
index a15e91876a..a2159429a0 100644
--- a/lib/hello_ext.c
+++ b/lib/hello_ext.c
@@ -408,25 +408,32 @@ int hello_ext_send(void *_ctx, gnutls_buffer_st * buf)
 	return 0;
 }
 
-static inline void swap_exts(extensions_t * exts1, extensions_t * exts2)
+static inline void swap_exts(extensions_t * exts, size_t index1, size_t index2)
 {
-	extensions_t temp = *exts1;
-	*exts1 = *exts2;
-	*exts2 = temp;
+	extensions_t temp = exts[index1];
+	exts[index1] = exts[index2];
+	exts[index2] = temp;
 }
 
 static
 int shuffle_exts(extensions_t * exts, size_t size)
 {
-	/* generating random permutation of extensions */
-	extensions_t rnd_n;
-	for (size_t i = size - 1; i > 0; i--) {
-		int ret = gnutls_rnd(GNUTLS_RND_RANDOM, (void *)&rnd_n,
-				     sizeof(extensions_t));
-		if (ret < 0)
-			return ret;
-		extensions_t j = rnd_n % (i + 1);
-		swap_exts(&exts[i], &exts[j]);
+	uint8_t permutation[MAX_EXT_TYPES];
+	size_t i;
+	int ret;
+
+	assert(size <= MAX_EXT_TYPES);
+
+	/* Generate random permutation, assuming MAX_EXT_TYPES <
+	 * UINT8_MAX.
+	 */
+	ret = gnutls_rnd(GNUTLS_RND_RANDOM, permutation, size);
+	if (ret < 0)
+		return gnutls_assert_val(ret);
+
+	for (i = size - 1; i > 0; i--) {
+		extensions_t j = permutation[i] % (i + 1);
+		swap_exts(exts, i, j);
 	}
 
 	return 0;
@@ -441,6 +448,8 @@ _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;
 
@@ -470,37 +479,31 @@ _gnutls_gen_hello_extensions(gnutls_session_t session,
 			     ret - 4);
 	}
 
-	/* To shuffle extension sending order */
-	extensions_t shuffled_exts[MAX_EXT_TYPES];
-
 	/* Initializing extensions array */
 	for (i = 0; i < MAX_EXT_TYPES; i++) {
-		shuffled_exts[i] = i;
+		indices[i] = i;
 	}
 
-	/* ordering dumbfw and pre_shared_key as last extensions */
-	swap_exts(&shuffled_exts[MAX_EXT_TYPES - 2],
-		  &shuffled_exts[GNUTLS_EXTENSION_DUMBFW]);
-	swap_exts(&shuffled_exts[MAX_EXT_TYPES - 1],
-		  &shuffled_exts[GNUTLS_EXTENSION_PRE_SHARED_KEY]);
-
-	if (session->internals.priorities->no_exts_shuffle == 1)
-		goto next;
+	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);
 
-	ret = shuffle_exts(shuffled_exts, MAX_EXT_TYPES - 2);
-	if (ret < 0)
-		return gnutls_assert_val(ret);
+		ret = shuffle_exts(indices, MAX_EXT_TYPES - 2);
+		if (ret < 0)
+			return gnutls_assert_val(ret);
+	}
 
- next:
 	/* hello_ext_send() ensures we don't send duplicates, in case
 	 * of overridden extensions */
-	for (size_t r = 0; r < MAX_EXT_TYPES; r++) {
-		i = shuffled_exts[r];
-		if (!extfunc[i])
+	for (i = 0; i < MAX_EXT_TYPES; i++) {
+		size_t ii = indices[i];
+		if (!extfunc[ii])
 			continue;
 
-		ctx.ext = extfunc[i];
-		ret = _gnutls_extv_append(buf, extfunc[i]->tls_id,
+		ctx.ext = extfunc[ii];
+		ret = _gnutls_extv_append(buf, extfunc[ii]->tls_id,
 					  &ctx, hello_ext_send);
 		if (ret < 0)
 			return gnutls_assert_val(ret);
diff --git a/lib/priority.c b/lib/priority.c
index 966de4d83d..0455b87ee8 100644
--- a/lib/priority.c
+++ b/lib/priority.c
@@ -1081,9 +1081,9 @@ static void disable_tls13_compat_mode(gnutls_priority_t c)
 	c->tls13_compat_mode = false;
 }
 
-static void enable_no_exts_shuffle(gnutls_priority_t c)
+static void enable_no_shuffle_extensions(gnutls_priority_t c)
 {
-	c->no_exts_shuffle = 1;
+	c->no_shuffle_extensions = 1;
 }
 
 static void dummy_func(gnutls_priority_t c)
diff --git a/lib/priority_options.gperf b/lib/priority_options.gperf
index d69c3b4b11..a8f182728b 100644
--- a/lib/priority_options.gperf
+++ b/lib/priority_options.gperf
@@ -43,4 +43,4 @@ NEW_PADDING, dummy_func
 DEBUG_ALLOW_KEY_USAGE_VIOLATIONS, enable_server_key_usage_violations
 ALLOW_SMALL_RECORDS, enable_allow_small_records
 DISABLE_TLS13_COMPAT_MODE, disable_tls13_compat_mode
-NO_EXTS_SHUFFLE, enable_no_exts_shuffle
+NO_SHUFFLE_EXTENSIONS, enable_no_shuffle_extensions
diff --git a/tests/tls13-early-data.c b/tests/tls13-early-data.c
index d7ed79b3bf..0676dc2002 100644
--- a/tests/tls13-early-data.c
+++ b/tests/tls13-early-data.c
@@ -91,8 +91,8 @@ extern unsigned int _gnutls_global_version;
  * selected during the initial handshake, not the resuming handshakes.
  */
 # define SESSIONS 3
-# define TLS13_AES_128_GCM "NONE:+VERS-TLS1.3:+AES-128-GCM:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_EXTS_SHUFFLE"
-# define TLS13_CHACHA20_POLY1305 "NONE:+VERS-TLS1.3:+CHACHA20-POLY1305:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_EXTS_SHUFFLE"
+# define TLS13_AES_128_GCM "NONE:+VERS-TLS1.3:+AES-128-GCM:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_SHUFFLE_EXTENSIONS"
+# define TLS13_CHACHA20_POLY1305 "NONE:+VERS-TLS1.3:+CHACHA20-POLY1305:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_SHUFFLE_EXTENSIONS"
 
 static const
 gnutls_datum_t hrnd = { (void *)
diff --git a/tests/tls13/prf-early.c b/tests/tls13/prf-early.c
index 0df9a18abd..a55970cf3c 100644
--- a/tests/tls13/prf-early.c
+++ b/tests/tls13/prf-early.c
@@ -183,9 +183,9 @@ static void client(int sds[])
 		 */
 		gnutls_init(&session, GNUTLS_CLIENT);
 
-		/* Use default priorities, sets %NO_EXTS_SHUFFLE */
+		/* Use default priorities, sets %NO_SHUFFLE_EXTENSIONS */
 		ret = gnutls_priority_set_direct(session,
-						 "NONE:+VERS-TLS1.3:+AES-256-GCM:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_EXTS_SHUFFLE",
+						 "NONE:+VERS-TLS1.3:+AES-256-GCM:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_SHUFFLE_EXTENSIONS",
 						 &err);
 		if (ret < 0) {
 			fail("client: priority set failed (%s): %s\n",
diff --git a/tests/tls13/prf.c b/tests/tls13/prf.c
index 003ed69b01..877f45867e 100644
--- a/tests/tls13/prf.c
+++ b/tests/tls13/prf.c
@@ -196,9 +196,9 @@ static void client(int fd)
 	 */
 	gnutls_init(&session, GNUTLS_CLIENT);
 
-	/* Use default priorities, sets %NO_EXTS_SHUFFLE */
+	/* Use default priorities, sets %NO_SHUFFLE_EXTENSIONS */
 	ret = gnutls_priority_set_direct(session,
-					 "NONE:+VERS-TLS1.3:+AES-256-GCM:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_EXTS_SHUFFLE",
+					 "NONE:+VERS-TLS1.3:+AES-256-GCM:+AEAD:+SIGN-RSA-PSS-RSAE-SHA384:+GROUP-SECP256R1:%NO_SHUFFLE_EXTENSIONS",
 					 &err);
 
 	if (ret < 0) {