fix: escape DNS label text per RFC 1035 §5.1 (closes #36) (#54)

* fix: escape dots and special characters in DNS label text per RFC 1035 §5.1

Closes #36

read_qname was pushing raw label bytes directly into the output string,
producing ambiguous text for labels containing dots, backslashes, or
non-printable bytes. fanf2 spotted this on HN: wire format
`[8]exa.mple[3]com[0]` (two labels, first containing a literal 0x2E)
was rendered as `exa.mple.com`, indistinguishable from three labels.

Fix both sides of the text representation per RFC 1035 §5.1:

read_qname — when rendering wire bytes to text:
- literal `.` within a label → `\.`
- literal `\` → `\\`
- bytes outside 0x21..=0x7E → `\DDD` (3-digit decimal)
- printable ASCII passes through unchanged

write_qname — when parsing text back to wire:
- `\.` produces a literal 0x2E inside the current label (not a separator)
- `\\` produces a literal 0x5C
- `\DDD` produces the byte with that decimal value (0..=255)
- unescaped `.` still separates labels, empty labels still skipped
- rejects trailing `\`, short `\DD`, and `\DDD` > 255

Impact in practice is low — real-world domains don't contain dots in
labels — but it's a correctness bug in the wire format parser that
could cause round-trip failures with adversarial input. The parser is
the core of the project, so correctness bugs take priority over
practical impact.

Adds 16 unit tests in a new `#[cfg(test)] mod tests` block covering:
plain domain read/write, literal-dot escaping on both sides, backslash
escaping, non-printable + space decimal escapes, full round-trip
preservation, and the three rejection cases for malformed escapes.

Credit: fanf2 (https://news.ycombinator.com/item?id=47612321)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: stream label writes directly into buffer (review feedback)

The first cut of this fix delegated write_qname to a helper
(parse_escaped_labels) that built Vec<Vec<u8>> up-front, then iterated
to emit the wire bytes. On a plain-ASCII domain like "www.google.com"
that's ~4 heap allocations per write_qname call, and record.rs calls
write_qname ~6 times per response — so this PR would regress
bench_buffer_serialize by roughly 24 extra allocations per response
vs. main, where the old non-escaping code had zero.

Rewrite write_qname as a streaming byte-level loop that reserves the
length byte up-front, writes the label body directly into the buffer,
then backpatches the length via set(). Zero intermediate allocations
on the common path, and the 63-byte label cap is now checked
incrementally so oversized labels fail fast.

Byte-level scanning is safe for UTF-8 input: continuation bytes are
always in 0x80..=0xBF, so they can never collide with the ASCII `.`
(0x2E) or `\` (0x5C) that drive label splitting and escape parsing.

Also inline the \DDD rendering in read_qname to avoid the per-byte
format!() allocation on non-printable input. Plain-ASCII reads hit
the unchanged push(c as char) fast path, so the common case has zero
regression.

The parse_escaped_labels helper is deleted — no remaining callers.

All 158 tests pass, clippy + fmt clean. Collapses three review
findings (HIGH allocation regression, MEDIUM format! allocation,
MEDIUM .unwrap() after digit guard) in one pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: route dnssec::name_to_wire through write_qname for escape handling

Closes #55.

dnssec::name_to_wire was a parallel implementation of the old
write_qname's splitting loop — it iterated qname.split('.') and pushed
raw bytes. It predated and duplicated the buffer.rs logic, and it did
not understand RFC 1035 §5.1 text escapes. After the read_qname fix in
this PR, names that come out of read_qname can contain \., \\, or
\DDD sequences; feeding those back into the old name_to_wire would
split on the literal '.' inside a \. sequence and produce corrupt
RRSIG signed-data blobs.

The underlying bug predates this PR — the old read_qname was broken
too, so both sides of the DNSSEC canonical form pipeline were
silently wrong in the same way. Making read_qname correct exposed the
divergence, so it's fixed here in the same PR that introduced the
exposure.

Reimplement name_to_wire on top of BytePacketBuffer::write_qname:
reserve a scratch buffer, let write_qname handle the escape parsing
and length-byte framing, copy the emitted bytes into a Vec, then
walk the wire once more to lowercase label bodies (length bytes stay
untouched). Canonical form per RFC 4034 §6.2 requires the
lowercasing, and it has to happen post-escape-resolution — a
decimal escape like \065 produces 0x41 ('A'), which must be
lowercased to 'a' in the final wire bytes.

Call sites in build_signed_data, record_to_canonical_wire,
record_rdata_canonical, and nsec3_hash are unchanged — the public
signature stays the same, infallible Vec<u8> return.

Tests:
- name_to_wire_escaped_dot_in_label_is_not_a_separator — verifies
  the fanf2 example round-trips correctly through canonical form
- name_to_wire_decimal_escape_is_lowercased — verifies post-escape
  lowercasing (the subtle correctness requirement)
- existing name_to_wire_root, name_to_wire_domain, ds_verification
  tests still pass unchanged

Test count: 158 → 160.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: tighten name_to_wire per review feedback

- Replace hand-rolled per-byte lowercase loop with stdlib
  [u8]::make_ascii_lowercase(). Shorter and idiomatic.
- Tighten the .expect() message to state the actual invariant
  (parseable DNS name) instead of vague "well-formed" language.
- Replace the doc comment's "see #55" with the real invariant —
  issue numbers rot, and by merge time #55 is closed anyway. The
  comment now explains WHY the lowercase pass has to happen
  post-escape-resolution (\065 → 'A' → 'a') instead of during
  write_qname.
- Drop the redundant `\065` test comment (the one-liner version
  is enough with the assertion showing the transform).

No behavior change; 160 tests still pass, clippy + fmt clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: cover label cap and empty-label rollback; trim doc comments

Closes coverage gaps left by PR #54:

- write_rejects_label_over_63_bytes: pins the incremental 63-byte cap
  inside write_qname's inner loop (boundary at 63 vs 64).
- write_skips_empty_labels: pins the rollback branch (pos = len_pos)
  triggered by leading or consecutive dots.

Doc comments tightened:

- write_qname: drop the streaming-impl walkthrough and the escape-grammar
  restatement (already documented on read_qname).
- name_to_wire: drop the implementation explanation; keep the
  post-escape lowercasing rationale, which pins behavior a future
  refactor could silently break.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit was merged in pull request #54.
This commit is contained in:
Razvan Dimescu
2026-04-10 08:53:46 +03:00
committed by GitHub
parent f556b60ce4
commit e860731c01
2 changed files with 266 additions and 24 deletions

View File

@@ -5,6 +5,7 @@ use log::{debug, trace};
use ring::digest;
use ring::signature;
use crate::buffer::BytePacketBuffer;
use crate::cache::{DnsCache, DnssecStatus};
use crate::packet::DnsPacket;
use crate::question::QueryType;
@@ -720,22 +721,29 @@ pub fn verify_ds(ds: &DnsRecord, dnskey: &DnsRecord, owner: &str) -> bool {
// -- Canonical wire format --
/// Encode a DNS name in canonical wire form per RFC 4034 §6.2:
/// uncompressed, with ASCII letters lowercased.
///
/// Lowercasing happens *after* escape resolution because `\065` yields
/// `'A'`, which canonical form must convert to `'a'`.
pub fn name_to_wire(name: &str) -> Vec<u8> {
let mut wire = Vec::with_capacity(name.len() + 2);
if name == "." || name.is_empty() {
wire.push(0);
return wire;
}
for label in name.split('.') {
if label.is_empty() {
continue;
}
wire.push(label.len() as u8);
for &b in label.as_bytes() {
wire.push(b.to_ascii_lowercase());
let mut buf = BytePacketBuffer::new();
buf.write_qname(name)
.expect("name_to_wire: input must parse as a valid DNS name");
let mut wire = buf.filled().to_vec();
let mut i = 0;
while i < wire.len() {
let label_len = wire[i] as usize;
if label_len == 0 {
break;
}
i += 1;
let end = i + label_len;
wire[i..end].make_ascii_lowercase();
i = end;
}
wire.push(0);
wire
}
@@ -1475,6 +1483,23 @@ mod tests {
);
}
#[test]
fn name_to_wire_escaped_dot_in_label_is_not_a_separator() {
// `exa\.mple.com` is two labels: `exa.mple` (8 bytes including the 0x2E) and `com`.
let wire = name_to_wire("exa\\.mple.com");
assert_eq!(
wire,
vec![8, b'e', b'x', b'a', b'.', b'm', b'p', b'l', b'e', 3, b'c', b'o', b'm', 0]
);
}
#[test]
fn name_to_wire_decimal_escape_is_lowercased() {
// \065 = 'A', must become 'a' in canonical form.
let wire = name_to_wire("\\065bc.com");
assert_eq!(wire, vec![3, b'a', b'b', b'c', 3, b'c', b'o', b'm', 0]);
}
#[test]
fn parent_zone_cases() {
assert_eq!(parent_zone("example.com"), "com");