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 fc6acab08c3c6b0d76057267b0e78998aa31800a..d4f7ef6be3669f72a6e47c90b391712209bae883 100644 (file)
--- 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 894306d1e537cf537e0f65e9a7fcd4e71f1988ca..c82e1368a636793f7d73a14e6f50bd710612ed2c 100644 (file)
--- 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;