Be more strict with names we allow in the answer section

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index 7aba1c1f61af8c3968d828f2582d3ea5394c55fc..7af37bceb64ac3fea4ea415e033397576ba00953 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -4232,6 +4232,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
   /* list of names for which we will allow A and AAAA records in the additional section
      to remain */
   std::unordered_set<DNSName> allowedAdditionals = {qname};
+  std::unordered_set<DNSName> allowedAnswerNames = {qname};
   bool haveAnswers = false;
   bool isNXDomain = false;
   bool isNXQType = false;
@@ -4263,7 +4264,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
       continue;
     }
 
-    // Disallow any name not part of qname requested
+    // Disallow any name not part of auth requested (i.e. disallow x.y.z if asking a NS authoritative for x.w.z)
     if (!rec->d_name.isPartOf(auth)) {
       LOG(prefix << qname << ": Removing record '" << rec->toString() << "' in the " << DNSResourceRecord::placeString(rec->d_place) << " section received from " << auth << endl);
       skipvec[counter] = true;
@@ -4272,6 +4273,8 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
     }
 
     // Disallow QType DNAME in non-answer section or containing an answer that is not a parent of or equal to the question name
+    // i.e. disallowed bar.example.com. DNAME bar.example.net. when asking foo.example.com
+    // But allow it when asking for foo.bar.example.com.
     if (rec->d_type == QType::DNAME && (rec->d_place != DNSResourceRecord::ANSWER || !qname.isPartOf(rec->d_name))) {
       LOG(prefix << qname << ": Removing invalid DNAME record '" << rec->toString() << "' in the " << DNSResourceRecord::placeString(rec->d_place) << " section received from " << auth << endl);
       skipvec[counter] = true;
@@ -4301,6 +4304,14 @@ 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());
+        }
+      }
+      else if (rec->d_type == QType::DNAME) {
+        allowedAnswerNames.insert(rec->d_name);
+      }
       allowAdditionalEntry(allowedAdditionals, *rec);
     }
 
@@ -4363,10 +4374,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, allowedAdditionals, isNXDomain, isNXQType, skipvec, skipCount);
+  sanitizeRecordsPass2(prefix, lwr, qname, auth, allowedAnswerNames, allowedAdditionals, isNXDomain, isNXQType, skipvec, skipCount);
 }
 
-void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, const DNSName& qname, const DNSName& auth, std::unordered_set<DNSName>& allowedAdditionals, bool isNXDomain, bool isNXQType, std::vector<bool>& skipvec, unsigned int& skipCount)
+void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, const DNSName& qname, const DNSName& auth, std::unordered_set<DNSName>& allowedAnswerNames, std::unordered_set<DNSName>& allowedAdditionals, bool isNXDomain, bool isNXQType, std::vector<bool>& skipvec, unsigned int& skipCount)
 {
   // Second loop, we know now if the answer was NxDomain or NoData
   unsigned int counter = 0;
@@ -4381,8 +4392,12 @@ void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, con
     }
 
     if (rec->d_place == DNSResourceRecord::ANSWER) {
-      // Fully handled ANSWER section in first loop. This might change with more strict validation
-      continue;
+      if (allowedAnswerNames.count(rec->d_name) == 0) {
+        LOG(prefix << qname << ": Removing irrelevent record '" << rec->toString() << "' in the ANSWER section received from " << auth << endl);
+        skipvec[counter] = true;
+        ++skipCount;
+        continue;
+      }
     }
     if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::NS) {
       if (isNXDomain || isNXQType) {

diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh
index 74b7aa6b354c7e79c456f2456ca771b0f43f3e68..9b14bf8d9194d5751a64e412d22fe0123cb9b324 100644 (file)
--- a/pdns/recursordist/syncres.hh
+++ b/pdns/recursordist/syncres.hh
@@ -663,7 +663,7 @@ private:
   vector<ComboAddress> retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector<std::pair<DNSName, float>>::const_iterator& tns, unsigned int depth, set<GetBestNSAnswer>& beenthere, const vector<std::pair<DNSName, float>>& 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<DNSName>& allowedAdditionals, bool isNXDomain, bool isNXQType, std::vector<bool>& skipvec, unsigned int& skipCount);
+  void sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, const DNSName& qname, const DNSName& auth, std::unordered_set<DNSName>& allowedAnswerNames, std::unordered_set<DNSName>& allowedAdditionals, bool isNXDomain, bool isNXQType, std::vector<bool>& 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_cc3.cc b/pdns/recursordist/test-syncres_cc3.cc
index 0939c12ba2855ac87e4f3903ab546e7e1be9dc80..303bf97c7ff5261ab36ad000da7ef9ca5b3a8b00 100644 (file)
--- a/pdns/recursordist/test-syncres_cc3.cc
+++ b/pdns/recursordist/test-syncres_cc3.cc
@@ -158,14 +158,11 @@ BOOST_AUTO_TEST_CASE(test_extra_answers)
   BOOST_REQUIRE_EQUAL(QType(cached.at(0).d_type).toString(), QType(QType::A).toString());
   BOOST_CHECK_EQUAL(getRR<ARecordContent>(cached.at(0))->getCA().toString(), ComboAddress("192.0.2.2").toString());
 
-  // The cache should also have an authoritative record for the extra in-bailiwick record
-  BOOST_REQUIRE_GT(g_recCache->get(now, target2, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
-  BOOST_REQUIRE_EQUAL(cached.size(), 1U);
-  BOOST_REQUIRE_EQUAL(QType(cached.at(0).d_type).toString(), QType(QType::A).toString());
-  BOOST_CHECK_EQUAL(getRR<ARecordContent>(cached.at(0))->getCA().toString(), ComboAddress("192.0.2.3").toString());
+  // The cache should not have an authoritative record for the extra in-bailiwick record
+  BOOST_REQUIRE_LE(g_recCache->get(now, target2, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
 
-  // But the out-of-bailiwick record should not be there
-  BOOST_REQUIRE_LT(g_recCache->get(now, target3, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
+  // And the out-of-bailiwick record should not be there
+  BOOST_REQUIRE_LE(g_recCache->get(now, target3, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_extra_answers)
@@ -229,14 +226,11 @@ BOOST_AUTO_TEST_CASE(test_dnssec_extra_answers)
   BOOST_REQUIRE_EQUAL(QType(cached.at(0).d_type).toString(), QType(QType::A).toString());
   BOOST_CHECK_EQUAL(getRR<ARecordContent>(cached.at(0))->getCA().toString(), ComboAddress("192.0.2.2").toString());
 
-  // The cache should also have an authoritative record for the extra in-bailiwick record
-  BOOST_REQUIRE_GT(g_recCache->get(now, target2, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
-  BOOST_REQUIRE_EQUAL(cached.size(), 1U);
-  BOOST_REQUIRE_EQUAL(QType(cached.at(0).d_type).toString(), QType(QType::A).toString());
-  BOOST_CHECK_EQUAL(getRR<ARecordContent>(cached.at(0))->getCA().toString(), ComboAddress("192.0.2.3").toString());
+  // The cache should not have an authoritative record for the extra in-bailiwick record
+  BOOST_REQUIRE_LE(g_recCache->get(now, target2, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
 
-  // But the out-of-bailiwick record should not be there
-  BOOST_REQUIRE_LT(g_recCache->get(now, target3, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
+  // And the out-of-bailiwick record should not be there
+  BOOST_REQUIRE_LE(g_recCache->get(now, target3, QType(QType::A), MemRecursorCache::RequireAuth, &cached, who), 0);
 }
 
 BOOST_AUTO_TEST_CASE(test_skip_opt_any)