From: Alan T. DeKok Date: Wed, 23 Oct 2019 19:58:52 +0000 (-0400) Subject: fr_pair_afrom_da() now copies unknown da's X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9b437be08a05e1e71b6590dca3eda614fcc5403d;p=thirdparty%2Ffreeradius-server.git fr_pair_afrom_da() now copies unknown da's which means that the caller has to free them. So we check all of the callers of fr_dict_unknown_afrom_fields(), and up them to free the unknown da's. For RADIUS, this means allocating a temporary talloc_ctx. That context is used for allocating all unknown da's. Then, then fr_radius_decode_pair() is done, it frees the tmp_ctx. That pattern means we don't have to explicitly free the da's in every function, and also that the unknown da's are freed relatively quickly. --- diff --git a/src/lib/util/pair.c b/src/lib/util/pair.c index 498d154967a..492108cc969 100644 --- a/src/lib/util/pair.c +++ b/src/lib/util/pair.c @@ -109,6 +109,17 @@ VALUE_PAIR *fr_pair_afrom_da(TALLOC_CTX *ctx, fr_dict_attr_t const *da) return NULL; } + /* + * If we get passed an unknown da, we need to ensure that + * it's parented by "vp". + */ + if (da->flags.is_unknown) { + fr_dict_attr_t const *unknown; + + unknown = fr_dict_unknown_acopy(vp, da); + da = unknown; + } + /* * Use the 'da' to initialize more fields. */ @@ -165,7 +176,7 @@ alloc: /* * Ensure that the DA is parented by the VP. */ - da = fr_dict_unknown_afrom_fields(ctx, fr_dict_root(fr_dict_internal), vendor, attr); + da = fr_dict_unknown_afrom_fields(vp, fr_dict_root(fr_dict_internal), vendor, attr); if (!da) { talloc_free(vp); return NULL; @@ -201,6 +212,7 @@ alloc: VALUE_PAIR *fr_pair_afrom_child_num(TALLOC_CTX *ctx, fr_dict_attr_t const *parent, unsigned int attr) { fr_dict_attr_t const *da; + VALUE_PAIR *vp; da = fr_dict_attr_child_by_num(parent, attr); if (!da) { @@ -220,7 +232,9 @@ VALUE_PAIR *fr_pair_afrom_child_num(TALLOC_CTX *ctx, fr_dict_attr_t const *paren if (!da) return NULL; } - return fr_pair_afrom_da(ctx, da); + vp = fr_pair_afrom_da(ctx, da); + fr_dict_unknown_free(&da); + return vp; } /** Deserialise a value pair from a string diff --git a/src/lib/util/struct.c b/src/lib/util/struct.c index ecc27e7e4e6..10b37b18713 100644 --- a/src/lib/util/struct.c +++ b/src/lib/util/struct.c @@ -41,7 +41,8 @@ VALUE_PAIR *fr_unknown_from_network(TALLOC_CTX *ctx, fr_dict_attr_t const *paren fr_dict_vendor_num_by_da(parent), parent->attr); if (!child) return NULL; - vp = fr_pair_afrom_da(ctx, child); + vp = fr_pair_afrom_da(ctx, child); /* makes a copy of 'child' */ + fr_dict_unknown_free(&child); if (!vp) return NULL; if (fr_value_box_from_network(vp, &vp->data, vp->da->type, vp->da, data, data_len, true) < 0) { diff --git a/src/protocols/radius/base.c b/src/protocols/radius/base.c index 0d9b1a3db75..ea78d655412 100644 --- a/src/protocols/radius/base.c +++ b/src/protocols/radius/base.c @@ -1057,6 +1057,7 @@ ssize_t fr_radius_decode(TALLOC_CTX *ctx, uint8_t const *packet, size_t packet_l uint8_t const *attr, *end; fr_radius_ctx_t packet_ctx; + packet_ctx.tmp_ctx = talloc_init("tmp"); packet_ctx.secret = secret; packet_ctx.vector = original + 4; @@ -1071,20 +1072,28 @@ ssize_t fr_radius_decode(TALLOC_CTX *ctx, uint8_t const *packet, size_t packet_l */ while (attr < end) { slen = fr_radius_decode_pair(ctx, &cursor, dict_radius, attr, (end - attr), &packet_ctx); - if (slen < 0) return slen; + if (slen < 0) { + talloc_free(packet_ctx.tmp_ctx); + return slen; + } /* * If slen is larger than the room in the packet, * all kinds of bad things happen. */ - if (!fr_cond_assert(slen <= (end - attr))) return -1; + if (!fr_cond_assert(slen <= (end - attr))) { + talloc_free(packet_ctx.tmp_ctx); + return -1; + } attr += slen; + talloc_free_children(packet_ctx.tmp_ctx); } /* * We've parsed the whole packet, return that. */ + talloc_free(packet_ctx.tmp_ctx); return packet_len; } diff --git a/src/protocols/radius/decode.c b/src/protocols/radius/decode.c index b2c271722fe..9163a82b170 100644 --- a/src/protocols/radius/decode.c +++ b/src/protocols/radius/decode.c @@ -419,6 +419,7 @@ ssize_t fr_radius_decode_tlv(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t con fr_dict_attr_t const *child; VALUE_PAIR *head = NULL; fr_cursor_t tlv_cursor; + fr_radius_ctx_t *packet_ctx = decoder_ctx; if (data_len < 3) return -1; /* type, length, value */ @@ -435,21 +436,18 @@ ssize_t fr_radius_decode_tlv(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t con child = fr_dict_attr_child_by_num(parent, p[0]); if (!child) { - fr_dict_attr_t const *unknown_child; - FR_PROTO_TRACE("Failed to find child %u of TLV %s", p[0], parent->name); /* * Build an unknown attr */ - unknown_child = fr_dict_unknown_afrom_fields(ctx, parent, - fr_dict_vendor_num_by_da(parent), p[0]); - if (!unknown_child) { + child = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, + fr_dict_vendor_num_by_da(parent), p[0]); + if (!child) { error: fr_pair_list_free(&head); return -1; } - child = unknown_child; } FR_PROTO_TRACE("decode context changed %s -> %s", parent->name, child->name); @@ -478,6 +476,7 @@ static ssize_t decode_vsa_internal(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict unsigned int attribute; ssize_t attrlen, my_len; fr_dict_attr_t const *da; + fr_radius_ctx_t *packet_ctx = decoder_ctx; /* * Parent must be a vendor @@ -541,7 +540,7 @@ static ssize_t decode_vsa_internal(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict * See if the VSA is known. */ da = fr_dict_attr_child_by_num(parent, attribute); - if (!da) da = fr_dict_unknown_afrom_fields(ctx, parent, dv->pen, attribute); + if (!da) da = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, dv->pen, attribute); if (!da) return -1; FR_PROTO_TRACE("decode context changed %s -> %s", da->parent->name, da->name); @@ -669,6 +668,7 @@ static ssize_t decode_wimax(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t cons uint8_t *head, *tail; uint8_t const *attr, *end; fr_dict_attr_t const *da; + fr_radius_ctx_t *packet_ctx = decoder_ctx; /* * data = VID VID VID VID WiMAX-Attr WiMAX-Len Continuation ... @@ -686,7 +686,7 @@ static ssize_t decode_wimax(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t cons if (((size_t) (data[5] + 4)) != attr_len) return -1; da = fr_dict_attr_child_by_num(parent, data[4]); - if (!da) da = fr_dict_unknown_afrom_fields(ctx, parent, vendor, data[4]); + if (!da) da = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, vendor, data[4]); if (!da) return -1; FR_PROTO_TRACE("decode context changed %s -> %s", da->parent->name, da->name); @@ -696,7 +696,8 @@ static ssize_t decode_wimax(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t cons if ((data[6] & 0x80) == 0) { rcode = fr_radius_decode_pair_value(ctx, cursor, dict, da, data + 7, data[5] - 3, data[5] - 3, decoder_ctx); - if (rcode < 0) return -1; + if (rcode < 0) return rcode; + return attr_len; } @@ -836,6 +837,7 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t const fr_dict_vendor_t my_dv; fr_dict_attr_t const *vendor_da; fr_cursor_t tlv_cursor; + fr_radius_ctx_t *packet_ctx = decoder_ctx; /* * Container must be a VSA @@ -870,7 +872,7 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t const return -1; } - if (fr_dict_unknown_vendor_afrom_num(ctx, &n, parent, vendor) < 0) return -1; + if (fr_dict_unknown_vendor_afrom_num(packet_ctx->tmp_ctx, &n, parent, vendor) < 0) return -1; vendor_da = n; /* @@ -885,14 +887,14 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t const dv = &my_dv; goto create_attrs; - } else { - /* - * We found an attribute representing the vendor - * so it *MUST* exist in the vendor tree. - */ - dv = fr_dict_vendor_by_num(dict, vendor); - if (!fr_cond_assert(dv)) return -1; } + + /* + * We found an attribute representing the vendor + * so it *MUST* exist in the vendor tree. + */ + dv = fr_dict_vendor_by_num(dict, vendor); + if (!fr_cond_assert(dv)) return -1; FR_PROTO_TRACE("decode context %s -> %s", parent->name, vendor_da->name); /* @@ -934,7 +936,6 @@ create_attrs: if (vsa_len < 0) { fr_strerror_printf("%s: Internal sanity check %d", __FUNCTION__, __LINE__); fr_pair_list_free(&head); - fr_dict_unknown_free(&vendor_da); return -1; } @@ -953,7 +954,6 @@ create_attrs: * attribute and first known attribute was cloned * meaning we can now free the unknown vendor. */ - fr_dict_unknown_free(&vendor_da); /* Only frees unknown vendors */ return total; } @@ -1230,7 +1230,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic * and succeed, instead of failing. So we don't * need to handle that case here. */ - child = fr_dict_unknown_afrom_fields(ctx, parent, 0, p[0]); + child = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, 0, p[0]); if (!child) goto raw; /* @@ -1283,7 +1283,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic * This can be used later by the encoder to rebuild * the attribute header. */ - parent = fr_dict_unknown_afrom_fields(ctx, parent, vendor, p[4]); + parent = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, vendor, p[4]); p += 5; data_len -= 5; break; @@ -1296,7 +1296,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic * fr_dict_unknown_afrom_fields will do the right thing * and only create the unknown attr. */ - parent = fr_dict_unknown_afrom_fields(ctx, parent, vendor, p[4]); + parent = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, vendor, p[4]); p += 5; data_len -= 5; break; @@ -1362,7 +1362,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic * therefore of type "octets", and will be * handled below. */ - parent = fr_dict_unknown_afrom_fields(ctx, parent->parent, + parent = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent->parent, fr_dict_vendor_num_by_da(parent), parent->attr); if (!parent) { fr_strerror_printf("%s: Internal sanity check %d", __FUNCTION__, __LINE__); @@ -1386,10 +1386,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic * information, decode the actual p. */ vp = fr_pair_afrom_da(ctx, parent); - if (!vp) { - if (parent->flags.is_unknown) fr_dict_unknown_free(&parent); - return -1; - } + if (!vp) return -1; vp->tag = tag; switch (parent->type) { @@ -1521,6 +1518,7 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co { ssize_t rcode; fr_dict_attr_t const *da; + fr_radius_ctx_t *packet_ctx = decoder_ctx; if ((data_len < 2) || (data[1] < 2) || (data[1] > data_len)) { fr_strerror_printf("%s: Insufficient data", __FUNCTION__); @@ -1530,7 +1528,7 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co da = fr_dict_attr_child_by_num(fr_dict_root(dict), data[0]); if (!da) { FR_PROTO_TRACE("Unknown attribute %u", data[0]); - da = fr_dict_unknown_afrom_fields(ctx, fr_dict_root(dict), 0, data[0]); + da = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, fr_dict_root(dict), 0, data[0]); } if (!da) return -1; FR_PROTO_TRACE("decode context changed %s -> %s",da->parent->name, da->name); @@ -1541,9 +1539,13 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co if (data_len == 2) { VALUE_PAIR *vp; - if (!fr_dict_root(dict)->flags.is_root) return 2; + if (!fr_dict_root(dict)->flags.is_root) { + return 2; + } - if (data[0] != FR_CHARGEABLE_USER_IDENTITY) return 2; + if (data[0] != FR_CHARGEABLE_USER_IDENTITY) { + return 2; + } /* * Hacks for CUI. The WiMAX spec says that it can be @@ -1556,6 +1558,7 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co */ vp = fr_pair_afrom_da(ctx, da); if (!vp) return -1; + fr_cursor_append(cursor, vp); vp->vp_tainted = true; /* not REALLY necessary, but what the heck */ @@ -1601,6 +1604,7 @@ static int decode_test_ctx(void **out, TALLOC_CTX *ctx) if (fr_radius_init() < 0) return -1; test_ctx = talloc_zero(ctx, fr_radius_ctx_t); + test_ctx->tmp_ctx = talloc(ctx, uint8_t); test_ctx->secret = talloc_strdup(test_ctx, "testing123"); test_ctx->vector = vector; talloc_set_destructor(test_ctx, _test_ctx_free); @@ -1622,6 +1626,7 @@ static ssize_t fr_radius_decode_proto(void *proto_ctx, uint8_t const *data, size rcode = fr_radius_decode(test_ctx, data, packet_len, test_ctx->vector - 4, /* decode adds 4 to this */ test_ctx->secret, talloc_array_length(test_ctx->secret) - 1, &vp); fr_pair_list_free(&vp); + talloc_free_children(test_ctx->tmp_ctx); return rcode; } diff --git a/src/protocols/radius/packet.c b/src/protocols/radius/packet.c index 63bdb7009bf..8d45f721569 100644 --- a/src/protocols/radius/packet.c +++ b/src/protocols/radius/packet.c @@ -168,6 +168,8 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original, return -1; } + packet_ctx.tmp_ctx = talloc(packet, uint8_t); + /* * Extract attribute-value pairs */ @@ -189,6 +191,8 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original, */ my_len = fr_radius_decode_pair(packet, &cursor, dict_radius, ptr, packet_length, &packet_ctx); if (my_len < 0) { + fail: + talloc_free(packet_ctx.tmp_ctx); fr_pair_list_free(&head); return -1; } @@ -212,18 +216,18 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original, if ((max_attributes > 0) && (num_attributes > max_attributes)) { char host_ipaddr[INET6_ADDRSTRLEN]; - fr_pair_list_free(&head); fr_strerror_printf("Possible DoS attack from host %s: Too many attributes in request " "(received %d, max %d are allowed)", inet_ntop(packet->src_ipaddr.af, &packet->src_ipaddr.addr, host_ipaddr, sizeof(host_ipaddr)), num_attributes, max_attributes); - return -1; + goto fail; } ptr += my_len; packet_length -= my_len; + talloc_free_children(packet_ctx.tmp_ctx); } fr_cursor_init(&out, &packet->vps); @@ -236,6 +240,7 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original, * random pool. */ fr_rand_seed(packet->data, RADIUS_HEADER_LENGTH); + talloc_free(packet_ctx.tmp_ctx); return 0; } diff --git a/src/protocols/radius/radius.h b/src/protocols/radius/radius.h index 27624cc44e4..00b482591a3 100644 --- a/src/protocols/radius/radius.h +++ b/src/protocols/radius/radius.h @@ -124,6 +124,7 @@ int fr_radius_packet_send(RADIUS_PACKET *packet, RADIUS_PACKET const *original, void _fr_radius_packet_log_hex(fr_log_t const *log, RADIUS_PACKET const *packet, char const *file, int line) CC_HINT(nonnull); typedef struct { + TALLOC_CTX *tmp_ctx; //!< for temporary things cleaned up during decoding uint8_t const *vector; //!< vector for encryption / decryption of data char const *secret; //!< shared secret. MUST be talloc'd bool tunnel_password_zeros;