Merge pull request #23875 from yuwata/resolve-mdns-fix-use-after-free

resolve: mdns: fix use-after-free

1 files changed, 152 insertions, 99 deletions

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index 27685a90a9..47af540338 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -1551,6 +1551,33 @@ static int on_transaction_timeout(sd_event_source *s, usec_t usec, void *userdat
         return 0;
 }
 
+static int dns_transaction_setup_timeout(
+                DnsTransaction *t,
+                usec_t timeout_usec /* relative */,
+                usec_t next_usec /* CLOCK_BOOTTIME */) {
+
+        int r;
+
+        assert(t);
+
+        dns_transaction_stop_timeout(t);
+
+        r = sd_event_add_time_relative(
+                t->scope->manager->event,
+                &t->timeout_event_source,
+                CLOCK_BOOTTIME,
+                timeout_usec, 0,
+                on_transaction_timeout, t);
+        if (r < 0)
+                return r;
+
+        (void) sd_event_source_set_description(t->timeout_event_source, "dns-transaction-timeout");
+
+        t->next_attempt_after = next_usec;
+        t->state = DNS_TRANSACTION_PENDING;
+        return 0;
+}
+
 static usec_t transaction_get_resend_timeout(DnsTransaction *t) {
         assert(t);
         assert(t->scope);
@@ -1568,11 +1595,12 @@ static usec_t transaction_get_resend_timeout(DnsTransaction *t) {
                 return DNS_TIMEOUT_USEC;
 
         case DNS_PROTOCOL_MDNS:
-                assert(t->n_attempts > 0);
                 if (t->probing)
                         return MDNS_PROBING_INTERVAL_USEC;
-                else
-                        return (1 << (t->n_attempts - 1)) * USEC_PER_SEC;
+
+                /* See RFC 6762 Section 5.1 suggests that timeout should be a few seconds. */
+                assert(t->n_attempts > 0);
+                return (1 << (t->n_attempts - 1)) * USEC_PER_SEC;
 
         case DNS_PROTOCOL_LLMNR:
                 return t->scope->resend_timeout;
@@ -1622,7 +1650,7 @@ static int dns_transaction_prepare(DnsTransaction *t, usec_t ts) {
                 return 0;
         }
 
-        if (t->n_attempts >= dns_transaction_attempts_max(t->scope->protocol, t->probing)) {
+        if (t->n_attempts >= TRANSACTION_ATTEMPTS_MAX(t->scope->protocol)) {
                 DnsTransactionState result;
 
                 if (t->scope->protocol == DNS_PROTOCOL_LLMNR)
@@ -1752,13 +1780,30 @@ static int dns_transaction_prepare(DnsTransaction *t, usec_t ts) {
         return 1;
 }
 
+static int dns_packet_append_zone(DnsPacket *p, DnsTransaction *t, DnsResourceKey *k, unsigned *nscount) {
+        _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL;
+        bool tentative;
+        int r;
+
+        assert(p);
+        assert(t);
+        assert(k);
+
+        if (k->type != DNS_TYPE_ANY)
+                return 0;
+
+        r = dns_zone_lookup(&t->scope->zone, k, t->scope->link->ifindex, &answer, NULL, &tentative);
+        if (r < 0)
+                return r;
+
+        return dns_packet_append_answer(p, answer, nscount);
+}
+
 static int dns_transaction_make_packet_mdns(DnsTransaction *t) {
         _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
-        bool add_known_answers = false;
-        DnsResourceKey *tkey;
         _cleanup_set_free_ Set *keys = NULL;
-        unsigned qdcount;
-        unsigned nscount = 0;
+        unsigned qdcount, ancount = 0 /* avoid false maybe-uninitialized warning */, nscount;
+        bool add_known_answers = false;
         usec_t ts;
         int r;
 
@@ -1768,6 +1813,7 @@ static int dns_transaction_make_packet_mdns(DnsTransaction *t) {
         /* Discard any previously prepared packet, so we can start over and coalesce again */
         t->sent = dns_packet_unref(t->sent);
 
+        /* First, create a dummy packet to calculate packet size. */
         r = dns_packet_new_query(&p, t->scope->protocol, 0, false);
         if (r < 0)
                 return r;
@@ -1781,11 +1827,14 @@ static int dns_transaction_make_packet_mdns(DnsTransaction *t) {
         if (dns_key_is_shared(dns_transaction_key(t)))
                 add_known_answers = true;
 
-        if (dns_transaction_key(t)->type == DNS_TYPE_ANY) {
-                r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(t));
-                if (r < 0)
-                        return r;
-        }
+        r = dns_packet_append_zone(p, t, dns_transaction_key(t), NULL);
+        if (r < 0)
+                return r;
+
+        /* Save appended keys */
+        r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(t));
+        if (r < 0)
+                return r;
 
         /*
          * For mDNS, we want to coalesce as many open queries in pending transactions into one single
@@ -1795,90 +1844,114 @@ static int dns_transaction_make_packet_mdns(DnsTransaction *t) {
 
         assert_se(sd_event_now(t->scope->manager->event, CLOCK_BOOTTIME, &ts) >= 0);
 
-        LIST_FOREACH(transactions_by_scope, other, t->scope->transactions) {
-
-                /* Skip ourselves */
-                if (other == t)
-                        continue;
+        for (bool restart = true; restart;) {
+                restart = false;
+                LIST_FOREACH(transactions_by_scope, other, t->scope->transactions) {
+                        size_t saved_packet_size;
+                        bool append = false;
 
-                if (other->state != DNS_TRANSACTION_PENDING)
-                        continue;
-
-                if (other->next_attempt_after > ts)
-                        continue;
-
-                if (qdcount >= UINT16_MAX)
-                        break;
+                        /* Skip ourselves */
+                        if (other == t)
+                                continue;
 
-                r = dns_packet_append_key(p, dns_transaction_key(other), 0, NULL);
+                        if (other->state != DNS_TRANSACTION_PENDING)
+                                continue;
 
-                /*
-                 * If we can't stuff more questions into the packet, just give up.
-                 * One of the 'other' transactions will fire later and take care of the rest.
-                 */
-                if (r == -EMSGSIZE)
-                        break;
+                        if (other->next_attempt_after > ts)
+                                continue;
 
-                if (r < 0)
-                        return r;
+                        if (!set_contains(keys, dns_transaction_key(other))) {
+                                r = dns_packet_append_key(p, dns_transaction_key(other), 0, &saved_packet_size);
+                                /* If we can't stuff more questions into the packet, just give up.
+                                 * One of the 'other' transactions will fire later and take care of the rest. */
+                                if (r == -EMSGSIZE)
+                                        break;
+                                if (r < 0)
+                                        return r;
 
-                r = dns_transaction_prepare(other, ts);
-                if (r <= 0)
-                        continue;
+                                r = dns_packet_append_zone(p, t, dns_transaction_key(other), NULL);
+                                if (r == -EMSGSIZE)
+                                        break;
+                                if (r < 0)
+                                        return r;
 
-                ts += transaction_get_resend_timeout(other);
+                                append = true;
+                        }
 
-                r = sd_event_add_time(
-                                other->scope->manager->event,
-                                &other->timeout_event_source,
-                                CLOCK_BOOTTIME,
-                                ts, 0,
-                                on_transaction_timeout, other);
-                if (r < 0)
-                        return r;
+                        r = dns_transaction_prepare(other, ts);
+                        if (r < 0)
+                                return r;
+                        if (r == 0) {
+                                if (append)
+                                        dns_packet_truncate(p, saved_packet_size);
 
-                (void) sd_event_source_set_description(other->timeout_event_source, "dns-transaction-timeout");
+                                /* In this case, not only this transaction, but multiple transactions may be
+                                 * freed. Hence, we need to restart the loop. */
+                                restart = true;
+                                break;
+                        }
 
-                other->state = DNS_TRANSACTION_PENDING;
-                other->next_attempt_after = ts;
+                        usec_t timeout = transaction_get_resend_timeout(other);
+                        r = dns_transaction_setup_timeout(other, timeout, usec_add(ts, timeout));
+                        if (r < 0)
+                                return r;
 
-                qdcount++;
+                        if (dns_key_is_shared(dns_transaction_key(other)))
+                                add_known_answers = true;
 
-                if (dns_key_is_shared(dns_transaction_key(other)))
-                        add_known_answers = true;
+                        if (append) {
+                                r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(other));
+                                if (r < 0)
+                                        return r;
+                        }
 
-                if (dns_transaction_key(other)->type == DNS_TYPE_ANY) {
-                        r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(other));
-                        if (r < 0)
-                                return r;
+                        qdcount++;
+                        if (qdcount >= UINT16_MAX)
+                                break;
                 }
         }
 
-        DNS_PACKET_HEADER(p)->qdcount = htobe16(qdcount);
-
         /* Append known answer section if we're asking for any shared record */
         if (add_known_answers) {
-                r = dns_cache_export_shared_to_packet(&t->scope->cache, p);
+                r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, 0);
                 if (r < 0)
                         return r;
+
+                ancount = be16toh(DNS_PACKET_HEADER(p)->ancount);
         }
 
-        SET_FOREACH(tkey, keys) {
-                _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL;
-                bool tentative;
+        /* Then, create acctual packet. */
+        p = dns_packet_unref(p);
+        r = dns_packet_new_query(&p, t->scope->protocol, 0, false);
+        if (r < 0)
+                return r;
 
-                r = dns_zone_lookup(&t->scope->zone, tkey, t->scope->link->ifindex, &answer, NULL, &tentative);
+        /* Questions */
+        DnsResourceKey *k;
+        SET_FOREACH(k, keys) {
+                r = dns_packet_append_key(p, k, 0, NULL);
                 if (r < 0)
                         return r;
+        }
+        DNS_PACKET_HEADER(p)->qdcount = htobe16(qdcount);
 
-                r = dns_packet_append_answer(p, answer, &nscount);
+        /* Known answers */
+        if (add_known_answers) {
+                r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, ancount);
+                if (r < 0)
+                        return r;
+        }
+
+        /* Authorities */
+        nscount = 0;
+        SET_FOREACH(k, keys) {
+                r = dns_packet_append_zone(p, t, k, &nscount);
                 if (r < 0)
                         return r;
         }
         DNS_PACKET_HEADER(p)->nscount = htobe16(nscount);
 
         t->sent = TAKE_PTR(p);
-
         return 0;
 }
 
@@ -1950,45 +2023,36 @@ int dns_transaction_go(DnsTransaction *t) {
 
         if (!t->initial_jitter_scheduled &&
             IN_SET(t->scope->protocol, DNS_PROTOCOL_LLMNR, DNS_PROTOCOL_MDNS)) {
-                usec_t jitter, accuracy;
+                usec_t jitter;
 
-                /* RFC 4795 Section 2.7 suggests all queries should be delayed by a random time from 0 to
-                 * JITTER_INTERVAL. */
+                /* RFC 4795 Section 2.7 suggests all LLMNR queries should be delayed by a random time from 0 to
+                 * JITTER_INTERVAL.
+                 * RFC 6762 Section 8.1 suggests initial probe queries should be delayed by a random time from
+                 * 0 to 250ms. */
 
                 t->initial_jitter_scheduled = true;
+                t->n_attempts = 0;
 
                 switch (t->scope->protocol) {
 
                 case DNS_PROTOCOL_LLMNR:
                         jitter = random_u64_range(LLMNR_JITTER_INTERVAL_USEC);
-                        accuracy = LLMNR_JITTER_INTERVAL_USEC;
                         break;
 
                 case DNS_PROTOCOL_MDNS:
-                        jitter = usec_add(random_u64_range(MDNS_JITTER_RANGE_USEC), MDNS_JITTER_MIN_USEC);
-                        accuracy = MDNS_JITTER_RANGE_USEC;
+                        if (t->probing)
+                                jitter = random_u64_range(MDNS_PROBING_INTERVAL_USEC);
+                        else
+                                jitter = 0;
                         break;
                 default:
                         assert_not_reached();
                 }
 
-                assert(!t->timeout_event_source);
-
-                r = sd_event_add_time_relative(
-                                t->scope->manager->event,
-                                &t->timeout_event_source,
-                                CLOCK_BOOTTIME,
-                                jitter, accuracy,
-                                on_transaction_timeout, t);
+                r = dns_transaction_setup_timeout(t, jitter, ts);
                 if (r < 0)
                         return r;
 
-                (void) sd_event_source_set_description(t->timeout_event_source, "dns-transaction-timeout");
-
-                t->n_attempts = 0;
-                t->next_attempt_after = ts;
-                t->state = DNS_TRANSACTION_PENDING;
-
                 log_debug("Delaying %s transaction %" PRIu16 " for " USEC_FMT "us.",
                           dns_protocol_to_string(t->scope->protocol),
                           t->id,
@@ -2062,22 +2126,11 @@ int dns_transaction_go(DnsTransaction *t) {
                 return dns_transaction_go(t);
         }
 
-        ts += transaction_get_resend_timeout(t);
-
-        r = sd_event_add_time(
-                        t->scope->manager->event,
-                        &t->timeout_event_source,
-                        CLOCK_BOOTTIME,
-                        ts, 0,
-                        on_transaction_timeout, t);
+        usec_t timeout = transaction_get_resend_timeout(t);
+        r = dns_transaction_setup_timeout(t, timeout, usec_add(ts, timeout));
         if (r < 0)
                 return r;
 
-        (void) sd_event_source_set_description(t->timeout_event_source, "dns-transaction-timeout");
-
-        t->state = DNS_TRANSACTION_PENDING;
-        t->next_attempt_after = ts;
-
         return 1;
 }