From: Tobias Brunner <tobias@strongswan.org>
Date: Tue, 25 Aug 2020 11:22:04 +0000 (+0200)
Subject: tls-crypto: Simplify handshake/application key derivation and rename methods
X-Git-Tag: 5.9.2rc1~23^2~91
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2921f43705f4b0113643ff8b9baf49a6bcc2355b;p=thirdparty%2Fstrongswan.git

tls-crypto: Simplify handshake/application key derivation and rename methods

Also consistently change the ciphers outside of tls_crypto_t and
simplify key derivation in tls_peer_t and fix a memory leak.
---

diff --git a/src/libtls/tls_crypto.c b/src/libtls/tls_crypto.c
index 77fc153095..0fbc51ec28 100644
--- a/src/libtls/tls_crypto.c
+++ b/src/libtls/tls_crypto.c
@@ -1244,10 +1244,6 @@ static bool create_aead(private_tls_crypto_t *this, suite_algs_t *algs)
 	{
 		this->aead_in = tls_aead_create_seq(algs->encr, algs->encr_size);
 		this->aead_out = tls_aead_create_seq(algs->encr, algs->encr_size);
-
-		this->protection->set_cipher(this->protection, TRUE, this->aead_in);
-		this->protection->set_cipher(this->protection, FALSE, this->aead_out);
-
 	}
 	if (!this->aead_in || !this->aead_out)
 	{
@@ -1982,148 +1978,79 @@ METHOD(tls_crypto_t, derive_secrets, bool,
 		   expand_keys(this, client_random, server_random);
 }
 
-METHOD(tls_crypto_t, derive_handshake_secret, bool,
-	private_tls_crypto_t *this, chunk_t shared_secret)
+/**
+ * Derive and configure the keys/IVs using the given labels.
+ */
+static bool derive_labeled_keys(private_tls_crypto_t *this,
+								tls_hkdf_label_t client_label,
+								tls_hkdf_label_t server_label)
 {
-	chunk_t c_key, c_iv, s_key, s_iv;
+	chunk_t c_key = chunk_empty, c_iv = chunk_empty;
+	chunk_t s_key = chunk_empty, s_iv = chunk_empty;
+	tls_aead_t *aead_c = this->aead_out, *aead_s = this->aead_in;
+	bool success = FALSE;
 
-	this->hkdf->set_shared_secret(this->hkdf, shared_secret);
+	if (this->tls->is_server(this->tls))
+	{
+		aead_c = this->aead_in;
+		aead_s = this->aead_out;
+	}
 
-	/* client key material */
-	if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_C_HS_TRAFFIC,
-									 this->handshake, NULL) ||
+	if (!this->hkdf->generate_secret(this->hkdf, client_label, this->handshake,
+									 NULL) ||
 		!this->hkdf->derive_key(this->hkdf, FALSE,
-								this->aead_out->
-								get_encr_key_size(this->aead_out), &c_key) ||
+								this->aead_out->get_encr_key_size(this->aead_out),
+								&c_key) ||
 		!this->hkdf->derive_iv(this->hkdf, FALSE,
-							   this->aead_out->
-							   get_iv_size(this->aead_out), &c_iv))
+							   this->aead_out->get_iv_size(this->aead_out),
+							   &c_iv))
 	{
 		DBG1(DBG_TLS, "deriving client key material failed");
-		chunk_clear(&c_key);
-		chunk_clear(&c_iv);
-		return FALSE;
+		goto out;
 	}
 
-	/* server key material */
-	if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_S_HS_TRAFFIC,
-	   this->handshake, NULL) ||
-	   !this->hkdf->derive_key(this->hkdf, TRUE, this->aead_in->
-	   get_encr_key_size(this->aead_in), &s_key) ||
-	   !this->hkdf->derive_iv(this->hkdf, TRUE, this->aead_in->
-	   get_iv_size(this->aead_in), &s_iv))
+	if (!this->hkdf->generate_secret(this->hkdf, server_label, this->handshake,
+									 NULL) ||
+		!this->hkdf->derive_key(this->hkdf, TRUE,
+								this->aead_in->get_encr_key_size(this->aead_in),
+								&s_key) ||
+		!this->hkdf->derive_iv(this->hkdf, TRUE,
+							   this->aead_in->get_iv_size(this->aead_in),
+							   &s_iv))
 	{
 		DBG1(DBG_TLS, "deriving server key material failed");
-		chunk_clear(&c_key);
-		chunk_clear(&c_iv);
-		chunk_clear(&s_key);
-		chunk_clear(&s_iv);
-		return FALSE;
+		goto out;
 	}
 
