TLS: fix broken reassembly with multiple PDUs in the same frame

When (1) a frame has multiple TLS application data records and (2) two
of them request reassembly of a new PDU, then the second fragment would
be considered conflicting with the first one since the PDUs (MSPs) are
identified by the frame number of the starting frame.

This behavior was observed in a firefox-http2-frag.pcap
(attachment 16616) which uses tcp_dissect_pdus to trigger reassembly:

    Frame 19: 8694 bytes on wire (69552 bits), 8694 bytes captured (69552 bits)
    ...
    Transport Layer Security            (8640 bytes)
        TLSv1.3 Record Layer: Application Data Protocol: http2
        SSL segment data (1369 bytes)   <-- 7/7 last segment of previous PDU
        SSL segment data (1203 bytes)   <-- 1/5 first segment of new PDU
        TLSv1.3 Record Layer: Application Data Protocol: http2
        SSL segment data (1369 bytes)   <-- 2/5
        TLSv1.3 Record Layer: Application Data Protocol: http2
        SSL segment data (1369 bytes)   <-- 3/5
        TLSv1.3 Record Layer: Application Data Protocol: http2
        SSL segment data (1369 bytes)   <-- 4/5
        TLSv1.3 Record Layer: Application Data Protocol: http2
        SSL segment data (976 bytes)    <-- 5/5
        TLSv1.3 Record Layer: Application Data Protocol: http2
        SSL segment data (1369 bytes)   <-- 1/? first segment of another PDU
    [5 Reassembled TLS segments (6286 bytes): #19(1203), #19(1369), #19(1369), #19(1369), #19(976)]
    [7 Reassembled TLS segments (8201 bytes): #17(1190), #17(1369), #17(1369), #18(1369), #18(1369), #18(1369), #19(166)]
    HyperText Transfer Protocol 2       (8201 bytes, reassembled PDU)
        Stream: DATA, Stream ID: 17, Length 8192 (partial entity body)
            ...
        (7/7 finishes previous reassembly, see "7 Reassembled TLS segments")
    HyperText Transfer Protocol 2       (1203 bytes, start of new PDU)
    HyperText Transfer Protocol 2       (6286 bytes, reassembled PDU)
        Stream: DATA, Stream ID: 17, Length 6277 (partial entity body)
            ...
        (all fragments are in this frame, see "5 Reassembled TLS segments")
    HyperText Transfer Protocol 2       (1369 bytes, start of another PDU)
    [Reassembly error, protocol SSL: Frame already added in first pass]

TLS records for fragments 1/5 and 1/? both start a new PDU and would
thus invoke fragment_add with the same identifier. That results in the
Reassembly error which breaks further decryption. Reduce the probability
of this issue by mixing in the TLS stream position of the fragment.

Bug: 11173
Change-Id: I5536f3010b156555f1d7ae6dc98e08c030c8f771
Reviewed-on: https://code.wireshark.org/review/29871
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>

1 files changed, 18 insertions, 2 deletions

diff --git a/epan/dissectors/packet-tls.c b/epan/dissectors/packet-tls.c
index 81abc9586f..7eededcbe7 100644
--- a/epan/dissectors/packet-tls.c
+++ b/epan/dissectors/packet-tls.c
@@ -1111,6 +1111,22 @@ process_ssl_payload(tvbuff_t *tvb, int offset, packet_info *pinfo,
                     proto_tree *tree, SslSession *session,
                     dissector_handle_t app_handle_port);
 
+static guint32
+tls_msp_frament_id(struct tcp_multisegment_pdu *msp)
+{
+    /*
+     * If a frame contains multiple appdata PDUs, then "first_frame" is not
+     * sufficient to uniquely identify groups of fragments. Therefore include
+     * seq (the position of the initial fragment in the TLS stream) in the ID.
+     * As a frame most likely does not have multiple PDUs (except maybe for
+     * HTTP2), just let 'seq' contibute only a few bits.
+     */
+    guint32 id = msp->first_frame;
+    id ^= (msp->seq & 0xff) << 24;
+    id ^= (msp->seq & 0xff00) << 16;
+    return id;
+}
+
 static void
 desegment_ssl(tvbuff_t *tvb, packet_info *pinfo, int offset,
               guint32 seq, guint32 nxtseq,
@@ -1197,7 +1213,7 @@ again:
         }
 
         ipfd_head = fragment_add(&ssl_reassembly_table, tvb, offset,
-                                 pinfo, msp->first_frame, NULL,
+                                 pinfo, tls_msp_frament_id(msp), NULL,
                                  seq - msp->seq,
                                  len, (LT_SEQ (nxtseq,msp->nxtpdu)));
 
@@ -1469,7 +1485,7 @@ again:
 
             /* add this segment as the first one for this new pdu */
             fragment_add(&ssl_reassembly_table, tvb, deseg_offset,
-                         pinfo, msp->first_frame, NULL,
+                         pinfo, tls_msp_frament_id(msp), NULL,
                          0, nxtseq - deseg_seq,
                          LT_SEQ(nxtseq, msp->nxtpdu));
         }