[PATCH 1/4] cert: Fix logic in cert_parse_asn1_time check

[PATCH 1/4] cert: Fix logic in cert_parse_asn1_time check

[Andrew Zaborowski]

Fix wrong operator in a condition that ended up being always true.
---
 ell/cert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/cert.c b/ell/cert.c
index b4f5df7..ab469c2 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -236,7 +236,7 @@ static uint64_t cert_parse_asn1_time(const uint8_t *data, size_t len,
 		return L_TIME_INVALID;
 
 	if (unlikely((len != i + 1 || data[i] != 'Z') &&
-			(len != i + 5 || data[i] != '+' || data[i] != '-')))
+			(len != i + 5 || (data[i] != '+' && data[i] != '-'))))
 		return L_TIME_INVALID;
 
 	tm.tm_year = (data[0] - '0') * 10 + (data[1] - '0');
-- 
2.34.1

[PATCH 2/4] cert: Check validity dates in l_certchain_verify

[Andrew Zaborowski]

Check the validity start and end dates of every certificate in the chain
being verified (must all be valid) and every trusted CA certificate
(ignore invalid ones, use the valid ones if any left).  The dates are
checked against current CLOCK_REALTIME value.  There may be use cases
for verifying a certificate chain without looking at the dates at all,
such as when the system clock hasn't been initialized in early boot and
that can be added if there's an actual user for that.  Until now we've
almost fully relied on the kernel to do all the verification and
consistency checks on the certificates and the kernel does not look at
validity dates.

Do these checks only in l_certchain_verify rather than directly in
l_cert_load_container_file() or l_pem_load_certificate_chain_from_data()
because 1. users may want to parse and display information about expired
certificates for a UI.  2. l_certchain_verify also returns a debug error
string which is going to be very helpful in diagnosing user issues from
logs, unlike the loader functions.  3. in some usages there may be a
longer periods of time between certchain loading, CA certificate loading
and/or verifying the chain against the CAs, and we mainly care about
them being valid at the time of the verification.
---
 ell/cert.c | 159 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 126 insertions(+), 33 deletions(-)

diff --git a/ell/cert.c b/ell/cert.c
index ab469c2..2c80c84 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -35,6 +35,7 @@
 #include "cipher.h"
 #include "pem-private.h"
 #include "time.h"
+#include "time-private.h"
 #include "utf8.h"
 #include "cert.h"
 #include "cert-private.h"
@@ -562,24 +563,25 @@ LIB_EXPORT void l_certchain_walk_from_ca(struct l_certchain *chain,
 			break;
 }
 
-static struct l_keyring *cert_set_to_keyring(struct l_queue *certs, char *error)
+static struct l_keyring *cert_set_to_keyring(struct l_cert **certs, char *error)
 {
 	struct l_keyring *ring;
-	const struct l_queue_entry *entry;
 	int i = 1;
+	int count;
 
 	ring = l_keyring_new();
 	if (!ring)
 		return NULL;
 
-	for (entry = l_queue_get_entries(certs); entry; entry = entry->next) {
-		struct l_cert *cert = entry->data;
+	for (count = 0; certs[count]; count++);
+
+	for (; *certs; certs++) {
+		struct l_cert *cert = *certs;
 		struct l_key *key = l_cert_get_pubkey(cert);
 
 		if (!key) {
 			sprintf(error, "Can't get public key from certificate "
-				"%i / %i in certificate set", i,
-				l_queue_length(certs));
+				"%i / %i in certificate set", i, count);
 			goto cleanup;
 		}
 
@@ -587,7 +589,7 @@ static struct l_keyring *cert_set_to_keyring(struct l_queue *certs, char *error)
 			l_key_free(key);
 			sprintf(error, "Can't link the public key from "
 				"certificate %i / %i to target keyring",
-				i, l_queue_length(certs));
+				i, count);
 			goto cleanup;
 		}
 
@@ -602,12 +604,10 @@ cleanup:
 	return NULL;
 }
 
