From 09b40be6e0e0a59ba4bd764067eb353241043a70 Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
Date: Mon, 28 Dec 2020 12:14:13 +0100
Subject: [PATCH] gnutls_x509_trust_list_verify_crt2: ignore duplicate
 certificates

The commit ebb19db9165fed30d73c83bab1b1b8740c132dfd caused a
regression, where duplicate certificates in a certificate chain are no
longer ignored but treated as a non-contiguous segment and that
results in calling the issuer callback, or a verification failure.

This adds a mechanism to record certificates already seen in the
chain, and skip them while still allow the caller to inject missing
certificates.

Signed-off-by: Daiki Ueno <ueno@gnu.org>
Co-authored-by: Andreas Metzler <ametzler@debian.org>
---
 lib/x509/common.c          |   8 ++
 lib/x509/verify-high.c     | 157 +++++++++++++++++++++++++++++++------
 tests/missingissuer.c      |   2 +
 tests/test-chains-issuer.h | 101 +++++++++++++++++++++++-
 4 files changed, 245 insertions(+), 23 deletions(-)

diff --git a/lib/x509/common.c b/lib/x509/common.c
index 3301aaad0c..10c8db53c0 100644
--- a/lib/x509/common.c
+++ b/lib/x509/common.c
@@ -1758,6 +1758,14 @@ unsigned int _gnutls_sort_clist(gnutls_x509_crt_t *clist,
 	 * increasing DEFAULT_MAX_VERIFY_DEPTH.
 	 */
 	for (i = 0; i < clist_size; i++) {
+		/* Self-signed certificate found in the chain; skip it
+		 * as it should only appear in the trusted set.
+		 */
+		if (gnutls_x509_crt_check_issuer(clist[i], clist[i])) {
+			_gnutls_cert_log("self-signed cert found", clist[i]);
+			continue;
+		}
+
 		for (j = 1; j < clist_size; j++) {
 			if (i == j)
 				continue;
diff --git a/lib/x509/verify-high.c b/lib/x509/verify-high.c
index 588e7ee0dc..9a16e6b42a 100644
--- a/lib/x509/verify-high.c
+++ b/lib/x509/verify-high.c
@@ -67,6 +67,80 @@ struct gnutls_x509_trust_list_iter {
 
 #define DEFAULT_SIZE 127
 
+struct cert_set_node_st {
+	gnutls_x509_crt_t *certs;
+	unsigned int size;
+};
+
+struct cert_set_st {
+	struct cert_set_node_st *node;
+	unsigned int size;
+};
+
+static int
+cert_set_init(struct cert_set_st *set, unsigned int size)
+{
+	memset(set, 0, sizeof(*set));
+
+	set->size = size;
+	set->node = gnutls_calloc(size, sizeof(*set->node));
+	if (!set->node) {
+		return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR);
+	}
+
+	return 0;
+}
+
+static void
+cert_set_deinit(struct cert_set_st *set)
+{
+	size_t i;
+
+	for (i = 0; i < set->size; i++) {
+		gnutls_free(set->node[i].certs);
+	}
+
+	gnutls_free(set->node);
+}
+
+static bool
+cert_set_contains(struct cert_set_st *set, const gnutls_x509_crt_t cert)
+{
+	size_t hash, i;
+
+	hash = hash_pjw_bare(cert->raw_dn.data, cert->raw_dn.size);
+	hash %= set->size;
+
+	for (i = 0; i < set->node[hash].size; i++) {
+		if (unlikely(gnutls_x509_crt_equals(set->node[hash].certs[i], cert))) {
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static int
+cert_set_add(struct cert_set_st *set, const gnutls_x509_crt_t cert)
+{
+	size_t hash;
+
+	hash = hash_pjw_bare(cert->raw_dn.data, cert->raw_dn.size);
+	hash %= set->size;
+
+	set->node[hash].certs =
+		gnutls_realloc_fast(set->node[hash].certs,
+				    (set->node[hash].size + 1) *
+				    sizeof(*set->node[hash].certs));
+	if (!set->node[hash].certs) {
+		return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR);
+	}
+	set->node[hash].certs[set->node[hash].size] = cert;
+	set->node[hash].size++;
+
+	return 0;
+}
+
 /**
  * gnutls_x509_trust_list_init:
  * @list: A pointer to the type to be initialized
@@ -1328,6 +1402,7 @@ gnutls_x509_trust_list_verify_crt2(gnutls_x509_trust_list_t list,
 	unsigned have_set_name = 0;
 	unsigned saved_output;
 	gnutls_datum_t ip = {NULL, 0};
+	struct cert_set_st cert_set = { NULL, 0 };
 
 	if (cert_list == NULL || cert_list_size < 1)
 		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
@@ -1376,36 +1451,68 @@ gnutls_x509_trust_list_verify_crt2(gnutls_x509_trust_list_t list,
 	memcpy(sorted, cert_list, cert_list_size * sizeof(gnutls_x509_crt_t));
 	cert_list = sorted;
 
+	ret = cert_set_init(&cert_set, DEFAULT_MAX_VERIFY_DEPTH);
+	if (ret < 0) {
+		return ret;
+	}
+
 	for (i = 0; i < cert_list_size &&
-		     cert_list_size <= DEFAULT_MAX_VERIFY_DEPTH; i++) {
-		if (!(flags & GNUTLS_VERIFY_DO_NOT_ALLOW_UNSORTED_CHAIN)) {
-			unsigned int sorted_size;
+		     cert_list_size <= DEFAULT_MAX_VERIFY_DEPTH; ) {
+		unsigned int sorted_size = 1;
+		unsigned int j;
+		gnutls_x509_crt_t issuer;
 
+		if (!(flags & GNUTLS_VERIFY_DO_NOT_ALLOW_UNSORTED_CHAIN)) {
 			sorted_size = _gnutls_sort_clist(&cert_list[i],
 							 cert_list_size - i);
-			i += sorted_size - 1;
 		}
 
-		if (i == cert_list_size - 1) {
-			gnutls_x509_crt_t issuer;
-
-			/* If it is the last certificate and its issuer is
-			 * known, don't need to run issuer callback. */
-			if (_gnutls_trust_list_get_issuer(list,
-							  cert_list[i],
-							  &issuer,
-							  0) == 0) {
+		/* Remove duplicates. Start with index 1, as the first element
+		 * may be re-checked after issuer retrieval. */
+		for (j = 1; j < sorted_size; j++) {
+			if (cert_set_contains(&cert_set, cert_list[i + j])) {
+				if (i + j < cert_list_size - 1) {
+					memmove(&cert_list[i + j],
+						&cert_list[i + j + 1],
+						sizeof(cert_list[i]));
+				}
+				cert_list_size--;
 				break;
 			}
-		} else if (gnutls_x509_crt_check_issuer(cert_list[i],
-							cert_list[i + 1])) {
-			/* There is no gap between this and the next
-			 * certificate. */
+		}
+		/* Found a duplicate, try again with the same index. */
+		if (j < sorted_size) {
+			continue;
+		}
+
+		/* Record the certificates seen. */
+		for (j = 0; j < sorted_size; j++, i++) {
+			ret = cert_set_add(&cert_set, cert_list[i]);
+			if (ret < 0) {
+				goto cleanup;
+			}
+		}
+
+		/* If the issuer of the certificate is known, no need
+		 * for further processing. */
+		if (_gnutls_trust_list_get_issuer(list,
+						  cert_list[i - 1],
+						  &issuer,
+						  0) == 0) {
+			cert_list_size = i;
+			break;
+		}
+
+		/* If there is no gap between this and the next certificate,
+		 * proceed with the next certificate. */
+		if (i < cert_list_size &&
+		    gnutls_x509_crt_check_issuer(cert_list[i - 1],
+						 cert_list[i])) {
 			continue;
 		}
 
 		ret = retrieve_issuers(list,
-				       cert_list[i],
+				       cert_list[i - 1],
 				       &retrieved[retrieved_size],
 				       DEFAULT_MAX_VERIFY_DEPTH -
 				       MAX(retrieved_size,
@@ -1413,15 +1520,20 @@ gnutls_x509_trust_list_verify_crt2(gnutls_x509_trust_list_t list,
 		if (ret < 0) {
 			break;
 		} else if (ret > 0) {
-			memmove(&cert_list[i + 1 + ret],
-				&cert_list[i + 1],
-				(cert_list_size - i - 1) *
+			assert((unsigned int)ret <=
+			       DEFAULT_MAX_VERIFY_DEPTH - cert_list_size);
+			memmove(&cert_list[i + ret],
+				&cert_list[i],
+				(cert_list_size - i) *
 				sizeof(gnutls_x509_crt_t));
-			memcpy(&cert_list[i + 1],
+			memcpy(&cert_list[i],
 			       &retrieved[retrieved_size],
 			       ret * sizeof(gnutls_x509_crt_t));
 			retrieved_size += ret;
 			cert_list_size += ret;
+
+			/* Start again from the end of the previous segment. */
+			i--;
 		}
 	}
 
@@ -1581,6 +1693,7 @@ gnutls_x509_trust_list_verify_crt2(gnutls_x509_trust_list_t list,
 	for (i = 0; i < retrieved_size; i++) {
 		gnutls_x509_crt_deinit(retrieved[i]);
 	}
+	cert_set_deinit(&cert_set);
 	return ret;
 }
 
diff --git a/tests/missingissuer.c b/tests/missingissuer.c
index f21e2b6b0c..226d095929 100644
--- a/tests/missingissuer.c
+++ b/tests/missingissuer.c
@@ -145,6 +145,8 @@ void doit(void)
 		printf("[%d]: Chain '%s'...\n", (int)i, chains[i].name);
 
 		for (j = 0; chains[i].chain[j]; j++) {
+			assert(j < MAX_CHAIN);
+
 			if (debug > 2)
 				printf("\tAdding certificate %d...", (int)j);
 
diff --git a/tests/test-chains-issuer.h b/tests/test-chains-issuer.h
index 543e2d71fb..bf1e65c956 100644
--- a/tests/test-chains-issuer.h
+++ b/tests/test-chains-issuer.h
@@ -24,7 +24,7 @@
 #ifndef GNUTLS_TESTS_TEST_CHAINS_ISSUER_H
 #define GNUTLS_TESTS_TEST_CHAINS_ISSUER_H
 
-#define MAX_CHAIN 6
+#define MAX_CHAIN 15
 
 #define SERVER_CERT "-----BEGIN CERTIFICATE-----\n"			\
 	"MIIDATCCAbmgAwIBAgIUQdvdegP8JFszFHLfV4+lrEdafzAwPQYJKoZIhvcNAQEK\n" \
@@ -338,11 +338,102 @@ static const char *missing_middle_unrelated_extra_insert[] = {
 	NULL,
 };
 
+static const char *missing_middle_single_duplicate[] = {
+	SERVER_CERT,
+	SERVER_CERT,
+	CA_CERT_5,
+	CA_CERT_5,
+	CA_CERT_4,
+	CA_CERT_4,
+	CA_CERT_2,
+	CA_CERT_2,
+	CA_CERT_1,
+	CA_CERT_1,
+	NULL,
+};
+
+static const char *missing_middle_multiple_duplicate[] = {
+	SERVER_CERT,
+	SERVER_CERT,
+	CA_CERT_5,
+	CA_CERT_5,
+	CA_CERT_4,
+	CA_CERT_4,
+	CA_CERT_1,
+	CA_CERT_1,
+	NULL,
+};
+
+static const char *missing_last_single_duplicate[] = {
+	SERVER_CERT,
+	SERVER_CERT,
+	CA_CERT_5,
+	CA_CERT_5,
+	CA_CERT_4,
+	CA_CERT_4,
+	CA_CERT_3,
+	CA_CERT_3,
+	CA_CERT_2,
+	CA_CERT_2,
+	NULL,
+};
+
+static const char *missing_last_multiple_duplicate[] = {
+	SERVER_CERT,
+	SERVER_CERT,
+	CA_CERT_5,
+	CA_CERT_5,
+	CA_CERT_4,
+	CA_CERT_4,
+	CA_CERT_3,
+	CA_CERT_3,
+	NULL,
+};
+
+static const char *missing_skip_single_duplicate[] = {
+	SERVER_CERT,
+	SERVER_CERT,
+	CA_CERT_5,
+	CA_CERT_5,
+	CA_CERT_3,
+	CA_CERT_3,
+	CA_CERT_1,
+	CA_CERT_1,
+	NULL,
+};
+
+static const char *missing_skip_multiple_duplicate[] = {
+	SERVER_CERT,
+	SERVER_CERT,
+	CA_CERT_5,
+	CA_CERT_5,
+	CA_CERT_3,
+	CA_CERT_3,
+	NULL,
+};
+
 static const char *missing_ca[] = {
 	CA_CERT_0,
 	NULL,
 };
 
+static const char *middle_single_duplicate_ca[] = {
+	SERVER_CERT,
+	CA_CERT_5,
+	CA_CERT_0,
+	CA_CERT_4,
+	CA_CERT_0,
+	CA_CERT_2,
+	CA_CERT_0,
+	CA_CERT_1,
+	NULL,
+};
+
+static const char *missing_middle_single_duplicate_ca_unrelated_insert[] = {
+	CA_CERT_0,
+	NULL,
+};
+
 static struct chains {
 	const char *name;
 	const char **chain;
@@ -377,6 +468,14 @@ static struct chains {
 	{ "skip multiple unsorted", missing_skip_multiple_unsorted, missing_skip_multiple_insert, missing_ca, 0, 0 },
 	{ "unrelated", missing_middle_single, missing_middle_unrelated_insert, missing_ca, 0, GNUTLS_CERT_INVALID | GNUTLS_CERT_SIGNER_NOT_FOUND },
 	{ "unrelated extra", missing_middle_single, missing_middle_unrelated_extra_insert, missing_ca, 0, 0 },
+	{ "middle single duplicate", missing_middle_single_duplicate, missing_middle_single_insert, missing_ca, 0, 0 },
+	{ "middle multiple duplicate", missing_middle_multiple_duplicate, missing_middle_multiple_insert, missing_ca, 0, 0 },
+	{ "last single duplicate", missing_last_single_duplicate, missing_last_single_insert, missing_ca, 0, 0 },
+	{ "last multiple duplicate", missing_last_multiple_duplicate, missing_last_multiple_insert, missing_ca, 0, 0 },
+	{ "skip single duplicate", missing_skip_single_duplicate, missing_skip_single_insert, missing_ca, 0, 0 },
+	{ "skip multiple duplicate", missing_skip_multiple_duplicate, missing_skip_multiple_insert, missing_ca, 0, 0 },
+	{ "middle single duplicate ca", middle_single_duplicate_ca, missing_middle_single_insert, missing_ca, 0, 0 },
+	{ "middle single duplicate ca - insert unrelated", middle_single_duplicate_ca, missing_middle_single_duplicate_ca_unrelated_insert, missing_ca, 0, GNUTLS_CERT_INVALID | GNUTLS_CERT_SIGNER_NOT_FOUND },
 	{ NULL, NULL, NULL, NULL },
 };
 
-- 
GitLab