Wireshark · Wireshark-commits: [Wireshark-commits] master-2.4 b7575fc: TLS: fix broken reassembly with multiple PDUs in the same fr

[Wireshark code review <code-review-do-not-reply@xxxxxxxxxxxxx>]

URL: https://code.wireshark.org/review/gitweb?p=wireshark.git;a=commit;h=b7575fc1a2a5d89c067de877f83d3ba1bb3fec46
Submitter: "Peter Wu <peter@xxxxxxxxxxxxx>"
Changed: branch: master-2.4
Repository: wireshark

Commits:

b7575fc by Peter Wu (peter@xxxxxxxxxxxxx):

    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@xxxxxxxxxxxxx>
    Tested-by: Petri Dish Buildbot
    Reviewed-by: Anders Broman <a.broman58@xxxxxxxxx>
    (cherry picked from commit be16d87c60ae82eaef60da2bd4ce3597a05c1a30)
    Reviewed-on: https://code.wireshark.org/review/29886
    Reviewed-by: Peter Wu <peter@xxxxxxxxxxxxx>
    

Actions performed:

    from  1ce272d   pcapng: Free option_content on error
     add  b7575fc   TLS: fix broken reassembly with multiple PDUs in the same frame


Summary of changes:
 epan/dissectors/packet-ssl.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)