CVE-2017-13040: Out-of-bounds Read (CWE-125)
value: The MPTCP parser in tcpdump before 4.9.2 has a buffer over-read in print-mptcp.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-mptcp.c, tests/TESTLIST, tests/mptcp-dss-oobr.out
Commit date: 2017-09-13 12:25:44 +0100
Commit message: CVE-2017-13040/MPTCP: Clean up printing DSS suboption.

Do the length checking inline; that means we print stuff up to the point
at which we run out of option data.

First check to make sure we have at least 4 bytes of option, so we have
flags to check.

This fixes a buffer over-read discovered by Kim Gwan Yeong.

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

Diff: diff --git a/print-mptcp.c b/print-mptcp.c
index e757ea48f3..392791e66e 100644
--- a/print-mptcp.c
+++ b/print-mptcp.c
@@ -176,11 +176,11 @@ static int
 mp_capable_print(netdissect_options *ndo,
                  const u_char *opt, u_int opt_len, u_char flags)
 {
         const struct mp_capable *mpc = (const struct mp_capable *) opt;
 
-        if (!(opt_len == 12 && flags & TH_SYN) &&
+        if (!(opt_len == 12 && (flags & TH_SYN)) &&
             !(opt_len == 20 && (flags & (TH_SYN | TH_ACK)) == TH_ACK))
                 return 0;
 
         if (MP_CAPABLE_OPT_VERSION(mpc->sub_ver) != 0) {
                 ND_PRINT((ndo, " Unknown Version (%d)", MP_CAPABLE_OPT_VERSION(mpc->sub_ver)));
@@ -200,13 +200,13 @@ static int
 mp_join_print(netdissect_options *ndo,
               const u_char *opt, u_int opt_len, u_char flags)
 {
         const struct mp_join *mpj = (const struct mp_join *) opt;
 
-        if (!(opt_len == 12 && flags & TH_SYN) &&
+        if (!(opt_len == 12 && (flags & TH_SYN)) &&
             !(opt_len == 16 && (flags & (TH_SYN | TH_ACK)) == (TH_SYN | TH_ACK)) &&
-            !(opt_len == 24 && flags & TH_ACK))
+            !(opt_len == 24 && (flags & TH_ACK)))
                 return 0;
 
         if (opt_len != 24) {
                 if (mpj->sub_b & MP_JOIN_B)
                         ND_PRINT((ndo, " backup"));
@@ -234,80 +234,96 @@ mp_join_print(netdissect_options *ndo,
                 break;
         }
         return 1;
 }
 
-static u_int mp_dss_len(const  struct mp_dss *m, int csum)
-{
-        u_int len;
-
-        len = 4;
-        if (m->flags & MP_DSS_A) {
-                /* Ack present - 4 or 8 octets */
-                len += (m->flags & MP_DSS_a) ? 8 : 4;
-        }
-        if (m->flags & MP_DSS_M) {
-                /*
-                 * Data Sequence Number (DSN), Subflow Sequence Number (SSN),
-                 * Data-Level Length present, and Checksum possibly present.
-                 * All but the Checksum are 10 bytes if the m flag is
-                 * clear (4-byte DSN) and 14 bytes if the m flag is set
-                 * (8-byte DSN).
-                 */
-                len += (m->flags & MP_DSS_m) ? 14 : 10;
-
-                /*
-                 * The Checksum is present only if negotiated.
-                 */
-                if (csum)
-                        len += 2;
-	}
-	return len;
-}
-
 static int
 mp_dss_print(netdissect_options *ndo,
              const u_char *opt, u_int opt_len, u_char flags)
 {
         const struct mp_dss *mdss = (const struct mp_dss *) opt;
 
-        if ((opt_len != mp_dss_len(mdss, 1) &&
-             opt_len != mp_dss_len(mdss, 0)) || flags & TH_SYN)
+        /* We need the flags, at a minimum. */
+        if (opt_len < 4)
+                return 0;
+
+        if (flags & TH_SYN)
                 return 0;
 
         if (mdss->flags & MP_DSS_F)
                 ND_PRINT((ndo, " fin"));
 
         opt += 4;
+        opt_len -= 4;
         if (mdss->flags & MP_DSS_A) {
+                /* Ack present */
                 ND_PRINT((ndo, " ack "));
+                /*
+                 * If the a flag is set, we have an 8-byte ack; if it's
+                 * clear, we have a 4-byte ack.
+                 */
                 if (mdss->flags & MP_DSS_a) {
+                        if (opt_len < 8)
+                                return 0;
                         ND_PRINT((ndo, "%" PRIu64, EXTRACT_64BITS(opt)));
                         opt += 8;
+                        opt_len -= 8;
                 } else {
+                        if (opt_len < 4)
+                                return 0;
                         ND_PRINT((ndo, "%u", EXTRACT_32BITS(opt)));
                         opt += 4;
+                        opt_len -= 4;
                 }
         }
 
         if (mdss->flags & MP_DSS_M) {
+                /*
+                 * Data Sequence Number (DSN), Subflow Sequence Number (SSN),
+                 * Data-Level Length present, and Checksum possibly present.
+                 */
                 ND_PRINT((ndo, " seq "));
+		/*
+                 * If the m flag is set, we have an 8-byte NDS; if it's clear,
+                 * we have a 4-byte DSN.
+                 */
                 if (mdss->flags & MP_DSS_m) {
+                        if (opt_len < 8)
+                                return 0;
                         ND_PRINT((ndo, "%" PRIu64, EXTRACT_64BITS(opt)));
                         opt += 8;
+                        opt_len -= 8;
                 } else {
+                        if (opt_len < 4)
+                                return 0;
                         ND_PRINT((ndo, "%u", EXTRACT_32BITS(opt)));
                         opt += 4;
+                        opt_len -= 4;
                 }
+                if (opt_len < 4)
+                        return 0;
                 ND_PRINT((ndo, " subseq %u", EXTRACT_32BITS(opt)));
                 opt += 4;
+                opt_len -= 4;
+                if (opt_len < 2)
+                        return 0;
                 ND_PRINT((ndo, " len %u", EXTRACT_16BITS(opt)));
                 opt += 2;
+                opt_len -= 2;
 
-                if (opt_len == mp_dss_len(mdss, 1))
+                /*
+                 * The Checksum is present only if negotiated.
+                 * If there are at least 2 bytes left, process the next 2
+                 * bytes as the Checksum.
+                 */
+                if (opt_len >= 2) {
                         ND_PRINT((ndo, " csum 0x%x", EXTRACT_16BITS(opt)));
+                        opt_len -= 2;
+                }
         }
+        if (opt_len != 0)
+                return 0;
         return 1;
 }
 
 static int
 add_addr_print(netdissect_options *ndo,
diff --git a/tests/TESTLIST b/tests/TESTLIST
index a830b0cdaf..c3f397338d 100644
--- a/tests/TESTLIST
+++ b/tests/TESTLIST
@@ -556,10 +556,13 @@ 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
 
+# bad packets from Kim Gwan Yeong
+mptcp-dss-oobr		mptcp-dss-oobr.pcap		mptcp-dss-oobr.out	-v
+
 # 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
 
diff --git a/tests/mptcp-dss-oobr.out b/tests/mptcp-dss-oobr.out
new file mode 100644
index 0000000000..f432e3cc13
--- /dev/null
+++ b/tests/mptcp-dss-oobr.out
@@ -0,0 +1,2 @@
+IP (tos 0x10, ttl 64, id 39991, offset 0, flags [DF], proto TCP (6), length 60)
+    127.0.0.1.57370 > 127.0.0.1.23: Flags [S], seq 1736820995, win 32792, options [mss 16396,sackOK,TS val 597120308 ecr 0,mptcp dss[bad opt]>
diff --git a/tests/mptcp-dss-oobr.pcap b/tests/mptcp-dss-oobr.pcap
new file mode 100644
index 0000000000..6cac7a530f
Binary files /dev/null and b/tests/mptcp-dss-oobr.pcap differ

Vulnerable code:         if (!(opt_len == 12 && flags & TH_SYN) &&
        if (!(opt_len == 12 && flags & TH_SYN) &&
            !(opt_len == 24 && flags & TH_ACK))
static u_int mp_dss_len(const  struct mp_dss *m, int csum)
{
        u_int len;

        len = 4;
        if (m->flags & MP_DSS_A) {
                /* Ack present - 4 or 8 octets */
                len += (m->flags & MP_DSS_a) ? 8 : 4;
        }
        if (m->flags & MP_DSS_M) {
                /*
                 * Data Sequence Number (DSN), Subflow Sequence Number (SSN),
                 * Data-Level Length present, and Checksum possibly present.
                 * All but the Checksum are 10 bytes if the m flag is
                 * clear (4-byte DSN) and 14 bytes if the m flag is set
                 * (8-byte DSN).
                 */
                len += (m->flags & MP_DSS_m) ? 14 : 10;

                /*
                 * The Checksum is present only if negotiated.
                 */
                if (csum)
                        len += 2;
	}
	return len;
}

        if ((opt_len != mp_dss_len(mdss, 1) &&
             opt_len != mp_dss_len(mdss, 0)) || flags & TH_SYN)
                if (opt_len == mp_dss_len(mdss, 1))
Fixed code:         if (!(opt_len == 12 && (flags & TH_SYN)) &&
        if (!(opt_len == 12 && (flags & TH_SYN)) &&
            !(opt_len == 24 && (flags & TH_ACK)))
        /* We need the flags, at a minimum. */
        if (opt_len < 4)
                return 0;

        if (flags & TH_SYN)
        opt_len -= 4;
                /* Ack present */
                /*
                 * If the a flag is set, we have an 8-byte ack; if it's
                 * clear, we have a 4-byte ack.
                 */
                        if (opt_len < 8)
                                return 0;
                        opt_len -= 8;
                        if (opt_len < 4)
                                return 0;
                        opt_len -= 4;
                /*
                 * Data Sequence Number (DSN), Subflow Sequence Number (SSN),
                 * Data-Level Length present, and Checksum possibly present.
                 */
		/*
                 * If the m flag is set, we have an 8-byte NDS; if it's clear,
                 * we have a 4-byte DSN.
                 */
                        if (opt_len < 8)
                                return 0;
                        opt_len -= 8;
                        if (opt_len < 4)
                                return 0;
                        opt_len -= 4;
                if (opt_len < 4)
                        return 0;
                opt_len -= 4;
                if (opt_len < 2)
                        return 0;
                opt_len -= 2;
                /*
                 * The Checksum is present only if negotiated.
                 * If there are at least 2 bytes left, process the next 2
                 * bytes as the Checksum.
                 */
                if (opt_len >= 2) {
                        opt_len -= 2;
                }
        if (opt_len != 0)
                return 0;
# bad packets from Kim Gwan Yeong
mptcp-dss-oobr		mptcp-dss-oobr.pcap		mptcp-dss-oobr.out	-v

IP (tos 0x10, ttl 64, id 39991, offset 0, flags [DF], proto TCP (6), length 60)
    127.0.0.1.57370 > 127.0.0.1.23: Flags [S], seq 1736820995, win 32792, options [mss 16396,sackOK,TS val 597120308 ecr 0,mptcp dss[bad opt]>