From: Miod Vallat Date: Thu, 7 Aug 2025 06:40:31 +0000 (+0200) Subject: Add optional backend method to let them return invalid records. X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F15966%2Fhead;p=thirdparty%2Fpdns.git Add optional backend method to let them return invalid records. Use this in pdnsutil check-zone to report these ill-formed records which would otherwise never made visible. Fixes: #4941 Signed-off-by: Miod Vallat --- diff --git a/pdns/backends/gsql/gsqlbackend.cc b/pdns/backends/gsql/gsqlbackend.cc index 0d7160487..06de81732 100644 --- a/pdns/backends/gsql/gsqlbackend.cc +++ b/pdns/backends/gsql/gsqlbackend.cc @@ -1141,8 +1141,6 @@ bool GSQLBackend::unpublishDomainKey(const ZoneName& name, unsigned int keyId) return true; } - - bool GSQLBackend::removeDomainKey(const ZoneName& name, unsigned int keyId) { if(!d_dnssecQueries) @@ -1340,7 +1338,6 @@ bool GSQLBackend::getAllDomainMetadata(const ZoneName& name, std::map& meta) { if(!d_dnssecQueries && isDnssecDomainMetadata(kind)) @@ -1589,6 +1586,35 @@ skiprow: return false; } +bool GSQLBackend::get_unsafe(DNSResourceRecord& rec, std::vector>& invalid) +{ + SSqlStatement::row_t row; + + if ((*d_query_stmt)->hasNextRow()) { + try { + (*d_query_stmt)->nextRow(row); + if (!d_list) { + ASSERT_ROW_COLUMNS(d_query_name, row, 8); // lookup(), listSubZone() + } + else { + ASSERT_ROW_COLUMNS(d_query_name, row, 9); // list() + } + } catch (SSqlException &e) { + throw PDNSException("GSQLBackend get: "+e.txtReason()); + } + extractRecord_unsafe(row, rec, invalid); + return true; + } + + try { + (*d_query_stmt)->reset(); + } catch (SSqlException &e) { + throw PDNSException("GSQLBackend get: "+e.txtReason()); + } + d_query_stmt = nullptr; + return false; +} + bool GSQLBackend::autoPrimaryAdd(const AutoPrimary& primary) { try{ @@ -1607,7 +1633,6 @@ bool GSQLBackend::autoPrimaryAdd(const AutoPrimary& primary) throw PDNSException("GSQLBackend unable to insert an autoprimary with IP " + primary.ip + " and nameserver name '" + primary.nameserver + "' and account '" + primary.account + "': " + e.txtReason()); } return true; - } bool GSQLBackend::autoPrimaryRemove(const AutoPrimary& primary) @@ -1627,7 +1652,6 @@ bool GSQLBackend::autoPrimaryRemove(const AutoPrimary& primary) throw PDNSException("GSQLBackend unable to remove an autoprimary with IP " + primary.ip + " and nameserver name '" + primary.nameserver + "': " + e.txtReason()); } return true; - } bool GSQLBackend::autoPrimariesList(std::vector& primaries) @@ -2375,6 +2399,97 @@ void GSQLBackend::extractRecord(SSqlStatement::row_t& row, DNSResourceRecord& r) } } +// Similar logic to extractRecord() above, but returns the invalid field names +// and their contents. +void GSQLBackend::extractRecord_unsafe(SSqlStatement::row_t& row, DNSResourceRecord& rec, std::vector>& invalid) +{ + invalid.clear(); + + try { + static const int defaultTTL = ::arg().asNum( "default-ttl" ); + + if (row[1].empty()) { + rec.ttl = defaultTTL; + } + else { + pdns::checked_stoi_into(rec.ttl, row[1]); + } + } + catch (...) { + invalid.emplace_back(std::make_pair("ttl", row[1])); + rec.ttl = 0; + } + + try { + if (!d_qname.empty()) { + rec.qname = d_qname; + } + else { + rec.qname = DNSName(row[6]); + } + } + catch (...) { + invalid.emplace_back(std::make_pair("qname", row[6])); + rec.qname.clear(); + } + + rec.qtype=row[3]; + + try { + if (d_upgradeContent && DNSRecordContent::isUnknownType(row[3]) && DNSRecordContent::isRegisteredType(rec.qtype, rec.qclass)) { + rec.content = DNSRecordContent::upgradeContent(rec.qname, rec.qtype, row[0]); + } + else if (rec.qtype==QType::MX || rec.qtype==QType::SRV) { + rec.content.reserve(row[2].size() + row[0].size() + 1); + rec.content=row[2]+" "+row[0]; + } + else { + rec.content=std::move(row[0]); + } + } + catch (...) { + invalid.emplace_back(std::make_pair("content", row[0])); + rec.content.clear(); + } + + rec.last_modified=0; + + if (d_dnssecQueries) { + rec.auth = !row[7].empty() && row[7][0]=='1'; + } + else { + rec.auth = true; + } + + rec.disabled = !row[5].empty() && row[5][0]=='1'; + + try { + pdns::checked_stoi_into(rec.domain_id, row[4]); + } + catch (...) { + invalid.emplace_back(std::make_pair("domain_id", row[4])); + rec.domain_id = 0; + } + + if (row.size() > 8) { // if column 8 exists, it holds an ordername + try { + if (!row.at(8).empty()) { + rec.ordername=DNSName(boost::replace_all_copy(row.at(8), " ", ".")).labelReverse(); + } + else { + rec.ordername.clear(); + } + } + catch (...) { + invalid.emplace_back(std::make_pair("ordername", row.at(8))); + rec.ordername.clear(); + } + } + else { + rec.ordername.clear(); + } +} + void GSQLBackend::extractComment(SSqlStatement::row_t& row, Comment& comment) { pdns::checked_stoi_into(comment.domain_id, row[0]); diff --git a/pdns/backends/gsql/gsqlbackend.hh b/pdns/backends/gsql/gsqlbackend.hh index 44c36865d..926441ecd 100644 --- a/pdns/backends/gsql/gsqlbackend.hh +++ b/pdns/backends/gsql/gsqlbackend.hh @@ -261,10 +261,12 @@ public: string directBackendCmd(const string &query) override; bool searchRecords(const string &pattern, size_t maxResults, vector& result) override; bool searchComments(const string &pattern, size_t maxResults, vector& result) override; + bool get_unsafe(DNSResourceRecord& rec, std::vector>& invalid) override; protected: string pattern2SQLPattern(const string& pattern); void extractRecord(SSqlStatement::row_t& row, DNSResourceRecord& rr); + void extractRecord_unsafe(SSqlStatement::row_t& row, DNSResourceRecord& rec, std::vector>& invalid); void extractComment(SSqlStatement::row_t& row, Comment& c); void setLastCheck(domainid_t domain_id, time_t lastcheck); bool isConnectionUsable() { diff --git a/pdns/dnsbackend.hh b/pdns/dnsbackend.hh index b5e0ffdcf..f7a1a8f6c 100644 --- a/pdns/dnsbackend.hh +++ b/pdns/dnsbackend.hh @@ -517,6 +517,18 @@ public: const string& getPrefix() { return d_prefix; }; + // The following routine allows callers to retrieve invalid records from + // the backend, which would not be returned by get(). This is used by + // pdnsutil for diagnostic purposes. + // The default implementation simply wraps get() and pretends there are + // no invalid or corrupted records in the backend storage. + + virtual bool get_unsafe(DNSResourceRecord& rec, std::vector>& invalid) + { + invalid.clear(); + return get(rec); + } + protected: bool mustDo(const string& key); const string& getArg(const string& key); diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index 3f4f9c552..f1dcb4fe9 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -812,6 +812,47 @@ static bool rectifyAllZones(DNSSECKeeper &dk, bool quiet = false) return result; } +// Returns a terminal-friendly version of its input, with non-printable +// characters replaced with hex sequences. +// Filters fewer printable characters than makeLuaString(). +static std::string terminalSafe(const std::string& input) +{ + size_t toRewrite{0}; + for (const auto& chr : input) { + if (::isprint(static_cast(chr)) == 0) { + ++toRewrite; + } + } + if (toRewrite == 0) { + return input; + } + std::string output; + std::array tmp{}; + output.reserve(input.size() + 3 * toRewrite); + for (const auto& chr : input) { + if (::isprint(static_cast(chr)) != 0) { + output.push_back(chr); + } + else { + switch (chr) { + case '\n': + output.append("\\n"); + break; + case '\r': + output.append("\\r"); + break; + case '\t': + output.append("\\t"); + break; + default: + snprintf(tmp.data(), tmp.size(), "\\x%02x", static_cast(chr)); + output.append(tmp.data()); + } + } + } + return output; +} + static int checkZone(DNSSECKeeper &dk, UeberBackend &B, const ZoneName& zone, const vector* suppliedrecords=nullptr) // NOLINT(readability-function-cognitive-complexity,readability-identifier-length) { int numerrors=0; @@ -931,10 +972,36 @@ static int checkZone(DNSSECKeeper &dk, UeberBackend &B, const ZoneName& zone, co vector records; if(suppliedrecords == nullptr) { + std::vector> invalid; DNSResourceRecord drr; sd.db->list(zone, sd.domain_id, g_verbose); - while(sd.db->get(drr)) { - records.push_back(drr); + while (sd.db->get_unsafe(drr, invalid)) { + if (invalid.empty()) { + records.push_back(drr); + continue; + } + // Emit this alert as a warning, as this is not something which pdnsutil + // can fix by itself, and that record will be silently ignored during + // regular operation. + cout << "[Warning] Ill-formed "; + // The invalid part might be the record name itself, only output it if + // non-empty. + if (!drr.qname.empty()) { + cout << "'" << drr.qname << "' "; + } + cout << "record in backend storage: "; + bool first = true; + for (const auto& pair : invalid) { + if (first) { + first = false; + } + else { + cout << ", "; + } + cout << "field " << pair.first << " has invalid content '" << terminalSafe(pair.second) << "'"; + } + cout << std::endl; + numwarnings++; } } else @@ -5656,7 +5723,7 @@ try << endl; for (const auto& group : topLevelDispatcher) { if (!group.second.first) { // toplevel synonyms (sugar), don't list - continue; + continue; } for (const auto& dispatcher : group.second.second) { displayCommandGroup(dispatcher, group.first); diff --git a/regression-tests.nobackend/gsqlite3-corrupted-record/command b/regression-tests.nobackend/gsqlite3-corrupted-record/command new file mode 100755 index 000000000..6f23f5462 --- /dev/null +++ b/regression-tests.nobackend/gsqlite3-corrupted-record/command @@ -0,0 +1,37 @@ +#!/usr/bin/env bash + +## source ../regression-tests/common + +rm -f pdns-gsqlite3.conf pdns.sqlite3 + +cat > pdns-gsqlite3.conf << __EOF__ +launch=gsqlite3 +gsqlite3-database=pdns.sqlite3 +module-dir=../regression-tests/modules +__EOF__ + +ARGS="--config-dir=. --config-name=gsqlite3" + +sqlite3 pdns.sqlite3 < ../modules/gsqlite3backend/schema.sqlite3.sql +tosql gsqlite | sqlite3 pdns.sqlite3 +echo ANALYZE\; | sqlite3 pdns.sqlite3 + +# Create zone +$PDNSUTIL $ARGS zone create bug.less + +# Add some valid records +$PDNSUTIL $ARGS rrset add bug.less bug.less NS no.bug.less +$PDNSUTIL $ARGS rrset add bug.less no.bug.less A 1.2.3.4 +$PDNSUTIL $ARGS rrset add bug.less never.bug.less A 1.2.3.4 + +# Alter some records to make their contents invalid +echo "UPDATE records SET name='this-name-turns-out-to-be-an-invalid-dns-name-because-it-is-way-larger-than-sixty-three-characters-and-this-is-not-allowed-for-labels.bug.less' WHERE name='no.bug.less';" | sqlite3 pdns.sqlite3 2>&1 +echo "UPDATE records SET ordername='..' WHERE name='never.bug.less';" | sqlite3 pdns.sqlite3 2>&1 + +# Check that pdnsutil zone list doesn't show the invalid records +$PDNSUTIL $ARGS zone list bug.less + +# Check that pdnsutil zone check reports the invalid records +$PDNSUTIL $ARGS zone check bug.less + +exit 0 diff --git a/regression-tests.nobackend/gsqlite3-corrupted-record/description b/regression-tests.nobackend/gsqlite3-corrupted-record/description new file mode 100644 index 000000000..13b03cb9f --- /dev/null +++ b/regression-tests.nobackend/gsqlite3-corrupted-record/description @@ -0,0 +1,2 @@ +This test checks that an intentionally crafted incorrect record gets ignored +during regular lookups and reported as incorrect by pdnsutil zone check. diff --git a/regression-tests.nobackend/gsqlite3-corrupted-record/expected_result b/regression-tests.nobackend/gsqlite3-corrupted-record/expected_result new file mode 100644 index 000000000..0cce24360 --- /dev/null +++ b/regression-tests.nobackend/gsqlite3-corrupted-record/expected_result @@ -0,0 +1,12 @@ +New rrset: +bug.less. 3600 IN NS no.bug.less +New rrset: +no.bug.less. 3600 IN A 1.2.3.4 +New rrset: +never.bug.less. 3600 IN A 1.2.3.4 +$ORIGIN . +bug.less 3600 IN NS no.bug.less. +bug.less 3600 IN SOA a.misconfigured.dns.server.invalid hostmaster.bug.less 0 10800 3600 604800 3600 +[Warning] Ill-formed 'never.bug.less' record in backend storage: field ordername has invalid content '..' +[Warning] Ill-formed record in backend storage: field qname has invalid content 'this-name-turns-out-to-be-an-invalid-dns-name-because-it-is-way-larger-than-sixty-three-characters-and-this-is-not-allowed-for-labels.bug.less' +Checked 2 records of 'bug.less', 0 errors, 2 warnings.