From 9e33399c7b47fde08b1371cfec843c31098ecc80 Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Sat, 22 Feb 2025 14:02:18 -0500 Subject: [PATCH] add 'size=MIN..MAX' and check it in more places --- share/dictionary/der/dictionary.oids | 2 +- src/protocols/der/base.c | 90 +++++++++++++++++++++++++++- src/protocols/der/decode.c | 55 +++++++++++++---- src/protocols/der/der.h | 5 +- 4 files changed, 136 insertions(+), 16 deletions(-) diff --git a/share/dictionary/der/dictionary.oids b/share/dictionary/der/dictionary.oids index 042787375a..fde1870e1f 100644 --- a/share/dictionary/der/dictionary.oids +++ b/share/dictionary/der/dictionary.oids @@ -36,7 +36,7 @@ ATTRIBUTE ds 2.5 sequence ATTRIBUTE attributeType 2.5.4 sequence ATTRIBUTE commonName 2.5.4.3 printablestring is_oid_leaf -ATTRIBUTE countryName 2.5.4.6 string[2] der_type=printablestring,is_oid_leaf +ATTRIBUTE countryName 2.5.4.6 printablestring size=2,is_oid_leaf ATTRIBUTE serialNumber 2.5.4.5 printablestring is_oid_leaf ATTRIBUTE localityName 2.5.4.7 string is_oid_leaf ATTRIBUTE stateOrProvinceName 2.5.4.8 string is_oid_leaf diff --git a/src/protocols/der/base.c b/src/protocols/der/base.c index a271dced58..b94e6ddfcd 100644 --- a/src/protocols/der/base.c +++ b/src/protocols/der/base.c @@ -342,6 +342,93 @@ static int dict_flag_is_oid_leaf(fr_dict_attr_t **da_p, UNUSED char const *value return 0; } +/* + * size=MIN..MAX + */ +static int dict_flag_size(fr_dict_attr_t **da_p, char const *value, UNUSED fr_dict_flag_parser_rule_t const *rules) +{ + fr_der_attr_flags_t *flags = fr_dict_attr_ext(*da_p, FR_DICT_ATTR_EXT_PROTOCOL_SPECIFIC); + unsigned long num; + char const *p = value; + char *end = NULL; + + if (fr_type_is_leaf((*da_p)->type) && !fr_type_is_variable_size((*da_p)->type)) { + fr_strerror_printf("Cannot use 'size=...' for type '%s'", fr_type_to_str((*da_p)->type)); + return -1; + } + + /* + * size=..max + */ + if ((p[0] == '.') && (p[1] == '.')) goto check_max; + + num = strtoul(p, &end, 10); + if (num == ULONG_MAX) { + invalid: + fr_strerror_printf("Invalid value in 'size=%s'", value); + return -1; + } + + if (num > UINT8_MAX) { + fr_strerror_printf("Invalid value in 'size=%s' - 'min' value is too large", value); + return -1; + } + + /* + * size=4 + * + * Fixed size, but not size=0. + */ + if (!*end) { + if (!num) goto invalid; + + /* + * printablestring size=2 + * + * instead of string[2] der_type=printablestring + */ + if (((*da_p)->type == FR_TYPE_OCTETS) || ((*da_p)->type == FR_TYPE_STRING)) { + (*da_p)->flags.is_known_width = !fr_type_is_structural((*da_p)->type); + (*da_p)->flags.length = num; + return 0; + } + + /* + * Sets and sequences can have a fixed number of elements. + */ + flags->min = flags->max = num; + return 0; + } + + if ((end[0] != '.') || (end[1] != '.')) { + fr_strerror_printf("Invalid value in 'size=%s' - unexpected data after 'min'", value); + return -1; + } + + flags->min = num; + + /* + * size=1.. + * + * Sets the minimum, but not the maximum. + */ + p = end + 2; + if (!*p) return 0; + +check_max: + num = strtoul(p, &end, 10); + if (num == ULONG_MAX) goto invalid; + + if (*end) { + fr_strerror_printf("Invalid value in 'size=%s' - unexpected data after 'max'", value); + return -1; + } + + flags->max = num; + + return 0; +} + static int dict_flag_max(fr_dict_attr_t **da_p, char const *value, UNUSED fr_dict_flag_parser_rule_t const *rules) { fr_der_attr_flags_t *flags = fr_dict_attr_ext(*da_p, FR_DICT_ATTR_EXT_PROTOCOL_SPECIFIC); @@ -349,7 +436,7 @@ static int dict_flag_max(fr_dict_attr_t **da_p, char const *value, UNUSED fr_dic char *end = NULL; num = strtoul(value, &end, 10); - if (*end || !num) { + if (*end || !num || (num == ULONG_MAX)) { fr_strerror_printf("Invalid value in 'max=%s'", value); return -1; } @@ -408,6 +495,7 @@ static const fr_dict_flag_parser_t der_flags[] = { { L("option"), { .func = dict_flag_option} }, { L("sequence_of"), { .func = dict_flag_sequence_of, .needs_value = true } }, { L("set_of"), { .func = dict_flag_set_of, .needs_value = true } }, + { L("size"), { .func = dict_flag_size, .needs_value=true } }, }; static bool type_parse(fr_type_t *type_p,fr_dict_attr_t **da_p, char const *name) diff --git a/src/protocols/der/decode.c b/src/protocols/der/decode.c index 02d7d68ccd..d36b39126a 100644 --- a/src/protocols/der/decode.c +++ b/src/protocols/der/decode.c @@ -1742,7 +1742,7 @@ static ssize_t fr_der_decode_x509_extensions(TALLOC_CTX *ctx, fr_pair_list_t *ou FR_PROTO_TRACE("Attribute %s, tag %u", parent->name, tag); - max = fr_der_flag_max(parent); /* Maximum number of extensions specified in the dictionary*/ + max = fr_der_flag_max(parent); /* Maximum number of extensions specified in the dictionary */ while (fr_dbuff_remaining(&our_in) > 0) { fr_dbuff_t sub_in = FR_DBUFF(&our_in); @@ -2074,7 +2074,6 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr fr_der_tag_decode_t *func; ssize_t slen; uint8_t tag; - uint64_t max; size_t len; fr_der_attr_flags_t const *flags = fr_der_attr_flags(parent); @@ -2124,7 +2123,7 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr * the next header. */ if ((fr_dbuff_extend_lowat(NULL, &our_in, 2) < 2)) { - if (fr_der_flag_has_default(parent)) { + if (flags->has_default) { fr_pair_t *vp = fr_pair_afrom_da(ctx, parent); fr_dict_enum_value_t *ev; @@ -2159,7 +2158,7 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr return 0; } - if (unlikely(fr_der_flag_is_choice(parent))) { + if (unlikely(flags->is_choice)) { slen = fr_der_decode_choice(ctx, out, parent, &our_in, decode_ctx); if (unlikely(slen <= 0)) return slen; @@ -2175,7 +2174,7 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr FR_PROTO_TRACE("Attribute %s, tag %u", parent->name, tag); if (unlikely(tag != FR_DER_TAG_NULL) && (!fr_type_to_der_tag_valid(parent->type, tag) || fr_dbuff_remaining(&our_in) == 0)) { - if (fr_der_flag_has_default(parent)) { + if (flags->has_default) { /* * If the attribute has a default value, we will use that. * We could end up here if we are decoding a sequence which has a default value @@ -2224,7 +2223,7 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr * could have defaults. */ fr_strerror_printf("Attribute %s of DER type '%s' cannot store DER type '%s'", parent->name, - fr_der_tag_to_str(fr_der_flag_der_type(parent)), + fr_der_tag_to_str(flags->der_type), fr_der_tag_to_str(tag)); return -1; } @@ -2248,7 +2247,7 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr return -1; } - if (fr_der_flag_is_extensions(parent)) { + if (flags->is_extensions) { slen = fr_der_decode_x509_extensions(ctx, out, &our_in, parent, decode_ctx); if (slen <= 0) return slen; @@ -2258,19 +2257,51 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr func = &tag_funcs[tag]; /* - * Make sure the data length is less than the maximum allowed + * Enforce limits on min/max. */ switch (tag) { case FR_DER_TAG_SEQUENCE: case FR_DER_TAG_SET: + /* + * min/max is the number of elements, NOT the number of bytes. The set / sequence + * decoder has to validate its input. + */ + break; + + /* + * min/max applies to the decoded values. + */ + case FR_DER_TAG_INTEGER: + case FR_DER_TAG_ENUMERATED: break; default: - max = fr_der_flag_max(parent); - fr_assert(max <= DER_MAX_STR); + /* + * min/max can be fixed width, but we only care for 'octets' and 'string'. + * + * @todo - when we support IP addresses (which DER usually encodes as strings), this + * check will have to be updated. + */ + if (parent->flags.is_known_width) { + if (!fr_type_is_variable_size(parent->type)) break; + + if (len != parent->flags.length) { + fr_strerror_printf("Data length (%zu) is different from expected fixed size (%u)", len, parent->flags.length); + return -1; + } + + break; + } + + if (flags->min && (len < flags->min)) { + fr_strerror_printf("Data length (%zu) is smaller than expected minimum size (%u)", len, flags->min); + return -1; + } + + fr_assert(flags->max <= DER_MAX_STR); /* 'max' is always set in the attr_valid() function */ - if (unlikely(len > max)) { - fr_strerror_printf("Data length (%zu) exceeds max size (%" PRIu64 ")", len, max); + if (unlikely(len > flags->max)) { + fr_strerror_printf("Data length (%zu) exceeds max size (%" PRIu64 ")", len, flags->max); return -1; } break; diff --git a/src/protocols/der/der.h b/src/protocols/der/der.h index 5dab7abf96..92210d33dc 100644 --- a/src/protocols/der/der.h +++ b/src/protocols/der/der.h @@ -91,7 +91,6 @@ typedef enum { #define DER_BOOLEAN_TRUE 0xff //!< DER encoded boolean true value. typedef struct { - uint8_t option; //!< an "attribute number" encoded in the tag field. fr_der_tag_class_t class; //!< tag Class fr_der_tag_t der_type; //!< the DER type, which is different from the FreeRADIUS type union { @@ -99,9 +98,11 @@ typedef struct { fr_der_tag_t set_of; }; uint64_t max; //!< maximum count of items in a sequence, set, or string. + uint8_t min; //!< mininum count + uint8_t option; //!< an "attribute number" encoded in the tag field. bool is_sequence_of : 1; //!< sequence_of has been defined bool is_set_of : 1; //!< set_of has been defined - bool is_pair : 1; + bool is_pair : 1; //!< is OID+value bool is_extensions : 1; //!< a list of X.509 extensions bool has_default : 1; //!< a default value exists bool is_oid_leaf : 1; -- 2.47.3