From 3584334b806b8a41e22d210014d7125ae8477345 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 5 Jul 2024 09:36:46 +0200 Subject: [PATCH] Replace s_redirectionQTypes by a simple function and simplify logging in sanitizeRecords --- pdns/dns.cc | 14 ++++++++++++++ pdns/dns.hh | 1 + pdns/dnsparser.hh | 10 ++++++++++ pdns/recursordist/syncres.cc | 34 +++++++++++++++++++--------------- pdns/recursordist/syncres.hh | 1 - 5 files changed, 44 insertions(+), 16 deletions(-) diff --git a/pdns/dns.cc b/pdns/dns.cc index 5e7e45e1f604..12f97eaee9e4 100644 --- a/pdns/dns.cc +++ b/pdns/dns.cc @@ -127,3 +127,17 @@ uint32_t hashQuestion(const uint8_t* packet, uint16_t packet_len, uint32_t init, return init; } +static const std::array placeNames = { + "QUESTION", + "ANSWER", + "AUTHORITY", + "ADDITIONAL" +}; + +std::string DNSResourceRecord::placeString(uint8_t place) +{ + if (place >= placeNames.size()) { + return "?"; + } + return placeNames.at(place); +} diff --git a/pdns/dns.hh b/pdns/dns.hh index c95a62f1c52f..b8dd761673c2 100644 --- a/pdns/dns.hh +++ b/pdns/dns.hh @@ -66,6 +66,7 @@ public: ADDITIONAL = 3 }; //!< Type describing the positioning within, say, a DNSPacket + [[nodiscard]] static std::string placeString(uint8_t place); void setContent(const string& content); [[nodiscard]] string getZoneRepresentation(bool noDot = false) const; diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh index 20ce6396beb8..fa54d2a1c956 100644 --- a/pdns/dnsparser.hh +++ b/pdns/dnsparser.hh @@ -338,6 +338,16 @@ public: return s.str(); } + [[nodiscard]] std::string toString() const + { + std::string ret(d_name.toLogString()); + ret += '|'; + ret += QType(d_type).toString(); + ret += '|'; + ret += getContent()->getZoneRepresentation(); + return ret; + } + void setContent(const std::shared_ptr& content) { d_content = content; diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 6aa0d11496e8..1bc9047bcdeb 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -409,7 +409,6 @@ SuffixMatchNode SyncRes::s_ednsdomains; EDNSSubnetOpts SyncRes::s_ecsScopeZero; string SyncRes::s_serverID; SyncRes::LogMode SyncRes::s_lm; -const std::unordered_set SyncRes::s_redirectionQTypes = {QType::CNAME, QType::DNAME}; static LockGuarded> s_fails; static LockGuarded> s_nonresolving; @@ -4207,6 +4206,11 @@ static void allowAdditionalEntry(std::unordered_set& allowedAdditionals } } +static bool isRedirection(QType qtype) +{ + return qtype == QType::CNAME || qtype == QType::DNAME; +} + 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; @@ -4241,7 +4245,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN // Disallow any name not part of qname requested if (!rec->d_name.isPartOf(auth)) { - LOG(prefix << qname << ": Removing record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section received from " << auth << endl); + LOG(prefix << qname << ": Removing record '" << rec->toString() << "' in the " << DNSResourceRecord::placeString(rec->d_place) << " section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4252,7 +4256,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN /* for now we allow a CNAME for the exact qname in ANSWER with AA=0, because Amazon DNS servers are sending such responses */ if (rec->d_type != QType::CNAME || qname != rec->d_name) { - LOG(prefix << qname << ": Removing record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the answer section without the AA bit set received from " << auth << endl); + LOG(prefix << qname << ": Removing record '" << rec->toString() << "' in the ANSWER section without the AA bit set received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4260,15 +4264,15 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN // Disallow QType DNAME in non-answer section or containing an answer that is not a parent of or equal to the question name if (rec->d_type == QType::DNAME && (rec->d_place != DNSResourceRecord::ANSWER || !qname.isPartOf(rec->d_name))) { - LOG(prefix << qname << ": Removing invalid DNAME record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section received from " << auth << endl); + LOG(prefix << qname << ": Removing invalid DNAME record '" << rec->toString() << "' in the " << DNSResourceRecord::placeString(rec->d_place) << " section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } // Disallow answer records not anwering the QType requested. ANY, CNAME, DNAME, RRSIG complicate matters here // Question: is the SOA check OK? See RFC2181 section 7.1 - if (rec->d_place == DNSResourceRecord::ANSWER && (qtype != QType::ANY && rec->d_type != qtype.getCode() && s_redirectionQTypes.count(rec->d_type) == 0 && rec->d_type != QType::SOA && rec->d_type != QType::RRSIG)) { - LOG(prefix << qname << ": Removing irrelevant record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the ANSWER section received from " << auth << endl); + if (rec->d_place == DNSResourceRecord::ANSWER && (qtype != QType::ANY && rec->d_type != qtype.getCode() && !isRedirection(rec->d_type) && rec->d_type != QType::SOA && rec->d_type != QType::RRSIG)) { + LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the ANSWER section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4284,7 +4288,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN /* dealing with the records in authority */ // Only allow NS, DS, SOA, RRSIG, NSEC, NSEC3 in AUTHORITY section 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 << qname << ": Removing irrelevant record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the AUTHORITY section received from " << auth << endl); + LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the AUTHORITY section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4292,14 +4296,14 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN if (rec->d_place == DNSResourceRecord::AUTHORITY && rec->d_type == QType::SOA) { // Disallow a SOA record with a name that is not a parent of or equal to the name we asked if (!qname.isPartOf(rec->d_name)) { - LOG(prefix << qname << ": Removing irrelevant SOA record '" << rec->d_name << "|" << rec->getContent()->getZoneRepresentation() << "' in the AUTHORITY section received from " << auth << endl); + LOG(prefix << qname << ": Removing irrelevant SOA record '" << rec->toString() << "' in the AUTHORITY section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } // Disallow SOA without AA bit (except for forward with RD=1) if (!(lwr.d_aabit || wasForwardRecurse)) { - LOG(prefix << qname << ": Removing irrelevant record (AA not set) '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the AUTHORITY section received from " << auth << endl); + LOG(prefix << qname << ": Removing irrelevant record (AA not set) '" << rec->toString() << "' in the AUTHORITY section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4322,7 +4326,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN * because they are somewhat easy to insert into a large, fragmented UDP response * for an off-path attacker by injecting spoofed UDP fragments. So do not add these to allowedAdditionals. */ - LOG(prefix << qname << ": Removing NS record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section of a " << (isNXDomain ? "NXD" : "NXQTYPE") << " response received from " << auth << endl); + LOG(prefix << qname << ": Removing NS record '" << rec->toString() << "' in the AUTHORITY section of a " << (isNXDomain ? "NXD" : "NXQTYPE") << " response received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4332,7 +4336,7 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN * We don't want to pick up root NS records in AUTHORITY and their associated ADDITIONAL sections of random queries. * So don't add them to allowedAdditionals. */ - LOG(prefix << qname << ": Removing NS record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the " << (int)rec->d_place << " section of a response received from " << auth << endl); + LOG(prefix << qname << ": Removing NS record '" << rec->toString() << "' in the AUTHORITY section of a response received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4343,13 +4347,13 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN /* 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 << qname << ": Removing irrelevant record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the ADDITIONAL section received from " << auth << endl); + LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the ADDITIONAL section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } if (rec->d_place == DNSResourceRecord::ADDITIONAL && allowedAdditionals.count(rec->d_name) == 0) { - LOG(prefix << qname << ": Removing irrelevant additional record '" << rec->d_name << "|" << DNSRecordContent::NumberToType(rec->d_type) << "|" << rec->getContent()->getZoneRepresentation() << "' in the ADDITIONAL section received from " << auth << endl); + LOG(prefix << qname << ": Removing irrelevant record '" << rec->toString() << "' in the ADDITIONAL section received from " << auth << endl); rec = lwr.d_records.erase(rec); continue; } @@ -4999,8 +5003,8 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co negIndicHasSignatures = !negEntry.authoritySOA.signatures.empty() || !negEntry.DNSSECRecords.signatures.empty(); negindic = true; } - else if (rec.d_place == DNSResourceRecord::ANSWER && s_redirectionQTypes.count(rec.d_type) > 0 && // CNAME or DNAME answer - s_redirectionQTypes.count(qtype.getCode()) == 0) { // But not in response to a CNAME or DNAME query + else if (rec.d_place == DNSResourceRecord::ANSWER && isRedirection(rec.d_type) && // CNAME or DNAME answer + !isRedirection(qtype.getCode())) { // But not in response to a CNAME or DNAME query if (rec.d_type == QType::CNAME && rec.d_name == qname) { if (!dnameOwner.empty()) { // We synthesize ourselves continue; diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh index bd1d02768ea5..a1e0e0e3e905 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -603,7 +603,6 @@ private: static EDNSSubnetOpts s_ecsScopeZero; static LogMode s_lm; static std::unique_ptr s_dontQuery; - const static std::unordered_set s_redirectionQTypes; struct GetBestNSAnswer {