[PATCH 1/9] tls: Drop local code to add hash OID, let kernel do it

[PATCH 1/9] tls: Drop local code to add hash OID, let kernel do it

[Andrew Zaborowski]

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

Drop tls.c code to build the PKCS#1 DigestInfo structure which basically
consisted of prepending the signature hash algorithm's OID to the hash.
Let the key API do it by setting the hash type in the l_key_sign and
l_key_verify calls.  Slightly simplify the tls_rsa_sign and
tls_rsa_verify code while doing this.
---
 ell/tls.c | 164 ++++++++++++++++++------------------------------------
 1 file changed, 53 insertions(+), 111 deletions(-)

diff --git a/ell/tls.c b/ell/tls.c
index 4cf0f86..9969fcc 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -734,65 +734,6 @@ static const char *tls_handshake_type_to_str(enum tls_handshake_type 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,
-};
-static const uint8_t pkcs1_digest_info_sha1_start[] = {
-	0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2b, 0x0e, 0x03, 0x02, 0x1a, 0x05,
-	0x00, 0x04, 0x14,
-};
-static const uint8_t pkcs1_digest_info_sha256_start[] = {
-	0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03,
-	0x04, 0x02, 0x01, 0x05, 0x00, 0x04, 0x20,
-};
-static const uint8_t pkcs1_digest_info_sha384_start[] = {
-	0x30, 0x41, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03,
-	0x04, 0x02, 0x02, 0x05, 0x00, 0x04, 0x30,
-};
-static const uint8_t pkcs1_digest_info_sha512_start[] = {
-	0x30, 0x51, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03,
-	0x04, 0x02, 0x03, 0x05, 0x00, 0x04, 0x40,
-};
-
-static void pkcs1_write_digest_info(enum l_checksum_type type,
-					uint8_t *out, size_t *out_len,
-					const uint8_t *hash, size_t hash_len)
-{
-	switch (type) {
-	case L_CHECKSUM_MD5:
-		memcpy(out, pkcs1_digest_info_md5_start,
-				sizeof(pkcs1_digest_info_md5_start));
-		*out_len = sizeof(pkcs1_digest_info_md5_start);
-		break;
-	case L_CHECKSUM_SHA1:
-		memcpy(out, pkcs1_digest_info_sha1_start,
-				sizeof(pkcs1_digest_info_sha1_start));
-		*out_len = sizeof(pkcs1_digest_info_sha1_start);
-		break;
-	case L_CHECKSUM_SHA256:
-		memcpy(out, pkcs1_digest_info_sha256_start,
-				sizeof(pkcs1_digest_info_sha256_start));
-		*out_len = sizeof(pkcs1_digest_info_sha256_start);
-		break;
-	case L_CHECKSUM_SHA384:
-		memcpy(out, pkcs1_digest_info_sha384_start,
-				sizeof(pkcs1_digest_info_sha384_start));
-		*out_len = sizeof(pkcs1_digest_info_sha384_start);
-		break;
-	case L_CHECKSUM_SHA512:
-		memcpy(out, pkcs1_digest_info_sha512_start,
-				sizeof(pkcs1_digest_info_sha512_start));
-		*out_len = sizeof(pkcs1_digest_info_sha512_start);
-		break;
-	default:
-		abort();
-	}
-
-	memcpy(out + *out_len, hash, hash_len);
-	*out_len += hash_len;
-}
-
 static void tls_send_alert(struct l_tls *tls, bool fatal,
 				enum l_tls_alert_desc alert_desc)
 {
@@ -1238,13 +1179,11 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
 				tls_get_hash_t get_hash)
 {
-	ssize_t result;
-	const struct tls_hash_algorithm *hash_type;
-	uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
+	ssize_t result = -EMSGSIZE;
+	enum l_checksum_type sign_checksum_type;
 	uint8_t sign_input[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];
 	size_t sign_input_len;
-	bool prepend_hash_type = false;
-	size_t expected_bytes;
+	uint8_t *ptr = out;
 
 	if (!tls->priv_key || !tls->priv_key_size) {
 		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
@@ -1253,54 +1192,49 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
 		return -ENOKEY;
 	}
 
-	expected_bytes = tls->priv_key_size + 2;
-
 	if (tls->negotiated_version >= TLS_V12) {
-		hash_type = &tls_handshake_hash_data[tls->signature_hash];
-		get_hash(tls, hash_type->tls_id, hash, NULL, NULL);
+		const struct tls_hash_algorithm *hash_type =
+			hash_type = &tls_handshake_hash_data[tls->signature_hash];
+
+		get_hash(tls, hash_type->tls_id, sign_input, NULL, NULL);
+		sign_checksum_type = hash_type->l_id;
+		sign_input_len = hash_type->length;
 
-		pkcs1_write_digest_info(hash_type->l_id,
-					sign_input, &sign_input_len,
-					hash, hash_type->length);
+		if (len < 2)
+			goto error;
 
-		prepend_hash_type = true;
-		expected_bytes += 2;
+		*ptr++ = hash_type->tls_id;
+		*ptr++ = 1;	/* RSA_sign */
+		len -= 2;
 	} else {
 		get_hash(tls, 1, sign_input + 0, NULL, NULL);	/* MD5 */
 		get_hash(tls, 2, sign_input + 16, NULL, NULL);	/* SHA1 */
+		sign_checksum_type = L_CHECKSUM_NONE;
 		sign_input_len = 36;
 	}
 
-	result = -EMSGSIZE;
-
-	if (len >= expected_bytes) {
-		if (prepend_hash_type) {
-			*out++ = hash_type->tls_id;
-			*out++ = 1;	/* RSA_sign */
-		}
+	if (len < tls->priv_key_size + 2)
+		goto error;
 
-		l_put_be16(tls->priv_key_size, out);
-		result = l_key_sign(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
-					L_CHECKSUM_NONE, sign_input, out + 2,
-					sign_input_len, tls->priv_key_size);
+	l_put_be16(tls->priv_key_size, ptr);
+	result = l_key_sign(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
+				sign_checksum_type, sign_input, ptr + 2,
+				sign_input_len, tls->priv_key_size);
+	ptr += tls->priv_key_size + 2;
 
-		if (result == (ssize_t) tls->priv_key_size)
-			result = expected_bytes;
-	}
-
-	if (result < 0)
-		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
-				"Signing the hash failed: %s",
-				strerror(-result));
+	if (result == (ssize_t) tls->priv_key_size)
+		return ptr - out; /* Success */
 
