[PATCH 01/11] settings: Fix a format string

[PATCH 01/11] settings: Fix a format string

[Andrew Zaborowski]

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

The %d conersion specifier is for integers.
---
 ell/settings.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/settings.c b/ell/settings.c
index c6cb0ff..e3e8303 100644
--- a/ell/settings.c
+++ b/ell/settings.c
@@ -1068,7 +1068,7 @@ LIB_EXPORT bool l_settings_set_double(struct l_settings *settings,
 {
 	L_AUTO_FREE_VAR(char *, buf);
 
-	buf = l_strdup_printf("%d", in);
+	buf = l_strdup_printf("%f", in);
 
 	return l_settings_set_value(settings, group_name, key, buf);
 }
-- 
2.19.1

[PATCH 02/11] Add format string checking to a few declarations

[Andrew Zaborowski]

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

Add the gcc attribute(format(printf)) to function declarations where
this may be useful.
---
 ell/dbus.h   | 3 ++-
 ell/string.h | 3 ++-
 ell/util.h   | 6 ++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/ell/dbus.h b/ell/dbus.h
index a7c08d2..d3af5d7 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -118,7 +118,8 @@ struct l_dbus_message *l_dbus_message_new_error_valist(
 struct l_dbus_message *l_dbus_message_new_error(
 					struct l_dbus_message *method_call,
 					const char *name,
-					const char *format, ...);
+					const char *format, ...)
+					__attribute__((format(printf, 3, 4)));
 
 struct l_dbus_message *l_dbus_message_ref(struct l_dbus_message *message);
 void l_dbus_message_unref(struct l_dbus_message *message);
diff --git a/ell/string.h b/ell/string.h
index c947c14..c1948ec 100644
--- a/ell/string.h
+++ b/ell/string.h
@@ -42,7 +42,8 @@ struct l_string *l_string_append_fixed(struct l_string *dest, const char *src,
 
 void l_string_append_vprintf(struct l_string *dest,
 					const char *format, va_list args);
-void l_string_append_printf(struct l_string *dest, const char *format, ...);
+void l_string_append_printf(struct l_string *dest, const char *format, ...)
+					__attribute__((format(printf, 2, 3)));
 
 struct l_string *l_string_truncate(struct l_string *string, size_t new_size);
 
diff --git a/ell/util.h b/ell/util.h
index 381721f..9fd775e 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -258,7 +258,8 @@ static inline void auto_free(void *a)
 
 char *l_strdup(const char *str);
 char *l_strndup(const char *str, size_t max);
-char *l_strdup_printf(const char *format, ...);
+char *l_strdup_printf(const char *format, ...)
+			__attribute__((format(printf, 1, 2)));
 char *l_strdup_vprintf(const char *format, va_list args);
 
 size_t l_strlcpy(char* dst, const char *src, size_t len);
@@ -280,7 +281,8 @@ void l_util_hexdumpv(bool in, const struct iovec *iov, size_t n_iov,
 					l_util_hexdump_func_t function,
 					void *user_data);
 void l_util_debug(l_util_hexdump_func_t function, void *user_data,
-						const char *format, ...);
+						const char *format, ...)
+			__attribute__((format(printf, 3, 4)));
 
 const char *l_util_get_debugfs_path(void);
 
-- 
2.19.1

[PATCH 03/11] tls: Add debug logging API

[Andrew Zaborowski]

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

Add a debug logging API similar to that in dbus.h and genl.h.  A lot of
messages are added to report on the negotiation progress (Handshake, Alert
and ChangeCipherSpec messages) and all the possible error situations.
Especially configuration errors -- the most common errors -- should be
clear when looking at the logs.

No Application Data messages (plaintext) or raw TLS record contents
(encrypted) are logged because this can be easily done by the user
code and could add a flood of messages that would make it actually
harder to solve TLS issues.
---
 ell/ell.sym       |   1 +
 ell/tls-private.h |  24 +++
 ell/tls-record.c  |  66 ++++---
 ell/tls.c         | 427 +++++++++++++++++++++++++++++++++++-----------
 ell/tls.h         |   6 +
 5 files changed, 400 insertions(+), 124 deletions(-)

diff --git a/ell/ell.sym b/ell/ell.sym
index f7b17b9..614bc93 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -413,6 +413,7 @@ global:
 	l_tls_set_cacert;
 	l_tls_set_auth_data;
 	l_tls_alert_to_str;
+	l_tls_set_debug;
 	/* uintset */
 	l_uintset_new_from_range;
 	l_uintset_new;
diff --git a/ell/tls-private.h b/ell/tls-private.h
index 8cf5be9..4f680a0 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -48,6 +48,7 @@ struct tls_hash_algorithm {
 	uint8_t tls_id;
 	enum l_checksum_type l_id;
 	size_t length;
+	const char *name;
 };
 
 typedef bool (*tls_get_hash_t)(struct l_tls *tls, uint8_t tls_id,
@@ -89,6 +90,7 @@ struct tls_cipher_suite {
 
 struct tls_compression_method {
 	int id;
+	const char *name;
 };
 
 enum tls_handshake_state {
@@ -136,6 +138,9 @@ struct l_tls {
 	l_tls_ready_cb_t ready_handle;
 	l_tls_disconnect_cb_t disconnected;
 	void *user_data;
+	l_tls_debug_cb_t debug_handler;
+	l_tls_destroy_cb_t debug_destroy;
+	void *debug_data;
 
 	char *ca_cert_path;
 	char *cert_path;
@@ -254,3 +259,22 @@ void tls_prf_get_bytes(struct l_tls *tls,
 				const char *label,
 				const void *seed, size_t seed_len,
 				uint8_t *buf, size_t len);
+
+#define TLS_DEBUG(fmt, args...)	\
+	l_util_debug(tls->debug_handler, tls->debug_data, "%s:%i " fmt,	\
+			__func__, __LINE__, ## args)
+#define TLS_SET_STATE(new_state)	\
+	do {	\
+		TLS_DEBUG("New state %s",	\
+				tls_handshake_state_to_str(new_state));	\
+		tls->state = new_state;	\
+	} while (0)
+#define TLS_DISCONNECT(desc, local_desc, fmt, args...)	\
+	do {	\
+		TLS_DEBUG("Disconnect desc=%s local-desc=%s reason=" fmt,\
+				l_tls_alert_to_str(desc),	\
+				l_tls_alert_to_str(local_desc), ## args);\
+		tls_disconnect(tls, desc, local_desc);	\
+	} while (0)
+
+const char *tls_handshake_state_to_str(enum tls_handshake_state state);
diff --git a/ell/tls-record.c b/ell/tls-record.c
index 0ad03ed..8038049 100644
--- a/ell/tls-record.c
+++ b/ell/tls-record.c
@@ -208,13 +208,9 @@ void tls_tx_record(struct l_tls *tls, enum tls_content_type type,
 static bool tls_handle_plaintext(struct l_tls *tls, const uint8_t *plaintext,
 					int len, uint8_t type, uint16_t version)
 {
-	int header_len, need_len;
-	int chunk_len;
-
-	uint32_t hs_len;
-
 	if (len > (1 << 14)) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"Plaintext message too long: %i", len);
 		return false;
 	}
 
@@ -246,30 +242,39 @@ static bool tls_handle_plaintext(struct l_tls *tls, const uint8_t *plaintext,
 		break;
 
 	default:
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"Unknown content type %i", type);
 		return false;
 	}
 
 	if (tls->message_buf_len && type != tls->message_content_type) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"Message fragment type %i doesn't match "
+				"previous type %i", type,
+				tls->message_content_type);
 		return false;
 	}
 
 	tls->message_content_type = type;
 
 	while (1) {
+		int header_len, need_len;
+		int chunk_len;
+
 		/* Do we have a full header in tls->message_buf? */
 		header_len = (type == TLS_CT_ALERT) ? 2 : 4;
 		need_len = header_len;
 
 		if (tls->message_buf_len >= header_len) {
 			if (type == TLS_CT_HANDSHAKE) {
-				hs_len = (tls->message_buf[1] << 16) |
+				uint32_t hs_len = (tls->message_buf[1] << 16) |
 					(tls->message_buf[2] << 8) |
 					(tls->message_buf[3] << 0);
 				if (hs_len > (1 << 14)) {
-					tls_disconnect(tls,
-						TLS_ALERT_DECODE_ERROR, 0);
+					TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR,
+							0, "Handshake message "
+							"too long: %i",
+							(int) hs_len);
 					return false;
 				}
 
@@ -333,19 +338,22 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 	fragment_len = l_get_be16(tls->record_buf + 3);
 
 	if (fragment_len > (1 << 14) + 2048) {
-		tls_disconnect(tls, TLS_ALERT_RECORD_OVERFLOW, 0);
+		TLS_DISCONNECT(TLS_ALERT_RECORD_OVERFLOW, 0,
+				"Record fragment too long: %u", fragment_len);
 		return false;
 	}
 
 	if ((tls->negotiated_version && tls->negotiated_version != version) ||
 			(!tls->negotiated_version &&
 			 tls->record_buf[1] != 0x03 /* Appending E.1 */)) {
-		tls_disconnect(tls, TLS_ALERT_PROTOCOL_VERSION, 0);
+		TLS_DISCONNECT(TLS_ALERT_PROTOCOL_VERSION, 0,
+				"Record version mismatch: %02x", version);
 		return false;
 	}
 
 	if (fragment_len < tls->mac_length[0]) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"Record fragment too short: %u", fragment_len);
 		return false;
 	}
 
@@ -366,7 +374,8 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 		else if (!l_cipher_decrypt(tls->cipher[0], tls->record_buf + 5,
 						compressed + 13,
 						cipher_output_len)) {
-			tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+					"Decrypting record fragment failed");
 			return false;
 		}
 
@@ -376,7 +385,8 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 
 		if (memcmp(mac_buf, compressed + 13 + compressed_len,
 							tls->mac_length[0])) {
-			tls_disconnect(tls, TLS_ALERT_BAD_RECORD_MAC, 0);
+			TLS_DISCONNECT(TLS_ALERT_BAD_RECORD_MAC, 0,
+					"Record fragment MAC mismatch");
 			return false;
 		}
 
@@ -390,7 +400,9 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 			i = tls->record_iv_length[0];
 
 		if (fragment_len <= tls->mac_length[0] + i) {
-			tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+					"Record fragment too short: %u",
+					fragment_len);
 			return false;
 		}
 
@@ -402,7 +414,11 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 			 * should be returned here but that was declared
 			 * unsafe in the TLS 1.1 spec.
 			 */
-			tls_disconnect(tls, TLS_ALERT_BAD_RECORD_MAC, 0);
+			TLS_DISCONNECT(TLS_ALERT_BAD_RECORD_MAC, 0,
+					"Fragment data len %i not a multiple "
+					"of block length %zi",
+					cipher_output_len,
+					tls->block_length[0]);
 			return false;
 		}
 
@@ -410,22 +426,23 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 			if (!l_cipher_set_iv(tls->cipher[0],
 						tls->record_buf + 5,
 						tls->record_iv_length[0])) {
-				tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-						0);
+				TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+						"Setting fragment IV failed");
 				return false;
 			}
 		} else if (tls->negotiated_version >= TLS_V11)
 			if (!l_cipher_decrypt(tls->cipher[0],
 						tls->record_buf + 5, iv,
 						tls->record_iv_length[0])) {
-				tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-						0);
+				TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+						"Setting fragment IV failed");
 				return false;
 			}
 
 		if (!l_cipher_decrypt(tls->cipher[0], tls->record_buf + 5 + i,
 					compressed + 13, cipher_output_len)) {
-			tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+					"Fragment decryption failed");
 			return false;
 		}
 
@@ -469,7 +486,8 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 		if ((tls->mac_length[0] && memcmp(mac_buf, compressed + 13 +
 					compressed_len, tls->mac_length[0])) ||
 				error) {
-			tls_disconnect(tls, TLS_ALERT_BAD_RECORD_MAC, 0);
+			TLS_DISCONNECT(TLS_ALERT_BAD_RECORD_MAC, 0,
+					"Record fragment MAC mismatch");
 			return false;
 		}
 
diff --git a/ell/tls.c b/ell/tls.c
index 3528ceb..4b37785 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -26,6 +26,7 @@
 #include <time.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <stdio.h>
 
 #include "util.h"
 #include "private.h"
@@ -185,7 +186,7 @@ static void tls_reset_handshake(struct l_tls *tls)
 	for (hash = 0; hash < __HANDSHAKE_HASH_COUNT; hash++)
 		tls_drop_handshake_hash(tls, hash);
 
-	tls->state = TLS_HANDSHAKE_WAIT_HELLO;
+	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO);
 	tls->cert_requested = 0;
 	tls->cert_sent = 0;
 }
@@ -197,11 +198,13 @@ static void tls_cleanup_handshake(struct l_tls *tls)
 	memset(tls->pending.master_secret, 0, 48);
 }
 
-static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx)
+static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx,
+					const char **error)
 {
 	struct tls_bulk_encryption_algorithm *enc;
 	struct tls_mac_algorithm *mac;
 	int key_offset;
+	char error_buf[50];
 
 	if (tls->cipher[txrx]) {
 		l_cipher_free(tls->cipher[txrx]);
@@ -240,8 +243,17 @@ static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx)
 		/* Wipe out the now unneeded part of the key block */
 		memset(tls->pending.key_block + key_offset, 0, mac->mac_length);
 
-		if (!tls->mac[txrx])
+		if (!tls->mac[txrx]) {
+			if (error) {
+				*error = error_buf;
+				snprintf(error_buf, sizeof(error_buf),
+						"Can't create %s's %s HMAC",
+						tls->cipher_suite[txrx]->name,
+						txrx ? "Tx" : "Rx");
+			}
+
 			return false;
+		}
 
 		tls->mac_length[txrx] = mac->mac_length;
 
@@ -262,8 +274,17 @@ static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx)
 		/* Wipe out the now unneeded part of the key block */
 		memset(tls->pending.key_block + key_offset, 0, enc->key_length);
 
-		if (!tls->cipher[txrx])
+		if (!tls->cipher[txrx]) {
+			if (error) {
+				*error = error_buf;
+				snprintf(error_buf, sizeof(error_buf),
+						"Can't create %s's %s cipher",
+						tls->cipher_suite[txrx]->name,
+						txrx ? "Tx" : "Rx");
+			}
+
 			return false;
+		}
 
 		tls->cipher_type[txrx] = enc->cipher_type;
 		if (enc->cipher_type == TLS_CIPHER_BLOCK) {
@@ -303,7 +324,7 @@ static void tls_reset_cipher_spec(struct l_tls *tls, bool txrx)
 
 	tls->pending.cipher_suite = NULL;
 
-	tls_change_cipher_spec(tls, txrx);
+	tls_change_cipher_spec(tls, txrx, NULL);
 }
 
 static bool tls_send_rsa_client_key_xchg(struct l_tls *tls);
@@ -440,9 +461,9 @@ static struct tls_cipher_suite *tls_find_cipher_suite(const uint8_t *id)
 }
 
 static struct tls_compression_method tls_compression_pref[] = {
-	/* CompressionMethod.null */
 	{
 		0,
+		"CompressionMethod.null",
 	},
 };
 
@@ -459,9 +480,9 @@ static struct tls_compression_method *tls_find_compression_method(
 }
 
 static const struct tls_hash_algorithm tls_handshake_hash_data[] = {
-	[HANDSHAKE_HASH_SHA256]	= { 4, L_CHECKSUM_SHA256, 32 },
-	[HANDSHAKE_HASH_MD5]	= { 1, L_CHECKSUM_MD5, 16 },
-	[HANDSHAKE_HASH_SHA1]	= { 2, L_CHECKSUM_SHA1, 20 },
+	[HANDSHAKE_HASH_SHA256]	= { 4, L_CHECKSUM_SHA256, 32, "SHA256" },
+	[HANDSHAKE_HASH_MD5]	= { 1, L_CHECKSUM_MD5, 16, "MD5" },
+	[HANDSHAKE_HASH_SHA1]	= { 2, L_CHECKSUM_SHA1, 20, "SHA1" },
 };
 
 static bool tls_init_handshake_hash(struct l_tls *tls)
