auth: Fix TSIG computation

diff --git a/pdns/dnspacket.cc b/pdns/dnspacket.cc
index a070dc1c112c8117da1431882bbe1e93792c9767..f01ad343a0443bc8fe4cad4d9529acdeb70fcec5 100644 (file)
--- a/pdns/dnspacket.cc
+++ b/pdns/dnspacket.cc
@@ -463,7 +463,7 @@ bool DNSPacket::getTSIGDetails(TSIGRecordContent* trc, string* 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 = boost::dynamic_pointer_cast<TSIGRecordContent>(i->first.d_content);
       if (!content) {
@@ -640,7 +640,10 @@ bool checkForCorrectTSIG(const DNSPacket* q, DNSBackend* B, string* keyname, str
 {
   string message;
 
-  q->getTSIGDetails(trc, keyname, &message);
+  if (!q->getTSIGDetails(trc, keyname, &message)) {
+    return false;
+  }
+
   uint64_t now = time(0);
   if(abs(trc->d_time - now) > trc->d_fudge) {
     L<<Logger::Error<<"Packet for '"<<q->qdomain<<"' denied: TSIG (key '"<<*keyname<<"') time delta "<< abs(trc->d_time - now)<<" > 'fudge' "<<trc->d_fudge<<endl;
@@ -666,7 +669,7 @@ bool checkForCorrectTSIG(const DNSPacket* q, DNSBackend* B, string* keyname, str
   }
 
   B64Decode(secret64, *secret);
-  bool result=calculateHMAC(*secret, message, algo) == trc->d_mac;
+  bool result=constantTimeStringEquals(calculateHMAC(*secret, message, algo), trc->d_mac);
   if(!result) {
     L<<Logger::Error<<"Packet for domain '"<<q->qdomain<<"' denied: TSIG signature mismatch using '"<<*keyname<<"' and algorithm '"<<trc->d_algoName<<"'"<<endl;
   }

diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc
index 7276101509423fb8553f945e55548e71c8e40349..039d93f29ae1a0bfee81f35d180f6319770ccc3f 100644 (file)
--- a/pdns/dnsparser.cc
+++ b/pdns/dnsparser.cc
@@ -278,8 +278,12 @@ void MOADNSParser::init(const char *packet, unsigned int len)
       dr.d_content=boost::shared_ptr<DNSRecordContent>(DNSRecordContent::mastermake(dr, pr, d_header.opcode));
       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 != DNSRecord::Additional || n != (unsigned int)(d_header.ancount + d_header.nscount + d_header.arcount) - 1) {
+          throw MOADNSException("Packet ("+d_qname+"|#"+lexical_cast<string>(d_qtype)+") has a TSIG record in an invalid position.");
+        }
         d_tsigPos = recordStartPos + sizeof(struct dnsheader);
+      }
     }
 
 #if 0    

diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh
index 60aab9ff0c6cba4fb5ad37b6cafdb5c5a1f694a2..13da37bf1d44785cb48d04a2aec61fa73772a642 100644 (file)
--- a/pdns/dnsparser.hh
+++ b/pdns/dnsparser.hh
@@ -335,7 +335,7 @@ public:
     return pr;
   }
 
-  uint16_t getTSIGPos()
+  uint16_t getTSIGPos() const
   {
     return d_tsigPos;
   }

diff --git a/pdns/dnssecinfra.cc b/pdns/dnssecinfra.cc
index 43842d1b871f250d44f96a5392bab9036a5f2584..668b9b0e89513ab1a71893de523603de7417a64c 100644 (file)
--- a/pdns/dnssecinfra.cc
+++ b/pdns/dnssecinfra.cc
@@ -516,7 +516,7 @@ string calculateSHAHMAC(const std::string& key, const std::string& text, TSIGHas
       break;
   };
   default:
-    throw new PDNSException("Unknown hash algorithm requested for SHA");
+    throw PDNSException("Unknown hash algorithm requested for SHA");
   };
 
   return res;
@@ -530,6 +530,23 @@ string calculateHMAC(const std::string& key, const std::string& text, TSIGHashEn
   return calculateSHAHMAC(key, text, hash);
 }
 
