From 1793f51bc6b300cce21551a8cd355a2aad9f9544 Mon Sep 17 00:00:00 2001 From: Tim Ledbetter Date: Mon, 13 Nov 2023 22:36:34 +0000 Subject: [PATCH] LibDNS: Make DNS packet parsing fallible Previously, a DNS packet containing an invalid name would be returned with an empty name. With this change, an error is returned if any error is encountered during parsing. --- Meta/Lagom/Fuzzers/FuzzDNSPacket.cpp | 6 +++--- Userland/Libraries/LibDNS/Name.cpp | 12 ++++++------ Userland/Libraries/LibDNS/Name.h | 2 +- Userland/Libraries/LibDNS/Packet.cpp | 18 +++++++++--------- Userland/Libraries/LibDNS/Packet.h | 2 +- Userland/Services/LookupServer/DNSServer.cpp | 7 +------ .../Services/LookupServer/LookupServer.cpp | 6 +++--- .../Services/LookupServer/MulticastDNS.cpp | 16 +++++----------- 8 files changed, 29 insertions(+), 40 deletions(-) diff --git a/Meta/Lagom/Fuzzers/FuzzDNSPacket.cpp b/Meta/Lagom/Fuzzers/FuzzDNSPacket.cpp index 40a4732b67..bb72c35e10 100644 --- a/Meta/Lagom/Fuzzers/FuzzDNSPacket.cpp +++ b/Meta/Lagom/Fuzzers/FuzzDNSPacket.cpp @@ -9,10 +9,10 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { AK::set_debug_enabled(false); - auto maybe_packet = DNS::Packet::from_raw_packet({ data, size }); - if (!maybe_packet.has_value()) + auto packet_or_error = DNS::Packet::from_raw_packet({ data, size }); + if (packet_or_error.is_error()) return 0; - (void)maybe_packet.value().to_byte_buffer(); + (void)packet_or_error.release_value().to_byte_buffer(); return 0; } diff --git a/Userland/Libraries/LibDNS/Name.cpp b/Userland/Libraries/LibDNS/Name.cpp index 7e7341baa1..0769845236 100644 --- a/Userland/Libraries/LibDNS/Name.cpp +++ b/Userland/Libraries/LibDNS/Name.cpp @@ -21,15 +21,15 @@ Name::Name(DeprecatedString const& name) m_name = name; } -Name Name::parse(ReadonlyBytes data, size_t& offset, size_t recursion_level) +ErrorOr Name::parse(ReadonlyBytes data, size_t& offset, size_t recursion_level) { if (recursion_level > 4) - return {}; + return Name {}; StringBuilder builder; while (true) { if (offset >= data.size()) - return {}; + return Error::from_string_literal("Unexpected EOF when parsing name"); u8 b = data[offset++]; if (b == '\0') { // This terminates the name. @@ -37,15 +37,15 @@ Name Name::parse(ReadonlyBytes data, size_t& offset, size_t recursion_level) } else if ((b & 0xc0) == 0xc0) { // The two bytes tell us the offset when to continue from. if (offset >= data.size()) - return {}; + return Error::from_string_literal("Unexpected EOF when parsing name"); size_t dummy = (b & 0x3f) << 8 | data[offset++]; - auto rest_of_name = parse(data, dummy, recursion_level + 1); + auto rest_of_name = TRY(parse(data, dummy, recursion_level + 1)); builder.append(rest_of_name.as_string()); return builder.to_deprecated_string(); } else { // This is the length of a part. if (offset + b >= data.size()) - return {}; + return Error::from_string_literal("Unexpected EOF when parsing name"); builder.append({ data.offset_pointer(offset), b }); builder.append('.'); offset += b; diff --git a/Userland/Libraries/LibDNS/Name.h b/Userland/Libraries/LibDNS/Name.h index 3140b01d2b..66b8ceb28e 100644 --- a/Userland/Libraries/LibDNS/Name.h +++ b/Userland/Libraries/LibDNS/Name.h @@ -17,7 +17,7 @@ public: Name() = default; Name(DeprecatedString const&); - static Name parse(ReadonlyBytes data, size_t& offset, size_t recursion_level = 0); + static ErrorOr parse(ReadonlyBytes data, size_t& offset, size_t recursion_level = 0); size_t serialized_size() const; DeprecatedString const& as_string() const { return m_name; } diff --git a/Userland/Libraries/LibDNS/Packet.cpp b/Userland/Libraries/LibDNS/Packet.cpp index 0671dc72d1..8250f1efaf 100644 --- a/Userland/Libraries/LibDNS/Packet.cpp +++ b/Userland/Libraries/LibDNS/Packet.cpp @@ -97,11 +97,11 @@ private: static_assert(sizeof(DNSRecordWithoutName) == 10); -Optional Packet::from_raw_packet(ReadonlyBytes bytes) +ErrorOr Packet::from_raw_packet(ReadonlyBytes bytes) { if (bytes.size() < sizeof(PacketHeader)) { - dbgln("DNS response not large enough ({} out of {}) to be a DNS packet.", bytes.size(), sizeof(PacketHeader)); - return {}; + dbgln_if(LOOKUPSERVER_DEBUG, "DNS response not large enough ({} out of {}) to be a DNS packet", bytes.size(), sizeof(PacketHeader)); + return Error::from_string_literal("DNS response not large enough to be a DNS packet"); } auto const& header = *bit_cast(bytes.data()); @@ -123,13 +123,13 @@ Optional Packet::from_raw_packet(ReadonlyBytes bytes) size_t offset = sizeof(PacketHeader); for (u16 i = 0; i < header.question_count(); i++) { - auto name = Name::parse(bytes, offset); + auto name = TRY(Name::parse(bytes, offset)); struct RawDNSAnswerQuestion { NetworkOrdered record_type; NetworkOrdered class_code; }; if (offset >= bytes.size() || bytes.size() - offset < sizeof(RawDNSAnswerQuestion)) - return {}; + return Error::from_string_literal("Unexpected EOF when parsing DNS packet"); auto const& record_and_class = *bit_cast(bytes.offset_pointer(offset)); u16 class_code = record_and_class.class_code & ~MDNS_WANTS_UNICAST_RESPONSE; @@ -141,21 +141,21 @@ Optional Packet::from_raw_packet(ReadonlyBytes bytes) } for (u16 i = 0; i < header.answer_count(); ++i) { - auto name = Name::parse(bytes, offset); + auto name = TRY(Name::parse(bytes, offset)); if (offset >= bytes.size() || bytes.size() - offset < sizeof(DNSRecordWithoutName)) - return {}; + return Error::from_string_literal("Unexpected EOF when parsing DNS packet"); auto const& record = *bit_cast(bytes.offset_pointer(offset)); offset += sizeof(DNSRecordWithoutName); if (record.data_length() > bytes.size() - offset) - return {}; + return Error::from_string_literal("Unexpected EOF when parsing DNS packet"); DeprecatedString data; switch ((RecordType)record.type()) { case RecordType::PTR: { size_t dummy_offset = offset; - data = Name::parse(bytes, dummy_offset).as_string(); + data = TRY(Name::parse(bytes, dummy_offset)).as_string(); break; } case RecordType::CNAME: diff --git a/Userland/Libraries/LibDNS/Packet.h b/Userland/Libraries/LibDNS/Packet.h index 159fd2abc3..8198bc6b06 100644 --- a/Userland/Libraries/LibDNS/Packet.h +++ b/Userland/Libraries/LibDNS/Packet.h @@ -24,7 +24,7 @@ class Packet { public: Packet() = default; - static Optional from_raw_packet(ReadonlyBytes bytes); + static ErrorOr from_raw_packet(ReadonlyBytes bytes); ErrorOr to_byte_buffer() const; bool is_query() const { return !m_query_or_response; } diff --git a/Userland/Services/LookupServer/DNSServer.cpp b/Userland/Services/LookupServer/DNSServer.cpp index 79e244d08e..afc642d824 100644 --- a/Userland/Services/LookupServer/DNSServer.cpp +++ b/Userland/Services/LookupServer/DNSServer.cpp @@ -29,12 +29,7 @@ ErrorOr DNSServer::handle_client() { sockaddr_in client_address; auto buffer = TRY(receive(1024, client_address)); - auto optional_request = Packet::from_raw_packet(buffer); - if (!optional_request.has_value()) { - dbgln("Got an invalid DNS packet"); - return {}; - } - auto& request = optional_request.value(); + auto request = TRY(Packet::from_raw_packet(buffer)); if (!request.is_query()) { dbgln("It's not a request"); diff --git a/Userland/Services/LookupServer/LookupServer.cpp b/Userland/Services/LookupServer/LookupServer.cpp index eb96ec986e..81570280f4 100644 --- a/Userland/Services/LookupServer/LookupServer.cpp +++ b/Userland/Services/LookupServer/LookupServer.cpp @@ -269,11 +269,11 @@ ErrorOr> LookupServer::lookup(Name const& name, DeprecatedString did_get_response = true; - auto o_response = Packet::from_raw_packet({ response_buffer, nrecv }); - if (!o_response.has_value()) + auto response_or_error = Packet::from_raw_packet({ response_buffer, nrecv }); + if (response_or_error.is_error()) return Vector {}; - auto& response = o_response.value(); + auto response = response_or_error.release_value(); if (response.id() != request.id()) { dbgln("LookupServer: ID mismatch ({} vs {}) :(", response.id(), request.id()); diff --git a/Userland/Services/LookupServer/MulticastDNS.cpp b/Userland/Services/LookupServer/MulticastDNS.cpp index 3c4b810993..ba8af23a96 100644 --- a/Userland/Services/LookupServer/MulticastDNS.cpp +++ b/Userland/Services/LookupServer/MulticastDNS.cpp @@ -56,13 +56,7 @@ MulticastDNS::MulticastDNS(Core::EventReceiver* parent) ErrorOr MulticastDNS::handle_packet() { auto buffer = TRY(receive(1024)); - auto optional_packet = Packet::from_raw_packet(buffer); - if (!optional_packet.has_value()) { - dbgln("Got an invalid mDNS packet"); - return {}; - } - auto& packet = optional_packet.value(); - + auto packet = TRY(Packet::from_raw_packet(buffer)); if (packet.is_query()) handle_query(packet); return {}; @@ -175,12 +169,12 @@ ErrorOr> MulticastDNS::lookup(Name const& name, RecordType record auto buffer = TRY(receive(1024)); if (buffer.is_empty()) return Vector {}; - auto optional_packet = Packet::from_raw_packet(buffer); - if (!optional_packet.has_value()) { - dbgln("Got an invalid mDNS packet"); + auto packet_or_error = Packet::from_raw_packet(buffer); + if (packet_or_error.is_error()) { + dbgln("Got an invalid mDNS packet: {}", packet_or_error.release_error()); continue; } - auto& packet = optional_packet.value(); + auto packet = packet_or_error.release_value(); if (packet.is_query()) continue;