From: Denis Ovsienko <denis@ovsienko.info>
Date: Wed, 23 Sep 2020 10:16:59 +0000 (+0100)
Subject: OpenFlow 1.0: Fix queue properties decoding.
X-Git-Tag: tcpdump-4.99-bp~198
X-Git-Url: https://git.tcpdump.org/tcpdump/commitdiff_plain/07ee811374b9ec7e4c152d12fdc2eaf483555215

OpenFlow 1.0: Fix queue properties decoding.

In of10_queue_props_print() the "skip" branch tested and skipped 4 bytes
too many, so a malformed queue property would likely result in an odd
truncation or incorrect decoding of subsequent data (this is based on
code analysis only).

Get the offset and the size right and add a comment to explain it. Add
and update other comments to clarify a few similar subtleties that are
easy to get wrong.
---

diff --git a/print-openflow-1.0.c b/print-openflow-1.0.c
index fc6acab0..d4f7ef6b 100644
--- a/print-openflow-1.0.c
+++ b/print-openflow-1.0.c
@@ -1231,8 +1231,12 @@ of10_queue_props_print(netdissect_options *ndo,
 			skip = 1;
 		}
 		if (skip) {
-			ND_TCHECK_LEN(cp, plen - 4);
-			cp += plen - 4;
+			/*
+			 * plen >= OF_QUEUE_PROP_HEADER_LEN
+			 * cp is OF_QUEUE_PROP_HEADER_LEN bytes in
+			 */
+			ND_TCHECK_LEN(cp, plen - OF_QUEUE_PROP_HEADER_LEN);
+			cp += plen - OF_QUEUE_PROP_HEADER_LEN;
 			goto next_property;
 		}
 		if (property == OFPQT_MIN_RATE) { /* the only case of property decoding */
@@ -1424,11 +1428,21 @@ of10_actions_print(netdissect_options *ndo,
 		alen = GET_BE_U_2(cp);
 		cp += 2;
 		ND_PRINT(", len %u", alen);
+		/*
+		 * The 4-byte "pad" in the specification is not a field of the
+		 * action header, but a placeholder to illustrate the 64-bit
+		 * alignment requirement. Action type specific case blocks
+		 * below fetch these 4 bytes.
+		 */
+
 		/* On action size underrun/overrun skip the rest of the action list. */
 		if (alen < OF_ACTION_HEADER_LEN || alen > len)
 			goto invalid;
-		/* On action size inappropriate for the given type or invalid type just skip
-		 * the current action, as the basic length constraint has been met. */
+		/*
+		 * After validating the basic length constraint it will be safe
+		 * to skip the current action if the action size is not valid
+		 * for the type or the type is invalid.
+		 */
 		switch (type) {
 		case OFPAT_OUTPUT:
 		case OFPAT_SET_VLAN_VID:
@@ -1457,6 +1471,10 @@ of10_actions_print(netdissect_options *ndo,
 			skip = 1;
 		}
 		if (skip) {
+			/*
+			 * alen >= OF_ACTION_HEADER_LEN
+			 * cp is 4 bytes in
+			 */
 			ND_TCHECK_LEN(cp, alen - 4);
 			cp += alen - 4;
 			goto next_action;
diff --git a/print-openflow.c b/print-openflow.c
index 894306d1..c82e1368 100644
--- a/print-openflow.c
+++ b/print-openflow.c
@@ -99,7 +99,13 @@ of_header_body_print(netdissect_options *ndo, const u_char *cp, const u_char *ep
 	 * the basic header. A message length underrun fails decoding of the rest of
 	 * the current packet. At the same time, try decoding as much of the current
 	 * message as possible even when it does not end within the current TCP
-	 * segment. */
+	 * segment.
+	 *
+	 * That is, do NOT require the header "length" to be small enough for
+	 * the full declared OpenFlow message to fit into the remainder of the
+	 * declared TCP segment given to this function, same as the full declared
+	 * TCP segment is not required to fit into the captured packet buffer.
+	 */
 	if (length < OF_HEADER_LEN) {
 		of_header_print(ndo, version, type, length, xid);
 		goto invalid;