Limit the number of queries sent out to get NS addresses per query.

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 712c9cc6cbe97b44dc88eeb8fc3090ffba900cac..c0752344088b0083cbb7819cb7b104a88770e572 100644 (file)
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -4235,6 +4235,7 @@ static int serviceMain(int argc, char*argv[])
   SyncRes::s_serverdownthrottletime=::arg().asNum("server-down-throttle-time");
   SyncRes::s_serverID=::arg()["server-id"];
   SyncRes::s_maxqperq=::arg().asNum("max-qperq");
+  SyncRes::s_maxnsaddressqperq=::arg().asNum("max-ns-address-qperq");
   SyncRes::s_maxtotusec=1000*::arg().asNum("max-total-msec");
   SyncRes::s_maxdepth=::arg().asNum("max-recursion-depth");
   SyncRes::s_rootNXTrust = ::arg().mustDo( "root-nx-trust");
@@ -5011,6 +5012,7 @@ int main(int argc, char **argv)
     ::arg().set("edns-outgoing-bufsize", "Outgoing EDNS buffer size")="1232";
     ::arg().set("minimum-ttl-override", "Set under adverse conditions, a minimum TTL")="0";
     ::arg().set("max-qperq", "Maximum outgoing queries per query")="60";
+    ::arg().set("max-ns-address-qperq", "Maximum outgoing NS address queries per query")="10";
     ::arg().set("max-total-msec", "Maximum total wall-clock time per query in milliseconds, 0 for unlimited")="7000";
     ::arg().set("max-recursion-depth", "Maximum number of internal recursion calls per query, 0 for unlimited")="40";
     ::arg().set("max-udp-queries-per-round", "Maximum number of UDP queries processed per recvmsg() round, before returning back to normal processing")="10000";

diff --git a/pdns/recursordist/docs/settings.rst b/pdns/recursordist/docs/settings.rst
index bec850bd0b4a58657e50b5cdee0e2ea25378f181..91f84fb2e781e5daa7d316e15002dbc00cc12f02 100644 (file)
--- a/pdns/recursordist/docs/settings.rst
+++ b/pdns/recursordist/docs/settings.rst
@@ -937,6 +937,22 @@ This is used to limit endlessly chasing CNAME redirections.
 If qname-minimization is enabled, the number will be forced to be 100
 at a minimum to allow for the extra queries qname-minimization generates when the cache is empty.
 
+.. _setting-max-ns-address-qperq:
+
+``max-ns-address-qperq``
+------------------------
+-  Integer
+-  Default: 10
+
+The maximum number of outgoing queries with empty replies for
+resolving nameserver names to addresses we allow during the resolution
+of a single client query. If IPv6 is enabled, an A and a AAAA query
+for a name counts as 1. If a zone publishes more than this number of
+NS records, the limit is further reduced for that zone by lowering
+it by the number of NS records found above the
+`max-ns-address-qperq`_ value. The limit wil not be reduced to a
+number lower than 5.
+
 .. _setting-max-negative-ttl:
 
 ``max-negative-ttl``

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 1eec4369cd65065f81fa54aa4e38716a094f56b9..ac95f95d3cf9250e2579c7fa1f735cca643b9b92 100644 (file)
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -108,6 +108,7 @@ void initSR(bool debug)
   s_RC = std::unique_ptr<MemRecursorCache>(new MemRecursorCache());
 
   SyncRes::s_maxqperq = 50;
+  SyncRes::s_maxnsaddressqperq = 10;
   SyncRes::s_maxtotusec = 1000 * 7000;
   SyncRes::s_maxdepth = 40;
   SyncRes::s_maxnegttl = 3600;

diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc
index cc804adc592d38448443357a373db8b99019d591..38004165e256a28a3edda2b65540fc3254d1b8da 100644 (file)
--- a/pdns/recursordist/test-syncres_cc2.cc
+++ b/pdns/recursordist/test-syncres_cc2.cc
@@ -1275,6 +1275,48 @@ BOOST_AUTO_TEST_CASE(test_completely_flawed_nsset)
   BOOST_CHECK_EQUAL(queriesCount, 5U);
 }
 