+error:
+	TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
+			"Signing the hash failed: %s",
+			strerror(-result));
 	return result;
 }
 
 static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 				tls_get_hash_t get_hash)
 {
-	uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
-	size_t hash_len;
 	enum l_checksum_type hash_type;
 	uint8_t expected[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];
 	size_t expected_len;
@@ -1340,7 +1274,8 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 			return false;
 		}
 
-		if (!get_hash(tls, in[0], hash, &hash_len, &hash_type)) {
+		if (!get_hash(tls, in[0], expected, &expected_len,
+				&hash_type)) {
 			TLS_DISCONNECT(TLS_ALERT_DECRYPT_ERROR, 0,
 					"Unknown hash type %i", in[0]);
 
@@ -1348,36 +1283,43 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 		}
 
 		/*
-		 * TODO: According to 4.7 we need to support at least two forms
-		 * of the signed content in the verification:
+		 * Note: Next we let the l_key_verify's underlying kernel
+		 * operation prepend the OID to the hash to build the
+		 * DigestInfo struct.  However according to 4.7 we need to
+		 * support at least two forms of the signed content in the
+		 * verification:
 		 *  - DigestInfo with NULL AlgorithmIdentifier.parameters,
-		 *  - DigestInfo with empty AlgorithmIdentifier.parameters.
+		 *  - DigestInfo with empty AlgorithmIdentifier.parameters,
+		 *
+		 * while the kernel only understands the former encoding.
+		 * Note PKCS#1 versions 2.0 and later section A.2.4 do
+		 * mandate NULL AlgorithmIdentifier.parameters.
 		 *
-		 * Additionally PKCS#1 now says BER is used in place of DER for
-		 * DigestInfo encoding which adds more ambiguity in the
+		 * Additionally PKCS#1 v1.5 said BER is used in place of DER
+		 * for DigestInfo encoding which adds more ambiguity in the
 		 * encoding.
 		 */
-		pkcs1_write_digest_info(hash_type, expected, &expected_len,
-					hash, hash_len);
 	} else {
 		get_hash(tls, 1, expected + 0, NULL, NULL);	/* MD5 */
 		get_hash(tls, 2, expected + 16, NULL, NULL);	/* SHA1 */
 		expected_len = 36;
+		hash_type = L_CHECKSUM_NONE;
 
 		/*
-		 * Within the RSA padding for signatures PKCS#1 1.5 allows
-		 * the block format to be either 0 or 1, while PKCS#1 2.0
-		 * mandates block type 1 making the signatures unambiguous.
-		 * The l_asymmetric_cipher_verify implementation only
-		 * accepts block type 1.
-		 * TODO: TLS 1.0 doesn't specify that block type must be 1
-		 * like TLS 1.2 does meaning that both PKCS#1 1.5 types are
-		 * probably allowed.
+		 * Note: Within the RSA padding for signatures PKCS#1 1.5
+		 * allows the block format to be either 0 or 1, while PKCS#1
+		 * v2.0+ mandates block type 1 making the signatures
+		 * unambiguous.  TLS 1.0 doesn't additionally specify which
+		 * block type is to be used (TLS 1.2 does) meaning that both
+		 * PKCS#1 v1.5 types are allowed.  The l_key_verify's
+		 * underlying kernel implementation only accepts block type
+		 * 1.  If this ever becomes an issue we'd need to go back to
+		 * using L_KEY_RSA_RAW and our own PKCS#1 v1.5 verify logic.
 		 */
 	}
 
 	success = l_key_verify(tls->peer_pubkey, L_KEY_RSA_PKCS1_V1_5,
-				L_CHECKSUM_NONE, expected, in + 4,
+				hash_type, expected, in + 4,
 				expected_len, tls->peer_pubkey_size);
 
 	if (!success)
-- 
2.19.1

[PATCH 2/9] tls: Fix length in TLS 1.0 signature verification

[Andrew Zaborowski]

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

Account for the shorter Certificate Verify message in TLS 1.0/1.1 when
parsing the message to verify the signature.  This fixes server side TLS
1.0/1.1 support which apparently has been broken since the migration to
l_key.
---
 ell/tls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/tls.c b/ell/tls.c
index 9969fcc..fd89206 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -1319,7 +1319,7 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 	}
 
 	success = l_key_verify(tls->peer_pubkey, L_KEY_RSA_PKCS1_V1_5,
-				hash_type, expected, in + 4,
+				hash_type, expected, in + offset + 2,
 				expected_len, tls->peer_pubkey_size);
 
 	if (!success)
-- 
2.19.1

[PATCH 3/9] tls: Propagate errors to l_tls_prf_get_bytes

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 4193 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 |  4 ++--
 ell/tls.c         | 38 ++++++++++++++++++++++++--------------
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/ell/tls-private.h b/ell/tls-private.h
index e48aaa0..f2b6b14 100644
--- a/ell/tls-private.h
+++ b/ell/tls-private.h
@@ -217,12 +217,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,
diff --git a/ell/tls.c b/ell/tls.c
index fd89206..cfdcdfa 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -42,7 +42,7 @@
 #include "key.h"
 #include "asn1-private.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)
@@ -61,24 +61,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,
@@ -88,6 +92,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);
@@ -114,26 +121,29 @@ void tls12_prf(enum l_checksum_type type, size_t hash_len,
 	}
 
 	l_checksum_free(hmac);
+	return true;
 }
 
