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 d942fd8f8b1bae1362f9125608beb085acabd34a..00c4fdd35a41acdfe0b10ae8ea4f9ff0d3d7b384 100644 (file)
--- 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 9e76ba1b29358fa86b7ac9b6af1d47abb9ee1416..8008c2f38ccb4d8746189957e13086abbd5d5c33 100644 (file)
--- 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 16a6fc1c083acc5e758b48cc24c5e194a4289562..96045f82b979a66ffaed6a60e6dac9bc49d70fa2 100644 (file)
--- 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 (file)
index 0000000..60bc83e
--- /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 (file)
index 0000000..32cd054
Binary files /dev/null and b/tests/of10_inv_QUEUE_GET_CONFIG_REPLY.pcap differ