OpenFlow 1.0: Improve handling of some lengths.

For OFPT_PACKET_OUT print "actions_len", as it is a part of the message
and should appear in its decoding (in other message types it is derived
from the message length).

ND_ICHECK_*() in of10_actions_print(), of10_flow_stats_reply_print() and
of10_packet_out_print() after printing at least some of the output.
This, compared to just "(invalid) (invalid)", makes it much easier to
understand  where and why the packet data was not fully decoded.  Define
OF_ACTION_MINLEN unsigned to squelch the induced compiler warnings.  A
number of similar checks still remain to be converted the same way.

5 files changed, 53 insertions, 14 deletions

diff --git a/print-openflow-1.0.c b/print-openflow-1.0.c
index 82a4f0ba..d942fd8f 100644
--- a/print-openflow-1.0.c
+++ b/print-openflow-1.0.c
@@ -550,7 +550,7 @@ static const struct uint_tokary of10_ofpet2tokary[] = {
 
 /* lengths (fixed or minimal) of particular protocol structures */
 #define OF_PHY_PORT_FIXLEN                    48
-#define OF_ACTION_MINLEN                       8
+#define OF_ACTION_MINLEN                       8U
 #define OF_MATCH_FIXLEN                       40
 #define OF_DESC_STATS_REPLY_FIXLEN          1056
 #define OF_FLOW_STATS_REQUEST_FIXLEN          44
@@ -1289,12 +1289,12 @@ of10_actions_print(netdissect_options *ndo,
 		uint16_t type, alen, output_port;
 		u_char alen_bogus = 0, skip = 0;
 
-		if (len < OF_ACTION_MINLEN)
-			goto invalid;
+		ND_PRINT("%saction", pfx);
+		ND_ICHECKMSG_U("remaining length", len, <, OF_ACTION_MINLEN);
 		/* type */
 		type = GET_BE_U_2(cp);
 		OF_FWD(2);
-		ND_PRINT("%saction type %s", pfx, tok2str(ofpat_str, "invalid (0x%04x)", type));
+		ND_PRINT(" type %s", tok2str(ofpat_str, "invalid (0x%04x)", type));
 		/* length */
 		alen = GET_BE_U_2(cp);
 		OF_FWD(2);
@@ -1307,8 +1307,8 @@ of10_actions_print(netdissect_options *ndo,
 		 */
 
 		/* On action size underrun/overrun skip the rest of the action list. */
-		if (alen < OF_ACTION_MINLEN || alen > len + 4)
-			goto invalid;
+		ND_ICHECK_U(alen, <, OF_ACTION_MINLEN);
+		ND_ICHECK_U(len, <, alen - 4U);
 		/*
 		 * After validating the basic length constraint it will be safe
 		 * to skip the current action if the action size is not valid
@@ -1717,13 +1717,13 @@ of10_flow_stats_reply_print(netdissect_options *ndo,
 	while (len) {
 		uint16_t entry_len;
 
-		if (len < OF_FLOW_STATS_REPLY_MINLEN)
-			goto invalid;
+		ND_PRINT("\n\t");
+		ND_ICHECKMSG_U("remaining length", len, <, OF_FLOW_STATS_REPLY_MINLEN);
 		/* length */
 		entry_len = GET_BE_U_2(cp);
-		ND_PRINT("\n\t length %u", entry_len);
-		if (entry_len < OF_FLOW_STATS_REPLY_MINLEN || entry_len > len)
-			goto invalid;
+		ND_PRINT(" length %u", entry_len);
+		ND_ICHECK_U(entry_len, <, OF_FLOW_STATS_REPLY_MINLEN);
+		ND_ICHECK_U(len, <, entry_len);
 		OF_FWD(2);
 		/* table_id */
 		ND_PRINT(", table_id %s",
@@ -1985,9 +1985,9 @@ of10_packet_out_print(netdissect_options *ndo,
 	OF_FWD(2);
 	/* actions_len */
 	actions_len = GET_BE_U_2(cp);
+	ND_PRINT(", actions_len %u", actions_len);
 	OF_FWD(2);
-	if (actions_len > len)
-		goto invalid;
+	ND_ICHECK_U(len, <, actions_len);
 	/* actions */
 	of10_actions_print(ndo, "\n\t ", cp, actions_len);
 	OF_FWD(actions_len);
diff --git a/tests/TESTLIST b/tests/TESTLIST
index 1e741d46..16a6fc1c 100644
--- a/tests/TESTLIST
+++ b/tests/TESTLIST
@@ -288,6 +288,7 @@ of10_7050sx_bsn-oobr of10_7050sx_bsn-oobr.pcap of10_7050sx_bsn-oobr.out -v
 of13_ericsson		of13_ericsson.pcapng	of13_ericsson.out
 of13_ericsson-v		of13_ericsson.pcapng	of13_ericsson-v.out	-v
 of13_ericsson-vv	of13_ericsson.pcapng	of13_ericsson-vv.out	-vv
+of10_inv_OFPST_FLOW-v	of10_inv_OFPST_FLOW.pcap	of10_inv_OFPST_FLOW-v.out -v
 
 # GeoNetworking and CALM FAST tests
 geonet-calm-fast	geonet_and_calm_fast.pcap	geonet_and_calm_fast.out	-vv
diff --git a/tests/of10_inv_OFPST_FLOW-v.out b/tests/of10_inv_OFPST_FLOW-v.out
new file mode 100644
index 00000000..ed76144d
--- /dev/null
+++ b/tests/of10_inv_OFPST_FLOW-v.out
@@ -0,0 +1,38 @@
+    1  04:47:19.498417 IP (tos 0x0, ttl 64, id 60713, offset 0, flags [DF], proto TCP (6), length 562, bad cksum 11cf (->28df)!)
+    109.74.179.168.6653 > 88.150.169.52.4756: Flags [P.], seq 3053183029:3053183539, ack 3441835434, win 59369, options [eol], length 510: OpenFlow
+	version 1.0, type STATS_REPLY, length 128, xid 0xffff0100
+	 type FLOW, flags 0x003e (bogus)
+	 length 92, table_id 30
+	  wildcards 0x00f5ffff (bogus)
+	  match nw_dst 164.164.164.164/9
+	  duration_sec 0, duration_nsec 0, priority 64000, idle_timeout 150, hard_timeout 43316, cookie 0x9f9f9f9f3d000100, packet_count 72057770400022750, byte_count 42407287746007700
+	  action [remaining length 4 < 8] (invalid)
+	 [remaining length 24 < 88] (invalid)
+	version 1.0, type STATS_REPLY, length 128, xid 0xff800000
+	 type FLOW, flags 0x003e (bogus)
+	 length 92, table_id 30
+	  wildcards 0xe9f4ffff (bogus)
+	  match nw_dst 0.127.173.0/13
+	  duration_sec 2341208104, duration_nsec 168053998, priority 61547, idle_timeout 2308, hard_timeout 19694, cookie 0x4ab3a85896a93419, packet_count 18235801350512325921, byte_count 2755546014170999099
+	  action [remaining length 4 < 8] (invalid)
+	 [remaining length 24 < 88] (invalid)
+	version unknown (0x5c), type unknown (0x1e), length 56297, xid 0xf4ffff04 [|openflow]
+    2  05:25:04.505352 IP (tos 0x0, ttl 64, id 60713, offset 0, flags [DF], proto TCP (6), length 562, bad cksum 11cf (->28df)!)
+    109.74.179.168.6653 > 88.150.169.52.4756: Flags [P.], seq 0:510, ack 1627389953, win 59369, options [eol], length 510: OpenFlow
+	version 1.0, type STATS_REPLY, length 128, xid 0xffff0100
+	 type FLOW, flags 0x003e (bogus)
+	 length 92, table_id 30
+	  wildcards 0xe9f4ffff (bogus)
+	  match nw_dst 0.0.255.255/13
+	  duration_sec 65517, duration_nsec 0, priority 64000, idle_timeout 150, hard_timeout 43316, cookie 0xe0352e263d000100, packet_count 72057770400022750, byte_count 42407287746007700
+	  action [remaining length 4 < 8] (invalid)
+	 [remaining length 24 < 88] (invalid)
+	version 1.0, type STATS_REPLY, length 128, xid 0xff800000
+	 type FLOW, flags 0x003e (bogus)
+	 length 92, table_id 30
+	  wildcards 0xe9f4ffff (bogus)
+	  match nw_dst 0.127.173.0/13
+	  duration_sec 2341208104, duration_nsec 168053998, priority 61547, idle_timeout 2308, hard_timeout 19694, cookie 0x0000200b30a10200, packet_count 288230376151711744, byte_count 0
+	  action [remaining length 4 < 8] (invalid)
+	 [remaining length 24 < 88] (invalid)
+	version 1.1, type unknown (0xea), length 0 (too short!), xid 0x000c8c00 (invalid) [|openflow]
diff --git a/tests/of10_inv_OFPST_FLOW.pcap b/tests/of10_inv_OFPST_FLOW.pcap
new file mode 100644
index 00000000..4191f80a
--- /dev/null
+++ b/tests/of10_inv_OFPST_FLOW.pcap
diff --git a/tests/of10_s4810-vvvv.out b/tests/of10_s4810-vvvv.out
index d1bd2885..508075db 100644
--- a/tests/of10_s4810-vvvv.out
+++ b/tests/of10_s4810-vvvv.out
@@ -1232,7 +1232,7 @@ STP 802.1s, Rapid STP, CIST Flags [Proposal, Learn, Forward, Agreement], length
   110  12:51:40.431060 IP (tos 0x0, ttl 64, id 53123, offset 0, flags [DF], proto TCP (6), length 144)
     10.0.0.20.6633 > 10.0.0.81.56068: Flags [P.], cksum 0x14e7 (incorrect -> 0x2671), seq 4729:4821, ack 14734, win 331, options [nop,nop,TS val 47837403 ecr 3], length 92: OpenFlow
 	version 1.0, type PACKET_OUT, length 84, xid 0x00000044
-	 buffer_id 0xffffffff, in_port CONTROLLER
+	 buffer_id 0xffffffff, in_port CONTROLLER, actions_len 8
 	 action type OUTPUT, len 8, port 1
 	 data (60 octets), frame decoding below
  [|llc]