systemd/18892.patch

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,