From 9ea82edc072be52e099db638aa510b53b35f5f0e Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Mon, 18 Jul 2022 17:05:39 -0300 Subject: [PATCH] DNS message parsing fixes and cleanup Fixes: - tighten reserved name detection to not match fooloki.loki, but instead only match "foo.loki.loki" and "loki.loki" (and similar for reserved name "snode.loki"). - IPv6 PTR parsing was completely broken. - Added tests for the above two issues. Cleanups: - Eliminate llarp::dns::Name_t typedef for std::string - Use optional return instead of bool + output param - Use string_views; we were doing a *lot* of string substr's during parsing, each of which allocates a new string. - Use fmt instead of stringstream - Simplify IPv4 PTR parsing --- llarp/dns/message.cpp | 10 +-- llarp/dns/name.cpp | 123 ++++++++++++++++---------------- llarp/dns/name.hpp | 19 +++-- llarp/dns/question.cpp | 6 +- llarp/dns/question.hpp | 2 +- llarp/dns/rr.cpp | 12 ++-- llarp/dns/rr.hpp | 4 +- llarp/handlers/exit.cpp | 13 ++-- llarp/handlers/tun.cpp | 37 +++++----- test/dns/test_llarp_dns_dns.cpp | 43 ++++++++--- 10 files changed, 143 insertions(+), 126 deletions(-) diff --git a/llarp/dns/message.cpp b/llarp/dns/message.cpp index 4519e0457..97bc3707e 100644 --- a/llarp/dns/message.cpp +++ b/llarp/dns/message.cpp @@ -218,7 +218,7 @@ namespace llarp rec.ttl = ttl; std::array tmp = {{0}}; llarp_buffer_t buf(tmp); - if (EncodeName(&buf, name)) + if (EncodeNameTo(&buf, name)) { buf.sz = buf.cur - buf.base; rec.rData.resize(buf.sz); @@ -243,7 +243,7 @@ namespace llarp rec.ttl = ttl; std::array tmp = {{0}}; llarp_buffer_t buf(tmp); - if (EncodeName(&buf, name)) + if (EncodeNameTo(&buf, name)) { buf.sz = buf.cur - buf.base; rec.rData.resize(buf.sz); @@ -268,7 +268,7 @@ namespace llarp rec.ttl = ttl; std::array tmp = {{0}}; llarp_buffer_t buf(tmp); - if (EncodeName(&buf, name)) + if (EncodeNameTo(&buf, name)) { buf.sz = buf.cur - buf.base; rec.rData.resize(buf.sz); @@ -294,7 +294,7 @@ namespace llarp std::array tmp = {{0}}; llarp_buffer_t buf(tmp); buf.put_uint16(priority); - if (EncodeName(&buf, name)) + if (EncodeNameTo(&buf, name)) { buf.sz = buf.cur - buf.base; rec.rData.resize(buf.sz); @@ -346,7 +346,7 @@ namespace llarp target = srv.target; } - if (not EncodeName(&buf, target)) + if (not EncodeNameTo(&buf, target)) { AddNXReply(); return; diff --git a/llarp/dns/name.cpp b/llarp/dns/name.cpp index 6f81f4f4f..4d7363b1f 100644 --- a/llarp/dns/name.cpp +++ b/llarp/dns/name.cpp @@ -1,22 +1,20 @@ -#define __USE_MINGW_ANSI_STDIO 1 #include "name.hpp" #include #include #include - -#include -#include +#include namespace llarp { namespace dns { - bool - DecodeName(llarp_buffer_t* buf, Name_t& name, bool trimTrailingDot) + std::optional + DecodeName(llarp_buffer_t* buf, bool trimTrailingDot) { if (buf->size_left() < 1) - return false; - std::stringstream ss; + return std::nullopt; + auto result = std::make_optional(); + auto& name = *result; size_t l; do { @@ -25,31 +23,26 @@ namespace llarp if (l) { if (buf->size_left() < l) - return false; + return std::nullopt; - ss << Name_t((const char*)buf->cur, l); - ss << "."; + name.append((const char*)buf->cur, l); + name += '.'; } buf->cur = buf->cur + l; } while (l); - name = ss.str(); /// trim off last dot if (trimTrailingDot) - name = name.substr(0, name.find_last_of('.')); - return true; + name.pop_back(); + return result; } bool - EncodeName(llarp_buffer_t* buf, Name_t name) + EncodeNameTo(llarp_buffer_t* buf, std::string_view name) { - std::stringstream ss; - if (name.size() && name[name.size() - 1] == '.') - ss << name.substr(0, name.size() - 1); - else - ss << name; + if (name.size() && name.back() == '.') + name.remove_suffix(1); - std::string part; - while (std::getline(ss, part, '.')) + for (auto part : llarp::split(name, ".")) { size_t l = part.length(); if (l > 63) @@ -60,7 +53,7 @@ namespace llarp return false; if (l) { - memcpy(buf->cur, part.data(), l); + std::memcpy(buf->cur, part.data(), l); buf->cur += l; } else @@ -71,8 +64,8 @@ namespace llarp return true; } - bool - DecodePTR(Name_t name, huint128_t& ip) + std::optional + DecodePTR(std::string_view name) { bool isV6 = false; auto pos = name.find(".in-addr.arpa"); @@ -82,56 +75,62 @@ namespace llarp isV6 = true; } if (pos == std::string::npos) - return false; - std::string sub = name.substr(0, pos + 1); - const auto numdots = std::count(sub.begin(), sub.end(), '.'); + return std::nullopt; + name = name.substr(0, pos + 1); + const auto numdots = std::count(name.begin(), name.end(), '.'); if (numdots == 4 && !isV6) { - uint8_t a, b, c, d; - pos = sub.find('.'); - d = atoi(sub.substr(0, pos).c_str()); - sub = sub.substr(pos + 1); - pos = sub.find('.'); - c = atoi(sub.substr(0, pos).c_str()); - sub = sub.substr(pos + 1); - pos = sub.find('.'); - b = atoi(sub.substr(0, pos).c_str()); - sub = sub.substr(pos + 1); - pos = sub.find('.'); - a = atoi(sub.substr(0, pos).c_str()); - ip = net::ExpandV4(llarp::ipaddr_ipv4_bits(a, b, c, d)); - return true; - } - if (numdots == 32 && isV6) - { - size_t idx = 0; - uint8_t lo, hi; - auto* ptr = (uint8_t*)&ip.h; - while (idx < 16) + std::array q; + for (int i = 3; i >= 0; i--) { - pos = sub.find('.'); - lo = (*sub.substr(0, pos).c_str()) - 'a'; - sub = sub.substr(pos + 1); - pos = sub.find('.'); - hi = (*sub.substr(0, pos).c_str()) - 'a'; - sub = sub.substr(pos + 1); - ptr[idx] = lo | (hi << 4); - ++idx; + pos = name.find('.'); + if (!llarp::parse_int(name.substr(0, pos), q[i])) + return std::nullopt; + name.remove_prefix(pos + 1); } - return true; + return net::ExpandV4(llarp::ipaddr_ipv4_bits(q[0], q[1], q[2], q[3])); } + if (numdots == 32 && name.size() == 64 && isV6) + { + // We're going to convert from nybbles a.b.c.d.e.f.0.1.2.3.[...] into hex string + // "badcfe1032...", then decode the hex string to bytes. + std::array in; + auto in_pos = in.data(); + for (size_t i = 0; i < 64; i += 4) + { + if (not(oxenc::is_hex_digit(name[i]) and name[i + 1] == '.' + and oxenc::is_hex_digit(name[i + 2]) and name[i + 3] == '.')) + return std::nullopt; - return false; + // Flip the nybbles because the smallest one is first + *in_pos++ = name[i + 2]; + *in_pos++ = name[i]; + } + assert(in_pos == in.data() + in.size()); + huint128_t ip; + static_assert(in.size() == 2 * sizeof(ip.h)); + // our string right now is the little endian representation, so load it as such on little + // endian, or in reverse on big endian. + if constexpr (oxenc::little_endian) + oxenc::from_hex(in.begin(), in.end(), reinterpret_cast(&ip.h)); + else + oxenc::from_hex(in.rbegin(), in.rend(), reinterpret_cast(&ip.h)); + + return ip; + } + return std::nullopt; } bool - NameIsReserved(Name_t name) + NameIsReserved(std::string_view name) { const std::vector reserved_names = { - "snode.loki"sv, "loki.loki"sv, "snode.loki."sv, "loki.loki."sv}; + ".snode.loki"sv, ".loki.loki"sv, ".snode.loki."sv, ".loki.loki."sv}; for (const auto& reserved : reserved_names) { - if (ends_with(name, reserved)) + if (ends_with(name, reserved)) // subdomain foo.loki.loki + return true; + if (name == reserved.substr(1)) // loki.loki itself return true; } return false; diff --git a/llarp/dns/name.hpp b/llarp/dns/name.hpp index 2b93941a5..01e28a636 100644 --- a/llarp/dns/name.hpp +++ b/llarp/dns/name.hpp @@ -4,26 +4,25 @@ #include #include +#include namespace llarp { namespace dns { - using Name_t = std::string; - - /// decode name from buffer - bool - DecodeName(llarp_buffer_t* buf, Name_t& name, bool trimTrailingDot = false); + /// decode name from buffer; return nullopt on failure + std::optional + DecodeName(llarp_buffer_t* buf, bool trimTrailingDot = false); /// encode name to buffer bool - EncodeName(llarp_buffer_t* buf, Name_t name); + EncodeNameTo(llarp_buffer_t* buf, std::string_view name); + + std::optional + DecodePTR(std::string_view name); bool - DecodePTR(Name_t name, huint128_t& ip); - - bool - NameIsReserved(Name_t name); + NameIsReserved(std::string_view name); } // namespace dns } // namespace llarp diff --git a/llarp/dns/question.cpp b/llarp/dns/question.cpp index a3ea0e886..b7552f59e 100644 --- a/llarp/dns/question.cpp +++ b/llarp/dns/question.cpp @@ -27,7 +27,7 @@ namespace llarp bool Question::Encode(llarp_buffer_t* buf) const { - if (!EncodeName(buf, qname)) + if (!EncodeNameTo(buf, qname)) return false; if (!buf->put_uint16(qtype)) return false; @@ -37,7 +37,9 @@ namespace llarp bool Question::Decode(llarp_buffer_t* buf) { - if (!DecodeName(buf, qname)) + if (auto name = DecodeName(buf)) + qname = *std::move(name); + else { llarp::LogError("failed to decode name"); return false; diff --git a/llarp/dns/question.hpp b/llarp/dns/question.hpp index ae5b9cd10..4f0c2b10d 100644 --- a/llarp/dns/question.hpp +++ b/llarp/dns/question.hpp @@ -34,7 +34,7 @@ namespace llarp return qname == other.qname && qtype == other.qtype && qclass == other.qclass; } - Name_t qname; + std::string qname; QType_t qtype; QClass_t qclass; diff --git a/llarp/dns/rr.cpp b/llarp/dns/rr.cpp index 332ff68a2..9e31104e9 100644 --- a/llarp/dns/rr.cpp +++ b/llarp/dns/rr.cpp @@ -24,7 +24,7 @@ namespace llarp , rData(std::move(other.rData)) {} - ResourceRecord::ResourceRecord(Name_t name, RRType_t type, RR_RData_t data) + ResourceRecord::ResourceRecord(std::string name, RRType_t type, RR_RData_t data) : rr_name{std::move(name)} , rr_type{type} , rr_class{qClassIN} @@ -35,7 +35,7 @@ namespace llarp bool ResourceRecord::Encode(llarp_buffer_t* buf) const { - if (not EncodeName(buf, rr_name)) + if (not EncodeNameTo(buf, rr_name)) return false; if (!buf->put_uint16(rr_type)) { @@ -113,12 +113,10 @@ namespace llarp { if (rr_type != qTypeCNAME) return false; - Name_t name; llarp_buffer_t buf(rData); - if (not DecodeName(&buf, name)) - return false; - return name.find(tld) != std::string::npos - && name.rfind(tld) == (name.size() - tld.size()) - 1; + if (auto name = DecodeName(&buf)) + return name->rfind(tld) == name->size() - tld.size() - 1; + return false; } } // namespace dns diff --git a/llarp/dns/rr.hpp b/llarp/dns/rr.hpp index 7f27594e9..7bf290312 100644 --- a/llarp/dns/rr.hpp +++ b/llarp/dns/rr.hpp @@ -22,7 +22,7 @@ namespace llarp ResourceRecord(const ResourceRecord& other); ResourceRecord(ResourceRecord&& other); - explicit ResourceRecord(Name_t name, RRType_t type, RR_RData_t rdata); + explicit ResourceRecord(std::string name, RRType_t type, RR_RData_t rdata); bool Encode(llarp_buffer_t* buf) const override; @@ -39,7 +39,7 @@ namespace llarp bool HasCNameForTLD(const std::string& tld) const; - Name_t rr_name; + std::string rr_name; RRType_t rr_type; RRClass_t rr_class; RR_TTL_t ttl; diff --git a/llarp/handlers/exit.cpp b/llarp/handlers/exit.cpp index 73902ac04..48eec98a1 100644 --- a/llarp/handlers/exit.cpp +++ b/llarp/handlers/exit.cpp @@ -196,10 +196,9 @@ namespace llarp // always hook ptr for ranges we own if (msg.questions[0].qtype == dns::qTypePTR) { - huint128_t ip; - if (!dns::DecodePTR(msg.questions[0].qname, ip)) - return false; - return m_OurRange.Contains(ip); + if (auto ip = dns::DecodePTR(msg.questions[0].qname)) + return m_OurRange.Contains(*ip); + return false; } if (msg.questions[0].qtype == dns::qTypeA || msg.questions[0].qtype == dns::qTypeCNAME || msg.questions[0].qtype == dns::qTypeAAAA) @@ -217,8 +216,8 @@ namespace llarp { if (msg.questions[0].qtype == dns::qTypePTR) { - huint128_t ip; - if (!dns::DecodePTR(msg.questions[0].qname, ip)) + auto ip = dns::DecodePTR(msg.questions[0].qname); + if (not ip) return false; if (ip == m_IfAddr) { @@ -227,7 +226,7 @@ namespace llarp } else { - auto itr = m_IPToKey.find(ip); + auto itr = m_IPToKey.find(*ip); if (itr != m_IPToKey.end() && m_SNodeKeys.find(itr->second) != m_SNodeKeys.end()) { RouterID them = itr->second; diff --git a/llarp/handlers/tun.cpp b/llarp/handlers/tun.cpp index 66dcee92b..e6fc32bf0 100644 --- a/llarp/handlers/tun.cpp +++ b/llarp/handlers/tun.cpp @@ -479,25 +479,25 @@ namespace llarp const auto& answer = msg.answers[0]; if (answer.HasCNameForTLD(".snode")) { - dns::Name_t qname; llarp_buffer_t buf(answer.rData); - if (not dns::DecodeName(&buf, qname, true)) + auto qname = dns::DecodeName(&buf, true); + if (not qname) return false; RouterID addr; - if (not addr.FromString(qname)) + if (not addr.FromString(*qname)) return false; auto replyMsg = std::make_shared(clear_dns_message(msg)); return ReplyToSNodeDNSWhenReady(addr, std::move(replyMsg), false); } else if (answer.HasCNameForTLD(".loki")) { - dns::Name_t qname; llarp_buffer_t buf(answer.rData); - if (not dns::DecodeName(&buf, qname, true)) + auto qname = dns::DecodeName(&buf, true); + if (not qname) return false; service::Address addr; - if (not addr.FromString(qname)) + if (not addr.FromString(*qname)) return false; auto replyMsg = std::make_shared(clear_dns_message(msg)); @@ -745,20 +745,16 @@ namespace llarp else if (msg.questions[0].qtype == dns::qTypePTR) { // reverse dns - huint128_t ip = {0}; - if (!dns::DecodePTR(msg.questions[0].qname, ip)) + if (auto ip = dns::DecodePTR(msg.questions[0].qname)) { - msg.AddNXReply(); - reply(msg); - return true; + if (auto maybe = ObtainAddrForIP(*ip)) + { + var::visit([&msg](auto&& result) { msg.AddAReply(result.ToString()); }, *maybe); + reply(msg); + return true; + } } - if (auto maybe = ObtainAddrForIP(ip)) - { - var::visit([&msg](auto&& result) { msg.AddAReply(result.ToString()); }, *maybe); - reply(msg); - return true; - } msg.AddNXReply(); reply(msg); return true; @@ -816,10 +812,9 @@ namespace llarp // hook any ranges we own if (msg.questions[0].qtype == llarp::dns::qTypePTR) { - huint128_t ip = {0}; - if (!dns::DecodePTR(msg.questions[0].qname, ip)) - return false; - return m_OurRange.Contains(ip); + if (auto ip = dns::DecodePTR(msg.questions[0].qname)) + return m_OurRange.Contains(*ip); + return false; } } for (const auto& answer : msg.answers) diff --git a/test/dns/test_llarp_dns_dns.cpp b/test/dns/test_llarp_dns_dns.cpp index d572fa933..ff800b4ec 100644 --- a/test/dns/test_llarp_dns_dns.cpp +++ b/test/dns/test_llarp_dns_dns.cpp @@ -90,11 +90,19 @@ TEST_CASE("Test Get Subdomains" , "[dns]") TEST_CASE("Test PTR records", "[dns]") { - llarp::huint128_t ip = {0}; llarp::huint128_t expected = llarp::net::ExpandV4(llarp::ipaddr_ipv4_bits(10, 10, 10, 1)); - CHECK(llarp::dns::DecodePTR("1.10.10.10.in-addr.arpa.", ip)); - CHECK(ip == expected); + auto ip = llarp::dns::DecodePTR("1.10.10.10.in-addr.arpa."); + CHECK(ip); + CHECK(*ip == expected); + + expected.h.upper = 0x0123456789abcdefUL; + expected.h.lower = 0xeeee888812341234UL; + ip = llarp::dns::DecodePTR("4.3.2.1.4.3.2.1.8.8.8.8.e.e.e.e.f.e.d.c.b.a.9.8.7.6.5.4.3.2.1.0.ip6.arpa."); + CHECK(ip); + CHECK(oxenc::to_hex(std::string_view{reinterpret_cast(&expected.h), 16}) == + oxenc::to_hex(std::string_view{reinterpret_cast(&ip->h), 16})); + CHECK(*ip == expected); } TEST_CASE("Test Serialize Header", "[dns]") @@ -123,13 +131,12 @@ TEST_CASE("Test Serialize Header", "[dns]") TEST_CASE("Test Serialize Name" , "[dns]") { - const llarp::dns::Name_t name = "whatever.tld"; - const llarp::dns::Name_t expected = "whatever.tld."; - llarp::dns::Name_t other; + const std::string name = "whatever.tld"; + const std::string expected = "whatever.tld."; std::array data{}; llarp_buffer_t buf(data); - CHECK(llarp::dns::EncodeName(&buf, name)); + CHECK(llarp::dns::EncodeNameTo(&buf, name)); buf.cur = buf.base; @@ -147,8 +154,9 @@ TEST_CASE("Test Serialize Name" , "[dns]") CHECK(buf.base[11] == 'l'); CHECK(buf.base[12] == 'd'); CHECK(buf.base[13] == 0); - CHECK(llarp::dns::DecodeName(&buf, other)); - CHECK(expected == other); + auto other = llarp::dns::DecodeName(&buf); + CHECK(other); + CHECK(expected == *other); } TEST_CASE("Test serialize question", "[dns]") @@ -191,3 +199,20 @@ TEST_CASE("Test Encode/Decode RData" , "[dns]") CHECK(llarp::dns::DecodeRData(&buf, other_rdata)); CHECK(rdata == other_rdata); } + +TEST_CASE("Test reserved names", "[dns]") +{ + using namespace llarp::dns; + CHECK(NameIsReserved("loki.loki")); + CHECK(NameIsReserved("loki.loki.")); + CHECK(NameIsReserved("snode.loki")); + CHECK(NameIsReserved("snode.loki.")); + CHECK(NameIsReserved("foo.loki.loki")); + CHECK(NameIsReserved("foo.loki.loki.")); + CHECK(NameIsReserved("bar.snode.loki")); + CHECK(NameIsReserved("bar.snode.loki.")); + CHECK_FALSE(NameIsReserved("barsnode.loki.")); + CHECK_FALSE(NameIsReserved("barsnode.loki")); + CHECK_FALSE(NameIsReserved("alltheloki.loki")); + CHECK_FALSE(NameIsReserved("alltheloki.loki.")); +}