diff --git a/src/basic/dns-def.h b/src/basic/dns-def.h index 5c0f031df7..67276bb699 100644 --- a/src/basic/dns-def.h +++ b/src/basic/dns-def.h @@ -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); diff --git a/src/resolve/test-dns-packet-append.c b/src/resolve/test-dns-packet-append.c index 862f3bea69..5bcace319b 100644 --- a/src/resolve/test-dns-packet-append.c +++ b/src/resolve/test-dns-packet-append.c @@ -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) diff --git a/src/shared/dns-packet.c b/src/shared/dns-packet.c index 429e5c82c1..bb3b8f9235 100644 --- a/src/shared/dns-packet.c +++ b/src/shared/dns-packet.c @@ -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;