From 87d6a9fb2e5679922eb3dc2887bd831b9317ff10 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Oct 2024 09:01:43 +0200 Subject: [PATCH 1/5] dns-packet: refuse reading overlong DNS names from packets Even if we have no problem processing them they are invalid according to RFC, hence refuse. Fixes: #34416 --- src/resolve/resolved-dns-packet.c | 11 +++- src/resolve/test-dns-packet-extract.c | 93 +++++++++++++++++++++++---- 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index f9991d86aba..bfd4d48f523 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -1506,7 +1506,7 @@ int dns_packet_read_name( size_t after_rindex = 0, jump_barrier = p->rindex; _cleanup_free_ char *name = NULL; bool first = true; - size_t n = 0; + size_t n = 0, m = 0; int r; if (p->refuse_compression) @@ -1535,14 +1535,21 @@ int dns_packet_read_name( if (first) first = false; - else + else { name[n++] = '.'; + m++; + } r = dns_label_escape(label, c, name + n, DNS_LABEL_ESCAPED_MAX); if (r < 0) return r; n += r; + m += c; + + if (m > DNS_HOSTNAME_MAX) + return -EBADMSG; + continue; } else if (allow_compression && FLAGS_SET(c, 0xc0)) { uint16_t ptr; diff --git a/src/resolve/test-dns-packet-extract.c b/src/resolve/test-dns-packet-extract.c index 5cd5a83d49c..0bded090777 100644 --- a/src/resolve/test-dns-packet-extract.c +++ b/src/resolve/test-dns-packet-extract.c @@ -806,12 +806,7 @@ TEST(packet_query_single_long_domain) { 0x10, 'n', 'i', 't', 'r', 'o', 's', 'y', 'l', 's', 'u', 'l', 'f', 'u', 'r', 'i', 'c', 0x10, 'o', 'b', 'j', 'e', 'c', 't', 'l', 'e', 's', 's', 'n', 'e', 's', 's', 'e', 's', 0x10, 'p', 'a', 'r', 't', 'r', 'i', 'd', 'g', 'e', 'b', 'e', 'r', 'r', 'i', 'e', 's', - 0x10, 'r', 'e', 'a', 's', 'o', 'n', 'l', 'e', 's', 's', 'n', 'e', 's', 's', 'e', 's', - 0x10, 's', 'e', 'm', 'i', 'p', 'a', 't', 'h', 'o', 'l', 'o', 'g', 'i', 'c', 'a', 'l', - 0x10, 't', 'o', 'm', 'f', 'o', 'o', 'l', 'i', 's', 'h', 'n', 'e', 's', 's', 'e', 's', - 0x10, 'u', 'n', 'd', 'e', 'r', 'c', 'a', 'p', 'i', 't', 'a', 'l', 'i', 'z', 'e', 'd', - 0x10, 'v', 'e', 'c', 't', 'o', 'r', 'c', 'a', 'r', 'd', 'i', 'o', 'g', 'r', 'a', 'm', - 0x10, 'w', 'e', 'a', 't', 'h', 'e', 'r', 'p', 'r', 'o', 'o', 'f', 'n', 'e', 's', 's', + 0x0F, 'r', 'e', 'a', 's', 'o', 'n', 'l', 'e', 's', 's', 'n', 'e', 's', 's', 'e', 0x00, /* A */ 0x00, 0x01, /* IN */ 0x00, 0x01 @@ -823,12 +818,12 @@ TEST(packet_query_single_long_domain) { ASSERT_EQ(dns_question_size(packet->question), 1u); ASSERT_EQ(dns_answer_size(packet->answer), 0u); - key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, - "absorptivenesses.calligraphically.deacidifications.ecophysiological." - "falsifiabilities.heterochromatism.icositetrahedron.journalistically." - "kinaesthetically.lactovegetarians.misinterpretable.nitrosylsulfuric." - "objectlessnesses.partridgeberries.reasonlessnesses.semipathological." - "tomfoolishnesses.undercapitalized.vectorcardiogram.weatherproofness"); + key = dns_resource_key_new( + DNS_CLASS_IN, DNS_TYPE_A, + "absorptivenesses.calligraphically.deacidifications.ecophysiological." + "falsifiabilities.heterochromatism.icositetrahedron.journalistically." + "kinaesthetically.lactovegetarians.misinterpretable.nitrosylsulfuric." + "objectlessnesses.partridgeberries.reasonlessnesse"); ASSERT_NOT_NULL(key); ASSERT_TRUE(dns_question_contains_key(packet->question, key)); @@ -4777,4 +4772,78 @@ TEST(format_dns_svc_param_key) { ASSERT_STREQ(str, "ohttp"); } +TEST(overlong_domain) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX)); + ASSERT_NOT_NULL(packet); + dns_packet_truncate(packet, 0); + + const uint8_t data[] = { + 0x00, 0x42, 0x00, 0x00, + 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 0x00, + 0x00, DNS_TYPE_A, + 0x00, DNS_CLASS_IN, + }; + + ASSERT_OK(dns_packet_append_blob(packet, data, sizeof(data), NULL)); + ASSERT_OK(dns_packet_validate_query(packet)); + _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; + ASSERT_ERROR(dns_packet_read_key(packet, &key, NULL, NULL), EBADMSG); + + const uint8_t data2[] = { + 0x00, 0x42, 0x00, 0x00, + 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 62, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', + 0x00, + 0x00, DNS_TYPE_A, + 0x00, DNS_CLASS_IN, + }; + + packet = dns_packet_unref(packet); + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX)); + ASSERT_NOT_NULL(packet); + dns_packet_truncate(packet, 0); + ASSERT_OK(dns_packet_append_blob(packet, data2, sizeof(data2), NULL)); + ASSERT_OK(dns_packet_validate_query(packet)); + ASSERT_ERROR(dns_packet_read_key(packet, &key, NULL, NULL), EBADMSG); + + const uint8_t data3[] = { + 0x00, 0x42, 0x00, 0x00, + 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', + 61, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', + 0x00, + 0x00, DNS_TYPE_A, + 0x00, DNS_CLASS_IN, + }; + + packet = dns_packet_unref(packet); + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX)); + ASSERT_NOT_NULL(packet); + dns_packet_truncate(packet, 0); + ASSERT_OK(dns_packet_append_blob(packet, data3, sizeof(data3), NULL)); + ASSERT_OK(dns_packet_validate_query(packet)); + ASSERT_OK(dns_packet_read_key(packet, &key, NULL, NULL)); + + ASSERT_STREQ(dns_resource_key_name(key), + "012345678901234567890123456789012345678901234567890123456789012." + "012345678901234567890123456789012345678901234567890123456789012." + "012345678901234567890123456789012345678901234567890123456789012." + "0123456789012345678901234567890123456789012345678901234567890"); +} + DEFINE_TEST_MAIN(LOG_DEBUG) From 8ed2c62d46f93c2117d65a908c316a381073af16 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 12 Sep 2024 17:00:04 +0200 Subject: [PATCH 2/5] dns-domain: tweak hash table comparison function for DNS names Currently, when comparing two DNS names when storing them in a hashtable, and the DNS names are not actually valid we'll compare the error codes. This is not very smart however, since this means two invalid DNS names that happen to be equally "invalid" will be considered identical, even if their strings are entirely different. Let's find a better solution for this niche case: let's simple compare the domains as strings. This matters in case of DNS label compression: if we already added added an invalid DNS name into the label compression hash table, and lookup any other invalid DNS name, this lookup will likely return what the earlier one already returned, and that's confusing. --- src/shared/dns-domain.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/shared/dns-domain.c b/src/shared/dns-domain.c index c9963ade930..6dc45a3dd11 100644 --- a/src/shared/dns-domain.c +++ b/src/shared/dns-domain.c @@ -480,17 +480,17 @@ finish: return 0; } -void dns_name_hash_func(const char *p, struct siphash *state) { +void dns_name_hash_func(const char *name, struct siphash *state) { int r; - assert(p); + assert(name); - for (;;) { + for (const char *p = name;;) { char label[DNS_LABEL_MAX+1]; r = dns_label_unescape(&p, label, sizeof label, 0); if (r < 0) - break; + return string_hash_func(p, state); /* fallback for invalid DNS names */ if (r == 0) break; @@ -516,13 +516,13 @@ int dns_name_compare_func(const char *a, const char *b) { for (;;) { char la[DNS_LABEL_MAX+1], lb[DNS_LABEL_MAX+1]; - if (x == NULL && y == NULL) + if (!x && !y) return 0; r = dns_label_unescape_suffix(a, &x, la, sizeof(la)); q = dns_label_unescape_suffix(b, &y, lb, sizeof(lb)); if (r < 0 || q < 0) - return CMP(r, q); + return strcmp(a, b); /* if not valid DNS labels, then let's compare the whole strings as is */ r = ascii_strcasecmp_nn(la, r, lb, q); if (r != 0) From 360105f1e748148ba17bdb3f47525f01aba4127f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 12 Sep 2024 17:05:24 +0200 Subject: [PATCH 3/5] resolved: when adding names to packet fails, remove them from label compression hash table again let's make sure we undo any pollution of the label compression hash table. Fixes: #33671 --- src/resolve/resolved-dns-packet.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index bfd4d48f523..2684944b75e 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -557,7 +557,8 @@ int dns_packet_append_name( bool canonical_candidate, size_t *start) { - size_t saved_size; + _cleanup_free_ char **added_entries = NULL; /* doesn't own the strings! this is just regular pointer array, not a NULL-terminated strv! */ + size_t n_added_entries = 0, saved_size; int r; assert(p); @@ -598,6 +599,11 @@ int dns_packet_append_name( if (allow_compression) { _cleanup_free_ char *s = NULL; + if (!GREEDY_REALLOC(added_entries, n_added_entries + 1)) { + r = -ENOMEM; + goto fail; + } + s = strdup(z); if (!s) { r = -ENOMEM; @@ -608,7 +614,8 @@ int dns_packet_append_name( if (r < 0) goto fail; - TAKE_PTR(s); + /* Keep track of the entries we just added (note that the string is owned by the hashtable, not this array!) */ + added_entries[n_added_entries++] = TAKE_PTR(s); } } @@ -623,6 +630,12 @@ done: return 0; fail: + /* Remove all label compression names we added again */ + FOREACH_ARRAY(s, added_entries, n_added_entries) { + hashmap_remove(p->names, *s); + free(*s); + } + dns_packet_truncate(p, saved_size); return r; } From e63785611713cab0131599565cb3a1bb505640c7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 12 Sep 2024 17:08:36 +0200 Subject: [PATCH 4/5] resolved: explicitly refuse adding invalid DNS names to DNS packets Fixes: #33671 --- src/resolve/resolved-dns-packet.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 2684944b75e..c414ca800c3 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -564,6 +564,12 @@ int dns_packet_append_name( assert(p); assert(name); + r = dns_name_is_valid(name); + if (r < 0) + return r; + if (r == 0) + return -EINVAL; + if (p->refuse_compression) allow_compression = false; From 953ab987449ecb45c509dfbb0f6dcdb36ce51f27 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 12 Sep 2024 17:09:37 +0200 Subject: [PATCH 5/5] resolved: add test case from #33671 --- src/resolve/test-dns-packet-append.c | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/resolve/test-dns-packet-append.c b/src/resolve/test-dns-packet-append.c index 9ef77b3b983..89420509273 100644 --- a/src/resolve/test-dns-packet-append.c +++ b/src/resolve/test-dns-packet-append.c @@ -1267,4 +1267,34 @@ TEST(packet_append_answer_single_svcb) { ASSERT_EQ(memcmp(DNS_PACKET_DATA(packet), data, sizeof(data)), 0); } +static void dump_packet_data(DnsPacket *packet) { + assert(packet); + fprintf(stderr, "packet bytes:"); + for (size_t i = 0; i < packet->size; i++) + fprintf(stderr, " %x", DNS_PACKET_DATA(packet)[i]); + fprintf(stderr, "\n"); +} + +TEST(packet_append_key_name_too_long) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; + int r; + + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX)); + + DNS_PACKET_ID(packet) = htobe16(42); + DNS_PACKET_HEADER(packet)->flags = htobe16(DNS_PACKET_MAKE_FLAGS(0, 0, 0, 0, 1, 0, 0, 0, 0)); + DNS_PACKET_HEADER(packet)->qdcount = htobe16(1); + + key = ASSERT_PTR(dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "www.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com")); + r = dns_packet_append_key(packet, key, 0, NULL); + + log_debug("r = %d, size = %zu", r, packet->size); + log_debug("key name = <%s>", dns_resource_key_name(key)); + dump_packet_data(packet); + + ASSERT_EQ(r, -EINVAL); + ASSERT_EQ(packet->size, 12U); +} + DEFINE_TEST_MAIN(LOG_DEBUG)