[PATCH 1/2] tls: Pass optional data parameter to sign/verify callbacks

[PATCH 1/2] tls: Pass optional data parameter to sign/verify callbacks

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 15077 bytes --]

Add parameters to optinally provide a buffer with the data to be
signed/verified to the signature algorithm .sign and .verify functions,
in addition to the current get_hash callback parameter.  Currently .sign
and .verify would receive the get_hash callback and call it when
necessary to obtain the hash of the data being signed.

In the case of the handshake messages signature the hash data is part of
the handshake state so there was no need for additional parameters.
When we're hashing the ServerKeyExchange parameters though there's no
need to keep a buffer of the data around for longer than we're building
or processing the message so there's no point copying it to the
handshake state structs.  So instead we pass that buffer to
.sign/.verify which then passes it to the get_hash callbacks.

This removes some duplication and in case of ECDHE gets rid of two
instances of rebuilding the ServerECDHParams struct unnecessarily.
---
This is to illustrate the possible cleanup in DHE to avoid putting a
pointer to a stack structure in the handshake state.
---
 ell/tls-private.h |   3 +
 ell/tls-suites.c  | 198 ++++++++++++++++++++--------------------------
 ell/tls.c         |   6 +-
 3 files changed, 91 insertions(+), 116 deletions(-)

diff --git a/ell/tls-private.h b/ell/tls-private.h
index c82a4d5..134203f 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -59,6 +59,7 @@ struct tls_hash_algorithm {
 extern const struct tls_hash_algorithm tls_handshake_hash_data[];
 
 typedef bool (*tls_get_hash_t)(struct l_tls *tls, uint8_t tls_id,
+				const uint8_t *data, size_t data_len,
 				uint8_t *out, size_t *len,
 				enum l_checksum_type *type);
 
@@ -80,8 +81,10 @@ struct tls_key_exchange_algorithm {
 						const uint8_t *buf, size_t len);
 
 	ssize_t (*sign)(struct l_tls *tls, uint8_t *out, size_t len,
+			const uint8_t *data, size_t data_len,
 			tls_get_hash_t get_hash);
 	bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
+			const uint8_t *data, size_t data_len,
 			tls_get_hash_t get_hash);
 
 	void (*free_params)(struct l_tls *tls);
diff --git a/ell/tls-suites.c b/ell/tls-suites.c
index cb1416a..d3c7f8f 100644
--- a/ell/tls-suites.c
+++ b/ell/tls-suites.c
@@ -157,6 +157,7 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 }
 
 static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
+				const uint8_t *data, size_t data_len,
 				tls_get_hash_t get_hash)
 {
 	ssize_t result = -EMSGSIZE;
@@ -180,7 +181,8 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
 		if (len < 2)	/* Is there space for the algorithm IDs */
 			goto error;
 
-		get_hash(tls, hash_type->tls_id, sign_input, NULL, NULL);
+		get_hash(tls, hash_type->tls_id, data, data_len,
+				sign_input, NULL, NULL);
 		sign_checksum_type = hash_type->l_id;
 		sign_input_len = hash_type->length;
 
@@ -188,8 +190,10 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
 		*ptr++ = 1;	/* RSA_sign */
 		len -= 2;
 	} else {
-		get_hash(tls, 1, sign_input + 0, NULL, NULL);	/* MD5 */
-		get_hash(tls, 2, sign_input + 16, NULL, NULL);	/* SHA1 */
+		/* MD5 */
+		get_hash(tls, 1, data, data_len, sign_input + 0, NULL, NULL);
+		/* SHA1 */
+		get_hash(tls, 2, data, data_len, sign_input + 16, NULL, NULL);
 		sign_checksum_type = L_CHECKSUM_NONE;
 		sign_input_len = 36;
 	}
@@ -214,6 +218,7 @@ error:
 }
 
 static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
+				const uint8_t *data, size_t data_len,
 				tls_get_hash_t get_hash)
 {
 	enum l_checksum_type hash_type;
@@ -255,8 +260,8 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 			return false;
 		}
 
