From a6a07de0daa353bcd29056a4535a9c4784c113c8 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Tue, 20 Mar 2012 17:48:45 -0500 Subject: dns: iterators which are integer should always be unsigned, else an integer underflow is possible. Signed-off-by: William Pitcock --- src/dns.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/dns.cpp b/src/dns.cpp index 945e1fb15..abdd0a54c 100644 --- a/src/dns.cpp +++ b/src/dns.cpp @@ -690,7 +690,7 @@ DNSResult DNS::GetResult() /** A result is ready, process it */ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) { - int i = 0; + unsigned i = 0; int q = 0; int curanswer, o; ResourceRecord rr; -- cgit v1.2.3 From 9aa28f3730fb3dd69c1e06f78bb2bbc43d36c684 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Tue, 20 Mar 2012 18:31:14 -0500 Subject: dns: reject messages with lengths larger than DNSHeader with prejudice This also includes when decompressing name entries. --- src/dns.cpp | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/dns.cpp b/src/dns.cpp index abdd0a54c..2bfa0be20 100644 --- a/src/dns.cpp +++ b/src/dns.cpp @@ -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 @@ -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); @@ -690,9 +695,9 @@ DNSResult DNS::GetResult() /** A result is ready, process it */ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) { - unsigned i = 0; + unsigned i = 0, o; int q = 0; - int curanswer, o; + int curanswer; ResourceRecord rr; unsigned short ptr; @@ -717,7 +722,7 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) /* Subtract the length of the header from the length of the packet */ length -= 12; - while ((unsigned int)q < header.qdcount && i < length) + while ((unsigned int)q < header.qdcount && i < (unsigned) length) { if (header.payload[i] > 63) { @@ -738,7 +743,7 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) while ((unsigned)curanswer < header.ancount) { q = 0; - while (q == 0 && i < length) + while (q == 0 && i < (unsigned) length) { if (header.payload[i] > 63) { @@ -755,7 +760,7 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) else i += header.payload[i] + 1; /* skip length and label */ } } - if (length - i < 10) + if ((unsigned) length - i < 10) return std::make_pair((unsigned char*)NULL,"Incorrectly sized DNS reply"); /* XXX: We actually initialise 'rr' here including its ttl field */ @@ -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) + while (q == 0 && i < (unsigned) 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; } -- cgit v1.2.3 From 84ab0478f9dd7f7f8dc92aa1edaf6b71fe28035b Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Tue, 20 Mar 2012 18:38:13 -0500 Subject: 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) --- src/dns.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/dns.cpp b/src/dns.cpp index 2bfa0be20..21b0d9e39 100644 --- a/src/dns.cpp +++ b/src/dns.cpp @@ -845,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"); -- cgit v1.2.3 From eac05f8d05ce2e3878ac4a51675b11e64831adac Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Tue, 20 Mar 2012 18:41:09 -0500 Subject: dns: cleanup ResultIsReady() prototype --- src/dns.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/dns.cpp b/src/dns.cpp index 21b0d9e39..2e1c751c4 100644 --- a/src/dns.cpp +++ b/src/dns.cpp @@ -100,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); }; @@ -693,7 +693,7 @@ DNSResult DNS::GetResult() } /** A result is ready, process it */ -DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) +DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, unsigned length) { unsigned i = 0, o; int q = 0; @@ -722,7 +722,7 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) /* Subtract the length of the header from the length of the packet */ length -= 12; - while ((unsigned int)q < header.qdcount && i < (unsigned) length) + while ((unsigned int)q < header.qdcount && i < length) { if (header.payload[i] > 63) { @@ -743,7 +743,7 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) while ((unsigned)curanswer < header.ancount) { q = 0; - while (q == 0 && i < (unsigned) length) + while (q == 0 && i < length) { if (header.payload[i] > 63) { @@ -760,7 +760,7 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) else i += header.payload[i] + 1; /* skip length and label */ } } - if ((unsigned) length - i < 10) + if (length - i < 10) return std::make_pair((unsigned char*)NULL,"Incorrectly sized DNS reply"); /* XXX: We actually initialise 'rr' here including its ttl field */ @@ -802,7 +802,7 @@ DNSInfo DNSRequest::ResultIsReady(DNSHeader &header, int length) case DNS_QUERY_PTR: o = 0; q = 0; - while (q == 0 && i < (unsigned) length && o + 256 < 1023) + while (q == 0 && i < length && o + 256 < 1023) { /* DN label found (byte over 63) */ if (header.payload[i] > 63) -- cgit v1.2.3