]> git.ipfire.org Git - thirdparty/pdns.git/commitdiff
Alwas detect mismatches in outgoing and incoming ECS; add tests for that as well
authorOtto Moerbeek <otto.moerbeek@open-xchange.com>
Mon, 19 May 2025 12:31:38 +0000 (14:31 +0200)
committerOtto Moerbeek <otto.moerbeek@open-xchange.com>
Wed, 28 May 2025 11:25:59 +0000 (13:25 +0200)
pdns/dnsrecords.cc
pdns/dnsrecords.hh
pdns/iputils.hh
pdns/recursordist/RECURSOR-MIB.txt
pdns/recursordist/lwres.cc
pdns/recursordist/pdns_recursor.cc
pdns/recursordist/syncres.hh
regression-tests.recursor-dnssec/recursortests.py
regression-tests.recursor-dnssec/test_ECS.py

index a7c752be1fa95ab184a284596cc75598032f7c05..55e5b993c182987d85e4209a089ab5862627b858 100644 (file)
@@ -1028,6 +1028,16 @@ void checkHostnameCorrectness(const DNSResourceRecord& rr)
   }
 }
 
+vector<pair<uint16_t, string>>::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
 {
index f674dd4da9696fb6b206a7f4b6fc0e8e3995df79..1c6c37f75ebc32224b5bfae4b00acf6712f3443b 100644 (file)
@@ -1061,6 +1061,8 @@ struct EDNSOpts
   uint16_t d_packetsize{0};
   uint16_t d_extFlags{0};
   uint8_t d_extRCode, d_version;
+
+  [[nodiscard]] vector<pair<uint16_t, string>>::const_iterator getFirstOption(uint16_t optionCode) const;
 };
 //! Convenience function that fills out EDNS0 options, and returns true if there are any
 
index ba9c50d7817ec0f2379e45d7f8441f406433f951..44250afeb3b2540c8f6dab14606c930ef878b54b 100644 (file)
@@ -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;
index 99a7dc8d80d7236c08f5f2200ecc9fbdec3b0d80..01b8d6f56560a5be07f25db2830fe16a6b35c579 100644 (file)
@@ -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."
index d37d485e77f8d5b4477db5eb7f128e77790b1168..c0fd64ed89b604e5413b3af49871458d284b9917 100644 (file)
@@ -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);
             }
           }
         }
index 2cc23b2554f85cd7eac81560f4181d8569f9cad6..0f3c795c6ed8f931a7870c7f8fba91c034859ccd 100644 (file)
@@ -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<const char*>(&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<PacketID>& resend, const PacketBuffer& content, const std::optional<bool>& ecsReceived)
+static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptr<PacketID>& resend, const PacketBuffer& content, const std::optional<bool>& 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_ptr<Pac
     return;
   }
 
-  if (ecsReceived) {
-    iter->key->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);
     }
   }
 
index 13a74ace9cf9edc45af3a0db6fdc9dae535721a4..6d22624b47af81fa8ebbf51d62bee815835339be 100644 (file)
@@ -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<bool> ecsReceived; // only set in ecsHardened mode
+  std::optional<bool> matchingECSReceived; // only set in ecsHardened mode
   TCPAction highState{TCPAction::DoingRead};
   IOState lowState{IOState::NeedRead};
 
index 1385c1281132fbcbcfc9cd2d6921fd560483eca8..97a2cfc246c3aa3d2112488797a427f4cdda3a0a 100644 (file)
@@ -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()
index dde1dfe8ea8e7097bb290c28d43f7fe5bbeb9f41..7f9b15ed592dbe86e684129c0de040d1c4ed1467 100644 (file)
@@ -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()