From: Charles-Henri Bruyand Date: Fri, 10 Dec 2021 16:23:51 +0000 (+0100) Subject: Cleanup and issues raised by first rgacogne's review (thanks!) X-Git-Tag: auth-4.7.0-alpha1~108^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=62fdccd0c3119bdf136c2ce2dc6282a3629121aa;p=thirdparty%2Fpdns.git Cleanup and issues raised by first rgacogne's review (thanks!) --- diff --git a/pdns/dnsdist-lua-actions.cc b/pdns/dnsdist-lua-actions.cc index 0598d222a4..ca25b37bd6 100644 --- a/pdns/dnsdist-lua-actions.cc +++ b/pdns/dnsdist-lua-actions.cc @@ -1725,7 +1725,7 @@ class ClearRecordTypesResponseAction : public DNSResponseAction, public boost::n public: ClearRecordTypesResponseAction() {} - ClearRecordTypesResponseAction(std::set qtypes) : d_qtypes(qtypes) + ClearRecordTypesResponseAction(const std::set& qtypes) : d_qtypes(qtypes) { } @@ -2262,11 +2262,11 @@ void setupLuaActions(LuaContext& luaCtx) luaCtx.writeFunction("ClearRecordTypesResponseAction", [](boost::variant>> types) { std::set qtypes{}; - if(auto t = boost::get(types)) { + if (auto t = boost::get(types)) { qtypes.insert(t); } else { const auto& v = boost::get>>(types); - for(const auto& tpair: v) { + for (const auto& tpair: v) { qtypes.insert(tpair.second); } } diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc index b592205996..145e4943eb 100644 --- a/pdns/dnsparser.cc +++ b/pdns/dnsparser.cc @@ -745,9 +745,9 @@ void editDNSPacketTTL(char* packet, size_t length, const std::function& qtypes) +static int rewritePacketWithoutRecordTypes(const PacketBuffer& initialPacket, PacketBuffer& newContent, const std::set& qtypes) { - static const std::set& safeTypes{QType::A, QType::AAAA, QType::DHCID, QType::TXT, QType::LUA, QType::ENT, QType::OPT, QType::HINFO, QType::DNSKEY, QType::CDNSKEY, QType::DS, QType::CDS, QType::DLV, QType::SSHFP, QType::KEY, QType::CERT, QType::TLSA, QType::SMIMEA, QType::OPENPGPKEY, QType::SVCB, QType::HTTPS, QType::NSEC3, QType::CSYNC, QType::NSEC3PARAM, QType::LOC, QType::NID, QType::L32, QType::L64, QType::EUI48, QType::EUI64, QType::URI, QType::CAA}; + static const std::set& safeTypes{QType::A, QType::AAAA, QType::DHCID, QType::TXT, QType::OPT, QType::HINFO, QType::DNSKEY, QType::CDNSKEY, QType::DS, QType::CDS, QType::DLV, QType::SSHFP, QType::KEY, QType::CERT, QType::TLSA, QType::SMIMEA, QType::OPENPGPKEY, QType::SVCB, QType::HTTPS, QType::NSEC3, QType::CSYNC, QType::NSEC3PARAM, QType::LOC, QType::NID, QType::L32, QType::L64, QType::EUI48, QType::EUI64, QType::URI, QType::CAA}; if (initialPacket.size() < sizeof(dnsheader)) { return EINVAL; diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh index 631636bae2..7d57daddbf 100644 --- a/pdns/dnsparser.hh +++ b/pdns/dnsparser.hh @@ -545,14 +545,6 @@ public: return d_offset; } - void shrinkBytes(uint16_t by) - { - if (d_notyouroffset + by > d_length) { - throw std::out_of_range("shrinking dns packet out of range: " + std::to_string(by) + " bytes at " + std::to_string(d_notyouroffset) + " for a total of " + std::to_string(d_length) ); - } - memmove(d_packet + d_notyouroffset, d_packet + d_notyouroffset + by, d_length - (d_notyouroffset + by)); - d_length -= by; - } private: void moveOffset(uint16_t by) { diff --git a/pdns/test-dnsparser_cc.cc b/pdns/test-dnsparser_cc.cc index 2ec3802a5e..b040237881 100644 --- a/pdns/test-dnsparser_cc.cc +++ b/pdns/test-dnsparser_cc.cc @@ -549,4 +549,67 @@ BOOST_AUTO_TEST_CASE(test_clearDNSPacketRecordTypes) { } +BOOST_AUTO_TEST_CASE(test_clearDNSPacketUnsafeRecordTypes) { + { + auto generatePacket = []() { + const DNSName name("powerdns.com."); + const DNSName mxname("mx.powerdns.com."); + const ComboAddress v4("1.2.3.4"); + const ComboAddress v6("2001:db8::1"); + + vector packet; + DNSPacketWriter pwR(packet, name, QType::A, QClass::IN, 0); + pwR.getHeader()->qr = 1; + pwR.commit(); + + pwR.startRecord(name, QType::A, 255, QClass::IN, DNSResourceRecord::ANSWER); + pwR.xfrIP(v4.sin4.sin_addr.s_addr); + pwR.commit(); + + /* different type */ + pwR.startRecord(name, QType::AAAA, 42, QClass::IN, DNSResourceRecord::ANSWER); + pwR.xfrIP6(std::string(reinterpret_cast(v6.sin6.sin6_addr.s6_addr), 16)); + pwR.commit(); + + pwR.startRecord(name, QType::A, 256, QClass::IN, DNSResourceRecord::ADDITIONAL); + pwR.xfrIP(v4.sin4.sin_addr.s_addr); + pwR.commit(); + + pwR.startRecord(name, QType::MX, 256, QClass::IN, DNSResourceRecord::ADDITIONAL); + pwR.xfrName(mxname, false); + pwR.commit(); + + pwR.addOpt(4096, 0, 0); + pwR.commit(); + return packet; + }; + + auto packet = generatePacket(); + + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 1, QType::A), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 1, QType::AAAA), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 3, QType::A), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 3, QType::MX), 1); + + std::set toremove{QType::AAAA}; + clearDNSPacketRecordTypes(packet, toremove); + + // nothing should have been removed as an "unsafe" MX RR is in the packet + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 1, QType::A), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 1, QType::AAAA), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 3, QType::A), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 3, QType::MX), 1); + + toremove = {QType::MX, QType::AAAA}; + clearDNSPacketRecordTypes(packet, toremove); + + // MX is unsafe, but we asked to remove it + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 1, QType::A), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 1, QType::AAAA), 0); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 3, QType::A), 1); + BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast(packet.data()), packet.size(), 3, QType::MX), 0); + } + +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/regression-tests.dnsdist/test_Responses.py b/regression-tests.dnsdist/test_Responses.py index 301048b164..1fd2f4f054 100644 --- a/regression-tests.dnsdist/test_Responses.py +++ b/regression-tests.dnsdist/test_Responses.py @@ -399,8 +399,6 @@ class TestResponseLuaActionReturnSyntax(DNSDistTest): self.assertEqual(query, receivedQuery) self.assertEqual(receivedResponse, None) -from pprint import pprint - class TestResponseClearRecordsType(DNSDistTest): _config_params = ['_testServerPort']