From: Alan T. DeKok Date: Sun, 23 Feb 2025 01:51:44 +0000 (-0500) Subject: double-check more corner cases X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f267140d7de34b3ca4e05bd83073d99bc64f4f48;p=thirdparty%2Ffreeradius-server.git double-check more corner cases and update dicts to match. --- diff --git a/share/dictionary/der/dictionary.common b/share/dictionary/der/dictionary.common index 1f3aac5d30a..c0f3049b7e0 100644 --- a/share/dictionary/der/dictionary.common +++ b/share/dictionary/der/dictionary.common @@ -21,7 +21,7 @@ ATTRIBUTE directoryName 4 sequence option BEGIN directoryName DEFINE RDNSequence sequence sequence_of=set BEGIN RDNSequence -DEFINE RelativeDistinguishedName set +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeAndValue sequence sequence_of=oid_and_value,ref=OID-Tree END RelativeDistinguishedName @@ -51,7 +51,7 @@ DEFINE Name sequence BEGIN Name DEFINE RDNSequence sequence sequence_of=set BEGIN RDNSequence -DEFINE RelativeDistinguishedName set +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeAndValue sequence sequence_of=oid_and_value,ref=OID-Tree END RelativeDistinguishedName diff --git a/share/dictionary/der/dictionary.crl b/share/dictionary/der/dictionary.crl index 55ca8957970..63d5572a972 100644 --- a/share/dictionary/der/dictionary.crl +++ b/share/dictionary/der/dictionary.crl @@ -10,7 +10,7 @@ BEGIN distributionPointName ATTRIBUTE fullName 0 group ref=GeneralName,der_type=sequence,sequence_of=choice,option ATTRIBUTE nameRelativeToCRLIssuer 1 sequence option BEGIN nameRelativeToCRLIssuer -DEFINE RelativeDistinguishedName set +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeandValue sequence sequence_of=oid_and_value,ref=OID-Tree END RelativeDistinguishedName diff --git a/share/dictionary/der/dictionary.rfc2986 b/share/dictionary/der/dictionary.rfc2986 index d9ac415026e..6affcb534fa 100644 --- a/share/dictionary/der/dictionary.rfc2986 +++ b/share/dictionary/der/dictionary.rfc2986 @@ -11,7 +11,7 @@ DEFINE version integer DEFINE subject sequence BEGIN subject -DEFINE RelativeDistinguishedName set +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeandValue sequence BEGIN AttributeTypeAndValue diff --git a/share/dictionary/der/dictionary.rfc5280 b/share/dictionary/der/dictionary.rfc5280 index 1a9cd628d78..0b2674e51b7 100644 --- a/share/dictionary/der/dictionary.rfc5280 +++ b/share/dictionary/der/dictionary.rfc5280 @@ -16,7 +16,7 @@ DEFINE signature sequence sequence_of=oid_and_value,ref=OID-Tree DEFINE issuer sequence sequence_of=set BEGIN issuer -DEFINE RelativeDistinguishedName set +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeAndValue sequence sequence_of=oid_and_value,ref=OID-Tree END RelativeDistinguishedName @@ -30,7 +30,7 @@ END validity DEFINE subject sequence sequence_of=set BEGIN subject -DEFINE RelativeDistinguishedName set size=1.. +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeandValue sequence sequence_of=oid_and_value,ref=OID-Tree END RelativeDistinguishedName diff --git a/src/protocols/der/decode.c b/src/protocols/der/decode.c index 811552a05bf..1a05690b35f 100644 --- a/src/protocols/der/decode.c +++ b/src/protocols/der/decode.c @@ -874,6 +874,11 @@ static ssize_t fr_der_decode_sequence(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_d if (flags->sequence_of != FR_DER_TAG_CHOICE) { restriction_types[flags->sequence_of] = true; + child = fr_dict_attr_iterate_children(parent, &child); + if (!child) { + fr_strerror_printf("Sequence %s has no children", parent->name); + return -1; + } } else { /* @@ -881,20 +886,22 @@ static ssize_t fr_der_decode_sequence(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_d * This will be a list of the number of choices, starting at 0. */ fr_dict_attr_t const *choices = NULL; + fr_dict_attr_t const *ref; if (fr_type_is_group(parent->type)) { - fr_dict_attr_t const *ref = fr_dict_attr_ref(parent); - - while ((choices = fr_dict_attr_iterate_children(ref, &choices))) { - restriction_types[choices->attr] = true; - } + ref = fr_dict_attr_ref(parent); } else { - while ((choices = fr_dict_attr_iterate_children(parent, &choices))) { - restriction_types[choices->attr] = true; - } + ref = parent; + } + + while ((choices = fr_dict_attr_iterate_children(ref, &choices))) { + restriction_types[choices->attr] = true; } } + /* + * Decode all of the data. + */ while (fr_dbuff_remaining(&our_in) > 0) { ssize_t ret; uint8_t current_tag; @@ -914,18 +921,14 @@ static ssize_t fr_der_decode_sequence(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_d } + /* + * If we have a choice, the children are numbered. + */ if (unlikely(flags->sequence_of == FR_DER_TAG_CHOICE)) { child = fr_dict_attr_child_by_num(parent, current_tag); - - } else if (!child) { - child = fr_dict_attr_iterate_children(parent, &child); + if (!child) child = fr_dict_attr_unknown_raw_afrom_num(decode_ctx->tmp_ctx, parent, current_tag); } - /* - * @todo - we should be iterating over the tags, and comparing them to the children. - */ - if (!child) child = fr_dict_attr_unknown_raw_afrom_num(decode_ctx->tmp_ctx, parent, current_tag); - FR_PROTO_TRACE("decode context %s -> %s", parent->name, child->name); fr_dbuff_set(&our_in, current_marker); @@ -945,7 +948,12 @@ static ssize_t fr_der_decode_sequence(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_d } /* - * @todo - we should be iterating over the tags, and comparing them to the children. + * Decode the children. Since it's not a sequence_of=..., we must have a set of children. The + * children are packed in order. Some may be optional. + * + * We loop over all of the children, because some might have default values. + * + * @todo - implement OPTIONAL flag. */ while ((child = fr_dict_attr_iterate_children(parent, &child))) { ssize_t ret; @@ -962,7 +970,8 @@ static ssize_t fr_der_decode_sequence(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_d /* * Ensure that we grab all of the data. * - * @todo - if there is data left over, decode it as raw octets. + * @todo - if there is data left over, decode it as raw octets. We then also have to keep track + * of the maximum child number, and create unknown attributes starting from the last one. */ if (fr_dbuff_remaining(&our_in)) { FR_PROTO_TRACE("Ignoring extra data in sequence"); @@ -1034,6 +1043,21 @@ static ssize_t fr_der_decode_set(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a */ fr_der_tag_t restriction_type = flags->set_of; + /* + * There should only be one child in a "set_of". We can't check this when we load + * the dictionaries, because there is no "finalize" callback. + * + * @todo - we would need to walk through all of the dictionary attributes, and + * call a new function which would check whether or not the parent had any + * children. And if not, return a load-time error. + */ + child = NULL; + child = fr_dict_attr_iterate_children(parent, &child); + if (!child) { + fr_strerror_printf("Missing child for %s", parent->name); + return -1; + } + while (fr_dbuff_remaining(&our_in) > 0) { fr_dbuff_marker_t current_value_marker; ssize_t ret; @@ -1041,20 +1065,6 @@ static ssize_t fr_der_decode_set(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a uint8_t *current_marker = fr_dbuff_current(&our_in); size_t len; - /* - * There should only be one child in a "set_of". We can't check this when we load - * the dictionaries, because there is no "finalize" callback. - * - * @todo - we would need to walk through all of the dictionary attributes, and - * call a new function which would check whether or not the parent had any - * children. And if not, return a load-time error. - * - * @todo - the assertion !child needs to be a run-time check. - */ - child = NULL; - child = fr_dict_attr_iterate_children(parent, &child); - fr_assert(child); - FR_PROTO_TRACE("decode context %s -> %s", parent->name, child->name); if (unlikely(fr_der_decode_hdr(NULL, &our_in, ¤t_tag, &len) <= 0)) { @@ -1074,29 +1084,32 @@ static ssize_t fr_der_decode_set(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a fr_dbuff_marker(¤t_value_marker, &our_in); + /* + * Ensure that the contents of the tags are sorted. + */ if (previous_tag) { - uint8_t prev_char = 0, curr_char = 0; + uint8_t prev_byte = 0, curr_byte = 0; fr_dbuff_t previous_item = FR_DBUFF(&previous_marker); fr_dbuff_set_end(&previous_item, fr_dbuff_current(&previous_marker) + previous_len); do { - FR_DBUFF_OUT_RETURN(&prev_char, &previous_item); - FR_DBUFF_OUT_RETURN(&curr_char, &our_in); + FR_DBUFF_OUT_RETURN(&prev_byte, &previous_item); + FR_DBUFF_OUT_RETURN(&curr_byte, &our_in); - if (prev_char > curr_char) { + if (prev_byte > curr_byte) { fr_strerror_const("Set tags are not in ascending order"); ret = -1; goto error; } - if (prev_char < curr_char) { + if (prev_byte < curr_byte) { break; } } while (fr_dbuff_remaining(&our_in) > 0 && fr_dbuff_remaining(&previous_item) > 0); - if (prev_char > curr_char && fr_dbuff_remaining(&previous_item) > 0) { + if (prev_byte > curr_byte && fr_dbuff_remaining(&previous_item) > 0) { fr_strerror_const( "Set tags are not in ascending order. Previous item has more data"); ret = -1; @@ -1123,32 +1136,38 @@ static ssize_t fr_der_decode_set(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a } /* - * @todo - We should really be iterating over the tag here, not the child. + * Decode the children. Since it's not a sequence_of=..., we must have a set of children. The + * children are packed in order. Some may be optional. + * + * @todo - implement OPTIONAL flag. */ while ((child = fr_dict_attr_iterate_children(parent, &child))) { ssize_t ret; uint8_t current_tag; - uint8_t *current_marker = fr_dbuff_current(&our_in); FR_PROTO_TRACE("decode context %s -> %s", parent->name, child->name); - /* - * Check that the tag is in ascending order - */ - FR_DBUFF_OUT_RETURN(¤t_tag, &our_in); + if (fr_dbuff_remaining(&our_in)) { + uint8_t *current_ptr = fr_dbuff_current(&our_in); - if (unlikely(current_tag < previous_tag)) { - fr_strerror_const("Set tags are not in ascending order"); - talloc_free(vp); - return -1; - } + /* + * Check that the tag is in ascending order + */ + FR_DBUFF_OUT_RETURN(¤t_tag, &our_in); - previous_tag = current_tag; + if (unlikely(current_tag < previous_tag)) { + fr_strerror_const("Set tags are not in ascending order"); + talloc_free(vp); + return -1; + } - /* - * Reset the buffer to the start of the tag - */ - fr_dbuff_set(&our_in, current_marker); + previous_tag = current_tag; + + /* + * Reset the buffer to the start of the tag + */ + fr_dbuff_set(&our_in, current_ptr); + } /* * A child could have been encoded with zero bytes if it has a default value. @@ -1163,7 +1182,8 @@ static ssize_t fr_der_decode_set(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a /* * Ensure that we grab all of the data. * - * @todo - if there is data left over, decode it as raw octets. + * @todo - if there is data left over, decode it as raw octets. We then also have to keep track + * of the maximum child number, and create unknown attributes starting from the last one. */ if (fr_dbuff_remaining(&our_in)) { FR_PROTO_TRACE("Ignoring extra data in set"); diff --git a/src/tests/unit/protocols/der/dictionary.test b/src/tests/unit/protocols/der/dictionary.test index 5c1388fd400..fe2bb4cbd51 100644 --- a/src/tests/unit/protocols/der/dictionary.test +++ b/src/tests/unit/protocols/der/dictionary.test @@ -6,7 +6,7 @@ DEFINE Certificate-Extensions x509_extensions ref=OID-Tree DEFINE Issuer sequence sequence_of=set BEGIN Issuer -DEFINE RelativeDistinguishedName set +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeAndValue sequence sequence_of=oid_and_value,ref=OID-Tree END RelativeDistinguishedName @@ -14,7 +14,7 @@ END Issuer DEFINE Issuer-Set set set_of=set BEGIN Issuer-Set -DEFINE RelativeDistinguishedName set +DEFINE RelativeDistinguishedName set set_of=sequence,size=1.. BEGIN RelativeDistinguishedName DEFINE AttributeTypeAndValue sequence sequence_of=oid_and_value,ref=OID-Tree END RelativeDistinguishedName @@ -36,12 +36,12 @@ DEFINE Test-Boolean bool DEFINE Test-Integer integer -DEFINE Foo sequence +DEFINE Foo sequence sequence_of=integer BEGIN Foo DEFINE Test-Integer integer END Foo -DEFINE Bar sequence +DEFINE Bar sequence sequence_of=boolean BEGIN Bar DEFINE Test-Boolean bool END Bar