osmith
/
wireshark
forked from osmocom/wireshark
1
0
Fork 0
Code Pull Requests Releases Wiki Activity

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.


(cherry picked from commit 142cfb03ac)
This commit is contained in:
Nardi Ivan 2021-02-25 11:21:18 +00:00 committed by Ivan Nardi
parent 8c997a57da
commit 0de80702bd
1 changed files with 3 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
epan/dissectors/packet-gquic.c
View File

@ -1411,7 +1411,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);
@ -1424,6 +1423,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){
@ -1684,7 +1685,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){
@ -1696,7 +1697,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);