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

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

[Andrew Zaborowski]

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(-)

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 :
-- 
2.34.1

[PATCH 2/4] tls: Improve renegotiation

[Andrew Zaborowski]

On client, allow renegotiation triggered by the server.  Make sure to
reset tls->peer_authenticated before a new handshake.  Avoid calling the
ready callback multiple times, only call it after initial handshake.
While RFC 5746 makes the case for TLS APIs to inform the application
layer of renegotiations, some ell users don't consider the possiblity of
the ready callback happening more than once in a session and the name
implies that the TLS tunnel wasn't ready before the call.
---
 ell/tls.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/ell/tls.c b/ell/tls.c
index 5f3b717..14c8550 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -198,6 +198,7 @@ static void tls_reset_handshake(struct l_tls *tls)
 	tls->peer_cert = NULL;
 	tls->peer_pubkey = NULL;
 	tls->peer_pubkey_size = 0;
+	tls->peer_authenticated = false;
 	tls->negotiated_curve = NULL;
 	tls->negotiated_ff_group = NULL;
 
@@ -2898,6 +2899,7 @@ static void tls_finished(struct l_tls *tls)
 	uint64_t peer_cert_expiry;
 	bool resuming = tls->session_id_size && !tls->session_id_new;
 	bool session_update = false;
+	bool renegotiation = tls->ready;
 
 	if (tls->peer_authenticated && !resuming) {
 		peer_cert_identity = tls_get_peer_identity_str(tls->peer_cert);
@@ -2998,9 +3000,11 @@ static void tls_finished(struct l_tls *tls)
 			return;
 	}
 
-	tls->in_callback = true;
-	tls->ready_handle(peer_identity, tls->user_data);
-	tls->in_callback = false;
+	if (!renegotiation) {
+		tls->in_callback = true;
+		tls->ready_handle(peer_identity, tls->user_data);
+		tls->in_callback = false;
+	}
 
 	tls_cleanup_handshake(tls);
 }
@@ -3033,7 +3037,16 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 		 * and "MAY be ignored by the client if it does not wish to
 		 * renegotiate a session".
 		 */
+		if (tls->state != TLS_HANDSHAKE_DONE) {
+			TLS_DEBUG("Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
+			break;
+		}
+
+		if (!tls_send_client_hello(tls))
+			break;
 
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO);
 		break;
 
 	case TLS_CLIENT_HELLO:
-- 
2.34.1

[PATCH 3/4] tls: Implement RFC 5746 Secure Renegotiation

[Andrew Zaborowski]

Implement the renegotiation_info extension to prevent some types of
attacks using renegotiation (describen in the RFC) and make the l_tls
server compatible with more clients.

Move the tls_verify_data_length declaration to tls.h for use in
tls-extensions.c.

Due to an assertion in tls_save_verify_data(), tls_send_finished() now
returns a bool so its callers are also changed to check the return value.
---
 ell/tls-extensions.c | 133 +++++++++++++++++++++++++++++++++++++++++++
 ell/tls-private.h    |   9 +++
 ell/tls.c            |  79 +++++++++++++++++++++++--
 3 files changed, 216 insertions(+), 5 deletions(-)

diff --git a/ell/tls-extensions.c b/ell/tls-extensions.c
index d03c331..bc8fc3d 100644
--- a/ell/tls-extensions.c
+++ b/ell/tls-extensions.c
@@ -850,6 +850,130 @@ static bool tls_signature_algorithms_client_absent(struct l_tls *tls)
 	return true;
 }
 