@@ -469,14 +490,20 @@ static bool tls_init_handshake_hash(struct l_tls *tls)
 	enum handshake_hash_type hash;
 
 	for (hash = 0; hash < __HANDSHAKE_HASH_COUNT; hash++) {
-		if (tls->handshake_hash[hash])
+		if (tls->handshake_hash[hash]) {
+			TLS_DEBUG("Handshake hash %s already exists",
+					tls_handshake_hash_data[hash].name);
 			goto err;
+		}
 
 		tls->handshake_hash[hash] = l_checksum_new(
 					tls_handshake_hash_data[hash].l_id);
 
-		if (!tls->handshake_hash[hash])
+		if (!tls->handshake_hash[hash]) {
+			TLS_DEBUG("Can't create %s hash",
+					tls_handshake_hash_data[hash].name);
 			goto err;
+		}
 	}
 
 	return true;
@@ -500,6 +527,31 @@ enum tls_handshake_type {
 	TLS_FINISHED		= 20,
 };
 
+#define SWITCH_ENUM_TO_STR(val) \
+	case (val):		\
+		return L_STRINGIFY(val);
+
+static const char *tls_handshake_type_to_str(enum tls_handshake_type type)
+{
+	static char buf[40];
+
+	switch (type) {
+	SWITCH_ENUM_TO_STR(TLS_HELLO_REQUEST)
+	SWITCH_ENUM_TO_STR(TLS_CLIENT_HELLO)
+	SWITCH_ENUM_TO_STR(TLS_SERVER_HELLO)
+	SWITCH_ENUM_TO_STR(TLS_CERTIFICATE)
+	SWITCH_ENUM_TO_STR(TLS_SERVER_KEY_EXCHANGE)
+	SWITCH_ENUM_TO_STR(TLS_CERTIFICATE_REQUEST)
+	SWITCH_ENUM_TO_STR(TLS_SERVER_HELLO_DONE)
+	SWITCH_ENUM_TO_STR(TLS_CERTIFICATE_VERIFY)
+	SWITCH_ENUM_TO_STR(TLS_CLIENT_KEY_EXCHANGE)
+	SWITCH_ENUM_TO_STR(TLS_FINISHED)
+	}
+
+	snprintf(buf, sizeof(buf), "tls_handshake_type(%i)", type);
+	return buf;
+}
+
 static const uint8_t pkcs1_digest_info_md5_start[] = {
 	0x30, 0x20, 0x30, 0x0c, 0x06, 0x08, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d,
 	0x02, 0x05, 0x05, 0x00, 0x04, 0x10,
@@ -564,6 +616,9 @@ static void tls_send_alert(struct l_tls *tls, bool fatal,
 {
 	uint8_t buf[2];
 
+	TLS_DEBUG("Sending a %s Alert: %s", fatal ? "Fatal" : "Warning",
+			l_tls_alert_to_str(alert_desc));
+
 	buf[0] = fatal ? 2 : 1;
 	buf[1] = alert_desc;
 
@@ -601,6 +656,10 @@ static void tls_tx_handshake(struct l_tls *tls, int type, uint8_t *buf,
 {
 	int i;
 
+	TLS_DEBUG("Sending a %s of %zi bytes",
+			tls_handshake_type_to_str(type),
+			length - TLS_HANDSHAKE_HEADER_SIZE);
+
 	/* Fill in the handshake header */
 
 	buf[0] = type;
@@ -693,15 +752,18 @@ static bool tls_send_certificate(struct l_tls *tls)
 		cert = NULL;
 
 	if (tls->server && !cert) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-						TLS_ALERT_BAD_CERT);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
+				"Loading server certificate %s failed",
+				tls->cert_path);
 		return false;
 	}
 
 	if (cert && !tls_cert_find_certchain(cert, tls->ca_cert_path)) {
 		if (tls->server) {
-			tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-					TLS_ALERT_UNKNOWN_CA);
+			TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR,
+					TLS_ALERT_UNKNOWN_CA,
+					"Can't find certificate chain local "
+					"CA cert %s", tls->ca_cert_path);
 
 			return false;
 		} else
@@ -711,8 +773,10 @@ static bool tls_send_certificate(struct l_tls *tls)
 	/* TODO: might want check this earlier and exclude the cipher suite */
 	if (cert && !tls->pending.cipher_suite->key_xchg->
 			validate_cert_key_type(cert)) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-				TLS_ALERT_CERT_UNKNOWN);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_CERT_UNKNOWN,
+				"Local certificate has key type incompatible "
+				"with pending cipher suite %s",
+				tls->pending.cipher_suite->name);
 
 		return false;
 	}
@@ -898,7 +962,8 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 	ssize_t bytes_encrypted;
 
 	if (!tls->peer_pubkey) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Peer public key not received");
 
 		return false;
 	}
@@ -908,7 +973,9 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 	l_getrandom(pre_master_secret + 2, 46);
 
 	if (tls->peer_pubkey_size + 32 > (int) sizeof(buf)) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Peer public key too big: %zi",
+				tls->peer_pubkey_size);
 
 		return false;
 	}
@@ -921,7 +988,9 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 	ptr += tls->peer_pubkey_size + 2;
 
 	if (bytes_encrypted != (ssize_t) tls->peer_pubkey_size) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Encrypting PreMasterSecret failed: %s",
+				strerror(-bytes_encrypted));
 
 		return false;
 	}
@@ -946,8 +1015,8 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
 	size_t expected_bytes;
 
 	if (!tls->priv_key || !tls->priv_key_size) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-				TLS_ALERT_BAD_CERT);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
+				"No private key loaded");
 
 		return -ENOKEY;
 	}
@@ -988,7 +1057,9 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
 	}
 
 	if (result < 0)
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Signing the hash failed: %s",
+				strerror(-result));
 
 	return result;
 }
@@ -1011,14 +1082,18 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 
 	if (len < offset + 2 ||
 			(size_t) l_get_be16(in + offset) + offset + 2 != len) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0, "Signature msg too "
+				"short (%zi) or signature length doesn't match",
+				len);
 
 		return false;
 	}
 
 	/* Only the default hash type supported */
 	if (len != offset + 2 + tls->peer_pubkey_size) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"Signature length %zi not equal %zi", len,
+				offset + 2 + tls->peer_pubkey_size);
 
 		return false;
 	}
@@ -1026,13 +1101,16 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 	if (tls->negotiated_version >= TLS_V12) {
 		/* Only RSA supported */
 		if (in[1] != 1 /* RSA_sign */) {
-			tls_disconnect(tls, TLS_ALERT_DECRYPT_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_DECRYPT_ERROR, 0,
+					"Unknown signature algorithm %i",
+					in[1]);
 
 			return false;
 		}
 
 		if (!get_hash(tls, in[0], hash, &hash_len, &hash_type)) {
-			tls_disconnect(tls, TLS_ALERT_DECRYPT_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_DECRYPT_ERROR, 0,
+					"Unknown hash type %i", in[0]);
 
 			return false;
 		}
@@ -1071,7 +1149,10 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 				expected_len, tls->peer_pubkey_size);
 
 	if (!success)
-		tls_disconnect(tls, TLS_ALERT_DECRYPT_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECRYPT_ERROR, 0,
+				"Peer signature verification failed");
+	else
+		TLS_DEBUG("Peer signature verified");
 
 	return success;
 }
@@ -1190,7 +1271,9 @@ static bool tls_verify_finished(struct l_tls *tls, const uint8_t *received,
 	size_t seed_len;
 
 	if (len != (size_t) tls->cipher_suite[0]->verify_data_length) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"TLS_FINISHED length not %i",
+				tls->cipher_suite[0]->verify_data_length);
 
 		return false;
 	}
@@ -1214,7 +1297,8 @@ static bool tls_verify_finished(struct l_tls *tls, const uint8_t *received,
 				tls->cipher_suite[0]->verify_data_length);
 
 	if (memcmp(received, expected, len)) {
-		tls_disconnect(tls, TLS_ALERT_DECRYPT_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECRYPT_ERROR, 0,
+				"TLS_FINISHED contents don't match");
 
 		return false;
 	}
@@ -1306,7 +1390,9 @@ static void tls_handle_client_hello(struct l_tls *tls,
 	tls->client_version = l_get_be16(buf);
 
 	if (tls->client_version < TLS_MIN_VERSION) {
-		tls_disconnect(tls, TLS_ALERT_PROTOCOL_VERSION, 0);
+		TLS_DISCONNECT(TLS_ALERT_PROTOCOL_VERSION, 0,
+				"Client version too low: %02x",
+				tls->client_version);
 		return;
 	}
 
@@ -1319,6 +1405,8 @@ static void tls_handle_client_hello(struct l_tls *tls,
 			if (i != HANDSHAKE_HASH_SHA1 && i != HANDSHAKE_HASH_MD5)
 				tls_drop_handshake_hash(tls, i);
 
+	TLS_DEBUG("Negotiated TLS 1.%i", (tls->negotiated_version & 0xff) - 1);
+
 	/* Select a cipher suite according to client's preference list */
 	while (cipher_suites_size) {
 		/*
@@ -1338,15 +1426,21 @@ static void tls_handle_client_hello(struct l_tls *tls,
 	}
 
 	if (!cipher_suites_size) {
-		tls_disconnect(tls, TLS_ALERT_HANDSHAKE_FAIL, 0);
+		TLS_DISCONNECT(TLS_ALERT_HANDSHAKE_FAIL, 0,
+				"No common cipher suites");
 		return;
 	}
 
+	TLS_DEBUG("Negotiated %s", tls->pending.cipher_suite->name);
+
 	/* Select a compression method */
 
 	/* CompressionMethod.null must be present in the vector */
-	if (!memchr(compression_methods, 0, compression_methods_size))
-		goto decode_error;
+	if (!memchr(compression_methods, 0, compression_methods_size)) {
+		TLS_DISCONNECT(TLS_ALERT_HANDSHAKE_FAIL, 0,
+				"No common compression methods");
+		return;
+	}
 
 	while (compression_methods_size) {
 		tls->pending.compression_method =
@@ -1359,6 +1453,8 @@ static void tls_handle_client_hello(struct l_tls *tls,
 		compression_methods_size--;
 	}
 
+	TLS_DEBUG("Negotiated %s", tls->pending.compression_method->name);
+
 	tls_send_server_hello(tls);
 
 	if (tls->pending.cipher_suite->key_xchg->certificate_check &&
@@ -1376,14 +1472,15 @@ static void tls_handle_client_hello(struct l_tls *tls,
 
 	if (tls->pending.cipher_suite->key_xchg->certificate_check &&
 			tls->ca_cert_path)
-		tls->state = TLS_HANDSHAKE_WAIT_CERTIFICATE;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CERTIFICATE);
 	else
-		tls->state = TLS_HANDSHAKE_WAIT_KEY_EXCHANGE;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
 
 	return;
 
 decode_error:
-	tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+	TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+			"ClientHello decode error");
 }
 
 static void tls_handle_server_hello(struct l_tls *tls,
@@ -1410,7 +1507,8 @@ static void tls_handle_server_hello(struct l_tls *tls,
 	len -= session_id_size + 2 + 1;
 
 	if (len != 0) { /* We know we haven't solicited any extensions */
-		tls_disconnect(tls, TLS_ALERT_UNSUPPORTED_EXTENSION, 0);
+		TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_EXTENSION, 0,
+				"ServerHello contains extensions");
 		return;
 	}
 
@@ -1418,9 +1516,11 @@ static void tls_handle_server_hello(struct l_tls *tls,
 
 	if (tls->negotiated_version < TLS_MIN_VERSION ||
 			tls->negotiated_version > TLS_VERSION) {
-		tls_disconnect(tls, tls->negotiated_version < TLS_MIN_VERSION ?
+		TLS_DISCONNECT(tls->negotiated_version < TLS_MIN_VERSION ?
 				TLS_ALERT_PROTOCOL_VERSION :
-				TLS_ALERT_ILLEGAL_PARAM, 0);
+				TLS_ALERT_ILLEGAL_PARAM, 0,
+				"Unsupported version %02x",
+				tls->negotiated_version);
 		return;
 	}
 
@@ -1430,29 +1530,40 @@ static void tls_handle_server_hello(struct l_tls *tls,
 			if (i != HANDSHAKE_HASH_SHA1 && i != HANDSHAKE_HASH_MD5)
 				tls_drop_handshake_hash(tls, i);
 
+	TLS_DEBUG("Negotiated TLS 1.%i", (tls->negotiated_version & 0xff) - 1);
+
 	/* Set the new cipher suite and compression method structs */
 	tls->pending.cipher_suite = tls_find_cipher_suite(cipher_suite_id);
 	if (!tls->pending.cipher_suite) {
-		tls_disconnect(tls, TLS_ALERT_HANDSHAKE_FAIL, 0);
+		TLS_DISCONNECT(TLS_ALERT_HANDSHAKE_FAIL, 0,
+				"Unknown cipher suite %04x",
+				l_get_be16(cipher_suite_id));
 		return;
 	}
 
+	TLS_DEBUG("Negotiated %s", tls->pending.cipher_suite->name);
+
 	tls->pending.compression_method =
 		tls_find_compression_method(compression_method_id);
 	if (!tls->pending.compression_method) {
-		tls_disconnect(tls, TLS_ALERT_HANDSHAKE_FAIL, 0);
+		TLS_DISCONNECT(TLS_ALERT_HANDSHAKE_FAIL, 0,
+				"Unknown compression method %i",
+				compression_method_id);
 		return;
 	}
 
+	TLS_DEBUG("Negotiated %s", tls->pending.compression_method->name);
+
 	if (tls->pending.cipher_suite->key_xchg->certificate_check)
-		tls->state = TLS_HANDSHAKE_WAIT_CERTIFICATE;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CERTIFICATE);
 	else
-		tls->state = TLS_HANDSHAKE_WAIT_KEY_EXCHANGE;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
 
 	return;
 
 decode_error:
-	tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+	TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+			"ServerHello decode error");
 }
 
 static void tls_handle_certificate(struct l_tls *tls,
@@ -1473,8 +1584,12 @@ static void tls_handle_certificate(struct l_tls *tls,
 	if (total + 3 != len)
 		goto decode_error;
 
-	if (tls_cert_from_certificate_list(buf, total, &certchain) < 0)
-		goto decode_error;
+	if (tls_cert_from_certificate_list(buf, total, &certchain) < 0) {
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"Error decoding peer certificate chain");
+
+		goto done;
+	}
 
 	/*
 	 * "Note that a client MAY send no certificates if it does not have any
@@ -1486,12 +1601,13 @@ static void tls_handle_certificate(struct l_tls *tls,
 	 */
 	if (!certchain) {
 		if (!tls->server) {
-			tls_disconnect(tls, TLS_ALERT_HANDSHAKE_FAIL, 0);
+			TLS_DISCONNECT(TLS_ALERT_HANDSHAKE_FAIL, 0,
+					"Server sent no certificate chain");
 
 			goto done;
 		}
 
-		tls->state = TLS_HANDSHAKE_WAIT_KEY_EXCHANGE;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
 
 		goto done;
 	}
@@ -1504,15 +1620,18 @@ static void tls_handle_certificate(struct l_tls *tls,
 	if (tls->ca_cert_path) {
 		ca_cert = tls_cert_load_file(tls->ca_cert_path);
 		if (!ca_cert) {
-			tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-					TLS_ALERT_BAD_CERT);
+			TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR,
+					TLS_ALERT_BAD_CERT,
+					"Can't load %s", tls->ca_cert_path);
 
 			goto done;
 		}
 	}
 
 	if (!tls_cert_verify_certchain(certchain, ca_cert)) {
-		tls_disconnect(tls, TLS_ALERT_BAD_CERT, 0);
+		TLS_DISCONNECT(TLS_ALERT_BAD_CERT, 0,
+				"Peer certchain verification failed against "
+				"CA cert %s", tls->ca_cert_path);
 
 		goto done;
 	}
@@ -1525,7 +1644,10 @@ static void tls_handle_certificate(struct l_tls *tls,
 	 */
 	if (!tls->pending.cipher_suite->key_xchg->
 			validate_cert_key_type(certchain)) {
-		tls_disconnect(tls, TLS_ALERT_UNSUPPORTED_CERT, 0);
+		TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_CERT, 0,
+				"Peer certificate key type incompatible with "
+				"pending cipher suite %s",
+				tls->pending.cipher_suite->name);
 
 		goto done;
 	}
@@ -1540,7 +1662,8 @@ static void tls_handle_certificate(struct l_tls *tls,
 					tls->peer_cert->size);
 
 	if (!tls->peer_pubkey) {
-		tls_disconnect(tls, TLS_ALERT_UNSUPPORTED_CERT, 0);
+		TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_CERT, 0,
+				"Error loading peer public key to kernel");
 
 		goto done;
 	}
@@ -1548,7 +1671,8 @@ static void tls_handle_certificate(struct l_tls *tls,
 	if (!l_key_get_info(tls->peer_pubkey, L_KEY_RSA_PKCS1_V1_5,
 					L_CHECKSUM_NONE, &tls->peer_pubkey_size,
 					&dummy)) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"Can't l_key_get_info for peer public key");
 
 		goto done;
 	}
@@ -1556,14 +1680,15 @@ static void tls_handle_certificate(struct l_tls *tls,
 	tls->peer_pubkey_size /= 8;
 
 	if (tls->server)
-		tls->state = TLS_HANDSHAKE_WAIT_KEY_EXCHANGE;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
 	else
-		tls->state = TLS_HANDSHAKE_WAIT_HELLO_DONE;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO_DONE);
 
 	goto done;
 
 decode_error:
-	tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+	TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+			"TLS_CERTIFICATE decode error");
 
 done:
 	if (ca_cert)
