From 0d2e6033ecad9c1e33239b5976a7c1b356fe4173 Mon Sep 17 00:00:00 2001 From: Guy Harris Date: Wed, 8 Apr 2020 01:19:33 -0700 Subject: [PATCH] Add additional checks, clean up some stuff. Add checks for bad block lengths - either too short or not a multiple of 4. (Yes, the pcapng spec requires it to be a multiple of 4. And there is at least one implementation that requires it.) For various structures with a length field, create the top-level tree field for the item with a "run to the end of the packet" length and, once we're finished dissecting it, set the length to its actual value. Fetch various field values using proto_tree_item_add_uint. Fix some incorrect field types based on errors reported by that. If an end-of-options option has a non-zero length, 1) don't treat it as not an end-of-options option and 2) report an error on its length. Change-Id: I72b2c065f3e3c76d5b71a1cd2ef3c1f497623266 Reviewed-on: https://code.wireshark.org/review/36746 Petri-Dish: Guy Harris Tested-by: Petri Dish Buildbot Reviewed-by: Guy Harris --- epan/dissectors/file-pcapng.c | 138 +++++++++++++++++++++------------- 1 file changed, 85 insertions(+), 53 deletions(-) diff --git a/epan/dissectors/file-pcapng.c b/epan/dissectors/file-pcapng.c index 754127458c..ffc9ef0cd7 100644 --- a/epan/dissectors/file-pcapng.c +++ b/epan/dissectors/file-pcapng.c @@ -135,6 +135,8 @@ static int hf_pcapng_option_data_packet_darwin_dpeb_id = -1; static int hf_pcapng_option_data_packet_darwin_svc_class = -1; static int hf_pcapng_option_data_packet_darwin_edpeb_id = -1; +static expert_field ei_block_length_too_short = EI_INIT; +static expert_field ei_block_length_not_multiple_of_4 = EI_INIT; static expert_field ei_invalid_option_length = EI_INIT; static expert_field ei_invalid_record_length = EI_INIT; @@ -554,10 +556,11 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, proto_item *options_item; proto_tree *option_tree; proto_item *option_item; + proto_item *option_length_item; proto_item *p_item; gint offset = 0; - guint16 option_code; - gint option_length; + guint32 option_code; + guint32 option_length; gint hfj_pcapng_option_code; const guint8 *str = NULL; wmem_strbuf_t *strbuf; @@ -580,10 +583,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, while (tvb_captured_length_remaining(tvb, offset)) { str = NULL; - option_code = tvb_get_guint16(tvb, offset, encoding); - option_length = tvb_get_guint16(tvb, offset + 2, encoding); - - option_item = proto_tree_add_item(options_tree, hf_pcapng_option, tvb, offset, option_length + 2 * 2, ENC_NA); + option_item = proto_tree_add_item(options_tree, hf_pcapng_option, tvb, offset, -1, ENC_NA); option_tree = proto_item_add_subtree(option_item, ett_pcapng_option); switch (block_type) { @@ -619,16 +619,18 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, hfj_pcapng_option_code = hf_pcapng_option_code; } + proto_tree_add_item_ret_uint(option_tree, hfj_pcapng_option_code, tvb, offset, 2, encoding, &option_code); if (vals) proto_item_append_text(option_item, ": %s", val_to_str_const(option_code, vals, "Unknown")); - - proto_tree_add_item(option_tree, hfj_pcapng_option_code, tvb, offset, 2, encoding); offset += 2; - proto_tree_add_item(option_tree, hf_pcapng_option_length, tvb, offset, 2, encoding); + option_length_item = proto_tree_add_item_ret_uint(option_tree, hf_pcapng_option_length, tvb, offset, 2, encoding, &option_length); offset += 2; - if (option_code == 0 && option_length == 0) { + if (option_code == 0) { + if (option_length != 0) + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); + proto_item_set_len(option_item, option_length + 2 * 2); break; } else if (option_code == 1) { proto_tree_add_item_ret_string(option_tree, hf_pcapng_option_data_comment, tvb, offset, option_length, ENC_NA | ENC_UTF_8, wmem_packet_scope(), &str); @@ -667,7 +669,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 4: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -685,7 +687,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 5: if (option_length != 17) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -703,7 +705,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break;; case 6: if (option_length != 6) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -715,7 +717,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 7: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -729,7 +731,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 8: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -759,7 +761,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, guint64 resolution; if (option_length != 1) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -855,7 +857,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, } case 10: if (option_length != 4) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -870,7 +872,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 11: if (option_length == 0) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); break; } @@ -887,7 +889,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 13: if (option_length != 1) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -900,7 +902,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 14: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -930,7 +932,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, switch (option_code) { case 2: if (option_length != 4) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -973,7 +975,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 3: if (option_length != 4) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -987,7 +989,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 4: if (option_length != 16) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1009,7 +1011,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, switch (option_code) { case 2: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1020,7 +1022,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 3: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1031,7 +1033,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 4: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1044,7 +1046,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 5: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1057,7 +1059,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 6: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1070,7 +1072,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 7: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1083,7 +1085,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 8: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1104,7 +1106,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, switch (option_code) { case 2: if (option_length != 4) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1134,7 +1136,7 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, break; case 4: if (option_length != 8) { - proto_tree_add_expert(option_tree, pinfo, &ei_invalid_option_length, tvb, offset, option_length); + expert_add_info(pinfo, option_length_item, &ei_invalid_option_length); offset += option_length; break; } @@ -1202,12 +1204,13 @@ static gint dissect_options(proto_tree *tree, packet_info *pinfo, offset += option_length; } - if (option_length % 4) { + if ((option_length % 4) != 0) { proto_item_set_len(option_item, option_length + 2 * 2 + (4 - option_length % 4)); option_length = 4 - option_length % 4; proto_tree_add_item(option_tree, hf_pcapng_option_padding, tvb, offset, option_length, ENC_NA); offset += option_length; - } + } else + proto_item_set_len(option_item, option_length + 2 * 2); if (str) proto_item_append_text(option_item, " = %s", str); @@ -1256,6 +1259,7 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, { proto_tree *block_tree; proto_item *block_item; + proto_item *block_length_item; proto_tree *block_data_tree; proto_item *block_data_item; proto_item *byte_order_magic_item; @@ -1279,8 +1283,21 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, proto_tree_add_bitmask_with_flags(block_tree, tvb, offset, hf_pcapng_block_type, ett_pcapng_option, hfx_pcapng_block_type, encoding, BMT_NO_APPEND); offset += 4; - proto_tree_add_item(block_tree, hf_pcapng_block_length, tvb, offset, 4, encoding); - block_data_length = tvb_get_guint32(tvb, offset, encoding) - 3 * 4; + block_length_item = proto_tree_add_item_ret_uint(block_tree, hf_pcapng_block_length, tvb, offset, 4, encoding, &block_data_length); + if (block_data_length < 3*4) + expert_add_info(pinfo, block_length_item, &ei_block_length_too_short); + /* + * To quote the current pcapng spec, "Block Total Length (32 bits) ... + * This value MUST be a multiple of 4." + */ + if ((block_data_length % 4) != 0) + expert_add_info(pinfo, block_length_item, &ei_block_length_not_multiple_of_4); + + /* + * Subtract the per-block overhead (block type, block length, trailing + * block length). + */ + block_data_length -= 3*4; offset += 4; block_data_item = proto_tree_add_item(block_tree, hf_pcapng_block_data, tvb, offset, block_data_length, ENC_NA); @@ -1429,10 +1446,11 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, proto_item *records_item; proto_tree *record_tree; proto_item *record_item; + proto_item *record_length_item; gint offset_record_start; gint offset_string_start; - guint16 record_code; - gint record_length; + guint32 record_code; + guint32 record_length; gint string_length; gchar *str = NULL; address addr; @@ -1442,26 +1460,25 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, offset_record_start = offset; while (block_data_length - (offset_record_start - offset) > 0) { - record_code = tvb_get_guint16(tvb, offset, encoding); - record_length = tvb_get_guint16(tvb, offset + 2, encoding); - - record_item = proto_tree_add_item(records_tree, hf_pcapng_record, tvb, offset, record_length + 2 * 2, ENC_NA); + record_item = proto_tree_add_item(records_tree, hf_pcapng_record, tvb, offset, -1, ENC_NA); record_tree = proto_item_add_subtree(record_item, ett_pcapng_record); + proto_tree_add_item_ret_uint(record_tree, hf_pcapng_record_code, tvb, offset, 2, encoding, &record_code); proto_item_append_text(record_item, ": %s", val_to_str_const(record_code, record_code_vals, "Unknown")); - - proto_tree_add_item(record_tree, hf_pcapng_record_code, tvb, offset, 2, encoding); offset += 2; - proto_tree_add_item(record_tree, hf_pcapng_record_length, tvb, offset, 2, encoding); + record_length_item = proto_tree_add_item_ret_uint(record_tree, hf_pcapng_record_length, tvb, offset, 2, encoding, &record_length); offset += 2; - if (record_code == 0 && record_length == 0) { + if (record_code == 0) { + if (record_length != 0) + expert_add_info(pinfo, record_length_item, &ei_invalid_record_length); + proto_item_set_len(record_item, record_length + 2 * 2); break; } else switch (record_code) { case 0x0001: /* IPv4 Record */ if (record_length < 5) { - proto_tree_add_expert(record_tree, pinfo, &ei_invalid_record_length, tvb, offset, record_length); + expert_add_info(pinfo, record_length_item, &ei_invalid_record_length); offset += record_length; break; } @@ -1471,12 +1488,18 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, offset += 4; offset_string_start = offset; - while (offset - offset_string_start < record_length - 4) { + while ((guint)(offset - offset_string_start) < record_length - 4) { string_length = tvb_strnlen(tvb, offset, (offset - offset_string_start) + record_length - 4); if (string_length >= 0) { proto_tree_add_item(record_tree, hf_pcapng_record_name, tvb, offset, string_length + 1, encoding); offset += string_length + 1; } else { + /* + * XXX - flag with an error, as this means we didn't + * see a terminating NUL, but the spec says "zero + * or more zero-terminated UTF-8 strings containing + * the DNS entries for that address". + */ proto_tree_add_item(record_tree, hf_pcapng_record_data, tvb, offset, (record_length - 4) - (offset - offset_string_start), encoding); offset += (record_length - 4) - (offset - offset_string_start); } @@ -1486,7 +1509,7 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, break; case 0x0002: /* IPv6 Record */ if (record_length < 17) { - proto_tree_add_expert(record_tree, pinfo, &ei_invalid_option_length, tvb, offset, record_length); + expert_add_info(pinfo, record_length_item, &ei_invalid_record_length); offset += record_length; break; } @@ -1496,12 +1519,18 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, offset += 16; offset_string_start = offset; - while (offset - offset_string_start < record_length - 16) { + while ((guint)(offset - offset_string_start) < record_length - 16) { string_length = tvb_strnlen(tvb, offset, (offset - offset_string_start) + record_length - 16); if (string_length >= 0) { proto_tree_add_item(record_tree, hf_pcapng_record_name, tvb, offset, string_length + 1, encoding); offset += string_length + 1; } else { + /* + * XXX - flag with an error, as this means we didn't + * see a terminating NUL, but the spec says "zero + * or more zero-terminated UTF-8 strings containing + * the DNS entries for that address". + */ proto_tree_add_item(record_tree, hf_pcapng_record_data, tvb, offset, (record_length - 16) - (offset - offset_string_start), encoding); offset += (record_length - 16) - (offset - offset_string_start); } @@ -1520,7 +1549,8 @@ static gint dissect_block(proto_tree *tree, packet_info *pinfo, tvbuff_t *tvb, record_length = 4 - record_length % 4; proto_tree_add_item(record_tree, hf_pcapng_record_padding, tvb, offset, record_length, ENC_NA); offset += record_length; - } + } else + proto_item_set_len(record_item, record_length + 2 * 2); if (str) proto_item_append_text(record_item, " = %s", str); @@ -1781,7 +1811,7 @@ proto_register_pcapng(void) }, { &hf_pcapng_option_length, { "Length", "pcapng.options.option.length", - FT_INT64, BASE_DEC, NULL, 0x00, + FT_UINT16, BASE_DEC, NULL, 0x00, NULL, HFILL } }, { &hf_pcapng_option_data, @@ -2156,7 +2186,7 @@ proto_register_pcapng(void) }, { &hf_pcapng_record_length, { "Length", "pcapng.records.record.length", - FT_INT64, BASE_DEC, NULL, 0x00, + FT_UINT16, BASE_DEC, NULL, 0x00, NULL, HFILL } }, { &hf_pcapng_record_data, @@ -2217,6 +2247,8 @@ proto_register_pcapng(void) }; static ei_register_info ei[] = { + { &ei_block_length_too_short, { "pcapng.block_length_too_short", PI_PROTOCOL, PI_ERROR, "Block length is < 12 bytes", EXPFILL }}, + { &ei_block_length_not_multiple_of_4, { "pcapng.block_length_too_short", PI_PROTOCOL, PI_ERROR, "Block length is not a multiple of 4", EXPFILL }}, { &ei_invalid_option_length, { "pcapng.invalid_option_length", PI_PROTOCOL, PI_ERROR, "Invalid Option Length", EXPFILL }}, { &ei_invalid_record_length, { "pcapng.invalid_record_length", PI_PROTOCOL, PI_ERROR, "Invalid Record Length", EXPFILL }}, };