From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 22 Feb 2021 16:21:50 +0000 (+0100)
Subject: rec: Check that the salt and iterations count match for NSEC3 entries
X-Git-Tag: dnsdist-1.6.0-alpha2~12^2~13
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=684b0a56680ff0fb9ac4321ccba9966a1a54ce39;p=thirdparty%2Fpdns.git

rec: Check that the salt and iterations count match for NSEC3 entries

Otherwise we could end up using a hash computed with the wrong parameters,
and thus not proving what we expected.
---

diff --git a/pdns/recursordist/aggressive_nsec.cc b/pdns/recursordist/aggressive_nsec.cc
index 0b0b74d4dc..053dd2c3a9 100644
--- a/pdns/recursordist/aggressive_nsec.cc
+++ b/pdns/recursordist/aggressive_nsec.cc
@@ -230,6 +230,7 @@ void AggressiveNSECCache::insertNSEC(const DNSName& zone, const DNSName& owner,
   {
     std::lock_guard<std::mutex> lock(entry->d_lock);
     if (nsec3 && !entry->d_nsec3) {
+      d_entriesCount -= entry->d_entries.size();
       entry->d_entries.clear();
       entry->d_nsec3 = true;
     }
@@ -276,8 +277,16 @@ void AggressiveNSECCache::insertNSEC(const DNSName& zone, const DNSName& owner,
 
 #warning Ponder storing everything in raw form, without the zone instead. It still needs to be a DNSName for NSEC, though
       next = DNSName(toBase32Hex(content->d_nexthash)) + zone;
-      entry->d_iterations = content->d_iterations;
-      entry->d_salt = content->d_salt;
+
+      if (entry->d_iterations != content->d_iterations || entry->d_salt != content->d_salt) {
+        entry->d_iterations = content->d_iterations;
+        entry->d_salt = content->d_salt;
+
+        // Clearing the existing entries since we can't use them, and it's likely a rollover
+        // If it instead is different servers using different parameters, well, too bad.
+        d_entriesCount -= entry->d_entries.size();
+        entry->d_entries.clear();
+      }
     }
 
     /* the TTL is already a TTD by now */
@@ -337,8 +346,6 @@ bool AggressiveNSECCache::getNSECBefore(time_t now, std::shared_ptr<AggressiveNS
 
   if (it->d_ttd <= now) {
     moveCacheItemToFront<ZoneEntry::SequencedTag>(zoneEntry->d_entries, it);
-    //idx.erase(it);
-    //--d_entriesCount;
     return false;
   }
 
@@ -366,8 +373,6 @@ bool AggressiveNSECCache::getNSEC3(time_t now, std::shared_ptr<AggressiveNSECCac
     auto firstIndexIterator = zoneEntry->d_entries.project<ZoneEntry::OrderedTag>(it);
     if (it->d_ttd <= now) {
       moveCacheItemToBack<ZoneEntry::SequencedTag>(zoneEntry->d_entries, firstIndexIterator);
-      //idx.erase(it);
-      //--d_entriesCount;
       return false;
     }
 
@@ -490,8 +495,8 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
   if (getNSEC3(now, zoneEntry, nameHash, exactNSEC3)) {
     LOG("Found a direct NSEC3 match for "<<nameHash);
     auto nsec3 = std::dynamic_pointer_cast<NSEC3RecordContent>(exactNSEC3.d_record);
-    if (!nsec3) {
-      LOG(" but the content is not valid"<<endl);
+    if (!nsec3 || nsec3->d_iterations != iterations || nsec3->d_salt != salt) {
+      LOG(" but the content is not valid, or has a different salt or iterations count"<<endl);
       return false;
     }
 
@@ -529,6 +534,13 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
 
     if (getNSEC3(now, zoneEntry, closestHash, closestNSEC3)) {
       LOG("Found closest encloser at "<<closestEncloser<<" ("<<closestHash<<")"<<endl);
+
+      auto nsec3 = std::dynamic_pointer_cast<NSEC3RecordContent>(closestNSEC3.d_record);
+      if (!nsec3 || nsec3->d_iterations != iterations || nsec3->d_salt != salt) {
+        LOG(" but the content is not valid, or has a different salt or iterations count"<<endl);
+        break;
+      }
+
       found = true;
       break;
     }
@@ -561,6 +573,12 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
     return false;
   }
 
+  auto nextCloserNsec3 = std::dynamic_pointer_cast<NSEC3RecordContent>(nextCloserEntry.d_record);
+  if (!nextCloserNsec3 || nextCloserNsec3->d_iterations != iterations || nextCloserNsec3->d_salt != salt) {
+    LOG("The NSEC3 covering the next closer is not valid, or has a different salt or iterations count, bailing out"<<endl);
+    return false;
+  }
+
   DNSName wildcard(g_wildcarddnsname + closestEncloser);
   auto wcHash = toBase32Hex(hashQNameWithSalt(salt, iterations, wildcard));
   LOG("Looking for a NSEC3 covering the wildcard "<<wildcard<<" ("<<wcHash<<")"<<endl);
@@ -575,8 +593,8 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
     LOG("Found an exact match for the wildcard");
 
     auto nsec3 = std::dynamic_pointer_cast<NSEC3RecordContent>(wcEntry.d_record);
-    if (!nsec3) {
-      LOG(" but the content is not valid"<<endl);
+    if (!nsec3 || nsec3->d_iterations != iterations || nsec3->d_salt != salt) {
+      LOG(" but the content is not valid, or has a different salt or iterations count"<<endl);
       return false;
     }
 
@@ -593,6 +611,13 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
       LOG("No covering record found for the wildcard in aggressive cache"<<endl);
       return false;
     }
+
+    auto nsec3 = std::dynamic_pointer_cast<NSEC3RecordContent>(wcEntry.d_record);
+    if (!nsec3 || nsec3->d_iterations != iterations || nsec3->d_salt != salt) {
+      LOG("The content of the NSEC3 covering the wildcard is not valid, or has a different salt or iterations count"<<endl);
+      return false;
+    }
+
     res = RCode::NXDomain;
   }
 
@@ -616,6 +641,7 @@ bool AggressiveNSECCache::getDenial(time_t now, const DNSName& name, const QType
   vState cachedState;
   std::vector<DNSRecord> soaSet;
   std::vector<std::shared_ptr<RRSIGRecordContent>> soaSignatures;
+  /* we might not actually need the SOA if we find a matching wildcard, but let's not bother for now */
   if (g_recCache->get(now, zoneEntry->d_zone, QType::SOA, true, &soaSet, who, false, routingTag, doDNSSEC ? &soaSignatures : nullptr, nullptr, nullptr, &cachedState) <= 0 || cachedState != vState::Secure) {
     LOG("No valid SOA found for "<<zoneEntry->d_zone<<", which is the best match for "<<name<<endl);
     return false;
diff --git a/pdns/recursordist/test-aggressive_nsec_cc.cc b/pdns/recursordist/test-aggressive_nsec_cc.cc
index 5b5ae9c1e0..8be1c1f29e 100644
--- a/pdns/recursordist/test-aggressive_nsec_cc.cc
+++ b/pdns/recursordist/test-aggressive_nsec_cc.cc
@@ -881,7 +881,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec_dump)
   expected.push_back("z.powerdns.com. 10 IN NSEC zz.powerdns.com. AAAA RRSIG NSEC\n");
   expected.push_back("- RRSIG NSEC 5 3 10 20370101000000 20370101000000 24567 dummy. data\n");
   expected.push_back("; Zone powerdns.org.\n");
-  expected.push_back("www.powerdns.org. 10 IN NSEC3 1 0 500 ab HASG==== A RRSIG NSEC3\n");
+  expected.push_back("www.powerdns.org. 10 IN NSEC3 1 0 50 ab HASG==== A RRSIG NSEC3\n");
   expected.push_back("- RRSIG NSEC3 5 3 10 20370101000000 20370101000000 24567 dummy. data\n");
 
   struct timeval now;
@@ -902,7 +902,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec_dump)
   rec.d_name = DNSName("www.powerdns.org");
   rec.d_type = QType::NSEC3;
   rec.d_ttl = now.tv_sec + 10;
-  rec.d_content = getRecordContent(QType::NSEC3, "1 0 500 ab HASG==== A RRSIG NSEC3");
+  rec.d_content = getRecordContent(QType::NSEC3, "1 0 50 ab HASG==== A RRSIG NSEC3");
   rrsig = std::make_shared<RRSIGRecordContent>("NSEC3 5 3 10 20370101000000 20370101000000 24567 dummy. data");
   cache->insertNSEC(DNSName("powerdns.org"), rec.d_name, rec, {rrsig}, true);
 
@@ -934,4 +934,126 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec_dump)
   free(line);
 }
 
+BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_rollover)
+{
+  /* test that we don't compare a hash using the wrong (former) salt or iterations count in case of a rollover,
+     or when different servers use different parameters */
+  auto cache = make_unique<AggressiveNSECCache>(10000);
+  g_recCache = std::make_unique<MemRecursorCache>();
+
+  const DNSName zone("powerdns.com");
+  time_t now = time(nullptr);
+
+  /* first we need a SOA */
+  std::vector<DNSRecord> records;
+  time_t ttd = now + 30;
+  DNSRecord drSOA;
+  drSOA.d_name = zone;
+  drSOA.d_type = QType::SOA;
+  drSOA.d_class = QClass::IN;
+  drSOA.d_content = std::make_shared<SOARecordContent>("pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600");
+  drSOA.d_ttl = static_cast<uint32_t>(ttd); // XXX truncation
+  drSOA.d_place = DNSResourceRecord::ANSWER;
+  records.push_back(drSOA);
+
+  g_recCache->replace(now, zone, QType(QType::SOA), records, {}, {}, true, zone, boost::none, boost::none, vState::Secure);
+  BOOST_CHECK_EQUAL(g_recCache->size(), 1U);
+
+  std::string oldSalt = "ab";
+  std::string newSalt = "cd";
+  unsigned int oldIterationsCount = 2;
+  unsigned int newIterationsCount = 1;
+  DNSName name("www.powerdns.com");
+  std::string hashed = hashQNameWithSalt(oldSalt, oldIterationsCount, name);
+
+  DNSRecord rec;
+  rec.d_name = DNSName(toBase32Hex(hashed)) + zone;
+  rec.d_type = QType::NSEC3;
+  rec.d_ttl = now + 10;
+
+  NSEC3RecordContent nrc;
+  nrc.d_algorithm = 1;
+  nrc.d_flags = 0;
+  nrc.d_iterations = oldIterationsCount;
+  nrc.d_salt = oldSalt;
+  nrc.d_nexthash = hashed;
+  incrementHash(nrc.d_nexthash);
+  for (const auto& type : { QType::A }) {
+    nrc.set(type);
+  }
+
+  rec.d_content = std::make_shared<NSEC3RecordContent>(nrc);
+  auto rrsig = std::make_shared<RRSIGRecordContent>("NSEC3 5 3 10 20370101000000 20370101000000 24567 dummy. data");
+  cache->insertNSEC(zone, rec.d_name, rec, {rrsig}, true);
+
+  BOOST_CHECK_EQUAL(cache->getEntriesCount(), 1);
+
+  int res;
+  std::vector<DNSRecord> results;
+
+  /* we can use the NSEC3s we have */
+  /* direct match */
+  BOOST_CHECK_EQUAL(cache->getDenial(now, name, QType::AAAA, results, res, ComboAddress("192.0.2.1"), boost::none, true), true);
+
+  DNSName other("other.powerdns.com");
+  /* now we insert a new NSEC3, with a different salt, changing that value for the zone */
+  hashed = hashQNameWithSalt(newSalt, oldIterationsCount, other);
+  rec.d_name = DNSName(toBase32Hex(hashed)) + zone;
+  rec.d_type = QType::NSEC3;
+  rec.d_ttl = now + 10;
+  nrc.d_algorithm = 1;
+  nrc.d_flags = 0;
+  nrc.d_iterations = oldIterationsCount;
+  nrc.d_salt = newSalt;
+  nrc.d_nexthash = hashed;
+  incrementHash(nrc.d_nexthash);
+  for (const auto& type : { QType::A }) {
+    nrc.set(type);
+  }
+
+  rec.d_content = std::make_shared<NSEC3RecordContent>(nrc);
+  rrsig = std::make_shared<RRSIGRecordContent>("NSEC3 5 3 10 20370101000000 20370101000000 24567 dummy. data");
+  cache->insertNSEC(zone, rec.d_name, rec, {rrsig}, true);
+
+  /* the existing entries should have been cleared */
+  BOOST_CHECK_EQUAL(cache->getEntriesCount(), 1);
+
+  /* we should be able to find a direct match for that name */
+  /* direct match */
+  BOOST_CHECK_EQUAL(cache->getDenial(now, other, QType::AAAA, results, res, ComboAddress("192.0.2.1"), boost::none, true), true);
+
+  /* but we should not be able to use the other NSEC3s */
+  BOOST_CHECK_EQUAL(cache->getDenial(now, name, QType::AAAA, results, res, ComboAddress("192.0.2.1"), boost::none, true), false);
+
+  /* and the same thing but this time updating the iterations count instead of the salt */
+  DNSName other2("other2.powerdns.com");
+  hashed = hashQNameWithSalt(newSalt, newIterationsCount, other2);
+  rec.d_name = DNSName(toBase32Hex(hashed)) + zone;
+  rec.d_type = QType::NSEC3;
+  rec.d_ttl = now + 10;
+  nrc.d_algorithm = 1;
+  nrc.d_flags = 0;
+  nrc.d_iterations = newIterationsCount;
+  nrc.d_salt = newSalt;
+  nrc.d_nexthash = hashed;
+  incrementHash(nrc.d_nexthash);
+  for (const auto& type : { QType::A }) {
+    nrc.set(type);
+  }
+
+  rec.d_content = std::make_shared<NSEC3RecordContent>(nrc);
+  rrsig = std::make_shared<RRSIGRecordContent>("NSEC3 5 3 10 20370101000000 20370101000000 24567 dummy. data");
+  cache->insertNSEC(zone, rec.d_name, rec, {rrsig}, true);
+
+  /* the existing entries should have been cleared */
+  BOOST_CHECK_EQUAL(cache->getEntriesCount(), 1);
+
+  /* we should be able to find a direct match for that name */
+  /* direct match */
+  BOOST_CHECK_EQUAL(cache->getDenial(now, other2, QType::AAAA, results, res, ComboAddress("192.0.2.1"), boost::none, true), true);
+
+  /* but we should not be able to use the other NSEC3s */
+  BOOST_CHECK_EQUAL(cache->getDenial(now, other, QType::AAAA, results, res, ComboAddress("192.0.2.1"), boost::none, true), false);
+}
+
 BOOST_AUTO_TEST_SUITE_END()