diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index 6fd6a798cb..0e9de0e3db 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -9,7 +9,7 @@ from pcapkit.corekit.fields.numbers import NumberField from pcapkit.corekit.multidict import OrderedMultiDict from pcapkit.utilities.compat import List -from pcapkit.utilities.exceptions import FieldValueError +from pcapkit.utilities.exceptions import FieldValueError, ProtocolError __all__ = [ 'ListField', 'OptionField', @@ -29,6 +29,44 @@ _TS = TypeVar('_TS', bound='Schema') +class _ShortReadTracker: + """Stream proxy recording whether a read came back short. + + Args: + file: Stream to read from. + + Only the ``read``, ``seek`` and ``tell`` calls that + :meth:`Schema.unpack ` and + :meth:`OptionField.unpack` make are delegated. + + """ + + __slots__ = ('_file', 'short') + + def __init__(self, file: 'IO[bytes]') -> 'None': + self._file = file + #: bool: Whether a read since the last reset returned fewer octets than + #: it asked for. + self.short = False + + def read(self, size: 'Optional[int]' = -1) -> 'bytes': + """Read ``size`` octets, noting a short read.""" + if size is None: + size = -1 + data = self._file.read(size) + if 0 <= size and len(data) < size: + self.short = True + return data + + def seek(self, offset: 'int', whence: 'int' = io.SEEK_SET) -> 'int': + """Seek the underlying stream.""" + return self._file.seek(offset, whence) + + def tell(self) -> 'int': + """Tell the underlying stream's position.""" + return self._file.tell() + + class ListField(FieldBase[List[_TL]], Generic[_TL]): """Field list for protocol fields. @@ -362,6 +400,8 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list Raises: FieldValueError: If an option consumes nothing from ``buffer``, since the loop below has then no way to get past it. + ProtocolError: If an option that begins with octets present reads + past the end of the data. """ length = self._length @@ -421,8 +461,24 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list # number of octets to rewind to get back to the front of the option. type_field = self._type_field + # NOTE: An option whose declared length runs past the octets behind it + # reads short, and every field reading off the end pads the missing + # octets with zeros, so the option would rebuild octets that were never + # there: a HIP ``ACK`` declaring 256 octets with 4 present, or an MH + # ``PadN`` declaring 3 with none. Nothing downstream can tell, because + # the schema records the zeros as values. C.f. #1325. + # + # So the stream is read through a proxy that notes any short read, and + # an option that began with at least one octet present and then read + # short is rejected. An option that begins at the end of the data reads + # nothing at all and is left to the end-of-option-list and progress + # handling below, which is what an option area declared longer than its + # octets relies on. + tracker = file = _ShortReadTracker(file) # type: ignore[assignment] + temp = [] # type: list[_TS] while length > 0: + tracker.short = False if type_field is None: # unpack option type using base schema meta = self._base_schema.unpack(file, length, packet) # type: ignore[call-arg,misc,var-annotated] @@ -443,6 +499,13 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list # unpack option using option schema data = schema.unpack(file, length, packet) # type: ignore[call-arg,misc,var-annotated] + if tracker.short and consumed > 0: + raise ProtocolError( + f'Field {self.name} has an option that runs past the end of ' + f'the data: {code!r} at offset {offset - start} of ' + f'{self._length}, with only {file.tell() - offset} octet(s) ' + f'of it present' + ) new_packet[self.name].add(code, data) temp.append(data) diff --git a/pcapkit/protocols/internet/ipv6_ext.py b/pcapkit/protocols/internet/ipv6_ext.py index bd133225b2..34f32e2c16 100644 --- a/pcapkit/protocols/internet/ipv6_ext.py +++ b/pcapkit/protocols/internet/ipv6_ext.py @@ -121,7 +121,6 @@ The house convention for a declared length that does not fit what remains is warn-and-clip: emit a warning naming what was declared against what is left, then read only what is left. See -:func:`pcapkit.protocols.schema.misc.pcapng.bounded_option`, :func:`pcapkit.protocols.schema.misc.pcapng.bounded_area` and :meth:`pcapkit.protocols.misc.pcapng.PCAPNG.read`. diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 3e9f716045..ff47268f01 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -1143,6 +1143,17 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True, meth = name[0] block = meth(schema.block, header=schema) + # NOTE: An option or record area holding a malformed entry is kept as + # the octets captured, as one :obj:`bytes` item (see + # :class:`~pcapkit.protocols.schema.misc.pcapng.OptionAreaField`), and + # parses as no options. The octets are kept on the block as + # ``options_raw`` or ``records_raw``, which the block's ``make`` writes + # back verbatim (:issue:`1325`). + for name in ('records', 'options'): + area = getattr(schema.block, name, None) + if isinstance(area, list) and len(area) == 1 and isinstance(area[0], bytes): + block.__update__([(f'{name}_raw', area[0])]) + if not _read: # move backward to the beginning of the packet self._file.seek(0, io.SEEK_SET) @@ -1626,6 +1637,21 @@ def _make_data(cls, data: 'Data_PCAPNG') -> 'dict[str, Any]': # type: ignore[ov 'block': data, } + @staticmethod + def _block_area(block: 'Data_PCAPNG', name: 'str') -> 'Any': + """Option or record area of a parsed block, for its ``make``. + + Args: + block: Block data model. + name: ``'options'`` or ``'records'``. + + Returns: + The area's octets if :meth:`read` kept it as captured, as + ``_raw``; otherwise the parsed ````. + + """ + return getattr(block, f'{name}_raw', getattr(block, name)) + @staticmethod def _flags_enum(enum: 'Type[PacketDirection] | Type[PacketReception]', value: 'int', option: 'Enum_OptionType') -> 'Any': @@ -2383,6 +2409,10 @@ def _read_pcapng_options(self, options_schema: 'list[Schema_Option]') -> 'Option """ options = OrderedMultiDict() # type: Option + # an area kept as captured holds no options to read + if any(isinstance(schema, bytes) for schema in options_schema): + return options + for schema in options_schema: type = schema.type name = self._lookup_registry(self.__option__, _option_key(type)) @@ -3478,6 +3508,10 @@ def _read_nrb_records(self, records_schema: 'list[Schema_NameResolutionRecord]') """ records = OrderedMultiDict() # type: Record + # an area kept as captured holds no records to read + if any(isinstance(schema, bytes) for schema in records_schema): + return records + for schema in records_schema: type = schema.type name = self._lookup_registry(self.__record__, type) @@ -3789,7 +3823,7 @@ def _make_block_shb(self, block: 'Optional[Data_SectionHeaderBlock]' = None, *, major_version = block.version.major minor_version = block.version.minor section_length = block.section_length - options = block.options + options = self._block_area(block, 'options') else: if major_version is None: major_version = version[0] @@ -3838,7 +3872,7 @@ def _make_block_idb(self, block: 'Optional[Data_InterfaceDescriptionBlock]' = No if block is not None: linktype_val = block.linktype snaplen = block.snaplen - options = block.options + options = self._block_area(block, 'options') else: linktype_val = self._make_index(linktype, linktype_default, namespace=linktype_namespace, # type: ignore[assignment] reversed=linktype_reversed, pack=False) @@ -3886,7 +3920,7 @@ def _make_block_epb(self, block: 'Optional[Data_EnhancedPacketBlock]' = None, *, captured_len = block.captured_len original_len = block.original_len packet_data = self._make_packet_data(block) - options = block.options + options = self._block_area(block, 'options') timestamp_high, timestamp_low = self._make_timestamp(timestamp, interface_id=interface_id) if captured_len is None: @@ -3980,8 +4014,8 @@ def _make_block_nrb(self, block: 'Optional[Data_NameResolutionBlock]' = None, *, """ if block is not None: - records = block.records - options = block.options + records = self._block_area(block, 'records') + options = self._block_area(block, 'options') if records is not None: records_value, records_length = self._make_nrb_records(records) @@ -4021,7 +4055,7 @@ def _make_block_isb(self, block: 'Optional[Data_InterfaceStatisticsBlock]' = Non if block is not None: interface_id = block.interface_id timestamp = block.timestamp_epoch - options = block.options + options = self._block_area(block, 'options') timestamp_high, timestamp_low = self._make_timestamp(timestamp, interface_id=interface_id) # NOTE: ``isb_starttime`` and ``isb_endtime`` are scaled by the @@ -4120,7 +4154,7 @@ def _make_block_dsb(self, block: 'Optional[Data_DecryptionSecretsBlock]' = None, if block is not None: secrets_type_val = block.secrets_type secrets_data = block.secrets_data - options = block.options + options = self._block_area(block, 'options') else: secrets_type_val = self._make_index(secrets_type, secrets_type_default, namespace=secrets_type_namespace, # type: ignore[assignment] reversed=secrets_type_reversed, pack=False) @@ -4250,7 +4284,7 @@ def _make_block_packet(self, block: 'Optional[Data_PacketBlock]' = None, *, captured_len = block.captured_len original_len = block.original_len packet_data = self._make_packet_data(block) - options = block.options + options = self._block_area(block, 'options') timestamp_high, timestamp_low = self._make_timestamp(timestamp, interface_id=interface_id) if captured_len is None: @@ -4287,7 +4321,7 @@ def _make_block_packet(self, block: 'Optional[Data_PacketBlock]' = None, *, length2=total_length + 32 + packet_len, ) - def _make_pcapng_options(self, options: 'Option | list[Schema_Option | tuple[Enum_OptionType, dict[str, Any]] | bytes]', + def _make_pcapng_options(self, options: 'Option | list[Schema_Option | tuple[Enum_OptionType, dict[str, Any]] | bytes] | bytes', namespace: 'str') -> 'tuple[list[Schema_Option | bytes], int]': """Make options for PCAP-NG. @@ -4299,6 +4333,10 @@ def _make_pcapng_options(self, options: 'Option | list[Schema_Option | tuple[Enu Tuple of options and total length of options. """ + # an option area kept as captured is written back verbatim + if isinstance(options, bytes): + return [options], len(options) + has_endofopt = False total_length = 0 if isinstance(options, list): @@ -5725,7 +5763,7 @@ def _make_option_pack_hash(self, type: 'Enum_OptionType', option: 'Optional[Data data=hash, ) - def _make_nrb_records(self, records: 'Record | list[Schema_NameResolutionRecord | tuple[Enum_RecordType, dict[str, Any]] | bytes]') -> 'tuple[list[Schema_NameResolutionRecord | bytes], int]': + def _make_nrb_records(self, records: 'Record | list[Schema_NameResolutionRecord | tuple[Enum_RecordType, dict[str, Any]] | bytes] | bytes') -> 'tuple[list[Schema_NameResolutionRecord | bytes], int]': """Make name resolution records for PCAP-NG. Args: @@ -5735,6 +5773,10 @@ def _make_nrb_records(self, records: 'Record | list[Schema_NameResolutionRecord Tuple of name resolution records and total length of the records. """ + # a record area kept as captured is written back verbatim + if isinstance(records, bytes): + return [records], len(records) + has_record_end = False total_length = 0 if isinstance(records, list): diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index 9a55f1c7fd..4e1a25f817 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -62,7 +62,7 @@ if TYPE_CHECKING: from ipaddress import IPv4Address, IPv4Interface, IPv6Address, IPv6Interface - from typing import Any, Callable, DefaultDict, Iterable, Optional, Type + from typing import IO, Any, Callable, DefaultDict, Iterable, Optional, Type from typing_extensions import Literal, Self @@ -251,12 +251,11 @@ def nonnegative(length: 'Callable[[dict[str, Any]], int]') -> 'Callable[[dict[st for a read -- there are no octets here -- and zero says that, where a negative says something :mod:`struct` cannot express. - Clamping rather than refusing is the choice :func:`bounded_option` and - :func:`bounded_area` already made, for the reason their docstrings give: a - block read has no catch point above :meth:`FieldBase.unpack + Flooring rather than refusing is the choice :func:`bounded_area` makes too: + a block read has no catch point above :meth:`FieldBase.unpack `, so one refusal aborts the whole extraction rather than one block, so a capture cut short by its - snapshot length must keep parsing. The end of the file is the one case that is *not* a clamp, since + snapshot length must keep parsing. The end of the file is the one case that is *not* a floor, since there no block is being read at all -- see :meth:`PCAPNG._check_block_floor `, which reports it as the :exc:`~pcapkit.utilities.exceptions.StreamEOFError` the frame loop @@ -264,11 +263,11 @@ def nonnegative(length: 'Callable[[dict[str, Any]], int]') -> 'Callable[[dict[st Note: Unlike :func:`bounded_option` this needs no ``__length__`` opt-out for - the packing path, because it floors a *difference* rather than clamping + the packing path, because it floors a *difference* rather than checking against the remaining area: a negative difference is not a legitimate thing to pack either -- ``struct.pack('-8s', ...)`` raises exactly as - ``calcsize`` does -- where clamping against the remainder would have - shortened a perfectly good option. + ``calcsize`` does -- where checking against the remainder would have + refused a perfectly good option. That distinction is what keeps the three decryption-secrets payloads -- :attr:`UnknownSecrets.data`, :attr:`TLSKeyLog.data` and @@ -294,72 +293,34 @@ def callback(pkt: 'dict[str, Any]') -> 'int': def bounded_option(length: 'Callable[[dict[str, Any]], int]') -> 'Callable[[dict[str, Any]], int]': - """Clamp an option or record payload to the octets its area still declares. + """Refuse an option or record payload that runs past its area. Args: length: Callback computing the payload's nominal length, as the option's or record's own declared length field gives it. Returns: - A callback returning that length, never past the octets the enclosing - option or record area has left to give. - - An option's length is a 16-bit wire field, so it can declare up to 65,535 - octets of payload from four octets of option header. Nothing bounds that - against the area the option sits in: :meth:`OptionField.unpack - ` subtracts each - option's *parsed* size from the area it was given but never checks the - declared size against it, and :meth:`FieldBase.unpack - ` zero-pads any shortfall - within :data:`~pcapkit.corekit.fields.field._MAX_ZERO_PAD_SHORTFALL` - unconditionally -- deliberately, since that ceiling is the full span of a - 16-bit length and a capture cut short by its snapshot length must still - parse. Repeating such an option across many blocks therefore synthesised - padding without limit: 2,000 Enhanced Packet Blocks in 80,048 octets, each - with one option declaring 65,535 against none present, produced 131,070,000 - octets of zero padding, an amplification of 1,637x linear in the block - count. The field layer's own budget already covers the 32-bit band. - - The bound has to come from this layer because the field layer cannot see - it. What distinguishes the crafted case from the legitimate one is not the - shortfall's size -- both are inside a 16-bit length, which is why a plain - ``len(buffer) < length`` rejection does not work -- but whether the option - is inconsistent with the framing the block itself declares. Block Total - Length is authoritative and cross-checked against its own trailing copy, so - the area is ``length`` less the fixed fields, ``captured_len`` and its - padding; an option declaring more payload than that area has left is - malformed however complete the file behind it is. A snapshot-truncated - capture says so through ``captured_len`` instead, and leaves its options - whole, so it never trips this. - - Clamping rather than refusing keeps a capture cut short by its snapshot - length parsing: a block read has no catch point above - :meth:`FieldBase.unpack `, - so one refusal aborts the whole extraction rather than one block, and a - truncated capture would stop parsing at the cut instead of reporting the - frames before it. The clamp is - also history-independent -- it reads only this block's own declared framing - -- which a running threshold would not be. + A callback returning that length. + + Raises: + ProtocolError: If the payload declares more octets than the enclosing + option or record area has left. + + An option's length is a 16-bit wire field, so one four-octet option header + can declare 65,535 octets of payload. Read as declared, every shortfall + would be zero-padded at the field layer, and the zeros rebuilt as if they + had been captured (#594, #1325). So a payload that runs past the area is + refused here instead. Inside a block, :class:`OptionAreaField` then keeps + the whole option area as the octets captured, so the block still parses and + rebuilds byte for byte. Note: - The clamp is skipped when ``__length__`` is absent or negative, which is + The check is skipped when ``__length__`` is absent or negative, which is what :meth:`Schema.pack ` - leaves it as when no length is known. That matters here in a way it does - not for :func:`pcapkit.protocols.schema.transport.sctp.bounded`, whose - list fields ignore their length while packing: these are - :class:`~pcapkit.corekit.fields.strings.BytesField` and - :class:`~pcapkit.corekit.fields.strings.StringField` payloads, and - :meth:`FieldBase.pack ` - packs them through ``struct.pack('s', ...)``, which truncates - silently. A clamp applied while packing would therefore shorten a - perfectly good option rather than reject it. - - The nominal length still goes through :func:`nonnegative` first, on both - paths, since an option declaring less payload than the part of itself it - describes -- ``length`` below the four octets of an ``epb_hash`` or an - ``ns_dnsIP4addr`` record's own fields -- makes the subtraction negative - before there is anything to clamp it against, and a negative is not an - amount to read or to pack. + leaves it as when no length is known. The nominal length still goes + through :func:`nonnegative` first, on both paths, since an option + declaring less payload than the part of itself it describes makes the + subtraction negative. """ floored = nonnegative(length) @@ -371,13 +332,50 @@ def callback(pkt: 'dict[str, Any]') -> 'int': if not isinstance(remaining, int) or remaining < 0 or nominal <= remaining: return nominal - warn(f'PCAP-NG: option declares {nominal} octet(s) of payload with ' - f'{remaining} octet(s) left in its area; reading {remaining}', - SchemaWarning, stacklevel=stacklevel()) - return remaining + raise ProtocolError(f'PCAP-NG: option declares {nominal} octet(s) of payload ' + f'with {remaining} octet(s) left in its area') return callback +class OptionAreaField(OptionField): + """Option list that keeps a malformed option area as the octets captured. + + An option in the area that runs past it, or past the data, raises + :exc:`~pcapkit.utilities.exceptions.ProtocolError` from + :meth:`OptionField.unpack ` + or :func:`bounded_option`. A block has no catch point above its fields, so + raising would abort the whole extraction. This field instead returns the + area as a single :obj:`bytes` item, which packs back verbatim, and reports + no padding after it (#1325). + + """ + + def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list[Any]': + """Unpack field value from :obj:`bytes`. + + Args: + buffer: Field buffer. + packet: Packet data. + + Returns: + Unpacked field value, or the whole area as one :obj:`bytes` item if + an option in it is malformed. + + """ + if isinstance(buffer, bytes): + buffer = io.BytesIO(buffer) + start = buffer.tell() + try: + return super().unpack(buffer, packet) + except ProtocolError as error: + buffer.seek(start, io.SEEK_SET) + raw = buffer.read(max(self._length, 0)) + warn(f'PCAP-NG: {error}; option area of {len(raw)} octet(s) kept as captured', + ProtocolWarning, stacklevel=stacklevel()) + self._option_padding = 0 + return [raw] + + def bounded_area(length: 'Callable[[dict[str, Any]], int]') -> 'Callable[[dict[str, Any]], int]': """Clamp a packet block's option area to the octets the block itself holds. @@ -388,16 +386,15 @@ def bounded_area(length: 'Callable[[dict[str, Any]], int]') -> 'Callable[[dict[s Returns: A callback returning that span, never past the octets left of the block. - :func:`bounded_option` bounds a payload by the area, and the area by the - block's declared Block Total Length. That closes the band only while the - declared length is itself backed by real octets, and nothing checks that: - ``BlockType.post_process`` compares ``length`` against its own trailing copy - and never against the file. A block declaring 1,000,000 octets while holding - 36 therefore sizes its option area at 999,964, an option inside it declaring - 65,535 is under that and is not clamped, and 65,535 octets of zeros are - synthesised from 36 -- measured, 1,820x, with no warning. Clamping the area - to ``__length__`` as well removes the step: the payload is then bounded by - the octets the block was actually handed, whatever it declared. + :func:`bounded_option` refuses a payload that runs past the area, and the + area is sized from the block's declared Block Total Length. Nothing checks + that length against the file: ``BlockType.post_process`` compares + ``length`` only against its own trailing copy. A block declaring 1,000,000 + octets while holding 36 would therefore size its option area at 999,964, + read the trailing Block Total Length into it, and leave that field to be + zero-filled. Clamping the area to ``__length__`` keeps the trailing length + out of the area, so the area holds exactly the octets the block was handed + for it, whatever it declared. This is a no-op on every well-formed block rather than a second guess at the area. At the option field the only field still to come is the trailing Block @@ -993,7 +990,7 @@ class SectionHeaderBlock(BlockType, code=Enum_BlockType.Section_Header_Block): #: Section length. section_length: 'int' = Int64Field(callback=shb_byteorder_callback, default=0xFFFF_FFFF_FFFF_FFFF) #: Options. - options: 'list[Option]' = OptionField( + options: 'list[Option]' = OptionAreaField( length=nonnegative(lambda pkt: pkt['length'] - 28), base_schema=_OPT_Option, type_name='type', @@ -1317,7 +1314,7 @@ class InterfaceDescriptionBlock(BlockType, code=Enum_BlockType.Interface_Descrip #: Snap length. snaplen: 'int' = UInt32Field(default=0, callback=byteorder_callback) #: Options. - options: 'list[Option]' = OptionField( + options: 'list[Option]' = OptionAreaField( length=nonnegative(lambda pkt: pkt['length'] - 20), base_schema=_IF_Option, type_name='type', @@ -1449,7 +1446,7 @@ class EnhancedPacketBlock(BlockType, code=Enum_BlockType.Enhanced_Packet_Block): #: Padding. padding_data: 'bytes' = PaddingField(length=lambda pkt: (4 - pkt['captured_len'] % 4) % 4) #: Options. - options: 'list[Option]' = OptionField( + options: 'list[Option]' = OptionAreaField( # NOTE: The padding is recomputed here rather than read back from # ``padding_data``: a PaddingField is written straight into the schema # buffer while packing and never lands in the packet data, so its name @@ -1668,7 +1665,7 @@ class NameResolutionBlock(BlockType, code=Enum_BlockType.Name_Resolution_Block): #: Record total length. length: 'int' = UInt32Field(callback=byteorder_callback) #: Name resolution records. - records: 'list[NameResolutionRecord]' = OptionField( + records: 'list[NameResolutionRecord]' = OptionAreaField( length=nonnegative(lambda pkt: pkt['length'] - 12), base_schema=NameResolutionRecord, type_name='type', @@ -1676,7 +1673,7 @@ class NameResolutionBlock(BlockType, code=Enum_BlockType.Name_Resolution_Block): eool=Enum_RecordType.nrb_record_end, ) #: Options. - options: 'list[Option]' = OptionField( + options: 'list[Option]' = OptionAreaField( length=nonnegative(lambda pkt: pkt.get('__option_padding__', 0)), # key from OptionField base_schema=_NS_Option, type_name='type', @@ -1828,7 +1825,7 @@ class InterfaceStatisticsBlock(BlockType, code=Enum_BlockType.Interface_Statisti #: Timestamp (lower 32 bits). timestamp_low: 'int' = UInt32Field(callback=byteorder_callback) #: Options. - options: 'list[Option]' = OptionField( + options: 'list[Option]' = OptionAreaField( length=nonnegative(lambda pkt: pkt['length'] - 24), base_schema=_ISB_Option, type_name='type', @@ -2262,7 +2259,7 @@ class DecryptionSecretsBlock(BlockType, code=Enum_BlockType.Decryption_Secrets_B #: Padding. padding_data: 'bytes' = PaddingField(length=lambda pkt: (4 - pkt['secrets_length'] % 4) % 4) #: Options. - options: 'list[Option]' = OptionField( + options: 'list[Option]' = OptionAreaField( # NOTE: see EnhancedPacketBlock.options on why the padding is recomputed # here instead of being read back from ``padding_data``. length=nonnegative(lambda pkt: pkt['length'] - 20 - pkt['secrets_length'] @@ -2373,7 +2370,7 @@ class PacketBlock(BlockType, code=Enum_BlockType.Packet_Block): #: Padding. padding_data: 'bytes' = PaddingField(length=lambda pkt: (4 - pkt['captured_length'] % 4) % 4) #: Options. - options: 'list[Option]' = OptionField( + options: 'list[Option]' = OptionAreaField( # NOTE: see EnhancedPacketBlock.options on why the padding is recomputed # here instead of being read back from ``padding_data``. length=bounded_area(lambda pkt: pkt['length'] - 32 - pkt['captured_length'] diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index dd4e2684e6..c8bd577329 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -2001,82 +2001,49 @@ def test_an_option_area_longer_than_the_datagram_still_parses(self) -> None: ) def test_a_truncated_option_still_parses_its_declared_length(self) -> None: - """A capture cut short mid-option is tolerated, not just mid-header. C.f. #431, #572. + """A capture cut short mid-option is refused alone, and kept as captured in a frame. C.f. #431, #572, #1325. :meth:`test_an_option_area_longer_than_the_datagram_still_parses` above pins the *empty*-tail half of the #431 accommodation: an option area - that runs out before it starts. Nothing pinned the other half -- an - option that *does* start, declares more data than the capture actually - holds, and runs out partway through, the shape of a datagram cut short - by the snapshot length rather than one with no options at all. A - candidate fix for #554 (PR #571) turned that into an unwrapped - ``FieldValueError`` while the rest of the suite stayed green, because - nothing exercised it. See - ``TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length`` - for the same case on TCP, where the shortfall is simpler to reach. + that runs out before it starts. This is the other half -- an option + that *does* start, declares more data than the capture holds, and runs + out partway through. The header below sets ``ihl`` to 9 -- 16 declared octets of options -- - for an unassigned option (code 31) declaring ``length=12``, which asks - :class:`~pcapkit.protocols.schema.internet.ipv4.UnassignedOption`'s - ``data`` field (``BytesField(length=lambda pkt: pkt['length'] - 2)``, - 10 octets here) for more than the 6 octets actually behind it, and - :meth:`FieldBase.unpack ` - pads the short read with zero octets rather than raising -- the - same accommodation as the TCP case, reached the same way. The zeros go - on the *tail*, where the octets that were never read would have been; - before #604 they were placed at the front, which for a numeric field - corrupted the value outright. - - ``ihl`` has to declare *more* than the 8 octets actually present, - though, which the TCP case does not need. Unlike TCP, - :meth:`~pcapkit.protocols.internet.ipv4.IPv4._read_ipv4_options` sums - each option's self-*declared* ``length`` -- not what it actually - consumed -- and raises ``IPv4: invalid format`` once its loop over the - parsed options finishes, if that sum exceeds the declared option - area. Declaring exactly 8 does not skip the accommodation -- the short - ``data`` field is still read and tail-padded as above -- it just trips - that check afterwards, discarding the result before the test can - assert on it. Declaring 16 leaves headroom, at the cost of a second - effect: once the option loop's 16-octet budget outlives the 8 octets - its one real option consumed, the loop reads one further, fully - exhausted phantom option, decodes it as end-of-option-list (the same - mechanism the test above pins), and :meth:`Schema.unpack - ` hands the same 8 - octets to the schema a second time as padding, via the - ``option_padding`` rewind that #371 added -- code that predates #431 - and lives outside :meth:`OptionField.unpack - `. That is why - ``bytes(proto.__header__)`` does not round-trip to ``raw`` here and is - not asserted -- immaterial to what this test pins: the option's - declared ``length``, the full parsed options list, and the ``data`` - field's short-read reconstruction. + for an unassigned option (code 31) declaring ``length=12``, with only + 6 of its 10 data octets behind it. Those 4 missing octets used to be + read as zeros and rebuilt as if captured. :meth:`OptionField.unpack + ` now refuses + the option with :exc:`~pcapkit.utilities.exceptions.ProtocolError` + instead (#1325). Inside a frame the datagram is kept as the octets + captured, a :class:`~pcapkit.protocols.misc.raw.Raw` payload carrying + that error, and rebuilds byte for byte. """ from pcapkit.const.ipv4.option_number import OptionNumber from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.link.ethernet import Ethernet + from pcapkit.protocols.misc.raw import Raw + from pcapkit.utilities.exceptions import ProtocolError from tests._support import time_limit custom = OptionNumber.get(31) trailing = bytes.fromhex('aabbccddeeff') raw = (bytes.fromhex('4900001c00010000400600000a0000010a000002') + bytes([custom, 12]) + trailing) - with time_limit(): - proto = IPv4(raw, len(raw)) + with time_limit(), warnings.catch_warnings(): + warnings.simplefilter('ignore') + with self.assertRaisesRegex(ProtocolError, 'runs past the end of the data'): + IPv4(raw, len(raw)) - self.assertEqual(proto.info.hdr_len, 36) - self.assertEqual( - [(code, opt.length) for code, opt in proto.info.options.items(multi=True)], - [(custom, 12), (OptionNumber.EOOL, 1)], - ) - unassigned = next(opt for code, opt in proto.info.options.items(multi=True) - if code == custom) - # the six octets actually behind the option come first, where they were - # read, and the four synthesised for the ones that were not follow them. - # Before #604 they arrived the other way round. See #604 and - # ``FieldBaseShortReadPaddingSideTests`` in - # ``tests/corekit/test_fields_field.py``. - self.assertEqual(unassigned.data, trailing + b'\x00\x00\x00\x00') + frame = bytes.fromhex('0123456789ab' 'fedcba987654' '0800') + raw + proto = Ethernet(frame, len(frame)) + self.assertIsInstance(proto.payload, Raw) + self.assertIn('runs past the end of the data', str(proto.payload.info.error)) + # the option octets are kept as captured, with nothing synthesised + self.assertEqual(proto.payload.data, raw) + self.assertEqual(Ethernet.from_data(proto.info).data, frame) if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/misc/test_pcapng_option_area_kept_unit.py b/tests/protocols/misc/test_pcapng_option_area_kept_unit.py new file mode 100644 index 0000000000..782a0cc5df --- /dev/null +++ b/tests/protocols/misc/test_pcapng_option_area_kept_unit.py @@ -0,0 +1,128 @@ +# -*- coding: utf-8 -*- +"""A PCAP-NG option area holding a malformed option is kept as captured. + +GitHub issue #1325: an option or record whose declared length runs past its +area used to be clamped to the octets the area had left, so the block parsed +but rebuilt a different length or padding than it was read with. Now the whole +area is kept as the octets captured: the block's ``options`` (or ``records``) +are empty, ``options_raw`` (or ``records_raw``) holds the area, the block +rebuilds byte for byte, and a capture holding such a block still extracts. + +Every case builds its own octets in memory. :mod:`pcapkit` is imported inside +each test, after :func:`~tests._support.reimport_once_per_class`. + +""" + +import os +import struct +import tempfile +import unittest +import warnings + +from tests.protocols.misc.test_pcapng_roundtrip2_unit import PCAPNGTestCase + + +def _block(type_: int, body: bytes) -> bytes: + """A little-endian block of ``type_`` around ``body``.""" + length = len(body) + 12 + return struct.pack(' bytes: + return (b'\x0a\x0d\x0d\x0a' + struct.pack(' bytes: + return _block(1, struct.pack(' bytes: + return _block(6, struct.pack(' bytes: + return _block(5, struct.pack(' bytes: + return _block(4, records) + + +#: An option declaring 65,535 octets with none present, and one declaring 5 +#: with 4 present (#594's vector, and the smallest over-declaration). +OVER_ALL = bytes.fromhex('fa00ffff') +OVER_ONE = bytes.fromhex('fa000500 aabbccdd') + +#: (block octets, name of the area kept, the area's octets) +CASES = { + 'SHB 65535': (_shb(OVER_ALL), 'options', OVER_ALL), + # #1284: Option Length 3 cannot hold the custom option's 4-octet PEN + 'SHB custom shorter than PEN': (_shb(bytes.fromhex('ad0b0300 61626300')), 'options', + bytes.fromhex('ad0b0300 61626300')), + 'IDB 65535': (_idb(OVER_ALL), 'options', OVER_ALL), + 'EPB 65535': (_epb(OVER_ALL), 'options', OVER_ALL), + 'EPB over by one': (_epb(OVER_ONE), 'options', OVER_ONE), + # a complete option, then one declaring 8 octets with none left + 'EPB tail option': (_epb(bytes.fromhex('fa000200 aabb0000 fa000800')), 'options', + bytes.fromhex('fa000200 aabb0000 fa000800')), + 'ISB 65535': (_isb(OVER_ALL), 'options', OVER_ALL), + # an IPv4 record declaring 65,535 octets with none present + 'NRB record': (_nrb(bytes.fromhex('0100ffff')), 'records', bytes.fromhex('0100ffff')), +} + + +class TestPCAPNGOptionAreaKept(PCAPNGTestCase): + """Pin that a malformed option area is kept, not clamped (#1325).""" + + def test_the_area_is_kept_as_captured(self) -> None: + for case, (octets, name, area) in CASES.items(): + with self.subTest(case=case): + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + parsed = self._parse(octets) + block = parsed.info + self.assertEqual(getattr(block, f'{name}_raw'), area) + self.assertEqual(len(getattr(block, name)), 0) + + def test_the_block_rebuilds_byte_for_byte(self) -> None: + for case, (octets, _, _) in CASES.items(): + with self.subTest(case=case): + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + self.assertRebuilds(octets) + + def test_keeping_the_area_is_reported(self) -> None: + from pcapkit.utilities.warnings import ProtocolWarning + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + self._parse(CASES['EPB 65535'][0]) + self.assertTrue(any(issubclass(item.category, ProtocolWarning) + and 'kept as captured' in str(item.message) for item in caught)) + + def test_an_option_schema_alone_refuses_the_overrun(self) -> None: + from pcapkit.protocols.schema.misc.pcapng import UnknownOption + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaisesRegex(ProtocolError, 'left in its area'): + UnknownOption.unpack(OVER_ALL, len(OVER_ALL), {'byteorder': 'little'}) + + def test_a_capture_holding_the_block_still_extracts(self) -> None: + from pcapkit.foundation.extraction import Extractor + + capture = _shb(b'') + _idb(b'') + _epb(OVER_ALL) + _epb(b'') + with tempfile.TemporaryDirectory(prefix='w1325-') as tmp: + path = os.path.join(tmp, 'kept.pcapng') + with open(path, 'wb') as file: + file.write(capture) + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + extractor = Extractor(fin=path, nofile=True, store=True) + self.assertEqual(len(extractor.frame), 2) + self.assertEqual(extractor.frame[0].info.options_raw, OVER_ALL) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/misc/test_pcapng_roundtrip2_unit.py b/tests/protocols/misc/test_pcapng_roundtrip2_unit.py index ccfddde50b..8e9d10c7d4 100644 --- a/tests/protocols/misc/test_pcapng_roundtrip2_unit.py +++ b/tests/protocols/misc/test_pcapng_roundtrip2_unit.py @@ -140,13 +140,22 @@ def test_pcap_frame_clips_to_snaplen(self) -> None: self.assertEqual(frame.info.len, 16) def test_custom_option_shorter_than_pen_is_rejected(self) -> None: - """#1284: Option Length 3 cannot hold the 4-octet PEN.""" - from pcapkit.utilities.exceptions import ProtocolError + """#1284: Option Length 3 cannot hold the 4-octet PEN. + + Its PEN and padding run past the eight-octet option area, so the area + is kept as captured rather than read as an option (#1325), and the + block rebuilds byte for byte. + """ + import warnings octets = H('0a0d0d0a 24000000 4d3c2b1a 0100 0000 ffffffffffffffff' 'ad0b0300 61626300 24000000') - with self.assertRaisesRegex(ProtocolError, r'opt_custom\] invalid length'): - self._parse(octets) + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + block = self._parse(octets).info + self.assertEqual(len(block.options), 0) + self.assertEqual(block.options_raw, H('ad0b0300 61626300')) + self.assertRebuilds(octets) #: A big-endian section header with no options -- the repro of #1272. diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index fb84dc353e..23da0866d7 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -3488,10 +3488,13 @@ class PCAPNGOptionAreaBoundTests(unittest.TestCase): An option's length is a 16-bit wire field, so one four-octet option header can declare 65,535 octets of payload. Nothing bounded that against the area the option sits in, and every shortfall inside a 16-bit length is padded - unconditionally at the field layer -- deliberately, so that a capture cut - short by its snapshot length still parses. Repeating such an option across - blocks therefore amplified without limit. C.f. #594, and #593 for the - 32-bit band the field layer's own budget already covers. + unconditionally at the field layer. Repeating such an option across blocks + therefore amplified without limit. C.f. #594, and #593 for the 32-bit band + the field layer's own budget already covers. + + An option running past its area is refused, and the block keeps the whole + option area as the octets captured, as one :obj:`bytes` item that packs + back verbatim (#1325). It used to be clamped to what the area had left. """ @@ -3509,6 +3512,12 @@ def _unpack_epb(self, options: bytes, packet_data: bytes = bytes(4)): self.assertEqual(schema.length, len(raw) + 4) return schema + def assertKeptAsCaptured(self, schema, options: bytes) -> None: # pylint: disable=invalid-name + """Assert the block kept its option area ``options`` whole, and rebuilds.""" + self.assertEqual(schema.options, [options]) + self.assertEqual(self._payload_octets(schema), 0) + self.assertEqual(schema.pack(), block_body(epb_body(options))) + def _unpack_epb_declaring(self, options: bytes, declared: int, packet_data: bytes = bytes(4)): """Unpack an EPB whose Block Total Length lies about its own size.""" @@ -3530,20 +3539,18 @@ def _payload_octets(schema) -> int: total += len(payload) return total - def test_an_option_declaring_more_than_its_area_reads_only_the_area(self) -> None: + def test_an_option_declaring_more_than_its_area_keeps_the_area(self) -> None: """#594's vector, one block: 65,535 declared against a four-octet area. The area is exactly the option header, so there is nothing left for the - payload. Before the bound this read ``b''`` off an exhausted stream and + payload. Before any bound this read ``b''`` off an exhausted stream and zero-padded it to 65,535 octets -- from a 40-octet block. """ - schema = self._unpack_epb(over_declared(UNKNOWN_OPT, b'', 0xFFFF)) + options = over_declared(UNKNOWN_OPT, b'', 0xFFFF) + schema = self._unpack_epb(options) - self.assertEqual(len(schema.options), 1) - self.assertEqual(schema.options[0].length, 0xFFFF) - self.assertEqual(schema.options[0].data, b'') - self.assertEqual(self._payload_octets(schema), 0) + self.assertKeptAsCaptured(schema, options) def test_an_option_that_exactly_fills_its_area_is_untouched(self) -> None: """Four octets declared and four present, in an eight-octet area. @@ -3560,38 +3567,34 @@ def test_an_option_that_exactly_fills_its_area_is_untouched(self) -> None: self.assertEqual(schema.options[0].length, 4) self.assertEqual(schema.options[0].data, b'\xaa\xbb\xcc\xdd') - def test_an_option_over_declaring_by_one_octet_loses_only_that_octet(self) -> None: + def test_an_option_over_declaring_by_one_octet_keeps_the_area(self) -> None: """Five declared and four present, in an eight-octet area. - Before the bound the fifth octet was synthesised as a zero; the value - was five octets long for an option that held four. The bound reads the - four that are there. Paired with the exact-fit case above, this is what - separates "clamp to what the area has left" from either leaving the - shortfall padded or refusing the option outright. + Unbounded, the fifth octet was synthesised as a zero; clamped, the + option read four octets behind a length of five, and rebuilt three + octets of padding that were never there. Paired with the exact-fit + case above, this is the smallest over-declaration there is. """ - schema = self._unpack_epb(over_declared(UNKNOWN_OPT, b'\xaa\xbb\xcc\xdd', 5)) + options = over_declared(UNKNOWN_OPT, b'\xaa\xbb\xcc\xdd', 5) + schema = self._unpack_epb(options) - self.assertEqual(len(schema.options), 1) - self.assertEqual(schema.options[0].length, 5) - self.assertEqual(schema.options[0].data, b'\xaa\xbb\xcc\xdd') + self.assertKeptAsCaptured(schema, options) - def test_an_option_over_declaring_by_four_octets_is_clamped_to_the_remainder(self) -> None: + def test_an_option_over_declaring_by_four_octets_keeps_the_area(self) -> None: """Eight declared and four present, in an eight-octet area. The sharpest discriminator in this class. The option area is eight - octets and the option declares eight, so a bound taken against the - *area* leaves the declared length untouched and four zero octets are - still synthesised. The bound has to be taken against what the area has - left *at that field* -- four octets, the header having consumed the - other four -- which is what reads the four real octets and no more. + octets and the option declares eight, so a check taken against the + *area* lets it through and four zero octets are synthesised. The check + has to be taken against what the area has left *at that field* -- four + octets, the header having consumed the other four. """ - schema = self._unpack_epb(over_declared(UNKNOWN_OPT, b'\xaa\xbb\xcc\xdd', 8)) + options = over_declared(UNKNOWN_OPT, b'\xaa\xbb\xcc\xdd', 8) + schema = self._unpack_epb(options) - self.assertEqual(len(schema.options), 1) - self.assertEqual(schema.options[0].length, 8) - self.assertEqual(schema.options[0].data, b'\xaa\xbb\xcc\xdd') + self.assertKeptAsCaptured(schema, options) def test_a_whole_sixteen_bit_option_the_block_declares_room_for_is_untouched(self) -> None: """65,535 octets declared, 65,535 present, and the block says so. @@ -3609,25 +3612,22 @@ def test_a_whole_sixteen_bit_option_the_block_declares_room_for_is_untouched(sel self.assertEqual(schema.options[0].length, 0xFFFF) self.assertEqual(schema.options[0].data, value) - def test_a_tail_option_gets_only_what_the_options_before_it_left(self) -> None: + def test_a_tail_option_is_checked_against_what_the_options_before_it_left(self) -> None: """Two options in a twelve-octet area, the second declaring 65,535. The first option consumes eight of the twelve octets honestly, so the - second has four -- its own header -- and no payload. A bound taken + second has four -- its own header -- and no payload. A check taken against the area as declared, rather than against the area as the - earlier options left it, would hand the second option twelve octets and - synthesise eight of them. This is the input that separates the two. + earlier options left it, would let the second option through. The + whole area is kept, the good option included, so that it rebuilds as + captured. """ options = (tlv(UNKNOWN_OPT, b'\xaa\xaa\xaa\xaa') + over_declared(UNKNOWN_OPT + 1, b'', 0xFFFF)) schema = self._unpack_epb(options) - self.assertEqual(len(schema.options), 2) - self.assertEqual(schema.options[0].data, b'\xaa\xaa\xaa\xaa') - self.assertEqual(schema.options[1].length, 0xFFFF) - self.assertEqual(schema.options[1].data, b'') - self.assertEqual(self._payload_octets(schema), 4) + self.assertKeptAsCaptured(schema, options) def test_the_payload_never_exceeds_the_area_for_any_declared_length(self) -> None: """The bound itself, over the whole interesting space rather than one input. @@ -3681,7 +3681,7 @@ def test_the_amplification_does_not_grow_with_the_block_count(self) -> None: def test_the_same_option_area_answers_the_same_whatever_preceded_it(self) -> None: """No history dependence, which a running budget alone would not give. - The bound reads only the block's own declared framing, so byte-identical + The check reads only the block's own declared framing, so byte-identical input produces byte-identical output regardless of what was parsed before it. C.f. #593, whose own notes record a naive running threshold answering four different ways across 40 byte-identical calls. @@ -3689,26 +3689,24 @@ def test_the_same_option_area_answers_the_same_whatever_preceded_it(self) -> Non """ options = over_declared(UNKNOWN_OPT, b'', 0xFFFF) - first = self._unpack_epb(options) - expected = (first.options[0].length, first.options[0].data) - for index in range(200): schema = self._unpack_epb(options) - self.assertEqual((schema.options[0].length, schema.options[0].data), expected, - f'call {index + 2} answered differently from call 1') + self.assertEqual(schema.options, [options], + f'call {index + 1} answered differently') - def test_every_clamped_payload_is_bounded_and_none_was_missed(self) -> None: + def test_every_bounded_payload_is_refused_and_none_was_missed(self) -> None: """One subtest per option and record payload the bound is applied to. Each is handed an area holding its own header and fixed fields and nothing more, while declaring 65,535 octets of payload. Every one must - read an empty payload rather than synthesise the shortfall. The table is + refuse the payload rather than synthesise the shortfall. The table is the list of sites, so a payload added later without the bound shows up here as a subtest that was never written -- and one that lost the bound shows up as a subtest that fails. """ from pcapkit.protocols.schema.misc import pcapng as schema_module + from pcapkit.utilities.exceptions import ProtocolError # (schema class, octets of fixed field between the header and the # payload, payload attribute) @@ -3735,22 +3733,20 @@ def test_every_clamped_payload_is_bounded_and_none_was_missed(self) -> None: schema_cls = getattr(schema_module, name) raw = struct.pack(' None: - """A clamped option is reported; an option that fits is not.""" + def test_keeping_the_area_warns_rather_than_passing_in_silence(self) -> None: + """A kept option area is reported; an option that fits is not.""" from pcapkit.protocols.schema.misc.pcapng import UnknownOption - from pcapkit.utilities.warnings import SchemaWarning + from pcapkit.utilities.warnings import ProtocolWarning with mock.patch('pcapkit.protocols.schema.misc.pcapng.warn') as warn: - raw = struct.pack(' None: parsed = UnknownOption.unpack(packed, len(packed), {'byteorder': 'little'}) self.assertEqual(parsed.data, value) - def test_a_negative_remainder_is_left_alone_rather_than_clamped_to(self) -> None: - """A remainder already past zero is the field's problem, not the bound's. + def test_an_option_cut_inside_its_header_keeps_the_area(self) -> None: + """A ten-octet area: one whole option, then two octets of the next. - ``__length__`` can be negative by the time a payload sizes itself: - :meth:`Schema.unpack` warns and carries on when an earlier field has - over-consumed. A ten-octet area does it -- the first option takes eight, - leaving two, which is not a multiple of four, so the loop runs again with - two octets of area; the next option's type field takes both and its - length field short-reads to zero, putting ``__length__`` at -2 before the - payload's own callback is consulted. - - Clamping to that would hand ``-2`` to the field, whose struct template is - then ``'-2s'`` and whose unpack raises ``struct.error: bad char in struct - format`` -- not one of :mod:`pcapkit.utilities.exceptions`, and a new - failure on input that parses today. Measured: this input parses with the - guard and raises without it, while the eight-octet control parses either - way, so it isolates the guard rather than the clamp. + The first option takes eight octets, leaving two, so the loop runs again + with two octets of area. The next option's type field takes both and its + length field reads short, so it runs past the data and the area is kept + as captured. It used to parse as a second option of length zero, which + rebuilt two octets of length that were never there. """ options = (tlv(UNKNOWN_OPT, b'\xaa\xbb') + struct.pack(' None: - """The area is clamped too, so the payload cannot outrun the real block. + def test_a_block_declaring_more_room_than_it_holds_keeps_what_it_holds(self) -> None: + """The area is bounded by the real block, so the payload cannot outrun it. Block Total Length is checked only against its own trailing copy, never against the file, so a block may declare 1,000,000 octets while holding - 36. Its option area is then sized at 999,964, an option declaring 65,535 - is comfortably under that, and the payload bound alone does not fire: - before the area was clamped this synthesised 65,535 octets of zeros from - a 36-octet buffer -- 1,820x, with no warning at all. Found by the - cross-review of #676, which is why the area carries its own bound. + 36. Its option area is then sized at 999,964, and an option declaring + 65,535 is comfortably under that: unbounded, this synthesised 65,535 + octets of zeros from a 36-octet buffer -- 1,820x, with no warning at + all. Found by the cross-review of #676, which is why the area is bounded + by the octets the block holds. - The block still parses, and reports the length it declared; what it - cannot do is manufacture a payload the buffer never held. + The block still parses, and reports the length it declared; its option + area is the four octets it holds, kept as captured, and the trailing + length is read from where it really is. """ options = over_declared(UNKNOWN_OPT, b'', 0xFFFF) schema = self._unpack_epb_declaring(options, 1_000_000) self.assertEqual(schema.length, 1_000_000) - self.assertEqual(schema.options[0].length, 0xFFFF) + self.assertEqual(schema.length2, 1_000_000) + self.assertEqual(schema.options, [options]) self.assertEqual(self._payload_octets(schema), 0) def test_the_area_bound_does_not_touch_a_well_formed_block(self) -> None: diff --git a/tests/protocols/test_declared_length_overrun_unit.py b/tests/protocols/test_declared_length_overrun_unit.py new file mode 100644 index 0000000000..37ea41a87b --- /dev/null +++ b/tests/protocols/test_declared_length_overrun_unit.py @@ -0,0 +1,174 @@ +# -*- coding: utf-8 -*- +"""An option declaring more octets than are present is rejected, not zero-filled. + +GitHub issue #1325: :meth:`OptionField.unpack +` let an option schema +read past the end of its data, and every field reading off the end padded the +missing octets with zeros. A HIP ``ACK`` declaring 256 octets with 4 present +parsed as 64 update IDs and rebuilt to 304 octets; an MH ``PadN`` declaring 3 +octets with none present rebuilt 24 octets from 16. + +Such an option now raises :exc:`~pcapkit.utilities.exceptions.ProtocolError`. +Inside a parent layer the octets are kept as captured -- raw extension data +under IPv6, a :class:`~pcapkit.protocols.misc.raw.Raw` payload elsewhere -- and +the parent rebuilds byte for byte. + +An option that begins at the end of the data reads nothing at all, and is still +left to the end-of-option-list handling an over-long option area relies on. + +Every case builds its own octets in memory and reads no capture. + +""" + +import struct +import unittest +import warnings + +from tests._support import reimport_once_per_class + + +def _hip(params: bytes) -> bytes: + """A HIP ``I1`` header (40 octets) carrying ``params``.""" + return (bytes([59, (40 + len(params)) // 8 - 1, 1, 0x11, 0, 0, 0, 0]) + + b'\x11' * 16 + b'\x22' * 16 + params) + + +def _ext6(opts: bytes) -> bytes: + """An IPv6 options extension header carrying ``opts``.""" + return bytes([59, (2 + len(opts)) // 8 - 1]) + opts + + +def _sctp(chunks: bytes) -> bytes: + """An SCTP common header carrying ``chunks``.""" + return struct.pack('>HHII', 1, 2, 0, 0) + chunks + + +def _tcp(opts: bytes) -> bytes: + """A TCP header carrying ``opts``.""" + return struct.pack('>HHIIBBHHH', 1, 2, 0, 0, (5 + len(opts) // 4) << 4, 0x10, 0, 0, 0) + opts + + +def _ipv4(opts: bytes, ihl: int, proto: int = 253, payload: bytes = b'') -> bytes: + """An IPv4 header declaring ``ihl`` and carrying ``opts`` and ``payload``.""" + return (bytes([0x40 | ihl, 0]) + struct.pack('>H', 20 + len(opts) + len(payload)) + + bytes.fromhex('00010000 40') + bytes([proto]) + bytes(2) + + bytes.fromhex('0a000001 0a000002') + opts + payload) + + +def _ipv6(proto: int, payload: bytes) -> bytes: + """An IPv6 header carrying ``payload`` as next header ``proto``.""" + return (bytes.fromhex('60000000') + struct.pack('>H', len(payload)) + bytes([proto, 64]) + + b'\x11' * 16 + b'\x22' * 16 + payload) + + +#: Option areas whose last option declares more octets than the area holds. +OVERRUNS = { + # issue #1325: ACK declaring 0x0100 octets, 4 present + 'HIP ACK': ('pcapkit.protocols.internet.hip', 'HIP', _hip(bytes.fromhex('01c10100 00000000'))), + # issue #1325: PadN declaring 3 octets, none present + 'MH PadN': ('pcapkit.protocols.internet.mh', 'MH', + bytes.fromhex('3b010000 00000000 010400000000 0103')), + 'HOPOPT PadN': ('pcapkit.protocols.internet.hopopt', 'HOPOPT', _ext6(bytes.fromhex('010700000000'))), + 'IPv6-Opts PadN': ('pcapkit.protocols.internet.ipv6_opts', 'IPv6_Opts', _ext6(bytes.fromhex('010700000000'))), + 'SCTP INIT parameter': ('pcapkit.protocols.transport.sctp', 'SCTP', _sctp( + bytes.fromhex('01000020 00000001 00010000 00010001 00000001 000c0010 00050000'))), + # an unassigned kind declaring 12 octets, 8 present + 'TCP unassigned': ('pcapkit.protocols.transport.tcp', 'TCP', _tcp(bytes.fromhex('4f0caabb ccddeeff'))), + # an unassigned option declaring 12 octets, 8 present in a 16-octet area + 'IPv4 unassigned': ('pcapkit.protocols.internet.ipv4', 'IPv4', + _ipv4(bytes.fromhex('1f0caabb ccddeeff'), ihl=9)), +} + +#: The same option areas with every option complete. +CONTROLS = { + 'HIP ACK': ('pcapkit.protocols.internet.hip', 'HIP', _hip(bytes.fromhex('01c10004 00000001'))), + 'MH PadN': ('pcapkit.protocols.internet.mh', 'MH', + bytes.fromhex('3b010000 00000000 0106000000000000')), + 'HOPOPT PadN': ('pcapkit.protocols.internet.hopopt', 'HOPOPT', _ext6(bytes.fromhex('010400000000'))), + 'IPv6-Opts PadN': ('pcapkit.protocols.internet.ipv6_opts', 'IPv6_Opts', _ext6(bytes.fromhex('010400000000'))), + 'SCTP INIT parameter': ('pcapkit.protocols.transport.sctp', 'SCTP', _sctp( + bytes.fromhex('0100001c 00000001 00010000 00010001 00000001 000c0008 00050006'))), + 'TCP unassigned': ('pcapkit.protocols.transport.tcp', 'TCP', _tcp(bytes.fromhex('4f08aabb ccddeeff'))), + 'IPv4 unassigned': ('pcapkit.protocols.internet.ipv4', 'IPv4', + _ipv4(bytes.fromhex('1f08aabb ccddeeff'), ihl=7)), +} + +#: Each overrun inside a parent layer: (parent module, parent class, frame). +IN_PARENT = { + 'HIP ACK': ('pcapkit.protocols.internet.ipv6', 'IPv6', _ipv6(139, OVERRUNS['HIP ACK'][2])), + 'MH PadN': ('pcapkit.protocols.internet.ipv6', 'IPv6', _ipv6(135, OVERRUNS['MH PadN'][2])), + 'HOPOPT PadN': ('pcapkit.protocols.internet.ipv6', 'IPv6', _ipv6(0, OVERRUNS['HOPOPT PadN'][2])), + 'IPv6-Opts PadN': ('pcapkit.protocols.internet.ipv6', 'IPv6', _ipv6(60, OVERRUNS['IPv6-Opts PadN'][2])), + 'SCTP INIT parameter': ('pcapkit.protocols.internet.ipv4', 'IPv4', + _ipv4(b'', ihl=5, proto=132, payload=OVERRUNS['SCTP INIT parameter'][2])), + 'TCP unassigned': ('pcapkit.protocols.internet.ipv4', 'IPv4', + _ipv4(b'', ihl=5, proto=6, payload=OVERRUNS['TCP unassigned'][2])), + 'IPv4 unassigned': ('pcapkit.protocols.link.ethernet', 'Ethernet', + bytes.fromhex('0123456789ab fedcba987654 0800') + OVERRUNS['IPv4 unassigned'][2]), +} + + +class TestDeclaredLengthOverrun(unittest.TestCase): + """Pin the rejection of an option running past the end of its data.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + @staticmethod + def _protocol(module: str, name: str) -> type: + import importlib + + return getattr(importlib.import_module(module), name) + + def test_an_overrunning_option_raises_protocol_error(self) -> None: + from pcapkit.utilities.exceptions import ProtocolError + + for case, (module, name, data) in OVERRUNS.items(): + with self.subTest(case=case): + protocol = self._protocol(module, name) + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + with self.assertRaisesRegex(ProtocolError, 'runs past the end of the data'): + protocol(data, len(data)) + + def test_a_complete_option_still_rebuilds_byte_for_byte(self) -> None: + for case, (module, name, data) in CONTROLS.items(): + with self.subTest(case=case): + protocol = self._protocol(module, name) + parsed = protocol(data, len(data)) + self.assertEqual(protocol.from_data(parsed.info).data, data) + + def test_an_overrunning_layer_is_kept_as_captured_by_its_parent(self) -> None: + from pcapkit.protocols.misc.raw import Raw + + for case, (module, name, frame) in IN_PARENT.items(): + with self.subTest(case=case): + protocol = self._protocol(module, name) + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + parsed = protocol(frame, len(frame)) + + if name == 'IPv6': + # an extension header is kept as raw extension data + self.assertIn('runs past the end of the data', str(parsed.info.ext.error)) + else: + # any other layer is kept as a raw payload, as captured + self.assertIsInstance(parsed.payload, Raw) + self.assertIn('runs past the end of the data', str(parsed.payload.info.error)) + self.assertEqual(parsed.payload.data, OVERRUNS[case][2]) + self.assertEqual(protocol.from_data(parsed.info).data, frame) + + def test_an_option_area_declared_past_the_data_still_ends_at_eool(self) -> None: + from pcapkit.protocols.internet.ipv4 import IPv4 + + # ``ihl`` declares 20 octets of options with none present: the first + # option begins at the end of the data, so it is end-of-option-list. + data = bytes.fromhex('4a00001800010000400600000a0000010a000002') + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + parsed = IPv4(data, len(data)) + self.assertEqual(parsed.info.src.compressed, '10.0.0.1') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/test_make_raw_item_read_unit.py b/tests/protocols/test_make_raw_item_read_unit.py index 2c1d0e230a..a7bbd0b0ab 100644 --- a/tests/protocols/test_make_raw_item_read_unit.py +++ b/tests/protocols/test_make_raw_item_read_unit.py @@ -7,7 +7,9 @@ past the option area, or one cut short of its own fields -- escaped as the reader's bare :exc:`~pcapkit.utilities.exceptions.ProtocolError`. The read is now covered too: the error says ``malformed raw item given to make()``, -carries the reader's message, and chains it as ``__cause__``. +carries the reader's message, and chains it as ``__cause__``. An item whose +fields run past the octets given is refused by the schema instead (#1325), and +is wrapped the same way. Every case builds its own packet in memory and reads no capture. Classes are imported inside each test, after @@ -35,11 +37,6 @@ def _attr(path: 'str') -> 'Any': #: One raw item per protocol that the schema parses and ``read`` rejects: #: (protocol class, construction keywords, the reader's own message). READ_REJECTED = { - # the lone ``0x02`` takes the next item's kind octet as its length, 148, - # which runs past the 8-octet option area - 'IPv4 overrun': ('pcapkit.protocols.internet.ipv4.IPv4', - {'options': [b'\x02', b'\x94\x04\x00\x00'], 'protocol': 253}, - r'IPv4: invalid format'), 'IPv4 short': ('pcapkit.protocols.internet.ipv4.IPv4', {'options': [b'\x0c'], 'protocol': 253}, r'IPv4: \[OptNo 12\] invalid format'), @@ -49,19 +46,30 @@ def _attr(path: 'str') -> 'Any': 'HOPOPT': ('pcapkit.protocols.internet.hopopt.HOPOPT', {'extension': True, 'options': [b'\x23']}, r'HOPOPT: \[OptNo 35\] invalid format'), - 'IPv6_Opts': ('pcapkit.protocols.internet.ipv6_opts.IPv6_Opts', - {'extension': True, 'options': [b'\x26']}, - r'IPv6-Opts: \[OptNo 38\] invalid format'), 'MH': ('pcapkit.protocols.internet.mh.MH', {'data': {'options': [b'\x2b']}}, r'MH: \[Opt 43\] invalid format'), +} + +#: Raw items whose fields run past the octets given. The schema refuses those +#: itself (#1325) before ``read`` sees them, and the error is wrapped the same +#: way: (protocol class, construction keywords, the schema's own message). +SCHEMA_REJECTED = { + # the lone ``0x02`` takes the next item's kind octet as its length, 148, + # which runs past the 8-octet option area + 'IPv4 overrun': ('pcapkit.protocols.internet.ipv4.IPv4', + {'options': [b'\x02', b'\x94\x04\x00\x00'], 'protocol': 253}, + r'runs past the end of the data'), + 'IPv6_Opts': ('pcapkit.protocols.internet.ipv6_opts.IPv6_Opts', + {'extension': True, 'options': [b'\x26']}, + r'runs past the end of the data'), 'SCTP': ('pcapkit.protocols.transport.sctp.SCTP', {'chunks': [b'\x01']}, - r'SCTP: \[Chunk 1\] invalid format'), + r'runs past the end of the data'), # ESP_INFO needs a 12-octet body, and this one has 8 'HIP': ('pcapkit.protocols.internet.hip.HIP', {'extension': True, 'parameters': [b'\x00\x41\x00\x08' + bytes(8)]}, - r'HIPv6: \[ParamNo 65\] invalid format'), + r'runs past the end of the data'), } @@ -73,7 +81,7 @@ def setUp(self) -> None: def test_read_rejection_is_wrapped(self) -> None: exc = _attr('pcapkit.utilities.exceptions.ProtocolError') - for case, (klass, kwargs, message) in READ_REJECTED.items(): + for case, (klass, kwargs, message) in {**READ_REJECTED, **SCHEMA_REJECTED}.items(): with self.subTest(case=case): cls = _attr(klass) with warnings.catch_warnings(): diff --git a/tests/protocols/test_protocol_make_unit.py b/tests/protocols/test_protocol_make_unit.py index d157318620..7c6159d554 100644 --- a/tests/protocols/test_protocol_make_unit.py +++ b/tests/protocols/test_protocol_make_unit.py @@ -156,10 +156,12 @@ def items() -> 'list[bytes]': def test_malformed_raw_item_raises_protocol_error(self) -> None: # ESP_INFO needs a 12-octet body; an 8-octet one is a parse error, - # reported as such rather than as an AttributeError on raw bytes + # reported as such rather than as an AttributeError on raw bytes. Its + # fields read past the octets given, which the option field refuses + # (#1325) before the reader sees them. hip = _attr('pcapkit.protocols.internet.hip.HIP') exc = _attr('pcapkit.utilities.exceptions.ProtocolError') - with self.assertRaisesRegex(exc, r'\[ParamNo 65\] invalid format'): + with self.assertRaisesRegex(exc, r'runs past the end of the data'): hip(extension=True, parameters=[b'\x00\x41\x00\x08' + bytes(8)]) diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 4337e07c30..b75f43e24a 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1382,54 +1382,35 @@ def test_an_option_area_longer_than_the_segment_still_parses(self) -> None: self.assertEqual(mss.mss, 1460) def test_a_truncated_option_still_parses_its_declared_length(self) -> None: - """An option declaring more data than its option area holds is tolerated. C.f. #431, #572. + """An option declaring more data than its option area holds is refused alone, and kept in a datagram. C.f. #431, #572, #1325. :meth:`test_an_option_area_longer_than_the_segment_still_parses` above pins the *empty*-tail half of the #431 accommodation: an option area that runs out before it starts, so the type byte decodes as 0 and the - loop reads end-of-option-list. Nothing pinned the other half -- an - option that *does* start, declares more data than its own option area - actually holds, and runs out partway through its ``data`` field, - rather than one with no options at all. A candidate fix for #554 (PR - #571) turned that into an unwrapped ``FieldValueError`` while the rest - of the suite stayed green, because nothing exercised it. + loop reads end-of-option-list. This is the other half -- an option that + *does* start, declares more data than its option area holds, and runs + out partway through its ``data`` field. The segment below sets a data offset of 7 -- 8 octets of option area - -- for a reserved option kind (``0x4f``, which resolves to - ``Option.Reserved_79``) declaring ``length=12``, which asks - :class:`~pcapkit.protocols.schema.transport.tcp.UnassignedOption`'s - ``data`` field (``BytesField(length=lambda pkt: pkt['length'] - 2)``, - 10 octets here) for more than the 6 octets actually behind it. - :meth:`FieldBase.unpack ` - tail-pads the short read with zero octets rather than raising, so the - option parses with its declared ``length`` intact and a ``data`` value - of the six real octets followed by four zero ones -- the zeros go where - the octets that were never read would have been, which before #604 was - the other way round. That is reachable - here because :meth:`~pcapkit.protocols.transport.tcp.TCP._read_tcp_options` - pads the short read with zero octets rather than raising, so the - option parses with its declared ``length`` intact and a ``data`` value - of the six real octets followed by four zero ones. The zeros go on the - *tail*, where the octets that were never read would have been; before - #604 they were placed at the front, which for a numeric field corrupted - the value outright. That is reachable here because - :meth:`~pcapkit.protocols.transport.tcp.TCP._read_tcp_options` - sizes each parsed option by ``len(schema)`` -- what it actually - consumed (8 octets) -- rather than by its self-reported ``length``, so - its own ``TCP: invalid format`` threshold never sees the shortfall -- - unlike IPv4's equivalent check, which sums the *declared* lengths - instead and does see it (see - ``IPv4UnitTests.test_a_truncated_option_still_parses_its_declared_length``). - ``length=32`` (30 octets of data wanted, still only 6 available) is - checked alongside 12 because the pad width tracks ``length - 2``: 32 - yields 24 trailing zero octets where 12 yields 4, pinning that the - padding scales with the declared length rather than being a fixed 4. + -- for a reserved option kind (``0x4f``, ``Option.Reserved_79``) + declaring ``length=12`` or ``length=32``, with only 6 data octets + behind it. The missing octets used to be read as zeros, 4 or 24 of + them, and rebuilt as if captured. :meth:`OptionField.unpack + ` now refuses + the option with :exc:`~pcapkit.utilities.exceptions.ProtocolError` + instead (#1325). Inside an IPv4 datagram the segment is kept as the + octets captured, a :class:`~pcapkit.protocols.misc.raw.Raw` payload + carrying that error, and rebuilds byte for byte. """ import struct + import warnings from pcapkit.const.tcp.option import Option + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.misc.raw import Raw from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import ProtocolError from tests._support import time_limit def segment(data_offset: 'int', options: 'bytes') -> 'bytes': @@ -1438,29 +1419,22 @@ def segment(data_offset: 'int', options: 'bytes') -> 'bytes': custom = Option.get(0x4f) trailing = bytes.fromhex('aabbccddeeff') - for declared_length, zeroes in ((12, 4), (32, 24)): + for declared_length in (12, 32): with self.subTest(declared_length=declared_length): raw = segment(7, bytes([custom, declared_length]) + trailing) - with time_limit(): - proto = TCP(raw, len(raw)) - - self.assertEqual(proto.info.hdr_len, 28) - self.assertEqual( - [(code, opt.length) for code, opt in proto.info.options.items(multi=True)], - [(custom, declared_length)], - ) - unassigned = next(opt for code, opt in proto.info.options.items(multi=True) - if code == custom) - # The six octets actually behind the option come first, where - # they were read, and the zeros synthesised for the ones that - # were not follow them. Before #604 they arrived the other way - # round; since ``trailing`` is non-zero and ``zeroes`` is - # non-zero for both declared lengths, this assertion tells the - # two orders apart rather than holding for either. See #604 and - # ``FieldBaseShortReadPaddingSideTests`` in - # ``tests/corekit/test_fields_field.py``. - self.assertEqual(unassigned.data, trailing + b'\x00' * zeroes) - self.assertEqual(bytes(proto.__header__), raw) + datagram = (bytes.fromhex('4500') + struct.pack('!H', 20 + len(raw)) + + bytes.fromhex('000100004006' '0000' '0a000001' '0a000002') + raw) + with time_limit(), warnings.catch_warnings(): + warnings.simplefilter('ignore') + with self.assertRaisesRegex(ProtocolError, 'runs past the end of the data'): + TCP(raw, len(raw)) + proto = IPv4(datagram, len(datagram)) + + self.assertIsInstance(proto.payload, Raw) + self.assertIn('runs past the end of the data', str(proto.payload.info.error)) + # the option octets are kept as captured, with nothing synthesised + self.assertEqual(proto.payload.data, raw) + self.assertEqual(IPv4.from_data(proto.info).data, datagram) def test_unregistered_option_kind_does_not_mutate_the_class_registry(self) -> None: """Parsing must not write to the shared ``TCP.__option__``.