osmocom
/
wireshark
14
0
Fork 1
Code Issues Pull Requests Projects Releases Wiki Activity

radius: fix use-after-free after recent memleak fixes 

The same data is referenced by the ID-to-name and name-to-ID mapping, so
be make sure that the ID mapping is responsible (as the name mapping is
just used for duplicate detection and while parsing dictionary files).

Still to be done is fixing duplicate attribute numbers (by adding
support for OIDs and changing TLV attribute type IDs to OIDs) and fixing
duplicate attribute names (by prefixing the Vendor Names to them).
Also not handled is fixing Value memleaks.

Reproducers of the crash under ASAN:

    tshark -G fields >/dev/null
    tshark -r radius-ms-mppe-etrl-bug.cap   (from bug 796)

Change-Id: Ifa4055901072bc830e19fe06937af67ce524a3be
Fixes: v2.3.0rc0-2536-gd4cf57100c ("Free radius dissector memory on shutdown")
Reviewed-on: https://code.wireshark.org/review/20307
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Michael Mann <mmann78@netscape.net>
This commit is contained in:
Peter Wu 2017-02-27 23:47:11 +01:00 committed by Michael Mann
parent 87b7242e69
commit 3c6900f31f
3 changed files with 25 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
epan/dissectors/packet-radius.c
View File

@ -1506,7 +1506,11 @@ dissect_attribute_value_pairs(proto_tree *tree, packet_info *pinfo, tvbuff_t *tv
avp_vsa_len -= avp_vsa_header_len;
dictionary_entry = (radius_attr_info_t *)g_hash_table_lookup(vendor->attrs_by_id, GUINT_TO_POINTER(avp_vsa_type));
if (vendor->attrs_by_id) {
dictionary_entry = (radius_attr_info_t *)g_hash_table_lookup(vendor->attrs_by_id, GUINT_TO_POINTER(avp_vsa_type));
} else {
dictionary_entry = NULL;
}
if (!dictionary_entry) {
dictionary_entry = &no_dictionary_entry;
@ -2639,8 +2643,6 @@ register_radius_fields(const char *unused _U_)
expert_radius = expert_register_protocol(proto_radius);
expert_register_field_array(expert_radius, ei, array_length(ei));
no_vendor.attrs_by_id = g_hash_table_new(g_direct_hash, g_direct_equal);
/*
* Handle attributes that have a special format.
*/
@ -2695,13 +2697,16 @@ proto_register_radius(void)
proto_register_prefix("radius", register_radius_fields);
dict = (radius_dictionary_t *)g_malloc(sizeof(radius_dictionary_t));
/*
* IDs map to names and vice versa. The attribute and vendor is stored
* only once, but referenced by both name and ID mappings.
* See also radius_dictionary_t in packet-radius.h
*/
dict->attrs_by_id = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, free_radius_attr_info);
dict->attrs_by_name = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, free_radius_attr_info);
dict->attrs_by_name = g_hash_table_new(g_str_hash, g_str_equal);
dict->vendors_by_id = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, free_radius_vendor_info);
/* Both vendors_by_id and vendors_by_name share the same data, so only worry about
cleaning up the data from one of them. The other will just clean up its own hash entries */
dict->vendors_by_name = g_hash_table_new(g_str_hash, g_str_equal);
dict->tlvs_by_name = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, free_radius_attr_info);
dict->tlvs_by_name = g_hash_table_new(g_str_hash, g_str_equal);
radius_calls = wmem_map_new_autoreset(wmem_epan_scope(), wmem_file_scope(), radius_call_hash, radius_call_equal);

12
epan/dissectors/packet-radius.h
View File

@ -108,15 +108,23 @@ struct _radius_attr_info_t {
int hf_alt; /* 64-bit version for integers, encrypted version for strings, IPv6 for radius_combo_ip */
int hf_tag;
int hf_len;
GHashTable* tlvs_by_id;
GHashTable* tlvs_by_id; /**< Owns the data (see also radius_dictionary_t). */
};
/*
* Attributes and Vendors are a mapping between IDs and names. Names
* are normally uniquely identified by a number. Identifiers for
* Vendor-Specific Attributes (VSA) are scoped within the vendor.
*
* The attribute/vendor structures are owned by the by_id tables,
* the by_name tables point to the same data.
*/
typedef struct _radius_dictionary_t {
GHashTable* attrs_by_id;
GHashTable* attrs_by_name;
GHashTable* vendors_by_id;
GHashTable* vendors_by_name;
GHashTable* tlvs_by_name;
GHashTable* tlvs_by_name; /**< Used for debugging duplicate assignments, does not own the data. */
} radius_dictionary_t;
radius_attr_dissector_t radius_integer;

6
epan/radius_dict.l
View File

@ -523,10 +523,10 @@ static gboolean add_attribute(Radius_scanner_state_t* state, const gchar* name,
*/
if (g_strcmp0(a->name, name) != 0) {
/*
* Yes. Remove the entry from the by-name hash table
* Yes. Steal the entry from the by-name hash table
* and re-insert it with the new name.
*/
g_hash_table_remove(state->dict->attrs_by_name, (gpointer) (a->name));
g_hash_table_steal(state->dict->attrs_by_name, (gpointer) (a->name));
g_free((gpointer) a->name);
a->name = g_strdup(name);
g_hash_table_insert(state->dict->attrs_by_name, (gpointer) (a->name),a);
@ -554,7 +554,7 @@ static gboolean add_tlv(Radius_scanner_state_t* state, const gchar* name, const
if (! a->tlvs_by_id) {
a->tlvs_by_id = g_hash_table_new(g_direct_hash,g_direct_equal);
a->tlvs_by_id = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, free_radius_attr_info);
}
code = (guint32) strtoul(codestr, NULL, 10);