CVE-2017-12995: Loop with Unreachable Exit Condition ('Infinite Loop') (CWE-835)
value: The DNS parser in tcpdump before 4.9.2 could enter an infinite loop due to a bug in print-domain.c:ns_print().
CWE description: The product contains an iteration or loop with an exit condition that cannot be reached, i.e., an infinite loop.
CVSS v3 base score: 7.5
Published: 2017-09-14T06:29Z
Repository: https://github.com/the-tcpdump-group/tcpdump
Language: Other
Files changed: print-domain.c, tests/TESTLIST, tests/dns-zlip-1.out, tests/dns-zlip-2.out, tests/dns-zlip-3.out, tests/dns_fwdptr.out
Commit date: 2017-09-13 12:25:44 +0100
Commit message: CVE-2017-12995/Check for DNS compression pointers that don't point backwards.

This is what BIND 9.11.0-P2 does; it not only detects pointers that
loop, as "point backwards" means "point before anything we've processed
so far, including what we're processing right now", so the pointer can't
point to itself (as that's what we're processing right now).

This fixes an infinite loop discovered by Forcepoint's security
researchers Otto Airamo & Antti Levomäki.

Add a test using the capture file supplied by the reporter(s).

Also, add some infinite-pointer-loop captures.

More checks should be done.  We might, for example, want to make sure
the upper 2 bits of the label length/pointer byte are 00 or 11, and that
if we encounter a pointer and jump backwards to what we think is a label
the label ends before the beginning of the last label we processed, to
make sure the pointer doesn't point backwards into the *middle* of a
label, and also make sure the entire name is <= 255 bytes long.

