From: Denis Ovsienko Date: Wed, 5 Jan 2022 22:57:45 +0000 (+0000) Subject: OpenFlow: Refine more length checks. X-Git-Url: https://git.tcpdump.org/tcpdump/commitdiff_plain/beddcbb4ea5406ae22818a6cc7da7b315d04eb5e OpenFlow: Refine more length checks. In print-openflow-1.0.c and print-openflow-1.3.c replace the remaining "goto invalid" checks with ND_ICHECK_U() and ND_ICHECKMSG_U(). Redo the changes from commit 1ce16ea: use the same order of arguments and the same comparison operators as before the change and lose the unsigned compensation, which is no longer required. Add another test case using a malformed packet from Francois-Xavier. --- diff --git a/print-openflow-1.0.c b/print-openflow-1.0.c index d942fd8f..00c4fdd3 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 8U +#define OF_ACTION_MINLEN 8 #define OF_MATCH_FIXLEN 40 #define OF_DESC_STATS_REPLY_FIXLEN 1056 #define OF_FLOW_STATS_REQUEST_FIXLEN 44 @@ -699,12 +699,12 @@ of10_bsn_message_print(netdissect_options *ndo, { uint32_t subtype; - if (len < 4) - goto invalid; + ND_PRINT("\n\t"); + ND_ICHECK_U(len, <, 4); /* subtype */ subtype = GET_BE_U_4(cp); OF_FWD(4); - ND_PRINT("\n\t subtype %s", tok2str(bsn_subtype_str, "unknown (0x%08x)", subtype)); + ND_PRINT(" subtype %s", tok2str(bsn_subtype_str, "unknown (0x%08x)", subtype)); switch (subtype) { case BSN_GET_IP_MASK_REQUEST: /* @@ -719,8 +719,7 @@ of10_bsn_message_print(netdissect_options *ndo, * +---------------+---------------+---------------+---------------+ * */ - if (len != 8) - goto invalid; + ND_ICHECK_U(len, !=, 8); /* index */ ND_PRINT(", index %u", GET_U_1(cp)); OF_FWD(1); @@ -742,8 +741,7 @@ of10_bsn_message_print(netdissect_options *ndo, * +---------------+---------------+---------------+---------------+ * */ - if (len != 8) - goto invalid; + ND_ICHECK_U(len, !=, 8); /* index */ ND_PRINT(", index %u", GET_U_1(cp)); OF_FWD(1); @@ -765,8 +763,7 @@ of10_bsn_message_print(netdissect_options *ndo, * +---------------+---------------+---------------+---------------+ * */ - if (len != 4) - goto invalid; + ND_ICHECK_U(len, !=, 4); /* report_mirror_ports */ ND_PRINT(", report_mirror_ports %s", tok2str(bsn_onoff_str, "bogus (%u)", GET_U_1(cp))); @@ -788,8 +785,7 @@ of10_bsn_message_print(netdissect_options *ndo, * +---------------+---------------+---------------+---------------+ * */ - if (len) - goto invalid; + ND_ICHECK_U(len, !=, 0); break; case BSN_VIRTUAL_PORT_REMOVE_REQUEST: /* @@ -802,8 +798,7 @@ of10_bsn_message_print(netdissect_options *ndo, * +---------------+---------------+---------------+---------------+ * */ - if (len != 4) - goto invalid; + ND_ICHECK_U(len, !=, 4); /* vport_no */ ND_PRINT(", vport_no %u", GET_BE_U_4(cp)); break; @@ -820,8 +815,7 @@ of10_bsn_message_print(netdissect_options *ndo, * +---------------+---------------+-------- * */ - if (len < 4) - goto invalid; + ND_ICHECK_U(len, <, 4); /* service */ ND_PRINT(", service %u", GET_BE_U_4(cp)); OF_FWD(4); @@ -858,8 +852,7 @@ of10_bsn_message_print(netdissect_options *ndo, * +---------------+---------------+---------------+---------------+ * */ - if (len != 4) - goto invalid; + ND_ICHECK_U(len, !=, 4); /* status */ ND_PRINT(", status 0x%08x", GET_BE_U_4(cp)); break; @@ -879,12 +872,12 @@ of10_bsn_actions_print(netdissect_options *ndo, { uint32_t subtype, vlan_tag; - if (len < 4) - goto invalid; + ND_PRINT("\n\t "); + ND_ICHECK_U(len, <, 4); /* subtype */ subtype = GET_BE_U_4(cp); OF_FWD(4); - ND_PRINT("\n\t subtype %s", tok2str(bsn_action_subtype_str, "unknown (0x%08x)", subtype)); + ND_PRINT(" subtype %s", tok2str(bsn_action_subtype_str, "unknown (0x%08x)", subtype)); switch (subtype) { case BSN_ACTION_MIRROR: /* @@ -901,8 +894,7 @@ of10_bsn_actions_print(netdissect_options *ndo, * +---------------+---------------+---------------+---------------+ * */ - if (len != 12) - goto invalid; + ND_ICHECK_U(len, !=, 12); /* dest_port */ ND_PRINT(", dest_port %u", GET_BE_U_4(cp)); OF_FWD(4); @@ -944,8 +936,7 @@ of10_vendor_action_print(netdissect_options *ndo, uint32_t vendor; void (*decoder)(netdissect_options *, const u_char *, u_int); - if (len < 4) - goto invalid; + ND_ICHECK_U(len, <, 4); /* vendor */ vendor = GET_BE_U_4(cp); OF_FWD(4); @@ -988,8 +979,7 @@ of10_vendor_data_print(netdissect_options *ndo, { uint32_t vendor; - if (len < 4) - goto invalid; + ND_ICHECK_U(len, <, 4); /* vendor */ vendor = GET_BE_U_4(cp); OF_FWD(4); @@ -1098,18 +1088,18 @@ of10_queue_props_print(netdissect_options *ndo, uint16_t property, plen; u_char plen_bogus = 0, skip = 0; - if (len < OF_QUEUE_PROP_MINLEN) - goto invalid; + ND_PRINT("\n\t "); + ND_ICHECKMSG_U("remaining length", len, <, OF_QUEUE_PROP_MINLEN); /* property */ property = GET_BE_U_2(cp); OF_FWD(2); - ND_PRINT("\n\t property %s", tok2str(ofpqt_str, "invalid (0x%04x)", property)); + ND_PRINT(" property %s", tok2str(ofpqt_str, "invalid (0x%04x)", property)); /* len */ plen = GET_BE_U_2(cp); OF_FWD(2); ND_PRINT(", len %u", plen); - if (plen < OF_QUEUE_PROP_MINLEN || plen > len + 4) - goto invalid; + ND_ICHECKMSG_U("property length", plen, <, OF_QUEUE_PROP_MINLEN); + ND_ICHECKMSG_U("property length", plen, >, len + 4); /* pad */ /* Sometimes the last field, check bounds. */ OF_CHK_FWD(4); @@ -1164,17 +1154,17 @@ of10_queues_print(netdissect_options *ndo, while (len) { uint16_t desclen; - if (len < OF_PACKET_QUEUE_MINLEN) - goto invalid; + ND_PRINT("\n\t "); + ND_ICHECKMSG_U("remaining length", len, <, OF_PACKET_QUEUE_MINLEN); /* queue_id */ - ND_PRINT("\n\t queue_id %u", GET_BE_U_4(cp)); + ND_PRINT(" queue_id %u", GET_BE_U_4(cp)); OF_FWD(4); /* len */ desclen = GET_BE_U_2(cp); OF_FWD(2); ND_PRINT(", len %u", desclen); - if (desclen < OF_PACKET_QUEUE_MINLEN || desclen > len + 6) - goto invalid; + ND_ICHECKMSG_U("prop. desc. length", desclen, <, OF_PACKET_QUEUE_MINLEN); + ND_ICHECKMSG_U("prop. desc. length", desclen, >, len + 6); /* pad */ /* Sometimes the last field, check bounds. */ OF_CHK_FWD(2); @@ -1308,7 +1298,7 @@ of10_actions_print(netdissect_options *ndo, /* On action size underrun/overrun skip the rest of the action list. */ ND_ICHECK_U(alen, <, OF_ACTION_MINLEN); - ND_ICHECK_U(len, <, alen - 4U); + ND_ICHECK_U(alen, >, len + 4); /* * After validating the basic length constraint it will be safe * to skip the current action if the action size is not valid @@ -1467,8 +1457,7 @@ of10_features_reply_print(netdissect_options *ndo, OF_FWD(4); /* ports */ while (len) { - if (len < OF_PHY_PORT_FIXLEN) - goto invalid; + ND_ICHECKMSG_U("\n\t port def. length", len, <, OF_PHY_PORT_FIXLEN); of10_phy_port_print(ndo, cp); OF_FWD(OF_PHY_PORT_FIXLEN); } @@ -1616,13 +1605,11 @@ of10_stats_request_print(netdissect_options *ndo, switch(type) { case OFPST_DESC: case OFPST_TABLE: - if (len) - goto invalid; + ND_ICHECK_U(len, !=, 0); return; case OFPST_FLOW: case OFPST_AGGREGATE: - if (len != OF_FLOW_STATS_REQUEST_FIXLEN) - goto invalid; + ND_ICHECK_U(len, !=, OF_FLOW_STATS_REQUEST_FIXLEN); /* match */ of10_match_print(ndo, "\n\t ", cp); OF_FWD(OF_MATCH_FIXLEN); @@ -1637,8 +1624,7 @@ of10_stats_request_print(netdissect_options *ndo, tok2str(ofpp_str, "%u", GET_BE_U_2(cp))); return; case OFPST_PORT: - if (len != OF_PORT_STATS_REQUEST_FIXLEN) - goto invalid; + ND_ICHECK_U(len, !=, OF_PORT_STATS_REQUEST_FIXLEN); /* port_no */ ND_PRINT("\n\t port_no %s", tok2str(ofpp_str, "%u", GET_BE_U_2(cp))); @@ -1648,8 +1634,7 @@ of10_stats_request_print(netdissect_options *ndo, OF_CHK_FWD(6); return; case OFPST_QUEUE: - if (len != OF_QUEUE_STATS_REQUEST_FIXLEN) - goto invalid; + ND_ICHECK_U(len, !=, OF_QUEUE_STATS_REQUEST_FIXLEN); /* port_no */ ND_PRINT("\n\t port_no %s", tok2str(ofpp_str, "%u", GET_BE_U_2(cp))); @@ -1676,10 +1661,10 @@ static void of10_desc_stats_reply_print(netdissect_options *ndo, const u_char *cp, u_int len) { - if (len != OF_DESC_STATS_REPLY_FIXLEN) - goto invalid; + ND_PRINT("\n\t "); + ND_ICHECK_U(len, !=, OF_DESC_STATS_REPLY_FIXLEN); /* mfr_desc */ - ND_PRINT("\n\t mfr_desc '"); + ND_PRINT(" mfr_desc '"); nd_printjnp(ndo, cp, DESC_STR_LEN); ND_PRINT("'"); OF_FWD(DESC_STR_LEN); @@ -1723,7 +1708,7 @@ of10_flow_stats_reply_print(netdissect_options *ndo, entry_len = GET_BE_U_2(cp); ND_PRINT(" length %u", entry_len); ND_ICHECK_U(entry_len, <, OF_FLOW_STATS_REPLY_MINLEN); - ND_ICHECK_U(len, <, entry_len); + ND_ICHECK_U(entry_len, >, len); OF_FWD(2); /* table_id */ ND_PRINT(", table_id %s", @@ -1776,10 +1761,10 @@ static void of10_aggregate_stats_reply_print(netdissect_options *ndo, const u_char *cp, u_int len) { - if (len != OF_AGGREGATE_STATS_REPLY_FIXLEN) - goto invalid; + ND_PRINT("\n\t"); + ND_ICHECKMSG_U("remaining length", len, !=, OF_AGGREGATE_STATS_REPLY_FIXLEN); /* packet_count */ - ND_PRINT("\n\t packet_count %" PRIu64, GET_BE_U_8(cp)); + ND_PRINT(" packet_count %" PRIu64, GET_BE_U_8(cp)); OF_FWD(8); /* byte_count */ ND_PRINT(", byte_count %" PRIu64, GET_BE_U_8(cp)); @@ -1803,10 +1788,10 @@ of10_table_stats_reply_print(netdissect_options *ndo, const u_char *cp, u_int len) { while (len) { - if (len < OF_TABLE_STATS_REPLY_FIXLEN) - goto invalid; + ND_PRINT("\n\t"); + ND_ICHECKMSG_U("remaining length", len, <, OF_TABLE_STATS_REPLY_FIXLEN); /* table_id */ - ND_PRINT("\n\t table_id %s", + ND_PRINT(" table_id %s", tok2str(tableid_str, "%u", GET_U_1(cp))); OF_FWD(1); /* pad */ @@ -1846,10 +1831,10 @@ of10_port_stats_reply_print(netdissect_options *ndo, const u_char *cp, u_int len) { while (len) { - if (len < OF_PORT_STATS_REPLY_FIXLEN) - goto invalid; + ND_PRINT("\n\t "); + ND_ICHECKMSG_U("remaining length", len, <, OF_PORT_STATS_REPLY_FIXLEN); /* port_no */ - ND_PRINT("\n\t port_no %s", + ND_PRINT(" port_no %s", tok2str(ofpp_str, "%u", GET_BE_U_2(cp))); OF_FWD(2); if (ndo->ndo_vflag < 2) { @@ -1908,10 +1893,10 @@ of10_queue_stats_reply_print(netdissect_options *ndo, const u_char *cp, u_int len) { while (len) { - if (len < OF_QUEUE_STATS_REPLY_FIXLEN) - goto invalid; + ND_PRINT("\n\t "); + ND_ICHECKMSG_U("remaining length", len, <, OF_QUEUE_STATS_REPLY_FIXLEN); /* port_no */ - ND_PRINT("\n\t port_no %s", + ND_PRINT(" port_no %s", tok2str(ofpp_str, "%u", GET_BE_U_2(cp))); OF_FWD(2); /* pad */ @@ -1987,7 +1972,7 @@ of10_packet_out_print(netdissect_options *ndo, actions_len = GET_BE_U_2(cp); ND_PRINT(", actions_len %u", actions_len); OF_FWD(2); - ND_ICHECK_U(len, <, actions_len); + ND_ICHECK_U(actions_len, >, len); /* actions */ of10_actions_print(ndo, "\n\t ", cp, actions_len); OF_FWD(actions_len); diff --git a/print-openflow-1.3.c b/print-openflow-1.3.c index 9e76ba1b..8008c2f3 100644 --- a/print-openflow-1.3.c +++ b/print-openflow-1.3.c @@ -856,21 +856,20 @@ of13_hello_elements_print(netdissect_options *ndo, while (len) { uint16_t type, bmlen; - if (len < OF_HELLO_ELEM_MINSIZE) - goto invalid; + ND_PRINT("\n\t"); + ND_ICHECKMSG_U("remaining length", len, <, OF_HELLO_ELEM_MINSIZE); /* type */ type = GET_BE_U_2(cp); OF_FWD(2); - ND_PRINT("\n\t type %s", + ND_PRINT(" type %s", tok2str(ofphet_str, "unknown (0x%04x)", type)); /* length */ bmlen = GET_BE_U_2(cp); OF_FWD(2); ND_PRINT(", length %u", bmlen); /* cp is OF_HELLO_ELEM_MINSIZE bytes in */ - if (bmlen < OF_HELLO_ELEM_MINSIZE || - bmlen > OF_HELLO_ELEM_MINSIZE + len) - goto invalid; + ND_ICHECKMSG_U("bitmap length", bmlen, <, OF_HELLO_ELEM_MINSIZE); + ND_ICHECKMSG_U("bitmap length", bmlen, >, OF_HELLO_ELEM_MINSIZE + len); switch (type) { case OFPHET_VERSIONBITMAP: /* diff --git a/tests/TESTLIST b/tests/TESTLIST index 16a6fc1c..96045f82 100644 --- a/tests/TESTLIST +++ b/tests/TESTLIST @@ -289,6 +289,7 @@ 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 +of10_inv_QUEUE_GET_CONFIG_REPLY-vv of10_inv_QUEUE_GET_CONFIG_REPLY.pcap of10_inv_QUEUE_GET_CONFIG_REPLY-vv.out -vv # 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_QUEUE_GET_CONFIG_REPLY-vv.out b/tests/of10_inv_QUEUE_GET_CONFIG_REPLY-vv.out new file mode 100644 index 00000000..60bc83e9 --- /dev/null +++ b/tests/of10_inv_QUEUE_GET_CONFIG_REPLY-vv.out @@ -0,0 +1,8 @@ + 1 06:10:40.134396994 : IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto TCP (6), length 1540, bad cksum 11ce (->4fbd)!) + 109.74.202.0.6653 > 4.184.169.52.4708: Flags [P.], cksum 0x8000 (incorrect -> 0x4d3b), seq 3422244149:3422245637, ack 774246557, win 0, options [nop,nop,unknown-16 0x112e355c0302fa3b], length 1488: OpenFlow + version 1.0, type QUEUE_GET_CONFIG_REPLY, length 278, xid 0xc7ffffff + port 64773 + queue_id 0, len 255 + property NONE, len 0 [property length 0 < 8] (invalid) + [remaining length 7 < 8] (invalid) + version unknown (0x12), type unknown (0x12), length 4626, xid 0x12121212 [|openflow] diff --git a/tests/of10_inv_QUEUE_GET_CONFIG_REPLY.pcap b/tests/of10_inv_QUEUE_GET_CONFIG_REPLY.pcap new file mode 100644 index 00000000..32cd0549 Binary files /dev/null and b/tests/of10_inv_QUEUE_GET_CONFIG_REPLY.pcap differ