@@ -1649,7 +1774,8 @@ static void tls_handle_certificate_request(struct l_tls *tls,
 		else if ((int) first_supported != -1)
 			tls->signature_hash = first_supported;
 		else {
-			tls_disconnect(tls, TLS_ALERT_UNSUPPORTED_CERT, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_CERT, 0,
+					"No supported signature hash type");
 
 			return;
 		}
@@ -1672,14 +1798,18 @@ static void tls_handle_certificate_request(struct l_tls *tls,
 	return;
 
 decode_error:
-	tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+	TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+			"CertificateRequest decode error");
 }
 
 static void tls_handle_server_hello_done(struct l_tls *tls,
 						const uint8_t *buf, size_t len)
 {
+	const char *error;
+
 	if (len) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"ServerHello not empty");
 		return;
 	}
 
@@ -1696,14 +1826,15 @@ static void tls_handle_server_hello_done(struct l_tls *tls,
 
 	tls_send_change_cipher_spec(tls);
 
-	if (!tls_change_cipher_spec(tls, 1)) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+	if (!tls_change_cipher_spec(tls, 1, &error)) {
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+				"change_cipher_spec: %s", error);
 		return;
 	}
 
 	tls_send_finished(tls);
 
-	tls->state = TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC;
+	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC);
 }
 
 static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
@@ -1713,14 +1844,16 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 	ssize_t bytes_decrypted;
 
 	if (!tls->priv_key || !tls->priv_key_size) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-				TLS_ALERT_BAD_CERT);
+		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
+				"No private key");
 
 		return;
 	}
 
 	if (len != tls->priv_key_size + 2) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"ClientKeyExchange len %zi not %zi", len,
+				tls->priv_key_size + 2);
 
 		return;
 	}
@@ -1728,7 +1861,9 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 	len = l_get_be16(buf);
 
 	if (len != tls->priv_key_size) {
-		tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+				"EncryptedPreMasterSecret len %zi not %zi",
+				len, tls->priv_key_size);
 
 		return;
 	}
@@ -1753,9 +1888,13 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 	pre_master_secret[0] = tls->client_version >> 8;
 	pre_master_secret[1] = tls->client_version >> 0;
 
-	if (bytes_decrypted != 48)
+	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);
@@ -1819,7 +1958,7 @@ static void tls_handle_certificate_verify(struct l_tls *tls,
 	 */
 	tls->peer_authenticated = true;
 
-	tls->state = TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC;
+	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC);
 }
 
 static void tls_finished(struct l_tls *tls)
@@ -1829,7 +1968,7 @@ static void tls_finished(struct l_tls *tls)
 	/* Free up the resources used in the handshake */
 	tls_reset_handshake(tls);
 
-	tls->state = TLS_HANDSHAKE_DONE;
+	TLS_SET_STATE(TLS_HANDSHAKE_DONE);
 	tls->ready = true;
 
 	tls->ready_handle(peer_identity, tls->user_data);
@@ -1843,15 +1982,20 @@ static void tls_finished(struct l_tls *tls)
 static void tls_handle_handshake(struct l_tls *tls, int type,
 					const uint8_t *buf, size_t len)
 {
+	TLS_DEBUG("Handling a %s of %zi bytes",
+			tls_handshake_type_to_str(type), len);
+
 	switch (type) {
 	case TLS_HELLO_REQUEST:
 		if (tls->server) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in server mode");
 			break;
 		}
 
 		if (len != 0) {
-			tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+					"HelloRequest not empty");
 			break;
 		}
 
@@ -1866,13 +2010,16 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 
 	case TLS_CLIENT_HELLO:
 		if (!tls->server) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in client mode");
 			break;
 		}
 
 		if (tls->state != TLS_HANDSHAKE_WAIT_HELLO &&
 				tls->state != TLS_HANDSHAKE_DONE) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 			break;
 		}
 
@@ -1882,12 +2029,15 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 
 	case TLS_SERVER_HELLO:
 		if (tls->server) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in server mode");
 			break;
 		}
 
 		if (tls->state != TLS_HANDSHAKE_WAIT_HELLO) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 			break;
 		}
 
@@ -1897,7 +2047,9 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 
 	case TLS_CERTIFICATE:
 		if (tls->state != TLS_HANDSHAKE_WAIT_CERTIFICATE) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 			break;
 		}
 
@@ -1914,7 +2066,10 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 				tls->cert_requested ||
 				!tls->pending.cipher_suite->key_xchg->
 				certificate_check) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in current state "
+					"or certificate check not supported "
+					"in pending cipher suite");
 			break;
 		}
 
@@ -1924,7 +2079,9 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 
 	case TLS_SERVER_HELLO_DONE:
 		if (tls->state != TLS_HANDSHAKE_WAIT_HELLO_DONE) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 			break;
 		}
 
@@ -1934,7 +2091,9 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 
 	case TLS_CERTIFICATE_VERIFY:
 		if (tls->state != TLS_HANDSHAKE_WAIT_CERTIFICATE_VERIFY) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 			break;
 		}
 
@@ -1944,12 +2103,15 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 
 	case TLS_CLIENT_KEY_EXCHANGE:
 		if (!tls->server) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in client mode");
 			break;
 		}
 
 		if (tls->state != TLS_HANDSHAKE_WAIT_KEY_EXCHANGE) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 			break;
 		}
 
@@ -1964,15 +2126,17 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 		 * this isn't 100% clear.
 		 */
 		if (tls->peer_pubkey)
-			tls->state = TLS_HANDSHAKE_WAIT_CERTIFICATE_VERIFY;
+			TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CERTIFICATE_VERIFY);
 		else
-			tls->state = TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC;
+			TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC);
 
 		break;
 
 	case TLS_FINISHED:
 		if (tls->state != TLS_HANDSHAKE_WAIT_FINISHED) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Message invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 			break;
 		}
 
@@ -1980,10 +2144,13 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 			break;
 
 		if (tls->server) {
+			const char *error;
+
 			tls_send_change_cipher_spec(tls);
-			if (!tls_change_cipher_spec(tls, 1)) {
-				tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-						0);
+			if (!tls_change_cipher_spec(tls, 1, &error)) {
+				TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+						"change_cipher_spec: %s",
+						error);
 				break;
 			}
 			tls_send_finished(tls);
@@ -2014,7 +2181,8 @@ static void tls_handle_handshake(struct l_tls *tls, int type,
 		break;
 
 	default:
-		tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+		TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+				"Invalid message");
 	}
 }
 
@@ -2048,7 +2216,7 @@ LIB_EXPORT struct l_tls *l_tls_new(bool server,
 		tls_send_client_hello(tls);
 	}
 
-	tls->state = TLS_HANDSHAKE_WAIT_HELLO;
+	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO);
 
 	return tls;
 }
@@ -2078,6 +2246,9 @@ LIB_EXPORT void l_tls_free(struct l_tls *tls)
 	for (hash = 0; hash < __HANDSHAKE_HASH_COUNT; hash++)
 		tls_drop_handshake_hash(tls, hash);
 
+	if (tls->debug_destroy)
+		tls->debug_destroy(tls->debug_data);
+
 	l_free(tls);
 }
 
@@ -2094,35 +2265,42 @@ bool tls_handle_message(struct l_tls *tls, const uint8_t *message,
 			int len, enum tls_content_type type, uint16_t version)
 {
 	enum handshake_hash_type hash;
+	const char *error;
 
 	switch (type) {
 	case TLS_CT_CHANGE_CIPHER_SPEC:
 		if (len != 1 || message[0] != 0x01) {
-			tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+					"ChangeCipherSpec msg decode error");
 
 			return false;
 		}
 
 		if (tls->state != TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"ChangeCipherSpec invalid in state %s",
+					tls_handshake_state_to_str(tls->state));
 
 			return false;
 		}
 
-		if (!tls_change_cipher_spec(tls, 0)) {
-			tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+		if (!tls_change_cipher_spec(tls, 0, &error)) {
+			TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+					"change_cipher_spec: %s", error);
 
 			return false;
 		}
 
-		tls->state = TLS_HANDSHAKE_WAIT_FINISHED;
+		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_FINISHED);
 
 		return true;
 
 	case TLS_CT_ALERT:
 		/* Verify AlertLevel */
 		if (message[0] != 0x01 && message[0] != 0x02) {
-			tls_disconnect(tls, TLS_ALERT_DECODE_ERROR, 0);
+			TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
+					"Received bad AlertLevel %i",
+					message[0]);
 
 			return false;
 		}
@@ -2136,7 +2314,10 @@ bool tls_handle_message(struct l_tls *tls, const uint8_t *message,
 		 * a fatal alert, then disconnect, so we do that
 		 * regardless of the alert level.
 		 */
-		tls_disconnect(tls, TLS_ALERT_CLOSE_NOTIFY, message[1]);
+		TLS_DISCONNECT(TLS_ALERT_CLOSE_NOTIFY, message[1],
+				"Peer sent a %s Alert: %s",
+				message[0] == 0x02 ? "Fatal" : "Warning",
+				l_tls_alert_to_str(message[1]));
 
 		return false;
 
@@ -2196,7 +2377,9 @@ bool tls_handle_message(struct l_tls *tls, const uint8_t *message,
 
 	case TLS_CT_APPLICATION_DATA:
 		if (!tls->ready) {
-			tls_disconnect(tls, TLS_ALERT_UNEXPECTED_MESSAGE, 0);
+			TLS_DISCONNECT(TLS_ALERT_UNEXPECTED_MESSAGE, 0,
+					"Application data message before "
+					"handshake finished");
 
 			return false;
 		}
@@ -2214,11 +2397,13 @@ bool tls_handle_message(struct l_tls *tls, const uint8_t *message,
 
 LIB_EXPORT void l_tls_close(struct l_tls *tls)
 {
-	tls_disconnect(tls, TLS_ALERT_CLOSE_NOTIFY, 0);
+	TLS_DISCONNECT(TLS_ALERT_CLOSE_NOTIFY, 0, "Closing session");
 }
 
 LIB_EXPORT void l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path)
 {
+	TLS_DEBUG("ca-cert-path=%s", ca_cert_path);
+
 	if (tls->ca_cert_path) {
 		l_free(tls->ca_cert_path);
 		tls->ca_cert_path = NULL;
@@ -2232,6 +2417,9 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
 					const char *priv_key_path,
 					const char *priv_key_passphrase)
 {
+	TLS_DEBUG("cert-path=%s priv-key-path=%s priv-key-passphrase=%p",
+			cert_path, priv_key_path, priv_key_passphrase);
+
 	if (tls->cert_path) {
 		l_free(tls->cert_path);
 		tls->cert_path = NULL;
@@ -2251,6 +2439,9 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
 							priv_key_passphrase,
 							NULL,
 							&tls->priv_key_size);
+		TLS_DEBUG("l_pem_load_private_key returned %p, size %zi",
+				priv_key, tls->priv_key_size);
+
 		if (!priv_key)
 			return false;
 
@@ -2262,6 +2453,7 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
 		if (!l_key_get_info(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
 					L_CHECKSUM_NONE, &tls->priv_key_size,
 					&is_public) || is_public) {
+			TLS_DEBUG("Not a private key or l_key_get_info failed");
 			l_key_free(tls->priv_key);
 			tls->priv_key = NULL;
 			tls->priv_key_size = 0;
@@ -2335,6 +2527,25 @@ LIB_EXPORT const char *l_tls_alert_to_str(enum l_tls_alert_desc desc)
 	return NULL;
 }
 
+const char *tls_handshake_state_to_str(enum tls_handshake_state state)
+{
+	static char buf[40];
+
+	switch (state) {
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_WAIT_HELLO)
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_WAIT_CERTIFICATE)
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE)
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_WAIT_HELLO_DONE)
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_WAIT_CERTIFICATE_VERIFY)
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC)
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_WAIT_FINISHED)
+	SWITCH_ENUM_TO_STR(TLS_HANDSHAKE_DONE)
+	}
+
+	snprintf(buf, sizeof(buf), "tls_handshake_state(%i)", state);
+	return buf;
+}
+
 /* X509 Certificates and Certificate Chains */
 
 #define X509_CERTIFICATE_POS			0
@@ -2581,3 +2792,19 @@ enum tls_cert_key_type tls_cert_get_pubkey_type(struct tls_cert *cert)
 
 	return pkcs1_encryption_oids[i].key_type;
 }
