From e0ae456a554d0fce250f9a009c561b97f20c41f8 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
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 d29958261a3df80f5cf0e98b1cd307790a92b13b Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
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 4838dc4f2be1d29da9ce9a930c48717a4491d70e Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
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 a single 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..c3a28d390a4 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 they 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 key. */
+
+ 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 39005e187095062718621880e5d8ad707ac8fe8f Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
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 c3a28d390a4..b4df5837aad 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's check if we shall reply with EDNS0 DO? */
r = dns_stub_assign_sections(
q,
From b97fc57178932689bdcb9030e1e2bf299d49ce0b Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
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 b4df5837aad..85c4eda469c 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's 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 5d7da51ee1d27e86a0487a4b2abc3cfb0ed44c23 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
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 | 20 ++++++++++++++++++++
3 files changed, 46 insertions(+)
diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c
index ce3cbce308d..a667ab5ede4 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 entry'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 85c4eda469c..8e781dd7389 100644
--- a/src/resolve/resolved-dns-stub.c
+++ b/src/resolve/resolved-dns-stub.c
@@ -574,6 +574,24 @@ 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 in the "higher priority" sections from the "lower priority" sections.
+ *
+ * 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.
+ *
+ * Specifically: we remove ANSWER section RRs from the AUTHORITATIVE and ADDITIONAL sections, as well
+ * as AUTHORITATIVE section RRs from the ADDITIONAL section. */
+
+ 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 +612,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,