From 8eb86803de0d9a61ab5e3984c3e0a43cee37322b 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 --- 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 9abf83f864..d5d9f7c275 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -4275,6 +4275,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; @@ -4350,6 +4351,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 @@ -4417,10 +4419,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; @@ -4439,6 +4441,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; } } @@ -5777,6 +5785,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 c67a27f74b..61ee00bde1 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 42e5bc54d3..22d78e5c64 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