From af45251e4ce2ff48eb86769cccf48ffd51f49ce0 Mon Sep 17 00:00:00 2001 From: Tom Yan Date: Wed, 8 Dec 2021 10:53:07 +0800 Subject: [PATCH 1/3] resolved: filter stub listeners in manager_get_dns_server() Commit 49ef064c8dcd8ed12d98e6c705e676babade0897 attempts to handle "stub loop" by switching to the next server *after the query has been made*. The approach may be good enough for link scopes. However, for the manager / global scope, it is not. First of all, there are more than one types (SYSTEM and FALLBACK) of servers it can use. Also, whether those of type FALLBACK should be used depends. Besides, dns_scope_good_domain() determines whether things should be routed to a scope by checking whether the scope has a server. The decision made would be incorrect if stubs were not filtered beforehand. Therefore, to avoid failing query unnecessarily, and to make sure that extra stub listeners will not trigger unexpected and/or inconsistent behavior, make manager_get_dns_server() do what it should have done. --- src/resolve/resolved-dns-server.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index 7d180378b6..cd755b13d4 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -875,9 +875,18 @@ DnsServer *manager_get_dns_server(Manager *m) { manager_read_resolv_conf(m); /* If no DNS server was chosen so far, pick the first one */ - if (!m->current_dns_server) + if (!m->current_dns_server || + /* In case m->current_dns_server != m->dns_servers */ + manager_server_is_stub(m, m->current_dns_server)) manager_set_dns_server(m, m->dns_servers); + while (m->current_dns_server && + manager_server_is_stub(m, m->current_dns_server)) { + manager_next_dns_server(m, NULL); + if (m->current_dns_server == m->dns_servers) + manager_set_dns_server(m, NULL); + } + if (!m->current_dns_server) { bool found = false; From 9d84fdec287532c0d58914c64988b3027902c4e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Dec 2021 10:54:17 +0100 Subject: [PATCH 2/3] resolved: return immediately if we already know what to return --- src/resolve/resolved-conf.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/resolve/resolved-conf.c b/src/resolve/resolved-conf.c index a4e44f29be..930313b844 100644 --- a/src/resolve/resolved-conf.c +++ b/src/resolve/resolved-conf.c @@ -66,17 +66,13 @@ int manager_parse_dns_server_string_and_warn(Manager *m, DnsServerType type, con _cleanup_free_ char *word = NULL; r = extract_first_word(&string, &word, NULL, 0); - if (r < 0) + if (r <= 0) return r; - if (r == 0) - break; r = manager_add_dns_server_by_string(m, type, word); if (r < 0) log_warning_errno(r, "Failed to add DNS server address '%s', ignoring: %m", word); } - - return 0; } static int manager_add_search_domain_by_string(Manager *m, const char *domain) { @@ -121,17 +117,13 @@ int manager_parse_search_domains_and_warn(Manager *m, const char *string) { _cleanup_free_ char *word = NULL; r = extract_first_word(&string, &word, NULL, EXTRACT_UNQUOTE); - if (r < 0) + if (r <= 0) return r; - if (r == 0) - break; r = manager_add_search_domain_by_string(m, word); if (r < 0) log_warning_errno(r, "Failed to add search domain '%s', ignoring: %m", word); } - - return 0; } int config_parse_dns_servers( From 0ad4efb14beea9148838a0d974821e3b98cafc47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Dec 2021 11:42:59 +0100 Subject: [PATCH 3/3] resolved: filter out our own stub resolvers when parsing servers We get "upstream" dns server config from ~three places: /etc/resolv.conf, config files, and runtime config via dbus. With this commit, we'll filter out our own stub listeners if they are configured in either of the first two sources. For /etc/resolv.conf this is done quitely, and for our own config files, a LOG_INFO message is emitted, since this is a small inconsistency in the config. Setting loops like this over dbus is still allowed. The reason is that in the past we didn't treat this as an error, and if we were to start responding with an error, we could break a scenario that worked previously. E.g. NM sends us a list of servers, and one happens to be the our own. We would just not use that stub server before, but it'd still be shown in the dbus properties and such. We would have to return error for the whole message, also rejecting the other valid servers. I think it's easier to just keep that part unchanged. Test case: $ ls -l /etc/resolv.conf -rw-r--r-- 1 root root 57 Dec 15 10:26 /etc/resolv.conf $ cat /etc/resolv.conf nameserver 192.168.150.1 options edns0 trust-ad search . $ cat /etc/systemd/resolved.conf.d/stub.conf [Resolve] DNSStubListenerExtra=192.168.150.1 $ resolvectl ... Global resolv.conf mode: foreign DNS Servers: 192.168.150.1 Fallback DNS Servers: ... (with the patch): Global resolv.conf mode: foreign Fallback DNS Servers: ... --- src/resolve/resolved-conf.c | 28 +++++++++++++++------------- src/resolve/resolved-conf.h | 2 +- src/resolve/resolved-manager.c | 23 +++++++++++++++-------- src/resolve/resolved-manager.h | 1 + src/resolve/resolved-resolv-conf.c | 3 ++- 5 files changed, 34 insertions(+), 23 deletions(-) diff --git a/src/resolve/resolved-conf.c b/src/resolve/resolved-conf.c index 930313b844..7873c363b3 100644 --- a/src/resolve/resolved-conf.c +++ b/src/resolve/resolved-conf.c @@ -35,15 +35,16 @@ static int manager_add_dns_server_by_string(Manager *m, DnsServerType type, cons if (r < 0) return r; - /* Silently filter out 0.0.0.0, 127.0.0.53, 127.0.0.54 (our own stub DNS listener) */ - if (!dns_server_address_valid(family, &address)) - return 0; - - /* By default, the port number is determined with the transaction feature level. + /* By default, the port number is determined by the transaction feature level. * See dns_transaction_port() and dns_server_port(). */ if (IN_SET(port, 53, 853)) port = 0; + /* Refuse 0.0.0.0, 127.0.0.53, 127.0.0.54 and the rest of our own stub DNS listeners. */ + if (!dns_server_address_valid(family, &address) || + manager_server_address_is_stub(m, family, &address, port ?: 53)) + return -ELOOP; + /* Filter out duplicates */ s = dns_server_find(manager_get_first_dns_server(m, type), family, &address, port, ifindex, server_name); if (s) { @@ -56,7 +57,7 @@ static int manager_add_dns_server_by_string(Manager *m, DnsServerType type, cons return dns_server_new(m, NULL, type, NULL, family, &address, port, ifindex, server_name); } -int manager_parse_dns_server_string_and_warn(Manager *m, DnsServerType type, const char *string) { +int manager_parse_dns_server_string_and_warn(Manager *m, DnsServerType type, const char *string, bool ignore_self_quietly) { int r; assert(m); @@ -70,7 +71,10 @@ int manager_parse_dns_server_string_and_warn(Manager *m, DnsServerType type, con return r; r = manager_add_dns_server_by_string(m, type, word); - if (r < 0) + if (r == -ELOOP) + log_full(ignore_self_quietly ? LOG_DEBUG : LOG_INFO, + "DNS server string '%s' points to our own listener, ignoring.", word); + else if (r < 0) log_warning_errno(r, "Failed to add DNS server address '%s', ignoring: %m", word); } } @@ -151,7 +155,7 @@ int config_parse_dns_servers( dns_server_unlink_all(manager_get_first_dns_server(m, ltype)); else { /* Otherwise, add to the list */ - r = manager_parse_dns_server_string_and_warn(m, ltype, rvalue); + r = manager_parse_dns_server_string_and_warn(m, ltype, rvalue, false); if (r < 0) { log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to parse DNS server string '%s', ignoring.", rvalue); @@ -159,8 +163,7 @@ int config_parse_dns_servers( } } - /* If we have a manual setting, then we stop reading - * /etc/resolv.conf */ + /* If we have a manual setting, then we stop reading /etc/resolv.conf */ if (ltype == DNS_SERVER_SYSTEM) m->read_resolv_conf = false; if (ltype == DNS_SERVER_FALLBACK) @@ -202,8 +205,7 @@ int config_parse_search_domains( } } - /* If we have a manual setting, then we stop reading - * /etc/resolv.conf */ + /* If we have a manual setting, then we stop reading /etc/resolv.conf */ m->read_resolv_conf = false; return 0; @@ -485,7 +487,7 @@ int manager_parse_config_file(Manager *m) { return r; if (m->need_builtin_fallbacks) { - r = manager_parse_dns_server_string_and_warn(m, DNS_SERVER_FALLBACK, DNS_SERVERS); + r = manager_parse_dns_server_string_and_warn(m, DNS_SERVER_FALLBACK, DNS_SERVERS, false); if (r < 0) return r; } diff --git a/src/resolve/resolved-conf.h b/src/resolve/resolved-conf.h index 07ce2591a9..4639cefbd8 100644 --- a/src/resolve/resolved-conf.h +++ b/src/resolve/resolved-conf.h @@ -8,7 +8,7 @@ int manager_parse_config_file(Manager *m); int manager_parse_search_domains_and_warn(Manager *m, const char *string); -int manager_parse_dns_server_string_and_warn(Manager *m, DnsServerType type, const char *string); +int manager_parse_dns_server_string_and_warn(Manager *m, DnsServerType type, const char *string, bool ignore_self_quietly); const struct ConfigPerfItem* resolved_gperf_lookup(const char *key, GPERF_LEN_TYPE length); const struct ConfigPerfItem* resolved_dnssd_gperf_lookup(const char *key, GPERF_LEN_TYPE length); diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index 6b32ee4cf0..223ef36691 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -1620,30 +1620,37 @@ bool manager_next_dnssd_names(Manager *m) { return tried; } -bool manager_server_is_stub(Manager *m, DnsServer *s) { +bool manager_server_address_is_stub(Manager *m, int family, const union in_addr_union *address, uint16_t port) { DnsStubListenerExtra *l; assert(m); - assert(s); + assert(address); /* Safety check: we generally already skip the main stub when parsing configuration. But let's be * extra careful, and check here again */ - if (s->family == AF_INET && - s->address.in.s_addr == htobe32(INADDR_DNS_STUB) && - dns_server_port(s) == 53) + if (family == AF_INET && + address->in.s_addr == htobe32(INADDR_DNS_STUB) && + port == 53) return true; /* Main reason to call this is to check server data against the extra listeners, and filter things * out. */ ORDERED_SET_FOREACH(l, m->dns_extra_stub_listeners) - if (s->family == l->family && - in_addr_equal(s->family, &s->address, &l->address) && - dns_server_port(s) == dns_stub_listener_extra_port(l)) + if (family == l->family && + in_addr_equal(family, address, &l->address) && + port == dns_stub_listener_extra_port(l)) return true; return false; } +bool manager_server_is_stub(Manager *m, DnsServer *s) { + assert(m); + assert(s); + + return manager_server_address_is_stub(m, s->family, &s->address, dns_server_port(s)); +} + int socket_disable_pmtud(int fd, int af) { int r; diff --git a/src/resolve/resolved-manager.h b/src/resolve/resolved-manager.h index 35e0068a83..e969738978 100644 --- a/src/resolve/resolved-manager.h +++ b/src/resolve/resolved-manager.h @@ -207,6 +207,7 @@ void manager_cleanup_saved_user(Manager *m); bool manager_next_dnssd_names(Manager *m); +bool manager_server_address_is_stub(Manager *m, int family, const union in_addr_union *address, uint16_t port); bool manager_server_is_stub(Manager *m, DnsServer *s); int socket_disable_pmtud(int fd, int af); diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index 100894d6b2..e9785ab964 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -143,7 +143,8 @@ int manager_read_resolv_conf(Manager *m) { a = first_word(l, "nameserver"); if (a) { - r = manager_parse_dns_server_string_and_warn(m, DNS_SERVER_SYSTEM, a); + r = manager_parse_dns_server_string_and_warn(m, DNS_SERVER_SYSTEM, a, + true /* don't warn about loops to our own stub listeners */); if (r < 0) log_warning_errno(r, "Failed to parse DNS server address '%s', ignoring.", a);