fix: reject a bool IP address in-library instead of silently corrupting the packet

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
+        <pcapkit.protocols.internet.mh.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:

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.

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

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)