fix(dumpkit): a PCAP trace writes the octets it has, not a truncated record's claimed length (#1541)

pcapkit/dumpkit/pcap.py

@@ -230,6 +230,16 @@ def _append_value(self, value: 'Data_Frame', file: 'IO[bytes]', name: 'str') ->
             3-octet payload would raise ``SchemaWarning: packet length < 0: -3``
             from a dumper that only has to copy it.
 
+            ``incl_len`` is written as ``len(value.packet)``, the octets that
+            follow it, not as ``value.frame_info.incl_len``. The two are equal
+            for every record a file holds whole. A record whose captured length
+            runs past the end of its file, or a PCAP-NG block whose captured
+            length runs past the block (:issue:`1405`), is *kept as captured*
+            by its reader: the declared length stays in ``frame_info``, so the
+            source rebuilds byte-exactly, and ``packet`` is the octets present.
+            Copying that declared length would claim octets the trace does not
+            hold, and a reader would run off the end of the file (:issue:`1541`).
+
         """
         # NOTE: The payload is read before the metadata so that a caller passing a
         # mapping rather than a dissected frame -- which the flow-tracing adapters
@@ -242,7 +252,7 @@ def _append_value(self, value: 'Data_Frame', file: 'IO[bytes]', name: 'str') ->
 
         file.write(self._rechdr.pack(ts_sec & _UINT32_MASK,
                                      ts_frac & _UINT32_MASK,
-                                     frame_info.incl_len & _UINT32_MASK,
+                                     len(packet) & _UINT32_MASK,
                                      frame_info.orig_len & _UINT32_MASK) + packet)
         self._fnum += 1
 

pcapkit/protocols/misc/pcap/frame.py

@@ -236,6 +236,12 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True,
         Returns:
             Data_Frame: Parsed packet data.
 
+        Warns:
+            pcapkit.utilities.warnings.ProtocolWarning: If the timestamp is out
+                of range, or if the record's ``incl_len`` runs past the end of
+                the data being parsed, in which case the record is kept as
+                captured.
+
         """
         schema = self.__header__
 
@@ -315,6 +321,19 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True,
             #: bytes: Raw frame data.
             self._data = self._read_fileng(self.length + _ilen)
 
+            # NOTE: A record whose ``incl_len`` runs past the end of the data is
+            # kept as captured, as the PCAP-NG reader keeps a captured length
+            # running past its block (:issue:`1405`): ``incl_len`` and ``cap_len``
+            # stay as declared, so the record rebuilds byte-exactly, and the
+            # packet is the octets present. A writer must not copy the declared
+            # length, c.f. :meth:`PCAPIO._append_value
+            # <pcapkit.dumpkit.pcap.PCAPIO._append_value>` (:issue:`1541`).
+            held = len(self._data) - self.length
+            if held < _ilen:
+                warn(f'PCAP: [Frame {self._fnum}] captured length {_ilen} runs past the data, '
+                     f'which holds {held} octet(s); kept as captured',
+                     ProtocolWarning, stacklevel=stacklevel())
+
             # move forward to the beginning of the next frame
             self._file.seek(seek_cur, io.SEEK_SET)
 

tests/dumpkit/test_pcap_trace_truncated_record_1541_unit.py

