From 2a0e7b163a2d49e446264b6b8306b2d3d68a2a17 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 22:37:45 -0400 Subject: [PATCH] ipaddress/esp: reject a bool address in-library, not as silent corruption Closes #491, the unfixed IPv4 sibling of #469/#481. - `_IPAddressField.pre_process`, `IPv4InterfaceField.pre_process`, and `IPv6InterfaceField.pre_process` (pcapkit/corekit/fields/ipaddress.py) now reject a `bool` value before dispatching into `ipaddress.ip_address`/ `ip_interface`, via a new shared `_reject_bool` helper. `bool` is an `int` subclass, and `ipaddress.ip_address()` treats any `int` below `2**32` as IPv4 -- so `True`/`False` used to be silently packed as `0.0.0.1`/ `0.0.0.0` on an IPv4-typed field, with no exception and no warning. On an IPv6-typed field the same conversion happened to raise instead (version mismatch), which is the asymmetry that let this slip past #481's guard for MH's `_make_opt_mn_id` -- that PR fixed the mechanism at one call site, not at its shared root. - `SecurityAssociation.__init__` (pcapkit/protocols/internet/esp.py) calls `ipaddress.ip_address()` directly on `destination`, bypassing the Field abstraction entirely; it gets the same guard, raising `ProtocolError`. - Both guards point the caller at `int(...)`, matching #481's message shape. tests/corekit/test_fields_ipaddress.py sweeps `{True, False}` across all four field classes (IPv4/IPv6 address and interface) and reproduces the original corruption through the public `IPv4.make(src=True, dst=False)` API. tests/protocols/internet/test_esp_unit.py adds the equivalent case for `SecurityAssociation`. Build: tests/corekit/test_fields_ipaddress.py -> 14 passed, 25 subtests. tests/protocols/internet/test_esp_unit.py -> 30 passed, 38 subtests. Full tests/corekit + tests/protocols/internet -> 254 passed, 662 subtests, 0 failed. --- pcapkit/corekit/fields/ipaddress.py | 58 ++++++++++++++++++--- pcapkit/protocols/internet/esp.py | 25 ++++++++- tests/corekit/test_fields_ipaddress.py | 62 +++++++++++++++++++++++ tests/protocols/internet/test_esp_unit.py | 23 +++++++++ 4 files changed, 160 insertions(+), 8 deletions(-) diff --git a/pcapkit/corekit/fields/ipaddress.py b/pcapkit/corekit/fields/ipaddress.py index f4907de25d..5cd5a8e988 100644 --- a/pcapkit/corekit/fields/ipaddress.py +++ b/pcapkit/corekit/fields/ipaddress.py @@ -68,6 +68,43 @@ def _reraise_as_field_value_error(description: str) -> 'Iterator[None]': raise FieldValueError(f'{description}: {error}') from error +def _reject_bool(value: 'object', description: str) -> 'None': + """Reject a :obj:`bool` value before it reaches :mod:`ipaddress`. + + Args: + value: Value to check. + description: Human-readable description of what ``value`` is, used + to build the :exc:`FieldValueError` message. + + Raises: + FieldValueError: If ``value`` is a :obj:`bool`. + + Notes: + :obj:`bool` is an :class:`int` subclass, and every conversion in this + module ultimately calls :func:`ipaddress.ip_address` or + :func:`ipaddress.ip_interface`, both of which treat any :class:`int` + below ``2**32`` as IPv4 -- so without this guard, ``True``/``False`` + are silently accepted as ``0.0.0.1``/``0.0.0.0`` (or the equivalent + interface) on an IPv4-typed field, with **no exception and no + warning**. On an IPv6-typed field the same conversion happens to + raise instead, because the resulting + :class:`~ipaddress.IPv4Address`'s version mismatches -- and that + asymmetry is exactly what let this slip past #481's otherwise + equivalent guard for :meth:`MH._make_opt_mn_id + ` (c.f. #491). + + Every caller checks this *before* dispatching on the value's type, + for the same placement reason #481 gives: a correct check in the + wrong position does not fire, and that placement mistake has + already been made twice in this repository's history. + + """ + if isinstance(value, bool): + raise FieldValueError( + f'{description}: must not be a bool, not {value!r} -- pass ' + f'int({value!r}) if the numeric value is what is wanted') + + class _IPField(Field[_T], Generic[_T]): """Internal IP related value for protocol fields. @@ -105,10 +142,13 @@ def pre_process(self, value: '_AT | bytes | int | str', packet: 'dict[str, Any]' Processed field value. Raises: - FieldValueError: If ``value`` is not a valid IP address, or if it - is the wrong IP version for this field. + FieldValueError: If ``value`` is a :obj:`bool` (c.f. + :func:`_reject_bool`), is not a valid IP address, or is the + wrong IP version for this field. """ + _reject_bool(value, 'invalid IP address') + if isinstance(value, (ipaddress.IPv4Address, ipaddress.IPv6Address)): ip = value # type: IPv4Address | IPv6Address else: @@ -234,10 +274,13 @@ def pre_process(self, value: 'IPv4Interface | bytes | int | str', packet: 'dict[ Processed field value. Raises: - FieldValueError: If ``value`` is not a valid IP interface, or if - it is the wrong IP version for this field. + FieldValueError: If ``value`` is a :obj:`bool` (c.f. + :func:`_reject_bool`), is not a valid IP interface, or is the + wrong IP version for this field. """ + _reject_bool(value, 'invalid IP interface') + if isinstance(value, ipaddress.IPv4Interface): val = value else: @@ -319,10 +362,13 @@ def pre_process(self, value: 'IPv6Interface | bytes | int | str', packet: 'dict[ Processed field value. Raises: - FieldValueError: If ``value`` is not a valid IP interface, or if - it is the wrong IP version for this field. + FieldValueError: If ``value`` is a :obj:`bool` (c.f. + :func:`_reject_bool`), is not a valid IP interface, or is the + wrong IP version for this field. """ + _reject_bool(value, 'invalid IP interface') + if isinstance(value, ipaddress.IPv6Interface): val = value else: diff --git a/pcapkit/protocols/internet/esp.py b/pcapkit/protocols/internet/esp.py index 15384e68e9..505da86f88 100644 --- a/pcapkit/protocols/internet/esp.py +++ b/pcapkit/protocols/internet/esp.py @@ -505,7 +505,8 @@ class SecurityAssociation: an SA by ``(SPI, destination, protocol)``, and supplying the address is what lets several tunnels sharing an SPI be told apart. Matched only when the outer destination is known to - :mod:`pcapkit`; see :meth:`ESP.read`. + :mod:`pcapkit`; see :meth:`ESP.read`. Must not be a :obj:`bool` + -- see ``Raises`` below. strict: Whether a padding pattern that does not follow the monotonically increasing sequence of :rfc:`4303` ยง2.4 should be treated as a decryption failure. Only applied when nothing else @@ -514,7 +515,13 @@ class SecurityAssociation: with zeros; set to :data:`False` for those. Raises: - ProtocolError: If the algorithms or key lengths are inconsistent. + ProtocolError: If the algorithms or key lengths are inconsistent, or + ``destination`` is a :obj:`bool` (c.f. #491) -- :obj:`bool` is an + :class:`int` subclass, and :func:`ipaddress.ip_address` treats + any :class:`int` below ``2**32`` as IPv4, so without this check + ``destination=True`` would silently become + ``IPv4Address('0.0.0.1')`` with no exception and no warning; pass + ``int(destination)`` if the numeric value is what is wanted. Important: Key material is held in *private* attributes of this object, and is @@ -538,6 +545,20 @@ def __init__(self, spi: 'Optional[int]' = None, *, if spi is not None and not 0 <= spi <= 0xFFFFFFFF: raise ProtocolError(f'invalid SPI: {spi}') + if isinstance(destination, bool): + # NOTE: checked before the ``ipaddress.ip_address`` call below, for + # the same reason as ``_reject_bool`` in + # ``pcapkit.corekit.fields.ipaddress`` (this module calls + # ``ipaddress.ip_address`` directly rather than through a Field, so + # it needs its own copy of the guard): ``bool`` is an ``int`` + # subclass, and ``ipaddress.ip_address()`` treats any ``int`` below + # ``2**32`` as IPv4 -- so without this check, ``destination=True`` + # would silently become ``IPv4Address('0.0.0.1')``, with no + # exception and no warning (c.f. #491). + raise ProtocolError( + f'invalid destination: must not be a bool, not {destination!r} -- ' + f'pass int({destination!r}) if the numeric value is what is wanted') + #: Optional[int]: Security Parameters Index, or :data:`None` for any. self.spi = spi #: CipherSuite: How to apply the encryption algorithm. diff --git a/tests/corekit/test_fields_ipaddress.py b/tests/corekit/test_fields_ipaddress.py index 7ff6a901b4..5847aade8d 100644 --- a/tests/corekit/test_fields_ipaddress.py +++ b/tests/corekit/test_fields_ipaddress.py @@ -203,6 +203,68 @@ def test_wrong_version_message_is_not_relabelled_as_a_malformed_value(self) -> N self.assertIn('IP version mismatch', str(interface_context.exception)) self.assertNotIn('invalid IP', str(interface_context.exception)) + def test_address_and_interface_fields_reject_a_bool_value_for_every_version(self) -> None: + """A :obj:`bool` value must not be silently accepted as an IP address. + + :obj:`bool` is an :class:`int` subclass, and :func:`ipaddress.ip_address`/ + :func:`ipaddress.ip_interface` both treat any :class:`int` below + ``2**32`` as IPv4 -- so before this fix, ``True``/``False`` were + silently converted to ``0.0.0.1``/``0.0.0.0`` (or the equivalent + interface) on an IPv4-typed field, with no exception and no warning. + On an IPv6-typed field the same conversion happened to raise instead, + because the resulting :class:`~ipaddress.IPv4Address`'s version + mismatched -- and that asymmetry is exactly what let this survive + #481, which fixed the identical mechanism at only one call site + (``MH._make_opt_mn_id``). See #491. + + Sweeps ``{True, False}`` across both address and interface field + types, for both IPv4 and IPv6, since the asymmetry above is exactly + what a partial sweep would miss. + """ + from pcapkit.corekit.fields.ipaddress import ( + IPv4AddressField, IPv4InterfaceField, IPv6AddressField, IPv6InterfaceField, + ) + from pcapkit.utilities.exceptions import BaseError, FieldValueError + + fields = [ + IPv4AddressField(), + IPv6AddressField(), + IPv4InterfaceField(), + IPv6InterfaceField(), + ] + + for field in fields: + for value in (True, False): + with self.subTest(field=type(field).__name__, value=value): + with self.assertRaises(FieldValueError) as context: + field.pre_process(value, {}) + # ``FieldValueError`` subclasses ``BaseError``, but assert + # the in-library type directly (above) rather than only this. + self.assertIsInstance(context.exception, BaseError) + self.assertIn('must not be a bool', str(context.exception)) + self.assertIn(repr(value), str(context.exception)) + # the escape hatch the message points at must actually work + self.assertIn(f'int({value!r})', str(context.exception)) + + # the escape hatch: int(True)/int(False) still convert correctly, + # proving this tightens bool specifically rather than int generally + self.assertEqual(fields[0].pre_process(int(True), {}), b'\x00\x00\x00\x01') + self.assertEqual(fields[0].pre_process(int(False), {}), b'\x00\x00\x00\x00') + + def test_ipv4_make_rejects_a_bool_address_through_the_public_api(self) -> None: + """The corruption reported in #491, reproduced through the public API. + + Before this fix, ``IPv4.make(src=True, dst=False)`` packed and + decoded without any exception or warning, silently corrupting the + addresses to ``0.0.0.1``/``0.0.0.0``. It must now raise instead. + """ + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.utilities.exceptions import BaseError + + proto = object.__new__(IPv4) + with self.assertRaises(BaseError): + proto.make(src=True, dst=False).pack() + def test_ipv4_interface_post_process_rejects_a_non_contiguous_netmask(self) -> None: """``IPv4InterfaceField.post_process`` builds ``ipaddress.ip_interface(f'{ip}/{mask}')`` from wire bytes whose diff --git a/tests/protocols/internet/test_esp_unit.py b/tests/protocols/internet/test_esp_unit.py index a308917282..4f4ad5970c 100644 --- a/tests/protocols/internet/test_esp_unit.py +++ b/tests/protocols/internet/test_esp_unit.py @@ -298,6 +298,29 @@ def test_security_association_validation(self) -> None: self.assertFalse(null.authenticated) self.assertIsNone(null.unavailable()) + def test_security_association_rejects_a_bool_destination(self) -> None: + """``destination`` calls :func:`ipaddress.ip_address` directly, with + no version check at all -- unlike the ``Field`` classes in + :mod:`pcapkit.corekit.fields.ipaddress`, ESP does not route through + that abstraction. Before this fix, + ``SecurityAssociation(spi=1, destination=True).destination`` silently + became ``IPv4Address('0.0.0.1')``, with no exception and no warning. + See #491. + """ + from pcapkit.protocols.internet.esp import SecurityAssociation + from pcapkit.utilities.exceptions import ProtocolError + + for value in (True, False): + with self.subTest(destination=value): + with self.assertRaises(ProtocolError) as ctx: + SecurityAssociation(spi=1, destination=value) # type: ignore[arg-type] + self.assertIn('must not be a bool', str(ctx.exception)) + + # the escape hatch still works, and None still means "any" + sa = SecurityAssociation(spi=1, destination=int(True)) # type: ignore[arg-type] + self.assertEqual(str(sa.destination), '0.0.0.1') + self.assertIsNone(SecurityAssociation(spi=1).destination) + def test_security_association_repr_holds_no_key_material(self) -> None: from pcapkit.protocols.internet.esp import (Cipher, ESPContext, Integrity, SecurityAssociation)