diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index 8e7b6dd29f..b461992334 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -119,10 +119,10 @@ /*%< * The maximum number of EDNS options we allow to set. Reserve space for the - * options we know about. Extended DNS Errors may occur multiple times, but we - * will set only one per message (for now). + * options we know about. Extended DNS Errors may occur multiple times, see + * DNS_EDE_MAX_ERRORS. */ -#define DNS_EDNSOPTIONS 8 +#define DNS_EDNSOPTIONS 7 + DNS_EDE_MAX_ERRORS /*%< EDNS0 extended DNS errors */ #define DNS_EDE_OTHER 0 /*%< Other Error */ @@ -170,6 +170,8 @@ typedef ISC_LIST(dns_ede_t) dns_edelist_t; */ #define DNS_EDE_EXTRATEXT_LEN 64 +#define DNS_EDE_MAX_ERRORS 3 + #define DNS_MESSAGE_REPLYPRESERVE (DNS_MESSAGEFLAG_RD | DNS_MESSAGEFLAG_CD) #define DNS_MESSAGEEXTFLAG_REPLYPRESERVE (DNS_MESSAGEEXTFLAG_DO) diff --git a/lib/dns/message.c b/lib/dns/message.c index f095c2decf..94f2d92ab1 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -5029,19 +5029,9 @@ dns_ede_append(isc_mem_t *mctx, dns_edelist_t *list, uint16_t info_code, }; if (extra_text) { - size_t len = strlen(extra_text); - - if (len >= DNS_EDE_EXTRATEXT_LEN) { - isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, - DNS_LOGMODULE_MESSAGE, ISC_LOG_PRINTTIME, - "truncate EDE code %hu text: %s", - info_code, extra_text); - len = DNS_EDE_EXTRATEXT_LEN - 1; - } - - ede->extra_text = isc_mem_allocate(mctx, len + 1); - strncpy(ede->extra_text, extra_text, len); - ede->extra_text[len] = '\0'; + ede->extra_text = isc_mem_allocate(mctx, + strlen(extra_text) + 1); + strcpy(ede->extra_text, extra_text); } ISC_LIST_APPEND(*list, ede, link); diff --git a/lib/ns/client.c b/lib/ns/client.c index a51b82b889..63b806d522 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -225,27 +225,54 @@ ns_client_settimeout(ns_client_t *client, unsigned int seconds) { static void client_extendederror_reset(ns_client_t *client) { - if (client->ede == NULL) { - return; + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + if (client->ede[i]) { + dns_ednsopt_t *ede = client->ede[i]; + + isc_mem_put(client->manager->mctx, ede->value, + ede->length); + isc_mem_put(client->manager->mctx, ede, sizeof(*ede)); + client->ede[i] = NULL; + } } - isc_mem_put(client->manager->mctx, client->ede->value, - client->ede->length); - isc_mem_put(client->manager->mctx, client->ede, sizeof(dns_ednsopt_t)); - client->ede = NULL; } void ns_client_extendederror(ns_client_t *client, uint16_t code, const char *text) { - unsigned char ede[DNS_EDE_EXTRATEXT_LEN + 2]; - isc_buffer_t buf; - uint16_t len = sizeof(uint16_t); + const uint16_t codelen = sizeof(code); + uint16_t textlen = 0; + size_t pos = 0; + unsigned char *ede = NULL; + dns_ednsopt_t *edns = NULL; REQUIRE(NS_CLIENT_VALID(client)); - if (client->ede != NULL) { + /* + * As ede will be directly put in the DNS message we need to make sure + * the code is in big-endian format + */ + code = htobe16(code); + + for (pos = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { + edns = client->ede[pos]; + + if (edns == NULL) { + break; + } + + if (memcmp(&code, edns->value, sizeof(code)) == 0) { + ns_client_log(client, NS_LOGCATEGORY_CLIENT, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), + "ignoring duplicate ede %u %s", code, + text == NULL ? "(null)" : text); + return; + } + } + + if (pos >= DNS_EDE_MAX_ERRORS) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), - "already have ede, ignoring %u %s", code, + "too many ede, ignoring %u %s", code, text == NULL ? "(null)" : text); return; } @@ -254,24 +281,31 @@ ns_client_extendederror(ns_client_t *client, uint16_t code, const char *text) { ISC_LOG_DEBUG(1), "set ede: info-code %u extra-text %s", code, text == NULL ? "(null)" : text); - isc_buffer_init(&buf, ede, sizeof(ede)); - isc_buffer_putuint16(&buf, code); if (text != NULL && strlen(text) > 0) { - if (strlen(text) < DNS_EDE_EXTRATEXT_LEN) { - isc_buffer_putstr(&buf, text); - len += (uint16_t)(strlen(text)); - } else { + textlen = strlen(text); + + if (textlen > DNS_EDE_EXTRATEXT_LEN) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_WARNING, - "ede extra-text too long, ignoring"); + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), + "truncate EDE code %hu text: %s", code, + text); + textlen = DNS_EDE_EXTRATEXT_LEN; } } - client->ede = isc_mem_get(client->manager->mctx, sizeof(dns_ednsopt_t)); - client->ede->code = DNS_OPT_EDE; - client->ede->length = len; - client->ede->value = isc_mem_get(client->manager->mctx, len); - memmove(client->ede->value, ede, len); + ede = isc_mem_get(client->manager->mctx, codelen + textlen); + + memcpy(ede, &code, sizeof(code)); + if (textlen > 0) { + memcpy(ede + codelen, text, textlen); + } + + edns = isc_mem_get(client->manager->mctx, sizeof(*edns)); + *edns = (dns_ednsopt_t){ .code = DNS_OPT_EDE, + .length = codelen + textlen, + .value = ede }; + + client->ede[pos] = edns; } static void @@ -1233,11 +1267,17 @@ no_nsid: count++; } - if (client->ede != NULL) { + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + dns_ednsopt_t *ede = client->ede[i]; + + if (ede == NULL) { + break; + } + INSIST(count < DNS_EDNSOPTIONS); ednsopts[count].code = DNS_OPT_EDE; - ednsopts[count].length = client->ede->length; - ednsopts[count].value = client->ede->value; + ednsopts[count].length = ede->length; + ednsopts[count].value = ede->value; count++; } @@ -2022,7 +2062,6 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, } client->message->rcode = dns_rcode_noerror; - client->ede = NULL; /* * Deal with EDNS. diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 0711add8e5..e62fd8db07 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -180,7 +180,7 @@ struct ns_client { size_t tcpbuf_size; dns_message_t *message; dns_rdataset_t *opt; - dns_ednsopt_t *ede; + dns_ednsopt_t *ede[DNS_EDE_MAX_ERRORS]; uint16_t udpsize; uint16_t extflags; int16_t ednsversion; /* -1 noedns */ diff --git a/lib/ns/query.c b/lib/ns/query.c index 29000be3ee..714c8d6c93 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -902,6 +902,7 @@ ns_query_init(ns_client_t *client) { ISC_LIST_INIT(client->query.namebufs); ISC_LIST_INIT(client->query.activeversions); ISC_LIST_INIT(client->query.freeversions); + memset(client->ede, 0, sizeof(dns_ednsopt_t *) * DNS_EDE_MAX_ERRORS); /* * This mutex is destroyed when the client is destroyed in * exit_check(). diff --git a/tests/dns/ede_test.c b/tests/dns/ede_test.c index f7a8c19002..74194a4ee2 100644 --- a/tests/dns/ede_test.c +++ b/tests/dns/ede_test.c @@ -56,12 +56,16 @@ ISC_RUN_TEST_IMPL(ede_enqueue_unlink) { assert_string_equal(ede->extra_text, msg1); assert_ptr_not_equal(ede->extra_text, msg1); + /* + * Even though we limit the length of an EDE message to 64 bytes, + * this is done only at the ns/client.c level (to make sure to cover all + * the flows). + */ ede = ISC_LIST_NEXT(ede, link); assert_non_null(ede); assert_int_equal(ede->info_code, 4); - assert_string_not_equal(ede->extra_text, msg2); + assert_string_equal(ede->extra_text, msg2); assert_ptr_not_equal(ede->extra_text, msg2); - assert_int_equal(strlen(ede->extra_text), 63); dns_ede_unlinkall(mctx, &list); assert_true(ISC_LIST_EMPTY(list)); diff --git a/tests/ns/Makefile.am b/tests/ns/Makefile.am index c26bec85e6..23b3ccbf7b 100644 --- a/tests/ns/Makefile.am +++ b/tests/ns/Makefile.am @@ -18,7 +18,8 @@ check_PROGRAMS = \ listenlist_test \ notify_test \ plugin_test \ - query_test + query_test \ + client_test notify_test_SOURCES = \ notify_test.c \ diff --git a/tests/ns/client_test.c b/tests/ns/client_test.c new file mode 100644 index 0000000000..83a174c1d4 --- /dev/null +++ b/tests/ns/client_test.c @@ -0,0 +1,171 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +#include +#include /* IWYU pragma: keep */ +#include +#include +#include +#include +#include + +#define UNIT_TESTING +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include + +typedef struct { + uint16_t code; + const char *txt; +} client_tests_ede_expected_t; + +static ns_clientmgr_t client_ede_test_dummy_manager; + +static ns_client_t * +client_ede_test_initclient(void) { + client_ede_test_dummy_manager.mctx = mctx; + + ns_client_t *client = isc_mem_get(mctx, sizeof(*client)); + memset(client, 0, sizeof(*client)); + client->magic = NS_CLIENT_MAGIC; + client->manager = &client_ede_test_dummy_manager; + + return client; +} + +static void +client_ede_test_free(ns_client_t *client) { + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + dns_ednsopt_t *ede = client->ede[i]; + + if (ede) { + isc_mem_put(mctx, ede->value, ede->length); + isc_mem_put(mctx, ede, sizeof(*ede)); + } + client->ede[i] = NULL; + } + isc_mem_put(mctx, client, sizeof(*client)); +} + +static void +client_ede_test_equals(const client_tests_ede_expected_t *expected, + size_t expected_count, const ns_client_t *client) { + size_t count = 0; + + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + dns_ednsopt_t *edns = client->ede[i]; + + if (edns == NULL) { + break; + } + + uint16_t code; + const size_t codelen = sizeof(code); + const unsigned char *txt; + + assert_in_range(count, 0, expected_count); + assert_int_equal(edns->code, DNS_OPT_EDE); + + code = ISC_U8TO16_BE(edns->value); + assert_int_equal(code, expected[count].code); + + if (edns->length > codelen) { + assert_non_null(expected[count].txt); + txt = edns->value + codelen; + assert_memory_equal(expected[count].txt, txt, + edns->length - codelen); + } else { + assert_null(expected[count].txt); + } + + count++; + } + assert_int_equal(count, expected_count); +} + +ISC_RUN_TEST_IMPL(client_ede_test_text_max_count) { + ns_client_t *client = client_ede_test_initclient(); + + const char *txt1 = "foobar"; + const char *txt2 = "It's been a long time since I rock-and-rolled" + "Ooh, let me get it back, let me get it back"; + + ns_client_extendederror(client, 2, txt1); + ns_client_extendederror(client, 22, NULL); + ns_client_extendederror(client, 3, txt2); + + const client_tests_ede_expected_t expected[3] = { + { .code = 2, .txt = "foobar" }, + { .code = 22, .txt = NULL }, + { .code = 3, + .txt = "It's been a long time since I rock-and-rolledOoh, " + "let me get it " } + }; + + client_ede_test_equals(expected, 3, client); + client_ede_test_free(client); +} + +ISC_RUN_TEST_IMPL(client_ede_test_max_count) { + ns_client_t *client = client_ede_test_initclient(); + + ns_client_extendederror(client, 1, NULL); + ns_client_extendederror(client, 22, "two"); + ns_client_extendederror(client, 3, "three"); + ns_client_extendederror(client, 4, "four"); + ns_client_extendederror(client, 5, "five"); + + const client_tests_ede_expected_t expected[3] = { + { .code = 1, .txt = NULL }, + { .code = 22, .txt = "two" }, + { .code = 3, .txt = "three" }, + }; + + client_ede_test_equals(expected, 3, client); + client_ede_test_free(client); +} + +ISC_RUN_TEST_IMPL(client_ede_test_duplicates) { + ns_client_t *client = client_ede_test_initclient(); + + ns_client_extendederror(client, 1, NULL); + ns_client_extendederror(client, 1, "two"); + ns_client_extendederror(client, 1, "three"); + + const client_tests_ede_expected_t expected[] = { + { .code = 1, .txt = NULL }, + }; + + client_ede_test_equals(expected, 1, client); + + client_ede_test_free(client); +} + +ISC_TEST_LIST_START + +ISC_TEST_ENTRY(client_ede_test_text_max_count) +ISC_TEST_ENTRY(client_ede_test_max_count) +ISC_TEST_ENTRY(client_ede_test_duplicates) + +ISC_TEST_LIST_END + +ISC_TEST_MAIN