From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 14 Jan 2020 15:26:23 +0000 (+0100)
Subject: rec: Fix the evaluation order for filtering policies (RPZ)
X-Git-Tag: auth-4.3.0-beta1~30^2~5
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=06cfa23ffbc7ae170eee31f8130d74f0b07ec9d9;p=thirdparty%2Fpdns.git

rec: Fix the evaluation order for filtering policies (RPZ)

Since 272e9a0034e8c5ea29d1ab7d24630424f178e926 we scanned all policies
for an exact match before looking for wildcard matches. It brokes
the promise that filtering policies are evaluated in the order they
are defined.
---

diff --git a/pdns/filterpo.cc b/pdns/filterpo.cc
index 3cecbfff79..ef37ac3ac8 100644
--- a/pdns/filterpo.cc
+++ b/pdns/filterpo.cc
@@ -141,27 +141,37 @@ DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const DNSName& qnam
   }
 
   Policy pol;
-  if (!allEmpty) {
-    count = 0;
-    for(const auto& z : d_zones) {
-      if (zoneEnabled[count] && z->findExactNSPolicy(qname, pol)) {
-        // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
-        return pol;
-      }
+  if (allEmpty) {
+    return pol;
+  }
+
+  /* prepare the wildcard-based names */
+  std::vector<DNSName> wcNames;
+  wcNames.reserve(qname.countLabels());
+  DNSName s(qname);
+  while (s.chopOff()){
+    wcNames.emplace_back(g_wildcarddnsname+s);
+  }
+
+  count = 0;
+  for(const auto& z : d_zones) {
+    if (!zoneEnabled[count]) {
       ++count;
+      continue;
     }
 
-    DNSName s(qname);
-    while(s.chopOff()){
-      count = 0;
-      for(const auto& z : d_zones) {
-        if (zoneEnabled[count] && z->findExactNSPolicy(g_wildcarddnsname+s, pol)) {
-          // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
-          return pol;
-        }
-        ++count;
+    if (z->findExactNSPolicy(qname, pol)) {
+      // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
+      return pol;
+    }
+
+    for (const auto& wc : wcNames) {
+      if (z->findExactNSPolicy(wc, pol)) {
+        // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
+        return pol;
       }
     }
+    ++count;
   }
 
   return pol;
@@ -187,7 +197,7 @@ DNSFilterEngine::Policy DNSFilterEngine::getProcessingPolicy(const ComboAddress&
 
 DNSFilterEngine::Policy DNSFilterEngine::getQueryPolicy(const DNSName& qname, const ComboAddress& ca, const std::unordered_map<std::string,bool>& discardedPolicies) const
 {
-  //  cout<<"Got question for "<<qname<<" from "<<ca.toString()<<endl;
+  // cout<<"Got question for "<<qname<<" from "<<ca.toString()<<endl;
   std::vector<bool> zoneEnabled(d_zones.size());
   size_t count = 0;
   bool allEmpty = true;
@@ -198,7 +208,7 @@ DNSFilterEngine::Policy DNSFilterEngine::getQueryPolicy(const DNSName& qname, co
       enabled = false;
     }
     else {
-      if (z->hasQNamePolicies()) {
+      if (z->hasQNamePolicies() || z->hasClientPolicies()) {
         allEmpty = false;
       }
       else {
@@ -211,35 +221,42 @@ DNSFilterEngine::Policy DNSFilterEngine::getQueryPolicy(const DNSName& qname, co
   }
 
   Policy pol;
-  if (!allEmpty) {
-    count = 0;
-    for(const auto& z : d_zones) {
-      if (zoneEnabled[count] && z->findExactQNamePolicy(qname, pol)) {
-        //      cerr<<"Had a hit on the name of the query"<<endl;
-        return pol;
-      }
+  if (allEmpty) {
+    return pol;
+  }
+
+  /* prepare the wildcard-based names */
+  std::vector<DNSName> wcNames;
+  wcNames.reserve(qname.countLabels());
+  DNSName s(qname);
+  while (s.chopOff()){
+    wcNames.emplace_back(g_wildcarddnsname+s);
+  }
+
+  count = 0;
+  for (const auto& z : d_zones) {
+    if (!zoneEnabled[count]) {
       ++count;
+      continue;
     }
 
-    DNSName s(qname);
-    while(s.chopOff()){
-      count = 0;
-      for(const auto& z : d_zones) {
-        if (zoneEnabled[count] && z->findExactQNamePolicy(g_wildcarddnsname+s, pol)) {
-          //      cerr<<"Had a hit on the name of the query"<<endl;
-          return pol;
-        }
-        ++count;
+    if (z->findExactQNamePolicy(qname, pol)) {
+      // cerr<<"Had a hit on the name of the query"<<endl;
+      return pol;
+    }
+
+    for (const auto& wc : wcNames) {
+      if (z->findExactQNamePolicy(wc, pol)) {
+        // cerr<<"Had a hit on the name of the query"<<endl;
+        return pol;
       }
     }
-  }
 
-  count = 0;
-  for(const auto& z : d_zones) {
-    if (zoneEnabled[count] && z->findClientPolicy(ca, pol)) {
-      //	cerr<<"Had a hit on the IP address ("<<ca.toString()<<") of the client"<<endl;
+    if (z->findClientPolicy(ca, pol)) {
+      // cerr<<"Had a hit on the IP address ("<<ca.toString()<<") of the client"<<endl;
       return pol;
     }
+
     ++count;
   }
 
@@ -250,10 +267,10 @@ DNSFilterEngine::Policy DNSFilterEngine::getPostPolicy(const vector<DNSRecord>&
 {
   Policy pol;
   ComboAddress ca;
-  for(const auto& r : records) {
-    if(r.d_place != DNSResourceRecord::ANSWER)
+  for (const auto& r : records) {
+    if (r.d_place != DNSResourceRecord::ANSWER)
       continue;
-    if(r.d_type == QType::A) {
+    if (r.d_type == QType::A) {
       if (auto rec = getRR<ARecordContent>(r)) {
         ca = rec->getCA();
       }
@@ -266,13 +283,13 @@ DNSFilterEngine::Policy DNSFilterEngine::getPostPolicy(const vector<DNSRecord>&
     else
       continue;
 
-    for(const auto& z : d_zones) {
+    for (const auto& z : d_zones) {
       const auto zoneName = z->getName();
-      if(zoneName && discardedPolicies.find(*zoneName) != discardedPolicies.end()) {
+      if (zoneName && discardedPolicies.find(*zoneName) != discardedPolicies.end()) {
         continue;
       }
 
-      if(z->findResponsePolicy(ca, pol)) {
+      if (z->findResponsePolicy(ca, pol)) {
 	return pol;
       }
     }
diff --git a/pdns/recursordist/test-filterpo_cc.cc b/pdns/recursordist/test-filterpo_cc.cc
index 875b8ec429..71d4cdb1fa 100644
--- a/pdns/recursordist/test-filterpo_cc.cc
+++ b/pdns/recursordist/test-filterpo_cc.cc
@@ -428,9 +428,13 @@ BOOST_AUTO_TEST_CASE(test_multiple_filter_policies)
   zone2->setName("Unit test policy 1");
 
   const DNSName bad("bad.example.com.");
+  const DNSName badWildcard("*.bad-wildcard.example.com.");
+  const DNSName badUnderWildcard("sub.bad-wildcard.example.com.");
 
   zone1->addQNameTrigger(bad, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::QName, 0, nullptr, {DNSRecordContent::mastermake(QType::CNAME, QClass::IN, "garden1.example.net.")}));
   zone2->addQNameTrigger(bad, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::QName, 0, nullptr, {DNSRecordContent::mastermake(QType::CNAME, QClass::IN, "garden2.example.net.")}));
+  zone1->addQNameTrigger(badWildcard, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::QName, 0, nullptr, {DNSRecordContent::mastermake(QType::CNAME, QClass::IN, "garden1.example.net.")}));
+  zone2->addQNameTrigger(badUnderWildcard, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::QName, 0, nullptr, {DNSRecordContent::mastermake(QType::CNAME, QClass::IN, "garden2.example.net.")}));
 
   dfe.addZone(zone1);
   dfe.addZone(zone2);
@@ -450,6 +454,21 @@ BOOST_AUTO_TEST_CASE(test_multiple_filter_policies)
     BOOST_CHECK_EQUAL(content->getTarget().toString(), "garden1.example.net.");
   }
 
+  {
+    /* zone 2 has an exact match for badUnderWildcard, but the wildcard from the first zone should match first */
+    const auto matchingPolicy = dfe.getQueryPolicy(badUnderWildcard, ComboAddress("192.0.2.142"), std::unordered_map<std::string, bool>());
+    BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::QName);
+    BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Custom);
+    auto records = matchingPolicy.getCustomRecords(badUnderWildcard, QType::A);
+    BOOST_CHECK_EQUAL(records.size(), 1U);
+    const auto& record = records.at(0);
+    BOOST_CHECK(record.d_type == QType::CNAME);
+    BOOST_CHECK(record.d_class == QClass::IN);
+    auto content = std::dynamic_pointer_cast<CNAMERecordContent>(record.d_content);
+    BOOST_CHECK(content != nullptr);
+    BOOST_CHECK_EQUAL(content->getTarget().toString(), "garden1.example.net.");
+  }
+
   {
     /* zone 1 should still match if zone 2 has been disabled */
     const auto matchingPolicy = dfe.getQueryPolicy(bad, ComboAddress("192.0.2.142"), {{*(zone2->getName()), true}});