Wireshark · Wireshark-dev: Re: [Wireshark-dev] Wireshark may get ISN wrong

[Matt <mattator@xxxxxxxxx>]

Find enclosed a fix for HEAD.

% git diff --stat
 epan/dissectors/packet-tcp.c | 8 +++++---
 epan/dissectors/packet-tcp.h | 5 ++---
 2 files changed, 7 insertions(+), 6 deletions(-)

2014-11-18 15:54 GMT+01:00 Matt <mattator@xxxxxxxxx>:
> Thanks for the suggestion but relative seq nb is a really nice feature
> I use for plotting and analyzing data. If the TCP ISN can be 0 (I
> believe it can ?) then my report qualifies as a bug. The fix should be
> a ~10 lines patch with the expense of a boolean in tcp_analysis. I am
> willing to send a patch for it.
>
> 2014-11-17 18:41 GMT+01:00 ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
>> You can just disable relative sequence numbers in the preferences for tcp.
>>
>>
>> On Mon, Nov 17, 2014 at 9:38 AM, Matt <mattator@xxxxxxxxx> wrote:
>>> Hi,
>>>
>>> I use wireshark to examinate some traces generated by a network
>>> simulator (ns3 www.nsnam.org) which set the ISN to 0 (no randomization
>>> yet).
>>> As wireshark assumes base_seq == 0 to be an unitialized value, it
>>> triggers some error as wireshark tries to set again and again the base
>>> seq. Here is the output of a single 3WHS (custom printf), in peculiar
>>> in the 4th line, which is the ACK of the 3WHS, wiresharks sets
>>> base_seq =seq-1, ie 0-1 and it wraps the seq number (ugly).
>>>
>>> Setting base seq to : 0
>>> Setting base seq to : 0
>>> Setting rev base seq to : 0
>>> Setting base seq to : 4294967295
>>> Setting rev base seq to : 0
>>> Setting rev base seq to : 0
>>> Setting base seq to : 0
>>> Setting base seq to : 0
>>> Setting rev base seq to : 0
>>> Setting base seq to : 0
>>> Setting rev base seq to : 0
>>> Setting base seq to : 1
>>>
>>> I understand it seems a corner case but I don't believe have an ISN
>>> equal to 0 is forbidden by the RFC ?!
>>> I was wondering if I could add some boolean such as "base_seq_set" in
>>> mptcp_info_t to prevent such a behavior.
>>>
>>> Regards
>>> Matt
>>> ___________________________________________________________________________
>>> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
>>> Archives:    http://www.wireshark.org/lists/wireshark-dev
>>> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>>>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>> ___________________________________________________________________________
>> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
>> Archives:    http://www.wireshark.org/lists/wireshark-dev
>> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe

diff --git a/epan/dissectors/packet-tcp.c b/epan/dissectors/packet-tcp.c
index c6225f1..6ea9cab 100644
--- a/epan/dissectors/packet-tcp.c
+++ b/epan/dissectors/packet-tcp.c
@@ -1035,8 +1035,9 @@ tcp_analyze_sequence_number(packet_info *pinfo, guint32 seq, guint32 ack, guint3
      * event that the SYN or SYN/ACK packet is not seen
      * (this solves bug 1542)
      */
-    if(tcpd->fwd->base_seq==0) {
+    if(tcpd->fwd->base_seq_set == FALSE) {
         tcpd->fwd->base_seq = (flags & TH_SYN) ? seq : seq-1;
+        tcpd->fwd->base_seq_set = TRUE;
     }
 
     /* Only store reverse sequence if this isn't the SYN
@@ -1050,8 +1051,9 @@ tcp_analyze_sequence_number(packet_info *pinfo, guint32 seq, guint32 ack, guint3
      * other packets the ISN is unknown, so ack-1 is
      * as good a guess as ack.
      */
-    if( (tcpd->rev->base_seq==0) && (flags & TH_ACK) ) {
+    if( (tcpd->rev->base_seq_set==FALSE) && (flags & TH_ACK) ) {
         tcpd->rev->base_seq = ack-1;
+        tcpd->rev->base_seq_set = TRUE;
     }
 
     if( flags & TH_ACK ) {
@@ -4344,7 +4346,7 @@ dissect_tcp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
      * mission later.
      */
     if(tcpd && ((tcph->th_flags&(TH_SYN|TH_ACK))==TH_SYN) &&
-       (tcpd->fwd->base_seq!=0) &&
+       (tcpd->fwd->base_seq_set == TRUE) &&
        (tcph->th_seq!=tcpd->fwd->base_seq) ) {
         if (!(pinfo->fd->flags.visited)) {
             conv=conversation_new(pinfo->fd->num, &pinfo->src, &pinfo->dst, pinfo->ptype, pinfo->srcport, pinfo->destport, 0);
diff --git a/epan/dissectors/packet-tcp.h b/epan/dissectors/packet-tcp.h
index b502527..79781f8 100644
--- a/epan/dissectors/packet-tcp.h
+++ b/epan/dissectors/packet-tcp.h
@@ -151,9 +151,8 @@ struct tcp_multisegment_pdu {
 };
 
 typedef struct _tcp_flow_t {
-	guint32 base_seq;	/* base seq number (used by relative sequence numbers)
-				 * or 0 if not yet known.
-				 */
+    gboolean base_seq_set; /* true if base seq set */
+	guint32 base_seq;	/* base seq number (used by relative sequence numbers)*/
 	tcp_unacked_t *segments;
 	guint32 fin;		/* frame number of the final FIN */
 	guint32 lastack;	/* last seen ack */