-	if (this->tls->is_server(this->tls))
+	if (!aead_c->set_keys(aead_c, chunk_empty, c_key, c_iv) ||
+		!aead_s->set_keys(aead_s, chunk_empty, s_key, s_iv))
 	{
-		if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-			!this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-		{
-			DBG1(DBG_TLS, "setting aead server key material failed");
-			chunk_clear(&c_key);
-			chunk_clear(&c_iv);
-			chunk_clear(&s_key);
-			chunk_clear(&s_iv);
-			return FALSE;
-		}
-	}
-	else
-	{
-		if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-			!this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-		{
-			DBG1(DBG_TLS, "setting aead client key material failed");
-			chunk_clear(&c_key);
-			chunk_clear(&c_iv);
-			chunk_clear(&s_key);
-			chunk_clear(&s_iv);
-			return FALSE;
-		}
+		DBG1(DBG_TLS, "setting AEAD key material failed");
+		goto out;
 	}
+	success = TRUE;
 
+out:
 	chunk_clear(&c_key);
 	chunk_clear(&c_iv);
 	chunk_clear(&s_key);
 	chunk_clear(&s_iv);
-	return TRUE;
+	return success;
 }
 
-METHOD(tls_crypto_t, derive_app_secret, bool,
-	private_tls_crypto_t *this)
+METHOD(tls_crypto_t, derive_handshake_keys, bool,
+	private_tls_crypto_t *this, chunk_t shared_secret)
 {
-	chunk_t c_key, c_iv, s_key, s_iv;
-
-	/* client key material */
-	if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_C_AP_TRAFFIC,
-									 this->handshake, NULL) ||
-		!this->hkdf->derive_key(this->hkdf, FALSE,
-								this->aead_out->
-								get_encr_key_size(this->aead_out), &c_key) ||
-		!this->hkdf->derive_iv(this->hkdf, FALSE,
-							   this->aead_out->
-							   get_iv_size(this->aead_out), &c_iv))
-	{
-		DBG1(DBG_TLS, "deriving client key material failed");
-		chunk_clear(&c_key);
-		chunk_clear(&c_iv);
-		return FALSE;
-	}
-
-	/* server key material */
-	if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_S_AP_TRAFFIC,
-	   this->handshake, NULL) ||
-	   !this->hkdf->derive_key(this->hkdf, TRUE, this->aead_in->
-	   get_encr_key_size(this->aead_in), &s_key) ||
-	   !this->hkdf->derive_iv(this->hkdf, TRUE, this->aead_in->
-	   get_iv_size(this->aead_in), &s_iv))
-	{
-		DBG1(DBG_TLS, "deriving server key material failed");
-		chunk_clear(&c_key);
-		chunk_clear(&c_iv);
-		chunk_clear(&s_key);
-		chunk_clear(&s_iv);
-		return FALSE;
-	}
-
-	if (this->tls->is_server(this->tls))
-	{
-		if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-			!this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-		{
-			DBG1(DBG_TLS, "setting aead server key material failed");
-			chunk_clear(&c_key);
-			chunk_clear(&c_iv);
-			chunk_clear(&s_key);
-			chunk_clear(&s_iv);
-			return FALSE;
-		}
-	}
-	else
-	{
-		if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-			!this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-		{
-			DBG1(DBG_TLS, "setting aead client key material failed");
-			chunk_clear(&c_key);
-			chunk_clear(&c_iv);
-			chunk_clear(&s_key);
-			chunk_clear(&s_iv);
-			return FALSE;
-		}
-	}
+	this->hkdf->set_shared_secret(this->hkdf, shared_secret);
+	return derive_labeled_keys(this, TLS_HKDF_C_HS_TRAFFIC,
+							   TLS_HKDF_S_HS_TRAFFIC);
+}
 
