correct decode of length + offset fields in structs

diff --git a/src/lib/util/struct.c b/src/lib/util/struct.c
index 58d177f4a5c915654c28117a115b7503fef4f233..17a1296a132d2f0703e1b514932b7c62906fe8e4 100644 (file)
--- a/src/lib/util/struct.c
+++ b/src/lib/util/struct.c
@@ -79,47 +79,51 @@ ssize_t fr_struct_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
         *      Decode structs with length prefixes.
         */
        if (da_is_length_field(parent)) {
-               size_t struct_len, need, new_len;
+               size_t claimed_len, field_len, calc_len;
 
+               /*
+                *      Set how many bytes there are in the "length" field.
+                */
                if (parent->flags.subtype == FLAG_LENGTH_UINT8) {
-                       need = 1;
+                       field_len = 1;
                } else {
-                       need = 2;
+                       field_len = 2;
                }
 
-               if ((size_t) (end - p) < need) {
-                       FR_PROTO_TRACE("Insufficient room for length header");
+               if ((size_t) (end - p) < field_len) {
+                       FR_PROTO_TRACE("Insufficient room for length field");
                        goto unknown;
                }
 
-               struct_len = p[0];
-               if (need > 1) {
-                       struct_len <<= 8;
-                       struct_len |= p[1];
+               claimed_len = p[0];
+               if (field_len > 1) {
+                       claimed_len <<= 8;
+                       claimed_len |= p[1];
                }
+               p += field_len;
 
-               if (struct_len < da_length_offset(parent)) {
+               if (claimed_len < da_length_offset(parent)) {
                        FR_PROTO_TRACE("Length header (%zu) is smaller than minimum value (%u)",
-                                      struct_len, parent->flags.type_size);
+                                      claimed_len, parent->flags.type_size);
                        goto unknown;
                }
 
-               if ((p + struct_len + need - da_length_offset(parent)) > end) {
+               /*
+                *      Get the calculated length of the actual data.
+                */
+               calc_len = claimed_len - da_length_offset(parent);
+
+               if (calc_len > (size_t) (end - p)) {
                        FR_PROTO_TRACE("Length header (%zu) is larger than remaining data (%zu)",
-                                      struct_len + need, (end - p));
+                                      claimed_len + field_len, (end - p));
                        goto unknown;
                }
 
                /*
-                *      Skip the length field, and tell the decoder to
-                *      stop at the end of the data we're supposed to decode.
+                *      Limit the size of the decoded structure to the correct length.
                 */
-               p += need;
-               end = p + struct_len;
-               new_len = struct_len + need - offset;
-               if (new_len > data_len) goto unknown;
-
-               data_len = new_len;
+               data_len = calc_len;
+               end = p + data_len;
        }
 
        /*
@@ -244,9 +248,10 @@ ssize_t fr_struct_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
                 *      Eat up the rest of the data.
                 */
                if (!child_length || (child->flags.array)) {
-                       child_length = (end - p);
+                       child_length = end - p;
 
                } else if ((size_t) (end - p) < child_length) {
+                       FR_PROTO_TRACE("fr_struct_from_network - child length %zd underflows buffer", child_length);
                        goto unknown;
                }
 
@@ -368,7 +373,8 @@ ssize_t fr_struct_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
                         */
                        child = fr_dict_unknown_attr_afrom_num(child_ctx, key_vp->da, 0);
                        if (!child) {
-                               FR_PROTO_TRACE("failed allocating unknown child for key VP %s", key_vp->da->name);
+                               FR_PROTO_TRACE("failed allocating unknown child for key VP %s - %s",
+                                              key_vp->da->name, fr_strerror());
                                goto unknown;
                        }
 

diff --git a/src/tests/unit/protocols/bfd/base.txt b/src/tests/unit/protocols/bfd/base.txt
index e70ea6a28e15f8b28ba4591f5cef24497f53d64e..93ac238c374da7d54b7b8faf038c744c0414f7c6 100644 (file)
--- a/src/tests/unit/protocols/bfd/base.txt
+++ b/src/tests/unit/protocols/bfd/base.txt
@@ -30,11 +30,23 @@ match Packet-Type = Up, Packet = { version = 1, diagnostic = none, state = up, p
 #
 #  With authentication type
 #
-encode-proto Packet-Type = Up, Packet = { version = 1, diagnostic = none, state = up, poll = no, final = no, control-plane-independent = no, auth-present = yes, demand = no, multipoint = no, detect-multi = 3, length = 24, my-discriminator = 3735928559, your-discriminator = 554854409, desired-min-tx-interval = 31, required-min-tx-interval = 127, required-min-echo-interval = 255, auth-type.simple-password = { key-id = 0, reserved = 0, password = "hello" } }
-match 20 c4 03 21 de ad be ef 21 12 68 09 00 00 00 1f 00 00 00 7f 00 00 00 ff 00 09 00 00 68 65 6c 6c 6f
+decode-proto 20 c4 03 21 de ad be ef 21 12 68 09 00 00 00 1f 00 00 00 7f 00 00 00 ff 01 0a 0f 00 68 65 6c 6c 6f
+match Packet-Type = Up, Packet = { version = 1, diagnostic = none, state = up, poll = no, final = no, control-plane-independent = no, auth-present = yes, demand = no, multipoint = no, detect-multi = 3, length = 33, my-discriminator = 3735928559, your-discriminator = 554854409, desired-min-tx-interval = 31, required-min-tx-interval = 127, required-min-echo-interval = 255, auth-type = simple-password, auth-type.simple-password = { key-id = 15, reserved = 0, password = "hello" } }
 
-#decode-proto -
-#match Packet-Type = Up, raw.Packet = 0x20c40321deadbeef211268090000001f0000007f000000ff0009000068656c6c6f
+
+#
+#  @todo - this is wrong!  The auth-type field isn't encoded, likely because of confusion with Auth-Type :(
+#
+encode-proto Packet-Type = Up, Packet = { version = 1, diagnostic = none, state = up, poll = no, final = no, control-plane-independent = no, auth-present = yes, demand = no, multipoint = no, detect-multi = 3, length = 24, my-discriminator = 3735928559, your-discriminator = 554854409, desired-min-tx-interval = 31, required-min-tx-interval = 127, required-min-echo-interval = 255, auth-type.simple-password = { key-id = 15, reserved = 0, password = "hello" } }
+match 20 c4 03 21 de ad be ef 21 12 68 09 00 00 00 1f 00 00 00 7f 00 00 00 ff 00 0a 0f 00 68 65 6c 6c 6f
+
+#
+#  Because auth-type=0, we can't decode the above text.
+#
+decode-proto -
+match Packet-Type = Up, raw.Packet = 0x20c40321deadbeef211268090000001f0000007f000000ff000a0f0068656c6c6f
+
+#match Packet = { version = 1, diagnostic = none, state = up, poll = no, final = no, control-plane-independent = no, auth-present = yes, demand = no, multipoint = no, detect-multi = 3, length = 24, my-discriminator = 3735928559, your-discriminator = 554854409, desired-min-tx-interval = 31, required-min-tx-interval = 127, required-min-echo-interval = 255, auth-type.simple-password = { key-id = 0, reserved = 0, password = "hello" } }
 
 count
-match 13
+match 17