From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Wed, 20 Oct 2021 15:45:59 +0000 (+0200)
Subject: dnsdist: Fix a bug in the Dynamic Blocks when port ranges are used
X-Git-Tag: rec-4.6.0-beta1~39^2~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=008c061d4db00438696495b86d037def6744684b;p=thirdparty%2Fpdns.git

dnsdist: Fix a bug in the Dynamic Blocks when port ranges are used

Thanks Otto!
---

diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc
index 3ce9c065e9..07ee8f97c1 100644
--- a/pdns/dnsdist-lua.cc
+++ b/pdns/dnsdist-lua.cc
@@ -1392,6 +1392,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck)
                          until.tv_sec += actualSeconds;
                          for (const auto& capair : m) {
                            unsigned int count = 0;
+                           /* this legacy interface does not support ranges or ports, use DynBlockRulesGroup instead */
                            AddressAndPortRange requestor(capair.first, capair.first.isIPv4() ? 32 : 128, 0);
                            auto got = slow.lookup(requestor);
                            bool expired = false;
diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 10064e9bc6..41e8bd3745 100644
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -861,6 +861,7 @@ static bool applyRulesToQuery(LocalHolders& holders, DNSQuestion& dq, const stru
     }
   }
 
+  /* the Dynamic Block mechanism supports address and port ranges, so we need to pass the full address and port */
   if (auto got = holders.dynNMGBlock->lookup(AddressAndPortRange(*dq.remote, dq.remote->isIPv4() ? 32 : 128, 16))) {
     auto updateBlockStats = [&got]() {
       ++g_stats.dynBlocked;
diff --git a/pdns/dnsdistdist/dnsdist-dynblocks.cc b/pdns/dnsdistdist/dnsdist-dynblocks.cc
index a1230f1017..dccf2aa8c1 100644
--- a/pdns/dnsdistdist/dnsdist-dynblocks.cc
+++ b/pdns/dnsdistdist/dnsdist-dynblocks.cc
@@ -176,6 +176,7 @@ bool DynBlockRulesGroup::checkIfResponseCodeMatches(const Rings::Response& respo
 
 void DynBlockRulesGroup::addOrRefreshBlock(boost::optional<NetmaskTree<DynBlock, AddressAndPortRange> >& blocks, const struct timespec& now, const AddressAndPortRange& requestor, const DynBlockRule& rule, bool& updated, bool warning)
 {
+  /* network exclusions are address-based only (no port) */
   if (d_excludedSubnets.match(requestor.getNetwork())) {
     /* do not add a block for excluded subnets */
     return;
@@ -187,7 +188,7 @@ void DynBlockRulesGroup::addOrRefreshBlock(boost::optional<NetmaskTree<DynBlock,
   struct timespec until = now;
   until.tv_sec += rule.d_blockDuration;
   unsigned int count = 0;
-  const auto& got = blocks->lookup(requestor.getNetwork());
+  const auto& got = blocks->lookup(requestor);
   bool expired = false;
   bool wasWarning = false;
   bool bpf = false;
@@ -226,6 +227,7 @@ void DynBlockRulesGroup::addOrRefreshBlock(boost::optional<NetmaskTree<DynBlock,
     if (db.action == DNSAction::Action::Drop && g_defaultBPFFilter &&
         ((requestor.isIPv4() && requestor.getBits() == 32) || (requestor.isIPv6() && requestor.getBits() == 128))) {
       try {
+        /* the current BPF filter implementation only supports full addresses (/32 or /128) and no port */
         g_defaultBPFFilter->block(requestor.getNetwork());
         bpf = true;
       }
diff --git a/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc b/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
index 3193e6692b..f556a6048c 100644
--- a/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
+++ b/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
@@ -346,6 +346,39 @@ BOOST_AUTO_TEST_CASE(test_DynBlockRulesGroup_QueryRate_V4Ports) {
       /* outside of the range should not */
       BOOST_CHECK(g_dynblockNMG.getLocal()->lookup(AddressAndPortRange(ComboAddress("192.0.2.1:16384"), 32, 16)) == nullptr);
     }
+
+    /* we (again) insert just above 50 qps from several clients the same IPv4 port range, this should update the block which will
+       check by looking at the blocked counter */
+    {
+      auto block = g_dynblockNMG.getLocal()->lookup(AddressAndPortRange(ComboAddress("192.0.2.1:0"), 32, 16));
+      BOOST_REQUIRE(block != nullptr);
+      BOOST_CHECK_EQUAL(block->second.blocks, 0U);
+      block->second.blocks = 42U;
+    }
+
+    g_rings.clear();
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfQueryEntries(), 0U);
+
+    for (size_t idx = 0; idx < numberOfQueries; idx++) {
+      ComboAddress requestor("192.0.2.1:" + std::to_string(idx));
+      g_rings.insertQuery(now, requestor, qname, qtype, size, dh, protocol);
+      g_rings.insertResponse(now, requestor, qname, qtype, responseTime, size, dh, backend, outgoingProtocol);
+    }
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfQueryEntries(), numberOfQueries);
+
+    dbrg.apply(now);
+
+    BOOST_CHECK_EQUAL(g_dynblockNMG.getLocal()->size(), 1U);
+    {
+      /* previous address/port should still be blocked */
+      auto block = g_dynblockNMG.getLocal()->lookup(AddressAndPortRange(ComboAddress("192.0.2.1:0"), 32, 16));
+      BOOST_REQUIRE(block != nullptr);
+      BOOST_CHECK_EQUAL(block->second.blocks, 42U);
+    }
+
+    /* but not a different one */
+    BOOST_CHECK(g_dynblockNMG.getLocal()->lookup(AddressAndPortRange(ComboAddress("192.0.2.1:16384"), 32, 16)) == nullptr);
+
   }
 }