From 37d66d8109dbab0ac7aebe966c02e9468dfd2205 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 9 Dec 2016 10:11:38 +0100 Subject: [PATCH] auth: Fix TSIG computation --- pdns/dnspacket.cc | 9 ++++++--- pdns/dnsparser.cc | 6 +++++- pdns/dnsparser.hh | 2 +- pdns/dnssecinfra.cc | 19 ++++++++++++++++++- pdns/dnssecinfra.hh | 1 + pdns/resolver.cc | 8 +++++++- pdns/tcpreceiver.cc | 22 +++++++++++----------- 7 files changed, 49 insertions(+), 18 deletions(-) diff --git a/pdns/dnspacket.cc b/pdns/dnspacket.cc index a070dc1c11..f01ad343a0 100644 --- 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 content = boost::dynamic_pointer_cast(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<qdomain<<"' denied: TSIG (key '"<<*keyname<<"') time delta "<< abs(trc->d_time - now)<<" > 'fudge' "<d_fudge<d_mac; + bool result=constantTimeStringEquals(calculateHMAC(*secret, message, algo), trc->d_mac); if(!result) { L<qdomain<<"' denied: TSIG signature mismatch using '"<<*keyname<<"' and algorithm '"<d_algoName<<"'"<(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(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 60aab9ff0c..13da37bf1d 100644 --- 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 43842d1b87..668b9b0e89 100644 --- 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 7bffd979ae..e3ce5b136e 100644 --- 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 f4e30f2a0e..3f4cea62d7 100644 --- 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 trc = boost::dynamic_pointer_cast(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(delta) + " > fudge " + lexical_cast(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 ff6af977bf..7616e5d536 100644 --- a/pdns/tcpreceiver.cc +++ b/pdns/tcpreceiver.cc @@ -590,9 +590,9 @@ int TCPNameserver::doAXFR(const string &target, shared_ptr 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 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 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 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 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 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 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 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 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); -- 2.47.2