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 0598d222a42f660a9ca6a8eae34405a1e40e8990..ca25b37bd6dd7db9f5d9d0fc671534f72e5b8664 100644 (file)
--- 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<QType> qtypes) : d_qtypes(qtypes)
+  ClearRecordTypesResponseAction(const std::set<QType>& qtypes) : d_qtypes(qtypes)
   {
   }
 
@@ -2262,11 +2262,11 @@ void setupLuaActions(LuaContext& luaCtx)
 
   luaCtx.writeFunction("ClearRecordTypesResponseAction", [](boost::variant<int,vector<pair<int, int>>> types) {
       std::set<QType> qtypes{};
-      if(auto t = boost::get<int>(types)) {
+      if (auto t = boost::get<int>(types)) {
         qtypes.insert(t);
       } else {
         const auto& v = boost::get<vector<pair<int,int>>>(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 b5922059960a8676a1f5db2857be30f4f6222527..145e4943ebbaaea6a3ca80a7f63bd0969d7bc460 100644 (file)
--- a/pdns/dnsparser.cc
+++ b/pdns/dnsparser.cc
@@ -745,9 +745,9 @@ void editDNSPacketTTL(char* packet, size_t length, const std::function<uint32_t(
   }
 }
 
-int rewritePacketWithoutRecordTypes(const PacketBuffer& initialPacket, PacketBuffer& newContent, const std::set<QType>& qtypes)
+static int rewritePacketWithoutRecordTypes(const PacketBuffer& initialPacket, PacketBuffer& newContent, const std::set<QType>& qtypes)
 {
-  static const std::set<QType>& 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<QType>& 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 631636bae27c8db4bc7a4e7b2381d3f0f478bc80..7d57daddbf97e05ab8cd5b61e1835c39903b790d 100644 (file)
--- 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 2ec3802a5e7b8373ed6875e74090aac7d70721a4..b040237881f3d0b8c3f73f29b5027a7ba413b61e 100644 (file)
--- 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<uint8_t> 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<const char*>(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<char*>(packet.data()), packet.size(), 1, QType::A), 1);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(packet.data()), packet.size(), 1, QType::AAAA), 1);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(packet.data()), packet.size(), 3, QType::A), 1);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(packet.data()), packet.size(), 3, QType::MX), 1);
+
+    std::set<QType> 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<char*>(packet.data()), packet.size(), 1, QType::A), 1);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(packet.data()), packet.size(), 1, QType::AAAA), 1);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(packet.data()), packet.size(), 3, QType::A), 1);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(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<char*>(packet.data()), packet.size(), 1, QType::A), 1);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(packet.data()), packet.size(), 1, QType::AAAA), 0);
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(packet.data()), packet.size(), 3, QType::A), 1); 
+    BOOST_CHECK_EQUAL(getRecordsOfTypeCount(reinterpret_cast<char*>(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 301048b164462ee92977c0c214053d2bbb4472b1..1fd2f4f054f35bc7303339d79e8887773cc475a7 100644 (file)
--- 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']