From: Benjamin Peterson Date: Mon, 25 Sep 2023 14:23:27 +0000 (-0700) Subject: resolve: tolerate merging a zero-ttl RR and a nonzero-ttl RR if not mDNS X-Git-Tag: v255-rc1~361 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8ec951e8d5cdd3ad632b1cbd8bcbe21d68b17512;p=thirdparty%2Fsystemd.git resolve: tolerate merging a zero-ttl RR and a nonzero-ttl RR if not mDNS resolved rejected RRsets containing a RR with a zero TTL and a RR with a nonzero TTL. In practice—see the linked issues—, this case triggered when an AF_UNSPEC query to a CNAMEd domain returned a zero TTL for the CNAME on one address family and a nonzero TTL for the CNAME on the other address family. The zero-nonzero TTL check cites RFC 2181 § 5.2 in a comment. That section says DNS clients should reject any RRset containing differing TTLs, which the check only implements a very special case of. That the old behavior caused real-world false NXDOMAIN results is reason enough to completely ignore the RFC's recommendation. However, mDNS treats zero TTLs specially, so the error case needs to be kept for mDNS. Fixes https://github.com/systemd/systemd/issues/22177 Fixes https://github.com/systemd/systemd/issues/20617 Fixes https://github.com/systemd/systemd/issues/19118 --- diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index 3d42b0d000b..bf023a77e44 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -181,11 +181,23 @@ int dns_answer_add( exist = ordered_set_get(a->items, &tmp); if (exist) { - /* There's already an RR of the same RRset in place! Let's see if the TTLs more or less - * match. We don't really care if they match precisely, but we do care whether one is 0 and - * the other is not. See RFC 2181, Section 5.2. */ - if ((rr->ttl == 0) != (exist->rr->ttl == 0)) - return -EINVAL; + /* There's already an RR of the same RRset in place! Let's see if the TTLs more or + * less match. RFC 2181, Section 5.2 suggests clients should reject RRsets + * containing RRs with differing TTLs. We are more tolerant of this situation except + * if one RR has a zero TTL and the other a nonzero TTL. In mDNS, zero TTLs are + * special, so we must error in that case. */ + if ((rr->ttl == 0) != (exist->rr->ttl == 0)) { + if ((exist->flags | flags) & DNS_ANSWER_REFUSE_TTL_NO_MATCH) + return log_debug_errno( + SYNTHETIC_ERRNO(EINVAL), + "Refusing to merge RRs with zero TTL and non-zero TTL: %s vs. %s", + dns_resource_record_to_string(rr), + dns_resource_record_to_string(exist->rr)); + + log_debug("Merging RRs with zero TTL and non-zero TTL (not RFC 2181/5.2 compliant): %s vs. %s", + dns_resource_record_to_string(rr), + dns_resource_record_to_string(exist->rr)); + } /* Entry already exists, keep the entry with the higher TTL. */ if (rr->ttl > exist->rr->ttl) { diff --git a/src/resolve/resolved-dns-answer.h b/src/resolve/resolved-dns-answer.h index 93afea32d55..068803c6cb1 100644 --- a/src/resolve/resolved-dns-answer.h +++ b/src/resolve/resolved-dns-answer.h @@ -14,14 +14,15 @@ typedef struct DnsAnswerItem DnsAnswerItem; * Note that we usually encode the empty DnsAnswer object as a simple NULL. */ typedef enum DnsAnswerFlags { - DNS_ANSWER_AUTHENTICATED = 1 << 0, /* Item has been authenticated */ - DNS_ANSWER_CACHEABLE = 1 << 1, /* Item is subject to caching */ - DNS_ANSWER_SHARED_OWNER = 1 << 2, /* For mDNS: RRset may be owner by multiple peers */ - DNS_ANSWER_CACHE_FLUSH = 1 << 3, /* For mDNS: sets cache-flush bit in the rrclass of response records */ - DNS_ANSWER_GOODBYE = 1 << 4, /* For mDNS: item is subject to disappear */ - DNS_ANSWER_SECTION_ANSWER = 1 << 5, /* When parsing: RR originates from answer section */ - DNS_ANSWER_SECTION_AUTHORITY = 1 << 6, /* When parsing: RR originates from authority section */ - DNS_ANSWER_SECTION_ADDITIONAL = 1 << 7, /* When parsing: RR originates from additional section */ + DNS_ANSWER_AUTHENTICATED = 1 << 0, /* Item has been authenticated */ + DNS_ANSWER_CACHEABLE = 1 << 1, /* Item is subject to caching */ + DNS_ANSWER_SHARED_OWNER = 1 << 2, /* For mDNS: RRset may be owner by multiple peers */ + DNS_ANSWER_CACHE_FLUSH = 1 << 3, /* For mDNS: sets cache-flush bit in the rrclass of response records */ + DNS_ANSWER_GOODBYE = 1 << 4, /* For mDNS: item is subject to disappear */ + DNS_ANSWER_SECTION_ANSWER = 1 << 5, /* When parsing: RR originates from answer section */ + DNS_ANSWER_SECTION_AUTHORITY = 1 << 6, /* When parsing: RR originates from authority section */ + DNS_ANSWER_SECTION_ADDITIONAL = 1 << 7, /* When parsing: RR originates from additional section */ + DNS_ANSWER_REFUSE_TTL_NO_MATCH = 1 << 8, /* For mDNS; refuse to merge a zero TTL RR with a nonzero TTL RR */ DNS_ANSWER_MASK_SECTIONS = DNS_ANSWER_SECTION_ANSWER| DNS_ANSWER_SECTION_AUTHORITY| diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index d63760b7d1e..ecc75c6590b 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -2348,8 +2348,11 @@ static int dns_packet_extract_answer(DnsPacket *p, DnsAnswer **ret_answer) { } else { DnsAnswerFlags flags = 0; - if (p->protocol == DNS_PROTOCOL_MDNS && !cache_flush) - flags |= DNS_ANSWER_SHARED_OWNER; + if (p->protocol == DNS_PROTOCOL_MDNS) { + flags |= DNS_ANSWER_REFUSE_TTL_NO_MATCH; + if (!cache_flush) + flags |= DNS_ANSWER_SHARED_OWNER; + } /* According to RFC 4795, section 2.9. only the RRs from the Answer section shall be * cached. Hence mark only those RRs as cacheable by default, but not the ones from diff --git a/src/resolve/resolved-mdns.c b/src/resolve/resolved-mdns.c index d4a919fc60d..11e3b141253 100644 --- a/src/resolve/resolved-mdns.c +++ b/src/resolve/resolved-mdns.c @@ -314,7 +314,7 @@ static int mdns_scope_process_query(DnsScope *s, DnsPacket *p) { } DNS_ANSWER_FOREACH_ITEM(item, answer) { - DnsAnswerFlags flags = item->flags; + DnsAnswerFlags flags = item->flags | DNS_ANSWER_REFUSE_TTL_NO_MATCH; /* The cache-flush bit must not be set in legacy unicast responses. * See section 6.7 of RFC 6762. */ if (legacy_query)