Don't pick up random root NS records from AUTHORITY sections

diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc
index e44f61259dfa9f20da9d597d87bb77f611123a44..2f2020d3d92aab964d971cb9a4db0e532084a772 100644 (file)
--- a/pdns/recursordist/test-syncres_cc1.cc
+++ b/pdns/recursordist/test-syncres_cc1.cc
@@ -129,6 +129,64 @@ BOOST_AUTO_TEST_CASE(test_root_not_primed_and_no_response)
   }
 }
 
+BOOST_AUTO_TEST_CASE(test_root_ns_poison_resistance)
+{
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr);
+
+  primeHints();
+  const DNSName target("www.example.com.");
+
+  sr->setAsyncCallback([target](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional<Netmask>& srcmask, boost::optional<const ResolveContext&> context, LWResult* res, bool* chained) {
+
+    if (domain == g_rootdnsname && type == QType::NS) {
+
+      setLWResult(res, 0, true, false, true);
+      char addr[] = "a.root-servers.net.";
+      for (char idx = 'a'; idx <= 'm'; idx++) {
+        addr[0] = idx;
+        addRecordToLW(res, g_rootdnsname, QType::NS, std::string(addr), DNSResourceRecord::ANSWER, 3600);
+      }
+
+      addRecordToLW(res, "a.root-servers.net.", QType::A, "198.41.0.4", DNSResourceRecord::ADDITIONAL, 3600);
+      addRecordToLW(res, "a.root-servers.net.", QType::AAAA, "2001:503:ba3e::2:30", DNSResourceRecord::ADDITIONAL, 3600);
+
+      return LWResult::Result::Success;
+    }
+
+    if (domain == target && type == QType::A) {
+
+      setLWResult(res, 0, true, false, true);
+      addRecordToLW(res, target, QType::A, "1.2.3.4", DNSResourceRecord::ANSWER, 3600);
+
+      addRecordToLW(res, ".", QType::NS, "poison.name.", DNSResourceRecord::AUTHORITY, 3600);
+      addRecordToLW(res, "poison.name", QType::A, "4.5.6.7", DNSResourceRecord::ADDITIONAL, 3600);
+
+      return LWResult::Result::Success;
+    }
+
+    return LWResult::Result::Timeout;
+  });
+
+  vector<DNSRecord> ret;
+  // Check we have 13 root servers
+  int res = sr->beginResolve(g_rootdnsname, QType(QType::NS), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 13U);
+
+  // Try to poison
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 1U);
+
+  // Still should have 13
+  ret.clear();
+  res = sr->beginResolve(g_rootdnsname, QType(QType::NS), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 13U);
+}
+
 static void test_edns_formerr_fallback_f(bool sample)
 {
   std::unique_ptr<SyncRes> sr;

diff --git a/pdns/reczones.cc b/pdns/reczones.cc
index 2bfbfae3c31d8f70f886db675fece1b598b30e12..5d1b537a97a56c64aabeee67be75d04aaa8c02bd 100644 (file)
--- a/pdns/reczones.cc
+++ b/pdns/reczones.cc
@@ -114,11 +114,13 @@ bool primeHints(time_t ignored)
     for (auto const& r: seenA) {
       if (seenNS.count(r)) {
         reachableA = true;
+       break;
       }
     }
     for (auto const& r: seenAAAA) {
       if (seenNS.count(r)) {
         reachableAAAA = true;
+       break;
       }
     }
     if (SyncRes::s_doIPv4 && !SyncRes::s_doIPv6 && !reachableA) {

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 5e4e5db830e5ba475529bda51f2058398a97cf04..996717f7333fc60d99566cb7154a569ff1f50f48 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -2970,7 +2970,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
     }
 
     if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::NS && (isNXDomain || isNXQType)) {
-      /* we don't want to pick up NS records in AUTHORITY or ADDITIONAL sections of NXDomain answers
+      /* we don't want to pick up NS records in AUTHORITY and their ADDITIONAL sections of NXDomain answers
          because they are somewhat easy to insert into a large, fragmented UDP response
          for an off-path attacker by injecting spoofed UDP fragments.
       */
@@ -2979,6 +2979,14 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
       continue;
     }
 
+    if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::NS && !d_updatingRootNS && rec->d_name == g_rootdnsname) {
+      /* we don't want to pick up NS records in AUTHORITY and their ADDITIONALs sections of random queries
+      */
+      LOG(prefix<<"Removing NS record '"<<rec->d_name<<"|"<<DNSRecordContent::NumberToType(rec->d_type)<<"|"<<rec->d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section of a response received from "<<auth<<endl);
+      rec = lwr.d_records.erase(rec);
+      continue;
+    }
+
     if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::NS) {
       allowAdditionalEntry(allowedAdditionals, *rec);
     }