-static bool cert_is_in_set(struct l_cert *cert, struct l_queue *set)
+static bool cert_is_in_set(struct l_cert *cert, struct l_cert **set)
 {
-	const struct l_queue_entry *entry;
-
-	for (entry = l_queue_get_entries(set); entry; entry = entry->next) {
-		struct l_cert *cert2 = entry->data;
+	for (; *set; set++) {
+		struct l_cert *cert2 = *set;
 
 		if (cert == cert2)
 			return true;
@@ -621,6 +621,35 @@ static bool cert_is_in_set(struct l_cert *cert, struct l_queue *set)
 	return false;
 }
 
+static struct l_cert **cert_set_filter_by_validity(struct l_queue *set,
+							uint64_t now,
+							int *out_total,
+							int *out_valid)
+{
+	const struct l_queue_entry *entry;
+	_auto_(l_free) struct l_cert **valid;
+
+	*out_total = l_queue_length(set);
+	*out_valid = 0;
+	valid = l_new(struct l_cert *, *out_total + 1);
+
+	for (entry = l_queue_get_entries(set); entry; entry = entry->next) {
+		struct l_cert *cert = entry->data;
+		uint64_t not_before;
+		uint64_t not_after;
+
+		if (!l_cert_get_valid_times(cert, &not_before, &not_after))
+			return NULL;
+
+		if (now < not_before || (not_after && now > not_after))
+			continue;
+
+		valid[(*out_valid)++] = cert;
+	}
+
+	return l_steal_ptr(valid);
+}
+
 static struct l_key *cert_try_link(struct l_cert *cert, struct l_keyring *ring)
 {
 	struct l_key *key;
@@ -655,22 +684,81 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
 	struct l_key *prev_key = NULL;
 	int verified = 0;
 	int ca_match = 0;
-	int i = 0;
+	int i;
 	static char error_buf[200];
+	int total = 0;
+	uint64_t now;
+	_auto_(l_free) struct l_cert **ca_certs_valid = NULL;
+	int ca_certs_total_count = 0;
+	int ca_certs_valid_count = 0;
 
 	if (unlikely(!chain || !chain->leaf))
 		RETURN_ERROR("Chain empty");
 
+	for (cert = chain->ca; cert; cert = cert->issued, total++);
+
+	now = time_realtime_now();
+
+	for (cert = chain->ca, i = 0; cert; cert = cert->issued, i++) {
+		uint64_t not_before;
+		uint64_t not_after;
+		char time_str[100];
+
+		if (unlikely(!l_cert_get_valid_times(cert, &not_before,
+							&not_after)))
+			RETURN_ERROR("Can't parse validity in certificate "
+					"%i / %i", i + 1, total);
+
+		if (unlikely(now < not_before)) {
+			time_t t = not_before / L_USEC_PER_SEC;
+			struct tm *tm = gmtime(&t);
+
+			if (!tm || !strftime(time_str, sizeof(time_str),
+						"%a %F %T UTC", tm))
+				strcpy(time_str, "<error>");
+
+			RETURN_ERROR("Certificate %i / %i not valid before %s",
+					i + 1, total, time_str);
+		}
+
+		if (unlikely(not_after && now > not_after)) {
+			time_t t = not_after / L_USEC_PER_SEC;
+			struct tm *tm = gmtime(&t);
+
+			if (!tm || !strftime(time_str, sizeof(time_str),
+						"%a %F %T UTC", tm))
+				strcpy(time_str, "<error>");
+
+			RETURN_ERROR("Certificate %i / %i expired on %s",
+					i + 1, total, time_str);
+		}
+	}
+
+	if (ca_certs) {
+		if (unlikely(l_queue_isempty(ca_certs)))
+			RETURN_ERROR("No trusted CA certificates");
+
+		ca_certs_valid = cert_set_filter_by_validity(ca_certs, now,
+							&ca_certs_total_count,
+							&ca_certs_valid_count);
+		if (unlikely(!ca_certs_valid))
+			RETURN_ERROR("Can't parse validity in CA cert(s)");
+
+		if (unlikely(!ca_certs_valid_count))
+			RETURN_ERROR("All trusted CA certs are expired or "
+					"not-yet-valid");
+
+		for (cert = chain->ca, i = 0; cert; cert = cert->issued, i++)
+			if (cert_is_in_set(cert, ca_certs_valid)) {
+				ca_match = i + 1;
+				break;
+			}
+	}
+
 	verify_ring = l_keyring_new();
 	if (!verify_ring)
 		RETURN_ERROR("Can't create verify keyring");
 
-	for (cert = chain->ca; cert; cert = cert->issued, i++)
-		if (cert_is_in_set(cert, ca_certs)) {
-			ca_match = i + 1;
-			break;
-		}
-
 	cert = chain->ca;
 
 	/*
@@ -690,7 +778,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
 	 * all of the trusted certificates into the kernel, link them
 	 * to @ca_ring or link @ca_ring to @verify_ring, instead we
 	 * load the first certificate into @verify_ring before we set
-	 * the restric mode on it, same as when no trusted CAs are
+	 * the restrict mode on it, same as when no trusted CAs are
 	 * provided.
 	 *
 	 * Note this happens to work around a kernel issue preventing
@@ -701,7 +789,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
 	 * the chain.
 	 */
 	if (ca_certs && !ca_match) {
-		ca_ring = cert_set_to_keyring(ca_certs, error_buf);
+		ca_ring = cert_set_to_keyring(ca_certs_valid, error_buf);
 		if (!ca_ring) {
 			if (error)
 				*error = error_buf;
@@ -754,23 +842,28 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
 	}
 
 	if (!prev_key) {
-		int total = 0;
-		char str[100];
-
-		for (cert = chain->ca; cert; cert = cert->issued, total++);
+		char str1[100];
+		char str2[100] = "";
 
 		if (ca_match)
-			snprintf(str, sizeof(str), "%i / %i matched a trusted "
-					"certificate, root not verified",
+			snprintf(str1, sizeof(str1), "%i / %i matched a trusted"
+					" certificate, root not verified",
 					ca_match, total);
 		else
-			snprintf(str, sizeof(str), "root %sverified against "
+			snprintf(str1, sizeof(str1), "root %sverified against "
 					"trusted CA(s)",
-					ca_certs && !ca_match && verified ? "" :
-					"not ");
-
-		RETURN_ERROR("Linking certificate %i / %i failed, %s",
-				verified + 1, total, str);
+					ca_certs && verified ? "" : "not ");
+
+		if (ca_certs && !ca_match && !verified &&
+				ca_certs_valid_count < ca_certs_total_count)
+			snprintf(str2, sizeof(str2), ", %i out of %i trused "
+					"CA(s) were expired or not-yet-valid",
+					ca_certs_total_count -
+					ca_certs_valid_count,
+					ca_certs_total_count);
+
+		RETURN_ERROR("Linking certificate %i / %i failed, %s%s",
+				verified + 1, total, str1, str2);
 	}
 
 	l_key_free(prev_key);
-- 
2.34.1

[PATCH 3/4] build: Generate an expired test certificate

[Andrew Zaborowski]

---
 .gitignore  |  2 ++
 Makefile.am | 29 +++++++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 76f10ae..1540012 100644
--- a/.gitignore
+++ b/.gitignore
@@ -67,6 +67,8 @@ unit/cert-*.csr
 unit/cert-*.srl
 unit/cert-*.crt
 unit/cert-*.p12
+unit/cert-ca.cnf
+unit/cert-ca-index*
 unit/ec-cert-*.pem
 unit/ec-cert-*.csr
 unit/key-*.dat
diff --git a/Makefile.am b/Makefile.am
index 596771a..7daebde 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -242,7 +242,8 @@ cert_files = unit/cert-chain.pem \
 			unit/cert-entity-pkcs12-rc4-sha384.p12 \
 			unit/cert-entity-pkcs12-pkcs5-sha512.p12 \
 			unit/cert-entity-combined.pem \
-			unit/cert-no-keyid.pem
+			unit/cert-no-keyid.pem \
+			unit/cert-expired.pem
 
 cert_checks = unit/cert-intca \
 			unit/cert-entity-int \
@@ -617,6 +618,29 @@ unit/cert-no-keyid.pem: unit/cert-no-keyid.csr unit/cert-ca2.pem unit/gencerts.c
 unit/cert-no-keyid: unit/cert-no-keyid.pem unit/cert-ca2.pem
 	$(AM_V_GEN)openssl verify -CAfile $(builddir)/unit/cert-ca2.pem $<
 
+unit/cert-expired.csr: unit/cert-client-key-pkcs1.pem unit/gencerts.cnf
+	$(AM_V_GEN)openssl req -new -extensions cert_ext \
+			-config $(srcdir)/unit/gencerts.cnf \
+			-subj '/O=Bar Example Organization/CN=Bar Example Organization/emailAddress=bar@mail.example' \
+			-key $< -out $@
+
+unit/cert-ca.cnf:
+	$(AM_V_GEN)echo -e '[example]\ndatabase = unit/cert-ca-index.txt\npolicy = dummy\nserial = dummy\n[dummy]' > $@
+
+unit/cert-expired.pem: unit/cert-expired.csr unit/cert-ca.pem unit/gencerts.cnf unit/cert-ca.cnf
+	$(AM_V_at)> unit/cert-ca-index.txt
+	$(AM_V_at)$(MKDIR_P) unit/cert-ca-tmp
+	$(AM_V_GEN)openssl ca -batch \
+			-config $(builddir)/unit/cert-ca.cnf -name example \
+			-cert $(builddir)/unit/cert-ca.pem \
+			-keyfile $(builddir)/unit/cert-ca-key.pem \
+			-outdir $(builddir)/unit/cert-ca-tmp \
+			-rand_serial -extensions cert_ext \
+			-extfile $(srcdir)/unit/gencerts.cnf -md sha256 \
+			-startdate 000101120000Z -enddate 010101120000Z \
+			-preserveDN -notext -in $< -out $@ 2> /dev/null
+	$(AM_V_at)rm -r unit/cert-ca-tmp unit/cert-ca-index.txt
+
 unit/cert-entity-pkcs12-nomac.p12: unit/cert-entity-int-key.pem unit/cert-entity-int.pem
 	$(AM_V_GEN)openssl pkcs12 -inkey $< -in $(builddir)/unit/cert-entity-int.pem -out $@ -export -passout pass:abc -nomac # defaut ciphers
 
@@ -660,7 +684,8 @@ endif
 
 clean-local:
 	-rm -f unit/ec-cert*.pem unit/ec-cert-*.csr \
-		unit/cert-*.pem unit/cert-*.csr unit/cert-*.srl unit/key-*.dat
+		unit/cert-*.pem unit/cert-*.csr unit/cert-*.srl unit/key-*.dat \
+		unit/cert-ca-index* unit/cert-ca.cnf
 
 maintainer-clean-local:
 	-rm -rf build-aux
-- 
2.34.1

[PATCH 4/4] unit: Minimal l_certchain_verify validity dates test

[Andrew Zaborowski]

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

diff --git a/unit/test-tls.c b/unit/test-tls.c
index 53d4f38..e089859 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -231,7 +231,10 @@ static void test_certificates(const void *data)
 	struct l_queue *wrongca;
 	struct l_queue *wrongca2;
 	struct l_queue *twocas;
+	struct l_cert *expired;
+	struct l_queue *mixedcas;
 	struct l_certchain *chain;
+	struct l_certchain *expiredchain;
 
 	cacert = l_pem_load_certificate_list(CERTDIR "cert-ca.pem");
 	assert(cacert && !l_queue_isempty(cacert));
@@ -245,9 +248,18 @@ static void test_certificates(const void *data)
 	twocas = l_pem_load_certificate_list(CERTDIR "cert-chain.pem");
 	assert(twocas);
 
+	mixedcas = l_pem_load_certificate_list(CERTDIR "cert-chain.pem");
+	assert(mixedcas);
+	expired = load_cert_file(CERTDIR "cert-expired.pem");
+	assert(expired);
+	l_queue_push_tail(mixedcas, expired);
+
 	chain = l_pem_load_certificate_chain(CERTDIR "cert-server.pem");
 	assert(chain);
 
+	expiredchain = l_pem_load_certificate_chain(CERTDIR "cert-expired.pem");
+	assert(expiredchain);
+
 	assert(!l_certchain_verify(chain, wrongca, NULL));
 	assert(l_certchain_verify(chain, cacert, NULL));
 	assert(l_certchain_verify(chain, NULL, NULL));
@@ -294,6 +306,7 @@ static void test_certificates(const void *data)
 	assert(l_certchain_verify(chain, cacert, NULL));
 	assert(l_certchain_verify(chain, NULL, NULL));
 	assert(l_certchain_verify(chain, twocas, NULL));
+	assert(l_certchain_verify(chain, mixedcas, NULL));
 
 	l_certchain_free(chain);
 	l_queue_destroy(cacert, (l_queue_destroy_func_t) l_cert_free);
@@ -317,12 +330,17 @@ static void test_certificates(const void *data)
 	assert(l_certchain_verify(chain, cacert, NULL));
 	assert(l_certchain_verify(chain, NULL, NULL));
 	assert(!l_certchain_verify(chain, twocas, NULL));
+	assert(!l_certchain_verify(chain, mixedcas, NULL));
+
+	assert(!l_certchain_verify(expiredchain, NULL, NULL));
 
 	l_certchain_free(chain);
+	l_certchain_free(expiredchain);
 	l_queue_destroy(cacert, (l_queue_destroy_func_t) l_cert_free);
 	l_queue_destroy(wrongca, (l_queue_destroy_func_t) l_cert_free);
 	l_queue_destroy(wrongca2, (l_queue_destroy_func_t) l_cert_free);
 	l_queue_destroy(twocas, (l_queue_destroy_func_t) l_cert_free);
+	l_queue_destroy(mixedcas, (l_queue_destroy_func_t) l_cert_free);
 }
 
 static void test_ec_certificates(const void *data)
-- 
2.34.1

Re: [PATCH 2/4] cert: Check validity dates in l_certchain_verify

[Denis Kenzior]

On 10/31/22 05:53, Andrew Zaborowski wrote:
> Check the validity start and end dates of every certificate in the chain
> being verified (must all be valid) and every trusted CA certificate
> (ignore invalid ones, use the valid ones if any left).  The dates are
> checked against current CLOCK_REALTIME value.  There may be use cases
> for verifying a certificate chain without looking at the dates at all,
> such as when the system clock hasn't been initialized in early boot and
> that can be added if there's an actual user for that.  Until now we've
> almost fully relied on the kernel to do all the verification and
> consistency checks on the certificates and the kernel does not look at
> validity dates.
> 
> Do these checks only in l_certchain_verify rather than directly in
> l_cert_load_container_file() or l_pem_load_certificate_chain_from_data()
> because 1. users may want to parse and display information about expired
> certificates for a UI.  2. l_certchain_verify also returns a debug error
> string which is going to be very helpful in diagnosing user issues from
> logs, unlike the loader functions.  3. in some usages there may be a
> longer periods of time between certchain loading, CA certificate loading
> and/or verifying the chain against the CAs, and we mainly care about
> them being valid at the time of the verification.
> ---
>   ell/cert.c | 159 ++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 126 insertions(+), 33 deletions(-)

<snip>

> @@ -655,22 +684,81 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
>   	struct l_key *prev_key = NULL;
>   	int verified = 0;
>   	int ca_match = 0;
> -	int i = 0;
> +	int i;
>   	static char error_buf[200];

I bumped this to 1024 since gcc was complaining:
ell/cert.c: In function 'l_certchain_verify':
ell/cert.c:865:30: error: '%s' directive output may be truncated writing up to 
99 bytes into a region of size between 47 and 166 [-Werror=format-truncation=]
   865 |                 RETURN_ERROR("Linking certificate %i / %i failed, %s%s",
       |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   866 |                                 verified + 1, total, str1, str2);
       |                                                            ~~~~
ell/cert.c:672:64: note: in definition of macro 'RETURN_ERROR'
   672 |                     snprintf(error_buf, sizeof(error_buf), msg, ## args); \
       |                                                            ^~~

ell/cert.c:865:69: note: format string is defined here
   865 |               RETURN_ERROR("Linking certificate %i / %i failed, %s%s",
       |                                                                   ^~

ell/cert.c:672:25: note: 'snprintf' output between 35 and 253 bytes into a 
destination of size 200
   672 |                         snprintf(error_buf, sizeof(error_buf), msg, ## 
args); \
       | 
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ell/cert.c:865:17: note: in expansion of macro 'RETURN_ERROR'
   865 |                 RETURN_ERROR("Linking certificate %i / %i failed, %s%s",
       |                 ^~~~~~~~~~~~

> +	int total = 0;
> +	uint64_t now;
> +	_auto_(l_free) struct l_cert **ca_certs_valid = NULL;
> +	int ca_certs_total_count = 0;
> +	int ca_certs_valid_count = 0;
>   

All 4 applied, thanks.

Regards,
-Denis