]> git.ipfire.org Git - thirdparty/pdns.git/commitdiff
rec: Fix the evaluation order for filtering policies (RPZ)
authorRemi Gacogne <remi.gacogne@powerdns.com>
Tue, 14 Jan 2020 15:26:23 +0000 (16:26 +0100)
committerRemi Gacogne <remi.gacogne@powerdns.com>
Tue, 14 Jan 2020 15:26:23 +0000 (16:26 +0100)
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.

pdns/filterpo.cc
pdns/recursordist/test-filterpo_cc.cc

index 3cecbfff79b042adaf768e603fceba32991b2861..ef37ac3ac88b75ba07713e12dfd2d1a4bf79aae9 100644 (file)
@@ -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;
       }
     }
index 875b8ec429f0efcf5bec15c3988502761d28f793..71d4cdb1fac04bb0a7488ca78514a6d977959df6 100644 (file)
@@ -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}});