From e2745d741ec11f395d41c0aafa24df9dec136399 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Wed, 24 Feb 2016 12:45:57 +0100 Subject: ieee80211: fix deep recursion Restrict the list of possible (sub)elements to avoid deep recursion. Bug: 11824 Bug: 12187 Change-Id: I12deb9956c6ba9b6113cf45da4ee919e33ff8567 Reviewed-on: https://code.wireshark.org/review/14114 Reviewed-by: Peter Wu Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Michael Mann --- epan/dissectors/packet-capwap.c | 2 +- epan/dissectors/packet-ieee80211.c | 60 ++++++++++++++++++++++++++++++-------- epan/dissectors/packet-ieee80211.h | 4 ++- 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/epan/dissectors/packet-capwap.c b/epan/dissectors/packet-capwap.c index 565d800c20..0eed93d43f 100644 --- a/epan/dissectors/packet-capwap.c +++ b/epan/dissectors/packet-capwap.c @@ -2722,7 +2722,7 @@ hf_capwap_msg_element_type_ieee80211_ie_flags, ett_capwap_ieee80211_ie_flags, ie offset += 1; while (offset < offset_end) { - offset += add_tagged_field(pinfo, sub_msg_element_type_tree, tvb, offset, 0); + offset += add_tagged_field(pinfo, sub_msg_element_type_tree, tvb, offset, 0, NULL, 0); } break; diff --git a/epan/dissectors/packet-ieee80211.c b/epan/dissectors/packet-ieee80211.c index 98fd024bf8..642655ce33 100644 --- a/epan/dissectors/packet-ieee80211.c +++ b/epan/dissectors/packet-ieee80211.c @@ -11585,6 +11585,7 @@ dissect_ric_data(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset guint8 desc_cnt = 0; guint32 next_ie; int offset_r = 0; + const guint8 ids[] = { TAG_RIC_DESCRIPTOR }; if (tag_len != 4) { expert_add_info_format(pinfo, ti_len, &ei_ieee80211_tag_length, @@ -11621,7 +11622,7 @@ dissect_ric_data(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset next_ie = tvb_get_guint8(tvb,offset); proto_item_append_text(ti, " :(%d:%s)", desc_cnt,val_to_str_ext(next_ie, &tag_num_vals_ext, "Reserved (%d)")); /* Recursive call to avoid duplication of code*/ - offset_r = add_tagged_field(pinfo, sub_tree, tvb, offset, ftype); + offset_r = add_tagged_field(pinfo, sub_tree, tvb, offset, ftype, ids, G_N_ELEMENTS(ids)); if (offset_r == 0 )/* should never happen, returns a min of 2*/ break; /* This will ensure that the IE after RIC is processed @@ -11745,6 +11746,8 @@ dissect_channel_switch_wrapper(packet_info *pinfo, proto_tree *tree, tvbuff_t *t guint32 tag_len) { int tmp_sublen; + const guint8 ids[] = { TAG_COUNTRY_INFO, TAG_WIDE_BW_CHANNEL_SWITCH, + TAG_VHT_TX_PWR_ENVELOPE }; /* Decode three subelement in IE-196(Channel Switch Wrapper element): @@ -11753,12 +11756,12 @@ dissect_channel_switch_wrapper(packet_info *pinfo, proto_tree *tree, tvbuff_t *t (3) New VHT Transmit Power Envelope subelement */ while (tag_len > 0){ - tmp_sublen = tvb_get_guint8(tvb, offset + 1); - if(add_tagged_field(pinfo, tree, tvb, offset, 0) == 0){ - break; - } - tag_len -= (tmp_sublen + 2); - offset += (tmp_sublen + 2); + tmp_sublen = tvb_get_guint8(tvb, offset + 1); + if(add_tagged_field(pinfo, tree, tvb, offset, 0, ids, G_N_ELEMENTS(ids)) == 0){ + break; + } + tag_len -= (tmp_sublen + 2); + offset += (tmp_sublen + 2); } return offset; } @@ -12139,6 +12142,10 @@ static int dissect_tfs_request(packet_info *pinfo, proto_tree *tree, int ftype) { int end = offset + tag_len; + const guint8 ids[] = { + 1, /* TFS Subelement */ + TAG_VENDOR_SPECIFIC_IE + }; proto_tree_add_item(tree, hf_ieee80211_tag_tfs_request_id, tvb, offset, 1, ENC_LITTLE_ENDIAN); @@ -12176,7 +12183,8 @@ static int dissect_tfs_request(packet_info *pinfo, proto_tree *tree, s_offset = offset; s_end = offset + len; while (s_offset < s_end) { - int tlen = add_tagged_field(pinfo, tree, tvb, s_offset, ftype); + /* TODO 1 is interpreted as TAG_SUPP_RATES, fix this! */ + int tlen = add_tagged_field(pinfo, tree, tvb, s_offset, ftype, ids, G_N_ELEMENTS(ids)); if (tlen==0) break; s_offset += tlen; @@ -12216,6 +12224,11 @@ static int dissect_tfs_response(packet_info *pinfo, proto_tree *tree, int ftype) { int end = offset + tag_len; + const guint8 ids[] = { + 1, /* TFS Status subelement*/ + 2, /* TFS subelement */ + TAG_VENDOR_SPECIFIC_IE + }; while (offset + 3 <= end) { guint8 id, len; @@ -12245,7 +12258,8 @@ static int dissect_tfs_response(packet_info *pinfo, proto_tree *tree, s_offset = offset; s_end = offset + len; while (s_offset < s_end) { - int tlen = add_tagged_field(pinfo, tree, tvb, s_offset, ftype); + /* TODO Element IDs 1 and 2 are misinterpreted! */ + int tlen = add_tagged_field(pinfo, tree, tvb, s_offset, ftype, ids, G_N_ELEMENTS(ids)); if (tlen==0) break; s_offset += tlen; @@ -13929,7 +13943,8 @@ ieee80211_tag_fh_hopping_table(packet_info *pinfo, proto_tree *tree, } int -add_tagged_field(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset, int ftype) +add_tagged_field(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset, int ftype, + const guint8 *valid_element_ids, guint valid_element_ids_count) { guint32 oui; tvbuff_t *tag_tvb; @@ -13965,6 +13980,25 @@ add_tagged_field(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset "Tag Length is longer than remaining payload"); } + /* If the list of valid elements is restricted, require an Element ID to be + * present in that list. Otherwise stop decoding the value to prevent possible + * infinite recursions due to unexpected elements. */ + if (valid_element_ids_count) { + gboolean valid_tag_no; + guint i; + + for (i = 0; i < valid_element_ids_count; i++) { + valid_tag_no = valid_element_ids[i] == tag_no; + if (valid_tag_no) + break; + } + if (!valid_tag_no) { + expert_add_info_format(pinfo, ti_tag, &ei_ieee80211_tag_number, + "Unexpected Element ID %d", tag_no); + goto end_of_tag; + } + } + switch (tag_no) { case TAG_SSID: offset += ieee80211_tag_ssid(pinfo, tree, ti, ti_len, tag_len, tvb, @@ -16245,6 +16279,7 @@ add_tagged_field(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset /* TODO: add Expert info to indicate there is unknown data ! but all tagged option don't yet return offset. For the moment, this code only remove Clang Warnings about not used offset... */ } +end_of_tag: return tag_len + 1 + 1; } @@ -16255,7 +16290,8 @@ ieee_80211_add_tagged_parameters (tvbuff_t *tvb, int offset, packet_info *pinfo, int next_len; beacon_padding = 0; /* this is for the beacon padding confused with ssid fix */ while (tagged_parameters_len > 0) { - if ((next_len=add_tagged_field (pinfo, tree, tvb, offset, ftype)) == 0) + /* TODO make callers optionally specify the list of valid IE IDs? */ + if ((next_len=add_tagged_field (pinfo, tree, tvb, offset, ftype, NULL, 0)) == 0) break; if (next_len > tagged_parameters_len) { /* XXX - flag this as an error? */ @@ -27165,7 +27201,7 @@ proto_register_ieee80211 (void) { &ei_ieee80211_inv_val, { "ieee80211.invalid_value", PI_MALFORMED, PI_WARN, "Invalid value", EXPFILL }}, - { &ei_ieee80211_tag_number, { "wlan_mgt.tag.number.unexpected_ie", PI_MALFORMED, PI_ERROR, "Unexpected IE (expected Advertisement Protocol)", EXPFILL }}, + { &ei_ieee80211_tag_number, { "wlan_mgt.tag.number.unexpected_ie", PI_MALFORMED, PI_ERROR, "Unexpected Information Element ID", EXPFILL }}, { &ei_ieee80211_tag_length, { "wlan_mgt.tag.length.bad", PI_MALFORMED, PI_ERROR, "Bad tag length", EXPFILL }}, { &ei_ieee80211_extra_data, { "ieee80211.extra_data", PI_MALFORMED, PI_WARN, "Unexpected extra data in the end", EXPFILL }}, { &ei_ieee80211_ff_anqp_capability, { "wlan_mgt.fixed.anqp.capability.invalid", PI_MALFORMED, PI_ERROR, "Invalid vendor-specific ANQP capability", EXPFILL }}, diff --git a/epan/dissectors/packet-ieee80211.h b/epan/dissectors/packet-ieee80211.h index d441f52a9d..4e4acb43f2 100644 --- a/epan/dissectors/packet-ieee80211.h +++ b/epan/dissectors/packet-ieee80211.h @@ -50,7 +50,9 @@ void dissect_wifi_display_ie(packet_info *pinfo, proto_tree *tree, tvbuff_t *tvb, int offset, gint size); int add_tagged_field(packet_info *pinfo, proto_tree *tree, - tvbuff_t *tvb, int offset, int ftype); + tvbuff_t *tvb, int offset, int ftype, + const guint8 *valid_element_ids, + guint valid_element_ids_count); #define MAX_SSID_LEN 32 #define MAX_PROTECT_LEN 10 -- cgit v1.2.3