From 7ac6e829cbbe761a48309613a85d55fdcf9b420c Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Thu, 27 Feb 2025 19:47:26 -0500 Subject: [PATCH] hoist decode_oid_and_value() to its own function which avoids the sequence / set decoder checking for the flag. rename the is_pair flag to is_oid_and_value, which is clearer. rename other functions to match --- src/protocols/der/base.c | 6 +- src/protocols/der/decode.c | 222 +++++++++++++++++-------------------- src/protocols/der/der.h | 4 +- src/protocols/der/encode.c | 8 +- 4 files changed, 108 insertions(+), 132 deletions(-) diff --git a/src/protocols/der/base.c b/src/protocols/der/base.c index 5efdf9414f..69496804f2 100644 --- a/src/protocols/der/base.c +++ b/src/protocols/der/base.c @@ -294,7 +294,7 @@ static int dict_flag_sequence_of(fr_dict_attr_t **da_p, char const *value, UNUSE } if (strcmp(value, "oid_and_value") == 0) { - flags->is_pair = true; + flags->is_oid_and_value = true; flags->is_sequence_of = true; flags->sequence_of = FR_DER_TAG_SEQUENCE; return fr_dict_attr_set_group(da_p); @@ -328,7 +328,7 @@ static int dict_flag_set_of(fr_dict_attr_t **da_p, char const *value, UNUSED fr_ } if (strcmp(value, "oid_and_value") == 0) { - flags->is_pair = true; + flags->is_oid_and_value = true; flags->is_sequence_of = true; flags->sequence_of = FR_DER_TAG_SEQUENCE; return fr_dict_attr_set_group(da_p); @@ -756,7 +756,7 @@ static bool attr_valid(fr_dict_attr_t *da) * @todo - have a function called from dict_attr_finalize() ? */ #if 0 - if (flags->is_pair) { + if (flags->is_oid_and_value) { fr_dict_attr_t const *ref; fr_assert(da->type == FR_TYPE_GROUP); diff --git a/src/protocols/der/decode.c b/src/protocols/der/decode.c index 4ff7c31642..8b7092ab94 100644 --- a/src/protocols/der/decode.c +++ b/src/protocols/der/decode.c @@ -53,9 +53,6 @@ typedef ssize_t (*fr_der_decode_oid_t)(uint64_t subidentifier, void *uctx, bool static ssize_t fr_der_decode_oid(fr_dbuff_t *in, fr_der_decode_oid_t func, void *uctx) CC_HINT(nonnull); -static ssize_t fr_der_decode_oid_value_pair(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dbuff_t *in, - fr_dict_attr_t const *parent, fr_der_decode_ctx_t *decode_ctx) CC_HINT(nonnull); - static ssize_t fr_der_decode_hdr(fr_dict_attr_t const *parent, fr_dbuff_t *in, uint8_t *tag, size_t *len, fr_der_tag_t expected) CC_HINT(nonnull(2,3,4)); @@ -775,19 +772,6 @@ static ssize_t fr_der_decode_sequence(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_d return -1; } - if (unlikely(flags->is_pair)) { - fr_assert(fr_type_is_group(parent->type)); - - if (unlikely(fr_der_decode_oid_value_pair(vp, &vp->vp_group, &our_in, vp->da, decode_ctx) <= 0)) { - talloc_free(vp); - return -1; - } - - fr_pair_append(out, vp); - - return fr_dbuff_set(in, &our_in); - } - /* * This is a sequence-of, meaning there are restrictions on the types which can be present */ @@ -967,19 +951,6 @@ static ssize_t fr_der_decode_set(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a return -1; } - if (flags->is_pair) { - fr_assert(fr_type_is_group(parent->type)); - - if (unlikely(fr_der_decode_oid_value_pair(vp, &vp->vp_group, &our_in, vp->da, decode_ctx) <= 0)) { - talloc_free(vp); - return -1; - } - - fr_pair_append(out, vp); - - return fr_dbuff_set(in, &our_in); - } - if (flags->is_set_of) { /* * There should only be one child in a "set_of". We can't check this when we load @@ -1718,6 +1689,95 @@ static ssize_t fr_der_decode_oid_wrapper(TALLOC_CTX *ctx, fr_pair_list_t *out, f return slen; } +/** Decode an OID value pair + * + * @param[in] ctx Talloc context + * @param[out] out Output list + * @param[in] parent Parent attribute + * @param[in] in Input buffer + * @param[in] decode_ctx Decode context + * + * @return 0 on success, -1 on failure + */ +static ssize_t fr_der_decode_oid_and_value(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr_t const *parent, + fr_dbuff_t *in, fr_der_decode_ctx_t *decode_ctx) +{ + fr_dbuff_t our_in = FR_DBUFF(in); + fr_dbuff_t oid_in; + fr_der_decode_oid_to_da_ctx_t uctx; + fr_pair_t *vp; + + uint8_t tag; + size_t oid_len; + ssize_t slen; + + FR_PROTO_TRACE("Decoding OID value pair"); + + fr_assert(fr_type_is_group(parent->type)); + + /* + * A very common pattern in DER encoding is to have a sequence of set containing two things: an OID and a + * value, where the OID is used to determine how to decode the value. + * We will be decoding the OID first and then try to find the attribute associated with that OID to then + * decode the value. If no attribute is found, one will be created and the value will be stored as raw + * octets in the attribute. + */ + + if (unlikely((slen = fr_der_decode_hdr(parent, &our_in, &tag, &oid_len, FR_DER_TAG_OID)) <= 0)) { + error: + fr_strerror_printf_push("Failed decoding %s OID header", parent->name); + return slen; + } + + FR_PROTO_TRACE("Attribute %s, tag %u", parent->name, tag); + + vp = fr_pair_afrom_da(ctx, parent); + if (unlikely(vp == NULL)) { + fr_strerror_const_push("Out of memory"); + return -1; + } + + uctx.ctx = vp; + uctx.parent_da = fr_dict_attr_ref(parent); + uctx.parent_list = &vp->vp_group; + + fr_assert(uctx.parent_da != NULL); + + /* + * Limit the OID decoding to the length as given by the OID header. + */ + oid_in = FR_DBUFF(&our_in); + fr_dbuff_set_end(&oid_in, fr_dbuff_current(&oid_in) + oid_len); + + slen = fr_der_decode_oid(&oid_in, fr_der_decode_oid_to_da, &uctx); + if (unlikely(slen <= 0)) goto error; + + /* + * Skip the OID data. + */ + FR_DBUFF_ADVANCE_RETURN(&our_in, oid_len); + + if (unlikely(uctx.parent_da->flags.is_unknown)) { + /* + * This pair is not in the dictionary. + * We will store the value as raw octets. + */ + if (unlikely((slen = fr_der_decode_octetstring(uctx.ctx, uctx.parent_list, uctx.parent_da, &our_in, + decode_ctx)) < 0)) { + fr_strerror_printf_push("Failed decoding %s OID value", parent->name); + return -1; + } + } else if (unlikely((slen = fr_der_decode_pair_dbuff(uctx.ctx, uctx.parent_list, uctx.parent_da, &our_in, + decode_ctx)) < 0)) { + fr_strerror_printf_push("Failed decoding %s OID value", parent->name); + return -1; + } + + fr_pair_append(out, vp); + + return fr_dbuff_set(in, &our_in); +} + static const fr_der_tag_decode_t tag_funcs[FR_DER_TAG_VALUE_MAX] = { [FR_DER_TAG_BOOLEAN] = { .constructed = FR_DER_TAG_PRIMITIVE, .decode = fr_der_decode_boolean }, [FR_DER_TAG_INTEGER] = { .constructed = FR_DER_TAG_PRIMITIVE, .decode = fr_der_decode_integer }, @@ -1751,6 +1811,10 @@ static const fr_der_tag_decode_t type_funcs[FR_TYPE_MAX] = { [FR_TYPE_COMBO_IP_ADDR] = { .constructed = FR_DER_TAG_PRIMITIVE, .decode = fr_der_decode_combo_ip_addr }, }; +static const fr_der_tag_decode_t oid_and_value_func = { + .constructed = FR_DER_TAG_PRIMITIVE, .decode = fr_der_decode_oid_and_value, +}; + /** Decode the tag and length fields of a DER encoded structure * * @param[in] parent Parent attribute @@ -2262,100 +2326,6 @@ static ssize_t fr_der_decode_x509_extensions(TALLOC_CTX *ctx, fr_pair_list_t *ou return fr_dbuff_set(in, fr_dbuff_end(&our_in)); } -/** Decode an OID value pair - * - * @param[in] ctx Talloc context - * @param[out] out Output list - * @param[in] in Input buffer - * @param[in] parent Parent attribute - * @param[in] decode_ctx Decode context - * - * @return 0 on success, -1 on failure - */ -static ssize_t fr_der_decode_oid_value_pair(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dbuff_t *in, - fr_dict_attr_t const *parent, fr_der_decode_ctx_t *decode_ctx) -{ - fr_dbuff_t our_in = FR_DBUFF(in); - fr_dbuff_marker_t marker; - fr_der_decode_oid_to_da_ctx_t uctx; - - uint8_t tag; - size_t len; - ssize_t slen; - - FR_PROTO_TRACE("Decoding OID value pair"); - - fr_assert(fr_type_is_group(parent->type)); - - /* - * A very common pattern in DER encoding is to have a sequence of set containing two things: an OID and a - * value, where the OID is used to determine how to decode the value. - * We will be decoding the OID first and then try to find the attribute associated with that OID to then - * decode the value. If no attribute is found, one will be created and the value will be stored as raw - * octets in the attribute. - */ - - fr_dbuff_marker(&marker, in); - - if (unlikely((slen = fr_der_decode_hdr(parent, &our_in, &tag, &len, FR_DER_TAG_OID)) <= 0)) { - error: - fr_strerror_printf_push("Failed decoding %s OID header", parent->name); - fr_dbuff_marker_release(&marker); - return slen; - } - - FR_PROTO_TRACE("Attribute %s, tag %u", parent->name, tag); - - uctx.ctx = ctx; - uctx.parent_da = fr_dict_attr_ref(parent); - uctx.parent_list = out; - - fr_assert(uctx.parent_da != NULL); - - fr_dbuff_set_end(&our_in, fr_dbuff_current(&our_in) + len); - - FR_PROTO_HEX_DUMP(fr_dbuff_current(&our_in), fr_dbuff_remaining(&our_in), "DER pair value"); - - slen = fr_der_decode_oid(&our_in, fr_der_decode_oid_to_da, &uctx); - if (unlikely(slen <= 0)) goto error; - - /* - * We have the attribute associated with the OID - * We will now decode the value. - * - * We will advance the buffer to the end of the OID, and then reuse the our_in buffer to decode the value. - * This looks strange in the code, but it is necessary to reset the end restrictions on the our_in buffer - * which were set to avoid overreading the buffer when decoding the OID. - */ - fr_dbuff_set(in, &our_in); - - our_in = FR_DBUFF(in); - - FR_PROTO_HEX_DUMP(fr_dbuff_current(&our_in), fr_dbuff_remaining(&our_in), "DER pair value"); - - if (unlikely(uctx.parent_da->flags.is_unknown)) { - /* - * This pair is not in the dictionary - * We will store the value as raw octets - */ - if (unlikely(slen = fr_der_decode_octetstring(uctx.ctx, uctx.parent_list, uctx.parent_da, &our_in, - decode_ctx) < 0)) { - fr_strerror_printf_push("Failed decoding %s OID value", parent->name); - goto error; - } - } else if (unlikely(slen = fr_der_decode_pair_dbuff(uctx.ctx, uctx.parent_list, uctx.parent_da, &our_in, - decode_ctx) <= 0)) { - fr_strerror_printf_push("Failed decoding %s OID value", parent->name); - goto error; - } - - fr_dbuff_set(in, &our_in); - - FR_PROTO_HEX_DUMP(fr_dbuff_current(&our_in), fr_dbuff_remaining(&our_in), "DER pair value"); - - return fr_dbuff_marker_release_behind(&marker); -} - static ssize_t fr_der_decode_string(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr_t const *parent, fr_dbuff_t *in, bool const allowed_chars[], UNUSED fr_der_decode_ctx_t *decode_ctx) { @@ -2615,6 +2585,12 @@ static ssize_t fr_der_decode_pair_dbuff(TALLOC_CTX *ctx, fr_pair_list_t *out, fr * min/max is the number of elements, NOT the number of bytes. The set / sequence * decoder has to validate its input. */ + + /* + * If the sequence or set is an OID Value pair, then we decode it with the special OID + * Value decoder. + */ + if (flags->is_oid_and_value) func = &oid_and_value_func; break; /* diff --git a/src/protocols/der/der.h b/src/protocols/der/der.h index b74adc1ad5..70bc97c1af 100644 --- a/src/protocols/der/der.h +++ b/src/protocols/der/der.h @@ -107,7 +107,7 @@ typedef struct { bool optional : 1; //!< optional, we MUST already have set 'option' bool is_sequence_of : 1; //!< sequence_of has been defined bool is_set_of : 1; //!< set_of has been defined - bool is_pair : 1; //!< is OID+value + bool is_oid_and_value : 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; @@ -128,7 +128,7 @@ static inline fr_der_attr_flags_t const *fr_der_attr_flags(fr_dict_attr_t const #define fr_der_flag_set_of(_da) (fr_der_attr_flags(_da)->set_of) #define fr_der_flag_is_set_of(_da) (fr_der_attr_flags(_da)->is_set_of) #define fr_der_flag_max(_da) (fr_der_attr_flags(_da)->max) -#define fr_der_flag_is_pair(_da) (fr_der_attr_flags(_da)->is_pair) +#define fr_der_flag_is_oid_and_value(_da) (fr_der_attr_flags(_da)->is_oid_and_value) #define fr_der_flag_is_extensions(_da) (fr_der_attr_flags(_da)->is_extensions) #define fr_der_flag_has_default(_da) (fr_der_attr_flags(_da)->has_default) #define fr_der_flag_is_oid_leaf(_da) (fr_der_attr_flags(_da)->is_oid_leaf) diff --git a/src/protocols/der/encode.c b/src/protocols/der/encode.c index 725badc7d7..e39f536493 100644 --- a/src/protocols/der/encode.c +++ b/src/protocols/der/encode.c @@ -63,7 +63,7 @@ typedef struct { } fr_der_tag_encode_t; -static ssize_t fr_der_encode_oid_value_pair(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, fr_der_encode_ctx_t *encode_ctx) CC_HINT(nonnull); +static ssize_t fr_der_encode_oid_and_value(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, fr_der_encode_ctx_t *encode_ctx) CC_HINT(nonnull); /* * We have per-type function names to make it clear that different types have different encoders. @@ -755,8 +755,8 @@ static ssize_t fr_der_encode_sequence(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, f /* * Groups could be also be a pair, so we need to check for that. */ - if (fr_der_flag_is_pair(vp->da)) { - slen = fr_der_encode_oid_value_pair(&our_dbuff, cursor, encode_ctx); + if (fr_der_flag_is_oid_and_value(vp->da)) { + slen = fr_der_encode_oid_and_value(&our_dbuff, cursor, encode_ctx); if (slen < 0) { fr_strerror_printf("Failed to encode OID value pair: %s", fr_strerror()); return -1; @@ -1482,7 +1482,7 @@ static ssize_t fr_der_encode_X509_extensions(fr_dbuff_t *dbuff, fr_dcursor_t *cu return fr_dbuff_set(dbuff, &our_dbuff); } -static ssize_t fr_der_encode_oid_value_pair(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, fr_der_encode_ctx_t *encode_ctx) +static ssize_t fr_der_encode_oid_and_value(fr_dbuff_t *dbuff, fr_dcursor_t *cursor, fr_der_encode_ctx_t *encode_ctx) { fr_dbuff_t our_dbuff = FR_DBUFF(dbuff); fr_sbuff_t oid_sbuff; -- 2.47.2