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,