Skip to content

Commit

Permalink
Replace s_redirectionQTypes by a simple function and simplify logging…
Browse files Browse the repository at this point in the history 
… in sanitizeRecords
  • Loading branch information
@omoerbeek
omoerbeek committed Jul 5, 2024
1 parent f6ebc73 commit 3584334
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 16 deletions.
14 changes: 14 additions & 0 deletions pdns/dns.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,17 @@ uint32_t hashQuestion(const uint8_t* packet, uint16_t packet_len, uint32_t init,
return init;
}

static const std::array<std::string, 4> placeNames = {
"QUESTION",
"ANSWER",
"AUTHORITY",
"ADDITIONAL"
};

std::string DNSResourceRecord::placeString(uint8_t place)
{
if (place >= placeNames.size()) {
return "?";
}
return placeNames.at(place);
}
1 change: 1 addition & 0 deletions pdns/dns.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
10 changes: 10 additions & 0 deletions pdns/dnsparser.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const DNSRecordContent>& content)
{
d_content = content;
Expand Down
34 changes: 19 additions & 15 deletions pdns/recursordist/syncres.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<QType> SyncRes::s_redirectionQTypes = {QType::CNAME, QType::DNAME};
static LockGuarded<fails_t<ComboAddress>> s_fails;
static LockGuarded<fails_t<DNSName>> s_nonresolving;

Expand Down Expand Up @@ -4207,6 +4206,11 @@ static void allowAdditionalEntry(std::unordered_set<DNSName>& 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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -4252,23 +4256,23 @@ 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;
}
}

// 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;
}
Expand All @@ -4284,22 +4288,22 @@ 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;
}

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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion pdns/recursordist/syncres.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,6 @@ private:
static EDNSSubnetOpts s_ecsScopeZero;
static LogMode s_lm;
static std::unique_ptr<NetmaskGroup> s_dontQuery;
const static std::unordered_set<QType> s_redirectionQTypes;

struct GetBestNSAnswer
{
Expand Down

0 comments on commit 3584334

Please sign in to comment.