From: Alan T. DeKok Date: Tue, 12 Sep 2023 12:53:03 +0000 (-0400) Subject: fix RADIUS for nested attribute encoding X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=eb323bfd101caf29b30ac03b023e436cf831d81b;p=thirdparty%2Ffreeradius-server.git 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 bfda8388033..7e5589b0c0f 100644 --- 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 800f5c6bde1..2a1b2411f47 100644 --- 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 c31d6917ff9..110b67e222b 100644 --- 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 aa63640349b..7abadda685e 100644 --- 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 48e0ad74cf3..92c5fc977f1 100644 --- 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