CVE-2017-13039: Out-of-bounds Read (CWE-125)
value: The ISAKMP parser in tcpdump before 4.9.2 has a buffer over-read in print-isakmp.c, several functions.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2017-09-14T06:29Z
Repository: https://github.com/the-tcpdump-group/tcpdump
Language: Other
Files changed: print-isakmp.c, tests/TESTLIST, tests/isakmpv1-attr-oobr.out
Commit date: 2017-09-13 12:25:44 +0100
Commit message: CVE-2017-13039/IKEv1: Do more bounds checking.

Have ikev1_attrmap_print() and ikev1_attr_print() do full bounds
checking, and return null on a bounds overflow.  Have their callers
check for a null return.

This fixes a buffer over-read discovered by Bhargava Shastry,
SecT/TU Berlin.

Add a test using the capture file supplied by the reporter(s), modified
so the capture file won't be rejected as an invalid capture.

Diff: diff --git a/print-isakmp.c b/print-isakmp.c
index 9de9b75d21..013de1c283 100644
--- a/print-isakmp.c
+++ b/print-isakmp.c
@@ -910,74 +910,102 @@ struct attrmap {
 	const char *value[30];	/*XXX*/
 };
 
 static const u_char *
 ikev1_attrmap_print(netdissect_options *ndo,
-		    const u_char *p, const u_char *ep,
+		    const u_char *p, const u_char *ep2,
 		    const struct attrmap *map, size_t nmap)
 {
 	int totlen;
 	uint32_t t, v;
 
+	ND_TCHECK(p[0]);
 	if (p[0] & 0x80)
 		totlen = 4;
-	else
+	else {
+		ND_TCHECK_16BITS(&p[2]);
 		totlen = 4 + EXTRACT_16BITS(&p[2]);
-	if (ep < p + totlen) {
+	}
+	if (ep2 < p + totlen) {
 		ND_PRINT((ndo,"[|attr]"));
-		return ep + 1;
+		return ep2 + 1;
 	}
 
+	ND_TCHECK_16BITS(&p[0]);
 	ND_PRINT((ndo,"("));
 	t = EXTRACT_16BITS(&p[0]) & 0x7fff;
 	if (map && t < nmap && map[t].type)
 		ND_PRINT((ndo,"type=%s ", map[t].type));
 	else
 		ND_PRINT((ndo,"type=#%d ", t));
 	if (p[0] & 0x80) {
 		ND_PRINT((ndo,"value="));
+		ND_TCHECK_16BITS(&p[2]);
 		v = EXTRACT_16BITS(&p[2]);
 		if (map && t < nmap && v < map[t].nvalue && map[t].value[v])
 			ND_PRINT((ndo,"%s", map[t].value[v]));
-		else
-			rawprint(ndo, (const uint8_t *)&p[2], 2);
+		else {
+			if (!rawprint(ndo, (const uint8_t *)&p[2], 2)) {
+				ND_PRINT((ndo,")"));
+				goto trunc;
+			}
+		}
 	} else {
-		ND_PRINT((ndo,"len=%d value=", EXTRACT_16BITS(&p[2])));
-		rawprint(ndo, (const uint8_t *)&p[4], EXTRACT_16BITS(&p[2]));
+		ND_PRINT((ndo,"len=%d value=", totlen - 4));
+		if (!rawprint(ndo, (const uint8_t *)&p[4], totlen - 4)) {
+			ND_PRINT((ndo,")"));
+			goto trunc;
+		}
 	}
 	ND_PRINT((ndo,")"));
 	return p + totlen;
+
+trunc:
+	return NULL;
 }
 
 static const u_char *
-ikev1_attr_print(netdissect_options *ndo, const u_char *p, const u_char *ep)
+ikev1_attr_print(netdissect_options *ndo, const u_char *p, const u_char *ep2)
 {
 	int totlen;
 	uint32_t t;
 
+	ND_TCHECK(p[0]);
 	if (p[0] & 0x80)
 		totlen = 4;
-	else
+	else {
+		ND_TCHECK_16BITS(&p[2]);
 		totlen = 4 + EXTRACT_16BITS(&p[2]);
-	if (ep < p + totlen) {
+	}
+	if (ep2 < p + totlen) {
 		ND_PRINT((ndo,"[|attr]"));
-		return ep + 1;
+		return ep2 + 1;
 	}
 
+	ND_TCHECK_16BITS(&p[0]);
 	ND_PRINT((ndo,"("));
 	t = EXTRACT_16BITS(&p[0]) & 0x7fff;
 	ND_PRINT((ndo,"type=#%d ", t));
 	if (p[0] & 0x80) {
 		ND_PRINT((ndo,"value="));
 		t = p[2];
-		rawprint(ndo, (const uint8_t *)&p[2], 2);
+		if (!rawprint(ndo, (const uint8_t *)&p[2], 2)) {
+			ND_PRINT((ndo,")"));
+			goto trunc;
+		}
 	} else {
-		ND_PRINT((ndo,"len=%d value=", EXTRACT_16BITS(&p[2])));
-		rawprint(ndo, (const uint8_t *)&p[4], EXTRACT_16BITS(&p[2]));
+		ND_PRINT((ndo,"len=%d value=", totlen - 4));
+		if (!rawprint(ndo, (const uint8_t *)&p[4], totlen - 4)) {
+			ND_PRINT((ndo,")"));
+			goto trunc;
+		}
 	}
 	ND_PRINT((ndo,")"));
 	return p + totlen;
+
+trunc:
+	return NULL;
 }
 
 static const u_char *
 ikev1_sa_print(netdissect_options *ndo, u_char tpay _U_,
 	       const struct isakmp_gen *ext,
@@ -1254,15 +1282,16 @@ ikev1_t_print(netdissect_options *ndo, u_char tpay _U_,
 	else
 		ND_PRINT((ndo," #%d id=%d ", t.t_no, t.t_id));
 	cp = (const u_char *)(p + 1);
 	ep2 = (const u_char *)p + item_len;
 	while (cp < ep && cp < ep2) {
-		if (map && nmap) {
-			cp = ikev1_attrmap_print(ndo, cp, (ep < ep2) ? ep : ep2,
-				map, nmap);
-		} else
-			cp = ikev1_attr_print(ndo, cp, (ep < ep2) ? ep : ep2);
+		if (map && nmap)
+			cp = ikev1_attrmap_print(ndo, cp, ep2, map, nmap);
+		else
+			cp = ikev1_attr_print(ndo, cp, ep2);
+		if (cp == NULL)
+			goto trunc;
 	}
 	if (ep < ep2)
 		ND_PRINT((ndo,"..."));
 	return cp;
 trunc:
@@ -1722,12 +1751,15 @@ ikev1_n_print(netdissect_options *ndo, u_char tpay _U_,
 		    {
 			const struct attrmap *map = oakley_t_map;
 			size_t nmap = sizeof(oakley_t_map)/sizeof(oakley_t_map[0]);
 			ND_PRINT((ndo," attrs=("));
 			while (cp < ep && cp < ep2) {
-				cp = ikev1_attrmap_print(ndo, cp,
-					(ep < ep2) ? ep : ep2, map, nmap);
+				cp = ikev1_attrmap_print(ndo, cp, ep2, map, nmap);
+				if (cp == NULL) {
+					ND_PRINT((ndo,")"));
+					goto trunc;
+				}
 			}
 			ND_PRINT((ndo,")"));
 			break;
 		    }
 		case IPSECDOI_NTYPE_REPLAY_STATUS:
@@ -1924,14 +1956,15 @@ ikev2_t_print(netdissect_options *ndo, int tcount,
 			  t.t_id));
 	cp = (const u_char *)(p + 1);
 	ep2 = (const u_char *)p + item_len;
 	while (cp < ep && cp < ep2) {
 		if (map && nmap) {
-			cp = ikev1_attrmap_print(ndo, cp, (ep < ep2) ? ep : ep2,
-				map, nmap);
+			cp = ikev1_attrmap_print(ndo, cp, ep2, map, nmap);
 		} else
-			cp = ikev1_attr_print(ndo, cp, (ep < ep2) ? ep : ep2);
+			cp = ikev1_attr_print(ndo, cp, ep2);
+		if (cp == NULL)
+			goto trunc;
 	}
 	if (ep < ep2)
 		ND_PRINT((ndo,"..."));
 	return cp;
 trunc:
diff --git a/tests/TESTLIST b/tests/TESTLIST
index bdc7ff40c6..a830b0cdaf 100644
--- a/tests/TESTLIST
+++ b/tests/TESTLIST
@@ -551,10 +551,11 @@ pim_header_asan-3	pim_header_asan-3.pcap		pim_header_asan-3.out	-v
 pim_header_asan-4	pim_header_asan-4.pcap		pim_header_asan-4.out	-v
 ip6_frag_asan		ip6_frag_asan.pcap		ip6_frag_asan.out	-v
 radius_attr_asan	radius_attr_asan.pcap		radius_attr_asan.out	-v
 ospf6_decode_v3_asan	ospf6_decode_v3_asan.pcap	ospf6_decode_v3_asan.out -v
 ip_ts_opts_asan		ip_ts_opts_asan.pcap		ip_ts_opts_asan.out	-v
+isakmpv1-attr-oobr	isakmpv1-attr-oobr.pcap		isakmpv1-attr-oobr.out	-v
 
 # bad packets from Katie Holly
 mlppp-oobr		mlppp-oobr.pcap			mlppp-oobr.out
 
 # RTP tests
diff --git a/tests/isakmpv1-attr-oobr.out b/tests/isakmpv1-attr-oobr.out
new file mode 100644
index 0000000000..0816735baa
--- /dev/null
+++ b/tests/isakmpv1-attr-oobr.out
@@ -0,0 +1,3 @@
+IP (tos 0x60, ttl 254, id 40192, offset 0, flags [+, DF, rsvd], proto UDP (17), length 63264, options (unknown 255 [bad length 18]), bad cksum 8e30 (->f45)!)
+    251.73.77.150.32514 > 126.172.128.5.500: isakmp 1.0 msgid 2200af01: phase 2/others ? #40:
+    (t: #243 id=241 (type=#9472 len=2 value=0619) [|t]) (len mismatch: isakmp 4293885728/ip 2140)
diff --git a/tests/isakmpv1-attr-oobr.pcap b/tests/isakmpv1-attr-oobr.pcap
new file mode 100644
index 0000000000..53619af434
Binary files /dev/null and b/tests/isakmpv1-attr-oobr.pcap differ

Vulnerable code: 		    const u_char *p, const u_char *ep,
	else
	if (ep < p + totlen) {
		return ep + 1;
		else
			rawprint(ndo, (const uint8_t *)&p[2], 2);
		ND_PRINT((ndo,"len=%d value=", EXTRACT_16BITS(&p[2])));
		rawprint(ndo, (const uint8_t *)&p[4], EXTRACT_16BITS(&p[2]));
ikev1_attr_print(netdissect_options *ndo, const u_char *p, const u_char *ep)
	else
	if (ep < p + totlen) {
		return ep + 1;
		rawprint(ndo, (const uint8_t *)&p[2], 2);
		ND_PRINT((ndo,"len=%d value=", EXTRACT_16BITS(&p[2])));
		rawprint(ndo, (const uint8_t *)&p[4], EXTRACT_16BITS(&p[2]));
		if (map && nmap) {
			cp = ikev1_attrmap_print(ndo, cp, (ep < ep2) ? ep : ep2,
				map, nmap);
		} else
			cp = ikev1_attr_print(ndo, cp, (ep < ep2) ? ep : ep2);
				cp = ikev1_attrmap_print(ndo, cp,
					(ep < ep2) ? ep : ep2, map, nmap);
			cp = ikev1_attrmap_print(ndo, cp, (ep < ep2) ? ep : ep2,
				map, nmap);
			cp = ikev1_attr_print(ndo, cp, (ep < ep2) ? ep : ep2);
Fixed code: 		    const u_char *p, const u_char *ep2,
	ND_TCHECK(p[0]);
	else {
		ND_TCHECK_16BITS(&p[2]);
	}
	if (ep2 < p + totlen) {
		return ep2 + 1;
	ND_TCHECK_16BITS(&p[0]);
		ND_TCHECK_16BITS(&p[2]);
		else {
			if (!rawprint(ndo, (const uint8_t *)&p[2], 2)) {
				ND_PRINT((ndo,")"));
				goto trunc;
			}
		}
		ND_PRINT((ndo,"len=%d value=", totlen - 4));
		if (!rawprint(ndo, (const uint8_t *)&p[4], totlen - 4)) {
			ND_PRINT((ndo,")"));
			goto trunc;
		}

trunc:
	return NULL;
ikev1_attr_print(netdissect_options *ndo, const u_char *p, const u_char *ep2)
	ND_TCHECK(p[0]);
	else {
		ND_TCHECK_16BITS(&p[2]);
	}
	if (ep2 < p + totlen) {
		return ep2 + 1;
	ND_TCHECK_16BITS(&p[0]);
		if (!rawprint(ndo, (const uint8_t *)&p[2], 2)) {
			ND_PRINT((ndo,")"));
			goto trunc;
		}
		ND_PRINT((ndo,"len=%d value=", totlen - 4));
		if (!rawprint(ndo, (const uint8_t *)&p[4], totlen - 4)) {
			ND_PRINT((ndo,")"));
			goto trunc;
		}

trunc:
	return NULL;
		if (map && nmap)
			cp = ikev1_attrmap_print(ndo, cp, ep2, map, nmap);
		else
			cp = ikev1_attr_print(ndo, cp, ep2);
		if (cp == NULL)
			goto trunc;
				cp = ikev1_attrmap_print(ndo, cp, ep2, map, nmap);
				if (cp == NULL) {
					ND_PRINT((ndo,")"));
					goto trunc;
				}
			cp = ikev1_attrmap_print(ndo, cp, ep2, map, nmap);
			cp = ikev1_attr_print(ndo, cp, ep2);
		if (cp == NULL)
			goto trunc;
isakmpv1-attr-oobr	isakmpv1-attr-oobr.pcap		isakmpv1-attr-oobr.out	-v
IP (tos 0x60, ttl 254, id 40192, offset 0, flags [+, DF, rsvd], proto UDP (17), length 63264, options (unknown 255 [bad length 18]), bad cksum 8e30 (->f45)!)
    251.73.77.150.32514 > 126.172.128.5.500: isakmp 1.0 msgid 2200af01: phase 2/others ? #40:
    (t: #243 id=241 (type=#9472 len=2 value=0619) [|t]) (len mismatch: isakmp 4293885728/ip 2140)