dnsdist: Fix `BPFFilter::addRangeRule`

Reported by Prasanna Dabi (thanks!):
"The eBPF DDoS mitigation implementation in dnsdist contains a critical logic error that prevents new range-based block rules from being applied. When the BPFFilter::addRangeRule() function is called to block a subnet, it first checks the eBPF map to determine if the rule already exists. If the subnet is not currently in the map, the bpf_lookup_elem call returns -1. In this failure state, the local CounterAndActionValue value struct remains in its default, zeroed-out state, where the action field is automatically set to BPFFilter::MatchAction::Pass.

The conditional check intended to skip redundant rules contains a logic typo: it evaluates value.action == BPFFilter::MatchAction::Pass instead of comparing the requested action parameter.Because the default state of the unpopulated struct is always Pass, the condition (res == -1 && value.action == BPFFilter::MatchAction::Pass) evaluates to true for every new rule attempt.This causes the daemon to throw a std::runtime_error and reject the mitigation."

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>

diff --git a/pdns/dnsdistdist/bpf-filter.cc b/pdns/dnsdistdist/bpf-filter.cc
index e651a75b53ed526327586ae00c7df254ff556b3d..b5072cb58e20e2e71f0cdcb106ea0c8606ff7dcf 100644 (file)
--- a/pdns/dnsdistdist/bpf-filter.cc
+++ b/pdns/dnsdistdist/bpf-filter.cc
@@ -589,7 +589,7 @@ void BPFFilter::addRangeRule(const Netmask& addr, bool force, BPFFilter::MatchAc
     }
 
     res = bpf_lookup_elem(map.d_fd.getHandle(), &key, &value);
-    if (((res != -1 && value.action == action) || (res == -1 && value.action == BPFFilter::MatchAction::Pass)) && !force) {
+    if (((res != -1 && value.action == action) || (res == -1 && action == BPFFilter::MatchAction::Pass)) && !force) {
       throw std::runtime_error("Trying to add a useless rule: " + addr.toString());
     }
 
@@ -614,7 +614,7 @@ void BPFFilter::addRangeRule(const Netmask& addr, bool force, BPFFilter::MatchAc
     }
 
     res = bpf_lookup_elem(map.d_fd.getHandle(), &key, &value);
-    if (((res != -1 && value.action == action) || (res == -1 && value.action == BPFFilter::MatchAction::Pass)) && !force) {
+    if (((res != -1 && value.action == action) || (res == -1 && action == BPFFilter::MatchAction::Pass)) && !force) {
       throw std::runtime_error("Trying to add a useless rule: " + addr.toString());
     }
 

diff --git a/regression-tests.dnsdist/test_EBPF.py b/regression-tests.dnsdist/test_EBPF.py
index 762f2853504b4f9ad8c590dd98cdb34e5976731f..c4c6403239ed71762ae1c7f56823bf06bcb4089e 100644 (file)
--- a/regression-tests.dnsdist/test_EBPF.py
+++ b/regression-tests.dnsdist/test_EBPF.py
@@ -175,3 +175,35 @@ class TestSimpleEBPF(DNSDistTest):
 
         # unblock 127.0.0.1
         self.sendConsoleCommand('bpf:unblock(newCA("127.0.0.1"))')
+
+
+@unittest.skipUnless("ENABLE_SUDO_TESTS" in os.environ, "sudo is not available")
+class TestEBPFRange(DNSDistTest):
+    _config_template = """
+    newServer{address="127.0.0.1:%d"}
+
+    bpf = newBPFFilter({ipv4MaxItems=10, ipv6MaxItems=10, qnamesMaxItems=10, cidr4MaxItems=10, cidr6MaxItems=10, external=true})
+    -- note that this is NOT enforced by DNSdist itself, and we are not going to load a XDP program, but at the very least we check that the maps have been created and entries can be inserted
+    bpf:addRangeRule("192.0.2.1/8", 1)
+    bpf:addRangeRule("2001:db8::1/32", 2)
+    """
+    _config_params = [
+        "_testServerPort",
+    ]
+    _sudoMode = True
+
+    def testNotBlocked(self):
+        name = "simplea.range-ebpf.tests.powerdns.com."
+        query = dns.message.make_query(name, "A", "IN", use_edns=False)
+        response = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name, 3600, dns.rdataclass.IN, dns.rdatatype.A, "127.0.0.1")
+        response.answer.append(rrset)
+        for method in [
+            "sendUDPQuery",
+            "sendTCPQuery",
+        ]:
+            sender = getattr(self, method)
+            (receivedQuery, receivedResponse) = sender(query, response, timeout=1)
+            receivedQuery.id = query.id
+            self.assertEqual(query, receivedQuery)
+            self.assertEqual(response, receivedResponse)