From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Wed, 17 Jun 2020 12:49:55 +0000 (+0200)
Subject: rec: Copy the negative cache entry before validating it
X-Git-Tag: dnsdist-1.5.0-rc4~23^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f7d35114ab4153d0456d893c8694a0e6afa7ea20;p=thirdparty%2Fpdns.git

rec: Copy the negative cache entry before validating it

Otherwise, in the unlikely case that:
- we need to go to the network in order to validate, for example to
  get or a DNSKEY ;
- the negative cache cleaning is run at that exact moment ;
- and the entry we have a pointer to gets wiped during that cleanup

we might trigger a heap-based use-after-free (read), possibly leading
to a crash if the memory has been reused already.
---

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 2585187427..3041e9ad50 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1407,15 +1407,14 @@ static void reapRecordsFromNegCacheEntryForValidation(tcache_t& tcache, const ve
  * \param ttl     The new TTL for these records
  * \param ret     The vector of DNSRecords that should contain the records with the modified TTL
  */
-static void addTTLModifiedRecords(const vector<DNSRecord>& records, const uint32_t ttl, vector<DNSRecord>& ret) {
-  for (const auto& rec : records) {
-    DNSRecord r(rec);
-    r.d_ttl = ttl;
-    ret.push_back(r);
+static void addTTLModifiedRecords(vector<DNSRecord>& records, const uint32_t ttl, vector<DNSRecord>& ret) {
+  for (auto& rec : records) {
+    rec.d_ttl = ttl;
+    ret.push_back(std::move(rec));
   }
 }
 
-void SyncRes::computeNegCacheValidationStatus(const NegCache::NegCacheEntry* ne, const DNSName& qname, const QType& qtype, const int res, vState& state, unsigned int depth)
+void SyncRes::computeNegCacheValidationStatus(const NegCache::NegCacheEntry& ne, const DNSName& qname, const QType& qtype, const int res, vState& state, unsigned int depth)
 {
   DNSName subdomain(qname);
   /* if we are retrieving a DS, we only care about the state of the parent zone */
@@ -1425,10 +1424,10 @@ void SyncRes::computeNegCacheValidationStatus(const NegCache::NegCacheEntry* ne,
   computeZoneCuts(subdomain, g_rootdnsname, depth);
 
   tcache_t tcache;
-  reapRecordsFromNegCacheEntryForValidation(tcache, ne->authoritySOA.records);
-  reapRecordsFromNegCacheEntryForValidation(tcache, ne->authoritySOA.signatures);
-  reapRecordsFromNegCacheEntryForValidation(tcache, ne->DNSSECRecords.records);
-  reapRecordsFromNegCacheEntryForValidation(tcache, ne->DNSSECRecords.signatures);
+  reapRecordsFromNegCacheEntryForValidation(tcache, ne.authoritySOA.records);
+  reapRecordsFromNegCacheEntryForValidation(tcache, ne.authoritySOA.signatures);
+  reapRecordsFromNegCacheEntryForValidation(tcache, ne.DNSSECRecords.records);
+  reapRecordsFromNegCacheEntryForValidation(tcache, ne.DNSSECRecords.signatures);
 
   for (const auto& entry : tcache) {
     // this happens when we did store signatures, but passed on the records themselves
@@ -1456,10 +1455,10 @@ void SyncRes::computeNegCacheValidationStatus(const NegCache::NegCacheEntry* ne,
   }
 
   if (state == Secure) {
-    vState neValidationState = ne->d_validationState;
+    vState neValidationState = ne.d_validationState;
     dState expectedState = res == RCode::NXDomain ? NXDOMAIN : NXQTYPE;
-    dState denialState = getDenialValidationState(*ne, state, expectedState, false);
-    updateDenialValidationState(neValidationState, ne->d_name, state, denialState, expectedState, qtype == QType::DS || expectedState == NXDOMAIN);
+    dState denialState = getDenialValidationState(ne, state, expectedState, false);
+    updateDenialValidationState(neValidationState, ne.d_name, state, denialState, expectedState, qtype == QType::DS || expectedState == NXDOMAIN);
   }
   if (state != Indeterminate) {
     /* validation succeeded, let's update the cache entry so we don't have to validate again */
@@ -1467,7 +1466,7 @@ void SyncRes::computeNegCacheValidationStatus(const NegCache::NegCacheEntry* ne,
     if (state == Bogus) {
       capTTD = d_now.tv_sec + s_maxbogusttl;
     }
-    t_sstorage.negcache.updateValidationStatus(ne->d_name, ne->d_qtype, state, capTTD);
+    t_sstorage.negcache.updateValidationStatus(ne.d_name, ne.d_qtype, state, capTTD);
   }
 }
 
@@ -1546,9 +1545,17 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const DNSName& authname, bool w
 
     state = cachedState;
 
+    /* let's do a full copy now, since:
+       - we know we are going to use the records ;
+       - we might have to go to the network in computeNegCacheValidationStatus(),
+       and our pointer might get invalidated during that time.
+    */
+    NegCache::NegCacheEntry negativeEntry = *ne;
+    ne = nullptr;
+
     if (!wasAuthZone && shouldValidate() && state == Indeterminate) {
       LOG(prefix<<qname<<": got Indeterminate state for records retrieved from the negative cache, validating.."<<endl);
-      computeNegCacheValidationStatus(ne, qname, qtype, res, state, depth);
+      computeNegCacheValidationStatus(negativeEntry, qname, qtype, res, state, depth);
 
       if (state != cachedState && state == Bogus) {
         sttl = std::min(sttl, s_maxbogusttl);
@@ -1556,11 +1563,11 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const DNSName& authname, bool w
     }
 
     // Transplant SOA to the returned packet
-    addTTLModifiedRecords(ne->authoritySOA.records, sttl, ret);
+    addTTLModifiedRecords(negativeEntry.authoritySOA.records, sttl, ret);
     if(d_doDNSSEC) {
-      addTTLModifiedRecords(ne->authoritySOA.signatures, sttl, ret);
-      addTTLModifiedRecords(ne->DNSSECRecords.records, sttl, ret);
-      addTTLModifiedRecords(ne->DNSSECRecords.signatures, sttl, ret);
+      addTTLModifiedRecords(negativeEntry.authoritySOA.signatures, sttl, ret);
+      addTTLModifiedRecords(negativeEntry.DNSSECRecords.records, sttl, ret);
+      addTTLModifiedRecords(negativeEntry.DNSSECRecords.signatures, sttl, ret);
     }
 
     LOG(prefix<<qname<<": updating validation state with negative cache content for "<<qname<<" to "<<vStates[state]<<endl);
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 8ff14ddf68..f4c791675f 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -866,7 +866,7 @@ private:
   vState getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int depth);
   dState getDenialValidationState(const NegCache::NegCacheEntry& ne, const vState state, const dState expectedState, bool referralToUnsigned);
   void updateDenialValidationState(vState& neValidationState, const DNSName& neName, vState& state, const dState denialState, const dState expectedState, bool allowOptOut);
-  void computeNegCacheValidationStatus(const NegCache::NegCacheEntry* ne, const DNSName& qname, const QType& qtype, const int res, vState& state, unsigned int depth);
+  void computeNegCacheValidationStatus(const NegCache::NegCacheEntry& ne, const DNSName& qname, const QType& qtype, const int res, vState& state, unsigned int depth);
   vState getTA(const DNSName& zone, dsmap_t& ds);
   bool haveExactValidationStatus(const DNSName& domain);
   vState getValidationStatus(const DNSName& subdomain, bool allowIndeterminate=true);