| 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 |
| |