+BOOST_AUTO_TEST_CASE(test_completely_flawed_big_nsset)
+{
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr);
+
+  primeHints();
+
+  const DNSName target("powerdns.com.");
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([&queriesCount, target](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional<Netmask>& srcmask, boost::optional<const ResolveContext&> context, LWResult* res, bool* chained) {
+    queriesCount++;
+
+    if (isRootServer(ip) && domain == target) {
+      setLWResult(res, 0, false, false, true);
+      // 20 NS records
+      for (int i = 0; i < 20; i++)  {
+        string n = string("pdns-public-ns") + std::to_string(i) + string(".powerdns.com.");
+        addRecordToLW(res, domain, QType::NS, n, DNSResourceRecord::AUTHORITY, 172800);
+      }
+      return 1;
+    }
+    else if (domain.toString().length() > 14 && domain.toString().substr(0, 14) == "pdns-public-ns") {
+      setLWResult(res, 0, true, false, true);
+      addRecordToLW(res, ".", QType::SOA, "a.root-servers.net. nstld.verisign-grs.com. 2017032800 1800 900 604800 86400", DNSResourceRecord::AUTHORITY, 86400);
+      return 1;
+    }
+    return 0;
+  });
+
+  vector<DNSRecord> ret;
+  try {
+    sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+    BOOST_CHECK(0);
+  } catch (const ImmediateServFailException& ex) {
+      BOOST_CHECK_EQUAL(ret.size(), 0U);
+      // one query to get NSs, then A and AAAA for each NS, 5th NS hits the limit
+      // limit is reduced to 5, because zone publishes many (20) NS
+    BOOST_CHECK_EQUAL(queriesCount, 11);
+  }
+}
+
 BOOST_AUTO_TEST_CASE(test_cache_hit)
 {
   std::unique_ptr<SyncRes> sr;

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index fca6650fad06b91862de034378b8d2b1b1ed3e1e..872878e9403e4163887015f80a1a50bc5679e04d 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -53,6 +53,7 @@ unsigned int SyncRes::s_maxnegttl;
 unsigned int SyncRes::s_maxbogusttl;
 unsigned int SyncRes::s_maxcachettl;
 unsigned int SyncRes::s_maxqperq;
+unsigned int SyncRes::s_maxnsaddressqperq;
 unsigned int SyncRes::s_maxtotusec;
 unsigned int SyncRes::s_maxdepth;
 unsigned int SyncRes::s_minimumTTL;
@@ -915,7 +916,7 @@ struct speedOrderCA
 
 /** This function explicitly goes out for A or AAAA addresses
 */
-vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere, bool cacheOnly)
+vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere, bool cacheOnly, unsigned int& addressQueriesForNS)
 {
   typedef vector<DNSRecord> res_t;
   typedef vector<ComboAddress> ret_t;
@@ -924,6 +925,7 @@ vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth,
   bool oldCacheOnly = setCacheOnly(cacheOnly);
   bool oldRequireAuthData = d_requireAuthData;
   bool oldValidationRequested = d_DNSSECValidationRequested;
+  const unsigned int startqueries = d_outqueries;
   d_requireAuthData = false;
   d_DNSSECValidationRequested = false;
 
@@ -974,6 +976,10 @@ vector<ComboAddress> SyncRes::getAddrs(const DNSName &qname, unsigned int depth,
        of a NS and keep processing the current query */
   }
 
+  if (ret.empty() && d_outqueries > startqueries) {
+    // We did 1 or more outgoing queries to resolve this NS name but returned empty handed
+    addressQueriesForNS++;
+  }
   d_requireAuthData = oldRequireAuthData;
   d_DNSSECValidationRequested = oldValidationRequested;
   setCacheOnly(oldCacheOnly);
@@ -1854,13 +1860,13 @@ bool SyncRes::nameserverIPBlockedByRPZ(const DNSFilterEngine& dfe, const ComboAd
   return false;
 }
 
-vector<ComboAddress> SyncRes::retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, std::vector<std::pair<DNSName, float>>::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<std::pair<DNSName, float>>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly)
+vector<ComboAddress> SyncRes::retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, std::vector<std::pair<DNSName, float>>::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<std::pair<DNSName, float>>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly, unsigned int &retrieveAddressesForNS)
 {
   vector<ComboAddress> result;
 
   if(!tns->first.empty()) {
     LOG(prefix<<qname<<": Trying to resolve NS '"<<tns->first<< "' ("<<1+tns-rnameservers.begin()<<"/"<<(unsigned int)rnameservers.size()<<")"<<endl);
-    result = getAddrs(tns->first, depth+2, beenthere, cacheOnly);
+    result = getAddrs(tns->first, depth+2, beenthere, cacheOnly, retrieveAddressesForNS);
     pierceDontQuery=false;
   }
   else {
@@ -3462,10 +3468,24 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con
 
   LOG(endl);
 
+  unsigned int addressQueriesForNS = 0;
   for(;;) { // we may get more specific nameservers
     auto rnameservers = shuffleInSpeedOrder(nameservers, doLog() ? (prefix+qname.toString()+": ") : string() );
 
+    // We allow s_maxnsaddressqperq (default 10) queries with empty responses when resolving NS names.
+    // If a zone publishes many (more than s_maxnsaddressqperq) NS records, we allow less.
+    // This is to "punish" zones that publish many non-resolving NS names.
+    // We always allow 5 NS name resolving attempts with empty results.
+    unsigned int nsLimit = s_maxnsaddressqperq;
+    if (rnameservers.size() > nsLimit) {
+      int newLimit = static_cast<int>(nsLimit) - (rnameservers.size() - nsLimit);
+      nsLimit = std::max(5, newLimit);
+    }
+
     for(auto tns=rnameservers.cbegin();;++tns) {
+      if (addressQueriesForNS >= nsLimit) {
+        throw ImmediateServFailException(std::to_string(nsLimit)+" (adjusted max-ns-address-qperq) or more queries with empty results for NS addresses sent resolving "+qname.toLogString());
+      }
       if(tns==rnameservers.cend()) {
         LOG(prefix<<qname<<": Failed to resolve via any of the "<<(unsigned int)rnameservers.size()<<" offered NS at level '"<<auth<<"'"<<endl);
         if(!auth.isRoot() && flawedNSSet) {
@@ -3521,7 +3541,7 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con
       }
       else {
         /* if tns is empty, retrieveAddressesForNS() knows we have hardcoded servers (i.e. "forwards") */
-        remoteIPs = retrieveAddressesForNS(prefix, qname, tns, depth, beenthere, rnameservers, nameservers, sendRDQuery, pierceDontQuery, flawedNSSet, cacheOnly);
+        remoteIPs = retrieveAddressesForNS(prefix, qname, tns, depth, beenthere, rnameservers, nameservers, sendRDQuery, pierceDontQuery, flawedNSSet, cacheOnly, addressQueriesForNS);
 
         if(remoteIPs.empty()) {
           LOG(prefix<<qname<<": Failed to get IP for NS "<<tns->first<<", trying next if available"<<endl);

diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 9bbadcdb426868112256394c6ca6cefaada403dd..9fcff9ad1a3bfc9dc59d695d13cceed3141af717 100644 (file)
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -757,6 +757,7 @@ public:
   static unsigned int s_minimumTTL;
   static unsigned int s_minimumECSTTL;
   static unsigned int s_maxqperq;
+  static unsigned int s_maxnsaddressqperq;
   static unsigned int s_maxtotusec;
   static unsigned int s_maxdepth;
   static unsigned int s_maxnegttl;
@@ -839,13 +840,13 @@ private:
   inline vector<std::pair<DNSName, float>> shuffleInSpeedOrder(NsSet &nameservers, const string &prefix);
   inline vector<ComboAddress> shuffleForwardSpeed(const vector<ComboAddress> &rnameservers, const string &prefix, const bool wasRd);
   bool moreSpecificThan(const DNSName& a, const DNSName &b) const;
-  vector<ComboAddress> getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere, bool cacheOnly);
+  vector<ComboAddress> getAddrs(const DNSName &qname, unsigned int depth, set<GetBestNSAnswer>& beenthere, bool cacheOnly, unsigned int& addressQueriesForNS);
 
   bool nameserversBlockedByRPZ(const DNSFilterEngine& dfe, const NsSet& nameservers);
   bool nameserverIPBlockedByRPZ(const DNSFilterEngine& dfe, const ComboAddress&);
   bool throttledOrBlocked(const std::string& prefix, const ComboAddress& remoteIP, const DNSName& qname, const QType& qtype, bool pierceDontQuery);
 
-  vector<ComboAddress> retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector<std::pair<DNSName, float>>::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<std::pair<DNSName, float>>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly);
+  vector<ComboAddress> retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector<std::pair<DNSName, float>>::const_iterator& tns, const unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<std::pair<DNSName, float>>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly, unsigned int& addressQueriesForNS);
 
   void sanitizeRecords(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, bool rdQuery);
   RCode::rcodes_ updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional<Netmask>, vState& state, bool& needWildcardProof, bool& gatherWildcardProof, unsigned int& wildcardLabelsCount, bool sendRDQuery);