From 86d225c2d796d9edc217cb95bf40d0479f5ecb29 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 17:03:44 -0400 Subject: [PATCH] fix(tcp): correct MPTCP option length arithmetic at all six #576 sites Re-derived every length from RFC 8684 rather than from the issue body, which mis-cites one section and files one site against the wrong line. - MP_FASTCLOSE (s3.5 fig 14, 12 octets): the schema had no reserved field, so it packed 11 against a declared 12, and `_read_mptcp_fastclose` required 16. Added the reserved octet and corrected the guard to 12; the maker was already right. - MP_JOIN-SYN/ACK (s3.2 fig 6, 16 octets): `_make_join_synack` wrote 12, the SYN form's length, and `_read_join_synack` required 20, contradicting its own docstring. Both now 16. The `opt` branch also never assigned `hmac`, so reconstruction substituted `bytes(8)` for the parsed HMAC. - MP_JOIN-ACK (s3.2 fig 7, 24 octets): `_make_join_ack` wrote 8. Its reader already required 24, so nothing it produced could be parsed back. - REMOVE_ADDR (s3.4.2 fig 13, `3 + n`): length was a constant 4, right for exactly the one-ID case the fixture happens to use. - MP_PRIO (s3.3.8 fig 11, 3 octets): length was a constant 4, which satisfied `MPTCPPriority.addr_id`'s own `length == 4` predicate and so packed a phantom all-zero Address ID. The `opt` branch also dropped `backup`. - DSS (s3.3 fig 9): the maker's length expression is RFC-correct and unchanged; the defect is the schema field widths #576's own split names, `MPTCPDSS.ack` and `.dsn` packing 0 octets rather than 4 when unextended. Both now select `UInt32Field`/`UInt64Field` through a `SwitchField`, because `NumberField(length=)` cannot pack at all. MP_JOIN-SYN is not a defect: figure 5 gives 12 and all three sites agree. Deletes the now-stale `tcp-mptcp/MP_FASTCLOSE` entry from EXPECTED_FAILURES, which this fix makes pass; `tcp-mptcp/MP_JOIN` stays, failing for an unrelated `self._flags` ordering defect. Adds the two new selectors to the docs. New per-site tests in test_tcp_mptcp_length_arithmetic_unit.py, each failing on the unfixed tree. Coverage of the two changed modules 99% -> 100%. --- .../pcapkit/protocols/transport/tcp.rst | 2 + pcapkit/protocols/schema/transport/tcp.py | 122 ++- pcapkit/protocols/transport/tcp.py | 172 ++++- tests/protocols/test_option_roundtrip_unit.py | 31 +- .../test_tcp_mptcp_length_arithmetic_unit.py | 720 ++++++++++++++++++ .../transport/test_tcp_mptcp_subtype_unit.py | 53 +- .../protocols/transport/test_tcp_udp_unit.py | 31 +- 7 files changed, 1060 insertions(+), 71 deletions(-) create mode 100644 tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py diff --git a/docs/source/pcapkit/protocols/transport/tcp.rst b/docs/source/pcapkit/protocols/transport/tcp.rst index a2dc3b522..6492017a3 100644 --- a/docs/source/pcapkit/protocols/transport/tcp.rst +++ b/docs/source/pcapkit/protocols/transport/tcp.rst @@ -368,6 +368,8 @@ Auxiliary Functions .. autofunction:: pcapkit.protocols.schema.transport.tcp.mptcp_data_selector .. autofunction:: pcapkit.protocols.schema.transport.tcp.mptcp_add_address_selector +.. autofunction:: pcapkit.protocols.schema.transport.tcp.mptcp_dss_ack_selector +.. autofunction:: pcapkit.protocols.schema.transport.tcp.mptcp_dss_dsn_selector Data Models ----------- diff --git a/pcapkit/protocols/schema/transport/tcp.py b/pcapkit/protocols/schema/transport/tcp.py index 0e65953de..6164dd0c7 100644 --- a/pcapkit/protocols/schema/transport/tcp.py +++ b/pcapkit/protocols/schema/transport/tcp.py @@ -12,10 +12,10 @@ from pcapkit.const.tcp.option import Option as Enum_Option from pcapkit.corekit.fields.collections import ListField, OptionField from pcapkit.corekit.fields.ipaddress import IPv4AddressField, IPv6AddressField -from pcapkit.corekit.fields.misc import (ConditionalField, ForwardMatchField, PayloadField, - SchemaField, SwitchField) -from pcapkit.corekit.fields.numbers import (EnumField, NumberField, UInt8Field, UInt16Field, - UInt32Field, UInt64Field) +from pcapkit.corekit.fields.misc import (ConditionalField, ForwardMatchField, NoValueField, + PayloadField, SchemaField, SwitchField) +from pcapkit.corekit.fields.numbers import (EnumField, UInt8Field, UInt16Field, UInt32Field, + UInt64Field) from pcapkit.corekit.fields.strings import BitField, BytesField, PaddingField from pcapkit.protocols.schema.schema import EnumSchema, Schema, schema_final from pcapkit.utilities.exceptions import FieldError @@ -233,6 +233,90 @@ def mptcp_add_address_selector(pkt: 'dict[str, Any]') -> 'Field': raise FieldError(f'TCP: [OptNo {Enum_Option.Multipath_TCP}] {Enum_MPTCPOption.ADD_ADDR} invalid IP version') +def mptcp_dss_ack_selector(pkt: 'dict[str, Any]') -> 'Field': + """Selector function for :attr:`MPTCPDSS.ack` field. + + :rfc:`8684` section 3.3 figure 9 gives the Data ACK as "4 or 8 octets, + depending on flags": present only when ``A`` is set, and 8 octets wide only + when ``a`` is *also* set -- "a = Data ACK is 8 octets (if not set, Data ACK + is 4 octets)". + + Args: + pkt: Packet data. + + Returns: + * If ``A`` is clear, a :class:`~pcapkit.corekit.fields.misc.NoValueField` + instance -- the field is absent from the wire. + * If ``A`` is set and ``a`` is set, a + :class:`~pcapkit.corekit.fields.numbers.UInt64Field` instance. + * If ``A`` is set and ``a`` is clear, a + :class:`~pcapkit.corekit.fields.numbers.UInt32Field` instance. + + Note: + This is a :class:`~pcapkit.corekit.fields.misc.SwitchField` selector + rather than a :class:`~pcapkit.corekit.fields.misc.ConditionalField` + wrapping ``NumberField(length=lambda pkt: ...)``, which is what it was + until #576, for two independent reasons. + + The width lambda read ``8 if pkt['flags']['a'] else 0`` -- **0**, not 4 -- + so an unextended Data ACK packed no octets at all while the ``length`` + octet still counted 4 for it. That is the defect #576 records: the option + went onto the wire 4 (or 8, with ``dsn`` too) octets shorter than it + declared, and the ``ack`` value the caller supplied was simply not + present. + + Correcting the lambda to ``8 if ... else 4`` would not have worked, + because :class:`~pcapkit.corekit.fields.numbers.NumberField` cannot pack + a callable length at all: it calls ``build_template`` once at + ``__init__`` with the placeholder length ``-1``, which latches + ``_need_process = True``, and nothing clears that flag when + ``__call__`` later resolves the real length and rebuilds the template as + ``>I``/``>Q``. ``pre_process`` then hands :func:`struct.pack` bytes for + an integer template and it raises ``struct.error: required argument is + not an integer``. Measured on the 8-octet form, which the old lambda did + reach: ``_make_mptcp_dss(DSS, ack=1 << 40)`` raised exactly that. Fixing + that belongs to :mod:`pcapkit.corekit.fields.numbers`; selecting between + two fields that each fix ``__template__`` at class level sidesteps it + entirely and is the pattern :func:`mptcp_add_address_selector` already + uses here. + + """ + if not pkt['flags']['A']: + return NoValueField() + return UInt64Field() if pkt['flags']['a'] else UInt32Field() + + +def mptcp_dss_dsn_selector(pkt: 'dict[str, Any]') -> 'Field': + """Selector function for :attr:`MPTCPDSS.dsn` field. + + :rfc:`8684` section 3.3 figure 9 gives the Data Sequence Number as "4 or 8 + octets, depending on flags": present only when ``M`` is set, and 8 octets + wide only when ``m`` is *also* set -- "m = Data Sequence Number is 8 octets + (if not set, DSN is 4 octets)". + + Args: + pkt: Packet data. + + Returns: + * If ``M`` is clear, a :class:`~pcapkit.corekit.fields.misc.NoValueField` + instance -- the field is absent from the wire. + * If ``M`` is set and ``m`` is set, a + :class:`~pcapkit.corekit.fields.numbers.UInt64Field` instance. + * If ``M`` is set and ``m`` is clear, a + :class:`~pcapkit.corekit.fields.numbers.UInt32Field` instance. + + Note: + Identical in shape to :func:`mptcp_dss_ack_selector`, and it replaces the + identical defect: ``NumberField(length=lambda pkt: 8 if pkt['flags']['m'] + else 0, ...)``. See that function's note for why the ``0`` was wrong and + why a corrected lambda would not have packed either. C.f. #576. + + """ + if not pkt['flags']['M']: + return NoValueField() + return UInt64Field() if pkt['flags']['m'] else UInt32Field() + + class PortEnumField(EnumField): """Enumerated value for protocol fields. @@ -793,14 +877,20 @@ class MPTCPDSS(MPTCP, code=Enum_MPTCPOption.DSS): 'A': (7, 1), }) #: Data ACK. - ack: 'int' = ConditionalField( - NumberField(length=lambda pkt: 8 if pkt['flags']['a'] else 0, signed=False), - lambda pkt: pkt['flags']['A'], + #: + #: 4 octets when ``A`` is set, 8 when ``a`` is set as well, absent otherwise -- + #: :rfc:`8684` section 3.3 figure 9. Both the presence test and the width live + #: in :func:`mptcp_dss_ack_selector`, whose note records what this field + #: declared until #576 and why a narrower fix would not have packed. + ack: 'int' = SwitchField( + selector=mptcp_dss_ack_selector, ) #: Data sequence number. - dsn: 'int' = ConditionalField( - NumberField(length=lambda pkt: 8 if pkt['flags']['m'] else 0, signed=False), - lambda pkt: pkt['flags']['M'], + #: + #: 4 octets when ``M`` is set, 8 when ``m`` is set as well, absent otherwise -- + #: :rfc:`8684` section 3.3 figure 9. C.f. :func:`mptcp_dss_dsn_selector`. + dsn: 'int' = SwitchField( + selector=mptcp_dss_dsn_selector, ) #: Subflow sequence number. ssn: 'int' = ConditionalField( @@ -907,6 +997,18 @@ class MPTCPFastclose(MPTCP, code=Enum_MPTCPOption.MP_FASTCLOSE): test: 'MPTCPSubtype' = BitField(length=1, namespace={ 'subtype': (0, 4), }) + #: Reserved. + #: + #: :rfc:`8684` section 3.5 figure 14 spends a whole 32-bit row on + #: ``Kind``/``Length``/``Subtype``/``(reserved)``, i.e. the subtype's 4 bits + #: are followed by **12** reserved bits, not 4 -- so the subtype-and-reserved + #: part is 2 octets and the option is 12 octets in total. Until #576 this + #: field did not exist and ``test`` was the only octet between ``length`` and + #: ``key``, so the schema packed **11** octets against a ``length`` of 12. + #: Declared the same way :class:`MPTCPJoinACK` declares its own reserved + #: octet, for the same reason: a wider ``test`` would make the reserved bits + #: look like part of the subtype namespace. + reserved: 'bytes' = PaddingField(length=1) #: Option receiver's key. key: 'int' = UInt64Field() diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index a17182d7b..5ad23d793 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -1534,7 +1534,7 @@ def _read_mptcp_join(self, schema: 'Schema_MPTCPJoin', *, options: 'Option') -> def _read_join_syn(self, schema: 'Schema_MPTCPJoinSYN', *, options: 'Option') -> 'Data_MPTCPJoinSYN': # pylint: disable=unused-argument """Read Join Connection option for Initial SYN. - Structure of ``MP_JOIN-SYN`` [:rfc:`6824`]: + Structure of ``MP_JOIN-SYN`` [:rfc:`8684`, section 3.2, figure 5]: .. code-block:: text @@ -1577,7 +1577,7 @@ def _read_join_syn(self, schema: 'Schema_MPTCPJoinSYN', *, options: 'Option') -> def _read_join_synack(self, schema: 'Schema_MPTCPJoinSYNACK', options: 'Option') -> 'Data_MPTCPJoinSYNACK': # pylint: disable=unused-argument """Read Join Connection option for Responding SYN/ACK. - Structure of ``MP_JOIN-SYN/ACK`` [:rfc:`6824`]: + Structure of ``MP_JOIN-SYN/ACK`` [:rfc:`8684`, section 3.2, figure 6]: .. code-block:: text @@ -1601,10 +1601,25 @@ def _read_join_synack(self, schema: 'Schema_MPTCPJoinSYNACK', options: 'Option') Parsed option data. Raises: - ProtocolError: If length is **NOT** ``20``. + ProtocolError: If length is **NOT** ``16``. + + Note: + The accepted length is ``16``, which is what the figure above -- and + :rfc:`8684` section 3.2 figure 6, which it reproduces -- states, and + what :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPJoinSYNACK` + actually packs and unpacks: ``Kind`` (1) + ``Length`` (1) + + subtype/flags (1) + ``Address ID`` (1) + the truncated HMAC (8) + the + random number (4). + + This guard required ``20`` until #576 -- a value that appears in + neither the figure nor the schema, and that contradicted this method's + own docstring. Together with ``_make_join_synack``'s ``length=12`` it + made the SYN/ACK form unusable in both directions at once: the maker + could not produce a length this guard accepted, and a spec-correct + 16-octet option off the wire was rejected as an invalid format. """ - if schema.length != 20: + if schema.length != 16: raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] invalid format') data = Data_MPTCPJoinSYNACK( @@ -1622,7 +1637,7 @@ def _read_join_synack(self, schema: 'Schema_MPTCPJoinSYNACK', options: 'Option') def _read_join_ack(self, schema: 'Schema_MPTCPJoinACK', *, options: 'Option') -> 'Data_MPTCPJoinACK': # pylint: disable=unused-argument """Read Join Connection option for Third ACK. - Structure of ``MP_JOIN-ACK`` [:rfc:`6824`]: + Structure of ``MP_JOIN-ACK`` [:rfc:`8684`, section 3.2, figure 7]: .. code-block:: text @@ -1664,7 +1679,7 @@ def _read_join_ack(self, schema: 'Schema_MPTCPJoinACK', *, options: 'Option') -> def _read_mptcp_dss(self, schema: 'Schema_MPTCPDSS', *, options: 'Option') -> 'Data_MPTCPDSS': # pylint: disable=unused-argument """Read Data Sequence Signal (Data ACK and Data Sequence Mapping) option. - Structure of ``DSS`` [:rfc:`6824`]: + Structure of ``DSS`` [:rfc:`8684`, section 3.3, figure 9]: .. code-block:: text @@ -1752,7 +1767,7 @@ def _read_mptcp_addaddr(self, schema: 'Schema_MPTCPAddAddress', *, options: 'Opt def _read_mptcp_remove(self, schema: 'Schema_MPTCPRemoveAddress', *, options: 'Option') -> 'Data_MPTCPRemoveAddress': # pylint: disable=unused-argument """Read Remove Address option. - Structure of ``REMOVE_ADDR`` [:rfc:`6824`]: + Structure of ``REMOVE_ADDR`` [:rfc:`8684`, section 3.4.2, figure 13]: .. code-block:: text @@ -1773,6 +1788,22 @@ def _read_mptcp_remove(self, schema: 'Schema_MPTCPRemoveAddress', *, options: 'O Raises: ProtocolError: If the length is smaller than **3**. + Note: + ``Length = 3 + n``, per the figure above: the 3 is ``Kind`` (1) + + ``Length`` (1) + subtype-and-reserved (1), and each of the *n* Address + IDs is one further octet. + :attr:`~pcapkit.protocols.schema.transport.tcp.MPTCPRemoveAddress.addr_id` + sizes its list as ``pkt['length'] - 3`` from exactly this, which is why + ``_make_mptcp_remove``'s constant ``length=4`` (fixed in #576) also + mis-sized the parse rather than only the pack. + + The guard permits ``3``, i.e. ``n = 0``, which the figure does not + describe -- it shows one Address ID plus "n-1 Address IDs, if + required". Left as it stands: tightening it to reject an empty list is + a behaviour change beyond #576's scope, and a zero-ID REMOVE_ADDR now + at least round-trips honestly instead of declaring an octet it never + packed. + """ if schema.length < 3: raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] invalid format') @@ -1789,7 +1820,7 @@ def _read_mptcp_remove(self, schema: 'Schema_MPTCPRemoveAddress', *, options: 'O def _read_mptcp_prio(self, schema: 'Schema_MPTCPPriority', *, options: 'Option') -> 'Data_MPTCPPriority': # pylint: disable=unused-argument """Read Change Subflow Priority option. - Structure of ``MP_PRIO`` [RFC 6824]: + Structure of ``MP_PRIO`` [:rfc:`6824`, section 3.3.8, figure 11]: .. code-block:: text @@ -1807,7 +1838,25 @@ def _read_mptcp_prio(self, schema: 'Schema_MPTCPPriority', *, options: 'Option') Parsed option data. Raises: - ProtocolError: If the length is smaller than **3**. + ProtocolError: If the length is neither **3** nor **4**. + + Note: + The figure above is :rfc:`6824`'s, deliberately, because it is the one + with an Address ID in it and this method accepts both forms. + :rfc:`8684` section 3.3.8 figure 11 draws MP_PRIO as **3** octets and + nothing more -- ``Kind`` (1) + ``Length`` (1) + subtype/reserved/``B`` + (1) -- since section 5 of that document "specifies the removal of the + AddrID field [RFC6824] in the MP_PRIO option", closing a theoretical + attack in which a subflow could be forced into backup mode. The + 4-octet :rfc:`6824` form is therefore legacy, and the guard stays + permissive so that traffic carrying it still parses. + + ``_make_mptcp_prio`` declared a constant ``length=4`` until #576, + which meant the construction side could only ever emit the legacy + form -- and emitted it with an all-zero phantom Address ID when the + caller supplied none, because + :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPPriority`'s + ``addr_id`` is conditional on that very length being 4. """ if schema.length not in (3, 4): @@ -1865,7 +1914,7 @@ def _read_mptcp_fail(self, schema: 'Schema_MPTCPFallback', *, options: 'Option') def _read_mptcp_fastclose(self, schema: 'Schema_MPTCPFastclose', options: 'Option') -> 'Data_MPTCPFastclose': # pylint: disable=unused-argument """Read Fast Close option. - Structure of ``MP_FASTCLOSE`` [RFC 6824]: + Structure of ``MP_FASTCLOSE`` [:rfc:`8684`, section 3.5, figure 14]: .. code-block:: text @@ -1887,10 +1936,27 @@ def _read_mptcp_fastclose(self, schema: 'Schema_MPTCPFastclose', options: 'Optio Parsed option data. Raises: - ProtocolError: If the length is **NOT** 16. + ProtocolError: If the length is **NOT** 12. + + Note: + The figure above is :rfc:`8684` section 3.5 figure 14, and the option + it draws is **12** octets: ``Kind`` (1) + ``Length`` (1) + + subtype-and-reserved (2, being 4 subtype bits and 12 reserved) + the + option receiver's key (64 bits, 8). Note that section 3.5 is Fast + Close; section 3.7 is Fallback (MP_FAIL), which #576's own text cited + here by mistake. + + Three sites disagreed on this number before #576, all three now + reading 12: this guard required ``16``, an octet count nothing in the + RFC produces for MP_FASTCLOSE; ``_make_mptcp_fastclose`` declared the + correct 12 but the schema packed only **11**, missing the reserved + octet entirely. The net effect was that constructing an MP_FASTCLOSE + through :class:`TCP` raised ``ProtocolError`` from this very guard -- + the maker's *correct* length failing the parser's wrong check -- which + is why ``tcp-mptcp/MP_FASTCLOSE`` sat in ``EXPECTED_FAILURES``. """ - if schema.length != 16: + if schema.length != 12: raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] invalid format') data = Data_MPTCPFastclose( @@ -2782,12 +2848,25 @@ def _make_join_synack(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPT if opt is not None: backup = opt.backup addr_id = opt.addr_id - nonce = opt.nonce + # NOTE: ``hmac`` used to be missing from this branch entirely, while + # ``nonce = opt.nonce`` appeared on two consecutive lines -- so + # reconstructing a parsed MP_JOIN-SYN/ACK silently substituted the + # ``bytes(8)`` default for the truncated HMAC that was actually on + # the wire, and the HMAC is the whole point of this form of the + # option. C.f. #576. + hmac = opt.hmac nonce = opt.nonce return Schema_MPTCPJoinSYNACK( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), - length=12, + # NOTE: :rfc:`8684` section 3.2 figure 6 gives ``Length = 16`` for the + # SYN/ACK form: ``Kind`` (1) + ``Length`` (1) + subtype/flags (1) + + # ``Address ID`` (1) + the truncated HMAC (64 bits, 8) + the random + # number (32 bits, 4). This read ``12`` -- ``_make_join_syn``'s own + # correct length for the *SYN* form of figure 5, which carries a + # 4-octet token where this one carries an 8-octet HMAC -- copied + # across without recomputing. C.f. #576. + length=16, test={ 'subtype': subtype.value, 'backup': backup, @@ -2817,7 +2896,13 @@ def _make_join_ack(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTCPJ return Schema_MPTCPJoinACK( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), - length=8, + # NOTE: :rfc:`8684` section 3.2 figure 7 gives ``Length = 24`` for the + # ACK form: ``Kind`` (1) + ``Length`` (1) + subtype-and-reserved (2, + # being 4 subtype bits and 12 reserved) + the full HMAC (160 bits, + # 20). This read ``8``, a third of the truth -- and + # ``_read_join_ack``'s guard already required 24, so nothing this + # maker produced could be parsed back. C.f. #576. + length=24, test={ 'subtype': subtype.value, }, @@ -2870,17 +2955,35 @@ def _make_mptcp_dss(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTCP return Schema_MPTCPDSS( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), + # NOTE: this arithmetic is correct against :rfc:`8684` section 3.3 + # figure 9 and is deliberately left as it stands -- #576 filed it as + # one of six wrong lengths, and re-deriving it from the figure found + # it right. Read it as a base plus a widening increment rather than + # as one term per field: 4 for ``Kind``/``Length``/subtype/flags, then + # ``A`` contributes the 4-octet Data ACK and ``a`` a further 4 to make + # it 8; ``M`` contributes 12 (a 4-octet DSN, the 4-octet Subflow + # Sequence Number, the 2-octet Data-Level Length and the 2-octet + # Checksum) and ``m`` a further 4 to widen the DSN to 8. All flags set + # gives 4 + 4 + 4 + 12 + 4 = 28, which is the maximum the section + # states in prose. What was wrong was the *schema* it describes: + # ``MPTCPDSS.ack`` and ``.dsn`` packed 0 octets rather than 4 in the + # unextended case, so the option came out 4 or 8 octets short of this + # length and produced ``packet length < 0`` on the way back in. length=4 + (4 if flag_A else 0) + (4 if flag_a else 0) + (12 if flag_M else 0) + (4 if flag_m else 0), test={ 'subtype': subtype.value, }, + # NOTE: ``'A': flag_A`` used to appear twice in this literal, once at + # the top and once at the bottom. Harmless -- the same value under the + # same key, so the second simply won -- but it made the set of flags + # being written hard to read against figure 9's ``F|m|M|a|A``. C.f. + # #576. flags={ 'F': data_fin, 'A': flag_A, - 'm': flag_m, - 'M': flag_M, 'a': flag_a, - 'A': flag_A, + 'M': flag_M, + 'm': flag_m, }, ack=ack, dsn=dsn, @@ -2967,7 +3070,18 @@ def _make_mptcp_remove(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MP return Schema_MPTCPRemoveAddress( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), - length=4, + # NOTE: :rfc:`8684` section 3.4.2 figure 13 gives ``Length = 3 + n``, + # where the 3 is ``Kind`` (1) + ``Length`` (1) + subtype-and-reserved + # (1) and each of the *n* Address IDs is one further octet. This read + # a constant ``4``, which is right for exactly one list length -- and + # ``examples.generators.options``' own fixture passes ``addr_id=[1]``, + # so the round-trip suite exercised only that one. Measured before the + # fix: ``addr_id=[1, 2]`` packed 5 octets declaring 4, and + # ``addr_id=[]`` packed 3 declaring 4. This is the field + # ``MPTCPRemoveAddress.addr_id`` sizes itself from, as + # ``pkt['length'] - 3``, so the constant also mis-sized the parse. + # C.f. #576. + length=3 + len(addr_id_list), test={ 'subtype': subtype.value, }, @@ -2992,11 +3106,29 @@ def _make_mptcp_prio(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTC """ if opt is not None: + # NOTE: ``backup`` used to be missing from this branch, so + # reconstructing a parsed MP_PRIO always wrote ``B=0`` regardless of + # what was on the wire -- and the ``B`` flag is the entire payload of + # this option. The same shape as ``_make_join_synack``'s dropped + # ``hmac`` above. C.f. #576. + backup = opt.backup addr_id = opt.addr_id return Schema_MPTCPPriority( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), - length=4, + # NOTE: :rfc:`8684` section 3.3.8 figure 11 gives MP_PRIO as **3** + # octets -- ``Kind`` (1) + ``Length`` (1) + subtype/reserved/``B`` (1) + # -- with no Address ID at all: section 5 records that this document + # "specifies the removal of the AddrID field [RFC6824] in the MP_PRIO + # option". The 4-octet form is :rfc:`6824`'s, which this schema and + # ``_read_mptcp_prio`` both still accept, so the length has to follow + # whether an Address ID was actually given rather than being a + # constant. It read a constant ``4``, and because + # ``MPTCPPriority.addr_id`` is conditional on ``pkt['length'] == 4``, + # that constant *satisfied its own predicate*: with ``addr_id=None`` + # the option packed ``1e045000``, a phantom all-zero Address ID octet + # that no caller asked for. C.f. #576. + length=3 if addr_id is None else 4, test={ 'subtype': subtype.value, 'backup': backup, diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 9fb255081..d1014876e 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -159,22 +159,25 @@ class Gap(NamedTuple): # the seven (MP_CAPABLE, ADD_ADDR, REMOVE_ADDR, MP_PRIO, DSS, MP_FAIL) now # read ``'OK'`` and so have no entry below any more. # - # MP_FASTCLOSE does not: fixing ``subtype`` got it *past* the - # ``AttributeError`` this table used to record and into a second, - # unrelated defect that #566/#567 do not touch and #576 tracks -- see that - # entry for the detail. - 'tcp-mptcp/MP_FASTCLOSE': Gap( - 'CONSTRUCT', 'TCP: [OptNo 30] invalid format', - 'pcapkit/protocols/transport/tcp.py:1893 -- _read_mptcp_fastclose ' - 'requires schema.length == 16, which agrees with neither the maker ' - '(_make_mptcp_fastclose at pcapkit/protocols/transport/tcp.py:3054, ' - 'length=12, which is the RFC 8684 section 3.7 value) nor the schema ' - '(MPTCPFastclose.test at ' - 'pcapkit/protocols/schema/transport/tcp.py:907, which packs an ' - '11-octet option against the declared 12) (#576)'), + # MP_FASTCLOSE joined them in #576, and its entry is gone with them. Fixing + # ``subtype`` in #566 had got it *past* the ``AttributeError`` this table + # used to record and into a second, unrelated defect: three sites disagreed + # on its length, and the maker's *correct* value (12, from :rfc:`8684` + # section 3.5 figure 14 -- section 3.5 is Fast Close; the entry that used to + # sit here cited 3.7, which is Fallback) failed the parser's wrong check of + # 16, while the schema packed only 11 for want of a reserved octet. All + # three read 12 now. Note that REMOVE_ADDR, MP_PRIO and DSS in the list + # above read ``'OK'`` throughout that period *without* being correct -- this + # suite only checks the cycle is self-consistent, which a wrong length can + # be, so #576 covers them per option in + # :mod:`tests.protocols.transport.test_tcp_mptcp_length_arithmetic_unit` + # against RFC 8684 rather than against the cycle. # ``_make_mptcp_join`` branches on ``self._flags``, which only the parse - # path ever sets, so the constructor cannot be called at all. + # path ever sets, so the constructor cannot be called at all. Independent of + # #576 and left in place: ``TCP._make`` assigns ``_flags`` *after* it has + # already built the options, so this is a statement-ordering defect rather + # than a length one. 'tcp-mptcp/MP_JOIN': Gap( 'CONSTRUCT', "no attribute '_flags'", 'pcapkit/protocols/transport/tcp.py:2675 -- _make_mptcp_join reads ' diff --git a/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py new file mode 100644 index 000000000..3b7e1b7a4 --- /dev/null +++ b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py @@ -0,0 +1,720 @@ +# -*- coding: utf-8 -*- +"""Multipath TCP option lengths, re-derived from RFC 8684 at the six sites #576 names. + +GitHub issue #576, the follow-up #567 invited ("worth checking the sibling ``_make_mptcp_*`` +helpers' length arithmetic in the same pass") and #579 deliberately left alone, being scoped +to ``MPTCP.subtype`` and MP_CAPABLE. + +An MPTCP option carries its own octet count in its ``Length`` field, and three separate +places have to agree on it: the ``_make_mptcp_*`` maker that *writes* that octet, the schema +in :mod:`pcapkit.protocols.schema.transport.tcp` whose fields actually *pack* the octets, and +the ``_read_mptcp_*`` reader whose guard *accepts* it coming back. Each of the six sites below +had at least one of the three disagreeing with :rfc:`8684`, and one had all three disagreeing +with each other. + +What each site is, and where its number comes from +-------------------------------------------------- + +Every length here is derived from the figure named, by adding up the octets that figure +draws. The fixed head is 2 octets of ``Kind``/``Length`` plus the subtype row, which is 1 +octet where the subtype's 4 bits are followed by 4 bits of flags or reserved, and 2 octets +where they are followed by 12 reserved bits. + +* **MP_FASTCLOSE** -- :rfc:`8684` section 3.5, figure 14. Head 4 (``Kind`` 1 + ``Length`` 1 + + subtype-and-12-reserved-bits 2) + the receiver's key 8 = **12**. The maker already wrote 12; + :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPFastclose` packed **11**, having no + reserved field at all; and ``_read_mptcp_fastclose`` required **16**, a number neither of + the other two nor the RFC produces. Note section 3.5, not 3.7: 3.7 is Fallback (MP_FAIL), + and #576's own text -- and the ``EXPECTED_FAILURES`` entry it left behind -- cite 3.7 here + in error. +* **MP_JOIN-SYN/ACK** -- :rfc:`8684` section 3.2, figure 6. Head 4 (``Kind`` 1 + ``Length`` 1 + + subtype/rsv/``B`` 1 + ``Address ID`` 1) + the truncated HMAC 8 + the random number 4 = + **16**. The maker wrote **12**, which is ``_make_join_syn``'s own correct length for the + *SYN* form of figure 5 -- that one carries a 4-octet token where this one carries an + 8-octet HMAC -- copied across without recomputing. ``_read_join_synack`` independently + required **20**, contradicting its own docstring figure. +* **MP_JOIN-ACK** -- :rfc:`8684` section 3.2, figure 7. Head 4 (``Kind`` 1 + ``Length`` 1 + + subtype-and-12-reserved-bits 2) + the full 160-bit HMAC 20 = **24**. The maker wrote **8**. + Its reader already required 24, so nothing the maker produced could be parsed back at all. +* **REMOVE_ADDR** -- :rfc:`8684` section 3.4.2, figure 13, which states ``Length = 3 + n`` + outright: head 3 (``Kind`` 1 + ``Length`` 1 + subtype-and-4-reserved-bits 1) + one octet per + Address ID. The maker wrote a constant **4**, correct for exactly one list length -- and + ``examples.generators.options``' fixture passes ``addr_id=[1]``, so the round-trip suite + exercised only that one. +* **MP_PRIO** -- :rfc:`8684` section 3.3.8, figure 11: head 3 (``Kind`` 1 + ``Length`` 1 + + subtype/rsv/``B`` 1) and **nothing else**, since section 5 of that document "specifies the + removal of the AddrID field [RFC6824] in the MP_PRIO option", closing a theoretical attack + in which a subflow could be forced into backup mode. So **3**, with :rfc:`6824`'s 4-octet + Address ID form still accepted as legacy. The maker wrote a constant **4**, and because + :attr:`~pcapkit.protocols.schema.transport.tcp.MPTCPPriority.addr_id` is conditional on + ``pkt['length'] == 4``, that constant satisfied its own predicate: a caller who gave no + Address ID got a phantom all-zero one anyway. +* **DSS** -- :rfc:`8684` section 3.3, figure 9. Head 4, then the Data ACK (4 octets, or 8 when + ``a`` is set) if ``A``, the Data Sequence Number (4, or 8 when ``m`` is set), the Subflow + Sequence Number (4), the Data-Level Length (2) and the Checksum (2) if ``M``. All flags set + gives 28, which is the maximum the section states in prose. + +DSS is the one entry that is not a defect where #576 puts it +------------------------------------------------------------ + +#576 files the DSS defect against ``_make_mptcp_dss``'s length expression and suggests the +fix "will also change what its maker needs to compute". Re-derived from figure 9, that +expression is **already correct**, and it is left untouched. Read as a base plus widening +increments rather than one term per field: + +.. code-block:: python + + 4 + (4 if flag_A else 0) + (4 if flag_a else 0) + (12 if flag_M else 0) + (4 if flag_m else 0) + +the 4 is the head; ``A`` contributes the 4-octet Data ACK and ``a`` a further 4 to widen it to +8; ``M`` contributes 12 (a 4-octet DSN, the 4-octet SSN, the 2-octet Data-Level Length and the +2-octet Checksum) and ``m`` a further 4 to widen the DSN to 8. Every combination matches the +figure, and all flags set gives 4 + 4 + 4 + 12 + 4 = 28. + +What was wrong was the *schema* that expression describes. ``MPTCPDSS.ack`` and ``MPTCPDSS.dsn`` +sized themselves with ``NumberField(length=lambda pkt: 8 if pkt['flags']['a'] else 0)`` -- **0** +octets in the unextended case, where the figure says 4 -- so the option went onto the wire 4 +(or, with both fields unextended, 8) octets shorter than its own ``Length`` octet declared, and +the ``ack``/``dsn`` values the caller supplied were simply not present. Measured pre-fix with +the generator's own override arguments: 12 octets packed against a declared 20. + +A second defect sat behind the first: correcting the lambda to ``8 if ... else 4`` would still +not have packed, because :class:`~pcapkit.corekit.fields.numbers.NumberField` cannot pack a +callable length at all -- it calls ``build_template`` once at ``__init__`` with the placeholder +length ``-1``, latching ``_need_process = True``, and nothing clears that when ``__call__`` +later resolves the real length and rebuilds the template as ``>I``/``>Q``. Measured on the +8-octet form, which the old lambda did reach: ``_make_mptcp_dss(DSS, ack=1 << 40)`` raised +``struct.error: required argument is not an integer``. That belongs to +:mod:`pcapkit.corekit.fields.numbers` and is not fixed here; the schema instead selects between +:class:`~pcapkit.corekit.fields.numbers.UInt32Field` and +:class:`~pcapkit.corekit.fields.numbers.UInt64Field`, which each fix ``__template__`` at class +level, through a :class:`~pcapkit.corekit.fields.misc.SwitchField` -- the pattern +:func:`~pcapkit.protocols.schema.transport.tcp.mptcp_add_address_selector` already uses in that +module. :class:`TCPMPTCPDSSExtendedFieldsUnitTests` covers the 8-octet forms that could not be +packed before at all. + +MP_JOIN-SYN is not a defect either +----------------------------------- + +#576's body lists three MP_JOIN forms and a comment on the issue corrects the attribution. +Re-derived here: :rfc:`8684` section 3.2 figure 5 gives ``Length = 12`` for the initial SYN -- +head 4 (``Kind`` 1 + ``Length`` 1 + subtype/rsv/``B`` 1 + ``Address ID`` 1) + the receiver's +token 4 + the random number 4 -- ``_make_join_syn`` writes 12 and ``_read_join_syn`` guards +``!= 12``. All three agree. :class:`TCPMPTCPJoinSYNIsCorrectUnitTests` pins that, so a later +pass does not "fix" a correct number. + +Coverage +-------- + +One test class per site, not one test for all six, so a regression names the option it broke. + +Each class asserts the **byte-exact** packed option from its maker rather than only its +``length`` octet -- a declared length and a packed length can disagree, which is the whole +defect, so both are checked, and the octets themselves pin that the payload really is where +the figure puts it. Where the *reader* guard was also wrong (MP_FASTCLOSE, MP_JOIN-SYN/ACK) a +second test splices hand-built, spec-correct octets into a real TCP segment and parses them +back through :class:`~pcapkit.protocols.transport.tcp.TCP` proper, so a pack-side and a +parse-side bug cannot cancel out behind a closed round trip -- the pattern +:mod:`tests.protocols.transport.test_tcp_mptcp_capable_length_unit` established for #567, and +the blind spot :mod:`tests.protocols.test_option_roundtrip_unit`'s own docstring names ("a +defect can leave the cycle closed"). + +MP_JOIN cannot be built through the public ``TCP()`` constructor at all -- ``_make_mptcp_join`` +dispatches on ``self._flags``, which ``TCP._make`` assigns *after* it has already built the +options, so construction raises ``AttributeError: 'TCP' object has no attribute '_flags'``. +That is a separate, already-recorded gap (``tcp-mptcp/MP_JOIN`` in +:data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES`, with exactly that +diagnosis) and is not touched here, so the MP_JOIN classes drive the makers directly and reach +the readers by parsing bytes, where ``_flags`` *is* set. + +""" +from __future__ import annotations + +import unittest + +#: Header fields shared by the constructed TCP segments here, matching +#: :data:`examples.generators.options.TCP_BASE` and +#: :data:`tests.protocols.transport.test_tcp_mptcp_subtype_unit.TCP_BASE`. +TCP_BASE = { + 'srcport': 50000, 'dstport': 80, 'seq': 1, 'ack': 0, + 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack_flag': False, + 'psh': False, 'rst': False, 'syn': True, 'fin': False, + 'window': 8192, 'checksum': b'\x00\x00', 'urgent_pointer': 0, + 'payload': b'', +} + +#: A spec-correct MP_FASTCLOSE, :rfc:`8684` section 3.5 figure 14: ``Kind`` ``0x1e``, +#: ``Length`` ``12``, the subtype nibble ``7`` (MP_FASTCLOSE) with its 12 reserved bits as +#: ``0x70 0x00``, then the receiver's key, 8 octets of ``0xCC``. Hand-built, not made. +MP_FASTCLOSE_SPEC_OCTETS = bytes([0x1E, 0x0C, 0x70, 0x00]) + b'\xCC' * 8 + +#: A spec-correct MP_JOIN-SYN/ACK, :rfc:`8684` section 3.2 figure 6: ``Kind`` ``0x1e``, +#: ``Length`` ``16``, the subtype nibble ``1`` (MP_JOIN) with ``B=0`` as ``0x10``, ``Address +#: ID`` ``0x05``, the truncated HMAC as 8 octets of ``0xDD``, then the random number +#: ``0x00000009``. +MP_JOIN_SYNACK_SPEC_OCTETS = ( + bytes([0x1E, 0x10, 0x10, 0x05]) + b'\xDD' * 8 + bytes([0x00, 0x00, 0x00, 0x09]) +) + + +def build_tcp_segment(option_octets: 'bytes', *, syn: 'bool' = True, + ack_flag: 'bool' = False) -> 'bytes': + """Pack a whole TCP segment carrying ``option_octets`` verbatim as its only option. + + Goes through :class:`~pcapkit.protocols.schema.transport.tcp.TCP` (the schema, not the + protocol) directly, so nothing here calls a ``_make_mptcp_*`` maker: the option bytes + reach the wire exactly as given. + + Args: + option_octets: The whole option, header octets included, already padded to a multiple + of 4 octets. + syn: Whether to set the ``SYN`` flag. + ack_flag: Whether to set the ``ACK`` flag. + + Returns: + The packed TCP segment. + + Note: + ``syn``/``ack_flag`` are parameters rather than fixed because the MP_JOIN *parse* path + dispatches on them: ``TCP._read_mptcp_join`` picks between ``_read_join_syn``, + ``_read_join_synack`` and ``_read_join_ack`` by inspecting ``self._flags``, which the + read path sets from these very bits. A SYN/ACK option in a SYN-only segment would be + routed to the wrong reader. + + ``Schema_TCP`` is imported inside the function, not at module level -- see + :func:`tests.protocols.transport.test_tcp_mptcp_subtype_unit.build_mptcp_option`'s own + docstring for why: a name bound to ``pcapkit`` at collection time can go stale once + another module in this suite pops ``pcapkit``'s submodules out of ``sys.modules``. + + """ + from pcapkit.protocols.schema.transport.tcp import TCP as Schema_TCP + + if len(option_octets) % 4: + raise ValueError('option_octets must already be a multiple of 4 octets long') + + schema = Schema_TCP( + srcport=50000, dstport=80, seq=1, ack=0, + offset={'offset': 5 + len(option_octets) // 4, 'ns': 0}, + flags={'cwr': 0, 'ece': 0, 'urg': 0, 'ack': int(ack_flag), 'psh': 0, 'rst': 0, + 'syn': int(syn), 'fin': 0}, + window=8192, checksum=b'\x00\x00', urgent=0, + options=option_octets, + payload=b'', + ) + return schema.pack() + + +def make(meth: 'str', subtype_name: 'str', **kwargs: 'object') -> 'bytes': + """Build an MPTCP option through one maker and pack it. + + Args: + meth: Name of the ``_make_*`` method under test. + subtype_name: Attribute name on + :class:`~pcapkit.const.tcp.mp_tcp_option.MPTCPOption` for the subtype to pass. + **kwargs: forwarded to the maker as the option's own arguments. + + Returns: + The packed option bytes. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP.__new__(TCP) + schema = getattr(tcp, meth)(getattr(Enum_MPTCPOption, subtype_name), **kwargs) + return schema.pack() + + +class LengthInvariantMixin: + """The one invariant every site below broke: declared length is packed length.""" + + def assertLengthMatchesWire(self, packed: 'bytes', expected: 'int') -> 'None': # noqa: N802 + """Assert ``packed`` is ``expected`` octets long and says so in its length octet. + + Args: + packed: The packed option. + expected: The octet count :rfc:`8684` gives for this form. + + """ + self.assertEqual(len(packed), expected, # type: ignore[attr-defined] + 'the option must occupy the octet count RFC 8684 gives') + self.assertEqual(packed[1], expected, # type: ignore[attr-defined] + 'the declared length octet must match the wire length') + + +class TCPMPTCPFastcloseLengthUnitTests(LengthInvariantMixin, unittest.TestCase): + """Site 1 -- MP_FASTCLOSE is 12 octets: :rfc:`8684` section 3.5, figure 14.""" + + def test_maker_packs_twelve_octets(self) -> None: + """``_make_mptcp_fastclose`` packs 12 octets, reserved octet included. + + Pre-fix this packed **11** -- ``1e0c700000000000000009``, a declared 12 against an + 11-octet option -- because ``MPTCPFastclose`` had no reserved field and ``test`` was + the only octet between ``length`` and ``key``, where figure 14 draws 4 subtype bits + followed by 12 reserved ones. + + """ + packed = make('_make_mptcp_fastclose', 'MP_FASTCLOSE', key=9) + + self.assertLengthMatchesWire(packed, 12) + self.assertEqual(packed, bytes([0x1E, 0x0C, 0x70, 0x00]) + (9).to_bytes(8, 'big')) + + def test_spec_octets_parse_back(self) -> None: + """Hand-built figure 14 octets parse, with the key intact. + + Pre-fix ``_read_mptcp_fastclose`` required ``schema.length == 16``, so a + spec-correct 12-octet option off the wire raised ``ProtocolError: TCP: [OptNo 30] + invalid format`` -- and so did every option the maker produced, since it declared + the correct 12. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + raw = build_tcp_segment(MP_FASTCLOSE_SPEC_OCTETS) + data = TCP(raw, len(raw)).info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_FASTCLOSE) + self.assertEqual(data.length, 12) + self.assertEqual(data.rkey, 0xCCCCCCCCCCCCCCCC) + + def test_public_constructor_round_trips(self) -> None: + """MP_FASTCLOSE builds through ``TCP()`` and reports itself back. + + This is the case ``tcp-mptcp/MP_FASTCLOSE`` records in + :data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES`: the maker's + *correct* length failing the parser's wrong check made the whole construction raise. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP( + options=[(Enum_Option.Multipath_TCP, { + 'subtype': Enum_MPTCPOption.MP_FASTCLOSE, + 'key': 0x0102030405060708, + })], + **TCP_BASE, # type: ignore[arg-type] + ) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.length, 12) + self.assertEqual(data.rkey, 0x0102030405060708) + self.assertEqual( + bytes(tcp)[-12:], + bytes([0x1E, 0x0C, 0x70, 0x00]) + bytes.fromhex('0102030405060708'), + ) + + +class TCPMPTCPJoinSYNACKLengthUnitTests(LengthInvariantMixin, unittest.TestCase): + """Site 2 -- MP_JOIN-SYN/ACK is 16 octets: :rfc:`8684` section 3.2, figure 6.""" + + def test_maker_packs_sixteen_octets(self) -> None: + """``_make_join_synack`` packs 16 octets and declares 16. + + Pre-fix it declared **12** -- ``_make_join_syn``'s length for figure 5's SYN form -- + while packing the 16 octets figure 6 requires, since the HMAC here is 8 octets where + the SYN's token is 4. + + """ + packed = make('_make_join_synack', 'MP_JOIN', addr_id=5, hmac=b'\xDD' * 8, nonce=9) + + self.assertLengthMatchesWire(packed, 16) + self.assertEqual( + packed, + bytes([0x1E, 0x10, 0x10, 0x05]) + b'\xDD' * 8 + bytes([0x00, 0x00, 0x00, 0x09]), + ) + + def test_spec_octets_parse_back(self) -> None: + """Hand-built figure 6 octets parse in a SYN/ACK segment. + + Pre-fix ``_read_join_synack`` required ``schema.length == 20`` -- a value neither + figure 6 nor ``MPTCPJoinSYNACK`` produces, and one that contradicted this method's + own docstring figure -- so a spec-correct 16-octet option was rejected as an invalid + format. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + raw = build_tcp_segment(MP_JOIN_SYNACK_SPEC_OCTETS, syn=True, ack_flag=True) + data = TCP(raw, len(raw)).info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_JOIN) + self.assertEqual(data.length, 16) + self.assertEqual(data.addr_id, 5) + self.assertEqual(data.hmac, b'\xDD' * 8) + self.assertEqual(data.nonce, 9) + + def test_reconstruction_keeps_the_parsed_hmac(self) -> None: + """Rebuilding from parsed data preserves ``hmac``, which used to be dropped. + + ``_make_join_synack``'s ``if opt is not None:`` branch set ``backup``, ``addr_id`` + and ``nonce`` -- the last of those twice, on two consecutive lines -- but never + ``hmac``, so reconstructing a parsed MP_JOIN-SYN/ACK silently substituted the + ``bytes(8)`` default for the truncated HMAC that was actually on the wire. The HMAC + is the entire authentication payload of this form of the option, so an all-zero + substitute is not a cosmetic loss. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + raw = build_tcp_segment(MP_JOIN_SYNACK_SPEC_OCTETS, syn=True, ack_flag=True) + parsed = TCP(raw, len(raw)).info.options[Enum_Option.Multipath_TCP] + + rebuilt = TCP.__new__(TCP)._make_join_synack(Enum_MPTCPOption.MP_JOIN, parsed).pack() # pylint: disable=protected-access + + self.assertNotEqual(rebuilt[4:12], bytes(8), 'the HMAC must not come back all-zero') + self.assertEqual(rebuilt[4:12], b'\xDD' * 8) + self.assertEqual(rebuilt, MP_JOIN_SYNACK_SPEC_OCTETS) + self.assertEqual(Enum_Option.Multipath_TCP, rebuilt[0]) + + +class TCPMPTCPJoinACKLengthUnitTests(LengthInvariantMixin, unittest.TestCase): + """Site 3 -- MP_JOIN-ACK is 24 octets: :rfc:`8684` section 3.2, figure 7.""" + + def test_maker_packs_twenty_four_octets(self) -> None: + """``_make_join_ack`` packs 24 octets and declares 24. + + Pre-fix it declared **8**, a third of the truth, while packing the 24 octets figure 7 + requires: the 160-bit HMAC alone is 20. ``_read_join_ack`` already required 24, so + nothing this maker produced could be parsed back. + + """ + packed = make('_make_join_ack', 'MP_JOIN', hmac=b'\xBB' * 20) + + self.assertLengthMatchesWire(packed, 24) + self.assertEqual(packed, bytes([0x1E, 0x18, 0x10, 0x00]) + b'\xBB' * 20) + + def test_maker_output_satisfies_its_own_reader(self) -> None: + """What the maker declares is what the reader accepts. + + The two disagreed absolutely before the fix -- 8 against a guard of 24 -- so this is + the pairing that could not hold at all, rather than an off-by-one. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.corekit.multidict import OrderedMultiDict + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP.__new__(TCP) + schema = tcp._make_join_ack(Enum_MPTCPOption.MP_JOIN, hmac=b'\xBB' * 20) # pylint: disable=protected-access + object.__setattr__(schema, 'subtype', Enum_MPTCPOption.MP_JOIN) + + data = tcp._read_join_ack(schema, options=OrderedMultiDict()) # pylint: disable=protected-access + + self.assertEqual(data.length, 24) + self.assertEqual(data.hmac, b'\xBB' * 20) + + +class TCPMPTCPRemoveAddressLengthUnitTests(LengthInvariantMixin, unittest.TestCase): + """Site 4 -- REMOVE_ADDR is ``3 + n``: :rfc:`8684` section 3.4.2, figure 13.""" + + def test_two_address_ids_pack_five_octets(self) -> None: + """``addr_id=[1, 2]`` packs 5 octets and declares 5. + + Stated as its own test, not only as a case of the ``subTest`` sweep below, because + pytest 9.1.1 here has no ``pytest-subtests``: a failing ``subTest`` is reported but + its *parent* test still prints ``PASSED``, so a per-site regression is easier to read + from a test that fails outright. Pre-fix this packed ``1e04400102`` -- 5 octets + declaring 4. + + """ + packed = make('_make_mptcp_remove', 'REMOVE_ADDR', addr_id=[1, 2]) + + self.assertLengthMatchesWire(packed, 5) + self.assertEqual(packed, bytes([0x1E, 0x05, 0x40, 0x01, 0x02])) + + def test_length_tracks_the_number_of_address_ids(self) -> None: + """``3 + n`` for every ``n``, not a constant 4. + + Pre-fix, measured: ``addr_id=[1, 2]`` packed 5 octets declaring 4, ``addr_id=[]`` + packed 3 declaring 4, and ``addr_id=[1, 2, 3, 4]`` packed 7 declaring 4. Only + ``addr_id=[1]`` -- which is what ``examples.generators.options``' fixture passes -- + happened to agree, which is why the round-trip suite never saw this. + + """ + for addr_ids in ([], [1], [1, 2], [1, 2, 3, 4], list(range(1, 21))): + with self.subTest(addr_ids=addr_ids): + packed = make('_make_mptcp_remove', 'REMOVE_ADDR', addr_id=addr_ids) + + self.assertLengthMatchesWire(packed, 3 + len(addr_ids)) + self.assertEqual(packed, bytes([0x1E, 3 + len(addr_ids), 0x40]) + + bytes(addr_ids)) + + def test_public_constructor_reports_every_address_id(self) -> None: + """A multi-ID REMOVE_ADDR round-trips through ``TCP()`` with all IDs intact. + + The declared length is what + :attr:`~pcapkit.protocols.schema.transport.tcp.MPTCPRemoveAddress.addr_id` sizes its + list from, as ``pkt['length'] - 3``, so the old constant 4 truncated the parse to a + single ID as well as mis-declaring the pack. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP( + options=[(Enum_Option.Multipath_TCP, { + 'subtype': Enum_MPTCPOption.REMOVE_ADDR, + 'addr_id': [7, 8, 9], + })], + **TCP_BASE, # type: ignore[arg-type] + ) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.length, 6) + self.assertEqual(data.addr_id, (7, 8, 9)) + + +class TCPMPTCPPriorityLengthUnitTests(LengthInvariantMixin, unittest.TestCase): + """Site 5 -- MP_PRIO is 3 octets: :rfc:`8684` section 3.3.8, figure 11.""" + + def test_no_address_id_packs_three_octets(self) -> None: + """``addr_id=None`` packs 3 octets with no Address ID at all. + + Pre-fix this packed ``1e045000`` -- 4 octets, the last a phantom all-zero Address ID + no caller asked for. The constant ``length=4`` satisfied ``MPTCPPriority.addr_id``'s + own ``pkt['length'] == 4`` predicate, so declaring the legacy length *created* the + legacy field. + + """ + packed = make('_make_mptcp_prio', 'MP_PRIO') + + self.assertLengthMatchesWire(packed, 3) + self.assertEqual(packed, bytes([0x1E, 0x03, 0x50])) + + def test_backup_flag_is_packed_without_an_address_id(self) -> None: + """``B`` still reaches the wire in the 3-octet form.""" + packed = make('_make_mptcp_prio', 'MP_PRIO', backup=True) + + self.assertLengthMatchesWire(packed, 3) + self.assertEqual(packed, bytes([0x1E, 0x03, 0x51])) + + def test_explicit_address_id_keeps_the_legacy_four_octet_form(self) -> None: + """An explicit ``addr_id`` still packs :rfc:`6824`'s 4-octet form. + + :rfc:`8684` removed the field, but the schema and ``_read_mptcp_prio`` both still + accept it, so the length has to follow whether one was actually given rather than + collapsing to 3 unconditionally. + + """ + packed = make('_make_mptcp_prio', 'MP_PRIO', addr_id=7) + + self.assertLengthMatchesWire(packed, 4) + self.assertEqual(packed, bytes([0x1E, 0x04, 0x50, 0x07])) + + def test_reconstruction_keeps_the_parsed_backup_flag(self) -> None: + """Rebuilding from parsed data preserves ``backup``, which used to be dropped. + + ``_make_mptcp_prio``'s ``if opt is not None:`` branch set only ``addr_id``, so a + parsed MP_PRIO with ``B=1`` was rebuilt with ``B=0`` -- and ``B`` is the entire + payload of this option, so the rebuilt option said the opposite of the original. + The same shape as ``_make_join_synack``'s dropped ``hmac``. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + raw = build_tcp_segment(bytes([0x1E, 0x03, 0x51, 0x00])) + parsed = TCP(raw, len(raw)).info.options[Enum_Option.Multipath_TCP] + self.assertTrue(parsed.backup, 'the hand-built option must carry B=1 to begin with') + + rebuilt = TCP.__new__(TCP)._make_mptcp_prio(Enum_MPTCPOption.MP_PRIO, parsed).pack() # pylint: disable=protected-access + + self.assertEqual(rebuilt, bytes([0x1E, 0x03, 0x51])) + self.assertEqual(Enum_Option.Multipath_TCP, rebuilt[0]) + + +class TCPMPTCPDSSLengthUnitTests(LengthInvariantMixin, unittest.TestCase): + """Site 6 -- DSS packs what it declares: :rfc:`8684` section 3.3, figure 9.""" + + def test_data_ack_only_packs_eight_octets(self) -> None: + """``A`` set, ``a`` clear: head 4 + a 4-octet Data ACK = 8. + + Pre-fix this packed ``1e082001`` -- 4 octets declaring 8 -- because ``MPTCPDSS.ack`` + sized itself ``8 if pkt['flags']['a'] else 0`` and so contributed nothing at all. + The ``ack`` value the caller passed was not on the wire. + + """ + packed = make('_make_mptcp_dss', 'DSS', ack=1) + + self.assertLengthMatchesWire(packed, 8) + self.assertEqual(packed, bytes([0x1E, 0x08, 0x20, 0x01]) + (1).to_bytes(4, 'big')) + + def test_mapping_only_packs_sixteen_octets(self) -> None: + """``M`` set, ``m`` clear: head 4 + DSN 4 + SSN 4 + Data-Level Length 2 + Checksum 2. + + Pre-fix ``MPTCPDSS.dsn`` contributed 0 octets here for the same reason as ``ack``, so + this packed 12 against a declared 16 and the ``dsn`` value was lost. + + """ + packed = make('_make_mptcp_dss', 'DSS', dsn=2, ssn=3, dl_len=4, checksum=b'\x00\x00') + + self.assertLengthMatchesWire(packed, 16) + self.assertEqual( + packed, + bytes([0x1E, 0x10, 0x20, 0x04]) + (2).to_bytes(4, 'big') + (3).to_bytes(4, 'big') + + (4).to_bytes(2, 'big') + b'\x00\x00', + ) + + def test_generator_override_arguments_pack_twenty_octets(self) -> None: + """The exact arguments ``examples.generators.options`` uses pack their declared 20. + + This is #576's filed measurement: ``ack=1, dsn=2, ssn=3, dl_len=4, + checksum=b'\\x00\\x00'`` packed **12** octets against a declared 20, with neither + ``ack`` nor ``dsn`` on the wire. ``tcp-mptcp/DSS`` still read ``'OK'`` in the + round-trip suite, because construct -> parse -> reconstruct produced the same wrong + 12 octets every time and the suite only checks the cycle is self-consistent. + + """ + packed = make('_make_mptcp_dss', 'DSS', ack=1, dsn=2, ssn=3, dl_len=4, + checksum=b'\x00\x00') + + self.assertLengthMatchesWire(packed, 20) + self.assertEqual( + packed, + bytes([0x1E, 0x14, 0x20, 0x05]) + (1).to_bytes(4, 'big') + (2).to_bytes(4, 'big') + + (3).to_bytes(4, 'big') + (4).to_bytes(2, 'big') + b'\x00\x00', + ) + + def test_ack_and_dsn_survive_a_real_byte_round_trip(self) -> None: + """A DSS built through ``TCP()``, packed, and re-parsed still carries ``ack``/``dsn``. + + The re-parse is the point, and asserting on the *constructed* ``tcp.info`` instead + would prove nothing: ``TCP._make_mode_mp`` hands the schema it just built straight to + the matching ``_read_mptcp_*``, so ``data.ack`` reads back the attribute the caller + set regardless of how many octets the field packed. Measured -- that assertion passes + against the unfixed tree. Going out to bytes and back in is what exposes a field that + declared 4 octets and packed 0. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP( + options=[(Enum_Option.Multipath_TCP, { + 'subtype': Enum_MPTCPOption.DSS, + 'ack': 0x11223344, 'dsn': 0x55667788, 'ssn': 3, 'dl_len': 4, + 'checksum': b'\x00\x00', + })], + **TCP_BASE, # type: ignore[arg-type] + ) + raw = bytes(tcp) + reparsed = TCP(raw, len(raw)).info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(reparsed.length, 20) + self.assertEqual(reparsed.ack, 0x11223344) + self.assertEqual(reparsed.dsn, 0x55667788) + self.assertEqual(reparsed.ssn, 3) + self.assertEqual(reparsed.dl_len, 4) + + +class TCPMPTCPDSSExtendedFieldsUnitTests(LengthInvariantMixin, unittest.TestCase): + """Site 6, second half -- the 8-octet DSS forms, which could not be packed at all. + + ``MPTCPDSS.ack``/``.dsn`` used ``NumberField(length=)``, and that field latches + ``_need_process = True`` at ``__init__`` from the placeholder length ``-1`` and never + clears it, so once ``__call__`` resolved a real width of 8 and rebuilt the template as + ``>Q``, ``pre_process`` handed :func:`struct.pack` bytes for an integer template. Every + test in this class raised ``struct.error: required argument is not an integer`` before the + fix -- not a wrong length, no length at all. + + """ + + def test_extended_data_ack_packs_eight_octets(self) -> None: + """``a`` set: the Data ACK widens to 8 octets, head 4 + 8 = 12.""" + packed = make('_make_mptcp_dss', 'DSS', ack=1 << 40) + + self.assertLengthMatchesWire(packed, 12) + self.assertEqual(packed, + bytes([0x1E, 0x0C, 0x20, 0x03]) + (1 << 40).to_bytes(8, 'big')) + + def test_extended_dsn_packs_eight_octets(self) -> None: + """``m`` set: the DSN widens to 8 octets, head 4 + 8 + SSN 4 + 2 + 2 = 20. + + The flags octet is ``0x0c``: figure 9 orders the low bits ``F|m|M|a|A``, so with only + ``M`` and ``m`` set the octet is ``m << 3 | M << 2``. + + """ + packed = make('_make_mptcp_dss', 'DSS', dsn=2 << 40, ssn=3, dl_len=4, + checksum=b'\x00\x00') + + self.assertLengthMatchesWire(packed, 20) + self.assertEqual( + packed, + bytes([0x1E, 0x14, 0x20, 0x0C]) + (2 << 40).to_bytes(8, 'big') + + (3).to_bytes(4, 'big') + (4).to_bytes(2, 'big') + b'\x00\x00', + ) + + def test_all_flags_set_packs_the_rfc_maximum_of_twenty_eight(self) -> None: + """Every flag set gives 28 octets, the maximum section 3.3 states in prose. + + The prose figure is the independent check on the arithmetic: "the maximum length of + this option, with all flags set, is 28 octets" is stated in words, not derived from + the diagram, so matching it confirms the per-field widths add up as figure 9 draws + them. + + """ + packed = make('_make_mptcp_dss', 'DSS', data_fin=True, ack=1 << 40, dsn=2 << 40, + ssn=3, dl_len=4, checksum=b'\x00\x00') + + self.assertLengthMatchesWire(packed, 28) + self.assertEqual( + packed, + bytes([0x1E, 0x1C, 0x20, 0x1F]) + (1 << 40).to_bytes(8, 'big') + + (2 << 40).to_bytes(8, 'big') + (3).to_bytes(4, 'big') + (4).to_bytes(2, 'big') + + b'\x00\x00', + ) + + +class TCPMPTCPJoinSYNIsCorrectUnitTests(LengthInvariantMixin, unittest.TestCase): + """Not a defect -- MP_JOIN-SYN is 12 octets: :rfc:`8684` section 3.2, figure 5. + + #576's body groups the three MP_JOIN forms together and a comment on the issue corrects + the attribution to the SYN/ACK and ACK forms only. Pinned here so a later pass reading + the body alone does not "correct" a number that is already right: head 4 (``Kind`` 1 + + ``Length`` 1 + subtype/rsv/``B`` 1 + ``Address ID`` 1) + the receiver's token 4 + the + sender's random number 4 = 12, which is what figure 5 labels ``Length = 12``, what + ``_make_join_syn`` writes, and what ``_read_join_syn`` guards on. + + """ + + def test_maker_packs_twelve_octets(self) -> None: + """``_make_join_syn`` packs 12 octets and declares 12, before and after #576.""" + packed = make('_make_join_syn', 'MP_JOIN', addr_id=1, token=2, nonce=3) + + self.assertLengthMatchesWire(packed, 12) + self.assertEqual( + packed, + bytes([0x1E, 0x0C, 0x10, 0x01]) + (2).to_bytes(4, 'big') + (3).to_bytes(4, 'big'), + ) + + def test_spec_octets_parse_back_in_a_syn_segment(self) -> None: + """Hand-built figure 5 octets parse, confirming the reader guard is right too.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + raw = build_tcp_segment(bytes([0x1E, 0x0C, 0x11, 0x02]) + + (3).to_bytes(4, 'big') + (4).to_bytes(4, 'big')) + data = TCP(raw, len(raw)).info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_JOIN) + self.assertEqual(data.length, 12) + self.assertTrue(data.backup) + self.assertEqual(data.addr_id, 2) + self.assertEqual(data.token, 3) + self.assertEqual(data.nonce, 4) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py index 47d40d8ef..19652b93c 100644 --- a/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py @@ -73,11 +73,16 @@ ``_make_mptcp_*`` directly, because that in-memory, no-byte-round-trip path is exactly what exposed the defect. ``MP_JOIN`` is left out: it fails independently with ``AttributeError: 'TCP' object has no attribute '_flags'`` (``_make_mptcp_join`` reads -``self._flags``, which only the parse path ever sets), which is not this issue. ``MP_FASTCLOSE`` -is also left out, for a reason this fix does not touch: fixing ``subtype`` gets its -construction *past* the ``AttributeError`` this module would otherwise still see and into a -second, independent defect (its declared length agrees with neither its own maker nor its own -parser) tracked as GitHub issue #576. +``self._flags``, which only the parse path ever sets), which is not this issue. + +``MP_FASTCLOSE`` *is* covered, as of #576. When this module was written it was left out: fixing +``subtype`` got its construction past the ``AttributeError`` and into a second, independent +defect -- its declared length agreed with neither its own schema nor its own parser -- so the +case here could only assert the *shape of the exception* that defect raised. #576 has since +corrected all three sites, so the assertion is made directly. The length arithmetic itself is +covered per option in +:mod:`tests.protocols.transport.test_tcp_mptcp_length_arithmetic_unit`; what stays here is +only the ``subtype`` question this module is about. """ from __future__ import annotations @@ -208,27 +213,31 @@ def test_mp_fail_subtype_round_trips(self) -> None: self.assertEqual(data.subtype, Enum_MPTCPOption.MP_FAIL) - def test_mp_fastclose_still_fails_but_no_longer_on_subtype(self) -> None: - """``MP_FASTCLOSE`` no longer raises the ``subtype`` ``AttributeError`` this fixes. + def test_mp_fastclose_builds_and_reports_its_subtype(self) -> None: + """``MP_FASTCLOSE`` builds through ``TCP()`` and reports its ``subtype``. + + This was ``test_mp_fastclose_still_fails_but_no_longer_on_subtype`` when #579 + landed: it pinned that MP_FASTCLOSE no longer raised the ``subtype`` + ``AttributeError`` that #566 was about, while still raising ``ProtocolError: + TCP: [OptNo 30] invalid format`` from the *other* defect it named -- the + length disagreement filed as #576. - It still cannot be built through ``TCP()`` -- #576, not this issue -- so this pins - that the *remaining* failure is the length mismatch, not a regression back to the - defect this module is otherwise about. + #576 has since fixed that disagreement at all three sites (the parser's guard, + which required 16; the schema, which packed 11; and the maker, which already + declared the RFC's 12), so the construction now succeeds and the assertion this + module exists for -- that ``subtype`` survives construction -- can be made + directly instead of through the shape of an exception. """ from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption - from pcapkit.utilities.exceptions import ProtocolError - - with self.assertRaises(ProtocolError) as ctx: - build_mptcp_option(Enum_MPTCPOption.MP_FASTCLOSE, key=9) - - # NOTE: bare 'invalid format' occurs at some 205 sites across 13 modules (see - # Gap.fragment's docstring in test_option_roundtrip_unit.py), so it does not pin - # anything on its own. The alias plus the bracketed option-number code this message - # actually carries -- 'TCP: [OptNo 30] invalid format' -- narrows it to the one option - # that can print 30 (Multipath_TCP). - self.assertIn('TCP: [OptNo 30] invalid format', str(ctx.exception)) - self.assertNotIn('subtype', str(ctx.exception)) + from pcapkit.const.tcp.option import Option as Enum_Option + + tcp = build_mptcp_option(Enum_MPTCPOption.MP_FASTCLOSE, key=9) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_FASTCLOSE) + self.assertEqual(data.length, 12) + self.assertEqual(data.rkey, 9) if __name__ == '__main__': diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 400cdef7d..1efa219d7 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -950,11 +950,19 @@ def mark(schema, length: int, subtype: MPTCPOption): MPTCPOption.MP_JOIN, ), options=options) + # NOTE: :rfc:`8684` section 3.2 figure 6 gives MP_JOIN-SYN/ACK as 16 + # octets -- kind (1) + length (1) + subtype/flags (1) + addr_id (1) + + # the truncated HMAC (8) + the nonce (4) -- which is also exactly what + # MPTCPJoinSYNACK packs. These two cases used 20 (accept) and 19 + # (reject), the pre-#576 guard's own wrong constant, and so pinned the + # defect rather than the RFC. The rejection case is now 20: the value the + # guard used to *require*, which no MP_JOIN form produces, so it stays a + # genuine rejection rather than an off-by-one near the correct length. proto._flags = {Flags.SYN, Flags.ACK} join_synack = mark( MPTCPJoinSYNACK(test={'subtype': MPTCPOption.MP_JOIN.value, 'backup': 0}, addr_id=1, hmac=b'12345678', nonce=3), - 20, + 16, MPTCPOption.MP_JOIN, ) self.assertEqual(proto._read_mode_mp(join_synack, options=options).hmac, b'12345678') @@ -962,7 +970,7 @@ def mark(schema, length: int, subtype: MPTCPOption): proto._read_mode_mp(mark( MPTCPJoinSYNACK(test={'subtype': MPTCPOption.MP_JOIN.value, 'backup': 0}, addr_id=1, hmac=b'12345678', nonce=3), - 19, + 20, MPTCPOption.MP_JOIN, ), options=options) @@ -1009,9 +1017,15 @@ def mark(schema, length: int, subtype: MPTCPOption): MPTCPOption.ADD_ADDR, ), options=options) + # NOTE: length 5, not 4: :rfc:`8684` section 3.4.2 figure 13 gives + # ``Length = 3 + n``, so a two-Address-ID REMOVE_ADDR is 5 octets. The + # reader only requires ``>= 3`` and does not cross-check the list against + # the length, so 4 passed -- but it enshrined the very constant #576 + # removed from ``_make_mptcp_remove``, which is worth not leaving in a + # test as though it were correct. remove = mark( MPTCPRemoveAddress(test={'subtype': MPTCPOption.REMOVE_ADDR.value}, addr_id=[1, 2]), - 4, + 5, MPTCPOption.REMOVE_ADDR, ) self.assertEqual(proto._read_mode_mp(remove, options=options).addr_id, (1, 2)) @@ -1053,16 +1067,23 @@ def mark(schema, length: int, subtype: MPTCPOption): MPTCPOption.MP_FAIL, ), options=options) + # NOTE: :rfc:`8684` section 3.5 figure 14 gives MP_FASTCLOSE as 12 octets + # -- kind (1) + length (1) + subtype-and-reserved (2) + the receiver's key + # (8) -- which is what MPTCPFastclose packs since #576 added its missing + # reserved octet. These two cases used 16 (accept) and 15 (reject), the + # pre-#576 guard's own wrong constant. The rejection case is now 16, the + # value the guard used to require and that the RFC never produces for this + # option. fastclose = mark( MPTCPFastclose(test={'subtype': MPTCPOption.MP_FASTCLOSE.value}, key=123), - 16, + 12, MPTCPOption.MP_FASTCLOSE, ) self.assertEqual(proto._read_mode_mp(fastclose, options=options).rkey, 123) with self.assertRaises(ProtocolError): proto._read_mode_mp(mark( MPTCPFastclose(test={'subtype': MPTCPOption.MP_FASTCLOSE.value}, key=123), - 15, + 16, MPTCPOption.MP_FASTCLOSE, ), options=options)