From 043d3928780b474c38c3d7d0f5a5ef785fd7bc10 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 8 Dec 2017 16:51:46 +0100 Subject: [PATCH 1/6] resolved: fix a minimal race, when reading /etc/resolv.conf The user might replace a foreign /etc/resolv.conf with a symlink to one of ours between the time we did stat() and open the file. Hence, let's check the fstat() data right after opening the file, a second time. --- src/resolve/resolved-resolv-conf.c | 36 +++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index b182f4319e..a84b0fde1c 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -32,9 +32,29 @@ #include "string-util.h" #include "strv.h" +static bool file_is_our_own(const struct stat *st) { + struct stat own1, own2; + + assert(st); + + /* Is it symlinked to our own file? */ + if (stat(PRIVATE_UPLINK_RESOLV_CONF, &own1) >= 0 && + st->st_dev == own1.st_dev && + st->st_ino == own1.st_ino) + return true; + + /* Is it symlinked to our own stub file? */ + if (stat(PRIVATE_STUB_RESOLV_CONF, &own2) >= 0 && + st->st_dev == own2.st_dev && + st->st_ino == own2.st_ino) + return true; + + return false; +} + int manager_read_resolv_conf(Manager *m) { _cleanup_fclose_ FILE *f = NULL; - struct stat st, own; + struct stat st; char line[LINE_MAX]; usec_t t; int r; @@ -61,16 +81,7 @@ int manager_read_resolv_conf(Manager *m) { if (t == m->resolv_conf_mtime) return 0; - /* Is it symlinked to our own file? */ - if (stat(PRIVATE_UPLINK_RESOLV_CONF, &own) >= 0 && - st.st_dev == own.st_dev && - st.st_ino == own.st_ino) - return 0; - - /* Is it symlinked to our own stub file? */ - if (stat(PRIVATE_STUB_RESOLV_CONF, &own) >= 0 && - st.st_dev == own.st_dev && - st.st_ino == own.st_ino) + if (file_is_our_own(&st)) return 0; f = fopen("/etc/resolv.conf", "re"); @@ -87,6 +98,9 @@ int manager_read_resolv_conf(Manager *m) { goto clear; } + if (file_is_our_own(&st)) + return 0; + dns_server_mark_all(m->dns_servers); dns_search_domain_mark_all(m->search_domains); From ace68cd7117869fdeb3b3c352ae5eca730111830 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 8 Dec 2017 16:53:36 +0100 Subject: [PATCH 2/6] resolved: store the mtime of the file we read Let's make sure we use the mtime of the current fstat() data, rather than the mtime of the old stat(), fixing a theoretical race. --- src/resolve/resolved-resolv-conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index a84b0fde1c..48105eef0c 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -56,7 +56,6 @@ int manager_read_resolv_conf(Manager *m) { _cleanup_fclose_ FILE *f = NULL; struct stat st; char line[LINE_MAX]; - usec_t t; int r; assert(m); @@ -77,8 +76,7 @@ int manager_read_resolv_conf(Manager *m) { } /* Have we already seen the file? */ - t = timespec_load(&st.st_mtim); - if (t == m->resolv_conf_mtime) + if (timespec_load(&st.st_mtim) == m->resolv_conf_mtime) return 0; if (file_is_our_own(&st)) @@ -131,7 +129,7 @@ int manager_read_resolv_conf(Manager *m) { } } - m->resolv_conf_mtime = t; + m->resolv_conf_mtime = timespec_load(&st.st_mtim); /* Flush out all servers and search domains that are still * marked. Those are then ones that didn't appear in the new From a50d79103f31ece8cf2d63472b6542a047f5b5f7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 8 Dec 2017 16:55:00 +0100 Subject: [PATCH 3/6] resolved: don't make defines needlessly public --- src/resolve/resolved-resolv-conf.c | 3 +++ src/resolve/resolved-resolv-conf.h | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index 48105eef0c..f681efd00d 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -32,6 +32,9 @@ #include "string-util.h" #include "strv.h" +#define PRIVATE_UPLINK_RESOLV_CONF "/run/systemd/resolve/resolv.conf" +#define PRIVATE_STUB_RESOLV_CONF "/run/systemd/resolve/stub-resolv.conf" + static bool file_is_our_own(const struct stat *st) { struct stat own1, own2; diff --git a/src/resolve/resolved-resolv-conf.h b/src/resolve/resolved-resolv-conf.h index 99b3ca88fb..ef34f8e09e 100644 --- a/src/resolve/resolved-resolv-conf.h +++ b/src/resolve/resolved-resolv-conf.h @@ -22,8 +22,5 @@ #include "resolved-manager.h" -#define PRIVATE_UPLINK_RESOLV_CONF "/run/systemd/resolve/resolv.conf" -#define PRIVATE_STUB_RESOLV_CONF "/run/systemd/resolve/stub-resolv.conf" - int manager_read_resolv_conf(Manager *m); int manager_write_resolv_conf(Manager *m); From b6de578d73bdef40124bb551abe7123923b4c33f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 8 Dec 2017 17:01:47 +0100 Subject: [PATCH 4/6] resolved: beef up logic to detect our own configuration files Let's also check for the static resolv.conf, so that we filter all three of our own files out. --- src/resolve/resolved-resolv-conf.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index f681efd00d..b7c43d6ec4 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -32,25 +32,33 @@ #include "string-util.h" #include "strv.h" +/* A resolv.conf file containing the DNS server and domain data we learnt from uplink, i.e. the full uplink data */ #define PRIVATE_UPLINK_RESOLV_CONF "/run/systemd/resolve/resolv.conf" + +/* A resolv.conf file containing the domain data we learnt from uplink, but our own DNS server address. */ #define PRIVATE_STUB_RESOLV_CONF "/run/systemd/resolve/stub-resolv.conf" +/* A static resolv.conf file containing no domains, but only our own DNS sever address */ +#define PRIVATE_STATIC_RESOLV_CONF ROOTLIBEXECDIR "/resolv.conf" + static bool file_is_our_own(const struct stat *st) { - struct stat own1, own2; + const char *path; assert(st); - /* Is it symlinked to our own file? */ - if (stat(PRIVATE_UPLINK_RESOLV_CONF, &own1) >= 0 && - st->st_dev == own1.st_dev && - st->st_ino == own1.st_ino) - return true; + FOREACH_STRING(path, + PRIVATE_UPLINK_RESOLV_CONF, + PRIVATE_STUB_RESOLV_CONF, + PRIVATE_STATIC_RESOLV_CONF) { - /* Is it symlinked to our own stub file? */ - if (stat(PRIVATE_STUB_RESOLV_CONF, &own2) >= 0 && - st->st_dev == own2.st_dev && - st->st_ino == own2.st_ino) - return true; + struct stat own; + + /* Is it symlinked to our own uplink file? */ + if (stat(path, &own) >= 0 && + st->st_dev == own.st_dev && + st->st_ino == own.st_ino) + return true; + } return false; } From d937ef74edc405c3d0a014724a94ddd9d3d25960 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 8 Dec 2017 17:19:27 +0100 Subject: [PATCH 5/6] resolved: synchronize introduction blurbs in all three resolv.conf files we provide Let's use the same wording and same contents in all three versions. --- src/resolve/resolv.conf | 14 +++++++++---- src/resolve/resolved-resolv-conf.c | 33 ++++++++++++++++++++++-------- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/resolve/resolv.conf b/src/resolve/resolv.conf index b8034d6829..34a6b9257f 100644 --- a/src/resolve/resolv.conf +++ b/src/resolve/resolv.conf @@ -1,11 +1,17 @@ -# This is a static resolv.conf file for connecting local clients to -# systemd-resolved via its DNS stub listener on 127.0.0.53. +# This file belongs to man:systemd-resolved(8). Do not edit. +# +# This is a static resolv.conf file for connecting local clients to the +# internal DNS stub resolver of systemd-resolved. This file lists no search +# domains. +# +# Run "systemd-resolve --status" to see details about the uplink DNS servers +# currently in use. # # Third party programs must not access this file directly, but only through the -# symlink at /etc/resolv.conf. To manage resolv.conf(5) in a different way, +# symlink at /etc/resolv.conf. To manage man:resolv.conf(5) in a different way, # replace this symlink by a static file or a different symlink. # -# See systemd-resolved.service(8) for details about the supported modes of +# See man:systemd-resolved.service(8) for details about the supported modes of # operation for /etc/resolv.conf. nameserver 127.0.0.53 diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index b7c43d6ec4..4ea7490ac9 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -239,14 +239,18 @@ static void write_resolv_conf_search( static int write_uplink_resolv_conf_contents(FILE *f, OrderedSet *dns, OrderedSet *domains) { Iterator i; - fputs_unlocked("# This file is managed by man:systemd-resolved(8). Do not edit.\n#\n" + fputs_unlocked("# This file is managed by man:systemd-resolved(8). Do not edit.\n" + "#\n" "# This is a dynamic resolv.conf file for connecting local clients directly to\n" - "# all known DNS servers.\n#\n" + "# all known uplink DNS servers. This file lists all configured search domains.\n" + "#\n" "# Third party programs must not access this file directly, but only through the\n" "# symlink at /etc/resolv.conf. To manage man:resolv.conf(5) in a different way,\n" - "# replace this symlink by a static file or a different symlink.\n#\n" + "# replace this symlink by a static file or a different symlink.\n" + "#\n" "# See man:systemd-resolved.service(8) for details about the supported modes of\n" - "# operation for /etc/resolv.conf.\n\n", f); + "# operation for /etc/resolv.conf.\n" + "\n", f); if (ordered_set_isempty(dns)) fputs_unlocked("# No DNS servers known.\n", f); @@ -265,10 +269,23 @@ static int write_uplink_resolv_conf_contents(FILE *f, OrderedSet *dns, OrderedSe } static int write_stub_resolv_conf_contents(FILE *f, OrderedSet *dns, OrderedSet *domains) { - fputs("# This file is managed by man:systemd-resolved(8). Do not edit.\n#\n" - "# 127.0.0.53 is the systemd-resolved stub resolver.\n" - "# run \"systemd-resolve --status\" to see details about the actual nameservers.\n\n" - "nameserver 127.0.0.53\n", f); + fputs_unlocked("# This file is managed by man:systemd-resolved(8). Do not edit.\n" + "#\n" + "# This is a dynamic resolv.conf file for connecting local clients to the\n" + "# internal DNS stub resolver of systemd-resolved. This file lists all\n" + "# configured search domains.\n" + "#\n" + "# Run \"systemd-resolve --status\" to see details about the uplink DNS servers\n" + "# currently in use.\n" + "#\n" + "# Third party programs must not access this file directly, but only through the\n" + "# symlink at /etc/resolv.conf. To manage man:resolv.conf(5) in a different way,\n" + "# replace this symlink by a static file or a different symlink.\n" + "#\n" + "# See man:systemd-resolved.service(8) for details about the supported modes of\n" + "# operation for /etc/resolv.conf.\n" + "\n" + "nameserver 127.0.0.53\n", f); if (!ordered_set_isempty(domains)) write_resolv_conf_search(domains, f); From 613dca46d22ca7d47fcbd78bfb4bca7f6541ebaf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 8 Dec 2017 17:21:42 +0100 Subject: [PATCH 6/6] resolved: tweak domain routing logic a bit This makes sure that a classic DNS scope that has no DNS servers assigned is never considered for routing requests to even if it has matching search/routing domains associated. This is inspired by #7544, where lookup requests are refused since a scope with no DNS server is configured. This change does not deliver what the reporter intended, but is generally useful in general, as it makes us mor robust to misconfiguration. --- src/resolve/resolved-dns-scope.c | 37 +++++++++++++++++--------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index a9071ee73e..0a121cdcdf 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -403,7 +403,6 @@ int dns_scope_socket_tcp(DnsScope *s, int family, const union in_addr_union *add DnsScopeMatch dns_scope_good_domain(DnsScope *s, int ifindex, uint64_t flags, const char *domain) { DnsSearchDomain *d; - DnsServer *dns_server; assert(s); assert(domain); @@ -436,24 +435,27 @@ DnsScopeMatch dns_scope_good_domain(DnsScope *s, int ifindex, uint64_t flags, co if (dns_name_endswith(domain, "invalid") > 0) return DNS_SCOPE_NO; - /* Always honour search domains for routing queries. Note that - * we return DNS_SCOPE_YES here, rather than just - * DNS_SCOPE_MAYBE, which means wildcard scopes won't be - * considered anymore. */ - LIST_FOREACH(domains, d, dns_scope_get_search_domains(s)) - if (dns_name_endswith(domain, d->name) > 0) - return DNS_SCOPE_YES; - - /* If the DNS server has route-only domains, don't send other requests - * to it. This would be a privacy violation, will most probably fail - * anyway, and adds unnecessary load. */ - dns_server = dns_scope_get_dns_server(s); - if (dns_server && dns_server_limited_domains(dns_server)) - return DNS_SCOPE_NO; - switch (s->protocol) { - case DNS_PROTOCOL_DNS: + case DNS_PROTOCOL_DNS: { + DnsServer *dns_server; + + /* Never route things to scopes that lack DNS servers */ + dns_server = dns_scope_get_dns_server(s); + if (!dns_server) + return DNS_SCOPE_NO; + + /* Always honour search domains for routing queries, except if this scope lacks DNS servers. Note that + * we return DNS_SCOPE_YES here, rather than just DNS_SCOPE_MAYBE, which means other wildcard scopes + * won't be considered anymore. */ + LIST_FOREACH(domains, d, dns_scope_get_search_domains(s)) + if (dns_name_endswith(domain, d->name) > 0) + return DNS_SCOPE_YES; + + /* If the DNS server has route-only domains, don't send other requests to it. This would be a privacy + * violation, will most probably fail anyway, and adds unnecessary load. */ + if (dns_server_limited_domains(dns_server)) + return DNS_SCOPE_NO; /* Exclude link-local IP ranges */ if (dns_name_endswith(domain, "254.169.in-addr.arpa") == 0 && @@ -468,6 +470,7 @@ DnsScopeMatch dns_scope_good_domain(DnsScope *s, int ifindex, uint64_t flags, co return DNS_SCOPE_MAYBE; return DNS_SCOPE_NO; + } case DNS_PROTOCOL_MDNS: if ((s->family == AF_INET && dns_name_endswith(domain, "in-addr.arpa") > 0) ||