+
+LIB_EXPORT bool l_tls_set_debug(struct l_tls *tls, l_tls_debug_cb_t function,
+				void *user_data, l_tls_destroy_cb_t destroy)
+{
+	if (unlikely(!tls))
+		return false;
+
+	if (tls->debug_destroy)
+		tls->debug_destroy(tls->debug_data);
+
+	tls->debug_handler = function;
+	tls->debug_destroy = destroy;
+	tls->debug_data = user_data;
+
+	return true;
+}
diff --git a/ell/tls.h b/ell/tls.h
index 9ab53fa..893fd63 100644
--- a/ell/tls.h
+++ b/ell/tls.h
@@ -60,6 +60,9 @@ typedef void (*l_tls_write_cb_t)(const uint8_t *data, size_t len,
 typedef void (*l_tls_ready_cb_t)(const char *peer_identity, void *user_data);
 typedef void (*l_tls_disconnect_cb_t)(enum l_tls_alert_desc reason,
 					bool remote, void *user_data);
+typedef void (*l_tls_debug_cb_t)(const char *str, void *user_data);
+typedef void (*l_tls_destroy_cb_t)(void *user_data);
+
 
 /*
  * app_data_handler gets called with newly received decrypted data.
@@ -110,6 +113,9 @@ bool l_tls_prf_get_bytes(struct l_tls *tls,
 				size_t hash_len, bool use_master_secret,
 				const char *label, uint8_t *buf, size_t len);
 
+bool l_tls_set_debug(struct l_tls *tls, l_tls_debug_cb_t function,
+			void *user_data, l_tls_destroy_cb_t destroy);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.19.1

[PATCH 04/11] pem: Add l_pem_load_certificate_list utility

[Andrew Zaborowski]

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

l_pem_load_certificate_list loads all the certificates in a file with
multiple concatenated PEM structures.  This could also be done
previously by calling l_pem_load_file with consecutive index values
until it returned an error, this is a faster way to load all of the
certificates.
---
 ell/ell.sym |  1 +
 ell/pem.c   | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 ell/pem.h   |  8 +++++
 3 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/ell/ell.sym b/ell/ell.sym
index 614bc93..2b1d3d7 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -337,6 +337,7 @@ global:
 	l_pem_load_buffer;
 	l_pem_load_file;
 	l_pem_load_certificate;
+	l_pem_load_certificate_list;
 	l_pem_load_private_key;
 	/* pkcs5 */
 	l_pkcs5_pbkdf1;
diff --git a/ell/pem.c b/ell/pem.c
index e24dd7d..d65b99c 100644
--- a/ell/pem.c
+++ b/ell/pem.c
@@ -39,6 +39,7 @@
 #include "asn1-private.h"
 #include "pkcs5-private.h"
 #include "cipher.h"
+#include "queue.h"
 
 #define PEM_START_BOUNDARY	"-----BEGIN "
 #define PEM_END_BOUNDARY	"-----END "
@@ -119,7 +120,8 @@ static bool is_end_boundary(const uint8_t *buf, size_t buf_len,
 }
 
 static uint8_t *pem_load_buffer(const uint8_t *buf, size_t buf_len, int index,
-				char **type_label, size_t *len)
+				char **type_label, size_t *len,
+				const uint8_t **endp)
 {
 	const uint8_t *base64_data = NULL, *label = NULL, *eol;
 	uint8_t *data;
@@ -152,6 +154,9 @@ static uint8_t *pem_load_buffer(const uint8_t *buf, size_t buf_len, int index,
 				*type_label = l_strndup((const char *) label,
 							label_len);
 
+				if (endp)
+					*endp = eol + 1;
+
 				return data;
 			}
 
@@ -178,7 +183,7 @@ LIB_EXPORT uint8_t *l_pem_load_buffer(const uint8_t *buf, size_t buf_len,
 					int index, char **type_label,
 					size_t *out_len)
 {
-	return pem_load_buffer(buf, buf_len, index, type_label, out_len);
+	return pem_load_buffer(buf, buf_len, index, type_label, out_len, NULL);
 }
 
 LIB_EXPORT uint8_t *l_pem_load_file(const char *filename, int index,
@@ -205,7 +210,7 @@ LIB_EXPORT uint8_t *l_pem_load_file(const char *filename, int index,
 		return NULL;
 	}
 
-	result = pem_load_buffer(data, st.st_size, index, type_label, len);
+	result = pem_load_buffer(data, st.st_size, index, type_label, len, NULL);
 
 	munmap(data, st.st_size);
 	close(fd);
@@ -233,6 +238,79 @@ LIB_EXPORT uint8_t *l_pem_load_certificate(const char *filename, size_t *len)
 	return content;
 }
 
+static void pem_element_free(void *data)
+{
+	struct l_pem_list_element *element = data;
+
+	l_free(element->content);
+	l_free(element);
+}
+
+LIB_EXPORT struct l_queue *l_pem_load_certificate_list(const char *filename)
+{
+	int fd;
+	struct stat st;
+	uint8_t *data;
+	size_t start = 0;
+	struct l_queue *result = NULL;
+
+	fd = open(filename, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+
+	if (fstat(fd, &st) < 0) {
+		close(fd);
+
+		return NULL;
+	}
+
+	data = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
+	if (data == MAP_FAILED) {
+		close(fd);
+
+		return NULL;
+	}
+
+	while (1) {
+		uint8_t *certificate;
+		size_t len;
+		char *label;
+		struct l_pem_list_element *element;
+		const uint8_t *end;
+
+		certificate = pem_load_buffer(data + start, st.st_size - start, 0,
+						&label, &len, &end);
+		if (!certificate)
+			goto done;
+
+		if (strcmp(label, "CERTIFICATE")) {
+			l_free(certificate);
+			break;
+		}
+
+		l_free(label);
+		element = l_malloc(sizeof(struct l_pem_list_element));
+		element->content = certificate;
+		element->len = len;
+
+		if (!result)
+			result = l_queue_new();
+
+		l_queue_push_tail(result, element);
+		start = end - data;
+	}
+
+	/* Error occured */
+	l_queue_destroy(result, pem_element_free);
+	result = NULL;
+
+done:
+	munmap(data, st.st_size);
+	close(fd);
+
+	return result;
+}
+
 /**
  * l_pem_load_private_key
  * @filename: path string to the PEM file to load
diff --git a/ell/pem.h b/ell/pem.h
index 93282a3..9cead5c 100644
--- a/ell/pem.h
+++ b/ell/pem.h
@@ -27,12 +27,20 @@
 extern "C" {
 #endif
 
+struct l_queue;
+
+struct l_pem_list_element {
+	uint8_t *content;
+	size_t len;
+};
+
 uint8_t *l_pem_load_buffer(const uint8_t *buf, size_t buf_len,
 				int index, char **type_label, size_t *out_len);
 uint8_t *l_pem_load_file(const char *filename, int index,
 				char **type_label, size_t *len);
 
 uint8_t *l_pem_load_certificate(const char *filename, size_t *len);
+struct l_queue *l_pem_load_certificate_list(const char *filename);
 
 uint8_t *l_pem_load_private_key(const char *filename, const char *passphrase,
 				bool *encrypted, size_t *len);
-- 
2.19.1

Re: [PATCH 04/11] pem: Add l_pem_load_certificate_list utility

[Denis Kenzior]

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

Hi Andrew,

On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
> l_pem_load_certificate_list loads all the certificates in a file with
> multiple concatenated PEM structures.  This could also be done
> previously by calling l_pem_load_file with consecutive index values
> until it returned an error, this is a faster way to load all of the
> certificates.
> ---
>   ell/ell.sym |  1 +
>   ell/pem.c   | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>   ell/pem.h   |  8 +++++
>   3 files changed, 90 insertions(+), 3 deletions(-)
> 
> +static void pem_element_free(void *data)
> +{
> +	struct l_pem_list_element *element = data;
> +
> +	l_free(element->content);
> +	l_free(element);
> +}

So I see that we duplicate this free function a bunch of times in the 
later commits in this series.  Perhaps we should bite the bullet and 
make l_pem an actual first class object with some getters and a destructor.

E.g.

const void *l_pem_get_content(struct l_pem *);
size_t l_pem_get_content_length(struct l_pem *);

void l_pem_free(struct l_pem *);

and possibly:
enum l_pem_type l_pem_get_type(struct l_pem *);

> +
> +LIB_EXPORT struct l_queue *l_pem_load_certificate_list(const char *filename)
> +{
> +	int fd;
> +	struct stat st;
> +	uint8_t *data;
> +	size_t start = 0;
> +	struct l_queue *result = NULL;
> +
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0)
> +		return NULL;
> +
> +	if (fstat(fd, &st) < 0) {
> +		close(fd);
> +
> +		return NULL;
> +	}
> +
> +	data = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
> +	if (data == MAP_FAILED) {
> +		close(fd);
> +
> +		return NULL;
> +	}
> +
> +	while (1) {
> +		uint8_t *certificate;
> +		size_t len;
> +		char *label;
> +		struct l_pem_list_element *element;
> +		const uint8_t *end;
> +
> +		certificate = pem_load_buffer(data + start, st.st_size - start, 0,
> +						&label, &len, &end);
> +		if (!certificate)
> +			goto done;

I'm not so sure about this.  pem_load_buffer can fail for base64 decode 
failure reasons.  So I wouldn't think that you can assume that a failure 
means EOF + success.

> +
> +		if (strcmp(label, "CERTIFICATE")) {
> +			l_free(certificate);
> +			break;
> +		}
> +
> +		l_free(label);
> +		element = l_malloc(sizeof(struct l_pem_list_element));

l_new?

> +		element->content = certificate;
> +		element->len = len;
> +
> +		if (!result)
> +			result = l_queue_new();
> +
> +		l_queue_push_tail(result, element);
> +		start = end - data;

You really need serious mental gymnastics to get this part.  Can't we 
just have a curpos variable or something?

> +	}
> +
> +	/* Error occured */
> +	l_queue_destroy(result, pem_element_free);
> +	result = NULL;
> +
> +done:
> +	munmap(data, st.st_size);
> +	close(fd);
> +
> +	return result;
> +}
> +
>   /**
>    * l_pem_load_private_key
>    * @filename: path string to the PEM file to load
> diff --git a/ell/pem.h b/ell/pem.h
> index 93282a3..9cead5c 100644
> --- a/ell/pem.h
> +++ b/ell/pem.h
> @@ -27,12 +27,20 @@
>   extern "C" {
>   #endif
>   
> +struct l_queue;
> +
> +struct l_pem_list_element {
> +	uint8_t *content;
> +	size_t len;
> +};
> +

As mentioned above, this probably should be a first class opaque data now.

>   uint8_t *l_pem_load_buffer(const uint8_t *buf, size_t buf_len,
>   				int index, char **type_label, size_t *out_len);

and we can change this to

struct l_pem *l_pem_load_buffer(buf, buf_len, index, char **);

I also really wonder if we want to take this function out of the public 
API, though we do need to add support for TPM 1.2 certificates as well.

>   uint8_t *l_pem_load_file(const char *filename, int index,
>   				char **type_label, size_t *len);
>   
>   uint8_t *l_pem_load_certificate(const char *filename, size_t *len);
> +struct l_queue *l_pem_load_certificate_list(const char *filename);
>   
>   uint8_t *l_pem_load_private_key(const char *filename, const char *passphrase,
>   				bool *encrypted, size_t *len);
> 

Regards,
-Denis

Re: [PATCH 04/11] pem: Add l_pem_load_certificate_list utility

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 1 Nov 2018 at 20:40, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
> > +static void pem_element_free(void *data)
> > +{
> > +     struct l_pem_list_element *element = data;
> > +
> > +     l_free(element->content);
> > +     l_free(element);
> > +}
>
> So I see that we duplicate this free function a bunch of times in the
> later commits in this series.  Perhaps we should bite the bullet and
> make l_pem an actual first class object with some getters and a destructor.

So an l_pem would represent a single '----- BEGIN' to '----- END'
fragment of a PEM file?  I'd the rather call it an l_pem_element or
similar, and make l_pem_element_free() public, and could also make the
struct non-public and add accessors but that doesn't help much I
think.

However what I'd really prefer instead is to add back the l_cert type,
which could be a first class object with a lot of functionality and
tls.c would become much shorter.  It could hide the l_key usage (but
could have an accessor for the public key in the certificate) and also
could have l_cert_new_from_pem(), or alternatively pem.c could rather
have a l_pem_load_cert() returning l_cert objects.

Similarly maybe our current l_pem_load_private_key() could be changed
to return an l_key directly.

>
> E.g.
>
> const void *l_pem_get_content(struct l_pem *);
> size_t l_pem_get_content_length(struct l_pem *);
>
> void l_pem_free(struct l_pem *);
>
> and possibly:
> enum l_pem_type l_pem_get_type(struct l_pem *);
>
> > +
> > +LIB_EXPORT struct l_queue *l_pem_load_certificate_list(const char *filename)
> > +{
> > +     int fd;
> > +     struct stat st;
> > +     uint8_t *data;
> > +     size_t start = 0;
> > +     struct l_queue *result = NULL;
> > +
> > +     fd = open(filename, O_RDONLY);
> > +     if (fd < 0)
> > +             return NULL;
> > +
> > +     if (fstat(fd, &st) < 0) {
> > +             close(fd);
> > +
> > +             return NULL;
> > +     }
> > +
> > +     data = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
> > +     if (data == MAP_FAILED) {
> > +             close(fd);
> > +
> > +             return NULL;
> > +     }
> > +
> > +     while (1) {
> > +             uint8_t *certificate;
> > +             size_t len;
> > +             char *label;
> > +             struct l_pem_list_element *element;
> > +             const uint8_t *end;
> > +
> > +             certificate = pem_load_buffer(data + start, st.st_size - start, 0,
> > +                                             &label, &len, &end);
> > +             if (!certificate)
> > +                     goto done;
>
> I'm not so sure about this.  pem_load_buffer can fail for base64 decode
> failure reasons.  So I wouldn't think that you can assume that a failure
> means EOF + success.

I was thinking about the case of having 5 certificates already loaded
and the last one failing -- in this case we might just want to return
the 5 that parsed ok but I can add better error checking.

>
> > +
> > +             if (strcmp(label, "CERTIFICATE")) {
> > +                     l_free(certificate);
> > +                     break;
> > +             }
> > +
> > +             l_free(label);
> > +             element = l_malloc(sizeof(struct l_pem_list_element));
>
> l_new?
>
> > +             element->content = certificate;
> > +             element->len = len;
> > +
> > +             if (!result)
> > +                     result = l_queue_new();
> > +
> > +             l_queue_push_tail(result, element);
> > +             start = end - data;
>
> You really need serious mental gymnastics to get this part.  Can't we
> just have a curpos variable or something?

Ok, if I rename "start" to "curpos" and "end" to "newendpos" or
similar I think it should be pretty clear.

>
> > +     }
> > +
> > +     /* Error occured */
> > +     l_queue_destroy(result, pem_element_free);
> > +     result = NULL;
> > +
> > +done:
> > +     munmap(data, st.st_size);
> > +     close(fd);
> > +
> > +     return result;
> > +}
> > +
> >   /**
> >    * l_pem_load_private_key
> >    * @filename: path string to the PEM file to load
> > diff --git a/ell/pem.h b/ell/pem.h
> > index 93282a3..9cead5c 100644
> > --- a/ell/pem.h
> > +++ b/ell/pem.h
> > @@ -27,12 +27,20 @@
> >   extern "C" {
> >   #endif
> >
> > +struct l_queue;
> > +
> > +struct l_pem_list_element {
> > +     uint8_t *content;
> > +     size_t len;
> > +};
> > +
>
> As mentioned above, this probably should be a first class opaque data now.
>
> >   uint8_t *l_pem_load_buffer(const uint8_t *buf, size_t buf_len,
> >                               int index, char **type_label, size_t *out_len);
>
> and we can change this to
>
> struct l_pem *l_pem_load_buffer(buf, buf_len, index, char **);
>
> I also really wonder if we want to take this function out of the public
> API, though we do need to add support for TPM 1.2 certificates as well.

This could be done separately but I think having a public function to
just get the first PEM fragment in a file is practical (saves you from
loading all of them and freeing all but the first struct.)

Best regards

Re: [PATCH 04/11] pem: Add l_pem_load_certificate_list utility

[Denis Kenzior]

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

Hi Andrew,

> However what I'd really prefer instead is to add back the l_cert type,
> which could be a first class object with a lot of functionality and
> tls.c would become much shorter.  It could hide the l_key usage (but
> could have an accessor for the public key in the certificate) and also
> could have l_cert_new_from_pem(), or alternatively pem.c could rather
> have a l_pem_load_cert() returning l_cert objects.

Yep, lets try doing that.

>>
>> I'm not so sure about this.  pem_load_buffer can fail for base64 decode
>> failure reasons.  So I wouldn't think that you can assume that a failure
>> means EOF + success.
> 
> I was thinking about the case of having 5 certificates already loaded
> and the last one failing -- in this case we might just want to return
> the 5 that parsed ok but I can add better error checking.
> 

No.  Please no.  We never do things this way.  If the parsing was 
partially successful then it was a failure.  Period.  Fail early, get 
the user to address it.  Don't hide problems until they become painful 
to find.

Regards,
-Denis

[PATCH 05/11] tls: Support loading multiple root CAs

[Andrew Zaborowski]

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

If a file with multiple concatenated certificates is supplied to
set_auth_data, load all of them and verify received certificate chains
aginst any of them instead of just the first one.  This is also what
wpa_supplicant does for files supplied with the ca_cert= setting and
it's actually useful.  Some enterprise CAs have multiple root
certificates and provision clients with a file that contains all of them
concatenated and it does happen that they switch from a certificate
chain using one root to a new chain with a different root for their wifi
in which case our network config files break while wpa_supplicant
configs keep working.

Also simplify tls_cert_verify_certchain slightly.
---
 ell/tls-private.h |   3 +-
 ell/tls.c         | 137 ++++++++++++++++++++++++++++++++--------------
 ell/tls.h         |   2 +-
 3 files changed, 100 insertions(+), 42 deletions(-)

diff --git a/ell/tls-private.h b/ell/tls-private.h
index 4f680a0..95b8c65 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -240,6 +240,7 @@ enum tls_cert_key_type {
 };
 
 struct tls_cert *tls_cert_load_file(const char *filename);
+struct l_queue *tls_cert_list_load_file(const char *filename);
 int tls_cert_from_certificate_list(const void *data, size_t len,
 					struct tls_cert **out_certchain);
 
@@ -247,7 +248,7 @@ bool tls_cert_find_certchain(struct tls_cert *cert,
 				const char *cacert_filename);
 
 bool tls_cert_verify_certchain(struct tls_cert *certchain,
-				struct tls_cert *ca_cert);
+				struct l_queue *ca_certs);
 
 void tls_cert_free_certchain(struct tls_cert *cert);
 
diff --git a/ell/tls.c b/ell/tls.c
index 4b37785..fbbc1bf 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -38,6 +38,7 @@
 #include "tls-private.h"
 #include "key.h"
 #include "asn1-private.h"
+#include "queue.h"
 
 void tls10_prf(const void *secret, size_t secret_len,
 		const char *label,
@@ -1571,7 +1572,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 {
 	size_t total;
 	struct tls_cert *certchain = NULL;
-	struct tls_cert *ca_cert = NULL;
+	struct l_queue *ca_certs = NULL;
 	bool dummy;
 
 	if (len < 3)
@@ -1614,12 +1615,12 @@ static void tls_handle_certificate(struct l_tls *tls,
 
 	/*
 	 * Validate the certificate chain's consistency and validate it
-	 * against our CA if we have any.
+	 * against our CAs if we have any.
 	 */
 
 	if (tls->ca_cert_path) {
-		ca_cert = tls_cert_load_file(tls->ca_cert_path);
-		if (!ca_cert) {
+		ca_certs = tls_cert_list_load_file(tls->ca_cert_path);
+		if (!ca_certs) {
 			TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR,
 					TLS_ALERT_BAD_CERT,
 					"Can't load %s", tls->ca_cert_path);
@@ -1628,10 +1629,10 @@ static void tls_handle_certificate(struct l_tls *tls,
 		}
 	}
 
-	if (!tls_cert_verify_certchain(certchain, ca_cert)) {
+	if (!tls_cert_verify_certchain(certchain, ca_certs)) {
 		TLS_DISCONNECT(TLS_ALERT_BAD_CERT, 0,
 				"Peer certchain verification failed against "
-				"CA cert %s", tls->ca_cert_path);
+				"CA cert(s) in %s", tls->ca_cert_path);
 
 		goto done;
 	}
@@ -1691,8 +1692,8 @@ decode_error:
 			"TLS_CERTIFICATE decode error");
 
 done:
