From: Wouter Wijngaards <wouter@nlnetlabs.nl>
Date: Tue, 16 Oct 2007 13:03:57 +0000 (+0000)
Subject: buffer overflow code audit.
X-Git-Tag: release-0.6~64
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=189fafa1da7d2ae12d1c12c1f9651c31635416c4;p=thirdparty%2Funbound.git

buffer overflow code audit.


git-svn-id: file:///svn/unbound/trunk@680 be551aaa-1e26-0410-a405-d3ace91eadb9
---

diff --git a/daemon/unbound.c b/daemon/unbound.c
index e696fffbd..5cfd162e2 100644
--- a/daemon/unbound.c
+++ b/daemon/unbound.c
@@ -183,6 +183,7 @@ readpid (const char* file)
 		return -1;
 	}
 
+	pidbuf[sizeof(pidbuf)-1] = 0;
 	pid = (pid_t)strtol(pidbuf, &t, 10);
 	
 	if (*t && *t != '\n') {
diff --git a/doc/Changelog b/doc/Changelog
index 638db8be6..229789f81 100644
--- a/doc/Changelog
+++ b/doc/Changelog
@@ -2,6 +2,14 @@
 	- no malloc in log_hex.
 	- assertions around system calls.
 	- protect against gethostname without ending zero.
+	- ntop output is null terminated by unbound.
+	- pidfile content null termination
+	- various snprintf use sizeof(stringbuf) instead of fixed constant.
+	- changed loopdetect % 8 with & 0x7 since % can become negative for
+	  weird negative input and particular interpretation of integer math.
+	- dname_pkt_copy checks length of result, to protect result buffers.
+	  prints an error, this should not happen. Bad strings should have
+	  been rejected earlier in the program.
 
 15 October 2007: Wouter
 	- nicer warning.
diff --git a/services/listen_dnsport.c b/services/listen_dnsport.c
index 48f7c5e1a..960a8f376 100644
--- a/services/listen_dnsport.c
+++ b/services/listen_dnsport.c
@@ -74,6 +74,7 @@ verbose_print_addr(struct addrinfo *addr)
 			(socklen_t)sizeof(buf)) == 0) {
 			strncpy(buf, "(null)", sizeof(buf));
 		}
+		buf[sizeof(buf)-1] = 0;
 		verbose(VERB_ALGO, "creating %s%s socket %s %d", 
 			addr->ai_socktype==SOCK_DGRAM?"udp":
 			addr->ai_socktype==SOCK_STREAM?"tcp":"otherproto",
diff --git a/util/data/dname.c b/util/data/dname.c
index e0826a29f..cfa919613 100644
--- a/util/data/dname.c
+++ b/util/data/dname.c
@@ -152,8 +152,8 @@ loopcheck(uint8_t loop[], size_t pos)
 	const uint8_t bits[8] = {0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80};
 	uint8_t ret;
 	log_assert(pos < MAX_COMPRESS_POS);
-	ret = loop[ pos / 8 ] & bits[ pos % 8 ];
-	loop[ pos / 8 ] |= bits[ pos % 8 ];	
+	ret = loop[ pos / 8 ] & bits[ pos & 0x7 ];
+	loop[ pos / 8 ] |= bits[ pos & 0x7 ];	
 	return ret;
 }
 
