CVE-2017-13051: Out-of-bounds Read (CWE-125)
value: The RSVP parser in tcpdump before 4.9.2 has a buffer over-read in print-rsvp.c:rsvp_obj_print().
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-rsvp.c, tests/TESTLIST, tests/rsvp_uni-oobr-1.out, tests/rsvp_uni-oobr-2.out, tests/rsvp_uni-oobr-3.out
Commit date: 2017-09-13 12:25:44 +0100
Commit message: CVE-2017-13051/RSVP: fix bounds checks for UNI

Fixup the part of rsvp_obj_print() that decodes the GENERALIZED_UNI
object from RFC 3476 Section 3.1 to check the sub-objects inside that
object more thoroughly.

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).

Diff: diff --git a/print-rsvp.c b/print-rsvp.c
index bc599796a8..b0fb344694 100644
--- a/print-rsvp.c
+++ b/print-rsvp.c
@@ -1203,10 +1203,21 @@ rsvp_obj_print(netdissect_options *ndo,
                     return-1;
 
 		/* read variable length subobjects */
 		total_subobj_len = obj_tlen;
                 while(total_subobj_len > 0) {
+                    /* If RFC 3476 Section 3.1 defined that a sub-object of the
+                     * GENERALIZED_UNI RSVP object must have the Length field as
+                     * a multiple of 4, instead of the check below it would be
+                     * better to test total_subobj_len only once before the loop.
+                     * So long as it does not define it and this while loop does
+                     * not implement such a requirement, let's accept that within
+                     * each iteration subobj_len may happen to be a multiple of 1
+                     * and test it and total_subobj_len respectively.
+                     */
+                    if (total_subobj_len < 4)
+                        goto invalid;
                     subobj_len  = EXTRACT_16BITS(obj_tptr);
                     subobj_type = (EXTRACT_16BITS(obj_tptr+2))>>8;
                     af = (EXTRACT_16BITS(obj_tptr+2))&0x00FF;
 
                     ND_PRINT((ndo, "%s  Subobject Type: %s (%u), AF: %s (%u), length: %u",
@@ -1214,11 +1225,17 @@ rsvp_obj_print(netdissect_options *ndo,
                            tok2str(rsvp_obj_generalized_uni_values, "Unknown", subobj_type),
                            subobj_type,
                            tok2str(af_values, "Unknown", af), af,
                            subobj_len));
 
-                    if(subobj_len == 0)
+                    /* In addition to what is explained above, the same spec does not
+                     * explicitly say that the same Length field includes the 4-octet
+                     * sub-object header, but as long as this while loop implements it
+                     * as it does include, let's keep the check below consistent with
+                     * the rest of the code.
+                     */
+                    if(subobj_len < 4 || subobj_len > total_subobj_len)
                         goto invalid;
 
                     switch(subobj_type) {
                     case RSVP_GEN_UNI_SUBOBJ_SOURCE_TNA_ADDRESS:
                     case RSVP_GEN_UNI_SUBOBJ_DESTINATION_TNA_ADDRESS:
diff --git a/tests/TESTLIST b/tests/TESTLIST
index 62e705bf38..1681849a06 100644
--- a/tests/TESTLIST
+++ b/tests/TESTLIST
@@ -565,10 +565,13 @@ hncp_dhcpv4data-oobr	hncp_dhcpv4data-oobr.pcap	hncp_dhcpv4data-oobr.out -v -c1
 vqp-oobr		vqp-oobr.pcap			vqp-oobr.out		-v -c1
 bgp_pmsi_tunnel-oobr	bgp_pmsi_tunnel-oobr.pcap	bgp_pmsi_tunnel-oobr.out -v -c1
 bgp_mvpn_6_and_7	bgp_mvpn_6_and_7.pcap		bgp_mvpn_6_and_7.out	-v -c1
 rsvp_fast_reroute-oobr	rsvp_fast_reroute-oobr.pcap	rsvp_fast_reroute-oobr.out -v -c1
 esis_opt_prot-oobr	esis_opt_prot-oobr.pcap		esis_opt_prot-oobr.out	-v -c1
+rsvp_uni-oobr-1	rsvp_uni-oobr-1.pcap	rsvp_uni-oobr-1.out	-v -c1
+rsvp_uni-oobr-2	rsvp_uni-oobr-2.pcap	rsvp_uni-oobr-2.out	-v -c1
+rsvp_uni-oobr-3	rsvp_uni-oobr-3.pcap	rsvp_uni-oobr-3.out	-v -c3
 
 # bad packets from Katie Holly
 mlppp-oobr		mlppp-oobr.pcap			mlppp-oobr.out
 
 # bad packets from Kim Gwan Yeong
diff --git a/tests/rsvp_uni-oobr-1.out b/tests/rsvp_uni-oobr-1.out
new file mode 100644
index 0000000000..f5da6e7e9b
--- /dev/null
+++ b/tests/rsvp_uni-oobr-1.out
@@ -0,0 +1,5 @@
+IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3743 (->7e72)!)
+    54.35.0.0 > 58.16.0.0: 
+	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
+	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
+	    Subobject Type: Unknown (127), AF: HDLC (4), length: 2 (invalid)
diff --git a/tests/rsvp_uni-oobr-1.pcap b/tests/rsvp_uni-oobr-1.pcap
new file mode 100644
index 0000000000..16d20240f6
Binary files /dev/null and b/tests/rsvp_uni-oobr-1.pcap differ
diff --git a/tests/rsvp_uni-oobr-2.out b/tests/rsvp_uni-oobr-2.out
new file mode 100644
index 0000000000..cc41acd13e
--- /dev/null
+++ b/tests/rsvp_uni-oobr-2.out
@@ -0,0 +1,5 @@
+IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3743 (->3051)!)
+    54.35.78.33 > 58.16.0.0: 
+	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
+	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
+	    Subobject Type: Unknown (0), AF: HDLC (4), length: 2 (invalid)
diff --git a/tests/rsvp_uni-oobr-2.pcap b/tests/rsvp_uni-oobr-2.pcap
new file mode 100644
index 0000000000..c9265ce93f
Binary files /dev/null and b/tests/rsvp_uni-oobr-2.pcap differ
diff --git a/tests/rsvp_uni-oobr-3.out b/tests/rsvp_uni-oobr-3.out
new file mode 100644
index 0000000000..3afa86e7cb
--- /dev/null
+++ b/tests/rsvp_uni-oobr-3.out
@@ -0,0 +1,12 @@
+IP (tos 0x0, ttl 48, id 25615, offset 0, flags [+, DF, rsvd], proto UDP (17), length 61735, bad cksum 8ef1 (->10e1)!)
+    1.2.3.3.1812 > 64.112.0.96.4567:  wb-29!
+IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3701 (->8972)!)
+    54.35.0.0 > 47.16.0.0: 
+	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
+	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
+	    Subobject Type: Unknown (0), AF: HDLC (4), length: 1 (invalid)
+IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3701 (->7e72)!)
+    54.35.0.0 > 58.16.0.0: 
+	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
+	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
+	    Subobject Type: Unknown (225), AF: HDLC (4), length: 1 (invalid)
diff --git a/tests/rsvp_uni-oobr-3.pcap b/tests/rsvp_uni-oobr-3.pcap
new file mode 100644
index 0000000000..0006a4f9a9
Binary files /dev/null and b/tests/rsvp_uni-oobr-3.pcap differ

