From 1527177cb97071e78b186d70e89dc83ea7b7cd55 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Fri, 25 Jan 2019 14:05:24 +0100 Subject: [PATCH] TCP: pass data after a ZeroWindowProbe to subdissectors If the single byte within a ZeroWindowProbe triggers reassembly within a subdissector, a new MSP will be created with just a single byte. Be sure not to mark subsequent segments that contain the full segment data as retransmission as this prevents the subdissector from seeing the data. Bug: 15427 Change-Id: I36ae2622689c6606c99cdff70b6beba4b9d25ca7 Reviewed-on: https://code.wireshark.org/review/31732 Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Jasper Bongertz Reviewed-by: Peter Wu --- docbook/release-notes.asciidoc | 2 ++ epan/dissectors/packet-tcp.c | 23 +++++++++++++++++++++++ test/captures/retrans-tls.pcap | Bin 0 -> 158 bytes test/suite_dissection.py | 25 +++++++++++++++++++++++++ 4 files changed, 50 insertions(+) create mode 100644 test/captures/retrans-tls.pcap diff --git a/docbook/release-notes.asciidoc b/docbook/release-notes.asciidoc index 8b7451ad2e..1dfe7e37ed 100644 --- a/docbook/release-notes.asciidoc +++ b/docbook/release-notes.asciidoc @@ -29,6 +29,8 @@ The following bugs have been fixed: //* cveidlink:2014-2486[] //* Wireshark slowly leaked water under the kitchen sink over the course of several months, causing a big mess. +* Data following a TCP ZeroWindowProbe is marked as retransmission and not passed to subdissectors (wsbuglink:15427[]) + //_Non-empty section placeholder._ Dumpcap might not quit if Wireshark or TShark crashes. diff --git a/epan/dissectors/packet-tcp.c b/epan/dissectors/packet-tcp.c index ba1ebff815..44cf550364 100644 --- a/epan/dissectors/packet-tcp.c +++ b/epan/dissectors/packet-tcp.c @@ -3071,10 +3071,33 @@ again: if (tcpd) { /* Have we seen this PDU before (and is it the start of a multi- * segment PDU)? + * + * If the sequence number was seen before, it is part of a + * retransmission if the whole segment fits within the MSP. + * (But if this is this frame was already visited and the first frame of + * the MSP matches the current frame, then it is not a retransmission, + * but the start of a new MSP.) + * + * If only part of the segment fits in the MSP, then either: + * - The previous segment included with the MSP was a Zero Window Probe + * with one byte of data and the subdissector just asked for one more + * byte. Do not mark it as retransmission (Bug 15427). + * - Data was actually being retransmitted, but with additional data + * (Bug 13523). Do not mark it as retransmission to handle the extra + * bytes. (NOTE Due to the TCP_A_RETRANSMISSION check below, such + * extra data will still be ignored.) + * - The MSP contains multiple segments, but the subdissector finished + * reassembly using a subset of the final segment (thus "msp->nxtpdu" + * is smaller than the nxtseq of the previous segment). If that final + * segment was retransmitted, then "nxtseq > msp->nxtpdu". + * Unfortunately that will *not* be marked as retransmission here. + * The next TCP_A_RETRANSMISSION hopefully takes care of it though. + * * Only shortcircuit here when the first segment of the MSP is known, * and when this this first segment is not one to complete the MSP. */ if ((msp = (struct tcp_multisegment_pdu *)wmem_tree_lookup32(tcpd->fwd->multisegment_pdus, seq)) && + nxtseq <= msp->nxtpdu && !(msp->flags & MSP_FLAGS_MISSING_FIRST_SEGMENT) && msp->last_frame != pinfo->num) { const char* str; gboolean is_retransmission = FALSE; diff --git a/test/captures/retrans-tls.pcap b/test/captures/retrans-tls.pcap new file mode 100644 index 0000000000000000000000000000000000000000..90be0de74b05cffb260102297ddb376aea408d70 GIT binary patch literal 158 zcmca|c+)~A1{MYw`2U}Q;R%ov`O`CIQz