Skip to content

Four more HIP parameters underflow pkt['len'] - 2 and silently return an empty list #463

Description

@JarryShaw

Four more HIP parameters share the unguarded pkt['len'] - 2 shape that #455 fixed for RegInfoParameter, and they fail the silent way rather than crashing: a malformed parameter parses "successfully" to an empty list with no exception at all.

The four sites, on da2422728

pcapkit/protocols/schema/internet/hip.py:454   NATTraversalModeParameter.modes
pcapkit/protocols/schema/internet/hip.py:808   TransportFormatListParameter.formats
pcapkit/protocols/schema/internet/hip.py:826   ESPTransformParameter.suites
pcapkit/protocols/schema/internet/hip.py:941   HIPTransportModeParameter.mode

All four are the same expression on a wire-controlled UInt16Field len with no lower bound:

length=lambda pkt: pkt['len'] - 2,

Reproduction

A parameter with Length=0 makes the computed length -2:

NATTraversalModeParameter      NO EXCEPTION -> lists=[[]]
TransportFormatListParameter   NO EXCEPTION -> lists=[[]]
ESPTransformParameter          NO EXCEPTION -> lists=[[]]
HIPTransportModeParameter      NO EXCEPTION -> lists=[[]]

ListField's loop is while length > 0, so a negative length simply returns an empty list. Nothing signals that the input was malformed — which is arguably worse than the struct.error crash the PaddingField/BytesField sites produce, because a caller gets a plausible-looking parse.

Relationship to #455

#455 fixed exactly this shape at three sites and established the pattern: a named function raising FieldValueError from pcapkit.utilities.exceptions, following the module's own mpl_opt_seed_id_len convention, with duplicated expressions consolidated into a shared helper. PR #460 implemented that and named reg_info_list_len for the RegInfoParameter case. These four want the same treatment — four more named functions, or one shared parametrised helper, since the expression is byte-identical across all four.

Note the offset is genuinely - 2 for these, and it is - 2 for RegInfoParameter for a different reason: RegInfoParameter reads two UInt8Fields (min_lifetime, max_lifetime) before its list, whereas these four read a two-octet reserved or port field. So a shared helper is fine, but it should be named for the shape rather than for either field's meaning.

Two lines that are NOT affected, recorded so nobody re-checks them

pcapkit/protocols/schema/internet/hip.py:420 (HIPTransformParameter.suites) and :635 (HITSuiteListParameter.suites) use a bare pkt['len'] with no subtraction, so they cannot underflow this way. I conflated those two with the four above at one point; they are a different shape and are fine.

Provenance

Flagged out-of-scope by the agent implementing #455 in PR #460, then independently verified empirically by that PR's reviewer, and reproduced a third time here before filing. Correctly excluded from #460, whose scope is the three sites #455 named.

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

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions