From: Alan T. DeKok Date: Fri, 19 Dec 2025 08:16:12 +0000 (+0100) Subject: limit unknown depth, and be more careful about error returns X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6fa7fb79eb01900e1fd2d46e73fe6578db2718ff;p=thirdparty%2Ffreeradius-server.git limit unknown depth, and be more careful about error returns tweak value-box type "attr" parsing to catch more cases. --- diff --git a/src/lib/util/dict_unknown.c b/src/lib/util/dict_unknown.c index ef823db109f..744ae7d3a52 100644 --- a/src/lib/util/dict_unknown.c +++ b/src/lib/util/dict_unknown.c @@ -470,6 +470,7 @@ fr_slen_t fr_dict_attr_unknown_afrom_oid_substr(TALLOC_CTX *ctx, fr_sbuff_t our_in = FR_SBUFF(in); fr_dict_attr_t const *our_parent = parent; fr_dict_attr_t *n = NULL; + int depth; fr_dict_attr_flags_t flags = { .is_raw = true, }; *out = NULL; @@ -492,10 +493,18 @@ fr_slen_t fr_dict_attr_unknown_afrom_oid_substr(TALLOC_CTX *ctx, /* * Loop until there's no more component separators. */ - for (;;) { + for (depth = 0; depth < FR_DICT_MAX_TLV_STACK; depth++) { uint32_t num; fr_sbuff_parse_error_t sberr; + /* + * Cannot create attributes that are too deeply nested. + */ + if ((depth + parent->depth) >= FR_DICT_MAX_TLV_STACK) { + fr_strerror_printf("Attribute depth (%u) is too large", depth + parent->depth); + goto error; + } + fr_sbuff_out(&sberr, &num, &our_in); switch (sberr) { case FR_SBUFF_PARSE_OK: @@ -572,13 +581,32 @@ fr_slen_t fr_dict_attr_unknown_afrom_oid_substr(TALLOC_CTX *ctx, default: { + size_t len; fr_sbuff_marker_t c_start; fr_sbuff_marker(&c_start, &our_in); - fr_sbuff_adv_past_allowed(&our_in, FR_DICT_ATTR_MAX_NAME_LEN, fr_dict_attr_allowed_chars, NULL); - fr_strerror_printf("Unknown attribute \"%.*s\" for parent \"%s\"", + len = fr_sbuff_adv_past_allowed(&our_in, FR_DICT_ATTR_MAX_NAME_LEN, fr_dict_attr_allowed_chars, NULL); + + /* + * If we saw no valid characters at the start, it's a bad attribute name. + * + * If we saw valid characters but didn't parse them into an attribute name, it's + * a bad attribute name. + * + * Otherwise we parsed at least one attribute, and then ran out of valid + * attribute characters to parse. The result must be OK. + * + * This check is here really only because there's a lot of code which parses + * attributes, but does not properly set either the buffer size to be _just_ the + * attribute name, OR the set of terminal characters. This means that the + * attribute parsing code can't error out if there are invalid characters after a + * valid attribute name. Instead, the caller has to check the return code. + */ + if (((depth == 0) && (len == 0)) || ((depth > 0) && (len > 0))) { + fr_strerror_printf("Unknown attribute \"%.*s\" for parent \"%s\"", (int)fr_sbuff_behind(&c_start), fr_sbuff_current(&c_start), our_parent->name); - goto error; + goto error; + } } } break; diff --git a/src/lib/util/value.c b/src/lib/util/value.c index 0048feb801b..d00da041bc7 100644 --- a/src/lib/util/value.c +++ b/src/lib/util/value.c @@ -5689,33 +5689,36 @@ parse: fr_sbuff_advance(&our_in, len); FR_SBUFF_SET_RETURN(in, &our_in); - } else if (fr_sbuff_adv_past_str_literal(&our_in, "::")) { + } else { + fr_dict_attr_t const *da; + + (void) fr_sbuff_adv_past_str_literal(&our_in, "::"); slen = fr_dict_attr_by_oid_substr(NULL, &dst->vb_attr, dst_enumv, &our_in, rules->terminals); - if (slen <= 0) goto unknown_attr; + if (slen > 0) { + fr_assert(dst->vb_attr != NULL); - fr_assert(dst->vb_attr != NULL); + if (!fr_sbuff_next_if_char(&our_in, '.')) { + FR_SBUFF_SET_RETURN(in, &our_in); + } - if (!fr_sbuff_next_if_char(&our_in, '.')) { - FR_SBUFF_SET_RETURN(in, &our_in); + /* + * The next bit MUST be an unknown attribute. + */ } - /* - * Parse the unknown attribute starting from where we left off. - */ - dst_enumv = dst->vb_attr; - dst->vb_attr = NULL; - } + if (!fr_sbuff_is_digit(&our_in)) { + invalid_attr: + fr_strerror_printf_push("Failed to find the attribute in %s", dst_enumv->name); + return -2; + } -unknown_attr: - slen = fr_dict_attr_unknown_afrom_oid_substr(ctx, &dst->vb_attr, dst_enumv, &our_in, FR_TYPE_OCTETS); - if (slen <= 0) { - fr_strerror_printf_push("Failed to find the attribute in %s", dst_enumv->name); - return -2; - } + slen = fr_dict_attr_unknown_afrom_oid_substr(ctx, &da, dst->vb_attr, &our_in, FR_TYPE_OCTETS); + if (slen <= 0) goto invalid_attr; - fr_assert(dst->vb_attr != NULL); - FR_SBUFF_SET_RETURN(in, &our_in); + dst->vb_attr = da; + FR_SBUFF_SET_RETURN(in, &our_in); + } /* * Dealt with below @@ -5995,7 +5998,10 @@ ssize_t fr_value_box_print(fr_sbuff_t *out, fr_value_box_t const *data, fr_sbuff FR_SBUFF_IN_CHAR_RETURN(&our_out, ':', ':'); - fr_assert(data->enumv != NULL); + if (!data->enumv) { + fr_strerror_const("Value of type 'attribute' is missing the enum"); + return -1; + } switch (data->enumv->type) { case FR_TYPE_TLV: diff --git a/src/tests/unit/cast.txt b/src/tests/unit/cast.txt index 72cb20b5b46..4b0da9ee9e2 100644 --- a/src/tests/unit/cast.txt +++ b/src/tests/unit/cast.txt @@ -50,7 +50,13 @@ cast attribute ::Vendor-Specific.Cisco.AVPair -> attribute match ::Vendor-Specific.Cisco.AVPair cast string "::26.9.1" -> attribute -match Vendor-Specific.Cisco.AVPair +match ::Vendor-Specific.Cisco.AVPair + +cast string "26.9.1" -> attribute +match ::Vendor-Specific.Cisco.AVPair + +cast string "26.12345.1" -> attribute +match ::Vendor-Specific.12345.1 count -match 28 +match 34 diff --git a/src/tests/unit/protocols/der/base.txt b/src/tests/unit/protocols/der/base.txt index e852fe8e601..e2f4d2c2652 100644 --- a/src/tests/unit/protocols/der/base.txt +++ b/src/tests/unit/protocols/der/base.txt @@ -22,7 +22,7 @@ pair Test-Oid = "1.2.840.113549.1.1.11" match Test-Oid = ::1.2.840.113549.1.1.11 pair Test-Oid = ::iso.member-body.us.rsadsi.pkcs.pkcs-1.rsaEncryption -match Unknown attribute "iso" for parent "DER": Failed to find the attribute in DER +match Attribute 'iso' not found in namespace 'DER': Failed to find the attribute in DER # # These tests work