Skip to content

Address-typed SwitchField attributes bypass #491's bool guard: 3 mh.py makers still corrupt #508

Description

@JarryShaw

Follow-up to #491 (fixed in PR #500). The scope here is much narrower than I originally expected, and the correction is the useful part of this issue — I had recorded roughly ten suspect maker sites, and re-measuring on main at d2d9bb80c shows the merged root guard covers almost all of them.

What the merged guard already covers — measured, not assumed

#500 put the bool rejection in _IPAddressField.pre_process and _IPInterfaceField.pre_process, so every parameter routing through an address field is now guarded. I swept 36 address-ish maker parameters across mh.py, ipv4.py, sctp.py and hip.py with True and False:

ipv4._make_opt_rr  (route=[True])  -> FieldValueError (guarded)
ipv4._make_opt_lsr (route=[True])  -> FieldValueError (guarded)
ipv4._make_opt_ssr (route=[True])  -> FieldValueError (guarded)
sctp._make_param_ipv4(address=True) -> FieldValueError (guarded)
esp  SecurityAssociation(destination=True) -> ProtocolError (guarded)
mh._make_opt_aca / _alt_ipv4_coa / _anchored / _dlif_lla / _hnp / _ipv4_ack ... -> guarded

Every site I had listed as "not covered" is in fact covered, including SCTP._make_param_ipv4, which I had specifically called out. That claim was wrong and is withdrawn here rather than left in a stale note.

What genuinely still accepts a bool

Four maker parameters, all in mh.py:

mh._make_opt_bid(address=<bool>)     -> 000001000000000001
mh._make_opt_lma_up(address=<bool>)  -> 00000000000001
mh._make_opt_lmaa(address=<bool>)    -> 00010000000001

(mh._make_opt_ni(home=<bool>) also surfaced in the sweep and is not a defect — NonceIndicesOption.home is a UInt16Field at pcapkit/protocols/schema/internet/mh.py:670, a nonce index rather than an address. That one was a false positive of my own name-matching heuristic, and home=True becoming 1 in a uint16 field is correct behaviour.)

The mechanism — one cause, not three

These parameters are not backed by IPv4AddressField/IPv6AddressField. They are backed by SwitchField, which selects the concrete field at runtime from the option length or address family:

mh.LMAAddressOption.address:            IPv4Address | IPv6Address | bytes   field=SwitchField
mh.LMAUserPlaneAddressOption.address:   IPv4Address | IPv6Address | bytes   field=SwitchField
mh.TargetCareofAddressSuboption.address: IPv4Address | IPv6Address | bytes  field=SwitchField

Because the dispatch happens inside SwitchField, the value never reaches _IPAddressField.pre_process, so #500's guard cannot fire. BindingIdentifierOption is the same shape — per :rfc:5648#section-4.3 the option carries no address-family flag, so the family is selected from the option length alone, which is exactly why a switch is used.

Every address-typed SwitchField in the schema tree

Enumerated rather than sampled — 7 attributes, and the gap is not confined to mh.py:

module class.attribute annotation
internet.hip Locator.value IPv6Address | LocatorData
internet.mh DelegatedMNPOption.prefix IPv4Address | IPv6Address
internet.mh LMAAddressOption.address IPv4Address | IPv6Address | bytes
internet.mh LMAUserPlaneAddressOption.address IPv4Address | IPv6Address | bytes
internet.mh MNIDOption.identifier bytes | str | IPv6Address
internet.mh TargetCareofAddressSuboption.address IPv4Address | IPv6Address | bytes
transport.tcp MPTCPAddAddress.address IPv4Address | IPv6Address

Two of those are already effectively handled: MNIDOption.identifier is where #469/#481 put a guard at the call site, and DelegatedMNPOption.prefix raised ProtocolError in my sweep. The rest are untested for this and at least three demonstrably corrupt.

Suggested shape

The guard wants to sit where the switch resolves, not at each of the seven call sites — a call-site fix is what #481 did for one of them and is exactly how the root cause survived to become #491. Either SwitchField rejects a bool before dispatching, or each address-selecting switch routes through a wrapper that does.

MPTCPAddAddress.address is worth an explicit test either way: it is in tcp.py rather than mh.py, so a fix scoped to Mobility Header would miss it.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions