Skip to content

Commit e4ef0e5

Browse files
committed
fix(reassembly): report an unfragmented datagram whose BUFID is buffered, and leave IPv6 atomic fragments' buffers alone (#2270)
Problem: in IP.reassembly a datagram with FO 0 and MF clear whose BUFID was buffered submitted that buffer and returned, so the datagram itself was never reported; on IPv6 a colliding atomic fragment also flushed the datagram being reassembled, so its final fragment could no longer complete it. Fix: IPv4 keeps reporting the flushed buffer as incomplete and then reassembles the datagram in a fresh buffer (RFC 791 section 3.2). IPv6 sets IP._isolate_atomic, so an atomic fragment is reassembled in a buffer of its own and submitted without touching the queued one (RFC 6946 section 4). Test: test_ip_unfragmented_bufid_collision_2270_unit pins both cases in strict and loose mode; test_ip's flush test now expects the second datagram too.
1 parent 7b57bb0 commit e4ef0e5

5 files changed

Lines changed: 137 additions & 14 deletions

File tree

‎docs/source/pcapkit/foundation/reassembly/ip/index.rst‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,9 @@ Departures from the procedure above:
103103
:rfc:`8200#section-4.5` for IPv6.
104104
* A buffer flushed by an unfragmented datagram reusing its ``BUFID`` is
105105
reported as an incomplete datagram rather than discarded.
106+
* An IPv6 atomic fragment, one with fragment offset zero and **M** clear,
107+
flushes nothing: it is reassembled in isolation and the fragments buffered
108+
under its ``BUFID`` are kept, per :rfc:`6946#section-4` (:issue:`2270`).
106109
* ``TDL`` starts at ``-1`` rather than ``0``, so that a datagram carrying no
107110
data can complete (:issue:`1359`).
108111
* Only the final fragment's partial last ``RCVBT`` block is set. A non-final

