From 2c42a217a2ef4a62933290f5ef204bcf89ea52fd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 11 Nov 2020 21:03:04 +0100 Subject: [PATCH 1/3] resolved: when we can't parse a packet, downgrade feature level So far we didn't really handle the case where we can't parse a reply packet. Since this apparently happens in real-life though, let's add some minimal logic, to downgrade/restart if we see this. --- src/resolve/resolved-dns-server.c | 34 ++++++++++++++++++++++++-- src/resolve/resolved-dns-server.h | 9 ++++--- src/resolve/resolved-dns-transaction.c | 10 ++++++++ 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index 40da80b39a..70bb7178dd 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -240,6 +240,7 @@ static void dns_server_reset_counters(DnsServer *s) { s->n_failed_tcp = 0; s->n_failed_tls = 0; s->packet_truncated = false; + s->packet_invalid = false; s->verified_usec = 0; /* Note that we do not reset s->packet_bad_opt and s->packet_rrsig_missing here. We reset them only when the @@ -366,6 +367,17 @@ void dns_server_packet_rcode_downgrade(DnsServer *s, DnsServerFeatureLevel level log_debug("Downgrading transaction feature level fixed an RCODE error, downgrading server %s too.", strna(dns_server_string_full(s))); } +void dns_server_packet_invalid(DnsServer *s, DnsServerFeatureLevel level) { + assert(s); + + /* Invoked whenever we got a packet we couldn't parse at all */ + + if (s->possible_feature_level != level) + return; + + s->packet_invalid = true; +} + static bool dns_server_grace_period_expired(DnsServer *s) { usec_t ts; @@ -442,6 +454,22 @@ DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s) { log_debug("Server doesn't support DNS-over-TLS, downgrading protocol..."); s->possible_feature_level--; + + } else if (s->packet_invalid && + s->possible_feature_level > DNS_SERVER_FEATURE_LEVEL_UDP && + s->possible_feature_level != DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN) { + + /* Downgrade from DO to EDNS0 + from EDNS0 to UDP, from TLS+DO to plain TLS. Or in + * other words, if we receive a packet we cannot parse jump to the next lower feature + * level that actually has an influence on the packet layout (and not just the + * transport). */ + + log_debug("Got invalid packet from server, downgrading protocol..."); + s->possible_feature_level = + s->possible_feature_level == DNS_SERVER_FEATURE_LEVEL_TLS_DO ? DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN : + DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(s->possible_feature_level) ? DNS_SERVER_FEATURE_LEVEL_EDNS0 : + DNS_SERVER_FEATURE_LEVEL_UDP; + } else if (s->packet_bad_opt && s->possible_feature_level >= DNS_SERVER_FEATURE_LEVEL_EDNS0) { @@ -903,13 +931,15 @@ void dns_server_dump(DnsServer *s, FILE *f) { "\tFailed TCP attempts: %u\n" "\tSeen truncated packet: %s\n" "\tSeen OPT RR getting lost: %s\n" - "\tSeen RRSIG RR missing: %s\n", + "\tSeen RRSIG RR missing: %s\n" + "\tSeen invalid packet: %s\n", s->received_udp_packet_max, s->n_failed_udp, s->n_failed_tcp, yes_no(s->packet_truncated), yes_no(s->packet_bad_opt), - yes_no(s->packet_rrsig_missing)); + yes_no(s->packet_rrsig_missing), + yes_no(s->packet_invalid)); } void dns_server_unref_stream(DnsServer *s) { diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h index 96e46226ee..b6f9660748 100644 --- a/src/resolve/resolved-dns-server.h +++ b/src/resolve/resolved-dns-server.h @@ -41,6 +41,7 @@ typedef enum DnsServerFeatureLevel { #define DNS_SERVER_FEATURE_LEVEL_WORST 0 #define DNS_SERVER_FEATURE_LEVEL_BEST (_DNS_SERVER_FEATURE_LEVEL_MAX - 1) #define DNS_SERVER_FEATURE_LEVEL_IS_TLS(x) IN_SET(x, DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN, DNS_SERVER_FEATURE_LEVEL_TLS_DO) +#define DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(x) ((x) >= DNS_SERVER_FEATURE_LEVEL_DO) const char* dns_server_feature_level_to_string(int i) _const_; int dns_server_feature_level_from_string(const char *s) _pure_; @@ -78,9 +79,10 @@ struct DnsServer { unsigned n_failed_tcp; unsigned n_failed_tls; - bool packet_truncated:1; - bool packet_bad_opt:1; - bool packet_rrsig_missing:1; + bool packet_truncated:1; /* Set when TC bit was set on reply */ + bool packet_bad_opt:1; /* Set when OPT was missing or otherwise bad on reply */ + bool packet_rrsig_missing:1; /* Set when RRSIG was missing */ + bool packet_invalid:1; /* Set when we failed to parse a reply */ usec_t verified_usec; usec_t features_grace_period_usec; @@ -119,6 +121,7 @@ void dns_server_packet_truncated(DnsServer *s, DnsServerFeatureLevel level); void dns_server_packet_rrsig_missing(DnsServer *s, DnsServerFeatureLevel level); void dns_server_packet_bad_opt(DnsServer *s, DnsServerFeatureLevel level); void dns_server_packet_rcode_downgrade(DnsServer *s, DnsServerFeatureLevel level); +void dns_server_packet_invalid(DnsServer *s, DnsServerFeatureLevel level); DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index bd2ae5e4fb..66226c0681 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1189,6 +1189,16 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p, bool encrypt /* After the superficial checks, actually parse the message. */ r = dns_packet_extract(p); if (r < 0) { + if (t->server) { + dns_server_packet_invalid(t->server, t->current_feature_level); + + r = dns_transaction_maybe_restart(t); + if (r < 0) + goto fail; + if (r > 0) /* Transaction got restarted... */ + return; + } + dns_transaction_complete(t, DNS_TRANSACTION_INVALID_REPLY); return; } From d8592a4e2ff31610b3029a3067c8207a124f284a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 11 Nov 2020 21:19:22 +0100 Subject: [PATCH 2/3] resolved: make feature level checks a bit more discriptive The levels have an order, but the order is sometimes a bit arbitrary. Hence add simple macros to check for specific features and use those, so that the ordering leaks a bit less into all files. --- src/resolve/resolved-dns-server.c | 8 +++++--- src/resolve/resolved-dns-server.h | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index 70bb7178dd..5f0d54acc5 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -446,8 +446,10 @@ DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s) { * work. Upgrade back to UDP again. */ log_debug("Reached maximum number of failed TCP connection attempts, trying UDP again..."); s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_UDP; + } else if (s->n_failed_tls > 0 && - DNS_SERVER_FEATURE_LEVEL_IS_TLS(s->possible_feature_level) && dns_server_get_dns_over_tls_mode(s) != DNS_OVER_TLS_YES) { + DNS_SERVER_FEATURE_LEVEL_IS_TLS(s->possible_feature_level) && + dns_server_get_dns_over_tls_mode(s) != DNS_OVER_TLS_YES) { /* We tried to connect using DNS-over-TLS, and it didn't work. Downgrade to plaintext UDP * if we don't require DNS-over-TLS */ @@ -471,7 +473,7 @@ DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s) { DNS_SERVER_FEATURE_LEVEL_UDP; } else if (s->packet_bad_opt && - s->possible_feature_level >= DNS_SERVER_FEATURE_LEVEL_EDNS0) { + DNS_SERVER_FEATURE_LEVEL_IS_EDNS0(s->possible_feature_level)) { /* A reply to one of our EDNS0 queries didn't carry a valid OPT RR, then downgrade to below * EDNS0 levels. After all, some records generate different responses with and without OPT RR @@ -486,7 +488,7 @@ DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s) { log_level = LOG_NOTICE; } else if (s->packet_rrsig_missing && - s->possible_feature_level >= DNS_SERVER_FEATURE_LEVEL_DO) { + DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(s->possible_feature_level)) { /* RRSIG data was missing on a EDNS0 packet with DO bit set. This means the server doesn't * augment responses with DNSSEC RRs. If so, let's better not ask the server for it anymore, diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h index b6f9660748..16e492743d 100644 --- a/src/resolve/resolved-dns-server.h +++ b/src/resolve/resolved-dns-server.h @@ -40,6 +40,7 @@ typedef enum DnsServerFeatureLevel { #define DNS_SERVER_FEATURE_LEVEL_WORST 0 #define DNS_SERVER_FEATURE_LEVEL_BEST (_DNS_SERVER_FEATURE_LEVEL_MAX - 1) +#define DNS_SERVER_FEATURE_LEVEL_IS_EDNS0(x) ((x) >= DNS_SERVER_FEATURE_LEVEL_EDNS0) #define DNS_SERVER_FEATURE_LEVEL_IS_TLS(x) IN_SET(x, DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN, DNS_SERVER_FEATURE_LEVEL_TLS_DO) #define DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(x) ((x) >= DNS_SERVER_FEATURE_LEVEL_DO) From fba3e94df552b4fb6dd53b5e605259be42a27f4e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 12 Nov 2020 16:05:15 +0100 Subject: [PATCH 3/3] resolved: never go below DNSSEC feature level in DNSSEC strict mode This adjusts our feature level handling: when DNSSEC strict mode is on, let's never lower the feature level below the lowest DNSSEC mode. Also, when asking whether DNSSEC is supproted, always say yes in strict mode. This means that error reporting about transactions that fail because of missing DNSSEC RRs will not report "incompatible-server" but instead "missing-signature" or suchlike. The main difference here is that DNSSEC failures become local to a transaction, instead of propagating into the feature level we reuse for future transactions. This is beneficial with routers that implement "mostly a DNS proxy", i.e. that propagate most DNS requests 1:1 to their upstream servers, but synthesize local answers for a select few domains. For example, AVM Fritz!Boxes operate that way: they proxy most traffic 1:1 upstream in an DNSSEC-compatible fashion, but synthesize the "fritz.box" locally, so that it can be used to configure the router. This local domain cannot be DNSSEC verified, it comes without signatures. Previously this would mean once that domain was resolved feature level would be downgraded, and we'd thus fail all future DNSSEC attempts. With this change, the immediate lookup for "fritz.box" will fail validation, but for all other unrelated future ones that comes without prejudice. (While we are at it, also make a couple of other downgrade paths a bit tighter.) Fixes: #10570 #14435 #6490 --- src/resolve/resolved-dns-server.c | 73 +++++++++++++++++++++---------- src/resolve/resolved-dns-server.h | 1 + 2 files changed, 50 insertions(+), 24 deletions(-) diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index 5f0d54acc5..a561035faa 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -473,12 +473,19 @@ DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s) { DNS_SERVER_FEATURE_LEVEL_UDP; } else if (s->packet_bad_opt && - DNS_SERVER_FEATURE_LEVEL_IS_EDNS0(s->possible_feature_level)) { + DNS_SERVER_FEATURE_LEVEL_IS_EDNS0(s->possible_feature_level) && + dns_server_get_dnssec_mode(s) != DNSSEC_YES && + dns_server_get_dns_over_tls_mode(s) != DNS_OVER_TLS_YES) { - /* A reply to one of our EDNS0 queries didn't carry a valid OPT RR, then downgrade to below - * EDNS0 levels. After all, some records generate different responses with and without OPT RR - * in the request. Example: - * https://open.nlnetlabs.nl/pipermail/dnssec-trigger/2014-November/000376.html */ + /* A reply to one of our EDNS0 queries didn't carry a valid OPT RR, then downgrade to + * below EDNS0 levels. After all, some servers generate different responses with and + * without OPT RR in the request. Example: + * + * https://open.nlnetlabs.nl/pipermail/dnssec-trigger/2014-November/000376.html + * + * If we are in strict DNSSEC or DoT mode, we don't do this kind of downgrade + * however, as both modes imply EDNS0 to work (DNSSEC strictly requires it, and DoT + * only in our implementation). */ log_debug("Server doesn't support EDNS(0) properly, downgrading feature level..."); s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_UDP; @@ -488,42 +495,57 @@ DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s) { log_level = LOG_NOTICE; } else if (s->packet_rrsig_missing && - DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(s->possible_feature_level)) { + DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(s->possible_feature_level) && + dns_server_get_dnssec_mode(s) != DNSSEC_YES) { - /* RRSIG data was missing on a EDNS0 packet with DO bit set. This means the server doesn't - * augment responses with DNSSEC RRs. If so, let's better not ask the server for it anymore, - * after all some servers generate different replies depending if an OPT RR is in the query or - * not. */ + /* RRSIG data was missing on a EDNS0 packet with DO bit set. This means the server + * doesn't augment responses with DNSSEC RRs. If so, let's better not ask the server + * for it anymore, after all some servers generate different replies depending if an + * OPT RR is in the query or not. If we are in strict DNSSEC mode, don't allow such + * downgrades however, since a DNSSEC feature level is a requirement for strict + * DNSSEC mode. */ log_debug("Detected server responses lack RRSIG records, downgrading feature level..."); - s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_IS_TLS(s->possible_feature_level) ? DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN : DNS_SERVER_FEATURE_LEVEL_EDNS0; + s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_IS_TLS(s->possible_feature_level) ? DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN : + DNS_SERVER_FEATURE_LEVEL_EDNS0; } else if (s->n_failed_udp >= DNS_SERVER_FEATURE_RETRY_ATTEMPTS && - s->possible_feature_level >= (dns_server_get_dnssec_mode(s) == DNSSEC_YES ? DNS_SERVER_FEATURE_LEVEL_LARGE : DNS_SERVER_FEATURE_LEVEL_UDP)) { + DNS_SERVER_FEATURE_LEVEL_IS_UDP(s->possible_feature_level) && + ((s->possible_feature_level != DNS_SERVER_FEATURE_LEVEL_DO) || dns_server_get_dnssec_mode(s) != DNSSEC_YES)) { - /* We lost too many UDP packets in a row, and are on a feature level of UDP or higher. If the - * packets are lost, maybe the server cannot parse them, hence downgrading sounds like a good - * idea. We might downgrade all the way down to TCP this way. + /* We lost too many UDP packets in a row, and are on an UDP feature level. If the + * packets are lost, maybe the server cannot parse them, hence downgrading sounds + * like a good idea. We might downgrade all the way down to TCP this way. * * If strict DNSSEC mode is used we won't downgrade below DO level however, as packet loss * might have many reasons, a broken DNSSEC implementation being only one reason. And if the * user is strict on DNSSEC, then let's assume that DNSSEC is not the fault here. */ log_debug("Lost too many UDP packets, downgrading feature level..."); - s->possible_feature_level--; + if (s->possible_feature_level == DNS_SERVER_FEATURE_LEVEL_DO) /* skip over TLS_PLAIN */ + s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_EDNS0; + else + s->possible_feature_level--; } else if (s->n_failed_tcp >= DNS_SERVER_FEATURE_RETRY_ATTEMPTS && s->packet_truncated && - s->possible_feature_level > (dns_server_get_dnssec_mode(s) == DNSSEC_YES ? DNS_SERVER_FEATURE_LEVEL_LARGE : DNS_SERVER_FEATURE_LEVEL_UDP)) { + s->possible_feature_level > DNS_SERVER_FEATURE_LEVEL_UDP && + DNS_SERVER_FEATURE_LEVEL_IS_UDP(s->possible_feature_level) && + (!DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(s->possible_feature_level) || dns_server_get_dnssec_mode(s) != DNSSEC_YES)) { - /* We got too many TCP connection failures in a row, we had at least one truncated packet, and - * are on a feature level above UDP. By downgrading things and getting rid of DNSSEC or EDNS0 - * data we hope to make the packet smaller, so that it still works via UDP given that TCP - * appears not to be a fallback. Note that if we are already at the lowest UDP level, we don't - * go further down, since that's TCP, and TCP failed too often after all. */ + /* We got too many TCP connection failures in a row, we had at least one truncated + * packet, and are on feature level above UDP. By downgrading things and getting rid + * of DNSSEC or EDNS0 data we hope to make the packet smaller, so that it still + * works via UDP given that TCP appears not to be a fallback. Note that if we are + * already at the lowest UDP level, we don't go further down, since that's TCP, and + * TCP failed too often after all. */ log_debug("Got too many failed TCP connection failures and truncated UDP packets, downgrading feature level..."); - s->possible_feature_level--; + + if (DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(s->possible_feature_level)) + s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_EDNS0; /* Go DNSSEC → EDNS0 */ + else + s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_UDP; /* Go EDNS0 → UDP */ } if (p != s->possible_feature_level) { @@ -619,7 +641,10 @@ bool dns_server_dnssec_supported(DnsServer *server) { /* Returns whether the server supports DNSSEC according to what we know about it */ - if (server->possible_feature_level < DNS_SERVER_FEATURE_LEVEL_DO) + if (dns_server_get_dnssec_mode(server) == DNSSEC_YES) /* If strict DNSSEC mode is enabled, always assume DNSSEC mode is supported. */ + return true; + + if (!DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(server->possible_feature_level)) return false; if (server->packet_bad_opt) diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h index 16e492743d..7860e32183 100644 --- a/src/resolve/resolved-dns-server.h +++ b/src/resolve/resolved-dns-server.h @@ -43,6 +43,7 @@ typedef enum DnsServerFeatureLevel { #define DNS_SERVER_FEATURE_LEVEL_IS_EDNS0(x) ((x) >= DNS_SERVER_FEATURE_LEVEL_EDNS0) #define DNS_SERVER_FEATURE_LEVEL_IS_TLS(x) IN_SET(x, DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN, DNS_SERVER_FEATURE_LEVEL_TLS_DO) #define DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(x) ((x) >= DNS_SERVER_FEATURE_LEVEL_DO) +#define DNS_SERVER_FEATURE_LEVEL_IS_UDP(x) IN_SET(x, DNS_SERVER_FEATURE_LEVEL_UDP, DNS_SERVER_FEATURE_LEVEL_EDNS0, DNS_SERVER_FEATURE_LEVEL_DO, DNS_SERVER_FEATURE_LEVEL_LARGE) const char* dns_server_feature_level_to_string(int i) _const_; int dns_server_feature_level_from_string(const char *s) _pure_;