-		if (!get_hash(tls, in[0], expected, &expected_len,
-				&hash_type)) {
+		if (!get_hash(tls, in[0], data, data_len,
+				expected, &expected_len, &hash_type)) {
 			TLS_DISCONNECT(TLS_ALERT_DECRYPT_ERROR, 0,
 					"Unknown hash type %i", in[0]);
 
@@ -281,8 +286,10 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 		 * encoding.
 		 */
 	} else {
-		get_hash(tls, 1, expected + 0, NULL, NULL);	/* MD5 */
-		get_hash(tls, 2, expected + 16, NULL, NULL);	/* SHA1 */
+		/* MD5 */
+		get_hash(tls, 1, data, data_len, expected + 0, NULL, NULL);
+		/* SHA1 */
+		get_hash(tls, 2, data, data_len, expected + 16, NULL, NULL);
 		expected_len = 36;
 		hash_type = L_CHECKSUM_NONE;
 
@@ -322,59 +329,16 @@ static struct tls_key_exchange_algorithm tls_rsa = {
 	.verify = tls_rsa_verify,
 };
 
-struct tls_ecdhe_params {
-	const struct l_ecc_curve *curve;
-	struct l_ecc_scalar *private;
-	struct l_ecc_point *public;
-};
-
-static void tls_free_ecdhe_params(struct l_tls *tls)
-{
-	struct tls_ecdhe_params *params = tls->pending.key_xchg_params;
-
-	if (!params)
-		return;
-
-	tls->pending.key_xchg_params = NULL;
-
-	l_ecc_scalar_free(params->private);
-	l_ecc_point_free(params->public);
-	l_free(params);
-}
-
-static size_t tls_write_ecpoint(uint8_t *buf,
-				const struct tls_named_group *curve,
-				const struct l_ecc_point *point)
-{
-	/* RFC 8422, Section 5.4.1 */
-	buf[0] = 1 + curve->ec.point_bytes;	/* length */
-	buf[1] = 4;				/* form: uncompressed */
-	return 2 + l_ecc_point_get_data(point, buf + 2, curve->ec.point_bytes);
-}
-
-static size_t tls_write_server_ecdh_params(struct l_tls *tls, uint8_t *buf)
-{
-	struct tls_ecdhe_params *params = tls->pending.key_xchg_params;
-
-	/* RFC 8422, Section 5.4 */
-	buf[0] = 3;				/* curve_type: named_curve */
-	l_put_be16(tls->negotiated_curve->id, buf + 1);
-	return 3 + tls_write_ecpoint(buf + 3, tls->negotiated_curve,
-					params->public);
-}
-
-static bool tls_get_server_ecdh_params_hash(struct l_tls *tls, uint8_t tls_id,
-						uint8_t *out, size_t *len,
-						enum l_checksum_type *type)
+/* Used by both DHE and ECDHE */
+static bool tls_get_dh_params_hash(struct l_tls *tls, uint8_t tls_id,
+					const uint8_t *data, size_t data_len,
+					uint8_t *out, size_t *len,
+					enum l_checksum_type *type)
 {
 	unsigned int hash;
 	struct l_checksum *checksum;
-	uint8_t params[1024];
-	size_t params_len;
 	ssize_t hash_len, ret;
 
-	params_len = tls_write_server_ecdh_params(tls, params);
-
 	for (hash = 0; hash < __HANDSHAKE_HASH_COUNT; hash++)
 		if (tls_handshake_hash_data[hash].tls_id == tls_id)
 			break;
@@ -396,7 +360,7 @@ static bool tls_get_server_ecdh_params_hash(struct l_tls *tls, uint8_t tls_id,
 	 */
 	l_checksum_update(checksum, tls->pending.client_random, 32);
 	l_checksum_update(checksum, tls->pending.server_random, 32);
-	l_checksum_update(checksum, params, params_len);
+	l_checksum_update(checksum, data, data_len);
 	ret = l_checksum_get_digest(checksum, out, hash_len);
 	l_checksum_free(checksum);
 
@@ -412,12 +376,54 @@ static bool tls_get_server_ecdh_params_hash(struct l_tls *tls, uint8_t tls_id,
 	return true;
 }
 
+struct tls_ecdhe_params {
+	const struct l_ecc_curve *curve;
+	struct l_ecc_scalar *private;
+	struct l_ecc_point *public;
+};
+
+static void tls_free_ecdhe_params(struct l_tls *tls)
+{
+	struct tls_ecdhe_params *params = tls->pending.key_xchg_params;
+
+	if (!params)
+		return;
+
+	tls->pending.key_xchg_params = NULL;
+
+	l_ecc_scalar_free(params->private);
+	l_ecc_point_free(params->public);
+	l_free(params);
+}
+
+static size_t tls_write_ecpoint(uint8_t *buf,
+				const struct tls_named_group *curve,
+				const struct l_ecc_point *point)
+{
+	/* RFC 8422, Section 5.4.1 */
+	buf[0] = 1 + curve->ec.point_bytes;	/* length */
+	buf[1] = 4;				/* form: uncompressed */
+	return 2 + l_ecc_point_get_data(point, buf + 2, curve->ec.point_bytes);
+}
+
+static size_t tls_write_server_ecdh_params(struct l_tls *tls, uint8_t *buf)
+{
+	struct tls_ecdhe_params *params = tls->pending.key_xchg_params;
+
+	/* RFC 8422, Section 5.4 */
+	buf[0] = 3;				/* curve_type: named_curve */
+	l_put_be16(tls->negotiated_curve->id, buf + 1);
+	return 3 + tls_write_ecpoint(buf + 3, tls->negotiated_curve,
+					params->public);
+}
+
 static bool tls_send_ecdhe_server_key_xchg(struct l_tls *tls)
 {
 	uint8_t buf[1024];
 	uint8_t *ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
 	struct tls_ecdhe_params *params;
 	ssize_t sign_len;
+	const uint8_t *server_ecdh_params_ptr;
 
 	/*
 	 * RFC 8422, Section 5.4
@@ -438,11 +444,14 @@ static bool tls_send_ecdhe_server_key_xchg(struct l_tls *tls)
 		return false;
 	}
 
+	server_ecdh_params_ptr = ptr;
 	ptr += tls_write_server_ecdh_params(tls, ptr);
 
 	sign_len = tls->pending.cipher_suite->key_xchg->sign(tls, ptr,
-					buf + sizeof(buf) - ptr,
-					tls_get_server_ecdh_params_hash);
+						buf + sizeof(buf) - ptr,
+						server_ecdh_params_ptr,
+						ptr - server_ecdh_params_ptr,
+						tls_get_dh_params_hash);
 	if (sign_len < 0)
 		return false;
 
@@ -458,6 +467,7 @@ static void tls_handle_ecdhe_server_key_xchg(struct l_tls *tls,
 {
 	struct tls_ecdhe_params *params;
 	uint16_t namedcurve;
+	const uint8_t *server_ecdh_params_ptr = buf;
 
 	/* RFC 8422, Section 5.4 */
 
@@ -525,7 +535,9 @@ static void tls_handle_ecdhe_server_key_xchg(struct l_tls *tls,
 	}
 
 	if (!tls->pending.cipher_suite->key_xchg->verify(tls, buf, len,
-					tls_get_server_ecdh_params_hash))
+						server_ecdh_params_ptr,
+						buf - server_ecdh_params_ptr,
+						tls_get_dh_params_hash))
 		return;
 
 	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO_DONE);
@@ -698,8 +710,6 @@ struct tls_dhe_params {
 	struct l_key *generator;
 	struct l_key *private;
 	struct l_key *public;
-	const uint8_t *server_dh_params_buf;
-	size_t server_dh_params_len;
 };
 
 static void tls_free_dhe_params(struct l_tls *tls)
@@ -718,47 +728,6 @@ static void tls_free_dhe_params(struct l_tls *tls)
 	l_free(params);
 }
 
-static bool tls_get_server_dh_params_hash(struct l_tls *tls, uint8_t tls_id,
-						uint8_t *out, size_t *len,
-						enum l_checksum_type *type)
-{
-	unsigned int hash;
-	struct l_checksum *checksum;
-	struct tls_dhe_params *params = tls->pending.key_xchg_params;
-	ssize_t hash_len, ret;
-
-	for (hash = 0; hash < __HANDSHAKE_HASH_COUNT; hash++)
-		if (tls_handshake_hash_data[hash].tls_id == tls_id)
-			break;
-
-	if (hash == __HANDSHAKE_HASH_COUNT)
-		return false;
-
-	hash_len = tls_handshake_hash_data[hash].length;
-
-	checksum = l_checksum_new(tls_handshake_hash_data[hash].l_id);
-	if (!checksum)
-		return false;
-
-	l_checksum_update(checksum, tls->pending.client_random, 32);
-	l_checksum_update(checksum, tls->pending.server_random, 32);
-	l_checksum_update(checksum, params->server_dh_params_buf,
-				params->server_dh_params_len);
-	ret = l_checksum_get_digest(checksum, out, hash_len);
-	l_checksum_free(checksum);
-
-	if (ret != (ssize_t) hash_len)
-		return false;
-
-	if (len)
-		*len = hash_len;
-
-	if (type)
-		*type = tls_handshake_hash_data[hash].l_id;
-
-	return true;
-}
-
 static bool tls_send_dhe_server_key_xchg(struct l_tls *tls)
 {
 	uint8_t buf[1024 + TLS_DHE_MAX_SIZE * 3];
@@ -770,6 +739,7 @@ static bool tls_send_dhe_server_key_xchg(struct l_tls *tls)
 	size_t public_len;
 	unsigned int zeros = 0;
 	ssize_t sign_len;
+	const uint8_t *server_dh_params_ptr;
 
 	params = l_new(struct tls_dhe_params, 1);
 	prime_buf = tls->negotiated_ff_group->ff.prime;
@@ -803,7 +773,7 @@ static bool tls_send_dhe_server_key_xchg(struct l_tls *tls)
 	while (zeros < public_len && public_buf[zeros] == 0x00)
 		zeros++;
 
-	params->server_dh_params_buf = ptr;
+	server_dh_params_ptr = ptr;
 
 	/* RFC 5246, Section 7.4.3 */
 
@@ -819,16 +789,17 @@ static bool tls_send_dhe_server_key_xchg(struct l_tls *tls)
 	memcpy(ptr + 2, public_buf + zeros, public_len - zeros);
 	ptr += 2 + public_len - zeros;
 
-	params->server_dh_params_len = ptr - params->server_dh_params_buf;
-	tls->pending.key_xchg_params = params;
-
 	sign_len = tls->pending.cipher_suite->key_xchg->sign(tls, ptr,
-					buf + sizeof(buf) - ptr,
-					tls_get_server_dh_params_hash);
+						buf + sizeof(buf) - ptr,
+						server_dh_params_ptr,
+						ptr - server_dh_params_ptr,
+						tls_get_dh_params_hash);
 	if (sign_len < 0)
-		return false;
+		goto free_params;
 
 	ptr += sign_len;
+	tls->pending.key_xchg_params = params;
+
 	tls_tx_handshake(tls, TLS_SERVER_KEY_EXCHANGE, buf, ptr - buf);
 	return true;
 
@@ -849,13 +820,12 @@ static void tls_handle_dhe_server_key_xchg(struct l_tls *tls,
 	size_t generator_len;
 	const uint8_t *public_buf;
 	size_t public_len;
+	const uint8_t *server_dh_params_ptr = buf;
 
 	if (len < 2)
 		goto decode_error;
 
 	params = l_new(struct tls_dhe_params, 1);
-	params->server_dh_params_buf = buf;
-
 	params->prime_len = l_get_be16(buf);
 	if (len < 2 + params->prime_len + 2)
 		goto decode_error;
@@ -886,8 +856,6 @@ static void tls_handle_dhe_server_key_xchg(struct l_tls *tls,
 	buf += 2 + public_len;
 	len -= 2 + public_len;
 
-	params->server_dh_params_len = buf - params->server_dh_params_buf;
-
 	/*
 	 * Validate the values received.  Without requiring RFC 7919 from
 	 * the server, and there are many servers that don't implement it
@@ -958,7 +926,9 @@ static void tls_handle_dhe_server_key_xchg(struct l_tls *tls,
 	tls->pending.key_xchg_params = params;
 
 	if (!tls->pending.cipher_suite->key_xchg->verify(tls, buf, len,
-						tls_get_server_dh_params_hash))
+						server_dh_params_ptr,
+						buf - server_dh_params_ptr,
+						tls_get_dh_params_hash))
 		return;
 
 	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO_DONE);
diff --git a/ell/tls.c b/ell/tls.c
index f9e1e95..6e8c968 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -1143,6 +1143,7 @@ static void tls_get_handshake_hash(struct l_tls *tls,
 }
 
 static bool tls_get_handshake_hash_by_id(struct l_tls *tls, uint8_t hash_id,
+					const uint8_t *data, size_t data_len,
 					uint8_t *out, size_t *len,
 					enum l_checksum_type *type)
 {
@@ -1176,7 +1177,7 @@ static bool tls_send_certificate_verify(struct l_tls *tls)
 	sign_len = tls->pending.cipher_suite->key_xchg->sign(tls,
 					buf + TLS_HANDSHAKE_HEADER_SIZE,
 					2048 - TLS_HANDSHAKE_HEADER_SIZE,
-					tls_get_handshake_hash_by_id);
+					NULL, 0, tls_get_handshake_hash_by_id);
 
 	if (sign_len < 0)
 		return false;
@@ -1997,6 +1998,7 @@ static void tls_handle_server_hello_done(struct l_tls *tls,
 }
 
 static bool tls_get_prev_digest_by_id(struct l_tls *tls, uint8_t hash_id,
+					const uint8_t *data, size_t data_len,
 					uint8_t *out, size_t *out_len,
 					enum l_checksum_type *type)
 {
@@ -2026,7 +2028,7 @@ static void tls_handle_certificate_verify(struct l_tls *tls,
 {
 	int i;
 
-	if (!tls->pending.cipher_suite->key_xchg->verify(tls, buf, len,
+	if (!tls->pending.cipher_suite->key_xchg->verify(tls, buf, len, NULL, 0,
 						tls_get_prev_digest_by_id))
 		return;
 
-- 
2.19.1

[PATCH 2/2] tls: Separate signature algorithm and key exchange algorithm

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 27395 bytes --]

Separate definitions of the key exchange algorithms from the signature
algorithm.  DHE and ECDHE can be used with different signature
algorithms hopefully without any changes.
---
 ell/tls-private.h |  24 +--
 ell/tls-suites.c  | 369 +++++++++++++++++++++++++---------------------
 ell/tls.c         |  44 +++---
 3 files changed, 238 insertions(+), 199 deletions(-)

diff --git a/ell/tls-private.h b/ell/tls-private.h
index 134203f..10f25c5 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -63,15 +63,23 @@ typedef bool (*tls_get_hash_t)(struct l_tls *tls, uint8_t tls_id,
 				uint8_t *out, size_t *len,
 				enum l_checksum_type *type);
 
-struct tls_key_exchange_algorithm {
+struct tls_signature_algorithm {
 	uint8_t id;
 
-	bool certificate_check;
+	bool (*validate_cert_key_type)(struct l_cert *cert);
+
+	ssize_t (*sign)(struct l_tls *tls, uint8_t *out, size_t len,
+			const uint8_t *data, size_t data_len,
+			tls_get_hash_t get_hash);
+	bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
+			const uint8_t *data, size_t data_len,
+			tls_get_hash_t get_hash);
+};
+
+struct tls_key_exchange_algorithm {
 	bool need_ecc;
 	bool need_ffdh;
 
-	bool (*validate_cert_key_type)(struct l_cert *cert);
-
 	bool (*send_server_key_exchange)(struct l_tls *tls);
 	void (*handle_server_key_exchange)(struct l_tls *tls,
 						const uint8_t *buf, size_t len);
@@ -80,13 +88,6 @@ struct tls_key_exchange_algorithm {
 	void (*handle_client_key_exchange)(struct l_tls *tls,
 						const uint8_t *buf, size_t len);
 
-	ssize_t (*sign)(struct l_tls *tls, uint8_t *out, size_t len,
-			const uint8_t *data, size_t data_len,
-			tls_get_hash_t get_hash);
-	bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
-			const uint8_t *data, size_t data_len,
-			tls_get_hash_t get_hash);
-
 	void (*free_params)(struct l_tls *tls);
 };
 
@@ -102,6 +103,7 @@ struct tls_cipher_suite {
 	int verify_data_length;
 
 	struct tls_bulk_encryption_algorithm *encryption;
+	struct tls_signature_algorithm *signature;
 	struct tls_key_exchange_algorithm *key_xchg;
 	struct tls_mac_algorithm *mac;
 	enum l_checksum_type prf_hmac;
diff --git a/ell/tls-suites.c b/ell/tls-suites.c
index d3c7f8f..56d8795 100644
--- a/ell/tls-suites.c
+++ b/ell/tls-suites.c
@@ -42,120 +42,6 @@ static bool tls_rsa_validate_cert_key(struct l_cert *cert)
 	return l_cert_get_pubkey_type(cert) == L_CERT_KEY_RSA;
 }
 
-static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
-{
-	uint8_t buf[1024 + 32];
-	uint8_t *ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
-	uint8_t pre_master_secret[48];
-	ssize_t bytes_encrypted;
-
-	if (!tls->peer_pubkey) {
-		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
-				"Peer public key not received");
-
-		return false;
-	}
-
-	/* Must match the version in tls_send_client_hello */
-	pre_master_secret[0] = (uint8_t) (TLS_VERSION >> 8);
-	pre_master_secret[1] = (uint8_t) (TLS_VERSION >> 0);
-
-	l_getrandom(pre_master_secret + 2, 46);
-
-	if (tls->peer_pubkey_size + 32 > (int) sizeof(buf)) {
-		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
-				"Peer public key too big: %zi",
-				tls->peer_pubkey_size);
-
-		return false;
-	}
-
-	l_put_be16(tls->peer_pubkey_size, ptr);
-	bytes_encrypted = l_key_encrypt(tls->peer_pubkey,
-					L_KEY_RSA_PKCS1_V1_5, L_CHECKSUM_NONE,
-					pre_master_secret, ptr + 2, 48,
-					tls->peer_pubkey_size);
-	ptr += tls->peer_pubkey_size + 2;
-
-	if (bytes_encrypted != (ssize_t) tls->peer_pubkey_size) {
-		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
-				"Encrypting PreMasterSecret failed: %s",
-				strerror(-bytes_encrypted));
-
-		return false;
-	}
-
-	tls_tx_handshake(tls, TLS_CLIENT_KEY_EXCHANGE, buf, ptr - buf);
-
-	tls_generate_master_secret(tls, pre_master_secret, 48);
-	memset(pre_master_secret, 0, 48);
-
-	return true;
-}
-
-static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
-						const uint8_t *buf, size_t len)
-{
-	uint8_t pre_master_secret[48], random_secret[46];
-	ssize_t bytes_decrypted;
-
-	if (!tls->priv_key || !tls->priv_key_size) {
-		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
-				"No private key");
-
-		return;
-	}
-
-	if (len != tls->priv_key_size + 2) {
-		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
-				"ClientKeyExchange len %zi not %zi", len,
-				tls->priv_key_size + 2);
-
-		return;
-	}
-
-	len = l_get_be16(buf);
-
-	if (len != tls->priv_key_size) {
-		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
-				"EncryptedPreMasterSecret len %zi not %zi",
-				len, tls->priv_key_size);
-
-		return;
-	}
-
-	bytes_decrypted = l_key_decrypt(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
-					L_CHECKSUM_NONE, buf + 2,
-					pre_master_secret, tls->priv_key_size,
-					48);
-
-	/*
-	 * Assume correct premaster secret client version which according
-	 * to the TLS1.2 spec is unlikely in client implementations SSLv3
-	 * and prior.  Spec suggests either not supporting them or adding
-	 * a configurable override for <= SSLv3 clients.  For now we have
-	 * no need to support them.
-	 *
-	 * On any decode error randomise the Pre Master Secret as per the
-	 * countermeasures in 7.4.7.1 and don't generate any alerts.
-	 */
-	l_getrandom(random_secret, 46);
-
-	pre_master_secret[0] = tls->client_version >> 8;
-	pre_master_secret[1] = tls->client_version >> 0;
-
-	if (bytes_decrypted != 48) {
-		memcpy(pre_master_secret + 2, random_secret, 46);
-
-		TLS_DEBUG("Error decrypting PreMasterSecret: %s",
-				strerror(-bytes_decrypted));
-	}
-
-	tls_generate_master_secret(tls, pre_master_secret, 48);
-	memset(pre_master_secret, 0, 48);
-	memset(random_secret, 0, 46);
-}
-
 static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
 				const uint8_t *data, size_t data_len,
 				tls_get_hash_t get_hash)
