From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 26 Apr 2016 09:09:05 +0000 (+0200)
Subject: Add a minimum offset parameter to DNSName
X-Git-Tag: rec-4.0.0-alpha3~35^2
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=83fc9d8a9e8f04ed0bb082961b87a2c1494390f1;p=pdns

Add a minimum offset parameter to DNSName

PacketReader does not copy the header part of the DNS message,
therefore DNSName needs to be aware of the minimum valid offset
in order not to follow a pointer to an offset < sizeof(dnsheader),
as other callers expect every non-negative offset to be valid.
Found with American Fuzzy Lop and Address Sanitizer.
---

diff --git a/pdns/dnsname.cc b/pdns/dnsname.cc
index 55849c5a9..e8e2c853b 100644
--- a/pdns/dnsname.cc
+++ b/pdns/dnsname.cc
@@ -57,7 +57,7 @@ DNSName::DNSName(const char* p)
 }
 
 
-DNSName::DNSName(const char* pos, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed)
+DNSName::DNSName(const char* pos, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, uint16_t minOffset)
 {
   if (offset >= len)
     throw std::range_error("Trying to read past the end of the buffer ("+std::to_string(offset)+ " >= "+std::to_string(len)+")");
@@ -68,11 +68,11 @@ DNSName::DNSName(const char* pos, int len, int offset, bool uncompress, uint16_t
     }
   }
 
-  packetParser(pos, len, offset, uncompress, qtype, qclass, consumed);
+  packetParser(pos, len, offset, uncompress, qtype, qclass, consumed, 0, minOffset);
 }
 
 // this should be the __only__ dns name parser in PowerDNS.
-void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth)
+void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset)
 {
   const unsigned char* pos=(const unsigned char*)qpos;
   unsigned char labellen;
@@ -80,6 +80,8 @@ void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompres
 
   if (offset >= len)
     throw std::range_error("Trying to read past the end of the buffer ("+std::to_string(offset)+ " >= "+std::to_string(len)+")");
+  if (offset < (int) minOffset)
+    throw std::range_error("Trying to read before the beginning of the buffer ("+std::to_string(offset)+ " < "+std::to_string(minOffset)+")");
 
   const unsigned char* end = pos + len;
   pos += offset;
@@ -92,9 +94,11 @@ void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompres
       int newpos = (labellen << 8) + *(const unsigned char*)pos;
 
       if(newpos < offset) {
+        if(newpos < (int) minOffset)
+          throw std::range_error("Invalid label position during decompression ("+std::to_string(newpos)+ " < "+std::to_string(minOffset)+")");
         if (++depth > 100)
           throw std::range_error("Abort label decompression after 100 redirects");
-        packetParser((const char*)opos, len, newpos, true, 0, 0, 0, depth);
+        packetParser((const char*)opos, len, newpos, true, 0, 0, 0, depth, minOffset);
       } else
         throw std::range_error("Found a forward reference during label decompression");
       pos++;
diff --git a/pdns/dnsname.hh b/pdns/dnsname.hh
index a0662cb26..f2311655a 100644
--- a/pdns/dnsname.hh
+++ b/pdns/dnsname.hh
@@ -45,7 +45,7 @@ public:
   DNSName()  {}          //!< Constructs an *empty* DNSName, NOT the root!
   explicit DNSName(const char* p);      //!< Constructs from a human formatted, escaped presentation
   explicit DNSName(const std::string& str) : DNSName(str.c_str()) {}; //!< Constructs from a human formatted, escaped presentation
-  DNSName(const char* p, int len, int offset, bool uncompress, uint16_t* qtype=0, uint16_t* qclass=0, unsigned int* consumed=0); //!< Construct from a DNS Packet, taking the first question if offset=12
+  DNSName(const char* p, int len, int offset, bool uncompress, uint16_t* qtype=0, uint16_t* qclass=0, unsigned int* consumed=0, uint16_t minOffset=0); //!< Construct from a DNS Packet, taking the first question if offset=12
   
   bool isPartOf(const DNSName& rhs) const;   //!< Are we part of the rhs name?
   bool operator==(const DNSName& rhs) const; //!< DNS-native comparison (case insensitive) - empty compares to empty
@@ -112,7 +112,7 @@ public:
 private:
   string_t d_storage;
 
-  void packetParser(const char* p, int len, int offset, bool uncompress, uint16_t* qtype=0, uint16_t* qclass=0, unsigned int* consumed=0, int depth=0);
+  void packetParser(const char* p, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset);
   static std::string escapeLabel(const std::string& orig);
   static std::string unescapeLabel(const std::string& orig);
 };
diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc
index 4e28afdad..d7eecd2e8 100644
--- a/pdns/dnsparser.cc
+++ b/pdns/dnsparser.cc
@@ -399,7 +399,7 @@ DNSName PacketReader::getName()
 {
   unsigned int consumed;
   try {
-    DNSName dn((const char*) d_content.data() - 12, d_content.size() + 12, d_pos + sizeof(dnsheader), true /* uncompress */, 0 /* qtype */, 0 /* qclass */, &consumed);
+    DNSName dn((const char*) d_content.data() - 12, d_content.size() + 12, d_pos + sizeof(dnsheader), true /* uncompress */, 0 /* qtype */, 0 /* qclass */, &consumed, sizeof(dnsheader));
     
     // the -12 fakery is because we don't have the header in 'd_content', but we do need to get 
     // the internal offsets to work