From 008f23b7c5f255e48c71c3b4da74a85448667817 Mon Sep 17 00:00:00 2001 From: Ronan Pigott Date: Mon, 7 Oct 2024 11:05:18 -0700 Subject: [PATCH 1/4] resolved: authenticate bypass queries Following 13e15dae9f0b, resolved does not forward the AD bit for bypass queries, but resolved also didn't do it's own validation, making these replies appear to never be authentic. We should enable validation for bypass queries. Let's disable our own validation when processing a +cd query, and also ensure that it skips the cache so that we don't accidentally fail to return inauthentic replies from upstream. Previously, when we had a bypass transaction without cd, a cached, authenticated, reply with cd could be served, leaving the cd bit erroneously set in the reply. Only reply with a CD bit if the client requested it. Fixes: 13e15dae9f0b (resolved: clear the AD bit for bypass packets) --- src/resolve/resolved-dns-packet.h | 1 + src/resolve/resolved-dns-stub.c | 13 ++++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/resolve/resolved-dns-packet.h b/src/resolve/resolved-dns-packet.h index 13d65a02c3..437c220b6a 100644 --- a/src/resolve/resolved-dns-packet.h +++ b/src/resolve/resolved-dns-packet.h @@ -111,6 +111,7 @@ static inline uint8_t* DNS_PACKET_DATA(const DnsPacket *p) { #define DNS_PACKET_AD(p) ((be16toh(DNS_PACKET_HEADER(p)->flags) >> 5) & 1) #define DNS_PACKET_CD(p) ((be16toh(DNS_PACKET_HEADER(p)->flags) >> 4) & 1) +#define DNS_PACKET_FLAG_CD (UINT16_C(1) << 4) #define DNS_PACKET_FLAG_AD (UINT16_C(1) << 5) #define DNS_PACKET_FLAG_TC (UINT16_C(1) << 9) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index c604a51c4b..ee1e31010c 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -686,6 +686,7 @@ static int dns_stub_patch_bypass_reply_packet( DnsPacket **ret, /* Where to place the patched packet */ DnsPacket *original, /* The packet to patch */ DnsPacket *request, /* The packet the patched packet shall look like a reply to */ + bool validated, bool authenticated) { _cleanup_(dns_packet_unrefp) DnsPacket *c = NULL; int r; @@ -726,9 +727,14 @@ static int dns_stub_patch_bypass_reply_packet( DNS_PACKET_HEADER(c)->flags = htobe16(be16toh(DNS_PACKET_HEADER(c)->flags) | DNS_PACKET_FLAG_TC); } + /* Patch the cd bit to reflect the state of validation: set when both we and the upstream + * resolver have checking disabled. */ + DNS_PACKET_HEADER(c)->flags = htobe16(UPDATE_FLAG(be16toh(DNS_PACKET_HEADER(c)->flags), + DNS_PACKET_FLAG_CD, DNS_PACKET_CD(original) && !validated)); + /* Ensure we don't pass along an untrusted ad flag for bypass packets */ - if (!authenticated) - DNS_PACKET_HEADER(c)->flags = htobe16(be16toh(DNS_PACKET_HEADER(c)->flags) & ~DNS_PACKET_FLAG_AD); + DNS_PACKET_HEADER(c)->flags = htobe16(UPDATE_FLAG(be16toh(DNS_PACKET_HEADER(c)->flags), + DNS_PACKET_FLAG_AD, authenticated)); *ret = TAKE_PTR(c); return 0; @@ -751,6 +757,7 @@ static void dns_stub_query_complete(DnsQuery *query) { _cleanup_(dns_packet_unrefp) DnsPacket *reply = NULL; r = dns_stub_patch_bypass_reply_packet(&reply, q->answer_full_packet, q->request_packet, + /* validated = */ !FLAGS_SET(q->flags, SD_RESOLVED_NO_VALIDATE), FLAGS_SET(q->answer_query_flags, SD_RESOLVED_AUTHENTICATED)); if (r < 0) log_debug_errno(r, "Failed to patch bypass reply packet: %m"); @@ -982,7 +989,7 @@ static void dns_stub_process_query(Manager *m, DnsStubListenerExtra *l, DnsStrea protocol_flags| SD_RESOLVED_NO_CNAME| SD_RESOLVED_NO_SEARCH| - SD_RESOLVED_NO_VALIDATE| + (DNS_PACKET_CD(p) ? SD_RESOLVED_NO_VALIDATE | SD_RESOLVED_NO_CACHE : 0)| SD_RESOLVED_REQUIRE_PRIMARY| SD_RESOLVED_CLAMP_TTL| SD_RESOLVED_RELAX_SINGLE_LABEL); From 36074e01499ea4fc89a0c642cef83bd650014e5a Mon Sep 17 00:00:00 2001 From: Ronan Pigott Date: Fri, 18 Oct 2024 21:10:57 -0700 Subject: [PATCH 2/4] resolved: enable CD bit without DO set This is useful for a validating resolver to indicate to a non-validating resolver when checking was disabled for the query. This matches the behavior of the major public resovlers in response to queries with CD bu tnot DO set. --- src/resolve/resolved-dns-stub.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index ee1e31010c..bd0822e6dc 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -462,10 +462,6 @@ static int dns_stub_finish_reply_packet( rcode = DNS_RCODE_SERVFAIL; } - /* Don't set the CD bit unless DO is on, too */ - if (!edns0_do) - cd = false; - /* Note that we allow the AD bit to be set even if client didn't signal DO, as per RFC 6840, section * 5.7 */ @@ -631,7 +627,7 @@ static int dns_stub_send_reply( !!q->request_packet->opt, edns0_do, (DNS_PACKET_AD(q->request_packet) || DNS_PACKET_DO(q->request_packet)) && dns_query_fully_authenticated(q), - DNS_PACKET_CD(q->request_packet), + FLAGS_SET(q->flags, SD_RESOLVED_NO_VALIDATE), q->stub_listener_extra ? ADVERTISE_EXTRA_DATAGRAM_SIZE_MAX : ADVERTISE_DATAGRAM_SIZE_MAX, dns_packet_has_nsid_request(q->request_packet) > 0 && !q->stub_listener_extra); if (r < 0) @@ -997,6 +993,7 @@ static void dns_stub_process_query(Manager *m, DnsStubListenerExtra *l, DnsStrea r = dns_query_new(m, &q, p->question, p->question, NULL, 0, protocol_flags| SD_RESOLVED_NO_SEARCH| + (DNS_PACKET_CD(p) ? SD_RESOLVED_NO_VALIDATE | SD_RESOLVED_NO_CACHE : 0)| (DNS_PACKET_DO(p) ? SD_RESOLVED_REQUIRE_PRIMARY : 0)| SD_RESOLVED_CLAMP_TTL); if (r < 0) { From fa02d04ee9a4787c2c39032185b6fe3e7146b26b Mon Sep 17 00:00:00 2001 From: Ronan Pigott Date: Sat, 12 Oct 2024 10:00:56 -0700 Subject: [PATCH 3/4] resolved: update condition for caching full packets Previously a full packet was cached only if the CD bit was set, but this no longer corresponds to the cases where bypass is enabled. Update the cache to retain a full packet in the cases where it might actually be useful. --- src/resolve/resolved-dns-transaction.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 17a815cd44..f78bf0702b 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -840,10 +840,8 @@ static void dns_transaction_cache_answer(DnsTransaction *t) { dns_transaction_key(t), t->answer_rcode, t->answer, - DNS_PACKET_CD(t->received) ? t->received : NULL, /* only cache full packets with CD on, - * since our use case for caching them - * is "bypass" mode which is only - * enabled for CD packets. */ + /* If neither DO nor EDE is set, the full packet isn't useful to cache */ + DNS_PACKET_DO(t->received) || t->answer_ede_rcode > 0 || t->answer_ede_msg ? t->received : NULL, t->answer_query_flags, t->answer_dnssec_result, t->answer_nsec_ttl, From b7b1c50c6afd2ab8c2c779df9b73c6dbbc773f70 Mon Sep 17 00:00:00 2001 From: Ronan Pigott Date: Thu, 10 Oct 2024 22:40:03 -0700 Subject: [PATCH 4/4] test: exercise bypass mode on the sd-resolved stub A basic test will verify that we provide the right flags. --- test/units/TEST-75-RESOLVED.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/units/TEST-75-RESOLVED.sh b/test/units/TEST-75-RESOLVED.sh index 801104710b..4a7b759717 100755 --- a/test/units/TEST-75-RESOLVED.sh +++ b/test/units/TEST-75-RESOLVED.sh @@ -517,8 +517,17 @@ testcase_08_resolved() { grep -qF "fd00:dead:beef:cafe::11" "$RUN_OUT" grep -qF "authenticated: yes" "$RUN_OUT" - run dig +short signed.test + run dig +nostats signed.test grep -qF "10.0.0.10" "$RUN_OUT" + grep -q "flags:[^;]* ad" "$RUN_OUT" + run dig +nostats +cd signed.test + grep -qF "10.0.0.10" "$RUN_OUT" + grep -q "flags:[^;]* cd" "$RUN_OUT" + grep -qv "flags:[^;]* ad" "$RUN_OUT" + run dig +nostats +do signed.test + grep -qF "10.0.0.10" "$RUN_OUT" + grep -q "flags:[^;]* ad" "$RUN_OUT" + grep -qv "flags:[^;]* cd" "$RUN_OUT" run resolvectl query signed.test grep -qF "signed.test: 10.0.0.10" "$RUN_OUT" grep -qF "authenticated: yes" "$RUN_OUT"