@@ -319,16 +205,132 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 	return success;
 }
 
-static struct tls_key_exchange_algorithm tls_rsa = {
+static struct tls_signature_algorithm tls_rsa_signature = {
 	.id = 1, /* RSA_sign */
-	.certificate_check = true,
 	.validate_cert_key_type = tls_rsa_validate_cert_key,
-	.send_client_key_exchange = tls_send_rsa_client_key_xchg,
-	.handle_client_key_exchange = tls_handle_rsa_client_key_xchg,
 	.sign = tls_rsa_sign,
 	.verify = tls_rsa_verify,
 };
 
+static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
+{
+	uint8_t buf[1024 + 32];
+	uint8_t *ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
+	uint8_t pre_master_secret[48];
+	ssize_t bytes_encrypted;
+
+	if (!tls->peer_pubkey) {
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Peer public key not received");
+
+		return false;
+	}
+
+	/* Must match the version in tls_send_client_hello */
+	pre_master_secret[0] = (uint8_t) (TLS_VERSION >> 8);
+	pre_master_secret[1] = (uint8_t) (TLS_VERSION >> 0);
+
+	l_getrandom(pre_master_secret + 2, 46);
+
+	if (tls->peer_pubkey_size + 32 > (int) sizeof(buf)) {
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Peer public key too big: %zi",
+				tls->peer_pubkey_size);
+
+		return false;
+	}
+
+	l_put_be16(tls->peer_pubkey_size, ptr);
+	bytes_encrypted = l_key_encrypt(tls->peer_pubkey,
+					L_KEY_RSA_PKCS1_V1_5, L_CHECKSUM_NONE,
+					pre_master_secret, ptr + 2, 48,
+					tls->peer_pubkey_size);
+	ptr += tls->peer_pubkey_size + 2;
+
+	if (bytes_encrypted != (ssize_t) tls->peer_pubkey_size) {
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Encrypting PreMasterSecret failed: %s",
+				strerror(-bytes_encrypted));
+
+		return false;
+	}
+
+	tls_tx_handshake(tls, TLS_CLIENT_KEY_EXCHANGE, buf, ptr - buf);
+
+	tls_generate_master_secret(tls, pre_master_secret, 48);
+	memset(pre_master_secret, 0, 48);
+
+	return true;
+}
+
+static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
+						const uint8_t *buf, size_t len)
+{
+	uint8_t pre_master_secret[48], random_secret[46];
+	ssize_t bytes_decrypted;
+
+	if (!tls->priv_key || !tls->priv_key_size) {
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
+				"No private key");
+
+		return;
+	}
+
+	if (len != tls->priv_key_size + 2) {
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"ClientKeyExchange len %zi not %zi", len,
+				tls->priv_key_size + 2);
+
+		return;
+	}
+
+	len = l_get_be16(buf);
+
+	if (len != tls->priv_key_size) {
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"EncryptedPreMasterSecret len %zi not %zi",
+				len, tls->priv_key_size);
+
+		return;
+	}
+
+	bytes_decrypted = l_key_decrypt(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
+					L_CHECKSUM_NONE, buf + 2,
+					pre_master_secret, tls->priv_key_size,
+					48);
+
+	/*
+	 * Assume correct premaster secret client version which according
+	 * to the TLS1.2 spec is unlikely in client implementations SSLv3
+	 * and prior.  Spec suggests either not supporting them or adding
+	 * a configurable override for <= SSLv3 clients.  For now we have
+	 * no need to support them.
+	 *
+	 * On any decode error randomise the Pre Master Secret as per the
+	 * countermeasures in 7.4.7.1 and don't generate any alerts.
+	 */
+	l_getrandom(random_secret, 46);
+
+	pre_master_secret[0] = tls->client_version >> 8;
+	pre_master_secret[1] = tls->client_version >> 0;
+
+	if (bytes_decrypted != 48) {
+		memcpy(pre_master_secret + 2, random_secret, 46);
+
+		TLS_DEBUG("Error decrypting PreMasterSecret: %s",
+				strerror(-bytes_decrypted));
+	}
+
+	tls_generate_master_secret(tls, pre_master_secret, 48);
+	memset(pre_master_secret, 0, 48);
+	memset(random_secret, 0, 46);
+}
+
+static struct tls_key_exchange_algorithm tls_rsa_key_xchg = {
+	.send_client_key_exchange = tls_send_rsa_client_key_xchg,
+	.handle_client_key_exchange = tls_handle_rsa_client_key_xchg,
+};
+
 /* Used by both DHE and ECDHE */
 static bool tls_get_dh_params_hash(struct l_tls *tls, uint8_t tls_id,
 					const uint8_t *data, size_t data_len,
@@ -447,15 +449,17 @@ static bool tls_send_ecdhe_server_key_xchg(struct l_tls *tls)
 	server_ecdh_params_ptr = ptr;
 	ptr += tls_write_server_ecdh_params(tls, ptr);
 
-	sign_len = tls->pending.cipher_suite->key_xchg->sign(tls, ptr,
+	if (tls->pending.cipher_suite->signature) {
+		sign_len = tls->pending.cipher_suite->signature->sign(tls, ptr,
 						buf + sizeof(buf) - ptr,
 						server_ecdh_params_ptr,
 						ptr - server_ecdh_params_ptr,
 						tls_get_dh_params_hash);
-	if (sign_len < 0)
-		return false;
+		if (sign_len < 0)
+			return false;
 
-	ptr += sign_len;
+		ptr += sign_len;
+	}
 
 	tls_tx_handshake(tls, TLS_SERVER_KEY_EXCHANGE, buf, ptr - buf);
 
@@ -534,11 +538,16 @@ static void tls_handle_ecdhe_server_key_xchg(struct l_tls *tls,
 		return;
 	}
 
-	if (!tls->pending.cipher_suite->key_xchg->verify(tls, buf, len,
+	if (tls->pending.cipher_suite->signature) {
+		if (!tls->pending.cipher_suite->signature->verify(tls, buf, len,
 						server_ecdh_params_ptr,
 						buf - server_ecdh_params_ptr,
 						tls_get_dh_params_hash))
-		return;
+			return;
+	} else {
+		if (len)
+			goto decode_error;
+	}
 
 	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO_DONE);
 
@@ -687,18 +696,13 @@ decode_error:
 			"ClientKeyExchange decode error");
 }
 
-static struct tls_key_exchange_algorithm tls_ecdhe_rsa = {
-	.id = 1, /* RSA_sign */
-	.certificate_check = true,
+static struct tls_key_exchange_algorithm tls_ecdhe = {
 	.need_ecc = true,
-	.validate_cert_key_type = tls_rsa_validate_cert_key,
 	.send_server_key_exchange = tls_send_ecdhe_server_key_xchg,
 	.handle_server_key_exchange = tls_handle_ecdhe_server_key_xchg,
 	.send_client_key_exchange = tls_send_ecdhe_client_key_xchg,
 	.handle_client_key_exchange = tls_handle_ecdhe_client_key_xchg,
 	.free_params = tls_free_ecdhe_params,
-	.sign = tls_rsa_sign,
-	.verify = tls_rsa_verify,
 };
 
 /* Maximum FF DH prime modulus size in bytes */
@@ -789,15 +793,18 @@ static bool tls_send_dhe_server_key_xchg(struct l_tls *tls)
 	memcpy(ptr + 2, public_buf + zeros, public_len - zeros);
 	ptr += 2 + public_len - zeros;
 
-	sign_len = tls->pending.cipher_suite->key_xchg->sign(tls, ptr,
+	if (tls->pending.cipher_suite->signature) {
+		sign_len = tls->pending.cipher_suite->signature->sign(tls, ptr,
 						buf + sizeof(buf) - ptr,
 						server_dh_params_ptr,
 						ptr - server_dh_params_ptr,
 						tls_get_dh_params_hash);
-	if (sign_len < 0)
-		goto free_params;
+		if (sign_len < 0)
+			goto free_params;
+
+		ptr += sign_len;
+	}
 
-	ptr += sign_len;
 	tls->pending.key_xchg_params = params;
 
 	tls_tx_handshake(tls, TLS_SERVER_KEY_EXCHANGE, buf, ptr - buf);
@@ -925,11 +932,16 @@ static void tls_handle_dhe_server_key_xchg(struct l_tls *tls,
 
 	tls->pending.key_xchg_params = params;
 
-	if (!tls->pending.cipher_suite->key_xchg->verify(tls, buf, len,
-						server_dh_params_ptr,
-						buf - server_dh_params_ptr,
-						tls_get_dh_params_hash))
-		return;
+	if (tls->pending.cipher_suite->signature) {
+		if (!tls->pending.cipher_suite->signature->verify(tls, buf, len,
+							server_dh_params_ptr,
+							buf - server_dh_params_ptr,
+							tls_get_dh_params_hash))
+			return;
+	} else {
+		if (len)
+			goto decode_error;
+	}
 
 	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO_DONE);
 	return;
@@ -1063,18 +1075,13 @@ decode_error:
 			"ClientKeyExchange decode error");
 }
 
-static struct tls_key_exchange_algorithm tls_dhe_rsa = {
-	.id = 1, /* RSA_sign */
-	.certificate_check = true,
+static struct tls_key_exchange_algorithm tls_dhe = {
 	.need_ecc = true,
-	.validate_cert_key_type = tls_rsa_validate_cert_key,
 	.send_server_key_exchange = tls_send_dhe_server_key_xchg,
 	.handle_server_key_exchange = tls_handle_dhe_server_key_xchg,
 	.send_client_key_exchange = tls_send_dhe_client_key_xchg,
 	.handle_client_key_exchange = tls_handle_dhe_client_key_xchg,
 	.free_params = tls_free_dhe_params,
-	.sign = tls_rsa_sign,
-	.verify = tls_rsa_verify,
 };
 
 static struct tls_bulk_encryption_algorithm tls_rc4 = {
@@ -1139,145 +1146,166 @@ static struct tls_cipher_suite tls_rsa_with_rc4_128_md5 = {
 	.verify_data_length = 12,
 	.encryption = &tls_rc4,
 	.mac = &tls_md5,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_rsa_with_rc4_128_sha = {
 	.id = { 0x00, 0x05 },
 	.name = "TLS_RSA_WITH_RC4_128_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_rc4,
 	.mac = &tls_sha,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_rsa_with_3des_ede_cbc_sha = {
 	.id = { 0x00, 0x0a },
 	.name = "TLS_RSA_WITH_3DES_EDE_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_3des_ede,
 	.mac = &tls_sha,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_dhe_rsa_with_3des_ede_cbc_sha = {
 	.id = { 0x00, 0x16 },
 	.name = "TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_3des_ede,
 	.mac = &tls_sha,
-	.key_xchg = &tls_dhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_dhe,
 }, tls_rsa_with_aes_128_cbc_sha = {
 	.id = { 0x00, 0x2f },
 	.name = "TLS_RSA_WITH_AES_128_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128,
 	.mac = &tls_sha,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_dhe_rsa_with_aes_128_cbc_sha = {
 	.id = { 0x00, 0x33 },
 	.name = "TLS_DHE_RSA_WITH_AES_128_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128,
 	.mac = &tls_sha,
-	.key_xchg = &tls_dhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_dhe,
 }, tls_rsa_with_aes_256_cbc_sha = {
 	.id = { 0x00, 0x35 },
 	.name = "TLS_RSA_WITH_AES_256_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256,
 	.mac = &tls_sha,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_dhe_rsa_with_aes_256_cbc_sha = {
 	.id = { 0x00, 0x39 },
 	.name = "TLS_DHE_RSA_WITH_AES_256_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256,
 	.mac = &tls_sha,
-	.key_xchg = &tls_dhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_dhe,
 }, tls_rsa_with_aes_128_cbc_sha256 = {
 	.id = { 0x00, 0x3c },
 	.name = "TLS_RSA_WITH_AES_128_CBC_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128,
 	.mac = &tls_sha256,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_rsa_with_aes_256_cbc_sha256 = {
 	.id = { 0x00, 0x3d },
 	.name = "TLS_RSA_WITH_AES_256_CBC_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256,
 	.mac = &tls_sha256,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_dhe_rsa_with_aes_128_cbc_sha256 = {
 	.id = { 0x00, 0x67 },
 	.name = "TLS_DHE_RSA_WITH_AES_128_CBC_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128,
 	.mac = &tls_sha256,
-	.key_xchg = &tls_dhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_dhe,
 }, tls_dhe_rsa_with_aes_256_cbc_sha256 = {
 	.id = { 0x00, 0x6b },
 	.name = "TLS_DHE_RSA_WITH_AES_256_CBC_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256,
 	.mac = &tls_sha256,
-	.key_xchg = &tls_dhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_dhe,
 }, tls_rsa_with_aes_128_gcm_sha256 = {
 	.id = { 0x00, 0x9c },
 	.name = "TLS_RSA_WITH_AES_128_GCM_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128_gcm,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_rsa_with_aes_256_gcm_sha384 = {
 	.id = { 0x00, 0x9d },
 	.name = "TLS_RSA_WITH_AES_256_GCM_SHA384",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256_gcm,
 	.prf_hmac = L_CHECKSUM_SHA384,
-	.key_xchg = &tls_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_rsa_key_xchg,
 }, tls_dhe_rsa_with_aes_128_gcm_sha256 = {
 	.id = { 0x00, 0x9e },
 	.name = "TLS_DHE_RSA_WITH_AES_128_GCM_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128_gcm,
-	.key_xchg = &tls_dhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_dhe,
 }, tls_dhe_rsa_with_aes_256_gcm_sha384 = {
 	.id = { 0x00, 0x9f },
 	.name = "TLS_DHE_RSA_WITH_AES_256_GCM_SHA384",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256_gcm,
 	.prf_hmac = L_CHECKSUM_SHA384,
-	.key_xchg = &tls_dhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_dhe,
 }, tls_ecdhe_rsa_with_rc4_128_sha = {
 	.id = { 0xc0, 0x11 },
 	.name = "TLS_ECDHE_RSA_WITH_RC4_128_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_rc4,
 	.mac = &tls_sha,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_3des_ede_cbc_sha = {
 	.id = { 0xc0, 0x12 },
 	.name = "TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_3des_ede,
 	.mac = &tls_sha,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_aes_128_cbc_sha = {
 	.id = { 0xc0, 0x13 },
 	.name = "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128,
 	.mac = &tls_sha,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_aes_256_cbc_sha = {
 	.id = { 0xc0, 0x14 },
 	.name = "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256,
 	.mac = &tls_sha,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_aes_128_cbc_sha256 = {
 	.id = { 0xc0, 0x27 },
 	.name = "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128,
 	.mac = &tls_sha256,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_aes_256_cbc_sha384 = {
 	.id = { 0xc0, 0x28 },
 	.name = "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384",
@@ -1285,20 +1313,23 @@ static struct tls_cipher_suite tls_rsa_with_rc4_128_md5 = {
 	.encryption = &tls_aes256,
 	.mac = &tls_sha384,
 	.prf_hmac = L_CHECKSUM_SHA384,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_aes_128_gcm_sha256 = {
 	.id = { 0xc0, 0x2f },
 	.name = "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
 	.verify_data_length = 12,
 	.encryption = &tls_aes128_gcm,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_aes_256_gcm_sha384 = {
 	.id = { 0xc0, 0x30 },
 	.name = "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384",
 	.verify_data_length = 12,
 	.encryption = &tls_aes256_gcm,
 	.prf_hmac = L_CHECKSUM_SHA384,
-	.key_xchg = &tls_ecdhe_rsa,
+	.signature = &tls_rsa_signature,
+	.key_xchg = &tls_ecdhe,
 };
 
 struct tls_cipher_suite *tls_cipher_suite_pref[] = {
diff --git a/ell/tls.c b/ell/tls.c
index 6e8c968..f7d1d73 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -477,14 +477,21 @@ static bool tls_cipher_suite_is_compatible(struct l_tls *tls,
 		return false;
 	}
 
+	/*
+	 * If the certificate is compatible with the signature algorithm it
+	 * also must be compatible with the key exchange mechanism because
+	 * the cipher suites are defined so that the same certificates can
+	 * be used by both.
+	 */
 	leaf = l_certchain_get_leaf(tls->cert);
-	if (leaf && !suite->key_xchg->validate_cert_key_type(leaf)) {
+	if (leaf && suite->signature &&
+			!suite->signature->validate_cert_key_type(leaf)) {
 		if (error) {
 			*error = error_buf;
 			snprintf(error_buf, sizeof(error_buf),
 					"Local certificate has key type "
 					"incompatible with cipher suite %s's "
-					"key xchg mechanism", suite->name);
+					"signature algorithm", suite->name);
 		}
 
 		return false;
@@ -924,7 +931,7 @@ static bool tls_send_certificate(struct l_tls *tls)
 
 	/* TODO: might want check this earlier and exclude the cipher suite */
 	leaf = l_certchain_get_leaf(tls->cert);
-	if (leaf && !tls->pending.cipher_suite->key_xchg->
+	if (leaf && !tls->pending.cipher_suite->signature->
 			validate_cert_key_type(leaf)) {
 		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_CERT_UNKNOWN,
 				"Local certificate has key type incompatible "
@@ -1174,7 +1181,7 @@ static bool tls_send_certificate_verify(struct l_tls *tls)
 
 	/* Fill in the Certificate Verify body */
 
-	sign_len = tls->pending.cipher_suite->key_xchg->sign(tls,
+	sign_len = tls->pending.cipher_suite->signature->sign(tls,
 					buf + TLS_HANDSHAKE_HEADER_SIZE,
 					2048 - TLS_HANDSHAKE_HEADER_SIZE,
 					NULL, 0, tls_get_handshake_hash_by_id);
@@ -1591,7 +1598,7 @@ static void tls_handle_client_hello(struct l_tls *tls,
 
 	l_queue_destroy(extensions_offered, NULL);
 
-	if (tls->pending.cipher_suite->key_xchg->certificate_check && tls->cert)
+	if (tls->pending.cipher_suite->signature && tls->cert)
 		if (!tls_send_certificate(tls))
 			return;
 
@@ -1601,15 +1608,13 @@ static void tls_handle_client_hello(struct l_tls *tls,
 			return;
 
 	/* TODO: don't bother if configured to not authenticate client */
-	if (tls->pending.cipher_suite->key_xchg->certificate_check &&
-			tls->ca_certs)
+	if (tls->pending.cipher_suite->signature && tls->ca_certs)
 		if (!tls_send_certificate_request(tls))
 			return;
 
 	tls_send_server_hello_done(tls);
 
-	if (tls->pending.cipher_suite->key_xchg->certificate_check &&
-			tls->ca_certs)
+	if (tls->pending.cipher_suite->signature && tls->ca_certs)
 		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CERTIFICATE);
 	else
 		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
@@ -1726,7 +1731,7 @@ static void tls_handle_server_hello(struct l_tls *tls,
 
 	TLS_DEBUG("Negotiated %s", tls->pending.compression_method->name);
 
-	if (tls->pending.cipher_suite->key_xchg->certificate_check)
+	if (tls->pending.cipher_suite->signature)
 		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CERTIFICATE);
 	else
 		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
@@ -1807,7 +1812,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 	 * algorithm."
 	 */
 	leaf = l_certchain_get_leaf(certchain);
-	if (!tls->pending.cipher_suite->key_xchg->
+	if (!tls->pending.cipher_suite->signature->
 			validate_cert_key_type(leaf)) {
 		TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_CERT, 0,
 				"Peer certificate key type incompatible with "
@@ -1910,7 +1915,8 @@ static void tls_handle_certificate_request(struct l_tls *tls,
 
 			/* Ignore hash types for signatures other than ours */
 			if (signature_hash_data[i + 1] !=
-					tls->pending.cipher_suite->key_xchg->id)
+					tls->pending.cipher_suite->
+						signature->id)
 				continue;
 
 			if (hash_id == tls_handshake_hash_data[
@@ -2028,7 +2034,8 @@ static void tls_handle_certificate_verify(struct l_tls *tls,
 {
 	int i;
 
-	if (!tls->pending.cipher_suite->key_xchg->verify(tls, buf, len, NULL, 0,
+	if (!tls->pending.cipher_suite->signature->verify(tls, buf, len,
+						NULL, 0,
 						tls_get_prev_digest_by_id))
 		return;
 
@@ -2048,7 +2055,7 @@ static void tls_handle_certificate_verify(struct l_tls *tls,
 	 *   - If tls->ca_certs is non-NULL then tls_handle_certificate
 	 *     will have checked the whole certificate chain to be valid and
 	 *     additionally trusted by our CAs if known.
-	 *   - Additionally cipher_suite->key_xchg->verify has just confirmed
+	 *   - Additionally cipher_suite->signature->verify has just confirmed
 	 *     that the peer owns the end-entity certificate because it was
 	 *     able to sign the contents of the handshake messages and that
 	 *     signature could be verified with the public key from that
@@ -2253,8 +2260,7 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 		 */
 		if (tls->state != TLS_HANDSHAKE_WAIT_HELLO_DONE ||
 				tls->cert_requested ||
-				!tls->pending.cipher_suite->key_xchg->
-				certificate_check) {
+				!tls->pending.cipher_suite->signature) {
 			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
 					"Message invalid in current state "
 					"or certificate check not supported "
@@ -2307,7 +2313,7 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 		/*
 		 * If we accepted a client Certificate message with a
 		 * certificate that has signing capability (TODO: check
-		 * usage bitmask), Certiifcate Verify is received next.  It
+		 * usage bitmask), Certificate Verify is received next.  It
 		 * sounds as if this is mandatory for the client although
 		 * this isn't 100% clear.
 		 */
@@ -2374,8 +2380,8 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 		 *      ServerKeyExchange parameters using its certified key
 		 *      pair.
 		 */
-		if (!tls->server && tls->cipher_suite[0]->key_xchg->
-				certificate_check && tls->ca_certs)
+		if (!tls->server && tls->cipher_suite[0]->signature &&
+				tls->ca_certs)
 			tls->peer_authenticated = true;
 
 		tls_finished(tls);
-- 
2.19.1

Re: [PATCH 1/2] tls: Pass optional data parameter to sign/verify callbacks

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4445 bytes --]

Hi Andrew,

On 01/11/2019 07:14 AM, Andrew Zaborowski wrote:
> Add parameters to optinally provide a buffer with the data to be
> signed/verified to the signature algorithm .sign and .verify functions,
> in addition to the current get_hash callback parameter.  Currently .sign
> and .verify would receive the get_hash callback and call it when
> necessary to obtain the hash of the data being signed.
> 
> In the case of the handshake messages signature the hash data is part of
> the handshake state so there was no need for additional parameters.
> When we're hashing the ServerKeyExchange parameters though there's no
> need to keep a buffer of the data around for longer than we're building
> or processing the message so there's no point copying it to the
> handshake state structs.  So instead we pass that buffer to
> .sign/.verify which then passes it to the get_hash callbacks.
> 
> This removes some duplication and in case of ECDHE gets rid of two
> instances of rebuilding the ServerECDHParams struct unnecessarily.
> ---
> This is to illustrate the possible cleanup in DHE to avoid putting a
> pointer to a stack structure in the handshake state.
> ---
>   ell/tls-private.h |   3 +
>   ell/tls-suites.c  | 198 ++++++++++++++++++++--------------------------
>   ell/tls.c         |   6 +-
>   3 files changed, 91 insertions(+), 116 deletions(-)

So I like this approach way more than the current situation.

> 
> diff --git a/ell/tls-private.h b/ell/tls-private.h
> index c82a4d5..134203f 100644
> --- a/ell/tls-private.h
> +++ b/ell/tls-private.h
> @@ -59,6 +59,7 @@ struct tls_hash_algorithm {
>   extern const struct tls_hash_algorithm tls_handshake_hash_data[];
>   
>   typedef bool (*tls_get_hash_t)(struct l_tls *tls, uint8_t tls_id,
> +				const uint8_t *data, size_t data_len,
>   				uint8_t *out, size_t *len,

Can we call this out_len to make this clearer?

>   				enum l_checksum_type *type);

So one question I have is whether this type argument is needed.  It 
seems like you only ever use it in 1 place, and pass in NULL everywhere 
else.

You could simply add a convenience method to lookup l_checksum from a 
tls_id, no?

>   
> @@ -80,8 +81,10 @@ struct tls_key_exchange_algorithm {
>   						const uint8_t *buf, size_t len);
>   
>   	ssize_t (*sign)(struct l_tls *tls, uint8_t *out, size_t len,

Can we name this out_len to make it clearer

> +			const uint8_t *data, size_t data_len,

These maybe might need to come after the get_hash, since our general 
pattern is callback, userdata, not the other way around.

>   			tls_get_hash_t get_hash);
>   	bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
> +			const uint8_t *data, size_t data_len,
>   			tls_get_hash_t get_hash);

As above

>   
>   	void (*free_params)(struct l_tls *tls);
> diff --git a/ell/tls-suites.c b/ell/tls-suites.c
> index cb1416a..d3c7f8f 100644
> --- a/ell/tls-suites.c
> +++ b/ell/tls-suites.c

<snip>

> @@ -188,8 +190,10 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
>   		*ptr++ = 1;	/* RSA_sign */
>   		len -= 2;
>   	} else {
> -		get_hash(tls, 1, sign_input + 0, NULL, NULL);	/* MD5 */
> -		get_hash(tls, 2, sign_input + 16, NULL, NULL);	/* SHA1 */
> +		/* MD5 */
> +		get_hash(tls, 1, data, data_len, sign_input + 0, NULL, NULL);
> +		/* SHA1 */
> +		get_hash(tls, 2, data, data_len, sign_input + 16, NULL, NULL);

So for example, here this can be simplified to:

get_hash((tls, L_CHECKSUM_SHA1, data, data_len, sign_input + 16, NULL);
etc

>   		sign_checksum_type = L_CHECKSUM_NONE;
>   		sign_input_len = 36;
>   	}
> @@ -214,6 +218,7 @@ error:
>   }
>   
>   static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
> +				const uint8_t *data, size_t data_len,
>   				tls_get_hash_t get_hash)
>   {
>   	enum l_checksum_type hash_type;
> @@ -255,8 +260,8 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
>   			return false;
>   		}
>   
> -		if (!get_hash(tls, in[0], expected, &expected_len,
> -				&hash_type)) {
> +		if (!get_hash(tls, in[0], data, data_len,
> +				expected, &expected_len, &hash_type)) {

And here you'd lookup hash_type first...

>   			TLS_DISCONNECT(TLS_ALERT_DECRYPT_ERROR, 0,
>   					"Unknown hash type %i", in[0]);
>   

The rest seems fine to me.

Regards,
-Denis

Re: [PATCH 1/2] tls: Pass optional data parameter to sign/verify callbacks

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2451 bytes --]

On Thu, 17 Jan 2019 at 18:03, Denis Kenzior <denkenz@gmail.com> wrote:
> On 01/11/2019 07:14 AM, Andrew Zaborowski wrote:
> > This is to illustrate the possible cleanup in DHE to avoid putting a
> > pointer to a stack structure in the handshake state.
> > ---
> >   ell/tls-private.h |   3 +
> >   ell/tls-suites.c  | 198 ++++++++++++++++++++--------------------------
> >   ell/tls.c         |   6 +-
> >   3 files changed, 91 insertions(+), 116 deletions(-)
>
> So I like this approach way more than the current situation.
>
> >
> > diff --git a/ell/tls-private.h b/ell/tls-private.h
> > index c82a4d5..134203f 100644
> > --- a/ell/tls-private.h
> > +++ b/ell/tls-private.h
> > @@ -59,6 +59,7 @@ struct tls_hash_algorithm {
> >   extern const struct tls_hash_algorithm tls_handshake_hash_data[];
> >
> >   typedef bool (*tls_get_hash_t)(struct l_tls *tls, uint8_t tls_id,
> > +                             const uint8_t *data, size_t data_len,
> >                               uint8_t *out, size_t *len,
>
> Can we call this out_len to make this clearer?

Ok.

>
> >                               enum l_checksum_type *type);
>
> So one question I have is whether this type argument is needed.  It
> seems like you only ever use it in 1 place, and pass in NULL everywhere
> else.
>
> You could simply add a convenience method to lookup l_checksum from a
> tls_id, no?

Yeah, I was also wondering why I wasn't doing the mapping to the
l_checksum_type inside the caller, I'm going to re-check this and
possibly convert the signature.

>
> >
> > @@ -80,8 +81,10 @@ struct tls_key_exchange_algorithm {
> >                                               const uint8_t *buf, size_t len);
> >
> >       ssize_t (*sign)(struct l_tls *tls, uint8_t *out, size_t len,
>
> Can we name this out_len to make it clearer

Ok.

>
> > +                     const uint8_t *data, size_t data_len,
>
> These maybe might need to come after the get_hash, since our general
> pattern is callback, userdata, not the other way around.

Ok I can switch that around although it's a little different from the
other userdata's.

>
> >                       tls_get_hash_t get_hash);
> >       bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
> > +                     const uint8_t *data, size_t data_len,
> >                       tls_get_hash_t get_hash);
>
> As above

Ok.

Best regards