dns-packet: keep unreferenceable names out of the compression map (#43666)

Split out of https://github.com/systemd/systemd/pull/42983

## Problem
`dns_packet_append_name()` fails with `-EEXIST` once a name that was
first appended at an offset beyond RFC 1035's 14-bit compression-pointer
range (0x4000) occurs again in the same packet: the fall-through that
re-emits the labels trips `hashmap_ensure_put()` on the existing map
entry, and the whole packet build fails — reported by the caller as, for
example, "Failed to build reply packet: File exists".

The map is meant to hold only offsets a pointer can actually reference.
Names past that range were being inserted anyway, so they were dead
weight that could never be pointed at, and re-encountering one turned
into an error.

## Fix
Keep names at offsets ≥ 0x4000 out of the compression map entirely.
"Mapped implies referenceable" becomes an invariant of the map rather
than something callers have to reason about, and the `-EEXIST` collision
cannot arise. A re-occurrence of such a name is emitted as labels, still
compressed from the first suffix that *is* mapped in range — which is
the only encoding RFC 1035 section 4.1.4 permits in that case, since a
pointer must address ≤ 0x3FFF.

## Impact
Any packet large enough to push a name past 16KiB hits this — it is not
mDNS-specific, though that is where it was found: a DNS-SD announcement
covering a few hundred published services crosses the threshold easily,
and every record after it fails to encode.

## Test
`test-dns-packet-append` gains coverage for the boundary: a name first
written past the pointer range and then repeated, the same for a full
resource record, the rollback path when an append fails mid-name, and
the map invariant itself (asserted over the whole map, so a future
change that inserts an unreferenceable offset is caught rather than
silently reintroducing the bug).

## Testing status
Builds clean with `-Dwerror=true`; `meson test --suite resolve` passes
(19 ok, 1 skipped) at both commits.
This commit is contained in:
Luca Boccassi
2026-09-07 11:00:12 +01:00
committed by GitHub
3 changed files with 361 additions and 16 deletions
+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);
+330
View File
@@ -1,13 +1,18 @@
/* 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"
#include "unaligned.h"
#define BIT_QR (1 << 7)
#define BIT_AA (1 << 2)
@@ -1298,4 +1303,329 @@ TEST(packet_append_key_name_too_long) {
ASSERT_EQ(packet->size, 12U);
}
/* 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);
}
}
/* Every offset in the compression map must be pointer-expressible. */
static void assert_offsets_in_pointer_range(const DnsPacket *p) {
void *v;
HASHMAP_FOREACH(v, p->names)
ASSERT_LE(PTR_TO_SIZE(v), (size_t) DNS_COMPRESSION_OFFSET_MAX);
}
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);
assert_offsets_in_pointer_range(packet);
/* 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");
/* A name beyond the pointer range whose suffix IS mapped in range -- the shape every DNS-SD
* name hits via ".local" -- must still compress from that suffix: one label plus a two-byte
* pointer, on every occurrence, with the map unchanged. */
n_mapped = hashmap_size(packet->names);
for (size_t i = 0; i < 2; i++) {
size_t at, occurrence_start = packet->size;
ASSERT_OK(dns_packet_append_name(packet, "far.example.com",
/* allow_compression= */ true,
/* canonical_candidate= */ false,
&at));
ASSERT_GT(at, (size_t) DNS_COMPRESSION_OFFSET_MAX);
ASSERT_EQ(packet->size, occurrence_start + 1 + STRLEN("far") + 2);
ASSERT_EQ(hashmap_size(packet->names), n_mapped);
assert_offsets_in_pointer_range(packet);
/* The pointer must address the suffix's mapped prior occurrence, not just any
* in-range length octet (RFC 1035 section 4.1.4): compare it with the map. */
size_t suffix = PTR_TO_SIZE(hashmap_get(packet->names, "example.com"));
size_t pointer_at = occurrence_start + 1 + STRLEN("far");
ASSERT_GT(suffix, 0u);
ASSERT_LT(suffix, occurrence_start);
ASSERT_EQ(unaligned_read_be16(DNS_PACKET_DATA(packet) + pointer_at),
(uint16_t) (DNS_COMPRESSION_POINTER_FLAG | suffix));
parsed = mfree(parsed);
dns_packet_rewind(packet, at);
ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL));
ASSERT_STREQ(parsed, "far.example.com");
}
}
/* 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);
}
TEST(packet_append_name_at_compression_offset_max) {
_cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
size_t start;
/* A name whose first label starts at exactly the last pointer-expressible offset must be
* mapped and referenced by pointer... */
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX));
pad_packet_to(packet, DNS_COMPRESSION_OFFSET_MAX);
ASSERT_OK(dns_packet_append_name(packet, "edge.example.org",
/* allow_compression= */ true, /* canonical_candidate= */ false,
&start));
ASSERT_EQ(start, (size_t) DNS_COMPRESSION_OFFSET_MAX);
/* Only the whole name is mapped: its suffixes start past the bound. */
ASSERT_EQ(hashmap_size(packet->names), 1u);
size_t before = packet->size;
ASSERT_OK(dns_packet_append_name(packet, "edge.example.org",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL));
ASSERT_EQ(packet->size, before + 2);
/* The pointer's wire bytes, independent of the parser: offset 0x3FFF under the tag bits. */
ASSERT_EQ(DNS_PACKET_DATA(packet)[before], 0xFFu);
ASSERT_EQ(DNS_PACKET_DATA(packet)[before + 1], 0xFFu);
_cleanup_free_ char *parsed = NULL;
dns_packet_rewind(packet, before);
ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL));
ASSERT_STREQ(parsed, "edge.example.org");
/* The same two bytes must be refused, not followed, where compression is not allowed. */
parsed = mfree(parsed);
dns_packet_rewind(packet, before);
ASSERT_ERROR(dns_packet_read_name(packet, &parsed, /* allow_compression= */ false, NULL),
EBADMSG);
}
TEST(packet_append_name_past_compression_offset_max) {
_cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
_cleanup_free_ char *parsed = NULL;
size_t start, before;
/* One byte past the last pointer-expressible offset, a name stays out of the map and every
* occurrence is re-emitted as labels. */
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX));
pad_packet_to(packet, DNS_COMPRESSION_OFFSET_MAX + 1);
ASSERT_OK(dns_packet_append_name(packet, "past.example.org",
/* allow_compression= */ true, /* canonical_candidate= */ false,
&start));
ASSERT_EQ(start, (size_t) DNS_COMPRESSION_OFFSET_MAX + 1);
ASSERT_EQ(hashmap_size(packet->names), 0u);
before = packet->size;
ASSERT_OK(dns_packet_append_name(packet, "past.example.org",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL));
ASSERT_EQ(packet->size, before + STRLEN("past.example.org") + 2);
dns_packet_rewind(packet, before);
ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL));
ASSERT_STREQ(parsed, "past.example.org");
}
/* 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");
}
}
/* A packet with room for the labels but not the root byte, with one name and with two. */
TEST(packet_append_name_rolls_back_on_failure) {
_cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
size_t before;
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0,
/* max_size= */ DNS_PACKET_HEADER_SIZE + 4));
before = packet->size;
ASSERT_ERROR(dns_packet_append_name(packet, "abc",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL), EMSGSIZE);
/* Nothing of the half-written name is left on the wire... */
ASSERT_EQ(packet->size, before);
/* ...and, the point of the rollback, no compression entry either: the label was mapped
* before the root byte failed, and an entry surviving here would name an offset that no
* longer holds what it claims. */
ASSERT_EQ(hashmap_size(packet->names), 0u);
/* Positive control: one more byte of budget and the same name fits, so the shortfall above
* is the root byte and not a label -- the root byte being the append under test here. */
packet = dns_packet_unref(packet);
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0,
/* max_size= */ DNS_PACKET_HEADER_SIZE + 5));
ASSERT_OK(dns_packet_append_name(packet, "abc",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL));
/* Two labels, so more than one entry has to come back out: "a" and "b" fill the packet
* exactly and the root byte fails, with the map holding "a.b" and "b". */
packet = dns_packet_unref(packet);
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0,
/* max_size= */ DNS_PACKET_HEADER_SIZE + 4));
before = packet->size;
ASSERT_ERROR(dns_packet_append_name(packet, "a.b",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL), EMSGSIZE);
ASSERT_EQ(packet->size, before);
ASSERT_EQ(hashmap_size(packet->names), 0u);
/* Only this call's entries come out. The map owns the keys, so removing one an earlier call
* inserted would free a key the map still holds -- and a wholesale clear would satisfy the
* assertions above, which never have anything else in the map. */
packet = dns_packet_unref(packet);
ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0,
/* max_size= */ DNS_PACKET_HEADER_SIZE + 7));
ASSERT_OK(dns_packet_append_name(packet, "ab",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL));
ASSERT_EQ(hashmap_size(packet->names), 1u);
before = packet->size;
ASSERT_ERROR(dns_packet_append_name(packet, "cd",
/* allow_compression= */ true, /* canonical_candidate= */ false,
NULL), EMSGSIZE);
ASSERT_EQ(packet->size, before);
ASSERT_EQ(hashmap_size(packet->names), 1u);
ASSERT_EQ(PTR_TO_SIZE(hashmap_get(packet->names, "ab")), (size_t) DNS_PACKET_HEADER_SIZE);
}
DEFINE_TEST_MAIN(LOG_DEBUG)
+21 -16
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;
@@ -681,7 +686,7 @@ int dns_packet_append_name(
r = dns_packet_append_uint8(p, 0, NULL);
if (r < 0)
return r;
goto fail;
done:
if (start)
@@ -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;