-static void tls_prf_get_bytes(struct l_tls *tls,
+static bool tls_prf_get_bytes(struct l_tls *tls,
 				const void *secret, size_t secret_len,
 				const char *label,
 				const void *seed, size_t seed_len,
 				uint8_t *buf, size_t len)
 {
 	if (tls->negotiated_version >= TLS_V12)
-		tls12_prf(tls->prf_hmac->l_id, tls->prf_hmac->length,
-				secret, secret_len, label,
-				seed, seed_len, buf, len);
+		return tls12_prf(tls->prf_hmac->l_id, tls->prf_hmac->length,
+					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, bool use_master_secret,
 				const char *label, uint8_t *buf, size_t len)
 {
 	uint8_t seed[64];
+	bool r;
 
 	if (unlikely(!tls || !tls->prf_hmac))
 		return false;
@@ -142,14 +152,14 @@ LIB_EXPORT bool l_tls_prf_get_bytes(struct l_tls *tls, bool use_master_secret,
 	memcpy(seed + 32, tls->pending.server_random, 32);
 
 	if (use_master_secret)
-		tls_prf_get_bytes(tls, tls->pending.master_secret, 48,
+		r = tls_prf_get_bytes(tls, tls->pending.master_secret, 48,
 					label, seed, 64, buf, len);
 	else
-		tls_prf_get_bytes(tls, "", 0, label, seed, 64, buf, len);
+		r = tls_prf_get_bytes(tls, "", 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 4/9] tls: Pass the verified peer identity to "ready" callback

[Andrew Zaborowski]

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

Extract a human-readable peer identity string from the supplied
certificate and pass it to the ready callback provided by API user to
indicate that the peer has actually been authenticated.  For some users
it will only really matter whether the value is non-NULL meaning that
the peer is trusted by the locally configured CA, while for others,
such as web browsers it's useful to display a human-readable peer
identity to the user so they can see if a website is served by who they
think it belongs to.  If we got a peer certificate but user supplied no
CAs or peer couldn't be authenticated, don't bother to extract the
identity string and pass NULL.

Until now we'd always pass NULL (there was a TODO comment) because
we'd hoped we could at some point leverage the kernel's certificate
parser to extract the subject name.  The newly added
tls_get_peer_identity_str might also fit in cert.c instead of tls.c.
---
 ell/tls.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 3 deletions(-)

diff --git a/ell/tls.c b/ell/tls.c
index cfdcdfa..b13f551 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -2151,10 +2151,76 @@ static void tls_handle_certificate_verify(struct l_tls *tls,
 	TLS_SET_STATE(TLS_HANDSHAKE_WAIT_CHANGE_CIPHER_SPEC);
 }
 
+static const struct asn1_oid dn_organization_name_oid =
+	{ 3, { 0x55, 0x04, 0x0a } };
+static const struct asn1_oid dn_common_name_oid =
+	{ 3, { 0x55, 0x04, 0x03 } };
+
+static char *tls_get_peer_identity_str(struct l_cert *cert)
+{
+	const uint8_t *dn, *end;
+	size_t dn_size;
+	const uint8_t *printable_str = NULL;
+	size_t printable_str_len;
+
+	if (!cert)
+		return NULL;
+
+	dn = l_cert_get_dn(cert, &dn_size);
+	if (!dn)
+		return NULL;
+
+	end = dn + dn_size;
+	while (dn < end) {
+		const uint8_t *set, *seq, *oid, *name;
+		uint8_t tag;
+		size_t len, oid_len, name_len;
+
+		set = asn1_der_find_elem(dn, end - dn, 0, &tag, &len);
+		if (!set || tag != ASN1_ID_SET)
+			return NULL;
+
+		dn = set + len;
+
+		seq = asn1_der_find_elem(set, len, 0, &tag, &len);
+		if (!seq || tag != ASN1_ID_SEQUENCE)
+			return NULL;
+
+		oid = asn1_der_find_elem(seq, len, 0, &tag, &oid_len);
+		if (!oid || tag != ASN1_ID_OID)
+			return NULL;
+
+		name = asn1_der_find_elem(seq, len, 1, &tag, &name_len);
+		if (!oid || (tag != ASN1_ID_PRINTABLESTRING &&
+					tag != ASN1_ID_UTF8STRING))
+			continue;
+
+		/* organizationName takes priority, commonName is second */
+		if (asn1_oid_eq(&dn_organization_name_oid, oid_len, oid) ||
+				(!printable_str &&
+				 asn1_oid_eq(&dn_common_name_oid,
+						oid_len, oid))) {
+			printable_str = name;
+			printable_str_len = name_len;
+		}
+	}
+
+	if (printable_str)
+		return l_strndup((char *) printable_str, printable_str_len);
+	else
+		return NULL;
+}
+
 static void tls_finished(struct l_tls *tls)
 {
 	char *peer_identity = NULL;
 
+	if (tls->peer_authenticated) {
+		peer_identity = tls_get_peer_identity_str(tls->peer_cert);
+		if (!peer_identity)
+			TLS_DEBUG("tls_get_peer_identity_str failed");
+	}
+
 	/* Free up the resources used in the handshake */
 	tls_reset_handshake(tls);
 
@@ -2162,11 +2228,9 @@ static void tls_finished(struct l_tls *tls)
 	tls->ready = true;
 
 	tls->ready_handle(peer_identity, tls->user_data);
+	l_free(peer_identity);
 
 	tls_cleanup_handshake(tls);
-
-	if (peer_identity)
-		l_free(peer_identity);
 }
 
 static void tls_handle_handshake(struct l_tls *tls, int type,
-- 
2.19.1

[PATCH 5/9] unit: Validate the peer_identity in test-tls

[Andrew Zaborowski]

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

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

diff --git a/unit/test-tls.c b/unit/test-tls.c
index 5103bef..e8c18f0 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -397,6 +397,9 @@ static void tls_test_ready(const char *peer_identity, void *user_data)
 	struct tls_test_state *s = user_data;
 
 	assert(!s->ready);
+	assert((!s->expect_peer && !peer_identity) ||
+			(s->expect_peer && peer_identity &&
+			 !strcmp(s->expect_peer, peer_identity)));
 	s->ready = true;
 
 	l_tls_write(s->tls, (const uint8_t *) s->send_data,
-- 
2.19.1

[PATCH 6/9] cert: Rename l_certchain_foreach functions

[Andrew Zaborowski]

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

Change the "foreach" to "walk" in l_certchain_foreach_from_leaf/_ca
and in l_cert_foreach_cb_t to not imply that the whole chain is always
traversed because we let the callbacks interrupt the iteration by
returning true.  This can be used for searches or trust verification, or
can be ignored if the whole chain is to be walked over.  Add comments
for the two functions affected.
---
 ell/cert.c  | 18 ++++++++++++++----
 ell/cert.h  | 12 +++++-------
 ell/ell.sym |  4 ++--
 ell/tls.c   |  4 ++--
 4 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/ell/cert.c b/ell/cert.c
index 66433c2..0ecdfbb 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -253,8 +253,13 @@ LIB_EXPORT struct l_cert *l_certchain_get_leaf(struct l_certchain *chain)
 	return chain->leaf;
 }
 
-LIB_EXPORT bool l_certchain_foreach_from_leaf(struct l_certchain *chain,
-						l_cert_foreach_cb_t cb,
+/*
+ * Call @cb for each certificate in the chain starting from the leaf
+ * certificate.  If a call returns @true, stop and return @true to
+ * the user, otherwise return @false.
+ */
+LIB_EXPORT bool l_certchain_walk_from_leaf(struct l_certchain *chain,
+						l_cert_walk_cb_t cb,
 						void *user_data)
 {
 	struct l_cert *cert;
@@ -269,8 +274,13 @@ LIB_EXPORT bool l_certchain_foreach_from_leaf(struct l_certchain *chain,
 	return false;
 }
 
-LIB_EXPORT bool l_certchain_foreach_from_ca(struct l_certchain *chain,
-						l_cert_foreach_cb_t cb,
+/*
+ * Call @cb for each certificate in the chain starting from the root
+ * certificate.  If a call returns @true, stop and return @true to
+ * the user, otherwise return @false.
+ */
+LIB_EXPORT bool l_certchain_walk_from_ca(struct l_certchain *chain,
+						l_cert_walk_cb_t cb,
 						void *user_data)
 {
 	struct l_cert *cert;
diff --git a/ell/cert.h b/ell/cert.h
index c1513f1..5205fa8 100644
--- a/ell/cert.h
+++ b/ell/cert.h
@@ -38,7 +38,7 @@ enum l_cert_key_type {
 	L_CERT_KEY_UNKNOWN,
 };
 
-typedef bool (*l_cert_foreach_cb_t)(struct l_cert *cert, void *user_data);
+typedef bool (*l_cert_walk_cb_t)(struct l_cert *cert, void *user_data);
 
 struct l_cert *l_cert_new_from_der(const uint8_t *buf, size_t buf_len);
 void l_cert_free(struct l_cert *cert);
@@ -51,12 +51,10 @@ struct l_key *l_cert_get_pubkey(struct l_cert *cert);
 void l_certchain_free(struct l_certchain *chain);
 
 struct l_cert *l_certchain_get_leaf(struct l_certchain *chain);
-bool l_certchain_foreach_from_leaf(struct l_certchain *chain,
-					l_cert_foreach_cb_t cb,
-					void *user_data);
-bool l_certchain_foreach_from_ca(struct l_certchain *chain,
-					l_cert_foreach_cb_t cb,
-					void *user_data);
+bool l_certchain_walk_from_leaf(struct l_certchain *chain,
+				l_cert_walk_cb_t cb, void *user_data);
+bool l_certchain_walk_from_ca(struct l_certchain *chain,
+				l_cert_walk_cb_t cb, void *user_data);
 
 bool l_certchain_find(struct l_certchain *chain, struct l_queue *ca_certs);
 bool l_certchain_verify(struct l_certchain *chain, struct l_queue *ca_certs);
diff --git a/ell/ell.sym b/ell/ell.sym
index 9cea0c1..b8f3730 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -445,8 +445,8 @@ global:
 	l_cert_get_pubkey;
 	l_certchain_free;
 	l_certchain_get_leaf;
-	l_certchain_foreach_from_leaf;
-	l_certchain_foreach_from_ca;
+	l_certchain_walk_from_leaf;
+	l_certchain_walk_from_ca;
 	l_certchain_find;
 	l_certchain_verify;
 local:
diff --git a/ell/tls.c b/ell/tls.c
index b13f551..ebf4b0c 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -961,7 +961,7 @@ static bool tls_send_certificate(struct l_tls *tls)
 	 */
 
 	total = 0;
-	l_certchain_foreach_from_leaf(tls->cert, tls_cert_list_add_size, &total);
+	l_certchain_walk_from_leaf(tls->cert, tls_cert_list_add_size, &total);
 
 	buf = l_malloc(128 + total);
 	ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
@@ -971,7 +971,7 @@ static bool tls_send_certificate(struct l_tls *tls)
 	*ptr++ = total >> 16;
 	*ptr++ = total >>  8;
 	*ptr++ = total >>  0;
-	l_certchain_foreach_from_leaf(tls->cert, tls_cert_list_append, &ptr);
+	l_certchain_walk_from_leaf(tls->cert, tls_cert_list_append, &ptr);
 
 	tls_tx_handshake(tls, TLS_CERTIFICATE, buf, ptr - buf);
 
-- 
2.19.1

[PATCH 7/9] cert: Optionally return error string from l_certchain_verify

[Andrew Zaborowski]

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

Return a basic error string to the l_certchain_verify caller so it can
be included in debug logs.
---
 ell/cert.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------
 ell/cert.h |  3 ++-
 ell/tls.c  |  7 ++++---
 3 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/ell/cert.c b/ell/cert.c
index 0ecdfbb..bd49831 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -23,6 +23,7 @@
 #endif
 
 #include <string.h>
+#include <stdio.h>
 
 #include "private.h"
 #include "key.h"
@@ -309,17 +310,18 @@ LIB_EXPORT bool l_certchain_find(struct l_certchain *chain,
 	 * Also nothing to do if the user already supplied a working
 	 * certificate chain.
 	 */
-	if (l_certchain_verify(chain, ca_certs))
+	if (l_certchain_verify(chain, ca_certs, NULL))
 		return true;
 
 	/* Actual search for a chain to the CA cert is unimplemented, fail */
 	return false;
 }
 
-static struct l_keyring *cert_set_to_keyring(struct l_queue *certs)
+static struct l_keyring *cert_set_to_keyring(struct l_queue *certs, char *error)
 {
 	struct l_keyring *ring;
 	const struct l_queue_entry *entry;
+	int i = 1;
 
 	ring = l_keyring_new();
 	if (!ring)
@@ -329,15 +331,23 @@ static struct l_keyring *cert_set_to_keyring(struct l_queue *certs)
 		struct l_cert *cert = entry->data;
 		struct l_key *key = l_cert_get_pubkey(cert);
 
-		if (!key)
+		if (!key) {
+			sprintf(error, "Can't get public key from certificate "
+				"%i / %i in certificate set", i,
+				l_queue_length(certs));
 			goto cleanup;
+		}
 
 		if (!l_keyring_link(ring, key)) {
 			l_key_free(key);
+			sprintf(error, "Can't link the public key from "
+				"certificate %i / %i to target keyring",
+				i, l_queue_length(certs));
 			goto cleanup;
 		}
 
 		l_key_free_norevoke(key);
+		i++;
 	}
 
 	return ring;
@@ -386,21 +396,33 @@ static void cert_keyring_cleanup(struct l_keyring **p)
 	l_keyring_free(*p);
 }
 
+#define RETURN_ERROR(msg, args...)	\
+	do {	\
+		if (error) {	\
+			*error = error_buf;	\
+			snprintf(error_buf, sizeof(error_buf), msg, ## args); \
+		}	\
+		return false;	\
+	} while (0)
+
 LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
-					struct l_queue *ca_certs)
+					struct l_queue *ca_certs,
+					const char **error)
 {
 	struct l_keyring *ca_ring = NULL;
 	L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
 				cert_keyring_cleanup) = NULL;
 	struct l_cert *cert;
 	struct l_key *prev_key = NULL;
+	int verified = 0;
+	static char error_buf[200];
 
 	if (unlikely(!chain || !chain->leaf))
-		return false;
+		RETURN_ERROR("Chain empty");
 
 	verify_ring = l_keyring_new();
 	if (!verify_ring)
-		return false;
+		RETURN_ERROR("Can't create verify keyring");
 
 	cert = chain->ca;
 
@@ -422,19 +444,23 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
 	 * extension from being linked to a keyring.
 	 */
 	if (cert_is_in_set(cert, ca_certs)) {
+		verified++;
 		cert = cert->issued;
 		if (!cert)
 			return true;
 
 		prev_key = cert_try_link(cert, verify_ring);
 	} else if (ca_certs) {
-		ca_ring = cert_set_to_keyring(ca_certs);
-		if (!ca_ring)
+		ca_ring = cert_set_to_keyring(ca_certs, error_buf);
+		if (!ca_ring) {
+			if (error)
+				*error = error_buf;
 			return false;
+		}
 
 		if (!l_keyring_link_nested(verify_ring, ca_ring)) {
 			l_keyring_free(ca_ring);
-			return false;
+			RETURN_ERROR("Can't link CA ring to verify ring");
 		}
 	} else
 		prev_key = cert_try_link(cert, verify_ring);
@@ -447,7 +473,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
 				NULL)) {
 		l_key_free(prev_key);
 		l_keyring_free(ca_ring);
-		return false;
+		RETURN_ERROR("Can't restrict verify keyring");
 	}
 
 	if (ca_ring) {
@@ -474,10 +500,20 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
 		l_key_free(prev_key);
 		prev_key = new_key;
 		cert = cert->issued;
+		verified++;
 	}
 
-	if (!prev_key)
-		return false;
+	if (!prev_key) {
+		int total = 0;
+
+		for (cert = chain->ca; cert; cert = cert->issued, total++);
+		RETURN_ERROR("Linking certificate %i / %i failed, root %s"
+				"verified against trusted CA(s) and the "
+				"following %i top certificates verified ok",
+				verified + 1, total,
+				ca_certs && verified ? "" : "not ",
+				verified ? verified - 1 : 0);
+	}
 
 	l_key_free(prev_key);
 	return true;
diff --git a/ell/cert.h b/ell/cert.h
index 5205fa8..393b3a7 100644
--- a/ell/cert.h
+++ b/ell/cert.h
@@ -57,7 +57,8 @@ bool l_certchain_walk_from_ca(struct l_certchain *chain,
 				l_cert_walk_cb_t cb, void *user_data);
 
 bool l_certchain_find(struct l_certchain *chain, struct l_queue *ca_certs);
-bool l_certchain_verify(struct l_certchain *chain, struct l_queue *ca_certs);
+bool l_certchain_verify(struct l_certchain *chain, struct l_queue *ca_certs,
+			const char **error);
 
 #ifdef __cplusplus
 }
diff --git a/ell/tls.c b/ell/tls.c
index ebf4b0c..ade43d8 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -1780,6 +1780,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 	size_t der_len;
 	const uint8_t *der;
 	bool dummy;
+	const char *error_str;
 
 	if (len < 3)
 		goto decode_error;
@@ -1823,11 +1824,11 @@ static void tls_handle_certificate(struct l_tls *tls,
 	 * Validate the certificate chain's consistency and validate it
 	 * against our CAs if we have any.
 	 */
-	if (!l_certchain_verify(certchain, tls->ca_certs)) {
+	if (!l_certchain_verify(certchain, tls->ca_certs, &error_str)) {
 		TLS_DISCONNECT(TLS_ALERT_BAD_CERT, 0,
 				"Peer certchain verification failed "
-				"consistency check%s", tls->ca_certs ?
-				" or against local CA certs" : "");
+				"consistency check%s: %s", tls->ca_certs ?
+				" or against local CA certs" : "", error_str);
 
 		goto done;
 	}
-- 
2.19.1

[PATCH 8/9] unit: Update test-tls to new l_certchain_verify params

[Andrew Zaborowski]

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

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

diff --git a/unit/test-tls.c b/unit/test-tls.c
index e8c18f0..84dd950 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -232,18 +232,18 @@ static void test_certificates(const void *data)
 	chain = l_pem_load_certificate_chain(CERTDIR "cert-server.pem");
 	assert(chain);
 
-	assert(!l_certchain_verify(chain, wrongca));
-	assert(l_certchain_verify(chain, cacert));
-	assert(l_certchain_verify(chain, NULL));
-	assert(l_certchain_verify(chain, twocas));
+	assert(!l_certchain_verify(chain, wrongca, NULL));
+	assert(l_certchain_verify(chain, cacert, NULL));
+	assert(l_certchain_verify(chain, NULL, NULL));
+	assert(l_certchain_verify(chain, twocas, NULL));
 
 	chain2 = l_pem_load_certificate_chain(CERTDIR "cert-chain.pem");
 	assert(chain2);
 
-	assert(!l_certchain_verify(chain2, wrongca));
-	assert(l_certchain_verify(chain2, cacert));
-	assert(l_certchain_verify(chain2, NULL));
-	assert(l_certchain_verify(chain2, twocas));
+	assert(!l_certchain_verify(chain2, wrongca, NULL));
+	assert(l_certchain_verify(chain2, cacert, NULL));
+	assert(l_certchain_verify(chain2, NULL, NULL));
+	assert(l_certchain_verify(chain2, twocas, NULL));
 
 	chain3 = certchain_new_from_leaf(
 			tls_cert_load_file(CERTDIR "cert-server.pem"));
@@ -255,10 +255,10 @@ static void test_certificates(const void *data)
 			tls_cert_load_file(CERTDIR "cert-ca.pem"));
 	assert(chain3);
 
-	assert(!l_certchain_verify(chain3, wrongca));
-	assert(!l_certchain_verify(chain3, cacert));
-	assert(!l_certchain_verify(chain3, NULL));
-	assert(!l_certchain_verify(chain3, twocas));
+	assert(!l_certchain_verify(chain3, wrongca, NULL));
+	assert(!l_certchain_verify(chain3, cacert, NULL));
+	assert(!l_certchain_verify(chain3, NULL, NULL));
+	assert(!l_certchain_verify(chain3, twocas, NULL));
 
 	chain4 = certchain_new_from_leaf(
 			tls_cert_load_file(CERTDIR "cert-entity-int.pem"));
@@ -268,10 +268,10 @@ static void test_certificates(const void *data)
 			tls_cert_load_file(CERTDIR "cert-ca.pem"));
 	assert(chain4);
 
-	assert(!l_certchain_verify(chain4, wrongca));
-	assert(l_certchain_verify(chain4, cacert));
-	assert(l_certchain_verify(chain4, NULL));
-	assert(l_certchain_verify(chain4, twocas));
+	assert(!l_certchain_verify(chain4, wrongca, NULL));
+	assert(l_certchain_verify(chain4, cacert, NULL));
+	assert(l_certchain_verify(chain4, NULL, NULL));
+	assert(l_certchain_verify(chain4, twocas, NULL));
 
 	l_certchain_free(chain);
 	l_certchain_free(chain2);
-- 
2.19.1

[PATCH 9/9] tools: Update certchain-verify to new l_certchain_verify params

[Andrew Zaborowski]

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

---
 tools/certchain-verify.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/certchain-verify.c b/tools/certchain-verify.c
index 08eec90..1a1dab1 100644
--- a/tools/certchain-verify.c
+++ b/tools/certchain-verify.c
@@ -98,6 +98,7 @@ int main(int argc, char *argv[])
 	struct l_certchain *certchain;
 	struct l_queue *ca_certs;
 	int err;
+	const char *error_str;
 
 	if (argc != 3) {
 		usage(argv[0]);
@@ -122,8 +123,8 @@ int main(int argc, char *argv[])
 		goto free_certchain;
 	}
 
-	if (!l_certchain_verify(certchain, ca_certs)) {
-		fprintf(stderr, "Verification failed\n");
+	if (!l_certchain_verify(certchain, ca_certs, &error_str)) {
+		fprintf(stderr, "Verification failed: %s\n", error_str);
 		goto free_cacert;
 	}
 
-- 
2.19.1

Re: [PATCH 1/9] tls: Drop local code to add hash OID, let kernel do it

[Denis Kenzior]

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

Hi Andrew,

On 11/27/2018 08:38 PM, Andrew Zaborowski wrote:
> Drop tls.c code to build the PKCS#1 DigestInfo structure which basically
> consisted of prepending the signature hash algorithm's OID to the hash.
> Let the key API do it by setting the hash type in the l_key_sign and
> l_key_verify calls.  Slightly simplify the tls_rsa_sign and
> tls_rsa_verify code while doing this.
> ---
>   ell/tls.c | 164 ++++++++++++++++++------------------------------------
>   1 file changed, 53 insertions(+), 111 deletions(-)
> 

<snip>

> @@ -1238,13 +1179,11 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
>   static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
>   				tls_get_hash_t get_hash)
>   {
> -	ssize_t result;
> -	const struct tls_hash_algorithm *hash_type;
> -	uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
> +	ssize_t result = -EMSGSIZE;
> +	enum l_checksum_type sign_checksum_type;
>   	uint8_t sign_input[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];

Does this need to be updated?

>   	size_t sign_input_len;
> -	bool prepend_hash_type = false;
> -	size_t expected_bytes;
> +	uint8_t *ptr = out;
>   
>   	if (!tls->priv_key || !tls->priv_key_size) {
>   		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
> @@ -1253,54 +1192,49 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
>   		return -ENOKEY;
>   	}
>   
> -	expected_bytes = tls->priv_key_size + 2;
> -
>   	if (tls->negotiated_version >= TLS_V12) {
> -		hash_type = &tls_handshake_hash_data[tls->signature_hash];
> -		get_hash(tls, hash_type->tls_id, hash, NULL, NULL);
> +		const struct tls_hash_algorithm *hash_type =
> +			hash_type = &tls_handshake_hash_data[tls->signature_hash];

What is going on here?

> +
> +		get_hash(tls, hash_type->tls_id, sign_input, NULL, NULL);
> +		sign_checksum_type = hash_type->l_id;
> +		sign_input_len = hash_type->length;
>   
> -		pkcs1_write_digest_info(hash_type->l_id,
> -					sign_input, &sign_input_len,
> -					hash, hash_type->length);
> +		if (len < 2)
> +			goto error;

Why do we check this after generating the signature?

>   
> -		prepend_hash_type = true;
> -		expected_bytes += 2;
> +		*ptr++ = hash_type->tls_id;
> +		*ptr++ = 1;	/* RSA_sign */
> +		len -= 2;
>   	} else {
>   		get_hash(tls, 1, sign_input + 0, NULL, NULL);	/* MD5 */
>   		get_hash(tls, 2, sign_input + 16, NULL, NULL);	/* SHA1 */
> +		sign_checksum_type = L_CHECKSUM_NONE;
>   		sign_input_len = 36;
>   	}
>   
> -	result = -EMSGSIZE;
> -
> -	if (len >= expected_bytes) {
> -		if (prepend_hash_type) {
> -			*out++ = hash_type->tls_id;
> -			*out++ = 1;	/* RSA_sign */
> -		}
> +	if (len < tls->priv_key_size + 2)
> +		goto error;
>   
> -		l_put_be16(tls->priv_key_size, out);
> -		result = l_key_sign(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
> -					L_CHECKSUM_NONE, sign_input, out + 2,
> -					sign_input_len, tls->priv_key_size);
> +	l_put_be16(tls->priv_key_size, ptr);
> +	result = l_key_sign(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
> +				sign_checksum_type, sign_input, ptr + 2,
> +				sign_input_len, tls->priv_key_size);
> +	ptr += tls->priv_key_size + 2;
>   
> -		if (result == (ssize_t) tls->priv_key_size)
> -			result = expected_bytes;
> -	}
> -
> -	if (result < 0)
> -		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
> -				"Signing the hash failed: %s",
> -				strerror(-result));
> +	if (result == (ssize_t) tls->priv_key_size)
> +		return ptr - out; /* Success */
>   
> +error:
> +	TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
> +			"Signing the hash failed: %s",
> +			strerror(-result));
>   	return result;
>   }
>   
>   static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
>   				tls_get_hash_t get_hash)
>   {
> -	uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
> -	size_t hash_len;
>   	enum l_checksum_type hash_type;
>   	uint8_t expected[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];

Same question here?

>   	size_t expected_len;

Regards,
-Denis

Re: [PATCH 3/9] tls: Propagate errors to l_tls_prf_get_bytes

[Denis Kenzior]

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

Hi Andrew,

On 11/27/2018 08:38 PM, Andrew Zaborowski wrote:
> 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 |  4 ++--
>   ell/tls.c         | 38 ++++++++++++++++++++++++--------------
>   2 files changed, 26 insertions(+), 16 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 6/9] cert: Rename l_certchain_foreach functions

[Denis Kenzior]

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

Hi Andrew,

On 11/27/2018 08:38 PM, Andrew Zaborowski wrote:
> Change the "foreach" to "walk" in l_certchain_foreach_from_leaf/_ca
> and in l_cert_foreach_cb_t to not imply that the whole chain is always
> traversed because we let the callbacks interrupt the iteration by
> returning true.  This can be used for searches or trust verification, or
> can be ignored if the whole chain is to be walked over.  Add comments
> for the two functions affected.
> ---
>   ell/cert.c  | 18 ++++++++++++++----
>   ell/cert.h  | 12 +++++-------
>   ell/ell.sym |  4 ++--
>   ell/tls.c   |  4 ++--
>   4 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/ell/cert.c b/ell/cert.c
> index 66433c2..0ecdfbb 100644
> --- a/ell/cert.c
> +++ b/ell/cert.c
> @@ -253,8 +253,13 @@ LIB_EXPORT struct l_cert *l_certchain_get_leaf(struct l_certchain *chain)
>   	return chain->leaf;
>   }
>   
> -LIB_EXPORT bool l_certchain_foreach_from_leaf(struct l_certchain *chain,
> -						l_cert_foreach_cb_t cb,
> +/*
> + * Call @cb for each certificate in the chain starting from the leaf
> + * certificate.  If a call returns @true, stop and return @true to
> + * the user, otherwise return @false.
> + */
> +LIB_EXPORT bool l_certchain_walk_from_leaf(struct l_certchain *chain,
> +						l_cert_walk_cb_t cb,

I still don't understand what use the return value could be to the 
caller.  You don't even use the return value, so that reinforces my view 
that it is of dubious value.  Can we just make this return void?

Regards,
-Denis

Re: [PATCH 4/9] tls: Pass the verified peer identity to "ready" callback

[Denis Kenzior]

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

Hi Andrew,

On 11/27/2018 08:38 PM, Andrew Zaborowski wrote:
> Extract a human-readable peer identity string from the supplied
> certificate and pass it to the ready callback provided by API user to
> indicate that the peer has actually been authenticated.  For some users
> it will only really matter whether the value is non-NULL meaning that
> the peer is trusted by the locally configured CA, while for others,
> such as web browsers it's useful to display a human-readable peer
> identity to the user so they can see if a website is served by who they
> think it belongs to.  If we got a peer certificate but user supplied no
> CAs or peer couldn't be authenticated, don't bother to extract the
> identity string and pass NULL.
> 
> Until now we'd always pass NULL (there was a TODO comment) because
> we'd hoped we could at some point leverage the kernel's certificate
> parser to extract the subject name.  The newly added
> tls_get_peer_identity_str might also fit in cert.c instead of tls.c.
> ---
>   ell/tls.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 67 insertions(+), 3 deletions(-)
> 

I went ahead and applied this and patch 5.  But really the more 
interesting feature for us would be the certificate element name 
matching described in the appropriate iwd.git/TODO task.  But that one 
is a whole other story...

Regards,
-Denis

Re: [PATCH 6/9] cert: Rename l_certchain_foreach functions

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 28 Nov 2018 at 04:51, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/27/2018 08:38 PM, Andrew Zaborowski wrote:
> > Change the "foreach" to "walk" in l_certchain_foreach_from_leaf/_ca
> > and in l_cert_foreach_cb_t to not imply that the whole chain is always
> > traversed because we let the callbacks interrupt the iteration by
> > returning true.  This can be used for searches or trust verification, or
> > can be ignored if the whole chain is to be walked over.  Add comments
> > for the two functions affected.
> > ---
> >   ell/cert.c  | 18 ++++++++++++++----
> >   ell/cert.h  | 12 +++++-------
> >   ell/ell.sym |  4 ++--
> >   ell/tls.c   |  4 ++--
> >   4 files changed, 23 insertions(+), 15 deletions(-)
> >
> > diff --git a/ell/cert.c b/ell/cert.c
> > index 66433c2..0ecdfbb 100644
> > --- a/ell/cert.c
> > +++ b/ell/cert.c
> > @@ -253,8 +253,13 @@ LIB_EXPORT struct l_cert *l_certchain_get_leaf(struct l_certchain *chain)
> >       return chain->leaf;
> >   }
> >
> > -LIB_EXPORT bool l_certchain_foreach_from_leaf(struct l_certchain *chain,
> > -                                             l_cert_foreach_cb_t cb,
> > +/*
> > + * Call @cb for each certificate in the chain starting from the leaf
> > + * certificate.  If a call returns @true, stop and return @true to
> > + * the user, otherwise return @false.
> > + */
> > +LIB_EXPORT bool l_certchain_walk_from_leaf(struct l_certchain *chain,
> > +                                             l_cert_walk_cb_t cb,
>
> I still don't understand what use the return value could be to the
> caller.

The use would be to know if the search or verification succeeded (kind
of like in l_queue_find, l_queue_remove_if although these also return
the actual element)

> You don't even use the return value, so that reinforces my view
> that it is of dubious value.  Can we just make this return void?

Ok, can also be returned through user_data if needed.

Best regards

Re: [PATCH 4/9] tls: Pass the verified peer identity to "ready" callback

[Andrew Zaborowski]

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

On Wed, 28 Nov 2018 at 05:24, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/27/2018 08:38 PM, Andrew Zaborowski wrote:
> > Extract a human-readable peer identity string from the supplied
> > certificate and pass it to the ready callback provided by API user to
> > indicate that the peer has actually been authenticated.  For some users
> > it will only really matter whether the value is non-NULL meaning that
> > the peer is trusted by the locally configured CA, while for others,
> > such as web browsers it's useful to display a human-readable peer
> > identity to the user so they can see if a website is served by who they
> > think it belongs to.  If we got a peer certificate but user supplied no
> > CAs or peer couldn't be authenticated, don't bother to extract the
> > identity string and pass NULL.
> >
> > Until now we'd always pass NULL (there was a TODO comment) because
> > we'd hoped we could at some point leverage the kernel's certificate
> > parser to extract the subject name.  The newly added
> > tls_get_peer_identity_str might also fit in cert.c instead of tls.c.
> > ---
> >   ell/tls.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 67 insertions(+), 3 deletions(-)
> >
>
> I went ahead and applied this and patch 5.  But really the more
> interesting feature for us would be the certificate element name
> matching described in the appropriate iwd.git/TODO task.  But that one
> is a whole other story...

So with this patch we will be able to already kind of do this,
although maybe we need to pass more values for it to work better.  The
logic in this patch is to use organizationName and if that is not
available then commonName.  The TODO (is it really C8?) mentions
matching on dNSName or commonName.  Maybe we should pass all the
various strings and then separately a bool value (authenticated/not
authenticated) although they're pointless if the peer was not
authenticated.

On the other hand the CA certificate is optional in iwd configs and
until now we couldn't even check if peer identity was known so it's a
little premature to worry about it having a specific suffix :)

Also I'm not sure how effective this suffix matching actually is... if
a trusted CA is issuing certificates to rogue subjects then that CA
can no longer be trusted.   What if the CA issued a CA certificate?
The subject could then fake any subject string they want.  Maybe it's
the best we can do though.

Best regards

Re: [PATCH 1/9] tls: Drop local code to add hash OID, let kernel do it

[Andrew Zaborowski]

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

On Wed, 28 Nov 2018 at 04:45, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/27/2018 08:38 PM, Andrew Zaborowski wrote:
> > Drop tls.c code to build the PKCS#1 DigestInfo structure which basically
> > consisted of prepending the signature hash algorithm's OID to the hash.
> > Let the key API do it by setting the hash type in the l_key_sign and
> > l_key_verify calls.  Slightly simplify the tls_rsa_sign and
> > tls_rsa_verify code while doing this.
> > ---
> >   ell/tls.c | 164 ++++++++++++++++++------------------------------------
> >   1 file changed, 53 insertions(+), 111 deletions(-)
> >
>
> <snip>
>
> > @@ -1238,13 +1179,11 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
> >   static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
> >                               tls_get_hash_t get_hash)
> >   {
> > -     ssize_t result;
> > -     const struct tls_hash_algorithm *hash_type;
> > -     uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
> > +     ssize_t result = -EMSGSIZE;
> > +     enum l_checksum_type sign_checksum_type;
> >       uint8_t sign_input[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];
>
> Does this need to be updated?

It could be reduced to max(HANDSHAKE_HASH_MAX_SIZE, 36) although
HANDSHAKE_HASH_MAX_SIZE is now bigger than 36... for simplicity let's
do HANDSHAKE_HASH_MAX_SIZE + 36.

>
> >       size_t sign_input_len;
> > -     bool prepend_hash_type = false;
> > -     size_t expected_bytes;
> > +     uint8_t *ptr = out;
> >
> >       if (!tls->priv_key || !tls->priv_key_size) {
> >               TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, TLS_ALERT_BAD_CERT,
> > @@ -1253,54 +1192,49 @@ static ssize_t tls_rsa_sign(struct l_tls *tls, uint8_t *out, size_t len,
> >               return -ENOKEY;
> >       }
> >
> > -     expected_bytes = tls->priv_key_size + 2;
> > -
> >       if (tls->negotiated_version >= TLS_V12) {
> > -             hash_type = &tls_handshake_hash_data[tls->signature_hash];
> > -             get_hash(tls, hash_type->tls_id, hash, NULL, NULL);
> > +             const struct tls_hash_algorithm *hash_type =
> > +                     hash_type = &tls_handshake_hash_data[tls->signature_hash];
>
> What is going on here?

Oooh I don't know... I'll fix this.

>
> > +
> > +             get_hash(tls, hash_type->tls_id, sign_input, NULL, NULL);
> > +             sign_checksum_type = hash_type->l_id;
> > +             sign_input_len = hash_type->length;
> >
> > -             pkcs1_write_digest_info(hash_type->l_id,
> > -                                     sign_input, &sign_input_len,
> > -                                     hash, hash_type->length);
> > +             if (len < 2)
> > +                     goto error;
>
> Why do we check this after generating the signature?

Will move it sooner.

Best regards