-	if (ca_cert)
-		l_free(ca_cert);
+	if (ca_certs)
+		l_queue_destroy(ca_certs, l_free);
 
 	tls_cert_free_certchain(certchain);
 }
@@ -2592,6 +2593,44 @@ struct tls_cert *tls_cert_load_file(const char *filename)
 	return cert;
 }
 
+struct l_queue *tls_cert_list_load_file(const char *filename)
+{
+	struct l_queue *pem_list;
+	struct l_queue *cert_list;
+	bool error = false;
+
+	pem_list = l_pem_load_certificate_list(filename);
+	if (!pem_list)
+		return NULL;
+
+	cert_list = l_queue_new();
+
+	while (!l_queue_isempty(pem_list)) {
+		struct tls_cert *cert;
+		struct l_pem_list_element *elem = l_queue_pop_head(pem_list);
+		uint8_t *der = elem->content;
+
+		if (!elem->len || der[0] != ASN1_ID_SEQUENCE)
+			error = true;
+
+		cert = l_malloc(sizeof(struct tls_cert) + elem->len);
+		cert->size = elem->len;
+		cert->issuer = NULL;
+		memcpy(cert->asn1, der, cert->size);
+		l_queue_push_tail(cert_list, cert);
+		l_free(elem->content);
+		l_free(elem);
+	}
+
+	l_queue_destroy(pem_list, NULL);
+
+	if (!error)
+		return cert_list;
+
+	l_queue_destroy(cert_list, l_free);
+	return NULL;
+}
+
 int tls_cert_from_certificate_list(const void *data, size_t len,
 					struct tls_cert **out_certchain)
 {
@@ -2653,9 +2692,18 @@ static void tls_key_cleanup(struct l_key **p)
 	l_key_free_norevoke(*p);
 }
 
+static bool tls_cert_link(struct tls_cert *cert, struct l_keyring *ring)
+{
+	L_AUTO_CLEANUP_VAR(struct l_key *, key, tls_key_cleanup) = NULL;
+
+	key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
+
+	return key && l_keyring_link(ring, key);
+}
+
 static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
 						struct l_keyring *ring,
-						struct tls_cert *root,
+						struct l_queue *roots,
 						struct l_keyring *trusted)
 {
 	if (!cert)
@@ -2668,19 +2716,30 @@ static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
 	 * specifies the root certificate authority MAY be omitted from
 	 * the chain, under the assumption that the remote end must
 	 * already possess it in order to validate it in any case."
+	 *
+	 * This is an optimization to skip adding the root cert if
+	 * it's already in the trusted keyring.  It also happens to
+	 * work around a kernel issue preventing self-signed certificates
+	 * missing the AKID extension from being linked.
+	 *
+	 * If 'roots' were supplied we assume the keyring is already
+	 * restricted.
 	 */
-	if (!cert->issuer && root && cert->size == root->size &&
-			!memcmp(cert->asn1, root->asn1, root->size))
-		return true;
+	if (!cert->issuer && roots) {
+		const struct l_queue_entry *entry;
 
-	if (tls_cert_verify_with_keyring(cert->issuer, ring, root, trusted)) {
-		L_AUTO_CLEANUP_VAR(struct l_key *, key, tls_key_cleanup);
+		for (entry = l_queue_get_entries(roots); entry;
+				entry = entry->next) {
+			const struct tls_cert *root = entry->data;
 
-		key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
-		if (!key)
-			return false;
+			if (cert->size == root->size && !memcmp(cert->asn1,
+						root->asn1, root->size))
+				return true;
+		}
+	}
 
-		if (!l_keyring_link(ring, key))
+	if (tls_cert_verify_with_keyring(cert->issuer, ring, roots, trusted)) {
+		if (!tls_cert_link(cert, ring))
 			return false;
 
 		if (trusted || cert->issuer)
@@ -2711,46 +2770,44 @@ static void tls_keyring_cleanup(struct l_keyring **p)
 }
 
 bool tls_cert_verify_certchain(struct tls_cert *certchain,
-				struct tls_cert *ca_cert)
+				struct l_queue *ca_certs)
 {
 	L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, tls_keyring_cleanup);
 	L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
 				tls_keyring_cleanup);
 
 	ca_ring = NULL;
-	verify_ring = NULL;
-
-	if (ca_cert) {
-		L_AUTO_CLEANUP_VAR(struct l_key *, ca_key, tls_key_cleanup);
-		ca_key = NULL;
-
-		ca_ring = l_keyring_new();
-		if (!ca_ring)
-			return false;
-
-		ca_key = l_key_new(L_KEY_RSA, ca_cert->asn1, ca_cert->size);
-		if (!ca_key || !l_keyring_link(ca_ring, ca_key))
-			return false;
-	}
 
 	verify_ring = l_keyring_new();
 	if (!verify_ring)
 		return false;
 
 	/*
-	 * If a CA cert was supplied, restrict verify_ring now so
-	 * everything else in certchain is validated against the CA.
+	 * If CA cert(s) were supplied, restrict verify_ring now so
+	 * everything else in certchain is validated against the CA(s).
 	 * Otherwise, verify_ring will be restricted after the root of
 	 * certchain is added to verify_ring by
 	 * tls_cert_verify_with_keyring().
 	 */
-	if (ca_ring && !l_keyring_restrict(verify_ring,
-						L_KEYRING_RESTRICT_ASYM_CHAIN,
-						ca_ring)) {
-		return false;
+	if (ca_certs) {
+		const struct l_queue_entry *entry;
+
+		ca_ring = l_keyring_new();
+		if (!ca_ring)
+			return false;
+
+		for (entry = l_queue_get_entries(ca_certs); entry;
+				entry = entry->next)
+			if (!tls_cert_link(entry->data, ca_ring))
+				return false;
+
+		if (!l_keyring_restrict(verify_ring,
+					L_KEYRING_RESTRICT_ASYM_CHAIN,
+					ca_ring))
+			return false;
 	}
 
-	return tls_cert_verify_with_keyring(certchain, verify_ring, ca_cert,
+	return tls_cert_verify_with_keyring(certchain, verify_ring, ca_certs,
 						ca_ring);
 }
 
diff --git a/ell/tls.h b/ell/tls.h
index 893fd63..8eec360 100644
--- a/ell/tls.h
+++ b/ell/tls.h
@@ -86,7 +86,7 @@ void l_tls_write(struct l_tls *tls, const uint8_t *data, size_t len);
 /* Submit TLS payload from underlying transport to be decrypted */
 void l_tls_handle_rx(struct l_tls *tls, const uint8_t *data, size_t len);
 
-/* If peer is to be authenticated, supply the CA certificate */
+/* If peer is to be authenticated, supply the CA certificate(s) */
 void l_tls_set_cacert(struct l_tls *tls, const char *ca_cert_path);
 
 /*
-- 
2.19.1

Re: [PATCH 05/11] tls: Support loading multiple root CAs

[Denis Kenzior]

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

Hi Andrew,

On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
> If a file with multiple concatenated certificates is supplied to
> set_auth_data, load all of them and verify received certificate chains
> aginst any of them instead of just the first one.  This is also what
> wpa_supplicant does for files supplied with the ca_cert= setting and
> it's actually useful.  Some enterprise CAs have multiple root
> certificates and provision clients with a file that contains all of them
> concatenated and it does happen that they switch from a certificate
> chain using one root to a new chain with a different root for their wifi
> in which case our network config files break while wpa_supplicant
> configs keep working.
> 
> Also simplify tls_cert_verify_certchain slightly.

So mostly just a couple of nitpicks here:

>   
> +struct l_queue *tls_cert_list_load_file(const char *filename)
> +{
> +	struct l_queue *pem_list;
> +	struct l_queue *cert_list;
> +	bool error = false;
> +
> +	pem_list = l_pem_load_certificate_list(filename);
> +	if (!pem_list)
> +		return NULL;
> +
> +	cert_list = l_queue_new();
> +
> +	while (!l_queue_isempty(pem_list)) {
> +		struct tls_cert *cert;
> +		struct l_pem_list_element *elem = l_queue_pop_head(pem_list);
> +		uint8_t *der = elem->content;
> +
> +		if (!elem->len || der[0] != ASN1_ID_SEQUENCE)
> +			error = true;

Can we actually not do that?  Lets bugger out right away.  No sense in 
performing additional work when we know that we've failed already.

> +
> +		cert = l_malloc(sizeof(struct tls_cert) + elem->len);
> +		cert->size = elem->len;
> +		cert->issuer = NULL;
> +		memcpy(cert->asn1, der, cert->size);
> +		l_queue_push_tail(cert_list, cert);
> +		l_free(elem->content);
> +		l_free(elem);

l_pem_free()?

> +	}
> +
> +	l_queue_destroy(pem_list, NULL);
> +
> +	if (!error)
> +		return cert_list;
> +
> +	l_queue_destroy(cert_list, l_free);
> +	return NULL;
> +}
> +
>   int tls_cert_from_certificate_list(const void *data, size_t len,
>   					struct tls_cert **out_certchain)
>   {

Regards,
-Denis

[PATCH 06/11] tools: Update tls_cert_verify_certchain parameters

[Andrew Zaborowski]

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

---
 tools/certchain-verify.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/certchain-verify.c b/tools/certchain-verify.c
index 1800f68..9bd46b1 100644
--- a/tools/certchain-verify.c
+++ b/tools/certchain-verify.c
@@ -87,7 +87,7 @@ static void usage(const char *bin)
 	printf("%s - TLS certificate chain verification utility\n\n", bin);
 
 	printf("Usage: %s [options] <ca_cert file> <raw certificates file>\n"
-		"  <ca_cert file> - local CA Certificate to validate against\n"
+		"  <ca_cert file> - local CA Certificate(s) to validate against\n"
 		"  <raw certificates file> - Certificates obtained from PCAP\n"
 		"  --help\n\n", bin);
 }
@@ -96,7 +96,7 @@ int main(int argc, char *argv[])
 {
 	int status = EXIT_FAILURE;
 	struct tls_cert *certchain;
-	struct tls_cert *ca_cert;
+	struct l_queue *ca_certs;
 	int err;
 
 	if (argc != 3) {
@@ -116,13 +116,13 @@ int main(int argc, char *argv[])
 		goto done;
 	}
 
-	ca_cert = tls_cert_load_file(argv[1]);
-	if (!ca_cert) {
-		fprintf(stderr, "Unable to load CA certifiate\n");
+	ca_certs = tls_cert_list_load_file(argv[1]);
+	if (!ca_certs) {
+		fprintf(stderr, "Unable to load CA certifiates\n");
 		goto free_certchain;
 	}
 
-	if (!tls_cert_verify_certchain(certchain, ca_cert)) {
+	if (!tls_cert_verify_certchain(certchain, ca_certs)) {
 		fprintf(stderr, "Verification failed\n");
 		goto free_cacert;
 	}
@@ -131,7 +131,7 @@ int main(int argc, char *argv[])
 	status = EXIT_SUCCESS;
 
 free_cacert:
-	l_free(ca_cert);
+	l_queue_destroy(ca_certs, l_free);
 free_certchain:
 	tls_cert_free_certchain(certchain);
 done:
-- 
2.19.1

[PATCH 07/11] unit: Update tls_cert_verify_certchain parameters

[Andrew Zaborowski]

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

---
 unit/test-tls.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/unit/test-tls.c b/unit/test-tls.c
index d969ee7..75f36f0 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -214,16 +214,16 @@ static void test_tls12_prf(const void *data)
 static void test_certificates(const void *data)
 {
 	struct tls_cert *cert;
-	struct tls_cert *cacert;
-	struct tls_cert *wrongca;
+	struct l_queue *cacert;
+	struct l_queue *wrongca;
 
 	cert = tls_cert_load_file(CERTDIR "cert-server.pem");
 	assert(cert);
 
-	cacert = tls_cert_load_file(CERTDIR "cert-ca.pem");
+	cacert = tls_cert_list_load_file(CERTDIR "cert-ca.pem");
 	assert(cacert);
 
-	wrongca = tls_cert_load_file(CERTDIR "cert-intca.pem");
+	wrongca = tls_cert_list_load_file(CERTDIR "cert-intca.pem");
 	assert(wrongca);
 
 	assert(!tls_cert_verify_certchain(cert, wrongca));
@@ -233,8 +233,8 @@ static void test_certificates(const void *data)
 	assert(tls_cert_verify_certchain(cert, NULL));
 
 	l_free(cert);
-	l_free(cacert);
-	l_free(wrongca);
+	l_queue_destroy(cacert, l_free);
+	l_queue_destroy(wrongca, l_free);
 }
 
 struct tls_conn_test {
-- 
2.19.1

[PATCH 08/11] tls: Stricter certificate chain verification

[Andrew Zaborowski]

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

Rewrite tls_cert_verify_certchain to fix a long standing issue where it
didn't verify that each certificate in the chain was trusted by the next
one.  Since certificates were only being added to the two keyrings and
never removed the order in the chain was not being verified.  Now use a
single ring and remove the previously added certificates from it once
they've been used to show that the next certificate was signed with one
of them.  Add lots of comments.
---
 ell/tls.c | 206 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 125 insertions(+), 81 deletions(-)

diff --git a/ell/tls.c b/ell/tls.c
index fbbc1bf..7f42240 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -2687,27 +2687,99 @@ static const struct pkcs1_encryption_oid {
 	},
 };
 
+static struct l_key *tls_cert_verify_root(struct l_key *key,
+						struct l_keyring *ring,
+						struct l_queue *roots)
+{
+	const struct l_queue_entry *entry;
+	unsigned int i, n = l_queue_length(roots);
+	struct l_key *root_keys[n];
+	bool success = true;
+
+	/*
+	 * Check that @key is trusted by one of @roots by linking it
+	 * to a restricted ring containing only the roots.  If successful
+	 * return @key leaving only @key in the keyring.
+	 */
+
+	for (entry = l_queue_get_entries(roots), i = 0; i < n;
+			entry = entry->next, i++) {
+		const struct tls_cert *root = entry->data;
+
+		root_keys[i] = l_key_new(L_KEY_RSA, root->asn1, root->size);
+		if (!root_keys[i] || !l_keyring_link(ring, root_keys[i]))
+			success = false;
+	}
+
+	if (!l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN, NULL) ||
+			!l_keyring_link(ring, key))
+		success = false;
+
+	for (i = 0; i < n; i++)
+		l_key_free(root_keys[i]);
+
+	if (success)
+		return key;
+
+	l_key_free(key);
+	return NULL;
+}
+
 static void tls_key_cleanup(struct l_key **p)
 {
-	l_key_free_norevoke(*p);
+	l_key_free(*p);
 }
 
