From: Alan T. DeKok Date: Mon, 8 Dec 2025 20:13:51 +0000 (-0500) Subject: allow raw attributes in structs X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a8ec8a71401323fd9603695b06442cf31bb83677;p=thirdparty%2Ffreeradius-server.git allow raw attributes in structs but enforce that the length is correct, so that the parent struct is not malformed. Only encode a struct member once, even if the admin specifies it multiple times. update tests to match --- diff --git a/src/lib/util/dict_unknown.c b/src/lib/util/dict_unknown.c index 8834ef15f3c..98351278da1 100644 --- a/src/lib/util/dict_unknown.c +++ b/src/lib/util/dict_unknown.c @@ -71,10 +71,8 @@ static int dict_attr_unknown_init(fr_dict_attr_t const *parent, UNUSED fr_dict_a } } -#if 0 /* - * @todo - This is commented out because it changes some of the debug output, and we want to - * separate commits into logically associated ones. + * Ensure that raw members of a structure have the correct length. */ if (parent->type == FR_TYPE_STRUCT) { if (!da) { @@ -93,12 +91,14 @@ static int dict_attr_unknown_init(fr_dict_attr_t const *parent, UNUSED fr_dict_a } else if (da->type != type) { cannot_change_type: + /* + * @todo - why not? So long as the size is the same... + */ fr_strerror_printf("Cannot create 'raw' attribute in 'struct' which changes data type from '%s' to '%s'", fr_type_to_str(da->type), fr_type_to_str(type)); return -1; } } -#endif return 0; } diff --git a/src/lib/util/struct.c b/src/lib/util/struct.c index 5a091ed2319..771ee0b75dc 100644 --- a/src/lib/util/struct.c +++ b/src/lib/util/struct.c @@ -229,8 +229,7 @@ ssize_t fr_struct_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out, * of the input is suspect. */ if (child_length > (size_t) (end - p)) { - FR_PROTO_TRACE("fr_struct_from_network - child length %zu overflows buffer", child_length); - goto remainder_raw; + child_length = (size_t) (end - p); } } @@ -299,7 +298,7 @@ ssize_t fr_struct_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_assert(!fr_dict_attr_child_by_num(parent, child_num + 1)); /* has to be the last one */ if (!key_vp) { remainder_raw: - child_length = end - p; + child_length = (size_t) (end - p); goto raw; } @@ -758,6 +757,7 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff, bool do_length = false; uint8_t bit_buffer = 0; fr_pair_t const *vp = fr_dcursor_current(parent_cursor); + fr_pair_t const *last = NULL; fr_pair_t const *key_vp = NULL; fr_dict_attr_t const *child, *parent, *key_da = NULL; fr_dcursor_t child_cursor, *cursor; @@ -859,6 +859,15 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff, */ FR_PROTO_TRACE("fr_struct_to_network child %s", child->name); + /* + * If the caller specifies a member twice, then we only encode the first member. + */ + while (last && vp && (last->da->parent == vp->da->parent) && (last->da->attr == vp->da->attr)) { + fr_assert(last != vp); + vp = fr_dcursor_next(cursor); + } + last = vp; + /* * The MEMBER may be raw, in which case it is encoded as octets. * @@ -903,6 +912,7 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff, fr_strerror_printf("Failed encoding bit field %s", child->name); return offset; } + last = NULL; continue; } @@ -917,6 +927,11 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff, * Zero out the unused field. */ FR_DBUFF_MEMSET_RETURN(&work_dbuff, 0, child->flags.length); + + /* + * We didn't encode the current VP, so it's not the last one. + */ + last = NULL; continue; } @@ -965,6 +980,10 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff, return offset; } + /* + * We have to go to the next pair manually, as the protocol-specific + * encode_value() function will normally go to the next cursor entry. + */ vp = fr_dcursor_next(cursor); /* We need to continue, there may be more fields to encode */ diff --git a/src/tests/unit/protocols/dhcpv6/rfc6355.txt b/src/tests/unit/protocols/dhcpv6/rfc6355.txt index 42d6c6361af..f32cc104b74 100644 --- a/src/tests/unit/protocols/dhcpv6/rfc6355.txt +++ b/src/tests/unit/protocols/dhcpv6/rfc6355.txt @@ -74,7 +74,10 @@ match 00 01 00 12 00 04 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 00 00 # And if we see something that's too short, we get a bad attribute. decode-pair 00 01 00 10 00 04 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d -match Client-ID = { DUID = ::UUID, UUID = { raw.Value = 0x000102030405060708090a0b0c0d } } +match Client-ID = { DUID = ::UUID, Value = { raw.4 = 0x000102030405060708090a0b0c0d } } + +encode-pair - +match 00 01 00 10 00 04 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d # # Server Identifier @@ -111,7 +114,7 @@ match Server-ID = { DUID = ::UUID, UUID = { Value = 0x000102030405060708090a0b0c # And if we see something that's too short, we get a bad attribute. decode-pair 00 02 00 10 00 04 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d -match Server-ID = { DUID = ::UUID, UUID = { raw.Value = 0x000102030405060708090a0b0c0d } } +match Server-ID = { DUID = ::UUID, Value = { raw.4 = 0x000102030405060708090a0b0c0d } } count -match 37 +match 39 diff --git a/src/tests/unit/protocols/radius/foreign.txt b/src/tests/unit/protocols/radius/foreign.txt index 82cf72cab68..ab56efb48e7 100644 --- a/src/tests/unit/protocols/radius/foreign.txt +++ b/src/tests/unit/protocols/radius/foreign.txt @@ -55,7 +55,7 @@ match Extended-Attribute-5 = { DHCPv6-Options = { Rapid-Commit = yes, Informatio # Various cross-protocol tests taken from fuzzer output. # decode-proto 04ac00edf504000060800402f50303f5040403f5040402f50302d604040202046000f30303f5040402f502f50302d604040202046000f30303f5040402f50302040202046000f303040402280303f54404029e0200037d2c0404040404400404040402024404029e0200007d2c04040404044004210404020202046000f30303f50202046000f303029e2c7d0402040400000303f5040402f50302040202046000f303040402280303f54404029e0200037d2c040404040440040404040404020202046000f3000000e902046000f303029eaaaaaaaaaaaaaaaaaaaaaaaa2c7d04020404009004ffff60d000032ae00400000004040303f50404 -match Packet-Type = ::Accounting-Request, Packet-Authentication-Vector = 0xf504000060800402f50303f5040403f5, raw.NAS-IP-Address = 0x02f5, raw.214 = 0x0402, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x03, raw.Extended-Attribute-5 = 0x0402, raw.Extended-Attribute-5 = 0x02, raw.214 = 0x0402, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x03, raw.Extended-Attribute-5 = 0x0402, raw.Extended-Attribute-5 = 0x02, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x04, raw.Acct-Status-Type = 0x03, Extended-Attribute-5 = { DHCPv4-Options = { PCP-IPv4-Server-Addresses = { server = { }, raw.server = 0x03 }, raw.V-I-Vendor-Specific = 0x0404040404400404040402024404029e0200007d2c04040404044004210404020202046000f30303f5020204, raw.96 = 0x, Site-specific-19 = 0x029e2c, raw.V-I-Vendor-Specific = 0x02040400 }, raw.DHCPv4-Options = 0x9e0200037d2c040404040440040404040404020202046000f3000000e902046000f303029eaaaaaaaaaaaaaaaaaaaaaaaa2c7d04020404009004ffff60d00003 }, CHAP-Password = 0xf5, raw.NAS-IP-Address = 0x02f5, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x04, raw.Acct-Status-Type = 0x03 +match Packet-Type = ::Accounting-Request, Packet-Authentication-Vector = 0xf504000060800402f50303f5040403f5, raw.NAS-IP-Address = 0x02f5, raw.214 = 0x0402, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x03, raw.Extended-Attribute-5 = 0x0402, raw.Extended-Attribute-5 = 0x02, raw.214 = 0x0402, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x03, raw.Extended-Attribute-5 = 0x0402, raw.Extended-Attribute-5 = 0x02, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x04, raw.Acct-Status-Type = 0x03, Extended-Attribute-5 = { DHCPv4-Options = { raw.PCP-IPv4-Server-Addresses = 0x0003, raw.V-I-Vendor-Specific = 0x0404040404400404040402024404029e0200007d2c04040404044004210404020202046000f30303f5020204, raw.96 = 0x, Site-specific-19 = 0x029e2c, raw.V-I-Vendor-Specific = 0x02040400 }, raw.DHCPv4-Options = 0x9e0200037d2c040404040440040404040404020202046000f3000000e902046000f303029eaaaaaaaaaaaaaaaaaaaaaaaa2c7d04020404009004ffff60d00003 }, CHAP-Password = 0xf5, raw.NAS-IP-Address = 0x02f5, User-Password = "\366\356", raw.Extended-Attribute-3 = 0x04, raw.Acct-Status-Type = 0x03 decode-proto 03 12 02 00 00 00 00 00 00 03 03 03 03 02 fc 03 02 d7 00 04 04 02 02 02 03 02 3d 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 04 02 02 02 04 04 04 03 f5 04 04 02 f5 03 02 d6 04 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 04 02 02 02 04 04 04 04 04 04 04 04 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 02 02 04 04 04 04 04 04 04 04 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 9a 04 02 f5 03 03 f5 f1 04 04 04 03 f5 03 02 03 03 04 02 02 00 f3 04 60 03 03 f5 04 04 02 f5 03 03 f5 04 04 02 02 02 03 02 3d 04 60 00 f3 03 03 f5 04 03 03 f5 03 03 03 03 03 03 03 03 03 03 03 03 03 03 03 03 03 03 03 e8 03 03 02 5c 03 03 03 03 03 03 02 5c 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 02 02 04 04 04 04 04 04 04 04 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 03 03 f5 04 04 02 f5 03 03 04 02 02 02 3d 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f5 04 02 f3 03 03 04 f5 03 03 f5 04 04 02 02 02 04 04 04 03 f5 04 04 02 f5 03 02 d6 04 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 04 02 02 02 04 04 04 04 04 04 04 04 f3 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 03 03 f5 04 04 02 f5 03 03 04 02 02 04 60 00 f3 03 03 f5 04 04 02 f5 03 03 f5 04 04 02 02 02 03 02 46 04 2a 04 04 04 44 03 03 03 03 03 03 03 03 03 72 diff --git a/src/tests/unit/protocols/radius/struct.txt b/src/tests/unit/protocols/radius/struct.txt index 768bf55cd78..b6cf59a8035 100644 --- a/src/tests/unit/protocols/radius/struct.txt +++ b/src/tests/unit/protocols/radius/struct.txt @@ -94,14 +94,21 @@ encode-pair - match ff 0d 02 00 00 1a 99 ff fe fd fc fb fa # -# @todo - these are perhaps wrong? The raw attribute should perhaps -# be ignored, or else it should be an error to create it. +# raw.2 has the same number as "Filler". We don't want to encode fields twice. # encode-pair Test-Struct = { Key-Field = 2, Filler = 6809, raw.2 = 0xfffefdfcfbfa } -match ff 0d 02 00 00 1a 99 ff fe fd fc fb fa +match ff 07 02 00 00 1a 99 + +decode-pair - +match Test-Struct = { Key-Field = 2, Filler = 6809 } -encode-pair Test-Struct = { Key-Field = 3, Filler = 6809, raw.2 = 0xfffefdfcfbfa } -match ff 0d 03 00 00 1a 99 ff fe fd fc fb fa +# +# The final field is too long, but we still decode it. +# +# @todo - arguably we should ignore the extra data, as the contents of "data" should only be 2 octets? +# +decode-pair ff 0d 02 00 00 1a 99 ff fe fd fc fb fa +match Test-Struct = { Key-Field = 2, Filler = 6809, Data = { raw.2 = 0xfffefdfcfbfa } } count -match 50 +match 52