Vulnerable code:                     if(subobj_len == 0)
Fixed code:                     /* If RFC 3476 Section 3.1 defined that a sub-object of the
                     * GENERALIZED_UNI RSVP object must have the Length field as
                     * a multiple of 4, instead of the check below it would be
                     * better to test total_subobj_len only once before the loop.
                     * So long as it does not define it and this while loop does
                     * not implement such a requirement, let's accept that within
                     * each iteration subobj_len may happen to be a multiple of 1
                     * and test it and total_subobj_len respectively.
                     */
                    if (total_subobj_len < 4)
                        goto invalid;
                    /* In addition to what is explained above, the same spec does not
                     * explicitly say that the same Length field includes the 4-octet
                     * sub-object header, but as long as this while loop implements it
                     * as it does include, let's keep the check below consistent with
                     * the rest of the code.
                     */
                    if(subobj_len < 4 || subobj_len > total_subobj_len)
rsvp_uni-oobr-1	rsvp_uni-oobr-1.pcap	rsvp_uni-oobr-1.out	-v -c1
rsvp_uni-oobr-2	rsvp_uni-oobr-2.pcap	rsvp_uni-oobr-2.out	-v -c1
rsvp_uni-oobr-3	rsvp_uni-oobr-3.pcap	rsvp_uni-oobr-3.out	-v -c3
IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3743 (->7e72)!)
    54.35.0.0 > 58.16.0.0: 
	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
	    Subobject Type: Unknown (127), AF: HDLC (4), length: 2 (invalid)
IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3743 (->3051)!)
    54.35.78.33 > 58.16.0.0: 
	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
	    Subobject Type: Unknown (0), AF: HDLC (4), length: 2 (invalid)
IP (tos 0x0, ttl 48, id 25615, offset 0, flags [+, DF, rsvd], proto UDP (17), length 61735, bad cksum 8ef1 (->10e1)!)
    1.2.3.3.1812 > 64.112.0.96.4567:  wb-29!
IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3701 (->8972)!)
    54.35.0.0 > 47.16.0.0: 
	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
	    Subobject Type: Unknown (0), AF: HDLC (4), length: 1 (invalid)
IP (tos 0x2,ECT(0), ttl 248, id 0, offset 0, flags [none], proto RSVP (46), length 54312, bad cksum 3701 (->7e72)!)
    54.35.0.0 > 58.16.0.0: 
	RSVPv1 Hello Message (20), Flags: [Refresh reduction capable], length: 65527, ttl: 15, checksum: 0x0902
	  Generalized UNI Object (229) Flags: [ignore and forward if unknown], Class-Type: 1 (1), length: 12
	    Subobject Type: Unknown (225), AF: HDLC (4), length: 1 (invalid)