From 142cfb03ac5d0473d70f3e8adeabdc4f4496e953 Mon Sep 17 00:00:00 2001 From: Nardi Ivan Date: Thu, 25 Feb 2021 12:21:18 +0100 Subject: [PATCH] GQUIC: fix parsing of unknown (but valid) tags This commit should be a proper fix for the regression reported in #17250 (7fd71536 is a simple workaround). Such regression has been introduced by b287e716 while fixing the infinite loop reported in #16897. b287e716, while fixing the infinite loop, broke the decoding of perfectly valid tags not yet supported by Wireshark. AFAIK, the root cause of the infinite loop is the overflow of the `offset` variable. Therefore checking for this overflow should be sufficient to avoid the loop. Note that we already check for sensible values for the 'tag_len' variable; we should update `total_tag_len` accordingly. Some words about testing: other than correctly handling unknown but valid tags, it is important that this commit doesn't reintroduce the infinite loop bug. Fortunately #16897 provided a POC trace. Unfortunately, if you revert b287e716, this POC doesn't work anymore in master-3.4 and master branches, but it still triggers the infinite loop in master-3.2 branch. Therefore I have been able to manually check that this MR + the overflow check is enough to avoid the infinite loop bug, at least in master-3.2. Some traffic with unknown but valid tags is available in e2ee14ae03. --- epan/dissectors/packet-gquic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/epan/dissectors/packet-gquic.c b/epan/dissectors/packet-gquic.c index 3a8b6677ba..8432543a0b 100644 --- a/epan/dissectors/packet-gquic.c +++ b/epan/dissectors/packet-gquic.c @@ -1417,7 +1417,6 @@ dissect_gquic_tag(tvbuff_t *tvb, packet_info *pinfo, proto_tree *gquic_tree, gui offset_end = tvb_get_guint32(tvb, offset, ENC_LITTLE_ENDIAN); tag_len = offset_end - tag_offset; - total_tag_len += tag_len; ti_len = proto_tree_add_uint(tag_tree, hf_gquic_tag_length, tvb, offset, 4, tag_len); proto_item_append_text(ti_tag, " (l=%u)", tag_len); proto_item_set_generated(ti_len); @@ -1430,6 +1429,8 @@ dissect_gquic_tag(tvbuff_t *tvb, packet_info *pinfo, proto_tree *gquic_tree, gui expert_add_info(pinfo, ti_len, &ei_gquic_tag_length); } + total_tag_len += tag_len; + proto_tree_add_item(tag_tree, hf_gquic_tag_value, tvb, tag_offset_start + tag_offset, tag_len, ENC_NA); switch(tag){ @@ -1703,7 +1704,7 @@ dissect_gquic_tag(tvbuff_t *tvb, packet_info *pinfo, proto_tree *gquic_tree, gui "Dissector for (Google) QUIC Tag" " %s (%s) code not implemented, Contact" " Wireshark developers if you want this supported", tvb_get_string_enc(wmem_packet_scope(), tvb, offset-8, 4, ENC_ASCII|ENC_NA), val_to_str(tag, tag_vals, "Unknown")); - goto end; + tag_offset += tag_len; break; } if(tag_offset != offset_end){ @@ -1715,7 +1716,6 @@ dissect_gquic_tag(tvbuff_t *tvb, packet_info *pinfo, proto_tree *gquic_tree, gui tag_number--; } - end: if (offset + total_tag_len <= offset) { expert_add_info_format(pinfo, gquic_tree, &ei_gquic_length_invalid, "Invalid total tag length: %u", total_tag_len);