From: Otto Moerbeek Date: Tue, 4 Feb 2020 12:52:51 +0000 (+0100) Subject: Limit the number of queries sent out to get NS addresses per query. X-Git-Tag: dnsdist-1.5.0-rc3~50^2~5 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=86f95f85295696c0b264455472b8e270fccb6542;p=thirdparty%2Fpdns.git 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 712c9cc6cb..c075234408 100644 --- 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 bec850bd0b..91f84fb2e7 100644 --- 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 1eec4369cd..ac95f95d3c 100644 --- 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(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 cc804adc59..38004165e2 100644 --- 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 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& srcmask, boost::optional 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 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 sr; diff --git a/pdns/syncres.cc b/pdns/syncres.cc index fca6650fad..872878e940 100644 --- 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 SyncRes::getAddrs(const DNSName &qname, unsigned int depth, set& beenthere, bool cacheOnly) +vector SyncRes::getAddrs(const DNSName &qname, unsigned int depth, set& beenthere, bool cacheOnly, unsigned int& addressQueriesForNS) { typedef vector res_t; typedef vector ret_t; @@ -924,6 +925,7 @@ vector 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 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 SyncRes::retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, std::vector>::const_iterator& tns, const unsigned int depth, set& beenthere, const vector>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly) +vector SyncRes::retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, std::vector>::const_iterator& tns, const unsigned int depth, set& beenthere, const vector>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly, unsigned int &retrieveAddressesForNS) { vector result; if(!tns->first.empty()) { LOG(prefix<first<< "' ("<<1+tns-rnameservers.begin()<<"/"<<(unsigned int)rnameservers.size()<<")"<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(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<first<<", trying next if available"<> shuffleInSpeedOrder(NsSet &nameservers, const string &prefix); inline vector shuffleForwardSpeed(const vector &rnameservers, const string &prefix, const bool wasRd); bool moreSpecificThan(const DNSName& a, const DNSName &b) const; - vector getAddrs(const DNSName &qname, unsigned int depth, set& beenthere, bool cacheOnly); + vector getAddrs(const DNSName &qname, unsigned int depth, set& 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 retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector>::const_iterator& tns, const unsigned int depth, set& beenthere, const vector>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly); + vector retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector>::const_iterator& tns, const unsigned int depth, set& beenthere, const vector>& 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, vState& state, bool& needWildcardProof, bool& gatherWildcardProof, unsigned int& wildcardLabelsCount, bool sendRDQuery);