From 7155c3e56212aa054c082e5417122dc009395ce5 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Mon, 24 Dec 2018 19:16:33 +0100 Subject: [PATCH] rec: Sanitize records received before doing anything else --- pdns/syncres.cc | 148 ++++++++++++++++++++++++++++++++++++++++++++++++ pdns/syncres.hh | 4 +- 2 files changed, 151 insertions(+), 1 deletion(-) diff --git a/pdns/syncres.cc b/pdns/syncres.cc index ae7b72252..9f486ac80 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -1969,6 +1969,152 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname return Bogus; } +static bool allowAdditionalEntry(std::unordered_set& allowedAdditionals, const DNSRecord& rec) +{ + switch(rec.d_type) { + case QType::MX: + { + if (auto mxContent = getRR(rec)) { + allowedAdditionals.insert(mxContent->d_mxname); + } + return true; + } + case QType::NS: + { + if (auto nsContent = getRR(rec)) { + allowedAdditionals.insert(nsContent->getNS()); + } + return true; + } + case QType::SRV: + { + if (auto srvContent = getRR(rec)) { + allowedAdditionals.insert(srvContent->d_target); + } + return true; + } + default: + return false; + } +} + +void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, bool rdQuery) +{ + const bool wasForwardRecurse = wasForwarded && rdQuery; + /* list of names for which we will allow A and AAAA records in the additional section + to remain */ + std::unordered_set allowedAdditionals = { qname }; + bool haveAnswers = false; + bool isNXDomain = false; + bool isNXQType = false; + + for(auto rec = lwr.d_records.begin(); rec != lwr.d_records.end(); ) { + + if (rec->d_type == QType::OPT) { + ++rec; + continue; + } + + if (rec->d_class != QClass::IN) { + LOG(prefix<<"Removing non internet-classed data received from "<d_type == QType::ANY) { + LOG(prefix<<"Removing 'ANY'-typed data received from "<d_name.isPartOf(auth)) { + LOG(prefix<<"Removing record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section received from "<d_place == DNSResourceRecord::ANSWER) { + LOG(prefix<<"Removing record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the answer section without the AA bit set received from "<d_type == QType::DNAME && (rec->d_place != DNSResourceRecord::ANSWER || !qname.isPartOf(rec->d_name))) { + LOG(prefix<<"Removing invalid DNAME record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section received from "<d_place == DNSResourceRecord::ANSWER && (qtype != QType::ANY && rec->d_type != qtype.getCode() && rec->d_type != QType::CNAME && rec->d_type != QType::SOA && rec->d_type != QType::RRSIG)) { + LOG(prefix<<"Removing irrelevant record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section received from "<d_place == DNSResourceRecord::ANSWER && !haveAnswers) { + haveAnswers = true; + } + + if (rec->d_place == DNSResourceRecord::ANSWER) { + allowAdditionalEntry(allowedAdditionals, *rec); + } + + /* dealing with the records in authority */ + if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type != QType::NS && rec->d_type != QType::DS && rec->d_type != QType::SOA && rec->d_type != QType::RRSIG && rec->d_type != QType::NSEC && rec->d_type != QType::NSEC3) { + LOG(prefix<<"Removing irrelevant record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section received from "<d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::SOA) { + if (!qname.isPartOf(rec->d_name)) { + LOG(prefix<<"Removing irrelevant record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section received from "<d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::NS && (isNXDomain || isNXQType)) { + /* we don't want to pick up NS records in AUTHORITY or ADDITIONAL sections of NXDomain answers + because they are somewhat easy to insert into a large, fragmented UDP response + for an off-path attacker by injecting spoofed UDP fragments. + */ + LOG(prefix<<"Removing NS record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section of a "<<(isNXDomain ? "NXD" : "NXQTYPE")<<" response received from "<d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::NS) { + allowAdditionalEntry(allowedAdditionals, *rec); + } + + /* dealing with the records in additional */ + if (rec->d_place == DNSResourceRecord::ADDITIONAL && rec->d_type != QType::A && rec->d_type != QType::AAAA && rec->d_type != QType::RRSIG) { + LOG(prefix<<"Removing irrelevant record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section received from "<d_place == DNSResourceRecord::ADDITIONAL && allowedAdditionals.count(rec->d_name) == 0) { + LOG(prefix<<"Removing irrelevant additional record '"<d_name<<"|"<d_type)<<"|"<d_content->getZoneRepresentation()<<"' in the "<<(int)rec->d_place<<" section received from "< ednsmask, vState& state, bool& needWildcardProof, unsigned int& wildcardLabelsCount, bool rdQuery) { bool wasForwardRecurse = wasForwarded && rdQuery; @@ -1980,6 +2126,8 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr prefix.append(depth, ' '); } + sanitizeRecords(prefix, lwr, qname, qtype, auth, wasForwarded, rdQuery); + std::vector> authorityRecs; const unsigned int labelCount = qname.countLabels(); bool isCNAMEAnswer = false; diff --git a/pdns/syncres.hh b/pdns/syncres.hh index 75eaee04d..53825ac81 100644 --- a/pdns/syncres.hh +++ b/pdns/syncres.hh @@ -766,7 +766,9 @@ private: bool throttledOrBlocked(const std::string& prefix, const ComboAddress& remoteIP, const DNSName& qname, const QType& qtype, bool pierceDontQuery); vector retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector::const_iterator& tns, const unsigned int depth, set& beenthere, const vector& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly); - RCode::rcodes_ updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional, vState& state, bool& needWildcardProof, unsigned int& wildcardLabelsCount, bool rdQuery); + + void sanitizeRecords(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, bool rdQuery); + RCode::rcodes_ updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional, vState& state, bool& needWildcardProof, unsigned int& wildcardLabelsCount, bool sendRDQuery); bool processRecords(const std::string& prefix, const DNSName& qname, const QType& qtype, const DNSName& auth, LWResult& lwr, const bool sendRDQuery, vector& ret, set& nsset, DNSName& newtarget, DNSName& newauth, bool& realreferral, bool& negindic, vState& state, const bool needWildcardProof, const unsigned int wildcardLabelsCount); bool doSpecialNamesResolve(const DNSName &qname, const QType &qtype, const uint16_t qclass, vector &ret); -- 2.40.0