Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

harden DNS resolver (insp20 branch) #2

Closed
wants to merge 4 commits into from

1 participant

William Pitcock
William Pitcock

these commits harden the DNS resolver to fix the CERT bug.

new changes:

  • don't explicitly trust rr.rdlength
  • validate lengths on decompression (using same behaviour as charybdis)
  • check A/AAAA replies too, as they could be exploited using similar technique
kaniini added some commits
William Pitcock kaniini dns: iterators which are integer should always be unsigned, else an i…
…nteger underflow is possible.

Signed-off-by: William Pitcock <nenolod@dereferenced.org>
a6a07de
William Pitcock kaniini dns: reject messages with lengths larger than DNSHeader with prejudice
This also includes when decompressing name entries.
9aa28f3
William Pitcock kaniini dns: more hardening
- don't trust rr.rdlength
- don't accept replies we know are impossible for AAAA/A records
- don't try to process record types we do not know about specifically
  (this behaviour just leads to disaster)
84ab047
William Pitcock kaniini dns: cleanup ResultIsReady() prototype eac05f8
William Pitcock kaniini closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 20, 2012
  1. William Pitcock

    dns: iterators which are integer should always be unsigned, else an i…

    kaniini authored
    …nteger underflow is possible.
    
    Signed-off-by: William Pitcock <nenolod@dereferenced.org>
  2. William Pitcock

    dns: reject messages with lengths larger than DNSHeader with prejudice

    kaniini authored
    This also includes when decompressing name entries.
  3. William Pitcock

    dns: more hardening

    kaniini authored
    - don't trust rr.rdlength
    - don't accept replies we know are impossible for AAAA/A records
    - don't try to process record types we do not know about specifically
      (this behaviour just leads to disaster)
  4. William Pitcock
This page is out of date. Refresh to see the latest.
Showing with 38 additions and 10 deletions.
  1. +38 −10 src/dns.cpp
48 src/dns.cpp
View
@@ -38,6 +38,8 @@ looks like this, walks like this or tastes like this.
#include "configreader.h"
#include "socket.h"
+#define DN_COMP_BITMASK 0xC000 /* highest 6 bits in a DN label header */
+
/** Masks to mask off the responses we get from the DNSRequest methods
*/
enum QueryInfo
@@ -98,7 +100,7 @@ class DNSRequest
DNSRequest(DNS* dns, int id, const std::string &original);
~DNSRequest();
- DNSInfo ResultIsReady(DNSHeader &h, int length);
+ DNSInfo ResultIsReady(DNSHeader &h, unsigned length);
int SendRequests(const DNSHeader *header, const int length, QueryType qt);
};
@@ -161,7 +163,10 @@ int CachedQuery::CalcTTLRemaining()
/* Allocate the processing buffer */
DNSRequest::DNSRequest(DNS* dns, int rid, const std::string &original) : dnsobj(dns)
{
- res = new unsigned char[512];
+ /* hardening against overflow here: make our work buffer twice the theoretical
+ * maximum size so that hostile input doesn't screw us over.
+ */
+ res = new unsigned char[sizeof(DNSHeader) * 2];
*res = 0;
orig = original;
RequestTimeout* RT = new RequestTimeout(ServerInstance->Config->dns_timeout ? ServerInstance->Config->dns_timeout : 5, this, rid);
@@ -688,11 +693,11 @@ DNSResult DNS::GetResult()
}
/** A result is ready, process it */
-DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length)
+DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, unsigned length)
{
- int i = 0;
+ unsigned i = 0, o;
int q = 0;
- int curanswer, o;
+ int curanswer;
ResourceRecord rr;
unsigned short ptr;
@@ -790,17 +795,31 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length)
switch (rr.type)
{
+ /*
+ * CNAME and PTR are compressed. We need to decompress them.
+ */
case DNS_QUERY_CNAME:
- /* CNAME and PTR have the same processing code */
case DNS_QUERY_PTR:
o = 0;
q = 0;
while (q == 0 && i < length && o + 256 < 1023)
{
+ /* DN label found (byte over 63) */
if (header.payload[i] > 63)
{
memcpy(&ptr,&header.payload[i],2);
- i = ntohs(ptr) - 0xC000 - 12;
+
+ i = ntohs(ptr);
+
+ /* check that highest two bits are set. if not, we've been had */
+ if (!(i & DN_COMP_BITMASK))
+ return std::make_pair((unsigned char *) NULL, "DN label decompression header is bogus");
+
+ /* mask away the two highest bits. */
+ i &= ~DN_COMP_BITMASK;
+
+ /* and decrease length by 12 bytes. */
+ i =- 12;
}
else
{
@@ -813,7 +832,11 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length)
res[o] = 0;
if (o != 0)
res[o++] = '.';
- memcpy(&res[o],&header.payload[i + 1],header.payload[i]);
+
+ if (o + header.payload[i] > sizeof(DNSHeader))
+ return std::make_pair((unsigned char *) NULL, "DN label decompression is impossible -- malformed/hostile packet?");
+
+ memcpy(&res[o], &header.payload[i + 1], header.payload[i]);
o += header.payload[i];
i += header.payload[i] + 1;
}
@@ -822,16 +845,21 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length)
res[o] = 0;
break;
case DNS_QUERY_AAAA:
+ if (rr.rdlength != sizeof(struct in6_addr))
+ return std::make_pair((unsigned char *) NULL, "rr.rdlength is larger than 16 bytes for an ipv6 entry -- malformed/hostile packet?");
+
memcpy(res,&header.payload[i],rr.rdlength);
res[rr.rdlength] = 0;
break;
case DNS_QUERY_A:
+ if (rr.rdlength != sizeof(struct in_addr))
+ return std::make_pair((unsigned char *) NULL, "rr.rdlength is larger than 4 bytes for an ipv4 entry -- malformed/hostile packet?");
+
memcpy(res,&header.payload[i],rr.rdlength);
res[rr.rdlength] = 0;
break;
default:
- memcpy(res,&header.payload[i],rr.rdlength);
- res[rr.rdlength] = 0;
+ return std::make_pair((unsigned char *) NULL, "don't know how to handle undefined type (" + ConvToStr(rr.type) + ") -- rejecting");
break;
}
return std::make_pair(res,"No error");
Something went wrong with that request. Please try again.