Re: ZigBee APS re-assemble with re-used sequence number

[Kenneth Soerensen <knnthsrnsn () gmail com>]

Hi again

I have hacked a new fragment_add_seq_check() that uses a timeout to handle
the re-transmission in the file attached to bug 15021.

While playing around with this I have started thinking if this is a general
issue with fragment_add_seq_check()?

When re-assembly is completed fragment_add_seq_check() moves the
re-assembled frame to another hash table and stops considering it if more
fragments are dissected. I am not sure why it is done that way but if a
re-transmission for that frame is dissected after that point, the
re-transmission starts a new re-assembly instead of being put into the
completed one.

Is this the intended/preferred behavior of fragment_add_seq_check() or
should it handle re-transmissions past the point where the frame is
re-assembled? I guess ZigBee APS isn't the only protocol where this could
happen?

/Kenneth

2018-08-02 21:36 GMT+02:00 Kenneth Soerensen <knnthsrnsn () gmail com>:

> Hi Jaap and John
>
> I don't think ZigBee knowledge is required here and I guess this is a
> general issue for protocols with short sequence numbers.
>
> My concern with your suggestion is that we will need to maintain a sliding
> window and perform rollover detection for each node pair and direction (A
> ZigBee network has multiple nodes communicating in both directions).
>
> Currently, the APS dissector is using the generic fragment_add_seq_check().
>
> I have looked at fragment_add_seq_single_aging(), which seems to handle
> something like this. However, this is for protocols with a single sequence
> number (as John mentioned). Furthermore, the aging is based on frame
> numbers, which will not work for ZigBee because the number of frames
> between APS packets with the same sequence number will depend heavily on
> the amount of other traffic in the network.
>
> I have considered to create a function which uses timestamps for aging
> instead. A fragmented APS packet is considered to be completely transferred
> within a reasonable time compared to the time between sequence number
> rollover. But then again the _fragment_item struct used by the re-assembler
> does not store packet timestamps.
>
> The first fragment in an APS packet has a special attribute so I have also
> considered to simply re-start the reassembly for that sequence number when
> a first fragment is dissected.
>
> /Kenneth
>
>
> 2018-08-02 21:13 GMT+02:00 John Thacker <johnthacker () gmail com>:
>
> > I had to deal with this problem when I added support for protocols like
> > PPP MP with a single sequence number that increments each fragment instead
> > of separate sequence numbers and fragment numbers. (See Appendix A of RFC
> > 4623 (PWE3 Fragmentation and Reassembly) for a list of protocols that use
> > this style, including PPP MP (RFC 1990), PWE3 MPLS (RFC 4385), L2TPv2 (RFC
> > 2661), L2TPv3 (RFC 3931), ATM, and Frame Relay.)
> >
> > The problem happened in some long captures I had with some dropped
> > packets and where the "short sequence number" variant was used for MLPPP,
> > which is 12 bits (so a little bit bigger than the 8 bits with ZigBee, but
> > not having separate fragment sequence numbers effectively costs a couple
> > bits.) See in particular fragment_add_seq_single_aging(), and all the
> > fragment_add_seq_single_* functions in general. My approach, which may not
> > have been optimal, used a REASSEMBLE_FLAGS_AGING flag and a max_age
> > parameter to discard partially reassembled fragments. When wireshark gets a
> > new fragment and looks in the table of partially reassembled fragments to
> > add it to, if the partially reassembled fragment comes from a frame number
> > that is very old, that partially reassembled fragment is discarded. I ended
> > up using the previously unused frame field of the partially reassembled
> > fragment head for this (fh->frame), storing the frame number of most
> > recently added fragment in this as well.
> >
> > Take a look at those functions, as it may provide some help for your
> > approach. It worked for me on those files that I had.
> >
> > John Thacker
> >
> > On Thu, Aug 2, 2018 at 2:04 PM Jaap Keuter <jaap.keuter () xs4all nl> wrote:
> >
> > > Hi,
> > >
> > > Not burdened by any ZigBee domain knowledge I would say that a seq#
> > > rollover would require a clearing of the non-reassembled fragments. But not
> > > all of them because we could still be in the process of reassembling the
> > > part of the stream with the not-yet rolled over seq#. A sliding window of
> > > non-reassembled fragments, of about half the seq# range, moved forward by
> > > the next received seq#, could be sufficient. All in all this would be an
> > > extension of the generic reassembly routenes, assuming they are used...
> > >
> > > Thanks,
> > > Jaap
> > >
> > > On 2 Aug 2018, at 12:17, Kenneth Soerensen <knnthsrnsn () gmail com> wrote:
> > >
> > > Hi
> > >
> > > I have filled bug https://bugs.wireshark.org/bug
> > > zilla/show_bug.cgi?id=15021
> > >
> > > Any idea how we can fix this?
> > >
> > > The packet re-assembler is confused by ZigBee APS re-using sequence
> > > numbers, which makes it hard to distinguish what fragments belong to
> > > specific re-assembled packets.
> > >
> > > /Kenneth
> > >
> > >
> > > ____________________________________________________________
> > > _______________
> > > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > > Archives:    https://www.wireshark.org/lists/wireshark-dev
> > > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> > >              mailto:wireshark-dev-request () wireshark org?subject=unsubscr
> > > ibe
> >
> >
> > ____________________________________________________________
> > _______________
> > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > Archives:    https://www.wireshark.org/lists/wireshark-dev
> > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> >              mailto:wireshark-dev-request () wireshark org?subject=unsubscr
> > ibe
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe