From 60450a91b3f034bbb46597d074bd8d5b48388144 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 4 Mar 2025 09:52:15 +0100 Subject: [PATCH] If we see both a CNAME and answer records, follow CNAME and discard the answer records (cherry picked from commit 8eb86803de0d9a61ab5e3984c3e0a43cee37322b) --- pdns/recursordist/syncres.cc | 17 ++++- pdns/recursordist/syncres.hh | 2 +- pdns/recursordist/test-syncres_cc1.cc | 106 ++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 3 deletions(-) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 9a318dfc46..f3fd45c749 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -4244,6 +4244,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN to remain */ std::unordered_set allowedAdditionals = {qname}; std::unordered_set allowedAnswerNames = {qname}; + bool cnameSeen = false; bool haveAnswers = false; bool isNXDomain = false; bool isNXQType = false; @@ -4319,6 +4320,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN if (auto cnametarget = getRR(*rec); cnametarget != nullptr) { allowedAnswerNames.insert(cnametarget->getTarget()); } + cnameSeen = cnameSeen || qname == rec->d_name; } else if (rec->d_type == QType::DNAME) { // We have checked the DNAME rec->d_name above, the actual answer will be synthesized in a later step @@ -4386,10 +4388,10 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN } } // end of first loop, handled answer and most of authority section - sanitizeRecordsPass2(prefix, lwr, qname, auth, allowedAnswerNames, allowedAdditionals, isNXDomain, isNXQType, skipvec, skipCount); + sanitizeRecordsPass2(prefix, lwr, qname, qtype, auth, allowedAnswerNames, allowedAdditionals, cnameSeen, isNXDomain, isNXQType, skipvec, skipCount); } -void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, const DNSName& qname, const DNSName& auth, std::unordered_set& allowedAnswerNames, std::unordered_set& allowedAdditionals, bool isNXDomain, bool isNXQType, std::vector& skipvec, unsigned int& skipCount) +void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType qtype, const DNSName& auth, std::unordered_set& allowedAnswerNames, std::unordered_set& allowedAdditionals, bool cnameSeen, bool isNXDomain, bool isNXQType, std::vector& skipvec, unsigned int& skipCount) { // Second loop, we know now if the answer was NxDomain or NoData unsigned int counter = 0; @@ -4408,6 +4410,12 @@ void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, con LOG(prefix << qname << ": Removing irrelevent record '" << rec->toString() << "' in the ANSWER section received from " << auth << endl); skipvec[counter] = true; ++skipCount; + } + // If we have a CNAME, skip answer records for the requested type + if (cnameSeen && rec->d_type == qtype && rec->d_name == qname && qtype != QType::CNAME) { + LOG(prefix << qname << ": Removing answer record in presence of CNAME record '" << rec->toString() << "' in the ANSWER section received from " << auth << endl); + skipvec[counter] = true; + ++skipCount; continue; } } @@ -5738,6 +5746,11 @@ bool SyncRes::processAnswer(unsigned int depth, const string& prefix, LWResult& bool done = processRecords(prefix, qname, qtype, auth, lwr, sendRDQuery, ret, nsset, newtarget, newauth, realreferral, negindic, state, needWildcardProof, gatherWildcardProof, wildcardLabelsCount, *rcode, negIndicHasSignatures, depth); + // If we both have a CNAME and an answer, let the CNAME take precedence. This *should* not happen + // (because CNAMEs cannot co-exist with other records), but reality says otherwise. Other + // resolvers choose to follow the CNAME in this case as well. We removed the answer record from + // the records received from the auth when sanitizing, so `done' should not be set when a CNAME is + // present. if (done) { LOG(prefix << qname << ": Status=got results, this level of recursion done" << endl); LOG(prefix << qname << ": Validation status is " << state << endl); diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh index 6b31b300a2..4e14900856 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -663,7 +663,7 @@ private: vector retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector>::const_iterator& tns, unsigned int depth, set& beenthere, const vector>& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly, unsigned int& nretrieveAddressesForNS); void sanitizeRecords(const std::string& prefix, LWResult& lwr, const DNSName& qname, QType qtype, const DNSName& auth, bool wasForwarded, bool rdQuery); - void sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, const DNSName& qname, const DNSName& auth, std::unordered_set& allowedAnswerNames, std::unordered_set& allowedAdditionals, bool isNXDomain, bool isNXQType, std::vector& skipvec, unsigned int& skipCount); + void sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, const DNSName& qname, QType qtype, const DNSName& auth, std::unordered_set& allowedAnswerNames, std::unordered_set& allowedAdditionals, bool cnameSeen, bool isNXDomain, bool isNXQType, std::vector& skipvec, unsigned int& skipCount); /* This function will check whether the answer should have the AA bit set, and will set if it should be set and isn't. This is unfortunately needed to deal with very crappy so-called DNS servers */ void fixupAnswer(const std::string& prefix, LWResult& lwr, const DNSName& qname, QType qtype, const DNSName& auth, bool wasForwarded, bool rdQuery); diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc index d51953fd2f..da71d28fe3 100644 --- a/pdns/recursordist/test-syncres_cc1.cc +++ b/pdns/recursordist/test-syncres_cc1.cc @@ -1462,6 +1462,112 @@ BOOST_AUTO_TEST_CASE(test_following_cname) BOOST_CHECK_EQUAL(ret[1].d_name, cnameTarget); } +BOOST_AUTO_TEST_CASE(test_following_cname_with_a) +{ + std::unique_ptr resolver; + initSR(resolver); + + primeHints(); + + const DNSName target("cname.powerdns.com."); + const DNSName cnameTarget("cname-target.powerdns.com"); + + resolver->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int /* type */, 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) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::A, "192.1.1.1"); + addRecordToLW(res, domain, QType::CNAME, cnameTarget.toString()); + return LWResult::Result::Success; + } + if (domain == cnameTarget) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::A, "192.0.2.2"); + } + + return LWResult::Result::Success; + } + + return LWResult::Result::Timeout; + }); + + vector ret; + int res = resolver->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 2U); + BOOST_CHECK(ret[0].d_type == QType::CNAME); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK(ret[1].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[1].d_name, cnameTarget); + BOOST_CHECK_EQUAL(ret[1].getContent()->getZoneRepresentation(), "192.0.2.2"); +} + +BOOST_AUTO_TEST_CASE(test_following_cname_chain_with_a) +{ + std::unique_ptr resolver; + initSR(resolver); + + primeHints(); + + const DNSName target("cname.powerdns.com."); + const DNSName cnameTarget1("cname-target1.powerdns.com"); + const DNSName cnameTarget("cname-target.powerdns.com"); + + resolver->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int /* type */, 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) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::A, "192.1.1.1"); + addRecordToLW(res, domain, QType::CNAME, cnameTarget1.toString()); + addRecordToLW(res, cnameTarget1, QType::A, "192.1.1.2"); + addRecordToLW(res, cnameTarget1, QType::CNAME, cnameTarget.toString()); + } + if (domain == cnameTarget1) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, cnameTarget1, QType::A, "192.1.1.2"); + addRecordToLW(res, cnameTarget1, QType::CNAME, cnameTarget.toString()); + return LWResult::Result::Success; + } + if (domain == cnameTarget) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::A, "192.0.2.2"); + } + + return LWResult::Result::Success; + } + + return LWResult::Result::Timeout; + }); + + vector ret; + int res = resolver->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 3U); + BOOST_CHECK(ret[0].d_type == QType::CNAME); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(ret[0].getContent()->getZoneRepresentation(), cnameTarget1.toString()); + BOOST_CHECK(ret[1].d_type == QType::CNAME); + BOOST_CHECK_EQUAL(ret[1].d_name, cnameTarget1); + BOOST_CHECK_EQUAL(ret[1].getContent()->getZoneRepresentation(), cnameTarget.toString()); + BOOST_CHECK(ret[2].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[2].d_name, cnameTarget); + BOOST_CHECK_EQUAL(ret[2].getContent()->getZoneRepresentation(), "192.0.2.2"); +} + BOOST_AUTO_TEST_CASE(test_cname_nxdomain) { std::unique_ptr sr; -- 2.47.2