From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Fri, 7 May 2021 15:25:01 +0000 (+0200)
Subject: rec: Prevent a race in the aggressive NSEC cache
X-Git-Tag: dnsdist-1.7.0-alpha0~14^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F10375%2Fhead;p=thirdparty%2Fpdns.git

rec: Prevent a race in the aggressive NSEC cache

When a new NSEC3 record has a different salt than the one we know, we
update the zone entry with the new salt. Unfortunately, that salt was
read without holding the lock in `AggressiveNSECCache::getNSEC3Denial`,
leading to a possible data race.
---

diff --git a/pdns/recursordist/aggressive_nsec.cc b/pdns/recursordist/aggressive_nsec.cc
index a27635b292..4c9c708bdb 100644
--- a/pdns/recursordist/aggressive_nsec.cc
+++ b/pdns/recursordist/aggressive_nsec.cc
@@ -51,7 +51,6 @@ std::shared_ptr<AggressiveNSECCache::ZoneEntry> AggressiveNSECCache::getBestZone
 
 std::shared_ptr<AggressiveNSECCache::ZoneEntry> AggressiveNSECCache::getZone(const DNSName& zone)
 {
-  std::shared_ptr<AggressiveNSECCache::ZoneEntry> entry{nullptr};
   {
     ReadLock rl(d_lock);
     auto got = d_zones.lookup(zone);
@@ -60,8 +59,7 @@ std::shared_ptr<AggressiveNSECCache::ZoneEntry> AggressiveNSECCache::getZone(con
     }
   }
 
-  entry = std::make_shared<ZoneEntry>();
-  entry->d_zone = zone;
+  auto entry = std::make_shared<ZoneEntry>(zone);
 
   {
     WriteLock wl(d_lock);
@@ -506,9 +504,19 @@ bool AggressiveNSECCache::synthesizeFromNSECWildcard(time_t now, const DNSName&
 
 bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveNSECCache::ZoneEntry>& zoneEntry, std::vector<DNSRecord>& soaSet, std::vector<std::shared_ptr<RRSIGRecordContent>>& soaSignatures, const DNSName& name, const QType& type, std::vector<DNSRecord>& ret, int& res, bool doDNSSEC)
 {
-  const auto& salt = zoneEntry->d_salt;
-  const auto iterations = zoneEntry->d_iterations;
-  const auto& zone = zoneEntry->d_zone;
+  DNSName zone;
+  std::string salt;
+  uint16_t iterations;
+
+  {
+    std::unique_lock<std::mutex> lock(zoneEntry->d_lock, std::try_to_lock);
+    if (!lock.owns_lock()) {
+      return false;
+    }
+    salt = zoneEntry->d_salt;
+    zone = zoneEntry->d_zone;
+    iterations = zoneEntry->d_iterations;
+  }
 
   auto nameHash = DNSName(toBase32Hex(hashQNameWithSalt(salt, iterations, name))) + zone;
 
@@ -541,7 +549,7 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
     LOG(": done!" << endl);
     ++d_nsec3Hits;
     res = RCode::NoError;
-    addToRRSet(now, soaSet, soaSignatures, zoneEntry->d_zone, doDNSSEC, ret);
+    addToRRSet(now, soaSet, soaSignatures, zone, doDNSSEC, ret);
     addRecordToRRSet(now, exactNSEC3.d_owner, QType::NSEC3, exactNSEC3.d_ttd - now, exactNSEC3.d_record, exactNSEC3.d_signatures, doDNSSEC, ret);
     return true;
   }
@@ -642,7 +650,7 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
     res = RCode::NXDomain;
   }
 
-  addToRRSet(now, soaSet, soaSignatures, zoneEntry->d_zone, doDNSSEC, ret);
+  addToRRSet(now, soaSet, soaSignatures, zone, doDNSSEC, ret);
   addRecordToRRSet(now, closestNSEC3.d_owner, QType::NSEC3, closestNSEC3.d_ttd - now, closestNSEC3.d_record, closestNSEC3.d_signatures, doDNSSEC, ret);
   addRecordToRRSet(now, nextCloserEntry.d_owner, QType::NSEC3, nextCloserEntry.d_ttd - now, nextCloserEntry.d_record, nextCloserEntry.d_signatures, doDNSSEC, ret);
   addRecordToRRSet(now, wcEntry.d_owner, QType::NSEC3, wcEntry.d_ttd - now, wcEntry.d_record, wcEntry.d_signatures, doDNSSEC, ret);
diff --git a/pdns/recursordist/aggressive_nsec.hh b/pdns/recursordist/aggressive_nsec.hh
index 2a067b81b4..3c17c9882a 100644
--- a/pdns/recursordist/aggressive_nsec.hh
+++ b/pdns/recursordist/aggressive_nsec.hh
@@ -79,7 +79,8 @@ public:
 private:
   struct ZoneEntry
   {
-    ZoneEntry()
+    ZoneEntry(const DNSName& zone) :
+      d_zone(zone)
     {
     }
 
@@ -120,7 +121,7 @@ private:
       cache_t;
 
     cache_t d_entries;
-    DNSName d_zone;
+    const DNSName d_zone;
     std::string d_salt;
     std::mutex d_lock;
     uint16_t d_iterations{0};