+/* RFC 5746, Section 3.2 */
+static ssize_t tls_renegotiation_info_client_write(struct l_tls *tls,
+						uint8_t *buf, size_t len)
+{
+	/*
+	 * Section 4.2 implies we should send the client_verify_data on
+	 * renegotiation even if .secure_renegotiation is false, if we want
+	 * to go through with the renegotiation in the first place.
+	 */
+	if (tls->ready) {
+		size_t vdl = tls_verify_data_length(tls, 1);
+
+		if (len < vdl + 1)
+			return -ENOMEM;
+
+		buf[0] = vdl;
+		memcpy(buf + 1, tls->renegotiation_info.client_verify_data,
+			vdl);
+		return 1 + vdl;
+	} else {
+		if (len < 1)
+			return -ENOMEM;
+
+		buf[0] = 0x00;	/* Empty "renegotiated_connection" */
+		return 1;
+	}
+}
+
+static ssize_t tls_renegotiation_info_server_write(struct l_tls *tls,
+						uint8_t *buf, size_t len)
+{
+	if (tls->ready) {
+		size_t rx_vdl = tls_verify_data_length(tls, 0);
+		size_t tx_vdl = tls_verify_data_length(tls, 1);
+
+		if (len < rx_vdl + tx_vdl + 1)
+			return -ENOMEM;
+
+		buf[0] = rx_vdl + tx_vdl;
+		memcpy(buf + 1,
+			tls->renegotiation_info.client_verify_data, rx_vdl);
+		memcpy(buf + 1 + rx_vdl,
+			tls->renegotiation_info.server_verify_data, tx_vdl);
+		return 1 + rx_vdl + tx_vdl;
+	} else {
+		if (len < 1)
+			return -ENOMEM;
+
+		buf[0] = 0x00;	/* Empty "renegotiated_connection" */
+		return 1;
+	}
+}
+
+static bool tls_renegotiation_info_client_handle(struct l_tls *tls,
+						const uint8_t *buf, size_t len)
+{
+	if (tls->ready) {
+		size_t vdl = tls_verify_data_length(tls, 0);
+
+		return len >= 1 + vdl &&
+			tls->renegotiation_info.secure_renegotiation &&
+			!memcmp(tls->renegotiation_info.client_verify_data,
+				buf + 1, vdl);
+	}
+
+	/*
+	 * RFC 5746 Section 3.6: "The server MUST then verify that the length
+	 * of the "renegotiated_connection" field is zero, ..."
+	 */
+	if (len < 1 || buf[0] != 0x00)
+		return false;
+
+	tls->renegotiation_info.secure_renegotiation = true;
+	return true;
+}
+
+static bool tls_renegotiation_info_server_handle(struct l_tls *tls,
+						const uint8_t *buf, size_t len)
+{
+	if (tls->ready) {
+		size_t rx_vdl = tls_verify_data_length(tls, 0);
+		size_t tx_vdl = tls_verify_data_length(tls, 1);
+
+		return len >= 1 + rx_vdl + tx_vdl &&
+			tls->renegotiation_info.secure_renegotiation &&
+			!memcmp(tls->renegotiation_info.client_verify_data,
+				buf + 1, tx_vdl) &&
+			!memcmp(tls->renegotiation_info.server_verify_data,
+				buf + 1 + tx_vdl, rx_vdl);
+	}
+
+	/*
+	 * RFC 5746 Section 3.4: "The client MUST then verify that the length
+	 * of the "renegotiated_connection" field is zero, ..."
+	 */
+	if (len < 1 || buf[0] != 0x00)
+		return false;
+
+	tls->renegotiation_info.secure_renegotiation = true;
+	return true;
+}
+
+static bool tls_renegotiation_info_absent(struct l_tls *tls)
+{
+	/*
+	 * RFC 5746 Section 4.2: "It is possible that un-upgraded servers
+	 * will request that the client renegotiate.  It is RECOMMENDED
+	 * that clients refuse this renegotiation request." and Section 4.4:
+	 * "It is RECOMMENDED that servers not permit legacy renegotiation."
+	 *
+	 * This may need to be made configurable, for now follow the
+	 * recommendation and don't renegotiate.
+	 */
+	if (tls->ready)
+		return false;
+
+	/*
+	 * The normal policy otherwise is that the extension must be
+	 * present in renegotation if the previous Client or Server Hello
+	 * did include this extension, or the SCSV in the Client Hello case.
+	 */
+	return !tls->ready || !tls->renegotiation_info.secure_renegotiation;
+}
+
 const struct tls_hello_extension tls_extensions[] = {
 	{
 		"Supported Groups", "elliptic_curves", 10,
@@ -873,6 +997,15 @@ const struct tls_hello_extension tls_extensions[] = {
 		tls_signature_algorithms_client_absent,
 		NULL, NULL, NULL,
 	},
+	{
+		"Secure Renegotiation", "renegotiation_info", 0xff01,
+		tls_renegotiation_info_client_write,
+		tls_renegotiation_info_client_handle,
+		tls_renegotiation_info_absent,
+		tls_renegotiation_info_server_write,
+		tls_renegotiation_info_server_handle,
+		tls_renegotiation_info_absent,
+	},
 	{}
 };
 
diff --git a/ell/tls-private.h b/ell/tls-private.h
index a3a9393..6a9bd29 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -267,6 +267,13 @@ struct l_tls {
 	uint8_t session_compression_method_id;
 	char *session_peer_identity;
 
+	struct {
+		bool secure_renegotiation;
+		/* Max .verify_data_length over supported cipher suites */
+		uint8_t client_verify_data[12];
+		uint8_t server_verify_data[12];
+	} renegotiation_info;
+
 	/* SecurityParameters current and pending */
 
 	struct {
@@ -341,6 +348,8 @@ void tls_generate_master_secret(struct l_tls *tls,
 				const uint8_t *pre_master_secret,
 				int pre_master_secret_len);
 
+size_t tls_verify_data_length(struct l_tls *tls, unsigned int index);
+
 const struct tls_named_group *tls_find_group(uint16_t id);
 const struct tls_named_group *tls_find_ff_group(const uint8_t *prime,
 						size_t prime_len,
diff --git a/ell/tls.c b/ell/tls.c
index 14c8550..2a8d509 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -1234,6 +1234,7 @@ void tls_disconnect(struct l_tls *tls, enum l_tls_alert_desc desc,
 
 	tls->negotiated_version = 0;
 	tls->ready = false;
+	tls->renegotiation_info.secure_renegotiation = false;
 
 	if (forget_session) {
 		tls_forget_cached_session(tls, NULL, tls->session_id,
@@ -1749,7 +1750,7 @@ static void tls_send_change_cipher_spec(struct l_tls *tls)
 	tls_tx_record(tls, TLS_CT_CHANGE_CIPHER_SPEC, &buf, 1);
 }
 
-static size_t tls_verify_data_length(struct l_tls *tls, unsigned int index)
+size_t tls_verify_data_length(struct l_tls *tls, unsigned int index)
 {
 	/*
 	 * RFC 5246, Section 7.4.9:
@@ -1762,7 +1763,34 @@ static size_t tls_verify_data_length(struct l_tls *tls, unsigned int index)
 	return maxsize(tls->cipher_suite[index]->verify_data_length, 12);
 }
 
-static void tls_send_finished(struct l_tls *tls)
+static bool tls_save_verify_data(struct l_tls *tls, bool txrx,
+					const uint8_t *vd, size_t vdl)
+{
+	uint8_t *buf;
+
+	if (tls->server == txrx) {
+		if (vdl > sizeof(tls->renegotiation_info.server_verify_data))
+			goto error;
+
+		buf = tls->renegotiation_info.server_verify_data;
+	} else {
+		if (vdl > sizeof(tls->renegotiation_info.client_verify_data))
+			goto error;
+
+		buf = tls->renegotiation_info.client_verify_data;
+	}
+
+	memcpy(buf, vd, vdl);
+	return true;
+
+error:
+	TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+			"tls->renegotiation_info.*verify too small for %s, "
+			"report an ell bug", tls->cipher_suite[txrx]->name);
+	return false;
+}
+
+static bool tls_send_finished(struct l_tls *tls)
 {
 	uint8_t buf[512];
 	uint8_t *ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
@@ -1785,9 +1813,14 @@ static void tls_send_finished(struct l_tls *tls)
 				"client finished",
 				seed, seed_len,
 				ptr, vdl);
+
+	if (!tls_save_verify_data(tls, 1, ptr, vdl))
+		return false;
+
 	ptr += vdl;
 
 	tls_tx_handshake(tls, TLS_FINISHED, buf, ptr - buf);
+	return true;
 }
 
 static bool tls_verify_finished(struct l_tls *tls, const uint8_t *received,
@@ -1830,6 +1863,9 @@ static bool tls_verify_finished(struct l_tls *tls, const uint8_t *received,
 		return false;
 	}
 
+	if (!tls_save_verify_data(tls, 0, received, vdl))
+		return false;
+
 	return true;
 }
 
@@ -1992,6 +2028,10 @@ static void tls_server_resume_error(struct l_tls *tls)
 	l_free(l_steal_ptr(tls->session_peer_identity));
 }
 
+/* RFC 5746 */
+static const uint8_t tls_empty_renegotiation_info_scsv[2] = { 0x00, 0xff };
+static const uint16_t tls_renegotiation_info_id = 0xff01;
+
 static void tls_handle_client_hello(struct l_tls *tls,
 					const uint8_t *buf, size_t len)
 {
@@ -2100,6 +2140,30 @@ static void tls_handle_client_hello(struct l_tls *tls,
 		goto cleanup;
 	}
 
+	if (!tls->renegotiation_info.secure_renegotiation || tls->ready) {
+		for (i = 0; i < cipher_suites_size; i += 2)
+			if (l_get_be16(cipher_suites + i) == l_get_be16(
+					tls_empty_renegotiation_info_scsv))
+				break;
+
+		if (i < cipher_suites_size) {
+			if (unlikely(tls->ready)) {
+				TLS_DISCONNECT(TLS_ALERT_ILLEGAL_PARAM, 0,
+						"Empty renegotiation_info in "
+						"renegotiation Client Hello");
+				goto cleanup;
+			}
+
+			/*
+			 * RFC 5746 Section 3.6, act as if we had received an
+			 * empty renegotiation_info extension.
+			 */
+			tls->renegotiation_info.secure_renegotiation = true;
+			l_queue_push_tail(extensions_offered, L_UINT_TO_PTR(
+						tls_renegotiation_info_id));
+		}
+	}
+
 	/* Select a cipher suite according to client's preference list */
 	while (cipher_suites_size) {
 		struct tls_cipher_suite *suite =
@@ -2243,7 +2307,9 @@ static void tls_handle_client_hello(struct l_tls *tls,
 			return;
 		}
 
-		tls_send_finished(tls);
+		if (!tls_send_finished(tls))
+			return;
+
 		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC);
 		return;
 	}
@@ -2721,7 +2787,8 @@ static void tls_handle_server_hello_done(struct l_tls *tls,
 		return;
 	}
 
-	tls_send_finished(tls);
+	if (!tls_send_finished(tls))
+		return;
 
 	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC);
 }
@@ -3222,7 +3289,9 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 						error);
 				break;
 			}
-			tls_send_finished(tls);
+
+			if (!tls_send_finished(tls))
+				break;
 		}
 
 		/*
-- 
2.34.1

[PATCH 4/4] tls: Add missing continue in tls_load_cached_server_session

[Andrew Zaborowski]

After we removed an expired session cache entry, move on to the next
entry in the loop.  Ignore the removed entry in the count, in the search
for the requested session ID, etc.  Not ignoring it would cause some
warnings to be logged later and other minor issues.
---
 ell/tls.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ell/tls.c b/ell/tls.c
index 2a8d509..d363765 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -1084,6 +1084,7 @@ static bool tls_load_cached_server_session(struct l_tls *tls,
 					*group + prefix_len + 1);
 			l_settings_remove_group(tls->session_settings, *group);
 			changed = true;
+			continue;
 		}
 
 		cnt++;
-- 
2.34.1

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

[Denis Kenzior]

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