From 09b40be6e0e0a59ba4bd764067eb353241043a70 Mon Sep 17 00:00:00 2001 From: Daiki Ueno 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 Co-authored-by: Andreas Metzler --- 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 3301aaad0..10c8db53c 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 588e7ee0d..9a16e6b42 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 f21e2b6b0..226d09592 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 543e2d71f..bf1e65c95 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 }, }; -- 2.29.2