From: Otto Moerbeek Date: Mon, 19 May 2025 12:31:38 +0000 (+0200) Subject: Alwas detect mismatches in outgoing and incoming ECS; add tests for that as well X-Git-Tag: rec-5.4.0-alpha0~18^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bebb17f0de60db3c3cb69bab7239b1615ef85124;p=thirdparty%2Fpdns.git Alwas detect mismatches in outgoing and incoming ECS; add tests for that as well --- diff --git a/pdns/dnsrecords.cc b/pdns/dnsrecords.cc index 7db552d863..7895a788cf 100644 --- a/pdns/dnsrecords.cc +++ b/pdns/dnsrecords.cc @@ -1036,6 +1036,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 a79bd2fbb4..da1c8cb8d7 100644 --- a/pdns/dnsrecords.hh +++ b/pdns/dnsrecords.hh @@ -1299,6 +1299,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 8783bffe1f..d7b3ab9aec 100644 --- a/pdns/iputils.hh +++ b/pdns/iputils.hh @@ -896,6 +896,7 @@ public: { return std::tie(d_network, d_bits) == std::tie(rhs.d_network, rhs.d_bits); } + bool operator!=(const Netmask& rhs) const { return !operator==(rhs); diff --git a/pdns/recursordist/RECURSOR-MIB.in b/pdns/recursordist/RECURSOR-MIB.in index 638b214df0..7c05f1517c 100644 --- a/pdns/recursordist/RECURSOR-MIB.in +++ b/pdns/recursordist/RECURSOR-MIB.in @@ -21,6 +21,9 @@ rec MODULE-IDENTITY DESCRIPTION "This MIB module describes information gathered through PowerDNS Recursor." + REVISION "202505270000Z" + DESCRIPTION "Added metric for missing ECS in reply" + REVISION "202408280000Z" DESCRIPTION "Added metric for too many incoming TCP connections" diff --git a/pdns/recursordist/RECURSOR-MIB.txt b/pdns/recursordist/RECURSOR-MIB.txt index c9b83836d3..03b0128a9d 100644 --- a/pdns/recursordist/RECURSOR-MIB.txt +++ b/pdns/recursordist/RECURSOR-MIB.txt @@ -21,6 +21,9 @@ rec MODULE-IDENTITY DESCRIPTION "This MIB module describes information gathered through PowerDNS Recursor." + REVISION "202505270000Z" + DESCRIPTION "Added metric for missing ECS in reply" + REVISION "202408280000Z" DESCRIPTION "Added metric for too many incoming TCP connections" diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index f00fa48908..c0c9d67870 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -593,37 +593,37 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& lwr->d_records.push_back(answer); } - 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 (EDNSSubnetOpts::getFromString(opt.second, &reso)) { - if (g_ECSHardening && !(reso.getSource() == subnetOpts->getSource())) { - g_slogout->info(Logr::Notice, "Incoming ECS does not match outgoing", - "server", Logging::Loggable(address), - "qname", Logging::Loggable(domain), - "outgoing", Logging::Loggable(subnetOpts->getSource()), - "incoming", Logging::Loggable(reso.getSource())); - 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.getScopePrefixLength() != 0) { - uint8_t bits = std::min(reso.getScopePrefixLength(), subnetOpts->getSourcePrefixLength()); - auto outgoingECSAddr = subnetOpts->getSource().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 (EDNSSubnetOpts::getFromString(opt->second, &reso)) { + if (!doTCP && reso.getSource() != subnetOpts->getSource()) { + g_slogout->info(Logr::Notice, "Incoming ECS does not match outgoing", + "server", Logging::Loggable(address), + "qname", Logging::Loggable(domain), + "outgoing", Logging::Loggable(subnetOpts->getSource()), + "incoming", Logging::Loggable(reso.getSource())); + 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.getScopePrefixLength() != 0) { + uint8_t bits = std::min(reso.getScopePrefixLength(), subnetOpts->getSourcePrefixLength()); + auto outgoingECSAddr = subnetOpts->getSource().getNetwork(); + outgoingECSAddr.truncate(bits); + srcmask = Netmask(outgoingECSAddr, bits); } } } diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 35bd62144b..36d7526eea 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -372,9 +372,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; } @@ -2065,7 +2066,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 (EDNSSubnetOpts::getFromString(iter->second.values.at(0).content, iter->second.values.at(0).size, &eso)) { @@ -2913,7 +2914,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; @@ -2922,8 +2923,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); @@ -2970,7 +2972,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. } } } @@ -3057,8 +3060,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 bba70b522e..32a358ec39 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -804,7 +804,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 5e718dcf20..0215c2a379 100644 --- a/regression-tests.recursor-dnssec/recursortests.py +++ b/regression-tests.recursor-dnssec/recursortests.py @@ -13,7 +13,8 @@ import unittest import dns import dns.message import requests - +import threading +from twisted.internet import reactor from proxyprotocol import ProxyProtocol from eqdnsmessage import AssertEqualDNSMessageMixin @@ -1243,3 +1244,10 @@ distributor-threads={threads} self.assertEqual(value, expected, key + ": value " + str(value) + " is not 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 f3312f2faa..14eb2de950 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() class NoECSTest(ECSTest): _confdir = 'NoECS' @@ -194,7 +197,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) @@ -221,6 +224,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' @@ -801,7 +856,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) @@ -834,8 +889,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()