rec: Only the first filtering policy should match

Subsequent ones should not be applied.
Also make sure that NSDNAME and NSIP triggers really stop the
processing of the query, instead of just causing the current NS to
be skipped.

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index fb7d8eb35cd6a151d08c7b4991f6f172088ea633..9dd73d3b36beda4d25917438cbc018cc8f8f2692 100644 (file)
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -1324,7 +1324,7 @@ static void startDoResolve(void *p)
     }
 
     // Check if the query has a policy attached to it
-    if (wantsRPZ) {
+    if (wantsRPZ && appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) {
       appliedPolicy = luaconfsLocal->dfe.getQueryPolicy(dc->d_mdp.d_qname, dc->d_source, sr.d_discardedPolicies);
     }
 
@@ -1370,15 +1370,19 @@ static void startDoResolve(void *p)
 
       // Query got not handled for QNAME Policy reasons, now actually go out to find an answer
       try {
+        sr.d_appliedPolicy = appliedPolicy;
         res = sr.beginResolve(dc->d_mdp.d_qname, QType(dc->d_mdp.d_qtype), dc->d_mdp.d_qclass, ret);
         shouldNotValidate = sr.wasOutOfBand();
       }
-      catch(ImmediateServFailException &e) {
-        if(g_logCommonErrors)
+      catch(const ImmediateServFailException &e) {
+        if(g_logCommonErrors) {
           g_log<<Logger::Notice<<"Sending SERVFAIL to "<<dc->getRemote()<<" during resolve of '"<<dc->d_mdp.d_qname<<"' because: "<<e.reason<<endl;
+        }
         res = RCode::ServFail;
       }
-
+      catch(const PolicyHitException& e) {
+        res = -2;
+      }
       dq.validationState = sr.getValidationState();
 
       // During lookup, an NSDNAME or NSIP trigger was hit in RPZ
@@ -1422,7 +1426,7 @@ static void startDoResolve(void *p)
         }
       }
 
-      if (wantsRPZ) {
+      if (wantsRPZ && appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) {
         appliedPolicy = luaconfsLocal->dfe.getPostPolicy(ret, sr.d_discardedPolicies);
       }
 
@@ -1557,7 +1561,7 @@ static void startDoResolve(void *p)
             }
           }
         }
-        catch(ImmediateServFailException &e) {
+        catch(const ImmediateServFailException &e) {
           if(g_logCommonErrors)
             g_log<<Logger::Notice<<"Sending SERVFAIL to "<<dc->getRemote()<<" during validation of '"<<dc->d_mdp.d_qname<<"|"<<QType(dc->d_mdp.d_qtype).getName()<<"' because: "<<e.reason<<endl;
           pw.getHeader()->rcode=RCode::ServFail;
@@ -2936,18 +2940,21 @@ static void houseKeeping(void *)
        try {
          doSecPoll(&last_secpoll);
        }
-       catch(std::exception& e)
+       catch(const std::exception& e)
         {
           g_log<<Logger::Error<<"Exception while performing security poll: "<<e.what()<<endl;
         }
-        catch(PDNSException& e)
+        catch(const PDNSException& e)
         {
           g_log<<Logger::Error<<"Exception while performing security poll: "<<e.reason<<endl;
         }
-        catch(ImmediateServFailException &e)
+        catch(const ImmediateServFailException &e)
         {
           g_log<<Logger::Error<<"Exception while performing security poll: "<<e.reason<<endl;
         }
+        catch(const PolicyHitException& e) {
+          g_log<<Logger::Error<<"Policy hit while performing security poll"<<endl;
+        }
         catch(...)
         {
           g_log<<Logger::Error<<"Exception while performing security poll"<<endl;

diff --git a/pdns/recursordist/test-syncres_cc3.cc b/pdns/recursordist/test-syncres_cc3.cc
index 2a544ac886a7d4a45f291aae162b5a2cb826e2a7..bc6497c2d0114e3e79ca4e29cced6ba0eb27db8e 100644 (file)
--- a/pdns/recursordist/test-syncres_cc3.cc
+++ b/pdns/recursordist/test-syncres_cc3.cc
@@ -521,9 +521,7 @@ BOOST_AUTO_TEST_CASE(test_nameserver_ipv4_rpz)
   g_luaconfs.setState(luaconfsCopy);
 
   vector<DNSRecord> ret;
-  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
-  BOOST_CHECK_EQUAL(res, -2);
-  BOOST_CHECK_EQUAL(ret.size(), 0U);
+  BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::A), QClass::IN, ret), PolicyHitException);
 }
 
 BOOST_AUTO_TEST_CASE(test_nameserver_ipv6_rpz)
@@ -564,9 +562,7 @@ BOOST_AUTO_TEST_CASE(test_nameserver_ipv6_rpz)
   g_luaconfs.setState(luaconfsCopy);
 
   vector<DNSRecord> ret;
-  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
-  BOOST_CHECK_EQUAL(res, -2);
-  BOOST_CHECK_EQUAL(ret.size(), 0U);
+  BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::A), QClass::IN, ret), PolicyHitException);
 }
 
 BOOST_AUTO_TEST_CASE(test_nameserver_name_rpz)
