[PATCH 1/3] cert: Try TLS format in l_cert_load_container_file

[PATCH 1/3] cert: Try TLS format in l_cert_load_container_file

[Andrew Zaborowski]

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

---
 ell/cert.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/ell/cert.c b/ell/cert.c
index 14631b2..141ea1c 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -35,6 +35,8 @@
 #include "pem-private.h"
 #include "cert.h"
 #include "cert-private.h"
+#include "tls.h"
+#include "tls-private.h"
 #include "missing.h"
 
 #define X509_CERTIFICATE_POS			0
@@ -1635,14 +1637,34 @@ LIB_EXPORT bool l_cert_load_container_file(const char *filename,
 		if (err != -ENOMSG)
 			goto close;
 
-		/* Try PEM */
+		/* Try other formats */
+	}
+
+	/*
+	 * For backwards compatibility try the TLS internal struct Certificate
+	 * format as may be captured by PCAP (no future support guaranteed).
+	 */
+	if (out_certchain && !password && file.st.st_size &&
+			tls_parse_certificate_list(file.data, file.st.st_size,
+							out_certchain) == 0) {
+		error = false;
+
+		if (out_privkey)
+			*out_privkey = NULL;
+
+		if (out_encrypted)
+			*out_encrypted = false;
+
+		goto close;
 	}
 
 	/*
 	 * RFC 7486 allows whitespace and possibly other data before the
 	 * PEM "encapsulation boundary" so rather than check if the start
 	 * of the data looks like PEM, we fall back to this format if the
-	 * data didn't look like anything else we knew about.
+	 * data didn't look like anything else we knew about.  Note this
+	 * succeeds for empty files and files without any PEM markers,
+	 * returning NULL chain and privkey.
 	 */
 	if (cert_try_load_pem_format((const char *) file.data, file.st.st_size,
 					password, out_certchain, out_privkey,
-- 
2.27.0

[PATCH 2/3] tools: Convert certchain-verify to l_cert_load_container_file

[Andrew Zaborowski]

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

Drop the local certificate chain loading code in favour of
l_cert_load_container_file to support the format dumped by
l_tls_set_cert_dump_path and whlie there also other formats.
---
 tools/certchain-verify.c | 68 +++-------------------------------------
 1 file changed, 5 insertions(+), 63 deletions(-)

diff --git a/tools/certchain-verify.c b/tools/certchain-verify.c
index 1a1dab1..732c187 100644
--- a/tools/certchain-verify.c
+++ b/tools/certchain-verify.c
@@ -23,72 +23,16 @@
 #endif
 
 #include <stdio.h>
-#include <errno.h>
-#include <stdint.h>
-#include <stdbool.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <fcntl.h>
-#include <unistd.h>
-#include <sys/mman.h>
 
 #include <ell/ell.h>
-#include "ell/tls-private.h"
-
-static int load_cert_chain(const char *file, struct l_certchain **certchain)
-{
-	int fd;
-	struct stat st;
-	char *data;
-	int err;
-
-	fd = open(file, O_RDONLY);
-	if (fd < 0) {
-		fprintf(stderr, "Could not open %s: %s\n",
-						file, strerror(errno));
-		return -errno;
-	}
-
-	if (fstat(fd, &st) < 0) {
-		err = -errno;
-		fprintf(stderr, "Could not stat %s: %s\n",
-						file, strerror(errno));
-		goto close_file;
-	}
-
-	if (st.st_size == 0) {
-		err = -EINVAL;
-		fprintf(stderr, "Certificate file %s is empty!\n", file);
-		goto close_file;
-	}
-
-	data = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
-	if (data == MAP_FAILED) {
-		err = -errno;
-		fprintf(stderr, "Could not mmap %s: %s\n",
-						file, strerror(errno));
-		goto close_file;
-	}
-
-	err = tls_parse_certificate_list(data, st.st_size, certchain);
-	if (err < 0)
-		fprintf(stderr, "Could not parse certificate list: %s\n",
-						strerror(-err));
-
-	munmap(data, st.st_size);
-
-close_file:
-	close(fd);
-	return err;
-}
 
 static void usage(const char *bin)
 {
-	printf("%s - TLS certificate chain verification utility\n\n", bin);
+	printf("%s - 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"
-		"  <raw certificates file> - Certificates obtained from PCAP\n"
+	printf("Usage: %s [options] <ca_cert file> <certchain container>\n"
+		"  <ca_cert file> - local CA Certificates to validate against\n"
+		"  <certchain container> - certificate chain to verify\n"
 		"  --help\n\n", bin);
 }
 
@@ -97,7 +41,6 @@ int main(int argc, char *argv[])
 	int status = EXIT_FAILURE;
 	struct l_certchain *certchain;
 	struct l_queue *ca_certs;
-	int err;
 	const char *error_str;
 
 	if (argc != 3) {
@@ -107,8 +50,7 @@ int main(int argc, char *argv[])
 
 	l_log_set_stderr();
 
-	err = load_cert_chain(argv[2], &certchain);
-	if (err < 0)
+	if (!l_cert_load_container_file(argv[2], NULL, &certchain, NULL, NULL))
 		goto done;
 
 	if (!certchain) {
-- 
2.27.0

[PATCH 3/3] tls: Proceed after l_certchain_verify failure if no CA certs

[Andrew Zaborowski]

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

Until the mainstream kernel can handle the occasionally used
certificates without the AKID extension (both root, which is legal, and
non-root, which is iffy but still happens) don't fail on peer
certificate chain verification failure when CA certificates were not
provided.  Knowing that the chain is self-consistent alone doesn't
authenticate the peer in any way.  Only warn when it looks like the
chain is bad, but it parses and we can get the peer public key from it
for later key derivation etc.

Some patches for the kernel problem have been on the kernel lists for a
long while but have not been merged so far.
---
 ell/tls.c | 52 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/ell/tls.c b/ell/tls.c
index 2c3274a..c246f1f 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -1892,7 +1892,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 					const uint8_t *buf, size_t len)
 {
 	size_t total;
-	struct l_certchain *certchain = NULL;
+	_auto_(l_certchain_free) struct l_certchain *certchain = NULL;
 	struct l_cert *leaf;
 	size_t der_len;
 	const uint8_t *der;
@@ -1914,7 +1914,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 		TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
 				"Error decoding peer certificate chain");
 
-		goto done;
+		return;
 	}
 
 	/*
@@ -1930,12 +1930,12 @@ static void tls_handle_certificate(struct l_tls *tls,
 			TLS_DISCONNECT(TLS_ALERT_HANDSHAKE_FAIL, 0,
 					"Server sent no certificate chain");
 
-			goto done;
+			return;
 		}
 
 		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_KEY_EXCHANGE);
 
-		goto done;
+		return;
 	}
 
 	if (tls->cert_dump_path) {
@@ -1956,12 +1956,33 @@ static void tls_handle_certificate(struct l_tls *tls,
 	 * against our CAs if we have any.
 	 */
 	if (!l_certchain_verify(certchain, tls->ca_certs, &error_str)) {
-		TLS_DISCONNECT(TLS_ALERT_BAD_CERT, 0,
-				"Peer certchain verification failed "
-				"consistency check%s: %s", tls->ca_certs ?
-				" or against local CA certs" : "", error_str);
+		if (tls->ca_certs) {
+			TLS_DISCONNECT(TLS_ALERT_BAD_CERT, 0,
+					"Peer certchain verification failed "
+					"consistency check%s: %s",
+					tls->ca_certs ?
+					" or against local CA certs" : "",
+					error_str);
 
-		goto done;
+			return;
+		}
+
+		/*
+		 * Until the mainstream kernel can handle the occasionally
+		 * used certificates without the AKID extension (both root,
+		 * which is legal, and non-root, which is iffy but still
+		 * happens) don't fail on peer certificate chain verification
+		 * failure when CA certificates were not provided.  Knowing
+		 * that the chain is self-consistent alone doesn't
+		 * authenticate the peer in any way.  Only warn when it looks
+		 * like the chain is bad but parses and we can get the peer
+		 * public key from it below.
+		 */
+		TLS_DEBUG("Peer certchain verification failed (%s.)  No local "
+				"CA certs provided so proceeding anyway.  This "
+				"failure can signal a security issue or a "
+				"known kernel problem with some certificates.",
+				error_str);
 	}
 
 	/*
@@ -1978,7 +1999,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 				"pending cipher suite %s",
 				tls->pending.cipher_suite->name);
 
-		goto done;
+		return;
 	}
 
 	if (tls->subject_mask && !tls_cert_domains_match_mask(leaf,
@@ -1992,7 +2013,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 		l_free(mask);
 		l_free(subject_str);
 
-		goto done;
+		return;
 	}
 
 	/* Save the end-entity certificate and free the chain */
@@ -2004,7 +2025,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 		TLS_DISCONNECT(TLS_ALERT_UNSUPPORTED_CERT, 0,
 				"Error loading peer public key to kernel");
 
-		goto done;
+		return;
 	}
 
 	if (!l_key_get_info(tls->peer_pubkey, L_KEY_RSA_PKCS1_V1_5,
@@ -2013,7 +2034,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 		TLS_DISCONNECT(TLS_ALERT_INTERNAL_ERROR, 0,
 				"Can't l_key_get_info for peer public key");
 
-		goto done;
+		return;
 	}
 
 	tls->peer_pubkey_size /= 8;
@@ -2024,14 +2045,11 @@ static void tls_handle_certificate(struct l_tls *tls,
 	else
 		TLS_SET_STATE(TLS_HANDSHAKE_WAIT_HELLO_DONE);
 
-	goto done;
+	return;
 
 decode_error:
 	TLS_DISCONNECT(TLS_ALERT_DECODE_ERROR, 0,
 			"TLS_CERTIFICATE decode error");
-
-done:
-	l_certchain_free(certchain);
 }
 
 static void tls_handle_certificate_request(struct l_tls *tls,
-- 
2.27.0

Re: [PATCH 1/3] cert: Try TLS format in l_cert_load_container_file

[Denis Kenzior]

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

Hi Andrew,

On 4/28/21 12:30 PM, Andrew Zaborowski wrote:
> ---
>   ell/cert.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
> 

All applied, thanks.

Regards,
-Denis