Skip to content

A wire-derived length underflows into a struct format string, raising bare struct.error on untrusted input #438

Description

@JarryShaw

A wire-derived length field is used in arithmetic that can go negative, and the negative value reaches a struct format string, so parsing untrusted input raises a bare struct.error instead of an in-library exception.

Reproduction

On main (44aa38ae8):

from pcapkit.protocols.internet.hopopt import HOPOPT

buf = bytes.fromhex('3b00' '0800' '0000' '00' '00')   # SMF_DPD, I-DPD, Opt Data Len = 0
HOPOPT(buf, len(buf))
struct.error: bad char in struct format

The -1 is being formatted into a struct template as '-1s'.

Mechanism

pcapkit/protocols/schema/internet/hopopt.py:426, in SMFIdentificationBasedDPDOption:

id: 'bytes' = BytesField(length=lambda pkt: pkt['len'] - (
    ...
))

pkt['len'] is Opt Data Len, straight off the wire, and the subtraction underflows when a peer declares 0. Nothing clamps it, so the negative width propagates into the field's struct template.

Why it matters

pcapkit parses untrusted input, and struct.error is not one of the library's own exception types — a caller cannot catch it through pcapkit.utilities.exceptions, and it does not carry the field or protocol context an in-library error would. This is the class of defect the project has been closing elsewhere: it is the same shape as the FieldValueError guards added in #432 for non-progress, and the same shape as the unbounded FieldBase.unpack allocation noted in #432's body.

Related sites worth checking in the same pass

Three sibling lambdas in pcapkit/protocols/schema/internet/hip.py — lines 699, 717 and 735 — all compute pkt['len'] - 1 from a wire field with no floor. hopopt.py:361, :614 and ipv6_opts.py's counterparts subtract larger constants (pkt['len'] - 8 - pkt['cmpt_len'] * 4, pkt['len'] - 2 - …) and so have more headroom to underflow, not less.

There was an earlier unverified report that _option_padding could go negative and reach struct.calcsize('-1s'). This is very likely that defect, now with a reproduction — worth treating them as one item.

Provenance

Found by an independent review pass on #432 while probing truncated input, and reproduced here on main with the input above. It is not caused by #432 — it fails identically on both trees for a properly-sized, non-truncated option, so it predates that PR and its progress guard is not meant to cover it. Recorded separately rather than folded in, to keep #432 to the hang fix.

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