From: bert hubert <bert.hubert@netherlabs.nl>
Date: Wed, 23 Mar 2016 12:00:35 +0000 (+0100)
Subject: Closes #3390 by fixing validation of provably insecure delegation for NSEC records... 
X-Git-Tag: dnsdist-1.0.0-beta1~69^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3e9c6c0add512c63cc9aed4b18611944cdfc56e0;p=thirdparty%2Fpdns.git

Closes #3390 by fixing validation of provably insecure delegation for NSEC records, plus improving our DNSSEC validation logging with --trace output on
---

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index e0d54a5519..c28194d6c7 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -2358,6 +2358,7 @@ int serviceMain(int argc, char*argv[])
     SyncRes::setDefaultLogMode(SyncRes::Log);
     ::arg().set("quiet")="no";
     g_quiet=false;
+    g_dnssecLOG=true;
   }
 
   SyncRes::s_minimumTTL = ::arg().asNum("minimum-ttl-override");
diff --git a/pdns/validate-recursor.cc b/pdns/validate-recursor.cc
index d337fc445e..4057b30f84 100644
--- a/pdns/validate-recursor.cc
+++ b/pdns/validate-recursor.cc
@@ -1,9 +1,12 @@
 #include "validate.hh"
 #include "validate-recursor.hh"
 #include "syncres.hh"
+#include "logger.hh"
 
 DNSSECMode g_dnssecmode{DNSSECMode::Process};
 
