rec: allowed names should not include names from CNAMEs that cannot be reached

Signed-off-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index ed6df9974eba89fda2ada7ddc4e8a926c0223471..fc40285105d0cfb304a922ee51b086891176cfa3 100644 (file)
--- 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<DNSName> sanitizeCNAMEChain(const DNSName& qname, std::unordered_map<DNSName, DNSName>& cnameChain)
+{
+  std::unordered_set<DNSName> 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<DNSName> allowedAdditionals = {qname};
   std::unordered_set<DNSName> allowedAnswerNames = {qname};
+  std::unordered_map<DNSName, DNSName> 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<CNAMERecordContent>(*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 ec62d84ff0e8e35d28cbdab83f7cc5e146f1c366..af77456509a56f2ca9039d26ecbc83184fcefe63 100644 (file)
--- 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<SyncRes> 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<Netmask>& /* 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<DNSRecord> 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<SyncRes> sr;