SymbX/systemd
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

dns-packet: keep unreferenceable names out of the compression map

Browse Source 
dns_packet_append_name() added every name it appends to the packet's
compression map, including names written at offsets >= 0x4000 that an
RFC 1035 compression pointer (14 bits) can never reference. Such
entries are not just useless -- when the same name occurs again, the
lookup finds it, the pointer cannot be emitted, the code falls through
to appending the labels again, and the re-insertion into the map then
fails the whole packet build with hashmap_ensure_put() returning
-EEXIST.

This is easily triggered with mDNS: an unsolicited DNS-SD announcement
covering a zone with a few hundred services exceeds 16KiB, and
dns_scope_announce() then fails with "Failed to build reply packet:
File exists".

This lifts the failure only: dns_scope_announce() still builds the whole
zone into one packet and hands it to the socket as a single datagram,
fragmented as the link requires. Splitting announcements across
MTU-sized packets, as RFC 6762 section 17 asks, is done separately.

Only insert a name into the map if its offset is pointer-expressible,
making "mapped implies referenceable" a map invariant. Re-occurrences of
out-of-range names are emitted as labels again -- the only encoding RFC
1035 section 4.1.4 permits in that case -- still compressed from the
first suffix that is mapped in range.

The lookup restates that range as control flow rather than asserting it.
Under -Db_ndebug=true, which CI builds, assert() is
__builtin_unreachable(), so an out-of-range entry would put 0xC000 | n
truncated to 14 bits on the wire -- a pointer to an arbitrary earlier
offset -- where falling through emits the labels instead. This layer is
shared with unicast DNS, DoT and LLMNR.

The pointer's tag bits and its offset bound are now named once, in
dns-def.h next to the other wire-format constants rather than in
dns-packet.h, since the format is RFC 1035's and not this packet
abstraction's -- dhcp-message.c open-codes the same two bits to parse
compressed names out of DHCP options. Compile-time checks pin that the
two partition the 16-bit field and that the tag is its top two bits,
which is what lets the reader take it from the first octet alone.

The offset's other bound becomes control flow along with it: it was an
assert(), which under -Db_ndebug=true is no check at all, for the bound
whose violation puts a pointer to the wrong bytes on the wire. It is
also tightened to the start of the name being appended rather than the
end of the packet, which is the "prior occurrence" RFC 1035 section
4.1.4 allows a pointer to address and what dns_packet_read_name()
enforces on the way in.

