From 114829cc7f51a8d4cd34c84d854dc5a03ba3dec7 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 31 Jul 2017 18:40:16 +0200
Subject: [PATCH] rec: Be more careful about the validation of negative answers

We skipped the validation of:
- ECS-specific negative answers
- completely empty NODATA answers (no SOA, no NSEC, nothing)
---
 pdns/recursordist/test-syncres_cc.cc | 57 ++++++++++++++++++++++++++++
 pdns/syncres.cc                      | 38 +++++++++++--------
 2 files changed, 80 insertions(+), 15 deletions(-)

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index cca6af233..391ace836 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -6588,6 +6588,63 @@ BOOST_AUTO_TEST_CASE(test_dnssec_no_ta) {
   BOOST_CHECK_EQUAL(queriesCount, 1);
 }
 
+BOOST_AUTO_TEST_CASE(test_dnssec_bogus_nodata) {
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr, true);
+
+  g_dnssecmode = DNSSECMode::ValidateAll;
+
+  primeHints();
+  const DNSName target("powerdns.com.");
+  testkeysset_t keys;
+
+  auto luaconfsCopy = g_luaconfs.getCopy();
+  luaconfsCopy.dsAnchors.clear();
+  generateKeyMaterial(DNSName("."), DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys);
+  g_luaconfs.setState(luaconfsCopy);
+
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([target,&queriesCount,keys](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, std::shared_ptr<RemoteLogger> outgoingLogger, LWResult* res) {
+      queriesCount++;
+
+      if (type == QType::DS) {
+        return 0;
+      }
+      else if (type == QType::DNSKEY) {
+        setLWResult(res, 0, true, false, true);
+        addDNSKEY(keys, domain, 300, res->d_records);
+        addRRSIG(keys, res->d_records, domain, 300);
+        return 1;
+      }
+      else {
+
+        setLWResult(res, 0, true, false, true);
+        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_CHECK_EQUAL(sr->getValidationState(), Bogus);
+  BOOST_REQUIRE_EQUAL(ret.size(), 0);
+  /* com|NS, powerdns.com|NS, powerdns.com|A */
+  BOOST_CHECK_EQUAL(queriesCount, 3);
+
+  /* again, to test the cache */
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
+  BOOST_REQUIRE_EQUAL(ret.size(), 0);
+  /* we don't store empty results */
+  BOOST_CHECK_EQUAL(queriesCount, 6);
+}
+
 /*
 // cerr<<"asyncresolve called to ask "<<ip.toStringWithPort()<<" about "<<domain.toString()<<" / "<<QType(type).getName()<<" over "<<(doTCP ? "TCP" : "UDP")<<" (rd: "<<sendRDQuery<<", EDNS0 level: "<<EDNS0Level<<")"<<endl;
 
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 0f608abfb..0d2e124ba 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1899,15 +1899,17 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
       rec.d_ttl = min(rec.d_ttl, s_maxnegttl);
       if(newtarget.empty()) // only add a SOA if we're not going anywhere after this
         ret.push_back(rec);
-      if(!wasVariable()) {
-        NegCache::NegCacheEntry ne;
 
-        ne.d_ttd = d_now.tv_sec + rec.d_ttl;
-        ne.d_name = qname;
-        ne.d_qtype = QType(0); // this encodes 'whole record'
-        ne.d_auth = rec.d_name;
-        harvestNXRecords(lwr.d_records, ne);
-        getDenialValidationState(ne, state, NXDOMAIN, false);
+      NegCache::NegCacheEntry ne;
+
+      ne.d_ttd = d_now.tv_sec + rec.d_ttl;
+      ne.d_name = qname;
+      ne.d_qtype = QType(0); // this encodes 'whole record'
+      ne.d_auth = rec.d_name;
+      harvestNXRecords(lwr.d_records, ne);
+      getDenialValidationState(ne, state, NXDOMAIN, false);
+
+      if(!wasVariable()) {
         t_sstorage.negcache.add(ne);
         if(s_rootNXTrust && ne.d_auth.isRoot() && auth.isRoot()) {
           ne.d_name = ne.d_name.getLastLabel();
@@ -2000,14 +2002,16 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
       else {
         rec.d_ttl = min(s_maxnegttl, rec.d_ttl);
         ret.push_back(rec);
+
+        NegCache::NegCacheEntry ne;
+        ne.d_auth = rec.d_name;
+        ne.d_ttd = d_now.tv_sec + rec.d_ttl;
+        ne.d_name = qname;
+        ne.d_qtype = qtype;
+        harvestNXRecords(lwr.d_records, ne);
+        getDenialValidationState(ne, state, NXQTYPE, qtype == QType::DS);
+
         if(!wasVariable()) {
-          NegCache::NegCacheEntry ne;
-          ne.d_auth = rec.d_name;
-          ne.d_ttd = d_now.tv_sec + rec.d_ttl;
-          ne.d_name = qname;
-          ne.d_qtype = qtype;
-          harvestNXRecords(lwr.d_records, ne);
-          getDenialValidationState(ne, state, NXQTYPE, qtype == QType::DS);
           if(qtype.getCode()) {  // prevents us from blacking out a whole domain
             t_sstorage.negcache.add(ne);
           }
@@ -2212,6 +2216,10 @@ bool SyncRes::processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qn
   if(nsset.empty() && !lwr.d_rcode && (negindic || lwr.d_aabit || sendRDQuery)) {
     LOG(prefix<<qname<<": status=noerror, other types may exist, but we are done "<<(negindic ? "(have negative SOA) " : "")<<(lwr.d_aabit ? "(have aa bit) " : "")<<endl);
 
+    if(state == Secure && lwr.d_aabit && !negindic) {
+      updateValidationState(state, Bogus);
+    }
+
     if(d_doDNSSEC)
       addNXNSECS(ret, lwr.d_records);
 
-- 
2.40.0