Merge pull request #18588 from poettering/refuse-loops

resolved: try hard to never enter packet loops between resolved's stub and resolved's client

src/libsystemd/sd-bus/bus-common-errors.c

@@ -76,6 +76,7 @@ BUS_ERROR_MAP_ELF_REGISTER const sd_bus_error_map bus_common_errors[] = {
         SD_BUS_ERROR_MAP(BUS_ERROR_LINK_BUSY,                    EBUSY),
         SD_BUS_ERROR_MAP(BUS_ERROR_NETWORK_DOWN,                 ENETDOWN),
         SD_BUS_ERROR_MAP(BUS_ERROR_NO_SOURCE,                    ESRCH),
+        SD_BUS_ERROR_MAP(BUS_ERROR_STUB_LOOP,                    ELOOP),
         SD_BUS_ERROR_MAP(BUS_ERROR_NO_SUCH_DNSSD_SERVICE,        ENOENT),
         SD_BUS_ERROR_MAP(BUS_ERROR_DNSSD_SERVICE_EXISTS,         EEXIST),
 

src/libsystemd/sd-bus/bus-common-errors.h

@@ -74,6 +74,7 @@
 #define BUS_ERROR_LINK_BUSY                    "org.freedesktop.resolve1.LinkBusy"
 #define BUS_ERROR_NETWORK_DOWN                 "org.freedesktop.resolve1.NetworkDown"
 #define BUS_ERROR_NO_SOURCE                    "org.freedesktop.resolve1.NoSource"
+#define BUS_ERROR_STUB_LOOP                    "org.freedesktop.resolve1.StubLoop"
 #define BUS_ERROR_NO_SUCH_DNSSD_SERVICE        "org.freedesktop.resolve1.NoSuchDnssdService"
 #define BUS_ERROR_DNSSD_SERVICE_EXISTS         "org.freedesktop.resolve1.DnssdServiceExists"
 #define _BUS_ERROR_DNS                         "org.freedesktop.resolve1.DnsError."

src/resolve/resolved-bus.c

@@ -104,6 +104,9 @@ static int reply_query_state(DnsQuery *q) {
         case DNS_TRANSACTION_NO_SOURCE:
                 return sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_NO_SOURCE, "All suitable resolution sources turned off");
 
+        case DNS_TRANSACTION_STUB_LOOP:
+                return sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_STUB_LOOP, "Configured DNS server loops back to us");
+
         case DNS_TRANSACTION_RCODE_FAILURE: {
                 _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
 

src/resolve/resolved-dns-answer.c

@@ -740,16 +740,21 @@ int dns_answer_reserve(DnsAnswer **a, size_t n_free) {
                 if ((*a)->n_ref > 1)
                         return -EBUSY;
 
-                ns = (*a)->n_rrs + n_free;
-                if (ns > UINT16_MAX) /* Maximum number of RRs we can stick into a DNS packet section */
+                ns = (*a)->n_rrs;
+                assert(ns <= UINT16_MAX); /* Maximum number of RRs we can stick into a DNS packet section */
+
+                if (n_free > UINT16_MAX - ns) /* overflow check */
                         ns = UINT16_MAX;
+                else
+                        ns += n_free;
 
                 if ((*a)->n_allocated >= ns)
                         return 0;
 
-                /* Allocate more than we need */
-                ns *= 2;
-                if (ns > UINT16_MAX)
+                /* Allocate more than we need, but not more than UINT16_MAX */
+                if (ns <= UINT16_MAX/2)
+                        ns *= 2;
+                else
                         ns = UINT16_MAX;
 
                 /* This must be done before realloc() below. Otherwise, the original DnsAnswer object
@@ -780,33 +785,50 @@ int dns_answer_reserve(DnsAnswer **a, size_t n_free) {
 }
 
 int dns_answer_reserve_or_clone(DnsAnswer **a, size_t n_free) {
-        _cleanup_(dns_answer_unrefp) DnsAnswer *n = NULL;
         int r;
 
         assert(a);
 
-        /* Tries to extend the DnsAnswer object. And if that's not
-         * possible, since we are not the sole owner, then allocate a
-         * new, appropriately sized one. Either way, after this call
-         * the object will only have a single reference, and has room
-         * for at least the specified number of RRs. */
+        /* Tries to extend the DnsAnswer object. And if that's not possible, since we are not the sole owner,
+         * then allocate a new, appropriately sized one. Either way, after this call the object will only
+         * have a single reference, and has room for at least the specified number of RRs. */
 
-        r = dns_answer_reserve(a, n_free);
-        if (r != -EBUSY)
-                return r;
+        if (*a && (*a)->n_ref > 1) {
+                _cleanup_(dns_answer_unrefp) DnsAnswer *n = NULL;
+                size_t ns;
 
-        assert(*a);
+                ns = (*a)->n_rrs;
+                assert(ns <= UINT16_MAX); /* Maximum number of RRs we can stick into a DNS packet section */
 
-        n = dns_answer_new(((*a)->n_rrs + n_free) * 2);
-        if (!n)
-                return -ENOMEM;
+                if (n_free > UINT16_MAX - ns) /* overflow check */
+                        ns = UINT16_MAX;
+                else if (n_free > 0) { /* Increase size and double the result, just in case — except if the
+                                        * increase is specified as 0, in which case we just allocate the
+                                        * exact amount as before, under the assumption this is just a request
+                                        * to copy the answer. */
+                        ns += n_free;
 
-        r = dns_answer_add_raw_all(n, *a);
-        if (r < 0)
-                return r;
+                        if (ns <= UINT16_MAX/2) /* overflow check */
+                                ns *= 2;
+                        else
+                                ns = UINT16_MAX;
+                }
 
-        dns_answer_unref(*a);
-        *a = TAKE_PTR(n);
+                n = dns_answer_new(ns);
+                if (!n)
+                        return -ENOMEM;
+
+                r = dns_answer_add_raw_all(n, *a);
+                if (r < 0)
+                        return r;
+
+                dns_answer_unref(*a);
+                assert_se(*a = TAKE_PTR(n));
+        } else if (n_free > 0) {
+                r = dns_answer_reserve(a, n_free);
+                if (r < 0)
+                        return r;
+        }
 
         return 0;
 }

src/resolve/resolved-dns-packet.c

@@ -2555,6 +2555,10 @@ static int dns_packet_compare_func(const DnsPacket *x, const DnsPacket *y) {
 
 DEFINE_HASH_OPS(dns_packet_hash_ops, DnsPacket, dns_packet_hash_func, dns_packet_compare_func);
 
+bool dns_packet_equal(const DnsPacket *a, const DnsPacket *b) {
+        return dns_packet_compare_func(a, b) == 0;
+}
+
 static const char* const dns_rcode_table[_DNS_RCODE_MAX_DEFINED] = {
         [DNS_RCODE_SUCCESS] = "SUCCESS",
         [DNS_RCODE_FORMERR] = "FORMERR",

src/resolve/resolved-dns-packet.h

@@ -227,6 +227,8 @@ void dns_packet_rewind(DnsPacket *p, size_t idx);
 int dns_packet_skip_question(DnsPacket *p);
 int dns_packet_extract(DnsPacket *p);
 
+bool dns_packet_equal(const DnsPacket *a, const DnsPacket *b);
+
 /* https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-6 */
 enum {
         DNS_RCODE_SUCCESS = 0,

src/resolve/resolved-dns-scope.c

@@ -1126,7 +1126,7 @@ void dns_scope_check_conflicts(DnsScope *scope, DnsPacket *p) {
                         return;
         }
 
-        if (manager_our_packet(scope->manager, p))
+        if (manager_packet_from_local_address(scope->manager, p))
                 return;
 
         r = dns_packet_extract(p);

src/resolve/resolved-dns-stub.c

@@ -85,6 +85,15 @@ DnsStubListenerExtra *dns_stub_listener_extra_free(DnsStubListenerExtra *p) {
         return mfree(p);
 }
 
+uint16_t dns_stub_listener_extra_port(DnsStubListenerExtra *p) {
+        assert(p);
+
+        if (p->port > 0)
+                return p->port;
+
+        return 53;
+}
+
 static int dns_stub_collect_answer_by_question(
                 DnsAnswer **reply,
                 DnsAnswer *answer,
@@ -639,6 +648,7 @@ static void dns_stub_query_complete(DnsQuery *q) {
         case DNS_TRANSACTION_RR_TYPE_UNSUPPORTED:
         case DNS_TRANSACTION_NETWORK_DOWN:
         case DNS_TRANSACTION_NO_SOURCE:
+        case DNS_TRANSACTION_STUB_LOOP:
                 (void) dns_stub_send_reply(q, DNS_RCODE_SERVFAIL);
                 break;
 
@@ -688,6 +698,11 @@ static void dns_stub_process_query(Manager *m, DnsStubListenerExtra *l, DnsStrea
                 return;
         }
 
+        if (manager_packet_from_our_transaction(m, p)) {
+                log_debug("Got our own packet looped back, ignoring.");
+                return;
+        }
+
         r = dns_packet_extract(p);
         if (r < 0) {
                 log_debug_errno(r, "Failed to extract resources from incoming packet, ignoring packet: %m");
@@ -951,13 +966,13 @@ static int manager_dns_stub_fd_extra(Manager *m, DnsStubListenerExtra *l, int ty
         if (l->family == AF_INET)
                 sa = (union sockaddr_union) {
                         .in.sin_family = l->family,
-                        .in.sin_port = htobe16(l->port != 0 ? l->port : 53U),
+                        .in.sin_port = htobe16(dns_stub_listener_extra_port(l)),
                         .in.sin_addr = l->address.in,
                 };
         else
                 sa = (union sockaddr_union) {
                         .in6.sin6_family = l->family,
-                        .in6.sin6_port = htobe16(l->port != 0 ? l->port : 53U),
+                        .in6.sin6_port = htobe16(dns_stub_listener_extra_port(l)),
                         .in6.sin6_addr = l->address.in6,
                 };
 

src/resolve/resolved-dns-stub.h

@@ -33,6 +33,7 @@ extern const struct hash_ops dns_stub_listener_extra_hash_ops;
 
 int dns_stub_listener_extra_new(Manager *m, DnsStubListenerExtra **ret);
 DnsStubListenerExtra *dns_stub_listener_extra_free(DnsStubListenerExtra *p);
+uint16_t dns_stub_listener_extra_port(DnsStubListenerExtra *p);
 
 void manager_dns_stub_stop(Manager *m);
 int manager_dns_stub_start(Manager *m);

src/resolve/resolved-dns-transaction.c

@@ -317,8 +317,9 @@ static void dns_transaction_tentative(DnsTransaction *t, DnsPacket *p) {
 
         assert(t);
         assert(p);
+        assert(t->scope->protocol == DNS_PROTOCOL_LLMNR);
 
-        if (manager_our_packet(t->scope->manager, p) != 0)
+        if (manager_packet_from_local_address(t->scope->manager, p) != 0)
                 return;
 
         (void) in_addr_to_string(p->family, &p->sender, &pretty);
@@ -639,6 +640,9 @@ static int dns_transaction_emit_tcp(DnsTransaction *t) {
                 if (r < 0)
                         return r;
 
+                if (manager_server_is_stub(t->scope->manager, t->server))
+                        return -ELOOP;
+
                 if (!t->bypass) {
                         if (!dns_server_dnssec_supported(t->server) && dns_type_is_dnssec(dns_transaction_key(t)->type))
                                 return -EOPNOTSUPP;
@@ -1303,6 +1307,9 @@ static int dns_transaction_emit_udp(DnsTransaction *t) {
                 if (r < 0)
                         return r;
 
+                if (manager_server_is_stub(t->scope->manager, t->server))
+                        return -ELOOP;
+
                 if (t->current_feature_level < DNS_SERVER_FEATURE_LEVEL_UDP || DNS_SERVER_FEATURE_LEVEL_IS_TLS(t->current_feature_level))
                         return -EAGAIN; /* Sorry, can't do UDP, try TCP! */
 
@@ -1845,7 +1852,23 @@ int dns_transaction_go(DnsTransaction *t) {
                 if (IN_SET(r, -EMSGSIZE, -EAGAIN))
                         r = dns_transaction_emit_tcp(t);
         }
+        if (r == -ELOOP) {
+                if (t->scope->protocol != DNS_PROTOCOL_DNS)
+                        return r;
 
+                /* One of our own stub listeners */
+                log_debug_errno(r, "Detected that specified DNS server is our own extra listener, switching DNS servers.");
+
+                dns_scope_next_dns_server(t->scope);
+
+                if (dns_scope_get_dns_server(t->scope) == t->server) {
+                        log_debug_errno(r, "Still pointing to extra listener after switching DNS servers, refusing operation.");
+                        dns_transaction_complete(t, DNS_TRANSACTION_STUB_LOOP);
+                        return 0;
+                }
+
+                return dns_transaction_go(t);
+        }
         if (r == -ESRCH) {
                 /* No servers to send this to? */
                 dns_transaction_complete(t, DNS_TRANSACTION_NO_SERVERS);
@@ -3386,6 +3409,7 @@ static const char* const dns_transaction_state_table[_DNS_TRANSACTION_STATE_MAX]
         [DNS_TRANSACTION_NETWORK_DOWN] = "network-down",
         [DNS_TRANSACTION_NOT_FOUND] = "not-found",
         [DNS_TRANSACTION_NO_SOURCE] = "no-source",
+        [DNS_TRANSACTION_STUB_LOOP] = "stub-loop",
 };
 DEFINE_STRING_TABLE_LOOKUP(dns_transaction_state, DnsTransactionState);
 

src/resolve/resolved-dns-transaction.h

@@ -33,6 +33,7 @@ enum DnsTransactionState {
         DNS_TRANSACTION_NETWORK_DOWN,
         DNS_TRANSACTION_NOT_FOUND, /* like NXDOMAIN, but when LLMNR/TCP connections fail */
         DNS_TRANSACTION_NO_SOURCE, /* All suitable DnsTransactionSource turned off */
+        DNS_TRANSACTION_STUB_LOOP,
         _DNS_TRANSACTION_STATE_MAX,
         _DNS_TRANSACTION_STATE_INVALID = -EINVAL,
 };

src/resolve/resolved-llmnr.c

@@ -83,7 +83,7 @@ static int on_llmnr_packet(sd_event_source *s, int fd, uint32_t revents, void *u
         if (r <= 0)
                 return r;
 
-        if (manager_our_packet(m, p))
+        if (manager_packet_from_local_address(m, p))
                 return 0;
 
         scope = manager_find_scope(m, p);

src/resolve/resolved-manager.c

@@ -1255,13 +1255,31 @@ LinkAddress* manager_find_link_address(Manager *m, int family, const union in_ad
         return NULL;
 }
 
-bool manager_our_packet(Manager *m, DnsPacket *p) {
+bool manager_packet_from_local_address(Manager *m, DnsPacket *p) {
         assert(m);
         assert(p);
 
+        /* Let's see if this packet comes from an IP address we have on any local interface */
+
         return !!manager_find_link_address(m, p->family, &p->sender);
 }
 
+bool manager_packet_from_our_transaction(Manager *m, DnsPacket *p) {
+        DnsTransaction *t;
+
+        assert(m);
+        assert(p);
+
+        /* Let's see if we have a transaction with a query message with the exact same binary contents as the
+         * one we just got. If so, it's almost definitely a packet loop of some kind. */
+
+        t = hashmap_get(m->dns_transactions, UINT_TO_PTR(DNS_PACKET_ID(p)));
+        if (!t)
+                return false;
+
+        return t->sent && dns_packet_equal(t->sent, p);
+}
+
 DnsScope* manager_find_scope(Manager *m, DnsPacket *p) {
         Link *l;
 
@@ -1600,3 +1618,27 @@ bool manager_next_dnssd_names(Manager *m) {
 
         return tried;
 }
+
+bool manager_server_is_stub(Manager *m, DnsServer *s) {
+        DnsStubListenerExtra *l;
+
+        assert(m);
+        assert(s);
+
+        /* 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)
+                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))
+                        return true;
+
+        return false;
+}

src/resolve/resolved-manager.h

@@ -165,7 +165,9 @@ LinkAddress* manager_find_link_address(Manager *m, int family, const union in_ad
 void manager_refresh_rrs(Manager *m);
 int manager_next_hostname(Manager *m);
 
-bool manager_our_packet(Manager *m, DnsPacket *p);
+bool manager_packet_from_local_address(Manager *m, DnsPacket *p);
+bool manager_packet_from_our_transaction(Manager *m, DnsPacket *p);
+
 DnsScope* manager_find_scope(Manager *m, DnsPacket *p);
 
 void manager_verify_all(Manager *m);
@@ -195,3 +197,5 @@ void manager_reset_server_features(Manager *m);
 void manager_cleanup_saved_user(Manager *m);
 
 bool manager_next_dnssd_names(Manager *m);
+
+bool manager_server_is_stub(Manager *m, DnsServer *s);

src/resolve/resolved-mdns.c

@@ -254,7 +254,7 @@ static int on_mdns_packet(sd_event_source *s, int fd, uint32_t revents, void *us
         if (r <= 0)
                 return r;
 
-        if (manager_our_packet(m, p))
+        if (manager_packet_from_local_address(m, p))
                 return 0;
 
         scope = manager_find_scope(m, p);

src/resolve/resolved-varlink.c

@@ -60,6 +60,9 @@ static int reply_query_state(DnsQuery *q) {
         case DNS_TRANSACTION_NO_SOURCE:
                 return varlink_error(q->varlink_request, "io.systemd.Resolve.NoSource", NULL);
 
+        case DNS_TRANSACTION_STUB_LOOP:
+                return varlink_error(q->varlink_request, "io.systemd.Resolve.StubLoop", NULL);
+
         case DNS_TRANSACTION_NOT_FOUND:
                 /* We return this as NXDOMAIN. This is only generated when a host doesn't implement LLMNR/TCP, and we
                  * thus quickly know that we cannot resolve an in-addr.arpa or ip6.arpa address. */