From d451f0e84b1909ca0d24a1e3c135d5ffa2d8613c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 22:02:55 +0100 Subject: [PATCH 1/3] resolved: tweak sections we add answer RRs to Previously we'd stick all answer sections RRs we acquired into the authoritative section if we didn't find them directly answering our question. Let's put them into additional instead. The authoritative section should hence only include what comes from the upstream authoritative section, and nothing else. --- src/resolve/resolved-dns-stub.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index b6d14b9305..7cbbdc725c 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -323,16 +323,8 @@ static int dns_stub_assign_sections( if (r < 0) return r; - /* Include all RRs that originate from the answer or authority sections, and aren't listed in the + /* Include all RRs that originate from the authority sections, and aren't already listed in the * answer section, in the authority section */ - r = dns_stub_collect_answer_by_section( - &q->reply_authoritative, - q->answer, - DNS_ANSWER_SECTION_ANSWER, - q->reply_answer, NULL, - edns0_do); - if (r < 0) - return r; r = dns_stub_collect_answer_by_section( &q->reply_authoritative, q->answer, @@ -342,8 +334,16 @@ static int dns_stub_assign_sections( if (r < 0) return r; - /* Include all RRs that originate from the additional sections in the additional section (except if - * already listed in the other two sections). Also add all RRs with no section marking. */ + /* Include all RRs that originate from the answer or additional sections in the additional section + * (except if already listed in the other two sections). Also add all RRs with no section marking. */ + r = dns_stub_collect_answer_by_section( + &q->reply_additional, + q->answer, + DNS_ANSWER_SECTION_ANSWER, + q->reply_answer, q->reply_authoritative, + edns0_do); + if (r < 0) + return r; r = dns_stub_collect_answer_by_section( &q->reply_additional, q->answer, From 1db8e6d1db0880de240e5598e28d24d708479434 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Mar 2021 11:43:52 +0100 Subject: [PATCH 2/3] resolved: split dns_query_process_cname() into two separate functions This does some refactoring: the dns_query_process_cname() function becomes two: dns_query_process_cname_one() and dns_query_process_cname_many(). The former will process exactly one CNAME chain element, the latter will follow a chain for as long as possible within the current packet. dns_query_process_cname_many() is mostly identical to the old dns_query_process_cname(), and all existing code is moved over to using that. This is mostly preparation for the next commit, where we make direct use of dns_query_process_cname_one(). This also renames the DNS_QUERY_RESTARTED return value to DNS_QUERY_CNAME. That's because in the dns_query_process_cname_many() case as before if we return this we restarted the query in case we reached the end of the chain without a conclusive answer, as before. But in dns_query_process_cname_one() we'll only go one step anyway, and leave restarting if needed to the caller. Hence DNS_QUERY_RESTARTED is a bit of a misnomer in that case. This also gets rid of the weird tail recursion in dns_query_process_cname() and replaces it with an explicit loop in dns_query_process_cname_many(). The old recursion wasn't a security issue since we put a limit on the number of CNAMEs we follow anyway, but it's still icky to scale stack use by that. --- src/resolve/resolved-bus.c | 20 ++++---- src/resolve/resolved-dns-query.c | 82 ++++++++++++++++++++++++++++---- src/resolve/resolved-dns-query.h | 5 +- src/resolve/resolved-dns-stub.c | 4 +- src/resolve/resolved-varlink.c | 8 ++-- 5 files changed, 91 insertions(+), 28 deletions(-) diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 032ed0256b..c3624669ce 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -195,14 +195,14 @@ static void bus_method_resolve_hostname_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -486,14 +486,14 @@ static void bus_method_resolve_address_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -660,14 +660,14 @@ static void bus_method_resolve_record_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -1107,8 +1107,8 @@ static void resolve_service_hostname_complete(DnsQuery *q) { return; } - r = dns_query_process_cname(q); - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + r = dns_query_process_cname_many(q); + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; /* This auxiliary lookup is finished or failed, let's see if all are finished now. */ @@ -1180,14 +1180,14 @@ static void bus_method_resolve_service_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 8bc0607983..3a908a6b54 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -1036,7 +1036,7 @@ static int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname) return 0; } -int dns_query_process_cname(DnsQuery *q) { +int dns_query_process_cname_one(DnsQuery *q) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *cname = NULL; DnsQuestion *question; DnsResourceRecord *rr; @@ -1046,6 +1046,22 @@ int dns_query_process_cname(DnsQuery *q) { assert(q); + /* Processes a CNAME redirect if there's one. Returns one of three values: + * + * CNAME_QUERY_MATCH → direct RR match, caller should just use the RRs in this answer (and not + * bother with any CNAME/DNAME stuff) + * + * CNAME_QUERY_NOMATCH → no match at all, neither direct nor CNAME/DNAME, caller might decide to + * restart query or take things as NODATA reply. + * + * CNAME_QUERY_CNAME → no direct RR match, but a CNAME/DNAME match that we now followed for one step. + * + * The function might also return a failure, in particular -ELOOP if we encountered too many + * CNAMEs/DNAMEs in a chain or if following CNAMEs/DNAMEs was turned off. + * + * Note that this function doesn't actually restart the query. The caller can decide to do that in + * case of CNAME_QUERY_CNAME, though. */ + if (!IN_SET(q->state, DNS_TRANSACTION_SUCCESS, DNS_TRANSACTION_NULL)) return DNS_QUERY_NOMATCH; @@ -1112,17 +1128,63 @@ int dns_query_process_cname(DnsQuery *q) { if (r < 0) return r; - /* Let's see if the answer can already answer the new redirected question */ - r = dns_query_process_cname(q); - if (r != DNS_QUERY_NOMATCH) - return r; + return DNS_QUERY_CNAME; /* Tell caller that we did a single CNAME/DNAME redirection step */ +} - /* OK, it cannot, let's begin with the new query */ - r = dns_query_go(q); - if (r < 0) - return r; +int dns_query_process_cname_many(DnsQuery *q) { + int r; - return DNS_QUERY_RESTARTED; /* We restarted the query for a new cname */ + assert(q); + + /* Follows CNAMEs through the current packet: as long as the current packet can fulfill our + * redirected CNAME queries we keep going, and restart the query once the current packet isn't good + * enough anymore. It's a wrapper around dns_query_process_cname_one() and returns the same values, + * but with extended semantics. Specifically: + * + * DNS_QUERY_MATCH → as above + * + * DNS_QUERY_CNAME → we ran into a CNAME/DNAME redirect that we could not answer from the current + * message, and thus restarted the query to resolve it. + * + * DNS_QUERY_NOMATCH → we reached the end of CNAME/DNAME chain, and there are no direct matches nor a + * CNAME/DNAME match. i.e. this is a NODATA case. + * + * Note that this function will restart the query for the caller if needed, and that's the case + * DNS_QUERY_CNAME is returned. + */ + + r = dns_query_process_cname_one(q); + if (r != DNS_QUERY_CNAME) + return r; /* The first redirect is special: if it doesn't answer the question that's no + * reason to restart the query, we just accept this as a NODATA answer. */ + + for (;;) { + r = dns_query_process_cname_one(q); + if (r < 0 || r == DNS_QUERY_MATCH) + return r; + if (r == DNS_QUERY_NOMATCH) { + /* OK, so we followed one or more CNAME/DNAME RR but the existing packet can't answer + * this. Let's restart the query hence, with the new question. Why the different + * handling than the first chain element? Because if the server answers a direct + * question with an empty answer then this is a NODATA response. But if it responds + * with a CNAME chain that ultimately is incomplete (i.e. a non-empty but truncated + * CNAME chain) then we better follow up ourselves and ask for the rest of the + * chain. This is particular relevant since our cache will store CNAME/DNAME + * redirects that we learnt about for lookups of certain DNS types, but later on we + * can reuse this data even for other DNS types, but in that case need to follow up + * with the final lookup of the chain ourselves with the RR type we ourselves are + * interested in. */ + r = dns_query_go(q); + if (r < 0) + return r; + + return DNS_QUERY_CNAME; + } + + /* So we found a CNAME that the existing packet already answers, again via a CNAME, let's + * continue going then. */ + assert(r == DNS_QUERY_CNAME); + } } DnsQuestion* dns_query_question_for_protocol(DnsQuery *q, DnsProtocol protocol) { diff --git a/src/resolve/resolved-dns-query.h b/src/resolve/resolved-dns-query.h index 5d96cc06f8..39bf341d68 100644 --- a/src/resolve/resolved-dns-query.h +++ b/src/resolve/resolved-dns-query.h @@ -112,7 +112,7 @@ struct DnsQuery { enum { DNS_QUERY_MATCH, DNS_QUERY_NOMATCH, - DNS_QUERY_RESTARTED, + DNS_QUERY_CNAME, }; DnsQueryCandidate* dns_query_candidate_ref(DnsQueryCandidate*); @@ -129,7 +129,8 @@ int dns_query_make_auxiliary(DnsQuery *q, DnsQuery *auxiliary_for); int dns_query_go(DnsQuery *q); void dns_query_ready(DnsQuery *q); -int dns_query_process_cname(DnsQuery *q); +int dns_query_process_cname_one(DnsQuery *q); +int dns_query_process_cname_many(DnsQuery *q); void dns_query_complete(DnsQuery *q, DnsTransactionState state); diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 7cbbdc725c..cb5d075849 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -772,7 +772,7 @@ static void dns_stub_query_complete(DnsQuery *q) { switch (q->state) { case DNS_TRANSACTION_SUCCESS: - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(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); @@ -782,7 +782,7 @@ static void dns_stub_query_complete(DnsQuery *q) { log_debug_errno(r, "Failed to process CNAME: %m"); break; } - if (r == DNS_QUERY_RESTARTED) + if (r == DNS_QUERY_CNAME) return; _fallthrough_; diff --git a/src/resolve/resolved-varlink.c b/src/resolve/resolved-varlink.c index 77e75b8a8d..27d8c8967e 100644 --- a/src/resolve/resolved-varlink.c +++ b/src/resolve/resolved-varlink.c @@ -158,14 +158,14 @@ static void vl_method_resolve_hostname_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = varlink_error(q->varlink_request, "io.systemd.Resolve.CNAMELoop", NULL); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); @@ -395,14 +395,14 @@ static void vl_method_resolve_address_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = varlink_error(q->varlink_request, "io.systemd.Resolve.CNAMELoop", NULL); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); From 915ba31cfd6b87e9f3dd31f2bf5bd0b5c252738e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 23:29:16 +0100 Subject: [PATCH 3/3] resolved: rework CNAME logic a bit more When following CNAME/DNAME redirects in the stub we currently first iterate through the packet and pick up what we can use (in dns_stub_collect_answer_by_question() and friends), following all CNAMEs/DNAMEs, and would then issue dns_query_process_cname() to move the DnsQuery object forward too, where we'd then possibly restart the query and pick things up again, as above. There's one thought error in this though: dns_query_process_cname() tries to be smart and will internally follow not just a single CNAME/DNAME redirect, but a chain of them if they are contained inside the same packet until we reach the point where the answer is not included in the packet anymore, where we'd restart the query. This was great as long as we only focussed on the D-Bus and Varlink resolver APIs, since there the CNAME/DNAME chain in the middle doesn't actually matter, we just return information about the final name of the RR and its content, and aren't interested in the chain to it. For the DNS stub this is different however: there we need to place the full CNAME/DNAME chain (and all the appropriate metadata RRs) in the stub reply. Hence rework this so that we build on the fact that the previous commit split dns_query_process_cname() in two: 1. dns_query_process_cname_one() will do exactly one CNAME/DNAME redirect step. This will be called by the stub, so that we can pick up matching RRs for every single step along the way. 2. dns_query_process_cname_many() will follow a chain as long as that's possible within the same packet. It's thus pretty much identical to the old dns_query_process_cname() call. This is what we now use in the D-Bus and Varlink APIs. dns_query_process_cname_many() is basically just a loop around dns_query_process_cname_one(). Any logic to follow and pick up RRs manually in the stub along the CNAME/DNAME path is now dropped (i.e. dns_stub_collect_answer_by_question() becomes trivially simple again), we solely rely on dns_query_process_cname_one() to follow CNAME/DNAME now: each step followed by a full call of dns_stub_assign_sections() to copy out the RRs that matter. Net result: things are a bit simpler again, as the only place we follow CNAME/DNAME redirects is DnsQuery again, and stub answers are always complete: they contain all CNAME/DNAME RRs on the way including all their metadata we might pick up in the other sections. --- src/resolve/resolved-dns-stub.c | 159 +++++++++++++++----------------- 1 file changed, 76 insertions(+), 83 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index cb5d075849..177b88e025 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -161,89 +161,40 @@ static int dns_stub_collect_answer_by_question( DnsQuestion *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'. 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 */ + /* Copies all RRs from 'answer' into 'reply', if they match 'question'. */ - for (;;) { - _cleanup_(dns_resource_key_unrefp) DnsResourceKey *next_redirected_key = NULL; + DNS_ANSWER_FOREACH_ITEM(item, answer) { - 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 */ - - 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 */ - - 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; - - /* 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 */ - 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. */ - - r = reply_add_with_rrsig( - reply, - item->rr, - item->ifindex, - item->flags & ~DNS_ANSWER_MASK_SECTIONS, - item->rrsig, - with_rrsig); + /* 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; + if (!r) { + /* Maybe there's a CNAME/DNAME in here? If so, that's an answer too */ + r = dns_question_matches_cname_or_dname(question, item->rr, NULL); if (r < 0) return r; + if (!r) + continue; } - if (!next_redirected_key) - break; + /* 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. */ - dns_resource_key_unref(redirected_key); - redirected_key = TAKE_PTR(next_redirected_key); + 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; } return 0; @@ -771,21 +722,63 @@ static void dns_stub_query_complete(DnsQuery *q) { switch (q->state) { - case DNS_TRANSACTION_SUCCESS: - r = dns_query_process_cname_many(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; + case DNS_TRANSACTION_SUCCESS: { + bool first = true; + + for (;;) { + int cname_result; + + cname_result = dns_query_process_cname_one(q); + if (cname_result == -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 (cname_result < 0) { + log_debug_errno(cname_result, "Failed to process CNAME: %m"); + break; + } + + if (cname_result == DNS_QUERY_NOMATCH) { + /* This answer doesn't contain any RR that would answer our question + * positively, i.e. neither directly nor via CNAME. */ + + if (first) /* We never followed a CNAME and the answer doesn't match our + * question at all? Then this is final, the empty answer is the + * answer. */ + break; + + /* Otherwise, we already followed a CNAME once within this packet, and the + * packet doesn't answer our question. In that case let's restart the query, + * now with the redirected question. We'll */ + r = dns_query_go(q); + if (r < 0) + log_debug_errno(r, "Failed to restart query: %m"); + + return; + } + + r = dns_stub_assign_sections( + q, + dns_query_question_for_protocol(q, DNS_PROTOCOL_DNS), + dns_stub_reply_with_edns0_do(q)); + if (r < 0) { + log_debug_errno(r, "Failed to assign sections: %m"); + dns_query_free(q); + return; + } + + if (cname_result == DNS_QUERY_MATCH) /* A match? Then we are done, let's return what we got */ + break; + + /* We followed a CNAME. and collected the RRs that answer the redirected question + * successfully. Let's not try to do this again. */ + assert(cname_result == DNS_QUERY_CNAME); + first = false; } - if (r < 0) { - log_debug_errno(r, "Failed to process CNAME: %m"); - break; - } - if (r == DNS_QUERY_CNAME) - return; _fallthrough_; + } case DNS_TRANSACTION_RCODE_FAILURE: (void) dns_stub_send_reply(q, q->answer_rcode);