-	chunk_clear(&c_key);
-	chunk_clear(&c_iv);
-	chunk_clear(&s_key);
-	chunk_clear(&s_iv);
-	return TRUE;
+METHOD(tls_crypto_t, derive_app_keys, bool,
+	private_tls_crypto_t *this)
+{
+	return derive_labeled_keys(this, TLS_HKDF_C_AP_TRAFFIC,
+							   TLS_HKDF_S_AP_TRAFFIC);
 }
 
 METHOD(tls_crypto_t, resume_session, tls_cipher_suite_t,
@@ -2223,8 +2150,8 @@ tls_crypto_t *tls_crypto_create(tls_t *tls, tls_cache_t *cache)
 			.calculate_finished = _calculate_finished,
 			.calculate_finished_tls13 = _calculate_finished_tls13,
 			.derive_secrets = _derive_secrets,
-			.derive_handshake_secret = _derive_handshake_secret,
-			.derive_app_secret = _derive_app_secret,
+			.derive_handshake_keys = _derive_handshake_keys,
+			.derive_app_keys = _derive_app_keys,
 			.resume_session = _resume_session,
 			.get_session = _get_session,
 			.change_cipher = _change_cipher,
diff --git a/src/libtls/tls_crypto.h b/src/libtls/tls_crypto.h
index 5cd007241e..f980095ebf 100644
--- a/src/libtls/tls_crypto.h
+++ b/src/libtls/tls_crypto.h
@@ -1,4 +1,9 @@
 /*
+ * Copyright (C) 2020 Tobias Brunner
+ * Copyright (C) 2020 Pascal Knecht
+ * Copyright (C) 2020 Méline Sieber
+ * HSR Hochschule fuer Technik Rapperswil
+ *
  * Copyright (C) 2010 Martin Willi
  * Copyright (C) 2010 revosec AG
  *
@@ -514,7 +519,7 @@ struct tls_crypto_t {
 							 bio_reader_t *reader);
 
 	/**
-	 * Calculate the data of a legacyTLS finished message.
+	 * Calculate the data of a legacy TLS finished message.
 	 *
 	 * @param label			ASCII label to use for calculation
 	 * @param out			buffer to write finished data to
@@ -551,14 +556,14 @@ struct tls_crypto_t {
 	 * @param shared_secret 	input key material
 	 * @return 					TRUE if	secret derived successfully
 	 */
-	bool (*derive_handshake_secret)(tls_crypto_t *this, chunk_t shared_secret);
+	bool (*derive_handshake_keys)(tls_crypto_t *this, chunk_t shared_secret);
 
 	/**
 	 * Derive the application keys.
 	 *
 	 * @return 					TRUE if	secret derived successfully
 	 */
-	bool (*derive_app_secret)(tls_crypto_t *this);
+	bool (*derive_app_keys)(tls_crypto_t *this);
 
 	/**
 	 * Try to resume a TLS session, derive key material.
diff --git a/src/libtls/tls_peer.c b/src/libtls/tls_peer.c
index 870c75573c..96ce374472 100644
--- a/src/libtls/tls_peer.c
+++ b/src/libtls/tls_peer.c
@@ -262,22 +262,21 @@ static status_t process_server_hello(private_tls_peer_t *this,
 
 	if (this->tls->get_version_max(this->tls) == TLS_1_3)
 	{
-		chunk_t shared_secret;
+		chunk_t shared_secret = chunk_empty;
 
-		if (key_type != CURVE_25519 &&
-			!this->dh->set_other_public_value(this->dh, ext_key_share))
+		if (!this->dh->set_other_public_value(this->dh, ext_key_share) ||
+			!this->dh->get_shared_secret(this->dh, &shared_secret) ||
+			!this->crypto->derive_handshake_keys(this->crypto, shared_secret))
 		{
-			DBG2(DBG_TLS, "server key share unable to save");
-		}
-		if (!this->dh->get_shared_secret(this->dh, &shared_secret))
-		{
-			DBG2(DBG_TLS, "No shared secret key found");
+			DBG1(DBG_TLS, "DH key derivation failed");
+			this->alert->add(this->alert, TLS_FATAL, TLS_HANDSHAKE_FAILURE);
+			chunk_clear(&shared_secret);
+			return NEED_MORE;
 		}
+		chunk_clear(&shared_secret);
 
-		if (!this->crypto->derive_handshake_secret(this->crypto, shared_secret))
-		{
-			DBG2(DBG_TLS, "derive handshake traffic secret failed");
-		}
+		this->crypto->change_cipher(this->crypto, TRUE);
+		this->crypto->change_cipher(this->crypto, FALSE);
 	}
 
 	this->state = STATE_HELLO_RECEIVED;
@@ -1537,14 +1536,15 @@ METHOD(tls_handshake_t, build, status_t,
 			case STATE_INIT:
 				return send_client_hello(this, type, writer);
 			case STATE_HELLO_DONE:
-				/* otherwise fall through to next state */
 			case STATE_CIPHERSPEC_CHANGED_OUT:
 			case STATE_FINISHED_RECEIVED:
-				/* fall through since legacy TLS and TLS 1.3
-				* expect the same message */
 				return send_finished(this, type, writer);
 			case STATE_FINISHED_SENT:
-				this->crypto->derive_app_secret(this->crypto);
+				if (!this->crypto->derive_app_keys(this->crypto))
+				{
+					this->alert->add(this->alert, TLS_FATAL, TLS_INTERNAL_ERROR);
+					return NEED_MORE;
+				}
 				this->crypto->change_cipher(this->crypto, TRUE);
 				this->crypto->change_cipher(this->crypto, FALSE);
 				this->state = STATE_FINISHED_SENT_KEY_SWITCHED;