fix RADIUS for nested attribute encoding

the main difference is that we fix encode_extended() to correctly
handle nesting, and update the tests.  As a side effect, the
encode_extended() function now always requires nesting, and if
passed flat extended attributes, will return an encoding error.

We also fix up the fr_pair_cursor_to_network() function to remove
the flat vs nested hacks.  It now always expects nesting.

We also fix up fr_struct_to_network() to always expect nesting
for the trailing TLV in a struct.

diff --git a/src/lib/util/encode.c b/src/lib/util/encode.c
index bfda83880337464f944e44b3215d14ed54c0035c..7e5589b0c0fc308cf9b5104686c24cc3ef63ca1e 100644 (file)
--- a/src/lib/util/encode.c
+++ b/src/lib/util/encode.c
@@ -73,13 +73,16 @@ ssize_t fr_pair_cursor_to_network(fr_dbuff_t *dbuff,
                                  fr_dcursor_t *cursor, void *encode_ctx, fr_encode_dbuff_t encode_pair)
 {
        fr_dbuff_t              work_dbuff = FR_DBUFF(dbuff);
-       fr_pair_t const         *vp;
+       fr_pair_t const         *vp = fr_dcursor_current(cursor);
        fr_dict_attr_t const    *da = da_stack->da[depth];
        ssize_t                 len;
 
-       while ((vp = fr_dcursor_current(cursor)) != NULL) {
+       while (true) {
                FR_PROTO_STACK_PRINT(da_stack, depth);
 
+               vp = fr_dcursor_current(cursor);
+               fr_assert(!vp->da->flags.internal);
+
                len = encode_pair(&work_dbuff, da_stack, depth + 1, cursor, encode_ctx);
                if (len < 0) return len;
 
@@ -88,6 +91,11 @@ ssize_t fr_pair_cursor_to_network(fr_dbuff_t *dbuff,
                 */
                if (!fr_dcursor_current(cursor) || (vp == fr_dcursor_current(cursor))) break;
 
+               vp = fr_dcursor_current(cursor);
+               if (!vp) break;
+
+               fr_proto_da_stack_build(da_stack, vp->da);
+
                /*
                 *      We can encode multiple children, if after
                 *      rebuilding the DA Stack, the attribute at this

diff --git a/src/lib/util/struct.c b/src/lib/util/struct.c
index 800f5c6bde1f3008fa1ace641a0f8974e71ff849..2a1b2411f47f6bf007daa60413e93d6b26f18486 100644 (file)
--- a/src/lib/util/struct.c
+++ b/src/lib/util/struct.c
@@ -538,7 +538,7 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff,
         *      nested attributes.
         */
        if (vp && (vp->da->parent != parent)) {
-               fr_strerror_printf("%s: struct encoding is missing previous attributes for %s (parent %s, expecting %s)",
+               fr_strerror_printf("%s: Asked to encode %s, but its parent %s is not the expected parent %s",
                                   __FUNCTION__, vp->da->name, vp->da->parent->name, parent->name);
                return PAIR_ENCODE_FATAL_ERROR;
        }
@@ -814,8 +814,9 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff,
 
 done:
        vp = fr_dcursor_current(cursor);
-       if (tlv && vp) {
+       if (tlv && vp && (vp->da == tlv) && encode_pair) {
                ssize_t slen;
+               fr_dcursor_t tlv_cursor;
 
                if (!encode_pair) {
                        fr_strerror_printf("Asked to encode child attribute %s, but we were not passed an encoding function",
@@ -823,10 +824,17 @@ done:
                        return PAIR_ENCODE_FATAL_ERROR;
                }
 
-               fr_proto_da_stack_build(da_stack, vp->da);
+               fr_assert(fr_type_is_structural(vp->vp_type));
 
-               slen = fr_pair_cursor_to_network(&work_dbuff, da_stack, depth + 1, cursor, encode_ctx, encode_pair);
-               if (slen < 0) return slen;
+               vp = fr_pair_dcursor_init(&tlv_cursor, &vp->vp_group);
+               if (vp) {
+                       FR_PROTO_TRACE("fr_struct_to_network trailing TLVs of %s", tlv->name);
+                       fr_proto_da_stack_build(da_stack, vp->da);
+                       FR_PROTO_STACK_PRINT(da_stack, depth);
+
+                       slen = fr_pair_cursor_to_network(&work_dbuff, da_stack, depth + 1, &tlv_cursor, encode_ctx, encode_pair);
+                       if (slen < 0) return slen;
+               }
        }
 
        if (do_length) {

diff --git a/src/protocols/dhcpv6/encode.c b/src/protocols/dhcpv6/encode.c
index c31d6917ff934a881fae70803cff8ef870443664..110b67e222b95f7ea57f1c09f568a68868ec2724 100644 (file)
--- a/src/protocols/dhcpv6/encode.c
+++ b/src/protocols/dhcpv6/encode.c
@@ -96,9 +96,6 @@ static ssize_t encode_value(fr_dbuff_t *dbuff,
                slen = fr_struct_to_network(&work_dbuff, da_stack, depth, cursor, encode_ctx, encode_value, encode_child);
                if (slen <= 0) return slen;
 
-               /*
-                *      Rebuild the da_stack for the next option.
-                */
                vp = fr_dcursor_current(cursor);
                fr_proto_da_stack_build(da_stack, vp ? vp->da : NULL);
                return fr_dbuff_set(dbuff, &work_dbuff);
@@ -499,7 +496,6 @@ static ssize_t encode_tlv(fr_dbuff_t *dbuff,
        }
 
        if (!da_stack->da[depth + 1]) {
-               fr_assert(0);
                fr_strerror_printf("%s: Can't encode empty TLV", __FUNCTION__);
                return PAIR_ENCODE_FATAL_ERROR;
        }

diff --git a/src/protocols/radius/encode.c b/src/protocols/radius/encode.c
index aa63640349bc0f09c0c4988333d30f22fc5bdf91..7abadda685e4bd7a61f3e56118a866713fee3722 100644 (file)
--- a/src/protocols/radius/encode.c
+++ b/src/protocols/radius/encode.c
@@ -41,7 +41,6 @@ static ssize_t encode_child(fr_dbuff_t *dbuff,
                                fr_da_stack_t *da_stack, unsigned int depth,
                                fr_dcursor_t *cursor, void *encode_ctx);
 
-
 /** "encrypt" a password RADIUS style
  *
  * Input and output buffers can be identical if in-place encryption is needed.
@@ -364,9 +363,8 @@ static ssize_t encode_value(fr_dbuff_t *dbuff,
         *      This has special requirements.
         */
        if ((vp->vp_type == FR_TYPE_STRUCT) || (da->type == FR_TYPE_STRUCT)) {
-               slen = fr_struct_to_network(&work_dbuff, da_stack, depth, cursor, encode_ctx, encode_value,
-                                           encode_tlv);
-               if (slen < 0) return slen;
+               slen = fr_struct_to_network(&work_dbuff, da_stack, depth, cursor, encode_ctx, encode_value, encode_child);
+               if (slen <= 0) return slen;
 
                vp = fr_dcursor_current(cursor);
                fr_proto_da_stack_build(da_stack, vp ? vp->da : NULL);
@@ -728,8 +726,10 @@ static ssize_t encode_extended(fr_dbuff_t *dbuff,
        uint8_t                 hlen;
        size_t                  vendor_hdr;
        bool                    extra;
+       int                     my_depth;
+       fr_dict_attr_t const    *da;
        fr_dbuff_marker_t       hdr, length_field;
-       fr_pair_t const *vp = fr_dcursor_current(cursor);
+       fr_pair_t const         *vp = fr_dcursor_current(cursor);
        fr_dbuff_t              work_dbuff;
 
        PAIR_VERIFY(vp);
@@ -752,14 +752,14 @@ static ssize_t encode_extended(fr_dbuff_t *dbuff,
         *      Encode the header for "short" or "long" attributes
         */
        hlen = 3 + extra;
-       FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, (uint8_t)da_stack->da[depth++]->attr);
+       FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, (uint8_t)da_stack->da[0]->attr);
        fr_dbuff_marker(&length_field, &work_dbuff);
        FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, hlen); /* this gets overwritten later*/
 
        /*
         *      Encode which extended attribute it is.
         */
-       FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, (uint8_t)da_stack->da[depth]->attr);
+       FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, (uint8_t)da_stack->da[1]->attr);
 
        if (extra) FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, 0x00); /* flags start off at zero */
 
@@ -768,26 +768,47 @@ static ssize_t encode_extended(fr_dbuff_t *dbuff,
        /*
         *      Handle VSA as "VENDOR + attr"
         */
-       if (da_stack->da[depth]->type == FR_TYPE_VSA) {
-               depth++;
+       if (da_stack->da[1]->type == FR_TYPE_VSA) {
+               fr_assert(da_stack->da[2]);
+               fr_assert(da_stack->da[2]->type == FR_TYPE_VENDOR);
+
+               FR_DBUFF_IN_RETURN(&work_dbuff, (uint32_t) da_stack->da[2]->attr);
 
-               FR_DBUFF_IN_RETURN(&work_dbuff, (uint32_t) da_stack->da[depth++]->attr);
-               FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, (uint8_t)da_stack->da[depth]->attr);
+               fr_assert(da_stack->da[3]);
+
+               FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, (uint8_t)da_stack->da[3]->attr);
 
                hlen += 5;
                vendor_hdr = 5;
 
                FR_PROTO_STACK_PRINT(da_stack, depth);
                FR_PROTO_HEX_DUMP(fr_dbuff_current(&hdr), hlen, "header extended vendor specific");
+
+               my_depth = 3;
        } else {
                vendor_hdr = 0;
                FR_PROTO_HEX_DUMP(fr_dbuff_current(&hdr), hlen, "header extended");
+
+               my_depth = 1;
        }
 
        /*
-        *      This also handles TLVs
+        *      We're at the point where we need to encode something.
         */
-       slen = encode_value(&work_dbuff, da_stack, depth, cursor, encode_ctx);
+       da = da_stack->da[my_depth];
+       fr_assert(vp->da == da);
+
+       if (fr_type_is_leaf(da->type)) {
+               slen = encode_value(&work_dbuff, da_stack, my_depth, cursor, encode_ctx);
+
+       } else if (da->type == FR_TYPE_STRUCT) {
+               slen = fr_struct_to_network(&work_dbuff, da_stack, my_depth, cursor, encode_ctx, encode_value, encode_child);
+
+       } else {
+               fr_assert(da->type == FR_TYPE_TLV);
+
+               slen = encode_tlv(&work_dbuff, da_stack, my_depth, cursor, encode_ctx);
+       }
        if (slen <= 0) return slen;
 
        /*
@@ -836,14 +857,16 @@ static ssize_t encode_extended_nested(fr_dbuff_t *dbuff,
 
        (void) fr_pair_dcursor_init(&child_cursor, &parent->vp_group);
 
+       FR_PROTO_STACK_PRINT(da_stack, depth);
+
        while ((vp = fr_dcursor_current(&child_cursor)) != NULL) {
-               if (fr_type_is_leaf(vp->vp_type) ||
-                   ((depth > 1) && ((vp->vp_type == FR_TYPE_TLV) || (vp->vp_type == FR_TYPE_STRUCT)))) {
-                       fr_proto_da_stack_build(da_stack, vp ? vp->da : NULL);
-                       slen = encode_extended(&work_dbuff, da_stack, 0, &child_cursor, encode_ctx);
+               if ((vp->vp_type == FR_TYPE_VSA) || (vp->vp_type == FR_TYPE_VENDOR)) {
+                       slen = encode_extended_nested(&work_dbuff, da_stack, depth + 1, &child_cursor, encode_ctx);
 
                } else {
-                       slen = encode_extended_nested(&work_dbuff, da_stack, depth + 1, &child_cursor, encode_ctx);
+                       fr_proto_da_stack_build(da_stack, vp->da);
+                       slen = encode_extended(&work_dbuff, da_stack, depth, &child_cursor, encode_ctx);
+                       if (slen < 0) return slen;
                }
 
                if (slen < 0) return slen;
@@ -851,10 +874,6 @@ static ssize_t encode_extended_nested(fr_dbuff_t *dbuff,
 
        vp = fr_dcursor_next(cursor);
 
-       /*
-        *      @fixme: attributes with 'concat' MUST of type
-        *      'octets', and therefore CANNOT have any TLV data in them.
-        */
        fr_proto_da_stack_build(da_stack, vp ? vp->da : NULL);
 
        return fr_dbuff_set(dbuff, &work_dbuff);
@@ -929,13 +948,14 @@ static ssize_t encode_child(fr_dbuff_t *dbuff,
        fr_dbuff_t              work_dbuff = FR_DBUFF_MAX(dbuff, UINT8_MAX);
 
        FR_PROTO_STACK_PRINT(da_stack, depth);
+
+       fr_assert(da_stack->da[depth] != NULL);
+
        fr_dbuff_marker(&hdr, &work_dbuff);
 
        hlen = 2;
        FR_DBUFF_IN_BYTES_RETURN(&work_dbuff, (uint8_t)da_stack->da[depth]->attr, hlen);
 
-       fr_assert(da_stack->da[depth] != NULL);
-
        slen = encode_value(&work_dbuff, da_stack, depth, cursor, encode_ctx);
        if (slen <= 0) return slen;
 
@@ -1608,7 +1628,8 @@ ssize_t fr_radius_encode_pair(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, void *enc
                        slen = encode_child(&work_dbuff, &da_stack, 0, cursor, encode_ctx);
 
                } else if (vp->da != da) {
-                       slen = encode_extended(&work_dbuff, &da_stack, 0, cursor, encode_ctx);
+                       fr_strerror_printf("extended attributes must be nested");
+                       return PAIR_ENCODE_FATAL_ERROR;
 
                } else {
                        slen = encode_extended_nested(&work_dbuff, &da_stack, 0, cursor, encode_ctx);

diff --git a/src/tests/unit/protocols/radius/struct.txt b/src/tests/unit/protocols/radius/struct.txt
index 48e0ad74cf31d1d290115a7189879e071bdd065c..92c5fc977f119813a29794e5b16357c80b0a450f 100644 (file)
--- a/src/tests/unit/protocols/radius/struct.txt
+++ b/src/tests/unit/protocols/radius/struct.txt
@@ -2,6 +2,9 @@ proto radius
 proto-dictionary radius
 fuzzer-out radius
 
+encode-pair Extended-Attribute-1 = { Unit-Ext-241-Struct2 = { Unit-Struct2-Int1 = 1, Unit-Struct2-Short = 4 } }
+match f1 09 f8 00 00 00 01 00 04
+
 #
 #  Structs in RADIUS
 #
@@ -11,6 +14,15 @@ match f1 10 f7 00 00 00 01 00 00 00 02 00 04 66 6f 6f
 decode-pair -
 match Extended-Attribute-1.Unit-Ext-241-Struct1.Unit-Struct1-Int1 = 1, Extended-Attribute-1.Unit-Ext-241-Struct1.Unit-Struct1-Int2 = 2, Extended-Attribute-1.Unit-Ext-241-Struct1.Unit-Struct1-Short = 4, Extended-Attribute-1.Unit-Ext-241-Struct1.Unit-Struct1-String = "foo"
 
+pair Extended-Attribute-1.Unit-Ext-241-Struct2.Unit-Struct2-Int1 = 1, Extended-Attribute-1.Unit-Ext-241-Struct2.Unit-Struct2-Short = 4, Extended-Attribute-1.Unit-Ext-241-Struct2.Unit-Struct2-TLV.Unit-Struct2-TLV-Int1 = 6, Extended-Attribute-1.Unit-Ext-241-Struct2.Unit-Struct2-TLV.Unit-Struct2-TLV-Int2 = 7, Extended-Attribute-1.Unit-Ext-241-Struct2.Unit-Struct2-TLV.Unit-Struct2-TLV-String = "foo"
+match Extended-Attribute-1 = { Unit-Ext-241-Struct2 = { Unit-Struct2-Int1 = 1, Unit-Struct2-Short = 4, Unit-Struct2-TLV = { Unit-Struct2-TLV-Int1 = 6, Unit-Struct2-TLV-Int2 = 7, Unit-Struct2-TLV-String = "foo" } } }
+
+encode-pair Extended-Attribute-1 = { Unit-Ext-241-Struct2 = { Unit-Struct2-Int1 = 1, Unit-Struct2-Short = 4 } }
+match f1 09 f8 00 00 00 01 00 04
+
+encode-pair Extended-Attribute-1 = { Unit-Ext-241-Struct2 = { Unit-Struct2-Int1 = 1, Unit-Struct2-Short = 4, Unit-Struct2-TLV = { Unit-Struct2-TLV-Int1 = 6, Unit-Struct2-TLV-Int2 = 7, Unit-Struct2-TLV-String = "foo" } } }
+match f1 1a f8 00 00 00 01 00 04 01 06 00 00 00 06 02 06 00 00 00 07 03 05 66 6f 6f
+
 #
 # And structs where the last part is a TLV.
 #
@@ -73,4 +85,4 @@ encode-pair -
 match ff 0d 02 00 00 1a 99 ff fe fd fc fb fa
 
 count
-match 33
+match 41