From: Otto Moerbeek Date: Fri, 14 Apr 2023 08:43:23 +0000 (+0200) Subject: rec: Distinguish between recursion depth and cname length, they are not the same X-Git-Tag: rec-4.9.0-beta1~17^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=475718662b06927e54a379dcf311b2623830f7da;p=thirdparty%2Fpdns.git rec: Distinguish between recursion depth and cname length, they are not the same --- diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index f7270474f1..72ec1d0caa 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -468,6 +468,7 @@ bool SyncRes::s_dot_to_port_853; int SyncRes::s_event_trace_enabled; bool SyncRes::s_save_parent_ns_set; unsigned int SyncRes::s_max_busy_dot_probes; +unsigned int SyncRes::s_max_CNAMES_followed = 10; bool SyncRes::s_addExtendedResolutionDNSErrors; #define LOG(x) \ @@ -1825,7 +1826,8 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp if (s_maxdepth > 0) { auto bound = getAdjustedRecursionBound(); - if (depth > bound) { + // Use a stricter bound if throttling + if (depth > bound || (d_outqueries > 10 && d_throttledqueries > 5 && depth > bound * 2 / 3)) { string msg = "More than " + std::to_string(bound) + " (adjusted max-recursion-depth) levels of recursion needed while resolving " + qname.toLogString(); LOG(prefix << qname << ": " << msg << endl); throw ImmediateServFailException(msg); @@ -2403,30 +2405,22 @@ void SyncRes::updateValidationStatusInCache(const DNSName& qname, const QType qt } } -static bool scanForCNAMELoop(const DNSName& name, const vector& records) +static pair scanForCNAMELoop(const DNSName& name, const vector& records) { + unsigned int numCNames = 0; for (const auto& record : records) { if (record.d_type == QType::CNAME && record.d_place == DNSResourceRecord::ANSWER) { + ++numCNames; if (name == record.d_name) { - return true; + return {true, numCNames}; } } } - return false; + return {false, numCNames}; } bool SyncRes::doCNAMECacheCheck(const DNSName& qname, const QType qtype, vector& ret, unsigned int depth, const string& prefix, int& res, Context& context, bool wasAuthZone, bool wasForwardRecurse) { - // Even if s_maxdepth is zero, we want to have this check - auto bound = std::max(40U, getAdjustedRecursionBound()); - // Bounds were > 9 and > 15 originally, now they are derived from s_maxdepth (default 40) - // Apply more strict bound if we see throttling - if ((depth >= bound / 4 && d_outqueries > 10 && d_throttledqueries > 5) || depth > bound * 3 / 8) { - LOG(prefix << qname << ": Recursing (CNAME or other indirection) too deep, depth=" << depth << endl); - res = RCode::ServFail; - return true; - } - vector cset; vector> signatures; vector> authorityRecs; @@ -2599,12 +2593,18 @@ bool SyncRes::doCNAMECacheCheck(const DNSName& qname, const QType qtype, vector< return true; } - // Check to see if we already have seen the new target as a previous target - if (scanForCNAMELoop(newTarget, ret)) { + // Check to see if we already have seen the new target as a previous target or that we have a very long CNAME chain + const auto [CNAMELoop, numCNAMEs] = scanForCNAMELoop(newTarget, ret); + if (CNAMELoop) { string msg = "got a CNAME referral (from cache) that causes a loop"; LOG(prefix << qname << ": Status=" << msg << endl); throw ImmediateServFailException(msg); } + if (numCNAMEs > s_max_CNAMES_followed) { + string msg = "max number of CNAMEs exceeded"; + LOG(prefix << qname << ": Status=" << msg << endl); + throw ImmediateServFailException(msg); + } set beenthere; Context cnameContext; @@ -5327,26 +5327,24 @@ void SyncRes::handleNewTarget(const std::string& prefix, const DNSName& qname, c setQNameMinimization(false); } - // Was 10 originally, default s_maxdepth is 40, but even if it is zero we want to apply a bound - auto bound = std::max(40U, getAdjustedRecursionBound()) / 4; - if (depth > bound) { - LOG(prefix << qname << ": Status=got a CNAME referral, but recursing too deep, returning SERVFAIL" << endl); - rcode = RCode::ServFail; - return; - } - if (!d_followCNAME) { rcode = RCode::NoError; return; } - // Check to see if we already have seen the new target as a previous target - if (scanForCNAMELoop(newtarget, ret)) { + // Check to see if we already have seen the new target as a previous target or that the chain is too long + const auto [CNAMELoop, numCNAMEs] = scanForCNAMELoop(newtarget, ret); + if (CNAMELoop) { LOG(prefix << qname << ": Status=got a CNAME referral that causes a loop, returning SERVFAIL" << endl); ret.clear(); rcode = RCode::ServFail; return; } + if (numCNAMEs > s_max_CNAMES_followed) { + LOG(prefix << qname << ": Status=got a CNAME referral, but chain too long, returning SERVFAIL" << endl); + rcode = RCode::ServFail; + return; + } if (qtype == QType::DS || qtype == QType::DNSKEY) { LOG(prefix << qname << ": Status=got a CNAME referral, but we are looking for a DS or DNSKEY" << endl); diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh index b271acfbfa..445482b952 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -545,6 +545,7 @@ public: static bool s_tcp_fast_open_connect; static bool s_dot_to_port_853; static unsigned int s_max_busy_dot_probes; + static unsigned int s_max_CNAMES_followed; static const int event_trace_to_pb = 1; static const int event_trace_to_log = 2; diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc index 9d1c19d420..e265fa1042 100644 --- a/pdns/recursordist/test-syncres_cc1.cc +++ b/pdns/recursordist/test-syncres_cc1.cc @@ -1002,6 +1002,56 @@ BOOST_AUTO_TEST_CASE(test_glueless_referral) BOOST_CHECK_EQUAL(ret[0].d_name, target); } +BOOST_AUTO_TEST_CASE(test_endless_glueless_referral) +{ + std::unique_ptr sr; + initSR(sr); + + primeHints(); + + const DNSName target("powerdns.com."); + + size_t count = 0; + sr->setAsyncCallback([target, &count](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 */) { + if (isRootServer(ip)) { + setLWResult(res, 0, false, false, true); + + if (domain.isPartOf(DNSName("com."))) { + addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800); + } + else if (domain.isPartOf(DNSName("org."))) { + addRecordToLW(res, "org.", QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800); + } + else { + setLWResult(res, RCode::NXDomain, false, false, true); + return LWResult::Result::Success; + } + + addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600); + addRecordToLW(res, "a.gtld-servers.net.", QType::AAAA, "2001:DB8::1", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + if (domain == target) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, "powerdns.com.", QType::NS, "ns1.powerdns.org.", DNSResourceRecord::AUTHORITY, 172800); + addRecordToLW(res, "powerdns.com.", QType::NS, "ns2.powerdns.org.", DNSResourceRecord::AUTHORITY, 172800); + return LWResult::Result::Success; + } + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::NS, std::to_string(count) + ".ns1.powerdns.org", DNSResourceRecord::AUTHORITY, 172800); + addRecordToLW(res, domain, QType::NS, std::to_string(count) + ".ns2.powerdns.org", DNSResourceRecord::AUTHORITY, 172800); + count++; + return LWResult::Result::Success; + }); + + vector ret; + BOOST_CHECK_EXCEPTION(sr->beginResolve(target, QType(QType::A), QClass::IN, ret), + ImmediateServFailException, + [](const ImmediateServFailException& isfe) { + return isfe.reason.substr(0, 9) == "More than"; + }); +} + BOOST_AUTO_TEST_CASE(test_glueless_referral_aaaa_task) { std::unique_ptr sr; @@ -1647,17 +1697,17 @@ BOOST_AUTO_TEST_CASE(test_cname_long_loop) } } -BOOST_AUTO_TEST_CASE(test_cname_depth) +BOOST_AUTO_TEST_CASE(test_cname_length) { std::unique_ptr sr; initSR(sr); primeHints(); - size_t depth = 0; + size_t length = 0; const DNSName target("cname.powerdns.com."); - sr->setAsyncCallback([target, &depth](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 */) { + sr->setAsyncCallback([target, &length](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 */) { if (isRootServer(ip)) { setLWResult(res, 0, false, false, true); @@ -1668,8 +1718,8 @@ BOOST_AUTO_TEST_CASE(test_cname_depth) else if (ip == ComboAddress("192.0.2.1:53")) { setLWResult(res, 0, true, false, false); - addRecordToLW(res, domain, QType::CNAME, std::to_string(depth) + "-cname.powerdns.com"); - depth++; + addRecordToLW(res, domain, QType::CNAME, std::to_string(length) + "-cname.powerdns.com"); + length++; return LWResult::Result::Success; } @@ -1679,9 +1729,8 @@ BOOST_AUTO_TEST_CASE(test_cname_depth) vector ret; int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); BOOST_CHECK_EQUAL(res, RCode::ServFail); - BOOST_CHECK_EQUAL(ret.size(), depth); - /* we have an arbitrary limit at 10 when following a CNAME chain */ - BOOST_CHECK_EQUAL(depth, 10U + 2U); + BOOST_CHECK_EQUAL(ret.size(), length); + BOOST_CHECK_EQUAL(length, SyncRes::s_max_CNAMES_followed + 1); } BOOST_AUTO_TEST_CASE(test_time_limit)