+bool constantTimeStringEquals(const std::string& a, const std::string& b)
+{
+  if (a.size() != b.size()) {
+    return false;
+  }
+  const size_t size = a.size();
+  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;
+}
+
 string makeTSIGMessageFromTSIGPacket(const string& opacket, unsigned int tsigOffset, const string& keyname, const TSIGRecordContent& trc, const string& previous, bool timersonly, unsigned int dnsHeaderOffset)
 {
   string message;

diff --git a/pdns/dnssecinfra.hh b/pdns/dnssecinfra.hh
index 7bffd979ae5a77d1dab169cfbb04bae20af08d1a..e3ce5b136e42718a062904b3de7cd455e88a8b2b 100644 (file)
--- a/pdns/dnssecinfra.hh
+++ b/pdns/dnssecinfra.hh
@@ -134,6 +134,7 @@ typedef enum { TSIG_MD5, TSIG_SHA1, TSIG_SHA224, TSIG_SHA256, TSIG_SHA384, TSIG_
 string calculateMD5HMAC(const std::string& key, const std::string& text);
 string calculateSHAHMAC(const std::string& key, const std::string& text, TSIGHashEnum hash);
 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 string& keyname, const TSIGRecordContent& trc, const string& previous, bool timersonly, unsigned int dnsHeaderOffset=0);
 bool getTSIGHashEnum(const string &algoName, TSIGHashEnum& algoEnum);

diff --git a/pdns/resolver.cc b/pdns/resolver.cc
index f4e30f2a0e49708beb8645afe8606e0dc382848c..3f4cea62d72e2dab422431711cf6b07d3394b25a 100644 (file)
--- a/pdns/resolver.cc
+++ b/pdns/resolver.cc
@@ -500,6 +500,7 @@ int AXFRRetriever::getChunk(Resolver::res_t &res) // Implementation is making su
         shared_ptr<TSIGRecordContent> trc = boost::dynamic_pointer_cast<TSIGRecordContent>(answer.first.d_content);
         theirMac = trc->d_mac;
         d_trc.d_time = trc->d_time;
+        d_trc.d_fudge = trc->d_fudge;
         checkTSIG = true;
       }
     }
@@ -512,6 +513,11 @@ int AXFRRetriever::getChunk(Resolver::res_t &res) // Implementation is making su
       if (theirMac.empty())
         throw ResolverException("No TSIG on AXFR response from "+d_remote.toStringWithPort()+" , should be signed with TSIG key '"+d_tsigkeyname+"'");
 
+      uint64_t delta = std::abs((int64_t)d_trc.d_time - (int64_t)time(0));
+      if(delta > d_trc.d_fudge) {
+        throw ResolverException("Invalid TSIG time delta " + lexical_cast<string>(delta) + " >  fudge " + lexical_cast<string>(d_trc.d_fudge));
+      }
+
       string message;
       if (!d_prevMac.empty()) {
         message = makeTSIGMessageFromTSIGPacket(d_signData, d_tsigPos, d_tsigkeyname, d_trc, d_prevMac, true, d_signData.size()-len);
@@ -527,7 +533,7 @@ int AXFRRetriever::getChunk(Resolver::res_t &res) // Implementation is making su
       string ourMac=calculateHMAC(d_tsigsecret, message, algo);
 
       // ourMac[0]++; // sabotage == for testing :-)
-      if(ourMac != theirMac) {
+      if(!constantTimeStringEquals(ourMac, theirMac)) {
         throw ResolverException("Signature failed to validate on AXFR response from "+d_remote.toStringWithPort()+" signed with TSIG key '"+d_tsigkeyname+"'");
       }
 

diff --git a/pdns/tcpreceiver.cc b/pdns/tcpreceiver.cc
index ff6af977bf2656c6a77120d84ce2d25d818f3081..7616e5d536c6002cef88c53083182457eca69d91 100644 (file)
--- a/pdns/tcpreceiver.cc
+++ b/pdns/tcpreceiver.cc
@@ -590,9 +590,9 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr<DNSPacket> q, int out
   TSIGRecordContent trc;
   string tsigkeyname, tsigsecret;
 
-  q->getTSIGDetails(&trc, &tsigkeyname, 0);
+  bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname, 0);
 
-  if(!tsigkeyname.empty()) {
+  if(haveTSIGDetails && !tsigkeyname.empty()) {
     string tsig64;
     string algorithm=toLowerCanonic(trc.d_algoName);
     if (algorithm == "hmac-md5.sig-alg.reg.int")
@@ -616,7 +616,7 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr<DNSPacket> q, int out
     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);
@@ -799,7 +799,7 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr<DNSPacket> q, int out
       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;
@@ -850,7 +850,7 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr<DNSPacket> q, int out
             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;
@@ -885,7 +885,7 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr<DNSPacket> q, int out
         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;
@@ -906,7 +906,7 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr<DNSPacket> q, int out
   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;
@@ -925,7 +925,7 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr<DNSPacket> q, int out
   outpacket=getFreshAXFRPacket(q);
   outpacket->addRecord(soa);
   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);
@@ -1027,9 +1027,9 @@ int TCPNameserver::doIXFR(shared_ptr<DNSPacket> q, int outsock)
     TSIGRecordContent trc;
     string tsigkeyname, tsigsecret;
 
-    q->getTSIGDetails(&trc, &tsigkeyname, 0);
+    bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname, 0);
 
-    if(!tsigkeyname.empty()) {
+    if(haveTSIGDetails && !tsigkeyname.empty()) {
       string tsig64;
       string algorithm=toLowerCanonic(trc.d_algoName);
       if (algorithm == "hmac-md5.sig-alg.reg.int")
@@ -1052,7 +1052,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);