[1/4] tls: Allow ServerHello extensions when resuming session

Commit Message

Andrew Zaborowski Nov. 9, 2022, 5:47 p.m. UTC

While, as the spec notes, most extensions are not used during the
resumption of a cached session (are offered in the ClientHello but are
ignored and not be included in the ServerHello), this not a rule so allow
each extension to decide whether to skip adding itself to the ServerHello.

Specifically those extensions should rather be ignored by the server
when it is parsing the ClientHello but at that point we don't yet know
whether we'll allow the session to be resumed so instead the ServerHello
writer callbacks decide whether to reply or not.  As an example RFC 4492
Section 4 says: "In the case of session resumption, the server simply
ignores the Supported Elliptic Curves Extension and the Supported Point
Formats Extension appearing in the current ClientHello message.  These
extensions only play a role during handshakes negotiating a new session."
---
 ell/tls-extensions.c |  9 +++++++++
 ell/tls.c            | 31 ++++++++++---------------------
 2 files changed, 19 insertions(+), 21 deletions(-)

Comments

Denis Kenzior Nov. 9, 2022, 8:25 p.m. UTC | #1

Hi Andrew,

On 11/9/22 11:47, Andrew Zaborowski wrote:
> While, as the spec notes, most extensions are not used during the
> resumption of a cached session (are offered in the ClientHello but are
> ignored and not be included in the ServerHello), this not a rule so allow
> each extension to decide whether to skip adding itself to the ServerHello.
> 
> Specifically those extensions should rather be ignored by the server
> when it is parsing the ClientHello but at that point we don't yet know
> whether we'll allow the session to be resumed so instead the ServerHello
> writer callbacks decide whether to reply or not.  As an example RFC 4492
> Section 4 says: "In the case of session resumption, the server simply
> ignores the Supported Elliptic Curves Extension and the Supported Point
> Formats Extension appearing in the current ClientHello message.  These
> extensions only play a role during handshakes negotiating a new session."
> ---
>   ell/tls-extensions.c |  9 +++++++++
>   ell/tls.c            | 31 ++++++++++---------------------
>   2 files changed, 19 insertions(+), 21 deletions(-)
> 

All applied, thanks.

Regards,
-Denis

Patch

diff --git a/ell/tls-extensions.c b/ell/tls-extensions.c
index 7796825..d03c331 100644
--- a/ell/tls-extensions.c
+++ b/ell/tls-extensions.c
@@ -31,6 +31,13 @@ 
 #include "cert.h"
 #include "tls-private.h"
 
+/* Most extensions are not used when resuming a cached session */
+#define SKIP_ON_RESUMPTION()	\
+	do {	\
+		if (tls->session_id_size && !tls->session_id_new)	\
+			return -ENOMSG;	\
+	} while (0);
+
 /* RFC 7919, Section A.1 */
 static const uint8_t tls_ffdhe2048_prime[] = {
 	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xad, 0xf8, 0x54, 0x58,
@@ -560,6 +567,8 @@  static bool tls_ec_point_formats_client_handle(struct l_tls *tls,
 static ssize_t tls_ec_point_formats_server_write(struct l_tls *tls,
 						uint8_t *buf, size_t len)
 {
+	SKIP_ON_RESUMPTION(); /* RFC 4492 Section 4 */
+
 	if (len < 2)
 		return -ENOMEM;
 
diff --git a/ell/tls.c b/ell/tls.c
index 98bc682..5f3b717 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -2053,11 +2053,8 @@  static void tls_handle_client_hello(struct l_tls *tls,
 			tls_load_cached_server_session(tls, buf + 35,
 							session_id_size)) {
 		/*
-		 * Attempt a session resumption but don't skip parsing Hello
-		 * extensions just yet because we may decide to start a new
-		 * session instead after our cipher suite and compression
-		 * method checks below and in that case we will need to
-		 * handle the extensions and include them in the Server Hello.
+		 * Attempt a session resumption but note later checks may
+		 * spoil this.
 		 */
 		resuming = true;
 		session_id_str = l_util_hexstring(tls->session_id,
@@ -2228,7 +2225,7 @@  static void tls_handle_client_hello(struct l_tls *tls,
 		l_getrandom(tls->session_id, 32);
 	}
 
-	if (!tls_send_server_hello(tls, resuming ? extensions_offered : NULL))
+	if (!tls_send_server_hello(tls, extensions_offered))
 		goto cleanup;
 
 	l_queue_destroy(extensions_offered, NULL);
@@ -2324,23 +2321,16 @@  static void tls_handle_server_hello(struct l_tls *tls,
 			TLS_DEBUG("Negotiated resumption of cached session %s",
 					session_id_str);
 			resuming = true;
-
-			/*
-			 * Skip parsing extensions as none of the ones we
-			 * support are used in session resumption.  We could
-			 * as well signal an error if the ServerHello has any
-			 * extensions, for now ignore them.
-			 */
-			goto check_version;
+		} else {
+			TLS_DEBUG("Server decided not to resume cached session "
+					"%s, sent %s session ID",
+					session_id_str,
+					session_id_size ? "a new" : "no");
+			tls->session_id_size = 0;
 		}
-
-		TLS_DEBUG("Server decided not to resume cached session %s, "
-				"sent %s session ID", session_id_str,
-				session_id_size ? "a new" : "no");
-		tls->session_id_size = 0;
 	}
 
-	if (session_id_size && tls->session_settings) {
+	if (session_id_size && !resuming && tls->session_settings) {
 		tls->session_id_new = true;
 		tls->session_id_size = session_id_size;
 		memcpy(tls->session_id, buf + 35, session_id_size);
@@ -2354,7 +2344,6 @@  static void tls_handle_server_hello(struct l_tls *tls,
 	if (!result)
 		return;
 
-check_version:
 	if (version < tls->min_version || version > tls->max_version) {
 		TLS_DISCONNECT(version < tls->min_version ?
 				TLS_ALERT_PROTOCOL_VERSION :