@@ -0,0 +1,150 @@
+# -*- coding: utf-8 -*-
+"""A PCAP trace writes the octets a record has, not its claimed length. C.f. #1541.
+
+A record whose ``incl_len`` runs past the end of its file, or a PCAP-NG block
+whose captured length runs past the block (#1405), is kept as captured by its
+reader: the declared length stays in ``frame_info`` and the packet is the octets
+present. :class:`~pcapkit.dumpkit.pcap.PCAPIO` copied the declared length into the
+trace record header, so the trace claimed octets it did not hold -- measured on a
+record claiming ``0x7fffffff`` over 585 octets.
+
+Each case traces a TCP flow whose last record is cut that way, then reads the
+trace back with :mod:`struct` alone: every record must hold exactly ``incl_len``
+octets, and the last must end the file.
+
+Every case builds its own octets and reads no capture. Classes are imported
+inside each test, after :func:`~tests._support.reimport_once_per_class`.
+
+"""
+
+import importlib.util
+import io
+import os
+import struct
+import tempfile
+import unittest
+import warnings
+
+from tests._support import close_extractor, reimport_once_per_class
+from tests.foundation._roundtrip import ethernet, ipv4, pcap, pcapng, tcp
+
+HAS_RUNTIME = all(importlib.util.find_spec(name) is not None
+                  for name in ('tbtrim', 'aenum', 'chardet', 'dictdumper'))
+
+SECOND = 1_500_000_000
+#: Captured lengths a cut record declares: a little past the data, and the
+#: measured case.
+CLAIMS = (1000, 0x7FFF_FFFF)
+
+
+def _frames() -> 'list[bytes]':
+    """Three TCP segments of one connection, client to server."""
+    return [ethernet(ipv4(tcp(b'x' * number, seq=1000 + number, syn=not number), proto=6), 0x0800)
+            for number in range(3)]
+
+
+def _records(data: 'bytes') -> 'list[tuple[int, int, bytes]]':
+    """Every record of a little-endian PCAP file, refusing one that runs past it."""
+    records, offset = [], 24
+    while offset < len(data):
+        if offset + 16 > len(data):
+            raise AssertionError(f'{len(data) - offset} octet(s) after the last record')
+        _, _, incl, orig = struct.unpack_from('<IIII', data, offset)
+        if offset + 16 + incl > len(data):
+            raise AssertionError(f'record at {offset} claims {incl} octet(s), '
+                                 f'the file holds {len(data) - offset - 16}')
+        records.append((incl, orig, data[offset + 16:offset + 16 + incl]))
+        offset += 16 + incl
+    return records
+
+
+def _cut_pcap(claim: 'int') -> 'bytes':
+    """A PCAP file whose last record claims ``claim`` octets and holds its frame."""
+    frames = _frames()
+    data = pcap([(SECOND + number, 0, frame) for number, frame in enumerate(frames[:-1])])
+    return data + struct.pack('<IIII', SECOND + 2, 0, claim, len(frames[-1])) + frames[-1]
+
+
+def _overrun_pcapng(claim: 'int') -> 'bytes':
+    """A PCAP-NG file whose last EPB declares ``claim`` octets over its frame (#1405)."""
+    frames = _frames()
+    data = pcapng([(SECOND + number, 0, frame) for number, frame in enumerate(frames[:-1])])
+    ticks = (SECOND + 2) * 10 ** 6
+    body = struct.pack('<IIIII', 0, ticks >> 32, ticks & 0xFFFF_FFFF, claim, len(frames[-1])) + frames[-1]
+    body += bytes(-len(body) % 4)
+    return data + struct.pack('<II', 6, 12 + len(body)) + body + struct.pack('<I', 12 + len(body))
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class TestPCAPTraceTruncatedRecord(unittest.TestCase):
+    """A traced record's ``incl_len`` is the number of octets written after it."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+        self.tmpdir = tempfile.TemporaryDirectory()  # pylint: disable=consider-using-with
+        self.addCleanup(self.tmpdir.cleanup)
+
+    def trace(self, data: 'bytes') -> 'bytes':
+        """The PCAP file of the one TCP flow traced from ``data``."""
+        from pcapkit import extract
+
+        tracedir = tempfile.mkdtemp(dir=self.tmpdir.name)
+        with warnings.catch_warnings():
+            warnings.simplefilter('ignore')
+            extractor = extract(fin=io.BytesIO(data), nofile=True, store=False, trace=True, tcp=True,
+                                trace_fout=tracedir, trace_format='pcap', trace_byteorder='little')
+        close_extractor(extractor)
+        flows = list(extractor.trace.tcp)
+        self.assertEqual([list(flow.index) for flow in flows], [[1, 2, 3]])
+        with open(flows[0].fpout, 'rb') as file:
+            return file.read()
+
+    def check(self, trace: 'bytes', held: 'bytes') -> None:
+        """The trace holds every frame whole, the cut one as the octets it has."""
+        frames, records = _frames(), _records(trace)
+        self.assertEqual([octets for _, _, octets in records], frames[:-1] + [held])
+        self.assertEqual([(incl, orig) for incl, orig, _ in records],
+                         [(len(frame), len(frame)) for frame in frames[:-1]] + [(len(held), len(frames[-1]))])
+
+    def test_a_pcap_record_cut_at_eof_is_traced_as_the_octets_it_has(self) -> None:
+        """#1541: a record claiming more than the file holds writes what it holds."""
+        for claim in CLAIMS:
+            with self.subTest(claim=claim):
+                self.check(self.trace(_cut_pcap(claim)), _frames()[-1])
+
+    def test_a_pcapng_captured_length_past_its_block_is_traced_as_the_octets_it_has(self) -> None:
+        """#1541: so does an EPB whose captured length runs past the block (#1405)."""
+        for claim in CLAIMS:
+            with self.subTest(claim=claim):
+                self.check(self.trace(_overrun_pcapng(claim)), _frames()[-1])
+
+    def test_a_pcapng_block_cut_at_eof_is_traced_as_the_octets_it_has(self) -> None:
+        """#1541: and an EPB the file ends inside, which holds two octets fewer than it claims."""
+        data = pcapng([(SECOND + number, 0, frame) for number, frame in enumerate(_frames())])
+        # the trailing Block Total Length, then the last two octets of the frame
+        self.check(self.trace(data[:-6]), _frames()[-1][:-2])
+
+    def test_the_dumper_writes_the_packet_length(self) -> None:
+        """#1541: ``incl_len`` is ``len(packet)``, whatever ``frame_info`` declares."""
+        from pcapkit.const.reg.linktype import LinkType
+        from pcapkit.dumpkit.pcap import PCAPIO
+        from pcapkit.protocols.data.misc.pcap.frame import Frame, FrameInfo
+
+        path = os.path.join(self.tmpdir.name, 'out.pcap')
+        dumper = PCAPIO(path, protocol=LinkType.ETHERNET, byteorder='little', nanosecond=False)
+        for incl_len, packet in ((0x7FFF_FFFF, b'abc'), (3, b'abc'), (0, b'')):
+            frame = Frame(frame_info=FrameInfo(ts_sec=7, ts_usec=0, incl_len=incl_len, orig_len=86),
+                          time='time', number=1, time_epoch=7, len=86, cap_len=incl_len)
+            frame.__update__(packet=packet)
+            dumper(frame)
+        with open(path, 'rb') as file:
+            self.assertEqual(_records(file.read()), [(3, 86, b'abc'), (3, 86, b'abc'), (0, 86, b'')])
+
+    def test_a_whole_record_is_traced_unchanged(self) -> None:
+        """A record the file holds whole keeps its octets, as before #1541."""
+        data = pcap([(SECOND + number, 0, frame) for number, frame in enumerate(_frames())])
+        self.assertEqual(self.trace(data)[24:].hex(), data[24:].hex())
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/protocols/misc/pcap/test_frame_incl_len_overrun_1541_unit.py