-static bool tls_cert_link(struct tls_cert *cert, struct l_keyring *ring)
+static struct l_key *tls_cert_verify_with_keyring(struct tls_cert *cert,
+							struct l_keyring *ring,
+							struct l_queue *roots)
 {
-	L_AUTO_CLEANUP_VAR(struct l_key *, key, tls_key_cleanup) = NULL;
+	struct l_key *key;
+	const struct l_queue_entry *entry;
 
-	key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
+	if (cert->issuer) {
+		L_AUTO_CLEANUP_VAR(struct l_key *, trusted, tls_key_cleanup);
 
-	return key && l_keyring_link(ring, key);
-}
+		/*
+		 * Since we have the issuer, this is neither the root
+		 * certificate nor the closest-to-the-root certificate in
+		 * this chain, which may not contain the actual root.  Call
+		 * ourselves to verify that the chain is valid down to this
+		 * certificate's issuer.
+		 */
+		trusted = tls_cert_verify_with_keyring(cert->issuer,
+							ring, roots);
+		if (!trusted)
+			return NULL;
 
-static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
-						struct l_keyring *ring,
-						struct l_queue *roots,
-						struct l_keyring *trusted)
-{
-	if (!cert)
-		return true;
+		/*
+		 * The issuer is trusted, at this point we only have to
+		 * verify the issuer trusts this certificate and we will
+		 * know the whole chain is trusted.  After the call @ring
+		 * conveniently contains only the issuer certificate and
+		 * is restricted so that only certificates trusted by it
+		 * can be linked.
+		 */
+		key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
+		if (!key)
+			return NULL;
+
+		if (!l_keyring_link(ring, key))
+			goto free_key;
+
+		/*
+		 * The chain is valid down to @cert and @ring now
+		 * contains both the @trusted and @key.  However @trusted
+		 * is autofreed and will be revoked by l_key_free so
+		 * the caller will get a ring with only @key in it and
+		 * can try to link further certificates and unlink @key.
+		 */
+		return key;
+	}
+
+	key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
+	if (!key)
+		return NULL;
 
 	/*
 	 * RFC5246 7.4.2:
@@ -2717,51 +2789,47 @@ static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
 	 * the chain, under the assumption that the remote end must
 	 * already possess it in order to validate it in any case."
 	 *
-	 * This is an optimization to skip adding the root cert if
-	 * it's already in the trusted keyring.  It also happens to
-	 * work around a kernel issue preventing self-signed certificates
-	 * missing the AKID extension from being linked.
-	 *
-	 * If 'roots' were supplied we assume the keyring is already
-	 * restricted.
+	 * This is either the root cert or the closest-to-the-root
+	 * certificate in the chain.  If @roots were supplied we have
+	 * to verify it is either one of the roots or is trusted by one
+	 * of the roots.  If @roots is NULL, we only have to link it
+	 * to the keyring and restrict the keyring.
 	 */
-	if (!cert->issuer && roots) {
-		const struct l_queue_entry *entry;
+	if (!roots)
+		goto link_and_restrict;
 
-		for (entry = l_queue_get_entries(roots); entry;
-				entry = entry->next) {
-			const struct tls_cert *root = entry->data;
+	/*
+	 * This is an optimization to skip verifying the root cert if
+	 * it's identical to one of the supplied roots.  If it is, we just
+	 * have to add it to the keyring and restrict the keyring.
+	 * This also happens to work around a kernel issue preventing
+	 * self-signed certificates missing the AKID extension from being
+	 * linked.
+	 */
+	for (entry = l_queue_get_entries(roots); entry;
+			entry = entry->next) {
+		const struct tls_cert *root = entry->data;
 
-			if (cert->size == root->size && !memcmp(cert->asn1,
-						root->asn1, root->size))
-				return true;
-		}
+		if (cert->size == root->size && !memcmp(cert->asn1,
+					root->asn1, root->size))
+			goto link_and_restrict;
 	}
 
-	if (tls_cert_verify_with_keyring(cert->issuer, ring, roots, trusted)) {
-		if (!tls_cert_link(cert, ring))
-			return false;
+	return tls_cert_verify_root(key, ring, roots);
 
-		if (trusted || cert->issuer)
-			return true;
+link_and_restrict:
+	if (!l_keyring_link(ring, key))
+		goto free_key;
 
-		/*
-		 * If execution reaches this point, it's known that:
-		 *  * No trusted root key was supplied, so the chain is only
-		 *    being checked against its own root
-		 *  * The keyring 'ring' is not restricted yet
-		 *  * The chain's root cert was just linked in to the
-		 *    previously empty keyring 'ring'.
-		 *
-		 *  By restricting 'ring' now, the rest of the certs in
-		 *  the chain will have their signature validated using 'key'
-		 *  as the root.
-		 */
-		return l_keyring_restrict(ring,	L_KEYRING_RESTRICT_ASYM_CHAIN,
-						trusted);
-	}
+	if (!l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN,
+				NULL))
+		goto free_key;
 
-	return false;
+	return key;
+
+free_key:
+	l_key_free(key);
+	return NULL;
 }
 
 static void tls_keyring_cleanup(struct l_keyring **p)
@@ -2772,43 +2840,19 @@ static void tls_keyring_cleanup(struct l_keyring **p)
 bool tls_cert_verify_certchain(struct tls_cert *certchain,
 				struct l_queue *ca_certs)
 {
-	L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, tls_keyring_cleanup);
 	L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
-				tls_keyring_cleanup);
+				tls_keyring_cleanup) = l_keyring_new();
+	struct l_key *trusted;
 
-	ca_ring = NULL;
-
-	verify_ring = l_keyring_new();
 	if (!verify_ring)
 		return false;
 
-	/*
-	 * If CA cert(s) were supplied, restrict verify_ring now so
-	 * everything else in certchain is validated against the CA(s).
-	 * Otherwise, verify_ring will be restricted after the root of
-	 * certchain is added to verify_ring by
-	 * tls_cert_verify_with_keyring().
-	 */
-	if (ca_certs) {
-		const struct l_queue_entry *entry;
-
-		ca_ring = l_keyring_new();
-		if (!ca_ring)
-			return false;
-
-		for (entry = l_queue_get_entries(ca_certs); entry;
-				entry = entry->next)
-			if (!tls_cert_link(entry->data, ca_ring))
-				return false;
-
-		if (!l_keyring_restrict(verify_ring,
-					L_KEYRING_RESTRICT_ASYM_CHAIN,
-					ca_ring))
-			return false;
-	}
+	trusted = tls_cert_verify_with_keyring(certchain, verify_ring, ca_certs);
+	if (!trusted)
+		return false;
 
-	return tls_cert_verify_with_keyring(certchain, verify_ring, ca_certs,
-						ca_ring);
+	l_key_free(trusted);
+	return true;
 }
 
 void tls_cert_free_certchain(struct tls_cert *cert)
-- 
2.19.1

Re: [PATCH 08/11] tls: Stricter certificate chain verification

[Denis Kenzior]

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

Hi Andrew,

On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
> Rewrite tls_cert_verify_certchain to fix a long standing issue where it
> didn't verify that each certificate in the chain was trusted by the next
> one.  Since certificates were only being added to the two keyrings and
> never removed the order in the chain was not being verified.  Now use a
> single ring and remove the previously added certificates from it once
> they've been used to show that the next certificate was signed with one
> of them.  Add lots of comments.

So I actually like this approach a lot.  Few nitpicks:

> ---
>   ell/tls.c | 206 +++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 125 insertions(+), 81 deletions(-)
> 
> diff --git a/ell/tls.c b/ell/tls.c
> index fbbc1bf..7f42240 100644
> --- a/ell/tls.c
> +++ b/ell/tls.c
> @@ -2687,27 +2687,99 @@ static const struct pkcs1_encryption_oid {
>   	},
>   };
>   
> +static struct l_key *tls_cert_verify_root(struct l_key *key,
> +						struct l_keyring *ring,
> +						struct l_queue *roots)
> +{
> +	const struct l_queue_entry *entry;
> +	unsigned int i, n = l_queue_length(roots);
> +	struct l_key *root_keys[n];

So Marcel will likely hunt you down for VLA usage.  So maybe you want to 
rethink this.

> +	bool success = true;
> +
> +	/*
> +	 * Check that @key is trusted by one of @roots by linking it
> +	 * to a restricted ring containing only the roots.  If successful
> +	 * return @key leaving only @key in the keyring.
> +	 */
> +
> +	for (entry = l_queue_get_entries(roots), i = 0; i < n;
> +			entry = entry->next, i++) {
> +		const struct tls_cert *root = entry->data;
> +
> +		root_keys[i] = l_key_new(L_KEY_RSA, root->asn1, root->size);
> +		if (!root_keys[i] || !l_keyring_link(ring, root_keys[i]))
> +			success = false;

So why are we loading the CA keys into the verify ring?  Can't we just 
load all the CA keys into a temporary ring and then remove it when we're 
done?  E.g. the ca ring.

> +	}
> +
> +	if (!l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN, NULL) ||
> +			!l_keyring_link(ring, key))
> +		success = false;

And here we could restrict the ring against the ca_ring.  If that 
succeeds, then what we can do is call l_keyring_restrict, this time with 
last argument being NULL.  Or is that not possible? Mat?

> +
> +	for (i = 0; i < n; i++)
> +		l_key_free(root_keys[i]);
> +

Then this just becomes l_keyring_free(ca_ring);

> +	if (success)
> +		return key;
> +
> +	l_key_free(key);
> +	return NULL;
> +}
> +
>   static void tls_key_cleanup(struct l_key **p)
>   {
> -	l_key_free_norevoke(*p);
> +	l_key_free(*p);
>   }
>   
> -static bool tls_cert_link(struct tls_cert *cert, struct l_keyring *ring)

<snip>

> @@ -2772,43 +2840,19 @@ static void tls_keyring_cleanup(struct l_keyring **p)
>   bool tls_cert_verify_certchain(struct tls_cert *certchain,
>   				struct l_queue *ca_certs)
>   {
> -	L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, tls_keyring_cleanup);
>   	L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
> -				tls_keyring_cleanup);
> +				tls_keyring_cleanup) = l_keyring_new();
> +	struct l_key *trusted;

This function is so simple I would consider dropping the 
L_AUTO_CLEANUP_VAR completely.

>   
> -	ca_ring = NULL;
> -
> -	verify_ring = l_keyring_new();
>   	if (!verify_ring)
>   		return false;
>   
> -	/*
> -	 * If CA cert(s) were supplied, restrict verify_ring now so
> -	 * everything else in certchain is validated against the CA(s).
> -	 * Otherwise, verify_ring will be restricted after the root of
> -	 * certchain is added to verify_ring by
> -	 * tls_cert_verify_with_keyring().
> -	 */
> -	if (ca_certs) {
> -		const struct l_queue_entry *entry;
> -
> -		ca_ring = l_keyring_new();
> -		if (!ca_ring)
> -			return false;
> -
> -		for (entry = l_queue_get_entries(ca_certs); entry;
> -				entry = entry->next)
> -			if (!tls_cert_link(entry->data, ca_ring))
> -				return false;
> -
> -		if (!l_keyring_restrict(verify_ring,
> -					L_KEYRING_RESTRICT_ASYM_CHAIN,
> -					ca_ring))
> -			return false;
> -	}
> +	trusted = tls_cert_verify_with_keyring(certchain, verify_ring, ca_certs);
> +	if (!trusted)
> +		return false;
>   
> -	return tls_cert_verify_with_keyring(certchain, verify_ring, ca_certs,
> -						ca_ring);
> +	l_key_free(trusted);
> +	return true;
>   }
>   
>   void tls_cert_free_certchain(struct tls_cert *cert)
> 

Regards,
-Denis

Re: [PATCH 08/11] tls: Stricter certificate chain verification

[Mat Martineau]

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

On Thu, 1 Nov 2018, Denis Kenzior wrote:

> Hi Andrew,
>
> On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
>> Rewrite tls_cert_verify_certchain to fix a long standing issue where it
>> didn't verify that each certificate in the chain was trusted by the next
>> one.  Since certificates were only being added to the two keyrings and
>> never removed the order in the chain was not being verified.  Now use a
>> single ring and remove the previously added certificates from it once
>> they've been used to show that the next certificate was signed with one
>> of them.  Add lots of comments.
>
> So I actually like this approach a lot.  Few nitpicks:
>
>> ---
>>   ell/tls.c | 206 +++++++++++++++++++++++++++++++++---------------------
>>   1 file changed, 125 insertions(+), 81 deletions(-)
>> 
>> diff --git a/ell/tls.c b/ell/tls.c
>> index fbbc1bf..7f42240 100644
>> --- a/ell/tls.c
>> +++ b/ell/tls.c
>> @@ -2687,27 +2687,99 @@ static const struct pkcs1_encryption_oid {
>>   	},
>>   };
>>   +static struct l_key *tls_cert_verify_root(struct l_key *key,
>> +						struct l_keyring *ring,
>> +						struct l_queue *roots)

Have you considered loading the root certs in to a kernel keyring once 
in l_tls_set_cacert() then keeping it around to reuse?

>> +{
>> +	const struct l_queue_entry *entry;
>> +	unsigned int i, n = l_queue_length(roots);
>> +	struct l_key *root_keys[n];
>
> So Marcel will likely hunt you down for VLA usage.  So maybe you want to 
> rethink this.
>
>> +	bool success = true;
>> +
>> +	/*
>> +	 * Check that @key is trusted by one of @roots by linking it
>> +	 * to a restricted ring containing only the roots.  If successful
>> +	 * return @key leaving only @key in the keyring.
>> +	 */
>> +
>> +	for (entry = l_queue_get_entries(roots), i = 0; i < n;
>> +			entry = entry->next, i++) {
>> +		const struct tls_cert *root = entry->data;
>> +
>> +		root_keys[i] = l_key_new(L_KEY_RSA, root->asn1, root->size);
>> +		if (!root_keys[i] || !l_keyring_link(ring, root_keys[i]))
>> +			success = false;
>
> So why are we loading the CA keys into the verify ring?  Can't we just load 
> all the CA keys into a temporary ring and then remove it when we're done? 
> E.g. the ca ring.

This is possible, but I'm not sure it accomplishes much. Either way you 
still end up deleting all the CA keys, and if you add a temporary ring 
then you have to create and delete that too.

The use case I had in mind was to have a persistent ca_ring that could be 
used to restrict many other short-lived keyrings.

>
>> +	}
>> +
>> +	if (!l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN, NULL) ||
>> +			!l_keyring_link(ring, key))
>> +		success = false;
>
> And here we could restrict the ring against the ca_ring.  If that succeeds, 
> then what we can do is call l_keyring_restrict, this time with last argument 
> being NULL.  Or is that not possible? Mat?

I'm not 100% sure I'm following Denis here, but you only get one chance to 
call l_keyring_restrict() for a given keyring. So if you restrict against 
ca_ring, you can't change that to NULL later (but you could empty 
ca_ring and get the same effect).

>
>> +
>> +	for (i = 0; i < n; i++)
>> +		l_key_free(root_keys[i]);
>> +
>
> Then this just becomes l_keyring_free(ca_ring);

You still have to l_key_free() each root key. The l_keyring tells the 
kernel how to group the keys, but ELL doesn't free its l_key objects based 
on the keyring(s) they are in.

>
>> +	if (success)
>> +		return key;
>> +
>> +	l_key_free(key);
>> +	return NULL;
>> +}
>> +
>>   static void tls_key_cleanup(struct l_key **p)
>>   {
>> -	l_key_free_norevoke(*p);
>> +	l_key_free(*p);
>>   }
>>   -static bool tls_cert_link(struct tls_cert *cert, struct l_keyring *ring)
>
> <snip>
>
>> @@ -2772,43 +2840,19 @@ static void tls_keyring_cleanup(struct l_keyring 
>> **p)
>>   bool tls_cert_verify_certchain(struct tls_cert *certchain,
>>   				struct l_queue *ca_certs)
>>   {
>> -	L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, tls_keyring_cleanup);
>>   	L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
>> -				tls_keyring_cleanup);
>> +				tls_keyring_cleanup) = l_keyring_new();
>> +	struct l_key *trusted;
>
> This function is so simple I would consider dropping the L_AUTO_CLEANUP_VAR 
> completely.

The auto cleanup stuff was handy when there were many different 'return' 
statements, but with the simplified version of this function I agree with 
Denis.

>
>>   -	ca_ring = NULL;
>> -
>> -	verify_ring = l_keyring_new();
>>   	if (!verify_ring)
>>   		return false;
>>   -	/*
>> -	 * If CA cert(s) were supplied, restrict verify_ring now so
>> -	 * everything else in certchain is validated against the CA(s).
>> -	 * Otherwise, verify_ring will be restricted after the root of
>> -	 * certchain is added to verify_ring by
>> -	 * tls_cert_verify_with_keyring().
>> -	 */
>> -	if (ca_certs) {
>> -		const struct l_queue_entry *entry;
>> -
>> -		ca_ring = l_keyring_new();
>> -		if (!ca_ring)
>> -			return false;
>> -
>> -		for (entry = l_queue_get_entries(ca_certs); entry;
>> -				entry = entry->next)
>> -			if (!tls_cert_link(entry->data, ca_ring))
>> -				return false;
>> -
>> -		if (!l_keyring_restrict(verify_ring,
>> -					L_KEYRING_RESTRICT_ASYM_CHAIN,
>> -					ca_ring))
>> -			return false;
>> -	}
>> +	trusted = tls_cert_verify_with_keyring(certchain, verify_ring, 
>> ca_certs);
>> +	if (!trusted)
>> +		return false;
>>   -	return tls_cert_verify_with_keyring(certchain, verify_ring, ca_certs,
>> -						ca_ring);
>> +	l_key_free(trusted);
>> +	return true;
>>   }
>>     void tls_cert_free_certchain(struct tls_cert *cert)
>> 
>

