small memory leak in libc/inet/resolv.c

[Timothy Holdener]

We've discovered a small memory leak in __dns_lookup() when the A record 
in the DNS answer is preceded by one or more CNAME records.

This can be reproduced by performing a gethostbyname() lookup on
"www.google.com" or "www.yahoo.com".  After each CNAME record,
__dns_lookup() leaks by 256 bytes.

=====================================
The problem code is in this segment:

        first_answer = 1;
        for (j = 0; j < h.ancount; j++) {
            i = __decode_answer(packet, pos, packet_len, &ma);

            <...snipped code for brevity...>

            if (first_answer) {
                ma.buf = a->buf;
                ma.buflen = a->buflen;
                ma.add_count = a->add_count;
                memcpy(a, &ma, sizeof(ma));
                if (a->atype != T_SIG && (NULL == a->buf || (type != T_A && type != T_AAAA)))
                    break;
                if (a->atype != type)
                    continue;
                a->add_count = h.ancount - j - 1;
                if ((a->rdlength + sizeof(struct in_addr*)) * a->add_count > a->buflen)
                    break;
                a->add_count = 0;
                first_answer = 0;
            } else {
=====================================

On the first time through the loop (j == 0) with the CNAME record, 
ma.dotted is alloc'ed using a strdup() in decode_answer(), and memcpy() 
copies it to a->dotted. The loop is continued at "if (a->atype != type)".

On the second time through the loop (j == 1), ma.dotted is again 
alloc'ed in decode_answer().  Now we have two allocated segments, one
pointed-to by ma.dotted, and the other pointed-to by a->dotted.
But memcpy() will over-write the pointer stored at a->dotted.

My proposed solution is to check a->dotted before the memcpy() and free() 
it if it is non-NULL. (See the attached patch.)

I've tested my solution with a number of different public DNS servers
resolving a variety of names, but we're using an older version of uClibc.

This leak was introduced in April 2010 when a similar free() call was
removed for the test case when a DNS server returns a lone CNAME record.
(ipv6.google.com)

-- 
Tim Holdener
-------------- next part --------------
--- uClibc-0.9.32.orig/libc/inet/resolv.c	2011-10-12 04:28:18.000000000 -0700
+++ uClibc-0.9.32/libc/inet/resolv.c	2011-10-13 15:59:05.000000000 -0700
@@ -1503,6 +1503,7 @@
 		DPRINTF("Decoding answer at pos %d\n", pos);
 
 		first_answer = 1;
+		a->dotted = NULL;
 		for (j = 0; j < h.ancount; j++) {
 			i = __decode_answer(packet, pos, packet_len, &ma);
 			if (i < 0) {
@@ -1519,6 +1520,8 @@
 				ma.buf = a->buf;
 				ma.buflen = a->buflen;
 				ma.add_count = a->add_count;
+				if (a->dotted)
+					free(a->dotted);
 				memcpy(a, &ma, sizeof(ma));
 				if (a->atype != T_SIG && (NULL == a->buf || (type != T_A && type != T_AAAA)))
 					break;