Diff: diff --git a/print-domain.c b/print-domain.c
index d0b6996fd..9a2b9e856 100644
--- a/print-domain.c
+++ b/print-domain.c
@@ -149,19 +149,18 @@ ns_nprint(netdissect_options *ndo,
           register const u_char *cp, register const u_char *bp)
 {
 	register u_int i, l;
 	register const u_char *rp = NULL;
 	register int compress = 0;
-	int chars_processed;
 	int elt;
-	int data_size = ndo->ndo_snapend - bp;
+	u_int offset, max_offset;
 
 	if ((l = labellen(ndo, cp)) == (u_int)-1)
 		return(NULL);
 	if (!ND_TTEST2(*cp, 1))
 		return(NULL);
-	chars_processed = 1;
+	max_offset = (u_int)(cp - bp);
 	if (((i = *cp++) & INDIR_MASK) != INDIR_MASK) {
 		compress = 0;
 		rp = cp + l;
 	}
 
@@ -172,28 +171,32 @@ ns_nprint(netdissect_options *ndo,
 					rp = cp + 1;
 					compress = 1;
 				}
 				if (!ND_TTEST2(*cp, 1))
 					return(NULL);
-				cp = bp + (((i << 8) | *cp) & 0x3fff);
+				offset = (((i << 8) | *cp) & 0x3fff);
+				/*
+				 * This must move backwards in the packet.
+				 * No RFC explicitly says that, but BIND's
+				 * name decompression code requires it,
+				 * as a way of preventing infinite loops
+				 * and other bad behavior, and it's probably
+				 * what was intended (compress by pointing
+				 * to domain name suffixes already seen in
+				 * the packet).
+				 */
+				if (offset >= max_offset) {
+					ND_PRINT((ndo, "<BAD PTR>"));
+					return(NULL);
+				}
+				max_offset = offset;
+				cp = bp + offset;
 				if ((l = labellen(ndo, cp)) == (u_int)-1)
 					return(NULL);
 				if (!ND_TTEST2(*cp, 1))
 					return(NULL);
 				i = *cp++;
-				chars_processed++;
-
-				/*
-				 * If we've looked at every character in
-				 * the message, this pointer will make
-				 * us look at some character again,
-				 * which means we're looping.
-				 */
-				if (chars_processed >= data_size) {
-					ND_PRINT((ndo, "<LOOP>"));
-					return (NULL);
-				}
 				continue;
 			}
 			if ((i & INDIR_MASK) == EDNS0_MASK) {
 				elt = (i & ~INDIR_MASK);
 				switch(elt) {
@@ -210,18 +213,16 @@ ns_nprint(netdissect_options *ndo,
 				if (fn_printn(ndo, cp, l, ndo->ndo_snapend))
 					return(NULL);
 			}
 
 			cp += l;
-			chars_processed += l;
 			ND_PRINT((ndo, "."));
 			if ((l = labellen(ndo, cp)) == (u_int)-1)
 				return(NULL);
 			if (!ND_TTEST2(*cp, 1))
 				return(NULL);
 			i = *cp++;
-			chars_processed++;
 			if (!compress)
 				rp += l + 1;
 		}
 	else
 		ND_PRINT((ndo, "."));
diff --git a/tests/TESTLIST b/tests/TESTLIST
index ec627cb82..0ddc63e39 100644
--- a/tests/TESTLIST
+++ b/tests/TESTLIST
@@ -472,14 +472,23 @@ tok2str-oobr-2		tok2str-oobr-2.pcap		tok2str-oobr-2.out	-vvv -e
 eigrp-tlv-oobr		eigrp-tlv-oobr.pcap		eigrp-tlv-oobr.out	-vvv -e
 zephyr-oobr		zephyr-oobr.pcap		zephyr-oobr.out		-vvv -e
 isakmp-no-none-np	isakmp-no-none-np.pcap		isakmp-no-none-np.out	-vvv -e
 telnet-iac-check-oobr	telnet-iac-check-oobr.pcap	telnet-iac-check-oobr.out	-vvv -e
 resp_4_infiniteloop	resp_4_infiniteloop.pcap	resp_4_infiniteloop.out	-vvv -e
+dns_fwdptr		dns_fwdptr.pcap			dns_fwdptr.out		-vvv -e
 
 # RTP tests
 # fuzzed pcap
 rtp-seg-fault-1  rtp-seg-fault-1.pcap  rtp-seg-fault-1.out  -v -T rtp
 rtp-seg-fault-2  rtp-seg-fault-2.pcap  rtp-seg-fault-2.out  -v -T rtp
 
 # NFS tests
 # fuzzed pcap
 nfs-seg-fault-1  nfs-seg-fault-1.pcap  nfs-seg-fault-1.out
+
+# DNS infinite loop tests
+#
+# See http://marc.info/?l=tcpdump-workers&m=95552439022555
+#
+dns-zlip-1		dns-zlip-1.pcap		dns-zlip-1.out
+dns-zlip-2		dns-zlip-2.pcap		dns-zlip-2.out
+dns-zlip-3		dns-zlip-3.pcap		dns-zlip-3.out
diff --git a/tests/dns-zlip-1.out b/tests/dns-zlip-1.out
new file mode 100644
index 000000000..ce12748da
--- /dev/null
+++ b/tests/dns-zlip-1.out
@@ -0,0 +1 @@
+IP 10.0.0.1.1024 > 146.84.28.88.53: 60777 Type49159 (Class 49168)? <BAD PTR>[|domain]
diff --git a/tests/dns-zlip-1.pcap b/tests/dns-zlip-1.pcap
new file mode 100644
index 000000000..4ec844c0f
Binary files /dev/null and b/tests/dns-zlip-1.pcap differ
diff --git a/tests/dns-zlip-2.out b/tests/dns-zlip-2.out
new file mode 100644
index 000000000..dbabefbfc
--- /dev/null
+++ b/tests/dns-zlip-2.out
@@ -0,0 +1 @@
+IP 10.0.0.1.1024 > 146.84.28.88.53: 18992 Type49164 (Class 49168)? <BAD PTR>[|domain]
diff --git a/tests/dns-zlip-2.pcap b/tests/dns-zlip-2.pcap
new file mode 100644
index 000000000..5f74f2264
Binary files /dev/null and b/tests/dns-zlip-2.pcap differ
diff --git a/tests/dns-zlip-3.out b/tests/dns-zlip-3.out
new file mode 100644
index 000000000..beb173045
--- /dev/null
+++ b/tests/dns-zlip-3.out
@@ -0,0 +1 @@
+IP 10.0.0.1.1024 > 146.84.28.88.53: 65483 Type49164 (Class 49164)? thisleetostringwillcrashyourlittlenameserverforsurehahahahahah.<BAD PTR>[|domain]
diff --git a/tests/dns-zlip-3.pcap b/tests/dns-zlip-3.pcap
new file mode 100644
index 000000000..c0c076a63
Binary files /dev/null and b/tests/dns-zlip-3.pcap differ
diff --git a/tests/dns_fwdptr.out b/tests/dns_fwdptr.out
new file mode 100644
index 000000000..4a37b7fb3
--- /dev/null
+++ b/tests/dns_fwdptr.out
@@ -0,0 +1,2 @@
+be:af:ca:ce:ff:ff > de:ad:be:ef:00:01, ethertype IPv4 (0x0800), length 63207: (tos 0x0, ttl 128, id 36039, offset 0, flags [none], proto UDP (17), length 63193)
+    156.118.17.235.53 > 156.118.27.229.500: [udp sum ok] 51584 zoneRef NoChange*|$ [64259q] q: Type507 (Class 769)? M-{.^AM-{^C.M-{.^AM-{^C.M-{.^AM-{^C.M-{.^AM-{^C.M-{.^AM-{ .M-{^C^A.<BAD PTR>[|domain]
diff --git a/tests/dns_fwdptr.pcap b/tests/dns_fwdptr.pcap
new file mode 100644
index 000000000..67a846119
Binary files /dev/null and b/tests/dns_fwdptr.pcap differ

Vulnerable code: 	int chars_processed;
	int data_size = ndo->ndo_snapend - bp;
	chars_processed = 1;
				cp = bp + (((i << 8) | *cp) & 0x3fff);
				chars_processed++;

				/*
				 * If we've looked at every character in
				 * the message, this pointer will make
				 * us look at some character again,
				 * which means we're looping.
				 */
				if (chars_processed >= data_size) {
					ND_PRINT((ndo, "<LOOP>"));
					return (NULL);
				}
			chars_processed += l;
			chars_processed++;
Fixed code: 	u_int offset, max_offset;
	max_offset = (u_int)(cp - bp);
				offset = (((i << 8) | *cp) & 0x3fff);
				/*
				 * This must move backwards in the packet.
				 * No RFC explicitly says that, but BIND's
				 * name decompression code requires it,
				 * as a way of preventing infinite loops
				 * and other bad behavior, and it's probably
				 * what was intended (compress by pointing
				 * to domain name suffixes already seen in
				 * the packet).
				 */
				if (offset >= max_offset) {
					ND_PRINT((ndo, "<BAD PTR>"));
					return(NULL);
				}
				max_offset = offset;
				cp = bp + offset;
dns_fwdptr		dns_fwdptr.pcap			dns_fwdptr.out		-vvv -e

# DNS infinite loop tests
#
# See http://marc.info/?l=tcpdump-workers&m=95552439022555
#
dns-zlip-1		dns-zlip-1.pcap		dns-zlip-1.out
dns-zlip-2		dns-zlip-2.pcap		dns-zlip-2.out
dns-zlip-3		dns-zlip-3.pcap		dns-zlip-3.out
IP 10.0.0.1.1024 > 146.84.28.88.53: 60777 Type49159 (Class 49168)? <BAD PTR>[|domain]
IP 10.0.0.1.1024 > 146.84.28.88.53: 18992 Type49164 (Class 49168)? <BAD PTR>[|domain]
IP 10.0.0.1.1024 > 146.84.28.88.53: 65483 Type49164 (Class 49164)? thisleetostringwillcrashyourlittlenameserverforsurehahahahahah.<BAD PTR>[|domain]
be:af:ca:ce:ff:ff > de:ad:be:ef:00:01, ethertype IPv4 (0x0800), length 63207: (tos 0x0, ttl 128, id 36039, offset 0, flags [none], proto UDP (17), length 63193)
    156.118.17.235.53 > 156.118.27.229.500: [udp sum ok] 51584 zoneRef NoChange*|$ [64259q] q: Type507 (Class 769)? M-{.^AM-{^C.M-{.^AM-{^C.M-{.^AM-{^C.M-{.^AM-{^C.M-{.^AM-{ .M-{^C^A.<BAD PTR>[|domain]