]> 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 14:21:17 +0000 (16:21 +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 1cf4c2c82845799a98919dbf9d758089de35cacd..113bb63d3e9b0b28cf88581784252a9eac688d8d 100644 (file)
@@ -1016,6 +1016,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 dc2a1ce60048c0658f054702213e338c097f0b1c..8ea1a75cc08ec2aeae7f43b2ce2a68d85258f7c0 100644 (file)
@@ -1065,6 +1065,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 47ca071a4780a978821a8371dd1e7cb8df19cb22..0b7acd082ffb863179c05a76e03a6e321912906e 100644 (file)
@@ -656,7 +656,12 @@ public:
     return std::tie(d_network, d_bits) == std::tie(rhs.d_network, rhs.d_bits);
   }
 
-  bool empty() const
+  bool operator!=(const Netmask& rhs) const
+  {
+    return !(*this == rhs);
+  }
+
+  [[nodiscard]] bool empty() const
   {
     return d_network.sin4.sin_family==0;
   }
index c41fa681df9c1695f5417f3037f71142f7f148cd..4f8f8894d3bef84b66791075a712bad88ba6510f 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 11cf2fa90a4f881e957f5986618054e9dc102012..ff1da3af29139fe048d738ec27cace6d489ab8bf 100644 (file)
@@ -577,37 +577,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 848c75295ea8334b31bc4786fda596f88bb66b9d..f4530cfc7f9bd837b47012e9b0c73dc3c40410c5 100644 (file)
@@ -347,9 +347,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;
     }
@@ -2048,7 +2049,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)) {
@@ -2860,7 +2861,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;
@@ -2869,8 +2870,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;
   }
   for (auto i = iter->key->chain.begin(); i != iter->key->chain.end(); ++i) {
     auto packetID = std::make_shared<PacketID>(*resend);
@@ -2910,7 +2912,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.
       }
     }
   }
@@ -2997,8 +3000,10 @@ static void handleUDPServerResponse(int fileDesc, FDMultiplexer::funcparam_t& va
   if (!pident->domain.empty()) {
     auto iter = g_multiTasker->d_waiters.find(pident);
     if (iter != g_multiTasker->d_waiters.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 7f95d253e44a132aa7046aefe36d965224e08d74..9eef320a9cdeb3b2b0a99d84ebbe709849c7b335 100644 (file)
@@ -776,7 +776,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 1bb989c000ef9bfa5ed8ddc79b67e5bc820a307d..ead4de293fd6e7d1ed22e5ee298a302c275197ac 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
@@ -1186,3 +1188,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 ef43627dc7a0f8a927928f85301229293e0b0e5d..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.setDaemon(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()