From 0fe6fcc15c8b1e35c92f86a8ba11721528a81ccd Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 19 May 2025 14:31:38 +0200 Subject: [PATCH] Alwas detect mismatches in outgoing and incoming ECS; add tests for that as well --- pdns/dnsrecords.cc | 10 ++ pdns/dnsrecords.hh | 2 + pdns/iputils.hh | 5 + pdns/recursordist/RECURSOR-MIB.txt | 46 ++++++++- pdns/recursordist/lwres.cc | 54 +++++------ pdns/recursordist/pdns_recursor.cc | 25 +++-- pdns/recursordist/syncres.hh | 2 +- .../recursortests.py | 31 +++++- regression-tests.recursor-dnssec/test_ECS.py | 97 +++++++++++++++++-- 9 files changed, 224 insertions(+), 48 deletions(-) diff --git a/pdns/dnsrecords.cc b/pdns/dnsrecords.cc index a7c752be1f..55e5b993c1 100644 --- a/pdns/dnsrecords.cc +++ b/pdns/dnsrecords.cc @@ -1028,6 +1028,16 @@ void checkHostnameCorrectness(const DNSResourceRecord& rr) } } +vector>::const_iterator EDNSOpts::getFirstOption(uint16_t optionCode) const +{ + for (auto iter = d_options.cbegin(); iter != d_options.cend(); ++iter) { + if (iter->first == optionCode) { + return iter; + } + } + return d_options.cend(); +} + #if 0 static struct Reporter { diff --git a/pdns/dnsrecords.hh b/pdns/dnsrecords.hh index f674dd4da9..1c6c37f75e 100644 --- a/pdns/dnsrecords.hh +++ b/pdns/dnsrecords.hh @@ -1061,6 +1061,8 @@ struct EDNSOpts uint16_t d_packetsize{0}; uint16_t d_extFlags{0}; uint8_t d_extRCode, d_version; + + [[nodiscard]] vector>::const_iterator getFirstOption(uint16_t optionCode) const; }; //! Convenience function that fills out EDNS0 options, and returns true if there are any diff --git a/pdns/iputils.hh b/pdns/iputils.hh index ba9c50d781..44250afeb3 100644 --- a/pdns/iputils.hh +++ b/pdns/iputils.hh @@ -733,6 +733,11 @@ public: return std::tie(d_network, d_bits) == std::tie(rhs.d_network, rhs.d_bits); } + bool operator!=(const Netmask& rhs) const + { + return !(*this == rhs); + } + [[nodiscard]] bool empty() const { return d_network.sin4.sin_family == 0; diff --git a/pdns/recursordist/RECURSOR-MIB.txt b/pdns/recursordist/RECURSOR-MIB.txt index 99a7dc8d80..01b8d6f565 100644 --- a/pdns/recursordist/RECURSOR-MIB.txt +++ b/pdns/recursordist/RECURSOR-MIB.txt @@ -21,8 +21,50 @@ rec MODULE-IDENTITY DESCRIPTION "This MIB module describes information gathered through PowerDNS Recursor." - REVISION "201611290000Z" - DESCRIPTION "Initial revision." + REVISION "202505270000Z" + DESCRIPTION "Added metric for missing ECS in reply" + + REVISION "202408280000Z" + DESCRIPTION "Added metric for too many incoming TCP connections" + + REVISION "202408130000Z" + DESCRIPTION "Added metric for chain limits reached" + + REVISION "202405230000Z" + DESCRIPTION "Added metrics for maximum chain length and weight" + + REVISION "202306080000Z" + DESCRIPTION "Added metrics for NOD and UDR events" + + REVISION "202302240000Z" + DESCRIPTION "Added metrics for sharded packet cache contention" + + REVISION "202209120000Z" + DESCRIPTION "Added metrics for answers from auths by rcode" + + REVISION "202208220000Z" + DESCRIPTION "Added internal maintenance metrics." + + REVISION "202201310000Z" + DESCRIPTION "Added non-resolving NS name metric." + + REVISION "202111090000Z" + DESCRIPTION "Added NOTIFY-related metrics." + + REVISION "202110270000Z" + DESCRIPTION "Added more UDP errors metric." + + REVISION "202107200000Z" + DESCRIPTION "Added almost expired task metrics." + + REVISION "202101050000Z" + DESCRIPTION "Added Aggressive NSEC cache metrics." + + REVISION "202002170000Z" + DESCRIPTION "Added proxyProtocolInvalid metric." + + REVISION "201911140000Z" + DESCRIPTION "Added qnameMinFallbackSuccess stats." REVISION "201812240000Z" DESCRIPTION "Added the dnssecAuthenticDataQueries and dnssecCheckDisabledQueries stats." diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index d37d485e77..c0fd64ed89 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -594,37 +594,37 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& for (const auto& a : mdp.d_answers) lwr->d_records.push_back(a.first); - EDNSOpts edo; - if (EDNS0Level > 0 && getEDNSOpts(mdp, &edo)) { + if (EDNSOpts edo; EDNS0Level > 0 && getEDNSOpts(mdp, &edo)) { lwr->d_haveEDNS = true; // If we sent out ECS, we can also expect to see a return with or without ECS, the absent case - // is not handled explicitly. In hardening mode, if we do see a ECS in the reply, the source - // part *must* match with what we sent out. See - // https://www.rfc-editor.org/rfc/rfc7871#section-7.3 + // is not handled explicitly. If we do see a ECS in the reply, the source part *must* match + // with what we sent out. See https://www.rfc-editor.org/rfc/rfc7871#section-7.3. and section + // 11.2. + // For ECS hardening mode, the case where we sent out an ECS but did not receive a matching + // one is handled in arecvfrom(). if (subnetOpts) { - for (const auto& opt : edo.d_options) { - if (opt.first == EDNSOptionCode::ECS) { - EDNSSubnetOpts reso; - if (getEDNSSubnetOptsFromString(opt.second, &reso)) { - if (g_ECSHardening && !(reso.source == subnetOpts->source)) { - g_slogout->info(Logr::Notice, "Incoming ECS does not match outgoing", - "server", Logging::Loggable(address), - "qname", Logging::Loggable(domain), - "outgoing", Logging::Loggable(subnetOpts->source), - "incoming", Logging::Loggable(reso.source)); - return LWResult::Result::Spoofed; // XXXXX OK? - } - /* rfc7871 states that 0 "indicate[s] that the answer is suitable for all addresses in FAMILY", - so we might want to still pass the information along to be able to differentiate between - IPv4 and IPv6. Still I'm pretty sure it doesn't matter in real life, so let's not duplicate - entries in our cache. */ - if (reso.scope.getBits()) { - uint8_t bits = std::min(reso.scope.getBits(), subnetOpts->source.getBits()); - auto outgoingECSAddr = subnetOpts->source.getNetwork(); - outgoingECSAddr.truncate(bits); - srcmask = Netmask(outgoingECSAddr, bits); - } + // THE RFC is not clear about the case of having multiple ECS options. We only look at the first. + if (const auto opt = edo.getFirstOption(EDNSOptionCode::ECS); opt != edo.d_options.end()) { + EDNSSubnetOpts reso; + if (getEDNSSubnetOptsFromString(opt->second, &reso)) { + if (!doTCP && reso.source != subnetOpts->source) { + g_slogout->info(Logr::Notice, "Incoming ECS does not match outgoing", + "server", Logging::Loggable(address), + "qname", Logging::Loggable(domain), + "outgoing", Logging::Loggable(subnetOpts->source), + "incoming", Logging::Loggable(reso.source)); + return LWResult::Result::Spoofed; + } + /* rfc7871 states that 0 "indicate[s] that the answer is suitable for all addresses in FAMILY", + so we might want to still pass the information along to be able to differentiate between + IPv4 and IPv6. Still I'm pretty sure it doesn't matter in real life, so let's not duplicate + entries in our cache. */ + if (reso.scope.getBits() != 0) { + uint8_t bits = std::min(reso.scope.getBits(), subnetOpts->source.getBits()); + auto outgoingECSAddr = subnetOpts->source.getNetwork(); + outgoingECSAddr.truncate(bits); + srcmask = Netmask(outgoingECSAddr, bits); } } } diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 2cc23b2554..0f3c795c6e 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -375,9 +375,10 @@ LWResult::Result arecvfrom(PacketBuffer& packet, int /* flags */, const ComboAdd len = packet.size(); - // In ecs hardening mode, we consider a missing ECS in the reply as a case for retrying without ECS - // The actual logic to do that is in Syncres::doResolveAtThisIP() - if (g_ECSHardening && pident->ecsSubnet && !*pident->ecsReceived) { + // In ecs hardening mode, we consider a missing or a mismatched ECS in the reply as a case for + // retrying without ECS (matchingECSReceived only gets set if a matching ECS was received). The actual + // logic to do that is in Syncres::doResolveAtThisIP() + if (g_ECSHardening && pident->ecsSubnet && !*pident->matchingECSReceived) { t_Counters.at(rec::Counter::ecsMissingCount)++; return LWResult::Result::ECSMissing; } @@ -2075,7 +2076,7 @@ void getQNameAndSubnet(const std::string& question, DNSName* dnsname, uint16_t* /* we need to pass the record len */ int res = getEDNSOptions(reinterpret_cast(&question.at(pos - sizeof(drh->d_clen))), questionLen - pos + (sizeof(drh->d_clen)), *options); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast) if (res == 0) { - const auto& iter = options->find(EDNSOptionCode::ECS); + const auto iter = options->find(EDNSOptionCode::ECS); if (iter != options->end() && !iter->second.values.empty() && iter->second.values.at(0).content != nullptr && iter->second.values.at(0).size > 0) { EDNSSubnetOpts eso; if (getEDNSSubnetOptsFromString(iter->second.values.at(0).content, iter->second.values.at(0).size, &eso)) { @@ -2889,7 +2890,7 @@ void distributeAsyncFunction(const string& packet, const pipefunc_t& func) } // resend event to everybody chained onto it -static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptr& resend, const PacketBuffer& content, const std::optional& ecsReceived) +static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptr& resend, const PacketBuffer& content, const std::optional& matchingECSReceived) { // We close the chain for new entries, since they won't be processed anyway iter->key->closed = true; @@ -2898,8 +2899,9 @@ static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptrkey->ecsReceived = ecsReceived; + // Only set if g_ECSHardening + if (matchingECSReceived) { + iter->key->matchingECSReceived = matchingECSReceived; } auto maxWeight = t_Counters.at(rec::Counter::maxChainWeight); @@ -2946,7 +2948,8 @@ static bool checkIncomingECSSource(const PacketBuffer& packet, const Netmask& su foundMatchingECS = true; } } - break; // only look at first + break; // The RFC isn't clear about multiple ECS options. We chose to handle it like cookies + // and only look at the first. } } } @@ -3033,8 +3036,10 @@ static void handleUDPServerResponse(int fileDesc, FDMultiplexer::funcparam_t& va if (!pident->domain.empty()) { auto iter = g_multiTasker->getWaiters().find(pident); if (iter != g_multiTasker->getWaiters().end()) { - iter->key->ecsReceived = iter->key->ecsSubnet && checkIncomingECSSource(packet, *iter->key->ecsSubnet); - doResends(iter, pident, packet, iter->key->ecsReceived); + if (g_ECSHardening) { + iter->key->matchingECSReceived = iter->key->ecsSubnet && checkIncomingECSSource(packet, *iter->key->ecsSubnet); + } + doResends(iter, pident, packet, iter->key->matchingECSReceived); } } diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh index 13a74ace9c..6d22624b47 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -779,7 +779,7 @@ struct PacketID bool inIncompleteOkay{false}; uint16_t id{0}; // wait for a specific id/remote pair uint16_t type{0}; // and this is its type - std::optional ecsReceived; // only set in ecsHardened mode + std::optional matchingECSReceived; // only set in ecsHardened mode TCPAction highState{TCPAction::DoingRead}; IOState lowState{IOState::NeedRead}; diff --git a/regression-tests.recursor-dnssec/recursortests.py b/regression-tests.recursor-dnssec/recursortests.py index 1385c12811..97a2cfc246 100644 --- a/regression-tests.recursor-dnssec/recursortests.py +++ b/regression-tests.recursor-dnssec/recursortests.py @@ -12,7 +12,9 @@ import time import unittest import dns import dns.message - +import requests +import threading +from twisted.internet import reactor from proxyprotocol import ProxyProtocol from eqdnsmessage import AssertEqualDNSMessageMixin @@ -1190,3 +1192,30 @@ distributor-threads={threads}""".format(confdir=confdir, if data: message = dns.message.from_wire(data) return message + + def checkMetrics(self, map): + self.waitForTCPSocket("127.0.0.1", self._wsPort) + headers = {'x-api-key': self._apiKey} + url = 'http://127.0.0.1:' + str(self._wsPort) + '/api/v1/servers/localhost/statistics' + r = requests.get(url, headers=headers, timeout=self._wsTimeout) + self.assertEqual(r.status_code, 200) + self.assertTrue(r.json()) + content = r.json() + count = 0 + for entry in content: + for key, expected in map.items(): + if entry['name'] == key: + value = int(entry['value']) + if callable(expected): + self.assertTrue(expected(value)) + else: + self.assertEqual(value, expected) + count += 1 + self.assertEqual(count, len(map)) + + @classmethod + def startReactor(cls): + if not reactor.running: + cls.Responder = threading.Thread(name='Responder', target=reactor.run, args=(False,)) + cls.Responder.daemon = True + cls.Responder.start() diff --git a/regression-tests.recursor-dnssec/test_ECS.py b/regression-tests.recursor-dnssec/test_ECS.py index dde1dfe8ea..7f9b15ed59 100644 --- a/regression-tests.recursor-dnssec/test_ECS.py +++ b/regression-tests.recursor-dnssec/test_ECS.py @@ -7,10 +7,14 @@ import time import clientsubnetoption import unittest from recursortests import RecursorTest, have_ipv6 + +from twisted.internet.protocol import Factory +from twisted.internet.protocol import Protocol from twisted.internet.protocol import DatagramProtocol from twisted.internet import reactor emptyECSText = 'No ECS received' +mismatchedECSText = 'Mismatched ECS' nameECS = 'ecs-echo.example.' nameECSInvalidScope = 'invalid-scope.ecs-echo.example.' ttlECS = 60 @@ -86,16 +90,15 @@ ecs-add-for=0.0.0.0/0 if not ecsReactorRunning: reactor.listenUDP(port, UDPECSResponder(), interface=address) + reactor.listenTCP(port, TCPECSFactory(), interface=address) ecsReactorRunning = True if not ecsReactorv6Running and have_ipv6(): reactor.listenUDP(53000, UDPECSResponder(), interface='::1') + reactor.listenTCP(53000, TCPECSFactory(), interface='::1') ecsReactorv6Running = True - if not reactor.running: - cls._UDPResponder = threading.Thread(name='UDP Responder', target=reactor.run, args=(False,)) - cls._UDPResponder.daemon = True - cls._UDPResponder.start() + cls.startReactor() @classmethod def setUpClass(cls): @@ -212,7 +215,7 @@ webserver-allow-from=127.0.0.1 api-key=%s """ % (os.environ['PREFIX'], _wsPort, _wsPassword, _apiKey) - # All test below have ecs-missing count to be 1, as they result is a no ecs scoped answer in the cache + # All test below have ecs-missing count to be 1, as they result in a non ecs scoped answer in the cache def test1SendECS(self): expected = dns.rrset.from_text('x'+ nameECS, ttlECS, dns.rdataclass.IN, 'TXT', 'X') ecso = clientsubnetoption.ClientSubnetOption('192.0.2.1', 32) @@ -239,6 +242,58 @@ api-key=%s 'ecs-missing': 1 }) +class MismatchedECSInAnswerTest(ECSTest): + _confdir = 'MismatchedECSInAnswer' + + _config_template = """edns-subnet-allow-list=mecs-echo.example +forward-zones=mecs-echo.example=%s.21 +dont-throttle-netmasks=0.0.0.0/0 + """ % (os.environ['PREFIX']) + + def test1SendECS(self): + expected = dns.rrset.from_text('m'+ nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + ecso = clientsubnetoption.ClientSubnetOption('192.0.2.1', 32) + query = dns.message.make_query('m' + nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + + def test2NoECS(self): + expected = dns.rrset.from_text('m' + nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + query = dns.message.make_query('m' + nameECS, 'TXT') + self.sendECSQuery(query, expected) + + def test3RequireNoECS(self): + expected = dns.rrset.from_text('m' + nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query('m' + nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + +class MismatchedECSInAnswerHardenedTest(MismatchedECSInAnswerTest): + _confdir = 'MismatchedECSInAnswerHardened' + + _config_template = """ +edns-subnet-harden=yes +edns-subnet-allow-list=mecs-echo.example +forward-zones=mecs-echo.example=%s.21 +dont-throttle-netmasks=0.0.0.0/0 + """ % (os.environ['PREFIX']) + + def test1SendECS(self): + expected = dns.rrset.from_text('m'+ nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + ecso = clientsubnetoption.ClientSubnetOption('192.0.2.1', 32) + query = dns.message.make_query('m' + nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + + def test2NoECS(self): + expected = dns.rrset.from_text('m' + nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + query = dns.message.make_query('m' + nameECS, 'TXT') + self.sendECSQuery(query, expected) + + def test3RequireNoECS(self): + expected = dns.rrset.from_text('m' + nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query('m' + nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class IncomingNoECSTest(ECSTest): _confdir = 'IncomingNoECS' @@ -835,7 +890,7 @@ class UDPECSResponder(DatagramProtocol): option.ip & (2 ** 64 - 1))) return ip - def datagramReceived(self, datagram, address): + def question(self, datagram, tcp=False): request = dns.message.from_wire(datagram) response = dns.message.make_response(request) @@ -868,8 +923,36 @@ class UDPECSResponder(DatagramProtocol): elif request.question[0].name == dns.name.from_text('x' + nameECS): answer = dns.rrset.from_text(request.question[0].name, ttlECS, dns.rdataclass.IN, 'TXT', 'X') response.answer.append(answer) + elif request.question[0].name == dns.name.from_text('m' + nameECS): + incomingECS = False + for option in request.options: + if option.otype == clientsubnetoption.ASSIGNED_OPTION_CODE and isinstance(option, clientsubnetoption.ClientSubnetOption): + incomingECS = True + # Send mismatched ECS over UDP + flag = emptyECSText + if not tcp and incomingECS: + ecso = clientsubnetoption.ClientSubnetOption("193.0.2.1", 24, 25) + flag = mismatchedECSText + answer = dns.rrset.from_text(request.question[0].name, ttlECS, dns.rdataclass.IN, 'TXT', flag) + response.answer.append(answer) if ecso: response.use_edns(options = [ecso]) - self.transport.write(response.to_wire(), address) + return response.to_wire() + + def datagramReceived(self, datagram, address): + response = self.question(datagram) + self.transport.write(response, address) + +class TCPECSResponder(Protocol): + def dataReceived(self, data): + handler = UDPECSResponder() + response = handler.question(data[2:], True) + length = len(response) + header = length.to_bytes(2, 'big') + self.transport.write(header + response) + +class TCPECSFactory(Factory): + def buildProtocol(self, addr): + return TCPECSResponder() -- 2.47.2