It seems like the implementation is made more complicated by the fact that 
the cert list is a singly-linked list starting with the end-entity cert 
(which we then follow toward the root using the issuer pointers), while in 
order to verify we need to link keys to the restricted keyring starting 
from the root end of the chain.

If we know that it's a linear cert chain, the code could be simplified by 
making the cert chain a doubly-linked list that could be walked in the 
other direction. That would eliminate the recursion, leading to an 
implementation that looks more like this:

1. Create a keyring
2. Load it with the root certs
3. Try to load the first cert in the chain (from the root-facing end)
4. If that works, clear the root certs
5. Try to load the next cert in the chain
6. If that works, clear the previous cert
7. Repeat 5-6 until you get to the end-entity cert

That's the behavior of this patch (from the perspective of the kernel), 
but it's hard to see because recursive calls are used to follow the issuer 
chain toward the root first, then the keyring loads are attempted as the 
stack gets unwound.

(I realize the recursive code structure is inherited from my earlier 
implementation :) )


As Denis pointed out above, it would be convenient to add and remove the 
root certs as a group. The kernel lets keyrings contain keyrings. A 
restricted keyring R will search for signing keys in the trusted keyring T 
(if it wasn't NULL), and within R itself. If R "contains" a link to 
another keyring A, it will search T, R, and A for signing keys.

If we add l_keyring_link_nested(keyring, nested_keyring) to ELL, then you 
could have a persistent ca_ring that is temporarly linked to the verify 
ring to check the first cert, then ca_ring gets unlinked in a single 
operation when you want to check the second cert.


--
Mat Martineau
Intel OTC

Re: [PATCH 08/11] tls: Stricter certificate chain verification

[Denis Kenzior]

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

Hi Mat,

On 11/01/2018 07:35 PM, Mat Martineau wrote:
> On Thu, 1 Nov 2018, Denis Kenzior wrote:
> 
>> Hi Andrew,
>>
>> On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
>>> Rewrite tls_cert_verify_certchain to fix a long standing issue where it
>>> didn't verify that each certificate in the chain was trusted by the next
>>> one.  Since certificates were only being added to the two keyrings and
>>> never removed the order in the chain was not being verified.  Now use a
>>> single ring and remove the previously added certificates from it once
>>> they've been used to show that the next certificate was signed with one
>>> of them.  Add lots of comments.
>>
>> So I actually like this approach a lot.  Few nitpicks:
>>
>>> ---
>>>   ell/tls.c | 206 +++++++++++++++++++++++++++++++++---------------------
>>>   1 file changed, 125 insertions(+), 81 deletions(-)
>>>
>>> diff --git a/ell/tls.c b/ell/tls.c
>>> index fbbc1bf..7f42240 100644
>>> --- a/ell/tls.c
>>> +++ b/ell/tls.c
>>> @@ -2687,27 +2687,99 @@ static const struct pkcs1_encryption_oid {
>>>       },
>>>   };
>>>   +static struct l_key *tls_cert_verify_root(struct l_key *key,
>>> +                        struct l_keyring *ring,
>>> +                        struct l_queue *roots)
> 
> Have you considered loading the root certs in to a kernel keyring once 
> in l_tls_set_cacert() then keeping it around to reuse?
> 

Exactly.  This is ultimately what we want: to load root CAs once and 
reuse them between multiple connections.  So the CA keyring needs to be 
reusable.

>>> +{
>>> +    const struct l_queue_entry *entry;
>>> +    unsigned int i, n = l_queue_length(roots);
>>> +    struct l_key *root_keys[n];
>>
>> So Marcel will likely hunt you down for VLA usage.  So maybe you want 
>> to rethink this.
>>
>>> +    bool success = true;
>>> +
>>> +    /*
>>> +     * Check that @key is trusted by one of @roots by linking it
>>> +     * to a restricted ring containing only the roots.  If successful
>>> +     * return @key leaving only @key in the keyring.
>>> +     */
>>> +
>>> +    for (entry = l_queue_get_entries(roots), i = 0; i < n;
>>> +            entry = entry->next, i++) {
>>> +        const struct tls_cert *root = entry->data;
>>> +
>>> +        root_keys[i] = l_key_new(L_KEY_RSA, root->asn1, root->size);
>>> +        if (!root_keys[i] || !l_keyring_link(ring, root_keys[i]))
>>> +            success = false;
>>
>> So why are we loading the CA keys into the verify ring?  Can't we just 
>> load all the CA keys into a temporary ring and then remove it when 
>> we're done? E.g. the ca ring.
> 
> This is possible, but I'm not sure it accomplishes much. Either way you 
> still end up deleting all the CA keys, and if you add a temporary ring 
> then you have to create and delete that too.

As an intermediate transform until we get to the point where we keep the 
CAs around in a permanent keyring.  What I suggest above is mostly about 
getting rid of VLA usage.  We can l_key_free_norevoke here right away in 
this case.

> 
> The use case I had in mind was to have a persistent ca_ring that could 
> be used to restrict many other short-lived keyrings.
> 

Yep, I'm with you.

>>
>>> +    }
>>> +
>>> +    if (!l_keyring_restrict(ring, L_KEYRING_RESTRICT_ASYM_CHAIN, 
>>> NULL) ||
>>> +            !l_keyring_link(ring, key))
>>> +        success = false;
>>
>> And here we could restrict the ring against the ca_ring.  If that 
>> succeeds, then what we can do is call l_keyring_restrict, this time 
>> with last argument being NULL.  Or is that not possible? Mat?
> 
> I'm not 100% sure I'm following Denis here, but you only get one chance 
> to call l_keyring_restrict() for a given keyring. So if you restrict 
> against ca_ring, you can't change that to NULL later (but you could 
> empty ca_ring and get the same effect).

Okay, that's what I was wondering, whether we could call restrict 
multiple times.  So if we can't do that, and given the fact that we only 
have a single key we're talking about here, why don't we create a 
temporary keyring.  Put the key into that, and restrict the temporary to 
the CA ring.  If that succeeds, put the key into the actual ring and 
restrict it as we do above.  Hope that makes sense.

> 
>>
>>> +
>>> +    for (i = 0; i < n; i++)
>>> +        l_key_free(root_keys[i]);
>>> +
>>
>> Then this just becomes l_keyring_free(ca_ring);
> 
> You still have to l_key_free() each root key. The l_keyring tells the 
> kernel how to group the keys, but ELL doesn't free its l_key objects 
> based on the keyring(s) they are in.
> 

Right, I left out the l_key_free_norevoke detail above.

>>
>>> +    if (success)
>>> +        return key;
>>> +
>>> +    l_key_free(key);
>>> +    return NULL;
>>> +}
>>> +
>>>   static void tls_key_cleanup(struct l_key **p)
>>>   {
>>> -    l_key_free_norevoke(*p);
>>> +    l_key_free(*p);
>>>   }
>>>   -static bool tls_cert_link(struct tls_cert *cert, struct l_keyring 
>>> *ring)
>>
>> <snip>
>>
>>> @@ -2772,43 +2840,19 @@ static void tls_keyring_cleanup(struct 
>>> l_keyring **p)
>>>   bool tls_cert_verify_certchain(struct tls_cert *certchain,
>>>                   struct l_queue *ca_certs)
>>>   {
>>> -    L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, 
>>> tls_keyring_cleanup);
>>>       L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
>>> -                tls_keyring_cleanup);
>>> +                tls_keyring_cleanup) = l_keyring_new();
>>> +    struct l_key *trusted;
>>
>> This function is so simple I would consider dropping the 
>> L_AUTO_CLEANUP_VAR completely.
> 
> The auto cleanup stuff was handy when there were many different 'return' 
> statements, but with the simplified version of this function I agree 
> with Denis.
> 
>>
>>>   -    ca_ring = NULL;
>>> -
>>> -    verify_ring = l_keyring_new();
>>>       if (!verify_ring)
>>>           return false;
>>>   -    /*
>>> -     * If CA cert(s) were supplied, restrict verify_ring now so
>>> -     * everything else in certchain is validated against the CA(s).
>>> -     * Otherwise, verify_ring will be restricted after the root of
>>> -     * certchain is added to verify_ring by
>>> -     * tls_cert_verify_with_keyring().
>>> -     */
>>> -    if (ca_certs) {
>>> -        const struct l_queue_entry *entry;
>>> -
>>> -        ca_ring = l_keyring_new();
>>> -        if (!ca_ring)
>>> -            return false;
>>> -
>>> -        for (entry = l_queue_get_entries(ca_certs); entry;
>>> -                entry = entry->next)
>>> -            if (!tls_cert_link(entry->data, ca_ring))
>>> -                return false;
>>> -
>>> -        if (!l_keyring_restrict(verify_ring,
>>> -                    L_KEYRING_RESTRICT_ASYM_CHAIN,
>>> -                    ca_ring))
>>> -            return false;
>>> -    }
>>> +    trusted = tls_cert_verify_with_keyring(certchain, verify_ring, 
>>> ca_certs);
>>> +    if (!trusted)
>>> +        return false;
>>>   -    return tls_cert_verify_with_keyring(certchain, verify_ring, 
>>> ca_certs,
>>> -                        ca_ring);
>>> +    l_key_free(trusted);
>>> +    return true;
>>>   }
>>>     void tls_cert_free_certchain(struct tls_cert *cert)
>>>
>>
> 
> It seems like the implementation is made more complicated by the fact 
> that the cert list is a singly-linked list starting with the end-entity 
> cert (which we then follow toward the root using the issuer pointers), 
> while in order to verify we need to link keys to the restricted keyring 
> starting from the root end of the chain.
> 
> If we know that it's a linear cert chain, the code could be simplified 
> by making the cert chain a doubly-linked list that could be walked in 
> the other direction. That would eliminate the recursion, leading to an 
> implementation that looks more like this:
> 
> 1. Create a keyring
> 2. Load it with the root certs
> 3. Try to load the first cert in the chain (from the root-facing end)
> 4. If that works, clear the root certs
> 5. Try to load the next cert in the chain
> 6. If that works, clear the previous cert
> 7. Repeat 5-6 until you get to the end-entity cert
> 
> That's the behavior of this patch (from the perspective of the kernel), 
> but it's hard to see because recursive calls are used to follow the 
> issuer chain toward the root first, then the keyring loads are attempted 
> as the stack gets unwound.
> 
> (I realize the recursive code structure is inherited from my earlier 
> implementation :) )
> 

I agree here, I think recursive implementation is actually harder to 
read and more resource intensive.

> 
> As Denis pointed out above, it would be convenient to add and remove the 
> root certs as a group. The kernel lets keyrings contain keyrings. A 
> restricted keyring R will search for signing keys in the trusted keyring 
> T (if it wasn't NULL), and within R itself. If R "contains" a link to 
> another keyring A, it will search T, R, and A for signing keys.
> 
> If we add l_keyring_link_nested(keyring, nested_keyring) to ELL, then 
> you could have a persistent ca_ring that is temporarly linked to the 
> verify ring to check the first cert, then ca_ring gets unlinked in a 
> single operation when you want to check the second cert.
> 

So that might actually be an even better solution than what I proposed 
above with having a temporary restriction ring.

Regards,
-Denis

Re: [PATCH 08/11] tls: Stricter certificate chain verification

[Andrew Zaborowski]

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

Hi,

On Fri, 2 Nov 2018 at 01:55, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/01/2018 07:35 PM, Mat Martineau wrote:
> > Have you considered loading the root certs in to a kernel keyring once
> > in l_tls_set_cacert() then keeping it around to reuse?
> >
>
> Exactly.  This is ultimately what we want: to load root CAs once and
> reuse them between multiple connections.  So the CA keyring needs to be
> reusable.

Ok.  With it being possible to add nested keyrings this is easy.

> >> <snip>
> >>
> >>> @@ -2772,43 +2840,19 @@ static void tls_keyring_cleanup(struct
> >>> l_keyring **p)
> >>>   bool tls_cert_verify_certchain(struct tls_cert *certchain,
> >>>                   struct l_queue *ca_certs)
> >>>   {
> >>> -    L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring,
> >>> tls_keyring_cleanup);
> >>>       L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
> >>> -                tls_keyring_cleanup);
> >>> +                tls_keyring_cleanup) = l_keyring_new();
> >>> +    struct l_key *trusted;
> >>
> >> This function is so simple I would consider dropping the
> >> L_AUTO_CLEANUP_VAR completely.

Ok.

> >
> > The auto cleanup stuff was handy when there were many different 'return'
> > statements, but with the simplified version of this function I agree
> > with Denis.
> >
> >>
> >>>   -    ca_ring = NULL;
> >>> -
> >>> -    verify_ring = l_keyring_new();
> >>>       if (!verify_ring)
> >>>           return false;
> >>>   -    /*
> >>> -     * If CA cert(s) were supplied, restrict verify_ring now so
> >>> -     * everything else in certchain is validated against the CA(s).
> >>> -     * Otherwise, verify_ring will be restricted after the root of
> >>> -     * certchain is added to verify_ring by
> >>> -     * tls_cert_verify_with_keyring().
> >>> -     */
> >>> -    if (ca_certs) {
> >>> -        const struct l_queue_entry *entry;
> >>> -
> >>> -        ca_ring = l_keyring_new();
> >>> -        if (!ca_ring)
> >>> -            return false;
> >>> -
> >>> -        for (entry = l_queue_get_entries(ca_certs); entry;
> >>> -                entry = entry->next)
> >>> -            if (!tls_cert_link(entry->data, ca_ring))
> >>> -                return false;
> >>> -
> >>> -        if (!l_keyring_restrict(verify_ring,
> >>> -                    L_KEYRING_RESTRICT_ASYM_CHAIN,
> >>> -                    ca_ring))
> >>> -            return false;
> >>> -    }
> >>> +    trusted = tls_cert_verify_with_keyring(certchain, verify_ring,
> >>> ca_certs);
> >>> +    if (!trusted)
> >>> +        return false;
> >>>   -    return tls_cert_verify_with_keyring(certchain, verify_ring,
> >>> ca_certs,
> >>> -                        ca_ring);
> >>> +    l_key_free(trusted);
> >>> +    return true;
> >>>   }
> >>>     void tls_cert_free_certchain(struct tls_cert *cert)
> >>>
> >>
> >
> > It seems like the implementation is made more complicated by the fact
> > that the cert list is a singly-linked list starting with the end-entity
> > cert (which we then follow toward the root using the issuer pointers),
> > while in order to verify we need to link keys to the restricted keyring
> > starting from the root end of the chain.
> >
> > If we know that it's a linear cert chain, the code could be simplified
> > by making the cert chain a doubly-linked list that could be walked in
> > the other direction. That would eliminate the recursion, leading to an
> > implementation that looks more like this:
> >
> > 1. Create a keyring
> > 2. Load it with the root certs
> > 3. Try to load the first cert in the chain (from the root-facing end)
> > 4. If that works, clear the root certs
> > 5. Try to load the next cert in the chain
> > 6. If that works, clear the previous cert
> > 7. Repeat 5-6 until you get to the end-entity cert
> >
> > That's the behavior of this patch (from the perspective of the kernel),
> > but it's hard to see because recursive calls are used to follow the
> > issuer chain toward the root first, then the keyring loads are attempted
> > as the stack gets unwound.
> >
> > (I realize the recursive code structure is inherited from my earlier
> > implementation :) )
> >
>
> I agree here, I think recursive implementation is actually harder to
> read and more resource intensive.

Yep, definitely, but using a doubly-linked list for all tls_cert
structs also seems like unneeded overhead.  We can easily create a
temporary reversed list or a plain array in tls_cert_verify_certchain,
which is only called once per l_tls object anyway.