@@ -305,6 +305,7 @@ dname_pkt_hash(ldns_buffer* pkt, uint8_t* dname, hashvalue_t h)
 void dname_pkt_copy(ldns_buffer* pkt, uint8_t* to, uint8_t* dname)
 {
 	/* copy over the dname and decompress it at the same time */
+	size_t len = 0;
 	uint8_t lablen;
 	lablen = *dname++;
 	while(lablen) {
@@ -315,6 +316,12 @@ void dname_pkt_copy(ldns_buffer* pkt, uint8_t* to, uint8_t* dname)
 			continue;
 		}
 		log_assert(lablen <= LDNS_MAX_LABELLEN);
+		len += (size_t)lablen+1;
+		if(len >= LDNS_MAX_DOMAINLEN) {
+			*to = 0; /* end the result prematurely */
+			log_err("bad dname in dname_pkt_copy");
+			return;
+		}
 		*to++ = lablen;
 		memmove(to, dname, lablen);
 		dname += lablen;
diff --git a/util/net_help.c b/util/net_help.c
index 7eadca4fd..0fa6a44c1 100644
--- a/util/net_help.c
+++ b/util/net_help.c
@@ -164,6 +164,7 @@ log_addr(const char* str, struct sockaddr_storage* addr, socklen_t addrlen)
         if(inet_ntop(af, sinaddr, dest, (socklen_t)sizeof(dest)) == 0) {
                 strncpy(dest, "(inet_ntop error)", sizeof(dest));
         }
+	dest[sizeof(dest)-1] = 0;
         port = ntohs(((struct sockaddr_in*)addr)->sin_port);
         verbose(VERB_DETAIL, "%s %s %s %d (len %d)",
                 str, family, dest, (int)port, (int)addrlen);
@@ -244,14 +245,14 @@ log_nametypeclass(enum verbosity_value v, const char* str, uint8_t* name,
 	else if(ldns_rr_descript(type) && ldns_rr_descript(type)->_name)
 		ts = ldns_rr_descript(type)->_name;
 	else {
-		snprintf(t, 12, "TYPE%d", (int)type);
+		snprintf(t, sizeof(t), "TYPE%d", (int)type);
 		ts = t;
 	}
 	if(ldns_lookup_by_id(ldns_rr_classes, (int)dclass) &&
 		ldns_lookup_by_id(ldns_rr_classes, (int)dclass)->name)
 		cs = ldns_lookup_by_id(ldns_rr_classes, (int)dclass)->name;
 	else {
-		snprintf(c, 12, "CLASS%d", (int)dclass);
+		snprintf(c, sizeof(c), "CLASS%d", (int)dclass);
 		cs = c;
 	}
 	log_info("%s <%s %s %s>", str, buf, ts, cs);
@@ -279,6 +280,7 @@ void log_name_addr(enum verbosity_value v, const char* str, uint8_t* zone,
         if(inet_ntop(af, sinaddr, dest, (socklen_t)sizeof(dest)) == 0) {
                 strncpy(dest, "(inet_ntop error)", sizeof(dest));
         }
+	dest[sizeof(dest)-1] = 0;
         port = ntohs(((struct sockaddr_in*)addr)->sin_port);
 	dname_str(zone, namebuf);
 	if(af != AF_INET && af != AF_INET6)
diff --git a/util/region-allocator.c b/util/region-allocator.c
index 6cf4dafeb..23dac4d6b 100644
--- a/util/region-allocator.c
+++ b/util/region-allocator.c
@@ -510,7 +510,7 @@ region_log_stats(region_type *region)
 {
 	char buf[10240], *str=buf;
 	int len=0;
-	snprintf(str, 10240, "%lu objects (%lu small/%lu large), %lu bytes allocated (%lu wasted) in %lu chunks, %lu cleanups, %lu in recyclebin%n",
+	snprintf(str, sizeof(buf), "%lu objects (%lu small/%lu large), %lu bytes allocated (%lu wasted) in %lu chunks, %lu cleanups, %lu in recyclebin%n",
 		(unsigned long) (region->small_objects + region->large_objects),
 		(unsigned long) region->small_objects,
 		(unsigned long) region->large_objects,
@@ -532,7 +532,7 @@ region_log_stats(region_type *region)
 				el = el->next;
 			}
 			if(i%ALIGNMENT == 0 && i!=0) {
-				snprintf(str, 10240, " %lu%n", 
+				snprintf(str, sizeof(buf)-(str-buf), " %lu%n", 
 					(unsigned long)count, &len);
 				str+=len;
 			}