Check TSIG signature on IXFR

pdns/Makefile.am

@@ -198,6 +198,7 @@ pdns_server_SOURCES = \
 	statbag.cc statbag.hh \
 	stubresolver.cc stubresolver.hh \
 	tcpreceiver.cc tcpreceiver.hh \
+	tsigverifier.cc tsigverifier.hh \
 	tkey.cc \
 	ueberbackend.cc ueberbackend.hh \
 	unix_semaphore.cc \
@@ -584,6 +585,7 @@ ixplore_SOURCES = \
 	sillyrecords.cc \
 	sstuff.hh \
 	statbag.cc \
+	tsigverifier.cc tsigverifier.hh \
 	unix_utility.cc zoneparser-tng.cc
 
 ixplore_LDADD = $(LIBCRYPTO_LIBS)
@@ -735,6 +737,7 @@ tsig_tests_SOURCES = \
 	sstuff.hh \
 	statbag.cc \
 	tsig-tests.cc \
+	tsigverifier.cc tsigverifier.hh \
 	unix_utility.cc
 
 tsig_tests_LDADD = $(LIBCRYPTO_LIBS)

pdns/dnspacket.cc

@@ -469,7 +469,7 @@ bool DNSPacket::getTSIGDetails(TSIGRecordContent* trc, DNSName* keyname, string*
 
   bool gotit=false;
   for(MOADNSParser::answers_t::const_iterator i=mdp.d_answers.begin(); i!=mdp.d_answers.end(); ++i) {          
-    if(i->first.d_type == QType::TSIG) {
+    if(i->first.d_type == QType::TSIG && i->first.d_class == QType::ANY) {
       // cast can fail, f.e. if d_content is an UnknownRecordContent.
       shared_ptr<TSIGRecordContent> content = std::dynamic_pointer_cast<TSIGRecordContent>(i->first.d_content);
       if (!content) {
@@ -644,7 +644,10 @@ bool checkForCorrectTSIG(const DNSPacket* q, UeberBackend* B, DNSName* keyname,
 {
   string message;
 
-  q->getTSIGDetails(trc, keyname, &message);
+  if (!q->getTSIGDetails(trc, keyname, &message)) {
+    return false;
+  }
+
   uint64_t delta = std::abs((int64_t)trc->d_time - (int64_t)time(0));
   if(delta > trc->d_fudge) {
     L<<Logger::Error<<"Packet for '"<<q->qdomain<<"' denied: TSIG (key '"<<*keyname<<"') time delta "<< delta <<" > 'fudge' "<<trc->d_fudge<<endl;

pdns/dnsparser.cc

@@ -284,8 +284,12 @@ void MOADNSParser::init(bool query, const char *packet, unsigned int len)
 
       d_answers.push_back(make_pair(dr, pr.d_pos));
 
-      if(dr.d_type == QType::TSIG && dr.d_class == 0xff) 
+      if(dr.d_type == QType::TSIG && dr.d_class == QClass::ANY) {
+        if(dr.d_place != DNSResourceRecord::ADDITIONAL || n != (unsigned int)(d_header.ancount + d_header.nscount + d_header.arcount) - 1) {
+          throw MOADNSException("Packet ("+d_qname.toString()+"|#"+std::to_string(d_qtype)+") has a TSIG record in an invalid position.");
+        }
         d_tsigPos = recordStartPos + sizeof(struct dnsheader);
+      }
     }
 
 #if 0    

pdns/dnsparser.hh

@@ -371,7 +371,7 @@ public:
     return pr;
   }
 
-  uint16_t getTSIGPos()
+  uint16_t getTSIGPos() const
   {
     return d_tsigPos;
   }

pdns/dnssecinfra.cc

@@ -595,15 +595,36 @@ string calculateHMAC(const std::string& key, const std::string& text, TSIGHashEn
       md_type = EVP_sha512();
       break;
     default:
-      throw new PDNSException("Unknown hash algorithm requested from calculateHMAC()");
+      throw PDNSException("Unknown hash algorithm requested from calculateHMAC()");
   }
 
   unsigned char* out = HMAC(md_type, reinterpret_cast<const unsigned char*>(key.c_str()), key.size(), reinterpret_cast<const unsigned char*>(text.c_str()), text.size(), hash, &outlen);
-  if (out != NULL && outlen > 0) {
-    return string((char*) hash, outlen);
+  if (out == NULL || outlen == 0) {
+    throw PDNSException("HMAC computation failed");
   }
 
-  return "";
+  return string((char*) hash, outlen);
+}
+
+bool constantTimeStringEquals(const std::string& a, const std::string& b)
+{
+  if (a.size() != b.size()) {
+    return false;
+  }
+  const size_t size = a.size();
+#if OPENSSL_VERSION_NUMBER >= 0x0090819fL
+  return CRYPTO_memcmp(a.c_str(), b.c_str(), size) == 0;
+#else
+  const volatile unsigned char *_a = (const volatile unsigned char *) a.c_str();
+  const volatile unsigned char *_b = (const volatile unsigned char *) b.c_str();
+  unsigned char res = 0;
+
+  for (size_t idx = 0; idx < size; idx++) {
+    res |= _a[idx] ^ _b[idx];
+  }
+
+  return res == 0;
+#endif
 }
 
 string makeTSIGMessageFromTSIGPacket(const string& opacket, unsigned int tsigOffset, const DNSName& keyname, const TSIGRecordContent& trc, const string& previous, bool timersonly, unsigned int dnsHeaderOffset)

pdns/dnssecinfra.hh

@@ -157,6 +157,7 @@ class DNSPacket;
 void addRRSigs(DNSSECKeeper& dk, UeberBackend& db, const std::set<DNSName>& authMap, vector<DNSZoneRecord>& rrs);
 
 string calculateHMAC(const std::string& key, const std::string& text, TSIGHashEnum hash);
+bool constantTimeStringEquals(const std::string& a, const std::string& b);
 
 string makeTSIGMessageFromTSIGPacket(const string& opacket, unsigned int tsigoffset, const DNSName& keyname, const TSIGRecordContent& trc, const string& previous, bool timersonly, unsigned int dnsHeaderOffset=0);
 void addTSIG(DNSPacketWriter& pw, TSIGRecordContent* trc, const DNSName& tsigkeyname, const string& tsigsecret, const string& tsigprevious, bool timersonly);

pdns/ixfr.cc

@@ -24,7 +24,7 @@
 #include "dns_random.hh"
 #include "dnsrecords.hh"
 #include "dnssecinfra.hh"
-
+#include "tsigverifier.hh"
 
 // Returns pairs of "remove & add" vectors. If you get an empty remove, it means you got an AXFR!
 vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAddress& master, const DNSName& zone, const DNSRecord& oursr, 
@@ -40,10 +40,11 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAd
   oursr.d_content->toPacket(pw);
 
   pw.commit();
+  TSIGRecordContent trc;
+  TSIGTCPVerifier tsigVerifier(tt, master, trc);
   if(!tt.algo.empty()) {
     TSIGHashEnum the;
     getTSIGHashEnum(tt.algo, the);
-    TSIGRecordContent trc;
     try {
       trc.d_algoName = getTSIGAlgoName(the);
     } catch(PDNSException& pe) {
@@ -77,6 +78,7 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAd
   shared_ptr<SOARecordContent> masterSOA;
   vector<DNSRecord> records;
   size_t receivedBytes = 0;
+
   for(;;) {
     if(s.read((char*)&len, 2)!=2)
       break;
@@ -96,6 +98,11 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAd
       throw std::runtime_error("Got an error trying to IXFR zone '"+zone.toString()+"' from master '"+master.toStringWithPort()+"': "+RCode::to_s(mdp.d_header.rcode));
 
     //    cout<<"Got a response, rcode: "<<mdp.d_header.rcode<<", got "<<mdp.d_answers.size()<<" answers"<<endl;
+
+    if(!tt.algo.empty()) { // TSIG verify message
+      tsigVerifier.check(std::string(reply, len), mdp);
+    }
+
     for(auto& r: mdp.d_answers) {
       if(r.first.d_type == QType::TSIG) 
         continue;

pdns/misc.hh

@@ -611,3 +611,4 @@ uid_t strToUID(const string &str);
 gid_t strToGID(const string &str);
 
 unsigned int pdns_stou(const std::string& str, size_t * idx = 0, int base = 10);
+

pdns/recursordist/Makefile.am

@@ -129,6 +129,7 @@ pdns_recursor_SOURCES = \
 	sortlist.cc sortlist.hh \
 	sstuff.hh \
 	syncres.cc syncres.hh \
+	tsigverifier.cc tsigverifier.hh \
 	ueberbackend.hh \
 	unix_utility.cc \
 	utility.hh \

pdns/recursordist/tsigverifier.cc

@@ -0,0 +1 @@
+../tsigverifier.cc

pdns/recursordist/tsigverifier.hh

@@ -0,0 +1 @@
+../tsigverifier.hh

pdns/resolver.cc

@@ -371,7 +371,7 @@ AXFRRetriever::AXFRRetriever(const ComboAddress& remote,
                              const TSIGTriplet& tt, 
                              const ComboAddress* laddr,
                              size_t maxReceivedBytes)
-  : d_tt(tt), d_receivedBytes(0), d_maxReceivedBytes(maxReceivedBytes), d_tsigPos(0), d_nonSignedMessages(0)
+  : d_tsigVerifier(tt, remote, d_trc), d_receivedBytes(0), d_maxReceivedBytes(maxReceivedBytes)
 {
   ComboAddress local;
   if (laddr != NULL) {
@@ -482,76 +482,13 @@ int AXFRRetriever::getChunk(Resolver::res_t &res, vector<DNSRecord>* records) //
     if (answer.first.d_type == QType::SOA)
       d_soacount++;
 
-  if(!d_tt.name.empty()) { // TSIG verify message
-    // If we have multiple messages, we need to concatenate them together. We also need to make sure we know the location of 
-    // the TSIG record so we can remove it in makeTSIGMessageFromTSIGPacket
-    d_signData.append(d_buf.get(), len);
-    if (mdp.getTSIGPos() == 0)
-      d_tsigPos += len;
-    else 
-      d_tsigPos += mdp.getTSIGPos();
-
-    string theirMac;
-    bool checkTSIG = false;
-
-    for(const MOADNSParser::answers_t::value_type& answer :  mdp.d_answers) {
-      if (answer.first.d_type == QType::SOA)  // A SOA is either the first or the last record. We need to check TSIG if that's the case.
-        checkTSIG = true;
-
-      if(answer.first.d_type == QType::TSIG) {
-        shared_ptr<TSIGRecordContent> trc = getRR<TSIGRecordContent>(answer.first);
-        if(trc) {
-          theirMac = trc->d_mac;
-          d_trc.d_time = trc->d_time;
-          checkTSIG = true;
-        }
-      }
-    }
-
-    if( ! checkTSIG && d_nonSignedMessages > 99) { // We're allowed to get 100 digest without a TSIG.
-      throw ResolverException("No TSIG message received in last 100 messages of AXFR transfer.");
-    }
-
-    if (checkTSIG) {
-      if (theirMac.empty())
-        throw ResolverException("No TSIG on AXFR response from "+d_remote.toStringWithPort()+" , should be signed with TSIG key '"+d_tt.name.toString()+"'");
-
-      string message;
-      if (!d_prevMac.empty()) {
-        message = makeTSIGMessageFromTSIGPacket(d_signData, d_tsigPos, d_tt.name, d_trc, d_prevMac, true, d_signData.size()-len);
-      } else {
-        message = makeTSIGMessageFromTSIGPacket(d_signData, d_tsigPos, d_tt.name, d_trc, d_trc.d_mac, false);
-      }
-
-      TSIGHashEnum algo;
-      if (!getTSIGHashEnum(d_trc.d_algoName, algo)) {
-        throw ResolverException("Unsupported TSIG HMAC algorithm " + d_trc.d_algoName.toString());
-      }
-
-      if (algo == TSIG_GSS) {
-        GssContext gssctx(d_tt.name);
-        if (!gss_verify_signature(d_tt.name, message, theirMac)) {
-          throw ResolverException("Signature failed to validate on AXFR response from "+d_remote.toStringWithPort()+" signed with TSIG key '"+d_tt.name.toString()+"'");
-        }
-      } else {
-        string ourMac=calculateHMAC(d_tt.secret, message, algo);
-
-        // ourMac[0]++; // sabotage == for testing :-)
-        if(ourMac != theirMac) {
-          throw ResolverException("Signature failed to validate on AXFR response from "+d_remote.toStringWithPort()+" signed with TSIG key '"+d_tt.name.toString()+"'");
-        }
-      }
-
-      // Reset and store some values for the next chunks. 
-      d_prevMac = theirMac;
-      d_nonSignedMessages = 0;
-      d_signData.clear();
-      d_tsigPos = 0;
-    }
-    else
-      d_nonSignedMessages++;
+  try {
+    d_tsigVerifier.check(std::string(d_buf.get(), len), mdp);
   }
-
+  catch(const std::runtime_error& re) {
+    throw ResolverException(re.what());
+  }
+
   return true;
 }
 

pdns/resolver.hh

@@ -41,6 +41,7 @@
 #include "namespaces.hh"
 #include "dnsrecords.hh"
 #include "dnssecinfra.hh"
+#include "tsigverifier.hh"
 
 class ResolverException : public PDNSException
 {
@@ -101,14 +102,10 @@ class AXFRRetriever : public boost::noncopyable
     int d_sock;
     int d_soacount;
     ComboAddress d_remote;
-
-    TSIGTriplet d_tt;
-    string d_prevMac; // RFC2845 4.4
-    string d_signData;
+    TSIGTCPVerifier d_tsigVerifier;
+
     size_t d_receivedBytes;
     size_t d_maxReceivedBytes;
-    uint32_t d_tsigPos;
-    uint d_nonSignedMessages; // RFC2845 4.4
     TSIGRecordContent d_trc;
 };
 

pdns/saxfr.cc

@@ -35,7 +35,7 @@ bool validateTSIG(const string& message, const TSIGHashEnum& algo, const DNSName
     }
     return true;
   }
-  return calculateHMAC(secret, message, algo) == trc->d_mac;
+  return constantTimeStringEquals(calculateHMAC(secret, message, algo), trc->d_mac);
 }
 
 

pdns/tcpreceiver.cc

@@ -683,9 +683,9 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
   DNSName tsigkeyname;
   string tsigsecret;
 
-  q->getTSIGDetails(&trc, &tsigkeyname, 0);
+  bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname, 0);
 
-  if(!tsigkeyname.empty()) {
+  if(haveTSIGDetails && !tsigkeyname.empty()) {
     string tsig64;
     DNSName algorithm=trc.d_algoName; // FIXME400: check
     if (algorithm == DNSName("hmac-md5.sig-alg.reg.int"))
@@ -714,7 +714,7 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
     addRRSigs(dk, signatureDB, authSet, outpacket->getRRS());
   }
 
-  if(!tsigkeyname.empty())
+  if(haveTSIGDetails && !tsigkeyname.empty())
     outpacket->setTSIGDetails(trc, tsigkeyname, tsigsecret, trc.d_mac); // first answer is 'normal'
 
   sendPacket(outpacket, outsock);
@@ -969,7 +969,7 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
       for(;;) {
         outpacket->getRRS() = csp.getChunk();
         if(!outpacket->getRRS().empty()) {
-          if(!tsigkeyname.empty()) 
+          if(haveTSIGDetails && !tsigkeyname.empty())
             outpacket->setTSIGDetails(trc, tsigkeyname, tsigsecret, trc.d_mac, true);
           sendPacket(outpacket, outsock);
           trc.d_mac=outpacket->d_trc.d_mac;
@@ -1020,7 +1020,7 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
             for(;;) {
               outpacket->getRRS() = csp.getChunk();
               if(!outpacket->getRRS().empty()) {
-                if(!tsigkeyname.empty())
+                if(haveTSIGDetails && !tsigkeyname.empty())
                   outpacket->setTSIGDetails(trc, tsigkeyname, tsigsecret, trc.d_mac, true);
                 sendPacket(outpacket, outsock);
                 trc.d_mac=outpacket->d_trc.d_mac;
@@ -1055,7 +1055,7 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
         for(;;) {
           outpacket->getRRS() = csp.getChunk();
           if(!outpacket->getRRS().empty()) {
-            if(!tsigkeyname.empty())
+            if(haveTSIGDetails && !tsigkeyname.empty())
               outpacket->setTSIGDetails(trc, tsigkeyname, tsigsecret, trc.d_mac, true); 
             sendPacket(outpacket, outsock);
             trc.d_mac=outpacket->d_trc.d_mac;
@@ -1076,7 +1076,7 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
   for(;;) { 
     outpacket->getRRS() = csp.getChunk(true); // flush the pipe
     if(!outpacket->getRRS().empty()) {
-      if(!tsigkeyname.empty())
+      if(haveTSIGDetails && !tsigkeyname.empty())
         outpacket->setTSIGDetails(trc, tsigkeyname, tsigsecret, trc.d_mac, true); // first answer is 'normal'
       sendPacket(outpacket, outsock);
       trc.d_mac=outpacket->d_trc.d_mac;
@@ -1095,7 +1095,7 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
   outpacket=getFreshAXFRPacket(q);
   outpacket->addRecord(dzrsoa);
   editSOA(dk, sd.qname, outpacket.get());
-  if(!tsigkeyname.empty())
+  if(haveTSIGDetails && !tsigkeyname.empty())
     outpacket->setTSIGDetails(trc, tsigkeyname, tsigsecret, trc.d_mac, true); 
 
   sendPacket(outpacket, outsock);
@@ -1197,9 +1197,9 @@ int TCPNameserver::doIXFR(shared_ptr<DNSPacket> q, int outsock)
     DNSName tsigkeyname;
     string tsigsecret;
 
-    q->getTSIGDetails(&trc, &tsigkeyname, 0);
+    bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname, 0);
 
-    if(!tsigkeyname.empty()) {
+    if(haveTSIGDetails && !tsigkeyname.empty()) {
       string tsig64;
       DNSName algorithm=trc.d_algoName; // FIXME400: was toLowerCanonic, compare output
       if (algorithm == DNSName("hmac-md5.sig-alg.reg.int"))
@@ -1226,7 +1226,7 @@ int TCPNameserver::doIXFR(shared_ptr<DNSPacket> q, int outsock)
       addRRSigs(dk, signatureDB, authSet, outpacket->getRRS());
     }
 
-    if(!tsigkeyname.empty())
+    if(haveTSIGDetails && !tsigkeyname.empty())
       outpacket->setTSIGDetails(trc, tsigkeyname, tsigsecret, trc.d_mac); // first answer is 'normal'
 
     sendPacket(outpacket, outsock);