From e5f70dadb2eab5cf48c64fa379628c8e453bd09f Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Mar 06 2021 19:08:13 +0000 Subject: Backport PR #18892 to fix stub resolver CNAME chain resolving (#1933433) --- diff --git a/18892.patch b/18892.patch new file mode 100644 index 0000000..e503d26 --- /dev/null +++ b/18892.patch @@ -0,0 +1,493 @@ +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, diff --git a/systemd.spec b/systemd.spec index ca45f4e..cde6ea9 100644 --- a/systemd.spec +++ b/systemd.spec @@ -21,7 +21,7 @@ Name: systemd Url: https://www.freedesktop.org/wiki/Software/systemd Version: 248~rc2 -Release: 2%{?dist} +Release: 3%{?dist} # For a breakdown of the licensing, see README License: LGPLv2+ and MIT and GPLv2+ Summary: System and Service Manager @@ -70,6 +70,11 @@ i=1; for j in 00*patch; do printf "Patch%04d: %s\n" $i $j; i=$((i+1));done| GIT_DIR=../../src/systemd/.git git diffab -M v233..master@{2017-06-15} -- hwdb/[67]* hwdb/parse_hwdb.py > hwdb.patch %endif +# https://github.com/systemd/systemd/pull/18892 +# Fix stub resolver handling of CNAME chains +# https://bugzilla.redhat.com/show_bug.cgi?id=1933433 +Patch0: 18892.patch + # Downstream-only patches (5000–9999) # https://bugzilla.redhat.com/show_bug.cgi?id=1738828 Patch0500: use-bfq-scheduler.patch @@ -932,6 +937,9 @@ getent passwd systemd-network &>/dev/null || useradd -r -u 192 -l -g systemd-net %files standalone-sysusers -f .file-list-standalone-sysusers %changelog +* Fri Mar 05 2021 Adam Williamson - 248~rc2-3 +- Backport PR #18892 to fix stub resolver CNAME chain resolving (#1933433) + * Mon Mar 01 2021 Josh Boyer - 248~rc2-2 - Don't set the fallback hostname to Fedora on non-Fedora OSes