Add regression tests, including one through dns_packet_append_rr() --
the path the reported failure came from, where the name append is nested
inside the rdlength backpatch and two further rollbacks -- and one for
the reader's label-type dispatch, which the renamed constants touch.
This commit is contained in:
Christian Glombek
2026-09-07 04:26:33 +02:00
parent f789245918
commit a95358da6f
3 changed files with 192 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/basic/dns-def.h
+10
View File
@@ -17,3 +17,13 @@
/* Maximum number of labels per valid hostname */
#define DNS_N_LABELS_MAX 127
/* RFC 1035 § 4.1.4 name compression: the two high bits tag a 16-bit field as a pointer, the
* remaining 14 bits carry the offset, which is hence also the last position a name a pointer can
* reference may start at. */
#define DNS_COMPRESSION_POINTER_FLAG UINT16_C(0xC000)
#define DNS_COMPRESSION_OFFSET_MAX UINT16_C(0x3FFF)
assert_cc((DNS_COMPRESSION_POINTER_FLAG & DNS_COMPRESSION_OFFSET_MAX) == 0);
assert_cc((DNS_COMPRESSION_POINTER_FLAG | DNS_COMPRESSION_OFFSET_MAX) == UINT16_MAX);
/* The tag is the top two bits, which is what lets a reader take it from the first octet alone. */
assert_cc((DNS_COMPRESSION_POINTER_FLAG & UINT16_C(0xFF)) == 0);
src/resolve/test-dns-packet-append.c
+162
View File
@@ -1,12 +1,16 @@
/* SPDX-License-Identifier: LGPL-2.1-or-later */
#include "alloc-util.h"
#include "dns-answer.h"
#include "dns-def.h"
#include "dns-packet.h"
#include "dns-question.h"
#include "dns-rr.h"
#include "dns-type.h"
#include "hashmap.h"
#include "list.h"
#include "log.h"
#include "stdio-util.h"
#include "tests.h"
#define BIT_QR (1 << 7)
@@ -1298,4 +1302,162 @@ TEST(packet_append_key_name_too_long) {
ASSERT_EQ(packet->size, 12U);
}
/* Pad the packet to exactly `target` bytes, so the next append starts at a chosen offset. Opaque
* filler: nothing parses it, and it leaves the compression map empty. */
static void pad_packet_to(DnsPacket *p, size_t target) {
_cleanup_free_ void *filler = NULL;
size_t gap;
ASSERT_GT(target, p->size);
gap = target - p->size;
ASSERT_NOT_NULL(filler = malloc0(gap));
ASSERT_OK(dns_packet_append_blob(p, filler, gap, NULL));
ASSERT_EQ(p->size, target);
ASSERT_EQ(hashmap_size(p->names), 0u);
}
/* The reader's label-type dispatch. RFC 1035 section 4.1.4 gives a length octet the top bits 00 and
* a compression pointer 11, and reserves 01 and 10: it takes both bits to make a pointer. The
* reserved octet points at a real name here, so a dispatch accepting either bit follows it and
* parses that name; only rejecting the octet outright gives -EBADMSG. */
TEST(packet_read_name_rejects_reserved_label_types) {
uint8_t first;
FOREACH_ARGUMENT(first, 0x40, 0x80) {
_cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
_cleanup_free_ char *parsed = NULL;
const uint8_t reserved[] = { first, DNS_PACKET_HEADER_SIZE };
size_t at;
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0,
DNS_PACKET_SIZE_MAX));
ASSERT_OK(dns_packet_append_name(packet, "example.org",
/* allow_compression= */ false,
/* canonical_candidate= */ false,
NULL));
at = packet->size;
ASSERT_OK(dns_packet_append_blob(packet, reserved, sizeof(reserved), NULL));
dns_packet_rewind(packet, at);
ASSERT_ERROR(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true,
NULL), EBADMSG);
}
}
TEST(packet_append_name_beyond_compression_pointer_range) {
_cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
size_t start;
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX));
/* Fill the packet past the range a compression pointer can express (RFC 1035 pointers carry
* 14 bits, i.e. offsets up to 0x3fff) with unique names. */
for (size_t i = 0; packet->size <= DNS_COMPRESSION_OFFSET_MAX; i++) {
char name[64];
xsprintf(name, "filler%zu.example.com", i);
ASSERT_OK(dns_packet_append_name(packet, name,
/* allow_compression= */ true,
/* canonical_candidate= */ false,
NULL));
}
/* The first occurrence of this name lands beyond pointer range, so it must not be added to
* the compression map... */
size_t n_mapped = hashmap_size(packet->names);
ASSERT_OK(dns_packet_append_name(packet, "far.example.org",
/* allow_compression= */ true, /* canonical_candidate= */ false,
&start));
ASSERT_GT(start, (size_t) DNS_COMPRESSION_OFFSET_MAX);
ASSERT_EQ(hashmap_size(packet->names), n_mapped);
/* ...and since no pointer can reference it, later occurrences must be appended as labels
* again. This used to fail with -EEXIST when the name was reinserted into the map. */
size_t second;
ASSERT_OK(dns_packet_append_name(packet, "far.example.org",
/* allow_compression= */ true, /* canonical_candidate= */ false,
&second));
ASSERT_OK(dns_packet_append_name(packet, "far.example.org",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL));
ASSERT_EQ(hashmap_size(packet->names), n_mapped);
/* Names first seen within pointer range still compress: re-appending one takes exactly the
* two bytes of a compression pointer. */
size_t before = packet->size;
ASSERT_OK(dns_packet_append_name(packet, "filler0.example.com",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL));
ASSERT_EQ(packet->size, before + 2);
/* Both encodings must round-trip through the parser: the label-form re-occurrence beyond
* pointer range and the pointer-form one within it. */
_cleanup_free_ char *parsed = NULL;
dns_packet_rewind(packet, second);
ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL));
ASSERT_STREQ(parsed, "far.example.org");
parsed = mfree(parsed);
dns_packet_rewind(packet, before);
ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL));
ASSERT_STREQ(parsed, "filler0.example.com");
}
/* The same case via dns_packet_append_rr(), where the name append is nested inside the rdlength
* backpatch -- the path the reported dns_scope_announce() failure came from. */
TEST(packet_append_rr_beyond_compression_pointer_range) {
_cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
const size_t n_records = 8;
size_t first_rr = 0;
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0, DNS_PACKET_SIZE_MAX));
/* Start past the range a pointer can express, so every name these records carry -- owner
* and rdata alike -- has its first occurrence out of reach. */
pad_packet_to(packet, DNS_COMPRESSION_OFFSET_MAX + 1);
/* Distinct owners, one shared PTR target: the target recurs in every record, out of pointer
* range, so each occurrence is re-emitted as labels and none is re-inserted into the map. */
for (size_t i = 0; i < n_records; i++) {
_cleanup_(dns_resource_record_unrefp) DnsResourceRecord *ptr = NULL;
char name[64];
size_t start;
xsprintf(name, "%zu.instance.example.com", i);
ptr = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_PTR, name);
ASSERT_NOT_NULL(ptr);
ptr->ttl = 120;
ASSERT_NOT_NULL(ptr->ptr.name = strdup("shared.target.example.org"));
ASSERT_OK(dns_packet_append_rr(packet, ptr, /* flags= */ 0,
&start, /* rdata_start= */ NULL));
if (i == 0)
first_rr = start;
}
/* Nothing was mapped: every name in this packet starts beyond the range a pointer can
* express, which is what makes the appends above the re-occurrence case. */
ASSERT_EQ(hashmap_size(packet->names), 0u);
/* And every record reads back with its rdata intact: the rdlength backpatch has to agree
* with a target written as labels rather than pointed at. (The packet is read from the
* first record rather than through dns_packet_extract(), since the padding in front of it
* is not a question section.) */
dns_packet_rewind(packet, first_rr);
for (size_t i = 0; i < n_records; i++) {
_cleanup_(dns_resource_record_unrefp) DnsResourceRecord *parsed = NULL;
char name[64];
ASSERT_OK(dns_packet_read_rr(packet, &parsed, /* ret_cache_flush= */ NULL,
/* start= */ NULL));
ASSERT_NOT_NULL(parsed);
xsprintf(name, "%zu.instance.example.com", i);
ASSERT_EQ(parsed->key->type, (uint16_t) DNS_TYPE_PTR);
ASSERT_STREQ(dns_resource_key_name(parsed->key), name);
ASSERT_STREQ(parsed->ptr.name, "shared.target.example.org");
}
}
DEFINE_TEST_MAIN(LOG_DEBUG)
src/shared/dns-packet.c
+20 -15
View File
@@ -632,31 +632,36 @@ int dns_packet_append_name(
while (!dns_name_is_root(name)) {
const char *z = name;
char label[DNS_LABEL_MAX+1];
size_t n = 0;
size_t n = 0, offset;
if (allow_compression)
n = PTR_TO_SIZE(hashmap_get(p->names, name));
if (n > 0) {
assert(n < p->size);
/* Only pointer-expressible offsets of already-appended labels enter the map below,
* so both bounds should hold. Control flow and not assert(), which is no check at
* all under -Db_ndebug=true, for bounds that decide what goes on the wire.
* saved_size, not p->size: RFC 1035 § 4.1.4 allows a pointer only to a prior
* occurrence, which is what dns_packet_read_name() enforces on the way in. */
if (n > 0 && n <= DNS_COMPRESSION_OFFSET_MAX && n < saved_size) {
r = dns_packet_append_uint16(p, DNS_COMPRESSION_POINTER_FLAG | n, NULL);
if (r < 0)
goto fail;
if (n < 0x4000) {
r = dns_packet_append_uint16(p, 0xC000 | n, NULL);
if (r < 0)
goto fail;
goto done;
}
goto done;
}
r = dns_label_unescape(&name, label, sizeof label, 0);
if (r < 0)
goto fail;
r = dns_packet_append_label(p, label, r, canonical_candidate, &n);
r = dns_packet_append_label(p, label, r, canonical_candidate, &offset);
if (r < 0)
goto fail;
if (allow_compression) {
/* Remember the name for compression -- but only if this occurrence sits within the
* 14 bits an RFC 1035 pointer can express. An offset beyond that can never be
* referenced, so it doesn't belong in the map: it would only collide with a later
* occurrence of the same name, needlessly failing the whole append with -EEXIST. */
if (allow_compression && offset <= DNS_COMPRESSION_OFFSET_MAX) {
_cleanup_free_ char *s = NULL;
if (!GREEDY_REALLOC(added_entries, n_added_entries + 1)) {
@@ -670,7 +675,7 @@ int dns_packet_append_name(
goto fail;
}
r = hashmap_ensure_put(&p->names, &dns_name_hash_ops, s, SIZE_TO_PTR(n));
r = hashmap_ensure_put(&p->names, &dns_name_hash_ops, s, SIZE_TO_PTR(offset));
if (r < 0)
goto fail;
@@ -1626,7 +1631,7 @@ int dns_packet_read_name(
return -EBADMSG;
continue;
} else if (allow_compression && FLAGS_SET(c, 0xc0)) {
} else if (allow_compression && FLAGS_SET(c, DNS_COMPRESSION_POINTER_FLAG >> 8)) {
uint16_t ptr;
/* Pointer */
@@ -1634,7 +1639,7 @@ int dns_packet_read_name(
if (r < 0)
return r;
ptr = (uint16_t) (c & ~0xc0) << 8 | (uint16_t) d;
ptr = (uint16_t) (c & ~(DNS_COMPRESSION_POINTER_FLAG >> 8)) << 8 | (uint16_t) d;
if (ptr < DNS_PACKET_HEADER_SIZE || ptr >= jump_barrier)
return -EBADMSG;