From f5234115710094a8a7bd0b5ebddaa1fde83a52f0 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 11 Aug 2020 11:25:06 +0200
Subject: [PATCH] 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.
---
 pdns/dnsparser.cc          | 24 ++++++++++++++++--------
 pdns/test-dnsrecords_cc.cc | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc
index c7f9f5d4d4..6365eb5526 100644
--- 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 441f6464d0..c8e2bff70f 100644
--- a/pdns/test-dnsrecords_cc.cc
+++ b/pdns/test-dnsrecords_cc.cc
@@ -378,6 +378,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) {
 
-- 
2.47.2