>
> >
> > As Denis pointed out above, it would be convenient to add and remove the
> > root certs as a group. The kernel lets keyrings contain keyrings. A
> > restricted keyring R will search for signing keys in the trusted keyring
> > T (if it wasn't NULL), and within R itself. If R "contains" a link to
> > another keyring A, it will search T, R, and A for signing keys.
> >
> > If we add l_keyring_link_nested(keyring, nested_keyring) to ELL, then
> > you could have a persistent ca_ring that is temporarly linked to the
> > verify ring to check the first cert, then ca_ring gets unlinked in a
> > single operation when you want to check the second cert.
> >
>
> So that might actually be an even better solution than what I proposed
> above with having a temporary restriction ring.

Yes, makes sense, I had actually seen this in the kernel keyring
search function but it didn't occur to me to use it.

Best regards

[PATCH 09/11] tls: Support loading certificate chains from file

[Andrew Zaborowski]

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

When loading the file supplied with l_tls_set_auth_data to be sent as
Server Certificate or Client Certificate, allow for the possibility
of loading a chain of certificates instead of just one if the file
contains multiple concatenated PEM certificates, ordered from the
end-entity to the CA.  The chain has to end with one of the CA certs
set through l_tls_set_cacert or a cert directly signed by one of them,
if l_tls_set_cacert was called.
---
 ell/tls.c | 69 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 17 deletions(-)

diff --git a/ell/tls.c b/ell/tls.c
index 7f42240..0aa10e7 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -754,7 +754,7 @@ static bool tls_send_certificate(struct l_tls *tls)
 
 	if (tls->server && !cert) {
 		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
-				"Loading server certificate %s failed",
+				"Loading server certificate chain %s failed",
 				tls->cert_path);
 		return false;
 	}
@@ -828,7 +828,7 @@ static bool tls_send_certificate(struct l_tls *tls)
 	if (cert)
 		tls->cert_sent = true;
 
-	l_free(cert);
+	tls_cert_free_certchain(cert);
 
 	return true;
 }
@@ -2570,27 +2570,41 @@ const char *tls_handshake_state_to_str(enum tls_handshake_state state)
 
 struct tls_cert *tls_cert_load_file(const char *filename)
 {
-	uint8_t *der;
-	size_t len;
-	struct tls_cert *cert;
+	struct l_queue *pem_list;
+	struct tls_cert *chain = NULL;
+	struct tls_cert **root = &chain;
+	bool error = false;
 
-	der = l_pem_load_certificate(filename, &len);
-	if (!der)
+	pem_list = l_pem_load_certificate_list(filename);
+	if (!pem_list)
 		return NULL;
 
-	if (!len || der[0] != ASN1_ID_SEQUENCE) {
-		l_free(der);
-		return NULL;
+	while (!l_queue_isempty(pem_list)) {
+		struct tls_cert *cert;
+		struct l_pem_list_element *elem = l_queue_pop_head(pem_list);
+		uint8_t *der = elem->content;
+
+		if (!elem->len || der[0] != ASN1_ID_SEQUENCE)
+			error = true;
+
+		cert = l_malloc(sizeof(struct tls_cert) + elem->len);
+		cert->size = elem->len;
+		cert->issuer = NULL;
+		memcpy(cert->asn1, der, cert->size);
+		l_free(elem->content);
+		l_free(elem);
+
+		*root = cert;
+		root = &cert->issuer;
 	}
 
-	cert = l_malloc(sizeof(struct tls_cert) + len);
-	cert->size = len;
-	cert->issuer = NULL;
-	memcpy(cert->asn1, der, len);
+	l_queue_destroy(pem_list, NULL);
 
-	l_free(der);
+	if (!error)
+		return chain;
 
-	return cert;
+	tls_cert_free_certchain(chain);
+	return NULL;
 }
 
 struct l_queue *tls_cert_list_load_file(const char *filename)
@@ -2674,7 +2688,28 @@ decode_error:
 bool tls_cert_find_certchain(struct tls_cert *cert,
 				const char *cacert_filename)
 {
-	return true;
+	struct l_queue *ca_certs = NULL;
+	bool valid;
+
+	/* Nothing to do if no CA certificates supplied */
+	if (!cacert_filename)
+		return true;
+
+	ca_certs = tls_cert_list_load_file(cacert_filename);
+	if (!ca_certs)
+		return false;
+
+	/*
+	 * Also nothing to do if the user already supplied a working
+	 * certificate chain.
+	 */
+	valid = tls_cert_verify_certchain(cert, ca_certs);
+	l_queue_destroy(ca_certs, l_free);
+	if (valid)
+		return true;
+
+	/* Actual search for a chain to the CA cert is unimplemented, fail */
+	return false;
 }
 
 static const struct pkcs1_encryption_oid {
-- 
2.19.1

Re: [PATCH 09/11] tls: Support loading certificate chains from file

[Denis Kenzior]

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

Hi Andrew,

On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
> When loading the file supplied with l_tls_set_auth_data to be sent as
> Server Certificate or Client Certificate, allow for the possibility
> of loading a chain of certificates instead of just one if the file
> contains multiple concatenated PEM certificates, ordered from the
> end-entity to the CA.  The chain has to end with one of the CA certs
> set through l_tls_set_cacert or a cert directly signed by one of them,
> if l_tls_set_cacert was called.
> ---
>   ell/tls.c | 69 +++++++++++++++++++++++++++++++++++++++++--------------
>   1 file changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/ell/tls.c b/ell/tls.c
> index 7f42240..0aa10e7 100644
> --- a/ell/tls.c
> +++ b/ell/tls.c
> @@ -754,7 +754,7 @@ static bool tls_send_certificate(struct l_tls *tls)
>   
>   	if (tls->server && !cert) {
>   		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
> -				"Loading server certificate %s failed",
> +				"Loading server certificate chain %s failed",
>   				tls->cert_path);
>   		return false;
>   	}
> @@ -828,7 +828,7 @@ static bool tls_send_certificate(struct l_tls *tls)
>   	if (cert)
>   		tls->cert_sent = true;
>   
> -	l_free(cert);
> +	tls_cert_free_certchain(cert);
>   
>   	return true;
>   }
> @@ -2570,27 +2570,41 @@ const char *tls_handshake_state_to_str(enum tls_handshake_state state)
>   
>   struct tls_cert *tls_cert_load_file(const char *filename)
>   {
> -	uint8_t *der;
> -	size_t len;
> -	struct tls_cert *cert;
> +	struct l_queue *pem_list;
> +	struct tls_cert *chain = NULL;
> +	struct tls_cert **root = &chain;
> +	bool error = false;
>   
> -	der = l_pem_load_certificate(filename, &len);
> -	if (!der)
> +	pem_list = l_pem_load_certificate_list(filename);
> +	if (!pem_list)
>   		return NULL;
>   
> -	if (!len || der[0] != ASN1_ID_SEQUENCE) {
> -		l_free(der);
> -		return NULL;
> +	while (!l_queue_isempty(pem_list)) {
> +		struct tls_cert *cert;
> +		struct l_pem_list_element *elem = l_queue_pop_head(pem_list);
> +		uint8_t *der = elem->content;
> +
> +		if (!elem->len || der[0] != ASN1_ID_SEQUENCE)
> +			error = true;
> +
> +		cert = l_malloc(sizeof(struct tls_cert) + elem->len);
> +		cert->size = elem->len;
> +		cert->issuer = NULL;
> +		memcpy(cert->asn1, der, cert->size);
> +		l_free(elem->content);
> +		l_free(elem);
> +
> +		*root = cert;
> +		root = &cert->issuer;

Isn't this doing almost exactly the same thing as a function in patch 5?

>   	}
>   
> -	cert = l_malloc(sizeof(struct tls_cert) + len);
> -	cert->size = len;
> -	cert->issuer = NULL;
> -	memcpy(cert->asn1, der, len);
> +	l_queue_destroy(pem_list, NULL);
>   
> -	l_free(der);
> +	if (!error)
> +		return chain;
>   
> -	return cert;
> +	tls_cert_free_certchain(chain);
> +	return NULL;
>   }
>   
>   struct l_queue *tls_cert_list_load_file(const char *filename)

Regards,
-Denis

[PATCH 10/11] tls: Propagate errors to l_tls_prf_get_bytes

[Andrew Zaborowski]

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

Add bool return types to tls_prf_get_bytes, tls10_prf, tls12_prf so that
l_new_checksum errors can be reported.
---
 ell/tls-private.h |  6 +++---
 ell/tls.c         | 36 +++++++++++++++++++++++-------------
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/ell/tls-private.h b/ell/tls-private.h
index 95b8c65..d71a3bb 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -207,12 +207,12 @@ struct l_tls {
 	bool ready;
 };
 
-void tls10_prf(const void *secret, size_t secret_len,
+bool tls10_prf(const void *secret, size_t secret_len,
 		const char *label,
 		const void *seed, size_t seed_len,
 		uint8_t *out, size_t out_len);
 
-void tls12_prf(enum l_checksum_type type, size_t hash_len,
+bool tls12_prf(enum l_checksum_type type, size_t hash_len,
 		const void *secret, size_t secret_len,
 		const char *label,
 		const void *seed, size_t seed_len,
@@ -254,7 +254,7 @@ void tls_cert_free_certchain(struct tls_cert *cert);
 
 enum tls_cert_key_type tls_cert_get_pubkey_type(struct tls_cert *cert);
 
-void tls_prf_get_bytes(struct l_tls *tls,
+bool tls_prf_get_bytes(struct l_tls *tls,
 				enum l_checksum_type type, size_t hash_len,
 				const void *secret, size_t secret_len,
 				const char *label,
diff --git a/ell/tls.c b/ell/tls.c
index 0aa10e7..7420772 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -40,7 +40,7 @@
 #include "asn1-private.h"
 #include "queue.h"
 
-void tls10_prf(const void *secret, size_t secret_len,
+bool tls10_prf(const void *secret, size_t secret_len,
 		const char *label,
 		const void *seed, size_t seed_len,
 		uint8_t *out, size_t out_len)
@@ -59,24 +59,28 @@ void tls10_prf(const void *secret, size_t secret_len,
 	 * the first byte of S2.
 	 */
 
-	tls12_prf(L_CHECKSUM_MD5, 16,
+	if (!tls12_prf(L_CHECKSUM_MD5, 16,
 			secret, l_s1,
 			label, seed, seed_len,
-			out, out_len);
+			out, out_len))
+		return false;
 
 	if (secret_len > 0)
 		secret += secret_len - l_s1;
 
-	tls12_prf(L_CHECKSUM_SHA1, 20,
+	if (!tls12_prf(L_CHECKSUM_SHA1, 20,
 			secret, l_s1,
 			label, seed, seed_len,
-			p_hash2, out_len);
+			p_hash2, out_len))
+		return false;
 
 	for (i = 0; i < out_len; i++)
 		out[i] ^= p_hash2[i];
+
+	return true;
 }
 
-void tls12_prf(enum l_checksum_type type, size_t hash_len,
+bool tls12_prf(enum l_checksum_type type, size_t hash_len,
 		const void *secret, size_t secret_len,
 		const char *label,
 		const void *seed, size_t seed_len,
@@ -86,6 +90,9 @@ void tls12_prf(enum l_checksum_type type, size_t hash_len,
 	size_t a_len, chunk_len, prfseed_len = strlen(label) + seed_len;
 	uint8_t a[128], prfseed[prfseed_len];
 
+	if (!hmac)
+		return false;
+
 	/* Generate the hash seed or A(0) as label + seed */
 	memcpy(prfseed, label, strlen(label));
 	memcpy(prfseed + strlen(label), seed, seed_len);
@@ -112,9 +119,10 @@ void tls12_prf(enum l_checksum_type type, size_t hash_len,
 	}
 
 	l_checksum_free(hmac);
+	return true;
 }
 
-void tls_prf_get_bytes(struct l_tls *tls,
+bool tls_prf_get_bytes(struct l_tls *tls,
 				enum l_checksum_type type, size_t hash_len,
 				const void *secret, size_t secret_len,
 				const char *label,
@@ -122,10 +130,11 @@ void tls_prf_get_bytes(struct l_tls *tls,
 				uint8_t *buf, size_t len)
 {
 	if (tls->negotiated_version >= TLS_V12)
-		tls12_prf(type, hash_len, secret, secret_len, label,
-				seed, seed_len, buf, len);
+		return tls12_prf(type, hash_len, secret, secret_len, label,
+					seed, seed_len, buf, len);
 	else
-		tls10_prf(secret, secret_len, label, seed, seed_len, buf, len);
+		return tls10_prf(secret, secret_len, label,
+					seed, seed_len, buf, len);
 }
 
 LIB_EXPORT bool l_tls_prf_get_bytes(struct l_tls *tls,
@@ -134,6 +143,7 @@ LIB_EXPORT bool l_tls_prf_get_bytes(struct l_tls *tls,
 				const char *label, uint8_t *buf, size_t len)
 {
 	uint8_t seed[64];
+	bool r;
 
 	if (unlikely(!tls))
 		return false;
@@ -142,16 +152,16 @@ LIB_EXPORT bool l_tls_prf_get_bytes(struct l_tls *tls,
 	memcpy(seed + 32, tls->pending.server_random, 32);
 
 	if (use_master_secret)
-		tls_prf_get_bytes(tls, type, hash_len,
+		r = tls_prf_get_bytes(tls, type, hash_len,
 						tls->pending.master_secret, 48,
 						label, seed, 64, buf, len);
 	else
-		tls_prf_get_bytes(tls, type, hash_len, "", 0,
+		r = tls_prf_get_bytes(tls, type, hash_len, "", 0,
 						label, seed, 64, buf, len);
 
 	memset(seed, 0, 64);
 
-	return true;
+	return r;
 }
 
 static void tls_write_random(uint8_t *buf)
-- 
2.19.1

[PATCH 11/11] unit: More TLS certificate tests

[Andrew Zaborowski]

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

Add a few more certificate chain checks including some using
tls_cert_list_load_file and the stricter tls_cert_verify_certchain.
---
 unit/test-tls.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/unit/test-tls.c b/unit/test-tls.c
index 75f36f0..a134125 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -216,6 +216,9 @@ static void test_certificates(const void *data)
 	struct tls_cert *cert;
 	struct l_queue *cacert;
 	struct l_queue *wrongca;
+	struct l_queue *twocas;
+	struct tls_cert *chain;
+	struct tls_cert *chain2;
 
 	cert = tls_cert_load_file(CERTDIR "cert-server.pem");
 	assert(cert);
@@ -226,15 +229,49 @@ static void test_certificates(const void *data)
 	wrongca = tls_cert_list_load_file(CERTDIR "cert-intca.pem");
 	assert(wrongca);
 
+	twocas = tls_cert_list_load_file(CERTDIR "cert-chain.pem");
+	assert(wrongca);
+
 	assert(!tls_cert_verify_certchain(cert, wrongca));
 
 	assert(tls_cert_verify_certchain(cert, cacert));
 
 	assert(tls_cert_verify_certchain(cert, NULL));
 
-	l_free(cert);
+	assert(tls_cert_verify_certchain(cert, twocas));
+
+	chain = tls_cert_load_file(CERTDIR "cert-server.pem");
+	assert(chain);
+	chain->issuer = tls_cert_load_file(CERTDIR "cert-entity-int.pem");
+	assert(chain->issuer);
+	chain->issuer->issuer = tls_cert_load_file(CERTDIR "cert-intca.pem");
+	assert(chain->issuer->issuer);
+	chain->issuer->issuer->issuer =
+		tls_cert_load_file(CERTDIR "cert-ca.pem");
+	assert(chain->issuer->issuer->issuer);
+	assert(tls_cert_verify_certchain(chain->issuer, cacert));
+	assert(tls_cert_verify_certchain(chain->issuer, NULL));
+	assert(!tls_cert_verify_certchain(chain->issuer, wrongca));
+	assert(tls_cert_verify_certchain(chain->issuer, twocas));
+	assert(!tls_cert_verify_certchain(chain, cacert));
+	assert(!tls_cert_verify_certchain(chain, NULL));
+	assert(!tls_cert_verify_certchain(chain, twocas));
+
+	chain2 = tls_cert_load_file(CERTDIR "cert-chain.pem");
+	assert(chain2);
+	assert(chain2->issuer);
+	assert(!chain2->issuer->issuer);
+	assert(tls_cert_verify_certchain(chain2, cacert));
+	assert(tls_cert_verify_certchain(chain2, NULL));
+	assert(!tls_cert_verify_certchain(chain2, wrongca));
+	assert(tls_cert_verify_certchain(chain2, twocas));
+
+	tls_cert_free_certchain(cert);
+	tls_cert_free_certchain(chain);
+	tls_cert_free_certchain(chain2);
 	l_queue_destroy(cacert, l_free);
 	l_queue_destroy(wrongca, l_free);
+	l_queue_destroy(twocas, l_free);
 }
 
 struct tls_conn_test {
-- 
2.19.1

Re: [PATCH 01/11] settings: Fix a format string

[Denis Kenzior]

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

Hi Andrew,

On 11/01/2018 06:54 AM, Andrew Zaborowski wrote:
> The %d conersion specifier is for integers.
> ---
>   ell/settings.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Patches 1-3 applied, thanks.

Regards,
-Denis