@@ -608,9 +604,7 @@ BOOST_AUTO_TEST_CASE(test_nameserver_name_rpz)
   g_luaconfs.setState(luaconfsCopy);
 
   vector<DNSRecord> ret;
-  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
-  BOOST_CHECK_EQUAL(res, -2);
-  BOOST_CHECK_EQUAL(ret.size(), 0U);
+  BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::A), QClass::IN, ret), PolicyHitException);
 }
 
 BOOST_AUTO_TEST_CASE(test_nameserver_name_rpz_disabled)

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index e4a3b962eef8bc39da1e1b29ea725941eb4c5217..486bd2bee5e3c78197e029297e76e290b9ca6e23 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -733,6 +733,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector<DNSRecor
         QLOG("Delegation seen, continue at step 1");
         break;
       }
+
       if (res != RCode::NoError) {
         // Case 5: unexpected answer
         QLOG("Step5: other rcode, last effort final resolve");
@@ -923,46 +924,52 @@ vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth,
   d_requireAuthData = false;
   d_DNSSECValidationRequested = false;
 
-  vState newState = Indeterminate;
-  res_t resv4;
-  // If IPv4 ever becomes second class, we should revisit this
-  if (doResolve(qname, QType::A, resv4, depth+1, beenthere, newState) == 0) {  // this consults cache, OR goes out
-    for (auto const &i : resv4) {
-      if (i.d_type == QType::A) {
-        if (auto rec = getRR<ARecordContent>(i)) {
-          ret.push_back(rec->getCA(53));
+  try {
+    vState newState = Indeterminate;
+    res_t resv4;
+    // If IPv4 ever becomes second class, we should revisit this
+    if (doResolve(qname, QType::A, resv4, depth+1, beenthere, newState) == 0) {  // this consults cache, OR goes out
+      for (auto const &i : resv4) {
+        if (i.d_type == QType::A) {
+          if (auto rec = getRR<ARecordContent>(i)) {
+            ret.push_back(rec->getCA(53));
+          }
         }
       }
     }
-  }
-  if (s_doIPv6) {
-    if (ret.empty()) {
-      // We did not find IPv4 addresses, try to get IPv6 ones
-      newState = Indeterminate;
-      res_t resv6;
-      if (doResolve(qname, QType::AAAA, resv6, depth+1, beenthere, newState) == 0) {  // this consults cache, OR goes out
-        for (const auto &i : resv6) {
-          if (i.d_type == QType::AAAA) {
-            if (auto rec = getRR<AAAARecordContent>(i))
-              ret.push_back(rec->getCA(53));
+    if (s_doIPv6) {
+      if (ret.empty()) {
+        // We did not find IPv4 addresses, try to get IPv6 ones
+        newState = Indeterminate;
+        res_t resv6;
+        if (doResolve(qname, QType::AAAA, resv6, depth+1, beenthere, newState) == 0) {  // this consults cache, OR goes out
+          for (const auto &i : resv6) {
+            if (i.d_type == QType::AAAA) {
+              if (auto rec = getRR<AAAARecordContent>(i))
+                ret.push_back(rec->getCA(53));
+            }
           }
         }
-      }
-    } else {
-      // We have some IPv4 records, don't bother with going out to get IPv6, but do consult the cache
-      // Once IPv6 adoption matters, this needs to be revisited
-      res_t cset;
-      if (t_RC->get(d_now.tv_sec, qname, QType(QType::AAAA), false, &cset, d_cacheRemote) > 0) {
-        for (const auto &i : cset) {
-          if (i.d_ttl > (unsigned int)d_now.tv_sec ) {
-            if (auto rec = getRR<AAAARecordContent>(i)) {
-              ret.push_back(rec->getCA(53));
+      } else {
+        // We have some IPv4 records, don't bother with going out to get IPv6, but do consult the cache
+        // Once IPv6 adoption matters, this needs to be revisited
+        res_t cset;
+        if (t_RC->get(d_now.tv_sec, qname, QType(QType::AAAA), false, &cset, d_cacheRemote) > 0) {
+          for (const auto &i : cset) {
+            if (i.d_ttl > (unsigned int)d_now.tv_sec ) {
+              if (auto rec = getRR<AAAARecordContent>(i)) {
+                ret.push_back(rec->getCA(53));
+              }
             }
           }
         }
       }
     }
   }
+  catch (const PolicyHitException& e) {
+    /* we ignore a policy hit while trying to retrieve the addresses
+       of a NS and keep processing the current query */
+  }
 
   d_requireAuthData = oldRequireAuthData;
   d_DNSSECValidationRequested = oldValidationRequested;
@@ -1790,7 +1797,13 @@ static void addNXNSECS(vector<DNSRecord>&ret, const vector<DNSRecord>& records)
 
 bool SyncRes::nameserversBlockedByRPZ(const DNSFilterEngine& dfe, const NsSet& nameservers)
 {
-  if(d_wantsRPZ) {
+  /* we skip RPZ processing if:
+     - it was disabled (d_wantsRPZ is false) ;
+     - we already got a RPZ hit (d_appliedPolicy.d_type != DNSFilterEngine::PolicyType::None) since
+     the only way we can get back here is that it was a 'pass-thru' (NoAction) meaning that we should not
+     process any further RPZ rules.
+  */
+  if (d_wantsRPZ && d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) {
     for (auto const &ns : nameservers) {
       d_appliedPolicy = dfe.getProcessingPolicy(ns.first, d_discardedPolicies);
       if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
@@ -1813,7 +1826,13 @@ bool SyncRes::nameserversBlockedByRPZ(const DNSFilterEngine& dfe, const NsSet& n
 
 bool SyncRes::nameserverIPBlockedByRPZ(const DNSFilterEngine& dfe, const ComboAddress& remoteIP)
 {
-  if (d_wantsRPZ) {
+  /* we skip RPZ processing if:
+     - it was disabled (d_wantsRPZ is false) ;
+     - we already got a RPZ hit (d_appliedPolicy.d_type != DNSFilterEngine::PolicyType::None) since
+     the only way we can get back here is that it was a 'pass-thru' (NoAction) meaning that we should not
+     process any further RPZ rules.
+  */
+  if (d_wantsRPZ && d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) {
     d_appliedPolicy = dfe.getProcessingPolicy(remoteIP, d_discardedPolicies);
     if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) {
       LOG(" (blocked by RPZ policy '"+(d_appliedPolicy.d_name ? *d_appliedPolicy.d_name : "")+"')");
@@ -3385,12 +3404,11 @@ bool SyncRes::processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qn
 
     nameservers.clear();
     for (auto const &nameserver : nsset) {
-      if (d_wantsRPZ) {
+      if (d_wantsRPZ && d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) {
         d_appliedPolicy = dfe.getProcessingPolicy(nameserver, d_discardedPolicies);
         if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response
           LOG("however "<<nameserver<<" was blocked by RPZ policy '"<<(d_appliedPolicy.d_name ? *d_appliedPolicy.d_name : "")<<"'"<<endl);
-          *rcode = -2;
-          return true;
+          throw PolicyHitException();
         }
       }
       nameservers.insert({nameserver, {{}, false}});
@@ -3424,7 +3442,8 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con
   LOG(prefix<<qname<<": Cache consultations done, have "<<(unsigned int)nameservers.size()<<" NS to contact");
 
   if (nameserversBlockedByRPZ(luaconfsLocal->dfe, nameservers)) {
-    return -2;
+    /* RPZ hit */
+    throw PolicyHitException();
   }
 
   LOG(endl);
@@ -3508,8 +3527,10 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con
             }
           }
           LOG(endl);
-          if (hitPolicy) //implies d_wantsRPZ
-            return -2;
+          if (hitPolicy) { //implies d_wantsRPZ
+            /* RPZ hit */
+            throw PolicyHitException();
+          }
         }
 
         for(remoteIP = remoteIPs.cbegin(); remoteIP != remoteIPs.cend(); ++remoteIP) {
@@ -3663,6 +3684,10 @@ int directResolve(const DNSName& qname, const QType& qtype, int qclass, vector<D
     g_log<<Logger::Error<<"Failed to resolve "<<qname.toLogString()<<", got ImmediateServFailException: "<<e.reason<<endl;
     ret.clear();
   }
+  catch(const PolicyHitException& e) {
+    g_log<<Logger::Error<<"Failed to resolve "<<qname.toLogString()<<", got a policy hit"<<endl;
+    ret.clear();
+  }
   catch(const std::exception& e) {
     g_log<<Logger::Error<<"Failed to resolve "<<qname.toLogString()<<", got STL error: "<<e.what()<<endl;
     ret.clear();
@@ -3700,6 +3725,10 @@ int SyncRes::getRootNS(struct timeval now, asyncresolve_t asyncCallback) {
   catch(const ImmediateServFailException& e) {
     g_log<<Logger::Error<<"Failed to update . records, got an exception: "<<e.reason<<endl;
   }
+  catch(const PolicyHitException& e) {
+    g_log<<Logger::Error<<"Failed to update . records, got a policy hit"<<endl;
+    ret.clear();
+  }
   catch(const std::exception& e) {
     g_log<<Logger::Error<<"Failed to update . records, got an exception: "<<e.what()<<endl;
   }

diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 2e0c399b494e8921faee52d464b392301ce3ef18..0d8aef22bafa4870298ed66620c5ae81a1cc02ac 100644 (file)
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -1054,6 +1054,10 @@ public:
   string reason; //! Print this to tell the user what went wrong
 };
 
+class PolicyHitException
+{
+};
+
 typedef boost::circular_buffer<ComboAddress> addrringbuf_t;
 extern thread_local std::unique_ptr<addrringbuf_t> t_servfailremotes, t_largeanswerremotes, t_remotes, t_bogusremotes, t_timeouts;