Raise an exception on invalid hex content in unknown records

Otherwise we can end up reading uninitialised memory from the stack,
possibly leaking information.
This is only an issue if the content is read from an untrusted source
and can be passed back to an attacker.

diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc
index 4ff718711d6ffce323b006d038d0612eff4fbfb2..2c2669b6b172f89c3a6db96ba38bd902e1ffc0d8 100644 (file)
--- a/pdns/dnsparser.cc
+++ b/pdns/dnsparser.cc
@@ -40,17 +40,25 @@ public:
     // parse the input
     vector<string> parts;
     stringtok(parts, zone);
-    if(parts.size()!=3 && !(parts.size()==2 && equals(parts[1],"0")) )
-      throw MOADNSException("Unknown record was stored incorrectly, need 3 fields, got "+std::to_string(parts.size())+": "+zone );
-    const string& relevant=(parts.size() > 2) ? parts[2] : "";
-    unsigned int total=pdns_stou(parts[1]);
-    if(relevant.size() % 2 || relevant.size() / 2 != total)
+    // we need exactly 3 parts, except if the length field is set to 0 then we only need 2
+    if (parts.size() != 3 && !(parts.size() == 2 && equals(parts[1], "0"))) {
+      throw MOADNSException("Unknown record was stored incorrectly, need 3 fields, got " + std::to_string(parts.size()) + ": " + zone);
+    }
+
+    const string& relevant = (parts.size() > 2) ? parts[2] : "";
+    unsigned int total = pdns_stou(parts[1]);
+    if (relevant.size() % 2 || (relevant.size() / 2) != total) {
       throw MOADNSException((boost::format("invalid unknown record length: size not equal to length field (%d != 2 * %d)") % relevant.size() % total).str());
+    }
+
     string out;
-    out.reserve(total+1);
-    for(unsigned int n=0; n < total; ++n) {
+    out.reserve(total + 1);
+
+    for (unsigned int n = 0; n < total; ++n) {
       int c;
-      sscanf(relevant.c_str()+2*n, "%02x", &c);
+      if (sscanf(relevant.c_str()+2*n, "%02x", &c) != 1) {
+        throw MOADNSException("unable to read data at position " + std::to_string(2 * n) + " from unknown record of size " + std::to_string(relevant.size()));
+      }
       out.append(1, (char)c);
     }
 

diff --git a/pdns/test-dnsrecords_cc.cc b/pdns/test-dnsrecords_cc.cc
index 5e50111a3f1d6a3bef404f3c762124090d37aeae..2cd037ee695714a81f8b299e6c446914bed3b3ec 100644 (file)
--- a/pdns/test-dnsrecords_cc.cc
+++ b/pdns/test-dnsrecords_cc.cc
@@ -379,6 +379,38 @@ BOOST_AUTO_TEST_CASE(test_opt_record_out) {
   BOOST_CHECK_EQUAL(makeHexDump(std::string(pak.begin(),pak.end())), makeHexDump(packet));
 }
 
+// special record test, because Unknown record types are the worst
+BOOST_AUTO_TEST_CASE(test_unknown_records_in) {
+
+  auto validUnknown = DNSRecordContent::mastermake(static_cast<QType::typeenum>(65534), QClass::IN, "\\# 1 42");
+
+  // we need at least two parts
+  BOOST_CHECK_THROW(auto notEnoughPartsUnknown = DNSRecordContent::mastermake(static_cast<QType::typeenum>(65534), QClass::IN, "\\#"), MOADNSException);
+
+  // two parts are OK when the RDATA size is 0, not OK otherwise
+  auto validEmptyUnknown = DNSRecordContent::mastermake(static_cast<QType::typeenum>(65534), QClass::IN, "\\# 0");
+  BOOST_CHECK_THROW(auto twoPartsNotZeroUnknown = DNSRecordContent::mastermake(static_cast<QType::typeenum>(65534), QClass::IN, "\\# 1"), MOADNSException);
+
+  // RDATA length is not even
+  BOOST_CHECK_THROW(auto unevenUnknown = DNSRecordContent::mastermake(static_cast<QType::typeenum>(65534), QClass::IN, "\\# 1 A"), MOADNSException);
+
+  // RDATA length is not equal to the expected size
+  BOOST_CHECK_THROW(auto wrongRDATASizeUnknown = DNSRecordContent::mastermake(static_cast<QType::typeenum>(65534), QClass::IN, "\\# 2 AA"), MOADNSException);
+
+  // RDATA is invalid (invalid hex value)
+  try {
+    auto invalidRDATAUnknown = DNSRecordContent::mastermake(static_cast<QType::typeenum>(65534), QClass::IN, "\\# 1 JJ");
+    // we should not reach that code
+    BOOST_CHECK(false);
+    // but if we do let's see what we got (likely what was left over on the stack)
+    BOOST_CHECK_EQUAL(invalidRDATAUnknown->getZoneRepresentation(), "\\# 1 jj");
+  }
+  catch (const MOADNSException& e)
+  {
+    // it's expected to end up there
+  }
+}
+
 // special record test, because EUI are odd
 BOOST_AUTO_TEST_CASE(test_eui_records_in) {