From 670a1ebe9aa0b6da5a3ae62bf5ad927721fc812b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Mar 2021 17:47:45 +0100 Subject: [PATCH 1/6] dns-query: export CNAME_MAX, so that we can use it in other files, too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's rename it a bit, to be more explanatory while exporting it. (And let's bump the CNAME limit to 16 — 8 just sounded so little) --- src/resolve/resolved-dns-query.c | 3 +-- src/resolve/resolved-dns-query.h | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 7554d1e82f4..aa9d65d4a82 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -10,7 +10,6 @@ #include "resolved-etc-hosts.h" #include "string-util.h" -#define CNAME_MAX 8 #define QUERIES_MAX 2048 #define AUXILIARY_QUERIES_MAX 64 @@ -977,7 +976,7 @@ static int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname) assert(q); q->n_cname_redirects++; - if (q->n_cname_redirects > CNAME_MAX) + if (q->n_cname_redirects > CNAME_REDIRECT_MAX) return -ELOOP; r = dns_question_cname_redirect(q->question_idna, cname, &nq_idna); diff --git a/src/resolve/resolved-dns-query.h b/src/resolve/resolved-dns-query.h index ea296167b61..5d12171b0a1 100644 --- a/src/resolve/resolved-dns-query.h +++ b/src/resolve/resolved-dns-query.h @@ -145,3 +145,5 @@ static inline uint64_t dns_query_reply_flags_make(DnsQuery *q) { dns_query_fully_confidential(q)) | (q->answer_query_flags & (SD_RESOLVED_FROM_MASK|SD_RESOLVED_SYNTHETIC)); } + +#define CNAME_REDIRECT_MAX 16 From 6fe1d507354710c62d735e5fbd48e014b547a76e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Mar 2021 17:48:43 +0100 Subject: [PATCH 2/6] resolved: tighten checks in dns_resource_record_get_cname_target() Let's refuse to consider CNAME/DNAME replies matching for RR types where that is not really conceptually allow (i.e. on CNAME/DNAME lookups themselves). (And add a similar check to dns_resource_key_match_cname_or_dname() too, which implements a smilar match) --- src/resolve/resolved-dns-rr.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c index 823117e5c92..7e76e0c6cc0 100644 --- a/src/resolve/resolved-dns-rr.c +++ b/src/resolve/resolved-dns-rr.c @@ -244,6 +244,9 @@ int dns_resource_key_match_cname_or_dname(const DnsResourceKey *key, const DnsRe if (cname->class != key->class && key->class != DNS_CLASS_ANY) return 0; + if (!dns_type_may_redirect(key->type)) + return 0; + if (cname->type == DNS_TYPE_CNAME) r = dns_name_equal(dns_resource_key_name(key), dns_resource_key_name(cname)); else if (cname->type == DNS_TYPE_DNAME) @@ -1743,9 +1746,16 @@ int dns_resource_record_get_cname_target(DnsResourceKey *key, DnsResourceRecord assert(key); assert(cname); + /* Checks if the RR `cname` is a CNAME/DNAME RR that matches the specified `key`. If so, returns the + * target domain. If not, returns -EUNATCH */ + if (key->class != cname->key->class && key->class != DNS_CLASS_ANY) return -EUNATCH; + if (!dns_type_may_redirect(key->type)) /* This key type is not subject to CNAME/DNAME redirection? + * Then let's refuse right-away */ + return -EUNATCH; + if (cname->key->type == DNS_TYPE_CNAME) { r = dns_name_equal(dns_resource_key_name(key), dns_resource_key_name(cname->key)); From 8fb7a20db536b992135a2654f08af0007f268b48 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Mar 2021 17:53:31 +0100 Subject: [PATCH 3/6] resolved: handle multiple CNAME redirects in a single reply from upstream www.netflix.com responds with a chain of CNAMEs in the same packet. Let's handle that properly (so far we only followed CNAMEs on step when in the same packet) Fixes: #18819 --- src/resolve/resolved-dns-stub.c | 105 +++++++++++++++++--------------- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index c2734e57b9b..098a86fca3f 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -162,79 +162,88 @@ static int dns_stub_collect_answer_by_question( bool with_rrsig) { /* Add RRSIG RR matching each RR */ _cleanup_(dns_resource_key_unrefp) DnsResourceKey *redirected_key = NULL; + unsigned n_cname_redirects = 0; DnsAnswerItem *item; int r; assert(reply); - /* Copies all RRs from 'answer' into 'reply', if they match 'question'. */ + /* Copies all RRs from 'answer' into 'reply', if they match 'question'. There might be direct and + * indirect matches (i.e. via CNAME/DNAME). If the have an indirect one, remember where we need to + * go, and restart the loop */ + + for (;;) { + _cleanup_(dns_resource_key_unrefp) DnsResourceKey *next_redirected_key = NULL; + + DNS_ANSWER_FOREACH_ITEM(item, answer) { + DnsResourceKey *k = NULL; + + if (redirected_key) { + /* There was a redirect in this packet, let's collect all matching RRs for the redirect */ + r = dns_resource_key_match_rr(redirected_key, item->rr, NULL); + if (r < 0) + return r; + + k = redirected_key; + } else if (question) { + /* We have a question, let's see if this RR matches it */ + r = dns_question_matches_rr(question, item->rr, NULL); + if (r < 0) + return r; + + k = question->keys[0]; + } else + r = 1; /* No question, everything matches */ - DNS_ANSWER_FOREACH_ITEM(item, answer) { - if (question) { - r = dns_question_matches_rr(question, item->rr, NULL); - if (r < 0) - return r; if (r == 0) { _cleanup_free_ char *target = NULL; /* OK, so the RR doesn't directly match. Let's see if the RR is a matching * CNAME or DNAME */ - r = dns_resource_record_get_cname_target( - question->keys[0], - item->rr, - &target); + assert(k); + + r = dns_resource_record_get_cname_target(k, item->rr, &target); if (r == -EUNATCH) continue; /* Not a CNAME/DNAME or doesn't match */ if (r < 0) return r; - dns_resource_key_unref(redirected_key); + /* Oh, wow, this is a redirect. Let's remember where this points, and store + * it in 'next_redirected_key'. Once we finished iterating through the rest + * of the RR's we'll start again, with the redirected RR. */ + + n_cname_redirects++; + if (n_cname_redirects > CNAME_REDIRECT_MAX) /* don't loop forever */ + return -ELOOP; + + dns_resource_key_unref(next_redirected_key); /* There can only be one CNAME per name, hence no point in storing more than one here */ - redirected_key = dns_resource_key_new(question->keys[0]->class, question->keys[0]->type, target); - if (!redirected_key) + next_redirected_key = dns_resource_key_new(k->class, k->type, target); + if (!next_redirected_key) return -ENOMEM; } - } - /* Mask the section info, we want the primary answers to always go without section info, so - * that it is added to the answer section when we synthesize a reply. */ + /* Mask the section info, we want the primary answers to always go without section info, so + * that it is added to the answer section when we synthesize a reply. */ - r = reply_add_with_rrsig( - reply, - item->rr, - item->ifindex, - item->flags & ~DNS_ANSWER_MASK_SECTIONS, - item->rrsig, - with_rrsig); - if (r < 0) - return r; - } - - if (!redirected_key) - return 0; - - /* This is a CNAME/DNAME answer. In this case also append where the redirections point to to the main - * answer section */ - - DNS_ANSWER_FOREACH_ITEM(item, answer) { + r = reply_add_with_rrsig( + reply, + item->rr, + item->ifindex, + item->flags & ~DNS_ANSWER_MASK_SECTIONS, + item->rrsig, + with_rrsig); + if (r < 0) + return r; + } - r = dns_resource_key_match_rr(redirected_key, item->rr, NULL); - if (r < 0) - return r; - if (r == 0) - continue; + if (!next_redirected_key) + break; - r = reply_add_with_rrsig( - reply, - item->rr, - item->ifindex, - item->flags & ~DNS_ANSWER_MASK_SECTIONS, - item->rrsig, - with_rrsig); - if (r < 0) - return r; + dns_resource_key_unref(redirected_key); + redirected_key = TAKE_PTR(next_redirected_key); } return 0; From b54de8231bd35f08af46b76dae1028397a19a31e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Mar 2021 18:01:27 +0100 Subject: [PATCH 4/6] resolved: split out helper that checks whether we shall reply with EDNS0 DO Just some refactoring, no actual code changes. --- src/resolve/resolved-dns-stub.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 098a86fca3f..67e38bea6ea 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -561,6 +561,19 @@ static int dns_stub_send( return 0; } +static int dns_stub_reply_with_edns0_do(DnsQuery *q) { + assert(q); + + /* Reply with DNSSEC DO set? Only if client supports it; and we did any DNSSEC verification + * ourselves, or consider the data fully authenticated because we generated it locally, or the client + * set cd */ + + return DNS_PACKET_DO(q->request_packet) && + (q->answer_dnssec_result >= 0 || /* we did proper DNSSEC validation … */ + dns_query_fully_authenticated(q) || /* … or we considered it authentic otherwise … */ + DNS_PACKET_CD(q->request_packet)); /* … or client set CD */ +} + static int dns_stub_send_reply( DnsQuery *q, int rcode) { @@ -571,14 +584,7 @@ static int dns_stub_send_reply( assert(q); - /* Reply with DNSSEC DO set? Only if client supports it; and we did any DNSSEC verification - * ourselves, or consider the data fully authenticated because we generated it locally, or - * the client set cd */ - edns0_do = - DNS_PACKET_DO(q->request_packet) && - (q->answer_dnssec_result >= 0 || /* we did proper DNSSEC validation … */ - dns_query_fully_authenticated(q) || /* … or we considered it authentic otherwise … */ - DNS_PACKET_CD(q->request_packet)); /* … or client set CD */ + edns0_do = dns_stub_reply_with_edns0_do(q); /* let' check if we shall reply with EDNS0 DO? */ r = dns_stub_assign_sections( q, From fd67ea0d9804b8aaea4bda7527afe287060d14db Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Mar 2021 16:50:04 +0100 Subject: [PATCH 5/6] resolved: fully follow CNAMEs in the DNS stub after all In 2f4d8e577ca7bc51fb054b8c2c8dd57c2e188a41 I argued that following CNAMEs in the stub is not necessary anymore. However, I think it' better to revert to the status quo ante and follow it after all, given it is easy for us and makes sure our D-Bus/varlink replies are more similar to our DNS stub replies that way, and we save clients potential roundtrips. Hence, whenever we hit a CNAME/DNAME redirect, let's restart the query like we do for the D-Bus/Varlink case, and collect replies as we go. --- src/resolve/resolved-dns-stub.c | 38 +++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 67e38bea6ea..486b8146acf 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -586,13 +586,6 @@ static int dns_stub_send_reply( edns0_do = dns_stub_reply_with_edns0_do(q); /* let' check if we shall reply with EDNS0 DO? */ - r = dns_stub_assign_sections( - q, - q->request_packet->question, - edns0_do); - if (r < 0) - return log_debug_errno(r, "Failed to assign sections: %m"); - r = dns_stub_make_reply_packet( &reply, DNS_PACKET_PAYLOAD_SIZE_MAX(q->request_packet), @@ -743,13 +736,37 @@ static void dns_stub_query_complete(DnsQuery *q) { } } - /* Note that we don't bother with following CNAMEs here. We propagate the authoritative/additional - * sections from the upstream answer however, hence if the upstream server collected that information - * already we don't have to collect it ourselves anymore. */ + /* Take all data from the current reply, and merge it into the three reply sections we are building + * up. We do this before processing CNAME redirects, so that we gradually build up our sections, and + * and keep adding all RRs in the CNAME chain. */ + r = dns_stub_assign_sections( + q, + q->request_packet->question, + dns_stub_reply_with_edns0_do(q)); + if (r < 0) { + log_debug_errno(r, "Failed to assign sections: %m"); + dns_query_free(q); + return; + } switch (q->state) { case DNS_TRANSACTION_SUCCESS: + r = dns_query_process_cname(q); + if (r == -ELOOP) { /* CNAME loop, let's send what we already have */ + log_debug_errno(r, "Detected CNAME loop, returning what we already have."); + (void) dns_stub_send_reply(q, q->answer_rcode); + break; + } + if (r < 0) { + log_debug_errno(r, "Failed to process CNAME: %m"); + break; + } + if (r == DNS_QUERY_RESTARTED) + return; + + _fallthrough_; + case DNS_TRANSACTION_RCODE_FAILURE: (void) dns_stub_send_reply(q, q->answer_rcode); break; @@ -888,7 +905,6 @@ static void dns_stub_process_query(Manager *m, DnsStubListenerExtra *l, DnsStrea r = dns_query_new(m, &q, p->question, p->question, NULL, 0, SD_RESOLVED_PROTOCOLS_ALL| SD_RESOLVED_NO_SEARCH| - SD_RESOLVED_NO_CNAME| (DNS_PACKET_DO(p) ? SD_RESOLVED_REQUIRE_PRIMARY : 0)| SD_RESOLVED_CLAMP_TTL); if (r < 0) { From 42a0086a3e8939ab58cc81409f54ac64a2358923 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Mar 2021 18:20:59 +0100 Subject: [PATCH 6/6] resolved: when synthesizing stub replies from multiple upstream packet, let's avoid RR duplicates If we synthesize a stub reply from multiple upstream packet (i.e. a series of CNAME/DNAME redirects), it might happen that we add the same RR to a different reply section at a different CNAME/DNAME redirect chain element. Let's clean this up once we are about to send the reply message to the client: let's remove sections from "lower-priority" sections when they are already listed in a "higher-priority" section. --- src/resolve/resolved-dns-answer.c | 25 +++++++++++++++++++++++++ src/resolve/resolved-dns-answer.h | 1 + src/resolve/resolved-dns-stub.c | 18 ++++++++++++++++++ 3 files changed, 44 insertions(+) diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index ce3cbce308d..8db97dce567 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -640,6 +640,31 @@ int dns_answer_remove_by_rr(DnsAnswer **a, DnsResourceRecord *rm) { return 1; } +int dns_answer_remove_by_answer_keys(DnsAnswer **a, DnsAnswer *b) { + _cleanup_(dns_resource_key_unrefp) DnsResourceKey *prev = NULL; + DnsAnswerItem *item; + int r; + + /* Removes all items from '*a' that have a matching key in 'b' */ + + DNS_ANSWER_FOREACH_ITEM(item, b) { + + if (prev && dns_resource_key_equal(item->rr->key, prev)) /* Skip this one, we already looked at it */ + continue; + + r = dns_answer_remove_by_key(a, item->rr->key); + if (r < 0) + return r; + + /* Let's remember this entrie's RR key, to optimize the loop a bit: if we have an RRset with + * more than one item then we don#t need to remove the key multiple times */ + dns_resource_key_unref(prev); + prev = dns_resource_key_ref(item->rr->key); + } + + return 0; +} + int dns_answer_copy_by_key( DnsAnswer **a, DnsAnswer *source, diff --git a/src/resolve/resolved-dns-answer.h b/src/resolve/resolved-dns-answer.h index c2fd0c078f4..7d19eee4e2b 100644 --- a/src/resolve/resolved-dns-answer.h +++ b/src/resolve/resolved-dns-answer.h @@ -68,6 +68,7 @@ int dns_answer_reserve_or_clone(DnsAnswer **a, size_t n_free); int dns_answer_remove_by_key(DnsAnswer **a, const DnsResourceKey *key); int dns_answer_remove_by_rr(DnsAnswer **a, DnsResourceRecord *rr); +int dns_answer_remove_by_answer_keys(DnsAnswer **a, DnsAnswer *b); int dns_answer_copy_by_key(DnsAnswer **a, DnsAnswer *source, const DnsResourceKey *key, DnsAnswerFlags or_flags, DnsResourceRecord *rrsig); int dns_answer_move_by_key(DnsAnswer **to, DnsAnswer **from, const DnsResourceKey *key, DnsAnswerFlags or_flags, DnsResourceRecord *rrsig); diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 486b8146acf..7fc8b4fdd4f 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -574,6 +574,22 @@ static int dns_stub_reply_with_edns0_do(DnsQuery *q) { DNS_PACKET_CD(q->request_packet)); /* … or client set CD */ } +static void dns_stub_suppress_duplicate_section_rrs(DnsQuery *q) { + /* If we follow a CNAME/DNAME chain we might end up populating our sections with redundant RRs + * because we built up the sections from multiple reply packets (one from each CNAME/DNAME chain + * element). e.g. it could be that an RR that was included in the first reply's additional section + * ends up being relevant as main answer in a subsequent reply in the chain. Let's clean this up, and + * remove everything from the "higher priority" sections from the "lower priority" sections if they + * exists in both. + * + * Note that this removal matches by RR keys instead of the full RRs. This is because RRsets should + * always end up in one section fully or not at all, but never be split among sections. */ + + dns_answer_remove_by_answer_keys(&q->reply_authoritative, q->reply_answer); + dns_answer_remove_by_answer_keys(&q->reply_additional, q->reply_answer); + dns_answer_remove_by_answer_keys(&q->reply_additional, q->reply_authoritative); +} + static int dns_stub_send_reply( DnsQuery *q, int rcode) { @@ -594,6 +610,8 @@ static int dns_stub_send_reply( if (r < 0) return log_debug_errno(r, "Failed to build reply packet: %m"); + dns_stub_suppress_duplicate_section_rrs(q); + r = dns_stub_add_reply_packet_body( reply, q->reply_answer,