[WIP] rec: Skip NS records in authority/additional sections of NXD / NODATA

pdns/recursordist/test-syncres_cc.cc

@@ -10146,6 +10146,116 @@ BOOST_AUTO_TEST_CASE(test_getDSRecords_multialgo_two_highest) {
   }
 }
 
+BOOST_AUTO_TEST_CASE(test_cache_ns_nodata) {
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr);
+
+  primeHints();
+
+  const DNSName target("nodata.powerdns.com.");
+  const DNSName sub("sub.powerdns.com.");
+
+  sr->setAsyncCallback([sub](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) {
+
+      setLWResult(res, 0, true, false, true);
+      addRecordToLW(res, DNSName("powerdns.com."), QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600);
+      addRecordToLW(res, sub, QType::NS, "totally-legit-ns.evil.", DNSResourceRecord::AUTHORITY, 86400);
+      addRecordToLW(res, sub, QType::NS, "totally-legit-ns.evil.", DNSResourceRecord::ADDITIONAL, 86400);
+
+      return 1;
+    });
+
+  const time_t now = sr->getNow().tv_sec;
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 1);
+  BOOST_REQUIRE_EQUAL(QType(ret.at(0).d_type).getName(), QType(QType::SOA).getName());
+
+  /* check that we correctly cached only the SOA entry, not the NS */
+  const ComboAddress who;
+  vector<DNSRecord> cached;
+  BOOST_REQUIRE_GT(t_RC->get(now, DNSName("powerdns.com."), QType(QType::SOA), true, &cached, who), 0);
+  BOOST_REQUIRE_EQUAL(cached.size(), 1);
+  BOOST_REQUIRE_EQUAL(QType(cached.at(0).d_type).getName(), QType(QType::SOA).getName());
+
+  cached.clear();
+  BOOST_REQUIRE_EQUAL(t_RC->get(now, sub, QType(QType::NS), false, &cached, who), -1);
+  BOOST_REQUIRE_EQUAL(cached.size(), 0);
+}
+
+BOOST_AUTO_TEST_CASE(test_cache_ns_nxdomain) {
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr);
+
+  primeHints();
+
+  const DNSName target("nxd.powerdns.com.");
+  const DNSName sub("sub.powerdns.com.");
+
+  sr->setAsyncCallback([sub](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) {
+
+      setLWResult(res, RCode::NXDomain, true, false, true);
+      addRecordToLW(res, DNSName("powerdns.com."), QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600);
+      addRecordToLW(res, sub, QType::NS, "totally-legit-ns.evil.", DNSResourceRecord::AUTHORITY, 86400);
+      addRecordToLW(res, sub, QType::NS, "totally-legit-ns.evil.", DNSResourceRecord::ADDITIONAL, 86400);
+
+      return 1;
+    });
+
+  const time_t now = sr->getNow().tv_sec;
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NXDomain);
+  BOOST_REQUIRE_EQUAL(ret.size(), 1);
+  BOOST_REQUIRE_EQUAL(QType(ret.at(0).d_type).getName(), QType(QType::SOA).getName());
+
+  /* check that we correctly cached only the SOA entry, not the NS */
+  const ComboAddress who;
+  vector<DNSRecord> cached;
+  BOOST_REQUIRE_GT(t_RC->get(now, DNSName("powerdns.com."), QType(QType::SOA), true, &cached, who), 0);
+  BOOST_REQUIRE_EQUAL(cached.size(), 1);
+  BOOST_REQUIRE_EQUAL(QType(cached.at(0).d_type).getName(), QType(QType::SOA).getName());
+
+  cached.clear();
+  BOOST_REQUIRE_EQUAL(t_RC->get(now, sub, QType(QType::NS), false, &cached, who), -1);
+  BOOST_REQUIRE_EQUAL(cached.size(), 0);
+}
+
+BOOST_AUTO_TEST_CASE(test_cache_ns_delegation) {
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr);
+
+  primeHints();
+
+  const DNSName target("www.powerdns.com.");
+
+  sr->setAsyncCallback([](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 (isRootServer(ip)) {
+        setLWResult(res, RCode::NoError, false, false, true);
+        addRecordToLW(res, DNSName("powerdns.com."), QType::NS, "pdns-public-ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 86400);
+        addRecordToLW(res, DNSName("pdns-public-ns1.powerdns.com."), QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 86400);
+        return 1;
+      }
+      else if (ip == ComboAddress("192.0.2.1:53")) {
+        setLWResult(res, RCode::NoError, true, false, true);
+        addRecordToLW(res, DNSName("wwW.powerdns.com."), QType::A, "192.0.2.42");
+        return 1;
+      }
+
+      return 0;
+    });
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 1);
+  BOOST_REQUIRE_EQUAL(QType(ret.at(0).d_type).getName(), QType(QType::A).getName());
+}
+
 /*
 // cerr<<"asyncresolve called to ask "<<ip.toStringWithPort()<<" about "<<domain.toString()<<" / "<<QType(type).getName()<<" over "<<(doTCP ? "TCP" : "UDP")<<" (rd: "<<sendRDQuery<<", EDNS0 level: "<<EDNS0Level<<")"<<endl;
 

pdns/syncres.cc

@@ -1984,6 +1984,10 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
   std::vector<std::shared_ptr<DNSRecord>> authorityRecs;
   const unsigned int labelCount = qname.countLabels();
   bool isCNAMEAnswer = false;
+  bool haveAnswers = false;
+  bool isNXDomain = false;
+  bool isNXQType = false;
+
   for(const auto& rec : lwr.d_records) {
     if (rec.d_class != QClass::IN) {
       continue;
@@ -2045,6 +2049,23 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
     }
 
     if(rec.d_name.isPartOf(auth)) {
+
+      if (!haveAnswers) {
+        if (rec.d_place == DNSResourceRecord::ANSWER) {
+          haveAnswers = true;
+        }
+        else if (rec.d_place == DNSResourceRecord::AUTHORITY &&
+                 rec.d_type == QType::SOA &&
+                 qname.isPartOf(rec.d_name)) {
+          if (lwr.d_rcode == RCode::NXDomain) {
+            isNXDomain = true;
+          }
+          else if (lwr.d_rcode == RCode::NoError) {
+            isNXQType = true;
+          }
+        }
+      }
+
       if(rec.d_type == QType::RRSIG) {
         LOG("RRSIG - separate"<<endl);
       }
@@ -2088,8 +2109,9 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
         tcache[{rec.d_name,rec.d_type,rec.d_place}].records.push_back(dr);
       }
     }
-    else
+    else {
       LOG("NO!"<<endl);
+    }
   }
 
   // supplant
@@ -2183,6 +2205,15 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
       }
     }
 
+    if ((isNXDomain || isNXQType) && i->first.type == QType::NS && (i->first.place == DNSResourceRecord::AUTHORITY || i->first.place == DNSResourceRecord::ADDITIONAL)) {
+      /* we don't want to pick up NS records in AUTHORITY or 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.
+      */
+      LOG(d_prefix<<"Discarding "<<QType(i->first.type).getName()<<" in "<<(i->first.place == DNSResourceRecord::AUTHORITY ? "authority" : "additional")<<" section since this answer is a "<<(isNXDomain ? "NXD" : "NXQTYPE")<<", in an answer for "<<qname<<"|"<<qtype.getName()<<" from "<<auth<<" nameserver"<<endl);
+      continue;
+    }
+
     /* We don't need to store NSEC3 records in the positive cache because:
        - we don't allow direct NSEC3 queries
        - denial of existence proofs in wildcard expanded positive responses are stored in authorityRecs

pdns/test-common.hh

@@ -6,23 +6,29 @@ static inline std::shared_ptr<DNSRecordContent> getRecordContent(uint16_t type,
 {
   std::shared_ptr<DNSRecordContent> result = nullptr;
 
-  if (type == QType::NS) {
-    result = std::make_shared<NSRecordContent>(DNSName(content));
+  try {
+    if (type == QType::NS) {
+      result = std::make_shared<NSRecordContent>(DNSName(content));
+    }
+    else if (type == QType::A) {
+      result = std::make_shared<ARecordContent>(ComboAddress(content));
+    }
+    else if (type == QType::AAAA) {
+      result = std::make_shared<AAAARecordContent>(ComboAddress(content));
+    }
+    else if (type == QType::CNAME) {
+      result = std::make_shared<CNAMERecordContent>(DNSName(content));
+    }
+    else if (type == QType::OPT) {
+      result = std::make_shared<OPTRecordContent>();
+    }
+    else {
+      result = DNSRecordContent::mastermake(type, QClass::IN, content);
+    }
   }
-  else if (type == QType::A) {
-    result = std::make_shared<ARecordContent>(ComboAddress(content));
-  }
-  else if (type == QType::AAAA) {
-    result = std::make_shared<AAAARecordContent>(ComboAddress(content));
-  }
-  else if (type == QType::CNAME) {
-    result = std::make_shared<CNAMERecordContent>(DNSName(content));
-  }
-  else if (type == QType::OPT) {
-    result = std::make_shared<OPTRecordContent>();
-  }
-  else {
-    result = DNSRecordContent::mastermake(type, QClass::IN, content);
+  catch(...) {
+    cerr<<"Error in getRecordContent() while parsing content '" << content <<"' for type "<<QType(type).getName()<<endl;
+    throw;
   }
 
   return result;