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 ef823db109f6d81acf42710dedc74005f21ed552..744ae7d3a5238c1e65631dcd3ce9f095c9fa2681 100644 (file)
--- 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 0048feb801b287f706e49ab6c698f66c2a4efabb..d00da041bc7134c1264f94df8d948d404d9a97ee 100644 (file)
--- 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 72cb20b5b46907765309322e4643758e48f2de16..4b0da9ee9e2277dd7300ba915429c689050a7e47 100644 (file)
--- 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 e852fe8e60139e397061ece40c96f4d8e2155580..e2f4d2c265274d2a671ef99f13c7785665527f20 100644 (file)
--- 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