‎pcapkit/foundation/reassembly/ip.py‎

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,21 @@ class IP(ReassemblyBase[Packet[_AT], Datagram[_AT], BufferID, Buffer[_AT]], Gene
6767
<pcapkit.foundation.reassembly.data.ip.Datagram.conflict>`
6868
(:issue:`477`). An incomplete buffer expires
6969
after ``timeout`` seconds of capture time, 60 by default for both IPv4
70-
and IPv6.
70+
and IPv6. A datagram with fragment offset zero and **MF** clear is
71+
reported even when its BUFID is already buffered, rather than dropped
72+
(:issue:`2270`).
7173
7274
"""
7375
if TYPE_CHECKING:
7476
protocol: 'Type[IP_Protocol]'
7577

78+
#: bool: Whether a datagram with fragment offset zero and **MF** clear is
79+
#: reassembled apart from the buffer its BUFID names, rather than flushing
80+
#: that buffer first. IPv4 flushes it (:rfc:`791#section-3.2`); IPv6
81+
#: overrides this, since :rfc:`6946#section-4` keeps an atomic fragment's
82+
#: buffer intact (:issue:`2270`).
83+
_isolate_atomic: 'bool' = False
84+
7685
##########################################################################
7786
# Methods.
7887
##########################################################################
@@ -147,13 +156,22 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None':
147156
# belong to.
148157
self._dtgram.extend(self.expire(TS))
149158

150-
# when non-fragmented (possibly discarded) packet received
159+
# when non-fragmented (possibly discarded) packet received: it is
160+
# reassembled on its own, in a buffer of its own (#2270)
161+
isolated = False
151162
if not FO and not MF:
152-
if BUFID in self._buffer:
163+
if self._isolate_atomic:
164+
# an IPv6 atomic fragment "MUST" be processed in isolation, and
165+
# fragments queued under its BUFID "MUST NOT" be discarded
166+
# (:rfc:`6946#section-4`), so the buffer is left alone
167+
isolated = True
168+
elif BUFID in self._buffer:
169+
# :rfc:`791#section-3.2` flushes the buffer and then submits
170+
# this datagram, so the flushed one is reported incomplete and
171+
# this one goes on to a fresh buffer below
153172
self._dtgram.extend(
154173
self.submit(self._buffer.pop(BUFID), bufid=BUFID)
155174
)
156-
return
157175

158176
# the header of the fragment at offset zero is the reassembled datagram's
159177
header = b'' if FO else self._rectify_header(info.header, BUFID[3])
@@ -162,8 +180,9 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None':
162180
# block of the data buffer, its partial last block included -- 8191
163181
# would cover octets 0 to 65527 only, so a fragment reaching octet 65528,
164182
# e.g. at the largest Fragment Offset, would index past it (#1565)
165-
if BUFID not in self._buffer:
166-
self._buffer[BUFID] = Buffer(
183+
buf: 'Buffer[_AT]'
184+
if isolated or BUFID not in self._buffer:
185+
buf = Buffer(
167186
TDL=-1, # Total Data Length
168187
RCVBT=bytearray(8192), # Fragment Received Bit Table
169188
index=[], # index record
@@ -172,12 +191,13 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None':
172191
timestamp=TS, # first-arriving fragment's clock reading
173192
conflict=[], # conflicting octet ranges
174193
)
194+
if not isolated:
195+
self._buffer[BUFID] = buf
175196
else:
197+
buf = self._buffer[BUFID]
176198
# put header into header buffer
177-
if not FO: # pylint: disable=else-if-used
178-
self._buffer[BUFID].__update__(header=header)
179-
180-
buf = self._buffer[BUFID]
199+
if not FO:
200+
buf.__update__(header=header)
181201

182202
# append packet index
183203
buf.index.append(info.num)
@@ -270,8 +290,12 @@ def reassembly(self, info: 'Packet[_AT]') -> 'None':
270290
if not MF:
271291
buf.__update__(TDL=length + FO)
272292

293+
# an isolated datagram has no buffer to wait in, so it is submitted
294+
# whether or not it is complete
295+
if isolated:
296+
self._dtgram.extend(self.submit(buf, bufid=BUFID))
273297
# when datagram is reassembled in whole
274-
if self._is_complete(buf):
298+
elif self._is_complete(buf):
275299
self._dtgram.extend(
276300
self.submit(self._buffer.pop(BUFID), bufid=BUFID, checked=True)
277301
)

‎pcapkit/foundation/reassembly/ipv6.py‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,9 @@ class IPv6(IP):
9292
The reassembled ``header`` leaves out the Fragment header and carries
9393
its Next Header, per :rfc:`8200#section-4.5`, whichever engine fed it;
9494
1.4.x kept the Fragment header on the ``default`` and ``pcapng``
95-
engines (:issue:`415`).
95+
engines (:issue:`415`). An atomic fragment is reassembled in
96+
isolation, without flushing a buffer that shares its BUFID
97+
(:issue:`2270`).
9698
9799
"""
98100

@@ -124,6 +126,13 @@ class IPv6(IP):
124126
#: which reports the condition in its log record instead.
125127
__timeout__ = 60.0
126128

129+
#: bool: An atomic fragment, one with fragment offset zero and **M**
130+
#: clear, is reassembled on its own and leaves the buffer its BUFID names
131+
#: untouched: :rfc:`6946#section-4` says it "MUST" be processed in
132+
#: isolation and that the fragments queued under that BUFID "MUST NOT" be
133+
#: discarded (:issue:`2270`).
134+
_isolate_atomic = True
135+
127136
##########################################################################
128137
# Methods.
129138
##########################################################################

‎tests/foundation/reassembly/test_ip.py‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,19 @@ class TestIP(IP):
8888
reasm(self._packet(num=1, fo=0, mf=True, payload=b'abcdefgh'))
8989
reasm(self._packet(num=2, fo=0, mf=False, payload=b'ignored', header=b'fresh'))
9090

91-
datagram, = reasm.datagram
91+
# the flushed datagram is reported incomplete, and the unfragmented one
92+
# after it on its own (#2270)
93+
datagram, single = reasm.datagram
9294
self.assertFalse(datagram.completed)
9395
self.assertEqual(datagram.index, (1,))
9496
self.assertEqual(datagram.header, b'ip-header')
9597
self.assertEqual(datagram.payload, (b'abcdefgh',))
9698
self.assertIsNone(datagram.packet)
97-
self.assertEqual(len(callback_calls), 1)
99+
self.assertTrue(single.completed)
100+
self.assertEqual(single.index, (2,))
101+
self.assertEqual(single.header, b'fresh')
102+
self.assertEqual(single.payload, b'ignored')
103+
self.assertEqual(len(callback_calls), 2)
98104

99105
pending = TestIP()
100106
pending(self._packet(num=3, fo=8, mf=True, payload=b'ijkl', header=b'ignored'))
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
# -*- coding: utf-8 -*-
2+
"""An unfragmented datagram whose BUFID is already buffered is reported.
3+
4+
GitHub issue #2270. A datagram with fragment offset zero and **MF** clear whose
5+
BUFID matched a buffered one submitted that buffer and returned, so the datagram
6+
itself was never reported. IPv6 inherited this, so a colliding atomic fragment
7+
also flushed the datagram being reassembled, and its final fragment then opened
8+
a buffer of its own that never completed.
9+
10+
:rfc:`791#section-3.2` flushes the buffer and then submits the datagram, so
11+
IPv4 reports both, the flushed one incomplete. :rfc:`6946#section-4` says an
12+
atomic fragment is processed in isolation and the queued fragments are not
13+
discarded, so IPv6 reports the atomic fragment alone and still completes the
14+
datagram it interrupted.
15+
16+
Every case builds its fragments in memory and reads no capture. Classes are
17+
imported inside each test, after :func:`~tests._support.reimport_once_per_class`.
18+
19+
"""
20+
21+
from ipaddress import ip_address
22+
import unittest
23+
24+
from tests._support import reimport_once_per_class
25+
26+
27+
class TestIPReassemblyUnfragmentedBufidCollision(unittest.TestCase):
28+
"""Pin that an unfragmented datagram sharing a buffered BUFID is reported."""
29+
30+
def setUp(self) -> None:
31+
reimport_once_per_class(self)
32+
33+
def _packet(self, *, v6: bool, num: int, fo: int, mf: bool, payload: bytes):
34+
"""A fragment of the one BUFID every case shares."""
35+
from pcapkit.const.reg.transtype import TransType
36+
from pcapkit.foundation.reassembly.data.ip import Packet
37+
38+
if v6:
39+
src, dst, ihl = ip_address('2001:db8::1'), ip_address('2001:db8::2'), 40
40+
else:
41+
src, dst, ihl = ip_address('192.0.2.1'), ip_address('192.0.2.2'), 20
42+
return Packet((src, dst, 9, TransType.UDP), num, fo, ihl, mf, ihl + len(payload),
43+
b'H' * ihl, bytearray(payload), 1000.0 + num)
44+
45+
def _reassemble(self, cls, packets, strict: bool):
46+
reasm = cls(strict=strict)
47+
for packet in packets:
48+
reasm(packet)
49+
return reasm, [(datagram.index, datagram.completed, datagram.payload) for datagram in reasm.datagram]
50+
51+
def test_ipv4_flushes_the_buffer_and_reports_the_datagram(self) -> None:
52+
from pcapkit.foundation.reassembly.data.data import Completion
53+
from pcapkit.foundation.reassembly.ipv4 import IPv4
54+
55+
packets = [self._packet(v6=False, num=1, fo=0, mf=True, payload=b'A' * 16),
56+
self._packet(v6=False, num=2, fo=0, mf=False, payload=b'B' * 16)]
57+
for strict in (True, False):
58+
with self.subTest(strict=strict):
59+
reasm, datagrams = self._reassemble(IPv4, packets, strict)
60+
flushed = (b'A' * 16,) if strict else b'A' * 16
61+
self.assertEqual(datagrams, [((1,), Completion.PARTIAL, flushed),
62+
((2,), Completion.COMPLETE, b'B' * 16)])
63+
self.assertEqual(reasm._buffer, {})
64+
65+
def test_ipv6_atomic_fragment_leaves_the_buffer_alone(self) -> None:
66+
from pcapkit.foundation.reassembly.data.data import Completion
67+
from pcapkit.foundation.reassembly.ipv6 import IPv6
68+
69+
packets = [self._packet(v6=True, num=1, fo=0, mf=True, payload=b'A' * 16),
70+
self._packet(v6=True, num=2, fo=0, mf=False, payload=b'B' * 16),
71+
self._packet(v6=True, num=3, fo=16, mf=False, payload=b'C' * 16)]
72+
for strict in (True, False):
73+
with self.subTest(strict=strict):
74+
reasm, datagrams = self._reassemble(IPv6, packets, strict)
75+
self.assertEqual(datagrams, [((2,), Completion.COMPLETE, b'B' * 16),
76+
((1, 3), Completion.COMPLETE, b'A' * 16 + b'C' * 16)])
77+
self.assertEqual(reasm._buffer, {})
78+
79+
80+
if __name__ == '__main__':
81+
unittest.main()

0 commit comments

Comments
 (0)