diff options
| -rw-r--r-- | PLAN.md | 37 | ||||
| -rw-r--r-- | include/packeteer/l7/dns.hpp | 176 | ||||
| -rw-r--r-- | tests/test_dns.cpp | 128 |
3 files changed, 327 insertions, 14 deletions
@@ -627,3 +627,40 @@ None currently open. handshake afterward, and a repeat of the earlier real HTTP/3 capture against google.com confirmed genuine QUIC traffic still decodes correctly on UDP. +- DNS answer records: probably the single most-wanted thing a packet + analyzer shows that this one didn't yet - responses showed + ancount=N but never what a query actually resolved to. A, AAAA, and + CNAME rdata are rendered into readable text; every other type is + still walked correctly (name/type/ttl/rdlength all read and + bounds-checked, so parsing the rest of the message doesn't break) + but not rendered, the same "decode the common cases precisely rather + than guess at everything" pattern as TLS's cipher suite names. + Needed a second name reader alongside the existing question-only + read_dns_name(): real answer records almost always compress their + NAME field as a 2-byte pointer back to the question (RFC 1035 + 4.1.4), which the original reader deliberately rejects (a design + decision from when only the question was parsed, preserved as-is). + read_dns_name_following_pointers() actually follows them, bounded by + a maximum jump count rather than a backward-only check - a cycle + across several pointers pointing at each other would still loop + forever under "must point backward", but can't survive a hard cap on + jumps followed. AAAA is rendered with a plain, uncompressed + hex-group formatter local to dns.hpp rather than summarize.hpp's + RFC-5952-canonical ipv6_to_string, for the same circular-include + reason arp_summary/dhcp's yiaddr formatter are where they are: + correct, just not maximally compact. + Fuzzed the new pointer-chasing logic specifically before trusting it + (fuzz_dns, fuzz_summarize, ~2.4M and ~2.7M runs) - exactly the kind + of attacker-influenced-offset code this project's fuzzing exists + for, and the jump-bound is exactly the sort of thing worth confirming + can't be made to hang, not just reasoned about. Clean, no crashes or + timeouts either target. + Live-verified extensively against real DNS traffic on wlp1s0: a + direct query to 8.8.8.8 for example.com correctly resolved two real + A records; a query for www.github.com correctly showed a real CNAME + chain (-> github.com -> 20.87.245.0); and organic background DNS + traffic from this machine's own browser sessions incidentally + captured alongside it showed AAAA records (including one response + with 8 real IPv6 addresses, all correctly listed), and DNS RR type + 65 (HTTPS records, ancount=0 in these captures) correctly producing + no answers suffix since there was nothing to list. diff --git a/include/packeteer/l7/dns.hpp b/include/packeteer/l7/dns.hpp index 2626887..b122099 100644 --- a/include/packeteer/l7/dns.hpp +++ b/include/packeteer/l7/dns.hpp @@ -1,23 +1,30 @@ #pragma once #include <cstdint> +#include <cstdio> #include <optional> #include <span> #include <string> #include <utility> +#include <vector> #include "packeteer/byteio.hpp" #include "packeteer/l7/dissector.hpp" -// Hand-rolled DNS message parsing: header + the first question record. -// Answer/authority/additional records aren't decoded (not needed for a -// one-line summary), so name-compression pointers there are never -// followed - a pointer in the question section itself is rejected -// rather than chased, keeping this a pure forward scan with no risk of -// a pointer loop. +// Hand-rolled DNS message parsing: header, the first question record, +// and (for responses) answer records - enough to show what a query +// actually resolved to, not just that it resolved. Only A, AAAA, and +// CNAME rdata are decoded into readable text; other types are still +// walked over correctly (name, type, ttl, rdlength all read and +// bounds-checked) but their rdata isn't rendered, matching this +// project's pattern of decoding the common cases precisely rather +// than guessing at everything. namespace packeteer::net { inline constexpr std::uint16_t kDnsPort = 53; +inline constexpr std::uint16_t kDnsTypeA = 1; +inline constexpr std::uint16_t kDnsTypeCname = 5; +inline constexpr std::uint16_t kDnsTypeAaaa = 28; struct DnsHeader { std::uint16_t id; @@ -33,15 +40,33 @@ struct DnsQuestion { std::uint16_t qtype; }; +struct DnsAnswer { + std::string name; + std::uint16_t type; + std::uint32_t ttl; + // Rendered rdata for A/AAAA/CNAME only; nullopt for every other + // type (still correctly skipped over, just not rendered). + std::optional<std::string> rdata_text; +}; + struct DnsMessage { DnsHeader header; std::optional<DnsQuestion> question; // first question only + std::vector<DnsAnswer> answers; // up to kMaxAnswersDecoded }; -// Reads a (possibly multi-label) dotted name starting at offset. -// Returns the name and the offset just past it, or nullopt on -// truncation or a compression pointer (0xC0 prefix - valid in -// answer/authority records, not supported here). +// Bounds how many answer records get decoded into DnsMessage::answers +// - real responses rarely carry more than a handful; a message +// claiming far more either is unusual or is trying to make this do +// needless work for a one-line summary. +inline constexpr std::size_t kMaxAnswersDecoded = 16; + +// Reads a (possibly multi-label) dotted name starting at offset, for +// the question section specifically. Returns the name and the offset +// just past it, or nullopt on truncation or a compression pointer +// (0xC0 prefix) - real messages don't compress the question's own +// name (there's nothing earlier in the message to point back to), so +// rejecting one here is a correctness check, not a missing feature. inline std::optional<std::pair<std::string, std::size_t>> read_dns_name( std::span<const unsigned char> bytes, std::size_t offset) { std::string name; @@ -52,7 +77,7 @@ inline std::optional<std::pair<std::string, std::size_t>> read_dns_name( ++offset; break; } - if ((len & 0xC0) == 0xC0) return std::nullopt; // compression pointer: unsupported + if ((len & 0xC0) == 0xC0) return std::nullopt; // compression pointer: unsupported here ++offset; if (offset + len > bytes.size()) return std::nullopt; if (!name.empty()) name += '.'; @@ -62,6 +87,103 @@ inline std::optional<std::pair<std::string, std::size_t>> read_dns_name( return std::make_pair(std::move(name), offset); } +// Reads a name starting at offset, following compression pointers this +// time - needed for answer records, which almost always compress +// their NAME field as a 2-byte pointer back to the question (RFC 1035 +// 4.1.4). Bounded by a maximum jump count rather than just "pointers +// must point backward": a cycle across several pointers pointing at +// each other would still loop forever under a backward-only check, but +// can't survive a hard cap on how many jumps are followed. Returns the +// name and the offset just past *this record's own bytes* (i.e. past +// the pointer itself if one was used, not past wherever it pointed) -- +// that's what the caller needs to continue parsing the rest of the +// record. +inline std::optional<std::pair<std::string, std::size_t>> read_dns_name_following_pointers( + std::span<const unsigned char> bytes, std::size_t offset) { + std::string name; + std::size_t record_end = static_cast<std::size_t>(-1); // set once, on the first pointer/end + std::size_t pos = offset; + constexpr int kMaxJumps = 20; + int jumps = 0; + + while (true) { + if (pos >= bytes.size()) return std::nullopt; + std::uint8_t len = bytes[pos]; + + if (len == 0) { + ++pos; + if (record_end == static_cast<std::size_t>(-1)) record_end = pos; + break; + } + if ((len & 0xC0) == 0xC0) { + if (pos + 1 >= bytes.size()) return std::nullopt; + std::uint16_t pointer = + static_cast<std::uint16_t>(((len & 0x3F) << 8) | bytes[pos + 1]); + if (record_end == static_cast<std::size_t>(-1)) record_end = pos + 2; + if (++jumps > kMaxJumps) return std::nullopt; + if (pointer >= bytes.size()) return std::nullopt; + pos = pointer; + continue; + } + + ++pos; + if (pos + len > bytes.size()) return std::nullopt; + if (!name.empty()) name += '.'; + for (std::uint8_t i = 0; i < len; ++i) name += static_cast<char>(bytes[pos + i]); + pos += len; + } + + return std::make_pair(std::move(name), record_end); +} + +// A plain, uncompressed hex-group IPv6 formatter local to this file -- +// deliberately not the RFC 5952 canonical/zero-compressed form +// summarize.hpp's ipv6_to_string produces, since reaching that would +// mean dns.hpp depending on summarize.hpp, which already depends on +// dns.hpp (the same circular-include reasoning behind arp_summary +// living in summarize.hpp and dhcp.hpp's yiaddr formatter being local +// to it). Correct, just not maximally compact. +inline std::string dns_aaaa_to_string(std::span<const unsigned char> bytes16) { + char buf[40]; + std::snprintf(buf, sizeof(buf), "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x", + bytes16[0], bytes16[1], bytes16[2], bytes16[3], bytes16[4], bytes16[5], + bytes16[6], bytes16[7], bytes16[8], bytes16[9], bytes16[10], bytes16[11], + bytes16[12], bytes16[13], bytes16[14], bytes16[15]); + return buf; +} + +inline std::optional<std::pair<DnsAnswer, std::size_t>> read_dns_answer( + std::span<const unsigned char> bytes, std::size_t offset) { + auto name_result = read_dns_name_following_pointers(bytes, offset); + if (!name_result) return std::nullopt; + auto& [name, next] = *name_result; + + // TYPE(2) CLASS(2) TTL(4) RDLENGTH(2) + if (next + 10 > bytes.size()) return std::nullopt; + std::uint16_t type = read_be16(bytes, next); + std::uint32_t ttl = read_be32(bytes, next + 4); + std::uint16_t rdlength = read_be16(bytes, next + 8); + std::size_t rdata_start = next + 10; + if (rdata_start + rdlength > bytes.size()) return std::nullopt; + + DnsAnswer answer{name, type, ttl, std::nullopt}; + + if (type == kDnsTypeA && rdlength == 4) { + char buf[16]; + std::snprintf(buf, sizeof(buf), "%u.%u.%u.%u", bytes[rdata_start], bytes[rdata_start + 1], + bytes[rdata_start + 2], bytes[rdata_start + 3]); + answer.rdata_text = buf; + } else if (type == kDnsTypeAaaa && rdlength == 16) { + answer.rdata_text = dns_aaaa_to_string(bytes.subspan(rdata_start, 16)); + } else if (type == kDnsTypeCname) { + if (auto cname = read_dns_name_following_pointers(bytes, rdata_start)) { + answer.rdata_text = cname->first; + } + } + + return std::make_pair(std::move(answer), rdata_start + rdlength); +} + inline std::optional<DnsMessage> parse_dns(std::span<const unsigned char> bytes) { if (bytes.size() < 12) return std::nullopt; @@ -74,15 +196,32 @@ inline std::optional<DnsMessage> parse_dns(std::span<const unsigned char> bytes) header.qdcount = read_be16(bytes, 4); header.ancount = read_be16(bytes, 6); - DnsMessage msg{header, std::nullopt}; + DnsMessage msg{header, std::nullopt, {}}; + + std::size_t offset = 12; if (header.qdcount >= 1) { - if (auto result = read_dns_name(bytes, 12)) { + if (auto result = read_dns_name(bytes, offset)) { auto& [name, next_offset] = *result; if (next_offset + 4 <= bytes.size()) { - msg.question = DnsQuestion{std::move(name), read_be16(bytes, next_offset)}; + msg.question = DnsQuestion{name, read_be16(bytes, next_offset)}; + offset = next_offset + 4; // past QTYPE(2) + QCLASS(2) } } } + + // Answer records only get walked once the question parsed cleanly + // - without a reliable offset into the message, there's no safe + // place to start reading them from. + if (msg.question) { + std::size_t count = std::min<std::size_t>(header.ancount, kMaxAnswersDecoded); + for (std::size_t i = 0; i < count; ++i) { + auto result = read_dns_answer(bytes, offset); + if (!result) break; // malformed: stop, keep what was decoded so far + msg.answers.push_back(std::move(result->first)); + offset = result->second; + } + } + return msg; } @@ -101,6 +240,15 @@ public: if (msg->question) { out += " " + msg->question->name + " type=" + std::to_string(msg->question->qtype); } + if (!msg->answers.empty()) { + std::string rendered; + for (const auto& answer : msg->answers) { + if (!answer.rdata_text) continue; + if (!rendered.empty()) rendered += ","; + rendered += *answer.rdata_text; + } + if (!rendered.empty()) out += " -> " + rendered; + } return out; } }; diff --git a/tests/test_dns.cpp b/tests/test_dns.cpp index db5de56..45fb019 100644 --- a/tests/test_dns.cpp +++ b/tests/test_dns.cpp @@ -25,6 +25,55 @@ std::vector<unsigned char> example_com_query() { }; } +void append_be16(std::vector<unsigned char>& out, std::uint16_t v) { + out.push_back(static_cast<unsigned char>(v >> 8)); + out.push_back(static_cast<unsigned char>(v & 0xFF)); +} + +void append_be32(std::vector<unsigned char>& out, std::uint32_t v) { + out.push_back(static_cast<unsigned char>(v >> 24)); + out.push_back(static_cast<unsigned char>(v >> 16)); + out.push_back(static_cast<unsigned char>(v >> 8)); + out.push_back(static_cast<unsigned char>(v & 0xFF)); +} + +// Builds a real, well-formed DNS response for "example.com" A, with +// `answers` real resource records appended after the question - each +// using a compressed name pointer back to the question's name (offset +// 12, right after the header), exactly how real DNS servers answer, +// rather than repeating the literal name. +struct AnswerSpec { + std::uint16_t type; + std::uint32_t ttl; + std::vector<unsigned char> rdata; +}; + +std::vector<unsigned char> build_dns_response(const std::vector<AnswerSpec>& answers) { + std::vector<unsigned char> bytes = { + 0x12, 0x9d, + 0x81, 0x80, // flags: QR=1 (response), RD=1, RA=1 + }; + append_be16(bytes, 1); // qdcount + append_be16(bytes, static_cast<std::uint16_t>(answers.size())); + append_be16(bytes, 0); // nscount + append_be16(bytes, 0); // arcount + + bytes.insert(bytes.end(), {7, 'e', 'x', 'a', 'm', 'p', 'l', 'e', 3, 'c', 'o', 'm', 0}); + append_be16(bytes, 1); // qtype A + append_be16(bytes, 1); // qclass IN + + for (const auto& answer : answers) { + bytes.push_back(0xC0); + bytes.push_back(0x0C); // NAME: pointer to offset 12 (the question's name) + append_be16(bytes, answer.type); + append_be16(bytes, 1); // CLASS: IN + append_be32(bytes, answer.ttl); + append_be16(bytes, static_cast<std::uint16_t>(answer.rdata.size())); + bytes.insert(bytes.end(), answer.rdata.begin(), answer.rdata.end()); + } + return bytes; +} + } // namespace TEST_CASE("parse_dns decodes a query") { @@ -80,3 +129,82 @@ TEST_CASE("DnsDissector::summarize returns nullopt for a truncated payload") { std::vector<unsigned char> bytes(5, 0); CHECK_FALSE(dissector.summarize(bytes).has_value()); } + +TEST_CASE("read_dns_name_following_pointers follows a compressed name back to the question") { + auto bytes = build_dns_response({{kDnsTypeA, 300, {93, 184, 216, 34}}}); + // The answer's NAME field is the 2-byte pointer right after the + // question section (byte offset 12 + 17 = 29 in this layout). + auto result = read_dns_name_following_pointers(bytes, 29); + REQUIRE(result.has_value()); + CHECK(result->first == "example.com"); +} + +TEST_CASE("read_dns_name_following_pointers is bounded against a pointer cycle") { + // Two pointers pointing at each other - a backward-only check + // wouldn't catch this (pointer B points backward to A, which + // points forward to B), but the jump-count bound does. + std::vector<unsigned char> bytes = {0xC0, 0x02, 0xC0, 0x00}; + CHECK_FALSE(read_dns_name_following_pointers(bytes, 0).has_value()); +} + +TEST_CASE("parse_dns decodes a single A answer") { + auto bytes = build_dns_response({{kDnsTypeA, 300, {93, 184, 216, 34}}}); + auto msg = parse_dns(bytes); + REQUIRE(msg.has_value()); + REQUIRE(msg->answers.size() == 1); + CHECK(msg->answers[0].name == "example.com"); + CHECK(msg->answers[0].type == kDnsTypeA); + CHECK(msg->answers[0].ttl == 300); + REQUIRE(msg->answers[0].rdata_text.has_value()); + CHECK(*msg->answers[0].rdata_text == "93.184.216.34"); +} + +TEST_CASE("parse_dns decodes multiple answers, e.g. a CNAME followed by an A record") { + std::vector<unsigned char> cname_rdata = {3, 'w', 'w', 'w', 0xC0, 0x0C}; // "www" + pointer + auto bytes = build_dns_response( + {{kDnsTypeCname, 60, cname_rdata}, {kDnsTypeA, 300, {93, 184, 216, 34}}}); + auto msg = parse_dns(bytes); + REQUIRE(msg.has_value()); + REQUIRE(msg->answers.size() == 2); + REQUIRE(msg->answers[0].rdata_text.has_value()); + CHECK(*msg->answers[0].rdata_text == "www.example.com"); + REQUIRE(msg->answers[1].rdata_text.has_value()); + CHECK(*msg->answers[1].rdata_text == "93.184.216.34"); +} + +TEST_CASE("parse_dns decodes an AAAA answer") { + std::vector<unsigned char> aaaa_rdata = {0x20, 0x01, 0x0d, 0xb8, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 1}; + auto bytes = build_dns_response({{kDnsTypeAaaa, 300, aaaa_rdata}}); + auto msg = parse_dns(bytes); + REQUIRE(msg.has_value()); + REQUIRE(msg->answers.size() == 1); + REQUIRE(msg->answers[0].rdata_text.has_value()); + CHECK(*msg->answers[0].rdata_text == "2001:0db8:0000:0000:0000:0000:0000:0001"); +} + +TEST_CASE("parse_dns leaves rdata_text unset for an undecoded record type") { + auto bytes = build_dns_response({{15 /* MX */, 60, {0, 10, 4, 'm', 'a', 'i', 'l'}}}); + auto msg = parse_dns(bytes); + REQUIRE(msg.has_value()); + REQUIRE(msg->answers.size() == 1); + CHECK(msg->answers[0].type == 15); + CHECK_FALSE(msg->answers[0].rdata_text.has_value()); +} + +TEST_CASE("parse_dns stops decoding answers on the first malformed record") { + auto bytes = build_dns_response({{kDnsTypeA, 300, {93, 184, 216, 34}}}); + bytes.resize(bytes.size() - 2); // truncate the last answer's rdata + auto msg = parse_dns(bytes); + REQUIRE(msg.has_value()); + CHECK(msg->answers.empty()); + CHECK(msg->header.ancount == 1); // the header claim is preserved even though decode failed +} + +TEST_CASE("DnsDissector::summarize includes resolved addresses for a response") { + DnsDissector dissector; + auto bytes = build_dns_response({{kDnsTypeA, 300, {93, 184, 216, 34}}}); + auto summary = dissector.summarize(bytes); + REQUIRE(summary.has_value()); + CHECK(summary->find("-> 93.184.216.34") != std::string::npos); +} |