@@ -0,0 +1,93 @@
+# -*- coding: utf-8 -*-
+"""A PCAP record claiming more octets than its file holds warns, and is kept as captured.
+
+GitHub issue #1541. A record whose ``incl_len`` runs past the end of the file
+parsed silently. It is now reported the way the PCAP-NG reader reports a
+captured length running past its block (#1405), and is kept as captured in the
+same way: ``incl_len`` and ``cap_len`` stay as declared, the packet is the octets
+present, and the frame rebuilds as exactly the octets read, from ``info`` and
+from ``info.to_dict()`` alike.
+
+Every case builds its own octets and reads no capture. Classes are imported
+inside each test, after :func:`~tests._support.reimport_once_per_class`.
+
+"""
+
+import io
+import struct
+import unittest
+import warnings
+
+from tests._support import close_extractor, reimport_once_per_class
+
+#: Little-endian global header: v2.4, Ethernet.
+GLOBAL = bytes.fromhex('d4c3b2a1' '0200' '0400' '00000000' '00000000' 'ffff0000' '01000000')
+#: Sixty octets of packet data, none of them zero.
+PACKET = bytes(range(1, 61))
+
+
+def _record(claim: 'int', packet: 'bytes' = PACKET) -> 'bytes':
+    """A record whose header claims ``claim`` octets, followed by ``packet``."""
+    return struct.pack('<IIII', 1_500_000_000, 7, claim, 86) + packet
+
+
+class TestFrameInclLenOverrun(unittest.TestCase):
+    """Pin how a record whose ``incl_len`` runs past the data parses and rebuilds."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+
+    def parse(self, octets: 'bytes') -> 'tuple[object, list[str]]':
+        """The frame parsed from ``octets``, and the warnings it raised."""
+        from pcapkit.protocols.misc.pcap.frame import Frame
+        from pcapkit.protocols.misc.pcap.header import Header
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter('always')
+            frame = Frame(io.BytesIO(octets), num=4, header=Header(GLOBAL).info)
+        return frame, [str(item.message) for item in caught]
+
+    def test_overrun_warns_and_is_kept_as_captured(self) -> None:
+        from pcapkit.protocols.misc.pcap.frame import Frame
+        from pcapkit.protocols.misc.pcap.header import Header
+
+        header = Header(GLOBAL).info
+        for claim in (61, 1000, 0x7FFF_FFFF):
+            with self.subTest(claim=claim):
+                octets = _record(claim)
+                frame, messages = self.parse(octets)
+                self.assertIn(f'PCAP: [Frame 4] captured length {claim} runs past the data, '
+                              f'which holds {len(PACKET)} octet(s); kept as captured', messages)
+
+                info = frame.info  # type: ignore[attr-defined]
+                self.assertEqual((info.frame_info.incl_len, info.cap_len, info.len), (claim, claim, 86))
+                self.assertEqual(bytes(info.packet), PACKET)
+                self.assertEqual(frame.data, octets)  # type: ignore[attr-defined]
+                with warnings.catch_warnings():
+                    warnings.simplefilter('ignore')
+                    for data in (info, info.to_dict()):
+                        rebuilt = Frame.from_data(data, num=4, header=header)
+                        self.assertEqual(rebuilt.data.hex(), octets.hex())
+
+    def test_whole_record_does_not_warn(self) -> None:
+        for packet in (PACKET, PACKET + b'\x00' * 16):
+            with self.subTest(trailing=len(packet) - len(PACKET)):
+                frame, messages = self.parse(_record(len(PACKET), packet))
+                self.assertFalse([text for text in messages if 'runs past' in text], messages)
+                self.assertEqual(bytes(frame.info.packet), PACKET)  # type: ignore[attr-defined]
+
+    def test_extraction_keeps_the_cut_frame(self) -> None:
+        import pcapkit
+
+        octets = GLOBAL + _record(len(PACKET)) + _record(1000)
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter('always')
+            extractor = pcapkit.extract(fin=io.BytesIO(octets), nofile=True, store=True, engine='default')
+        close_extractor(extractor)
+        frames = list(extractor.frame)
+        self.assertEqual([bytes(frame.info.packet) for frame in frames], [PACKET, PACKET])
+        self.assertEqual(sum('runs past the data' in str(item.message) for item in caught), 1)
+
+
+if __name__ == '__main__':
+    unittest.main()