+#define LOG(x) if(g_dnssecLOG) { L <<Logger::Warning << x; }
+
 class SRRecordOracle : public DNSRecordOracle
 {
 public:
@@ -29,10 +32,10 @@ vState validateRecords(const vector<DNSRecord>& recs)
     return Insecure; // can't secure nothing 
 
   cspmap_t cspmap=harvestCSPFromRecs(recs);
-  //  cerr<<"Got "<<cspmap.size()<<" RRSETs: ";
+  LOG("Got "<<cspmap.size()<<" RRSETs: "<<endl);
   int numsigs=0;
   for(const auto& csp : cspmap) {
-    //    cerr<<"Going to validate: "<<csp.first.first<<'/'<<DNSRecordContent::NumberToType(csp.first.second)<<": "<<csp.second.signatures.size()<<" sigs for "<<csp.second.records.size()<<" records"<<endl;
+    LOG("Going to validate: "<<csp.first.first<<"/"<<DNSRecordContent::NumberToType(csp.first.second)<<": "<<csp.second.signatures.size()<<" sigs for "<<csp.second.records.size()<<" records"<<endl);
     numsigs+= csp.second.signatures.size();
   }
    
@@ -46,7 +49,7 @@ vState validateRecords(const vector<DNSRecord>& recs)
     for(const auto& csp : cspmap) {
       for(const auto& sig : csp.second.signatures) {
         state = getKeysFor(sro, sig->d_signer, keys); // XXX check validity here
-        //	cerr<<"! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys"<<endl;
+        LOG("! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys"<<endl);
         // this sort of charges on and 'state' ends up as the last thing to have been checked
         // maybe not the right idea
       }
@@ -57,14 +60,14 @@ vState validateRecords(const vector<DNSRecord>& recs)
     validateWithKeySet(cspmap, validrrsets, keys);
   }
   else {
-    //    cerr<<"no sigs, hoping for Insecure"<<endl;
+    LOG("! no sigs, hoping for Insecure status of "<<recs.begin()->d_name<<endl);
     state = getKeysFor(sro, recs.begin()->d_name, keys); // um WHAT DOES THIS MEAN - try first qname??
    
-    //    cerr<<"! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys "<<endl;
+    LOG("! state = "<<vStates[state]<<", now have "<<keys.size()<<" keys "<<endl);
     return state;
   }
   
-  //  cerr<<"Took "<<sro.d_queries<<" queries"<<endl;
+  LOG("Took "<<sro.d_queries<<" queries"<<endl);
   if(validrrsets.size() == cspmap.size()) // shortcut - everything was ok
     return Secure;
 
@@ -85,9 +88,9 @@ vState validateRecords(const vector<DNSRecord>& recs)
 #endif
   //  cerr<<"Input to validate: "<<endl;
   for(const auto& csp : cspmap) {
-    cerr<<csp.first.first<<"|"<<csp.first.second<<" with "<<csp.second.signatures.size()<<" signatures"<<endl;
+    LOG(csp.first.first<<"|"<<csp.first.second<<" with "<<csp.second.signatures.size()<<" signatures"<<endl);
     if(!csp.second.signatures.empty() && !validrrsets.count(csp.first)) {
-      //      cerr<<"Lacks signature, must have one"<<endl;
+      LOG("Lacks signature, must have one"<<endl);
       return Bogus;
     }
   }
diff --git a/pdns/validate.cc b/pdns/validate.cc
index f3a7645460..6dc897c79b 100644
--- a/pdns/validate.cc
+++ b/pdns/validate.cc
@@ -4,7 +4,9 @@
 #include "rec-lua-conf.hh"
 #include "base32.hh"
 #include "logger.hh"
+bool g_dnssecLOG{false};
 
+#define LOG(x) if(g_dnssecLOG) { L <<Logger::Warning << x; }
 void dotEdge(DNSName zone, string type1, DNSName name1, string tag1, string type2, DNSName name2, string tag2, string color="");
 void dotNode(string type, DNSName name, string tag, string content);
 string dotName(string type, DNSName name, string tag);
@@ -104,11 +106,11 @@ void validateWithKeySet(const cspmap_t& rrsets, cspmap_t& validated, const keyse
 	  if(signature->d_siginception < now && signature->d_sigexpire > now)
 	    isValid = DNSCryptoKeyEngine::makeFromPublicKeyString(l.d_algorithm, l.d_key)->verify(msg, signature->d_signature);
 	  else {
-	    DLOG(cerr<<"signature is expired/not yet valid"<<endl);
+	    LOG("signature is expired/not yet valid"<<endl);
           }
 	}
 	catch(std::exception& e) {
-	  DLOG(cerr<<"Error validating with engine: "<<e.what()<<endl);
+	  LOG("Error validating with engine: "<<e.what()<<endl);
 	}
 	if(isValid) {
 	  validated[i->first] = i->second;
@@ -116,7 +118,7 @@ void validateWithKeySet(const cspmap_t& rrsets, cspmap_t& validated, const keyse
 	  //	  cerr<<"! validated "<<i->first.first<<"/"<<DNSRecordContent::NumberToType(signature->d_type)<<endl;
 	}
 	else {
-          DLOG(cerr<<"signature invalid"<<endl);
+          LOG("signature invalid"<<endl);
         }
 	if(signature->d_type != QType::DNSKEY) {
 	  dotEdge(signature->d_signer,
@@ -287,7 +289,7 @@ vState getKeysFor(DNSRecordOracle& dro, const DNSName& zone, keyset_t &keyset)
             break;
           }
 	  else {
-	    DLOG(cerr<<"Validation did not succeed!"<<endl);
+	    LOG("Validation did not succeed!"<<endl);
           }
         }
 	//        if(validkeys.empty()) cerr<<"did not manage to validate DNSKEY set based on DS-validated KSK, only passing KSK on"<<endl;
@@ -325,9 +327,29 @@ vState getKeysFor(DNSRecordOracle& dro, const DNSName& zone, keyset_t &keyset)
       cspmap_t validrrsets;
       validateWithKeySet(cspmap, validrrsets, validkeys);
 
-      //      cerr<<"got "<<cspmap.count(make_pair(qname,QType::DS))<<" DS of which "<<validrrsets.count(make_pair(qname,QType::DS))<<" valid "<<endl;
+      LOG("got "<<cspmap.count(make_pair(qname,QType::DS))<<" records for DS query of which "<<validrrsets.count(make_pair(qname,QType::DS))<<" valid "<<endl);
 
       auto r = validrrsets.equal_range(make_pair(qname, QType::DS));
+      if(r.first == r.second) {
+        LOG("No DS for "<<qname<<", now look for a secure denial"<<endl);
+
+        for(const auto& v : validrrsets) {
+          LOG("Do have: "<<v.first.first<<"/"<<DNSRecordContent::NumberToType(v.first.second)<<endl);
+          if(v.first.second==QType::NSEC) { // check that it covers us!
+            for(const auto& r : v.second.records) {
+              LOG("\t"<<r->getZoneRepresentation()<<endl);
+              auto nsec = std::dynamic_pointer_cast<NSECRecordContent>(r);
+              if(v.first.first == qname && !nsec->d_set.count(QType::DS))
+                return Insecure;
+              else {
+                LOG("Did not deny existence of DS, "<<v.first.first<<"?="<<qname<<", "<<nsec->d_set.count(QType::DS)<<endl);
+              }
+            }
+
+          }
+        }
+        return Bogus;
+      }
       for(auto cspiter =r.first;  cspiter!=r.second; cspiter++) {
         for(auto j=cspiter->second.records.cbegin(); j!=cspiter->second.records.cend(); j++)
         {
diff --git a/pdns/validate.hh b/pdns/validate.hh
index 73011cfbdc..f72a98cfc7 100644
--- a/pdns/validate.hh
+++ b/pdns/validate.hh
@@ -6,6 +6,8 @@
 #include "namespaces.hh"
 #include "dnsrecords.hh"
  
+extern bool g_dnssecLOG;
+
 // 4033 5
 enum vState { Indeterminate, Bogus, Insecure, Secure };
 extern const char *vStates[];