From: Otto Moerbeek Date: Mon, 24 Nov 2025 11:08:51 +0000 (+0100) Subject: rec: allowed names should not include names from CNAMEs that cannot be reached X-Git-Tag: rec-5.5.0-alpha0~18^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5708aa5b72015f6cf8c045b134ae0e2605065ba2;p=thirdparty%2Fpdns.git rec: allowed names should not include names from CNAMEs that cannot be reached Signed-off-by: Otto Moerbeek --- diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index ed6df9974e..fc40285105 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -4202,6 +4202,25 @@ static bool isRedirection(QType qtype) return qtype == QType::CNAME || qtype == QType::DNAME; } +// Walk the chain from qname, only adding names that can be reached +static std::unordered_set sanitizeCNAMEChain(const DNSName& qname, std::unordered_map& cnameChain) +{ + std::unordered_set allowed = {qname}; + DNSName key{qname}; + while (true) { + if (auto probe = cnameChain.find(key); probe != cnameChain.end()) { + allowed.emplace(probe->second); + key = probe->second; + // This will prevent looping in this function. CNAME loops themselves we handle higher up, see handleNewTarget() + cnameChain.erase(probe); + } + else { + break; + } + } + return allowed; +} + void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType qtype, const DNSName& auth, bool wasForwarded, bool rdQuery) { const bool wasForwardRecurse = wasForwarded && rdQuery; @@ -4209,6 +4228,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN to remain */ std::unordered_set allowedAdditionals = {qname}; std::unordered_set allowedAnswerNames = {qname}; + std::unordered_map cnameChain; bool cnameSeen = false; bool haveAnswers = false; bool acceptDelegation = false; @@ -4283,7 +4303,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN haveAnswers = true; if (rec->d_type == QType::CNAME) { if (auto cnametarget = getRR(*rec); cnametarget != nullptr) { - allowedAnswerNames.insert(cnametarget->getTarget()); + cnameChain.emplace(rec->d_name, cnametarget->getTarget()); } cnameSeen = cnameSeen || qname == rec->d_name; } @@ -4347,6 +4367,10 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN acceptDelegation = true; } + if (cnameChain.size() > 0) { + auto allowed = sanitizeCNAMEChain(qname, cnameChain); + allowedAnswerNames.insert(allowed.begin(), allowed.end()); + } sanitizeRecordsPass2(prefix, lwr, qname, qtype, auth, allowedAnswerNames, allowedAdditionals, cnameSeen, acceptDelegation && !soaInAuth, skipvec, skipCount); } diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc index ec62d84ff0..af77456509 100644 --- a/pdns/recursordist/test-syncres_cc1.cc +++ b/pdns/recursordist/test-syncres_cc1.cc @@ -2186,6 +2186,69 @@ BOOST_AUTO_TEST_CASE(test_cname_target_servfail_servestale) BOOST_CHECK_EQUAL(ret[0].d_name, target); } +BOOST_AUTO_TEST_CASE(test_broken_cname_chain) +{ + std::unique_ptr sr; + initSR(sr); + + primeHints(); + + const DNSName target("www.powerdns.com."); + const DNSName subtarget("sub.www.powerdns.com."); + const DNSName subns("new-sub.www.powerdns.com."); + const DNSName unrelated("unrelated.com."); + + timeval now{}; + Utility::gettimeofday(&now, nullptr); + + sr->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int qtype, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional& /* srcmask */, const ResolveContext& /* context */, LWResult* res, bool* /* chained */) { + if (isRootServer(address)) { + + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800); + addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + if (address == ComboAddress("192.0.2.1:53")) { + if (domain == target) { + if (qtype == QType::NS) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, target, QType::NS, target.toString(), DNSResourceRecord::ANSWER); + addRecordToLW(res, subtarget, QType::NS, subns.toString(), DNSResourceRecord::ANSWER); + addRecordToLW(res, unrelated, QType::NS, subns.toString(), DNSResourceRecord::ANSWER); + addRecordToLW(res, subtarget, QType::NS, subtarget.toString(), DNSResourceRecord::ANSWER); + addRecordToLW(res, subtarget, QType::CNAME, subtarget.toString(), DNSResourceRecord::ANSWER); + return LWResult::Result::Success; + } + if (qtype == QType::A) { + setLWResult(res, 0, true, false, false); + return LWResult::Result::Success; + } + } + } + + return LWResult::Result::Timeout; + }); + + vector ret; + int res = sr->beginResolve(target, QType(QType::NS), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 1U); + + time_t cached = g_recCache->get(now.tv_sec, subtarget, QType::NS, MemRecursorCache::None, &ret, ComboAddress()); + BOOST_CHECK(cached <= 0); + cached = g_recCache->get(now.tv_sec, unrelated, QType::NS, MemRecursorCache::None, &ret, ComboAddress()); + BOOST_CHECK(cached <= 0); + cached = g_recCache->get(now.tv_sec, subtarget, QType::CNAME, MemRecursorCache::None, &ret, ComboAddress()); + BOOST_CHECK(cached <= 0); + + // And again to check cache + ret.clear(); + res = sr->beginResolve(target, QType(QType::NS), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 1U); +} + BOOST_AUTO_TEST_CASE(test_time_limit) { std::unique_ptr sr;