fix(corekit): bound the total zero padding a parse may synthesise (#573) - #593
Conversation
`_MAX_ZERO_PAD_LENGTH` bounds one field, and a packet holds many, so the sum was unbounded: a declared length just under the ceiling is honoured however often it is declared. 200 minimal PCAP-NG Decryption Secrets Blocks -- 4,800 wire octets, each declaring `secrets_length` of 262,142 against two supplied octets -- retained 50.0 MiB, 10,922x per block, with the per-field guard correctly letting every one of them through. - `FieldBase.unpack` now keeps a context-local `[supplied, synthesised]` ledger and refuses a shortfall past `_MAX_ZERO_PAD_SHORTFALL` once the total of such shortfalls passes `_MAX_ZERO_PAD_LENGTH + _ZERO_PAD_BUDGET_RATIO * supplied`. The same 200 blocks now retain 0.2 MiB, 54.6x rather than 10,922x. - A shortfall of 65,536 octets or fewer -- the whole span of a 16-bit wire length, so every shortfall a snapshot-truncated capture, a truncated option area or an over-long `ihl` can produce -- is padded unconditionally and charged to nothing. Without that band, a running budget alone made the same legitimate 54-octet frame parse to four different results across 40 identical calls, depending only on what preceded it. - `_zero_pad_budget` scopes the ledger for a caller that wants one parse bounded on its own. Nothing in the package calls it yet. Unit tier 1193 passed, 5 skipped. Every capture in examples/captures/, each truncated at some 8,700 offsets, 190 snapshot-length rewrites and the synthetic offload shapes all parse with identical tallies and identical outcomes. Fixes #573
|
✅ GOOD TO GO at head sha |
|
Reviewer: Sonnet; PR authored on Opus 5. The author reports having already run its own Sonnet cross-review and acted on four NEEDS CHANGES items — per instruction, I am treating that as the author's claim about its own process, not as a review, and re-deriving everything myself. Falsify-not-bless pass on PR #593 ( 1. The headline claim: is the partial fix justified? (independently reproduced, not taken on trust)Ran the PR's own math independently rather than accepting the "1,637x both before and after" narrative: These match to within rounding of the file-octet divisor. This is the actual substance of "crafted and legitimate are indistinguishable at this layer," confirmed by arithmetic rather than by reading the PR's assertion of it. 2. The counterexample ruling out a plain running budget — reproduced exactly, independentlyThis is the single most interesting claim in the PR and I reproduced it rather than trusting the narrative. Wrote a standalone script against the actual This matches the PR's claimed result exactly, call-index for call-index ("one result on 37 of 40 calls... and calls 26, 33 and 39" differ). A guard whose answer depends on parse history for byte-identical input is a real, demonstrated defect in the naive design, and this independently confirms the PR's justification for the two-band design (16-bit unconditional / 32-bit budgeted) is not hand-waving. 3.
|
…#594) A PCAP-NG option's length is a 16-bit wire field, so a four-octet option header can declare 65,535 octets of payload. Nothing bounded that against the option area the block frames, and the field layer pads every shortfall inside a 16-bit length unconditionally -- deliberately, so a snapshot-truncated capture still parses -- so repeating such an option across blocks amplified without limit. * `bounded_option()` clamps an option or record payload to the octets its area has left at that field, and warns (`SchemaWarning`) when it does. Applied to all 15 variable-width option and record payloads in the schema. * `bounded_area()` clamps a packet block's option area to the octets the block itself holds, less the trailing Block Total Length. Without it a block could declare 1,000,000 octets while holding 36, size its area from the lie, and synthesise 65,535 octets anyway -- 1,820x. A no-op on well-formed blocks, where the two are equal by construction. * The bound comes from this layer because the field layer cannot see it: what separates the crafted case from a legitimate one is inconsistency with the block's own declared framing, not the shortfall's size. * Clamping, not refusing: a block read has no catch point above `FieldBase.unpack`, so one refusal aborts the whole extraction. It reads only the block's own framing, so it is history-independent. * The negative-remainder guard is load-bearing on the unpack path, where `__length__` can already be past zero: without it a ten-octet area raises `struct.error` from a `'-2s'` template. * Block-level payload fields are left alone; #593 already budgets that 32-bit band, and the five non-packet option areas keep the framing assumption, which #678 is the general fix for. Crafted capture of 2,000 Enhanced Packet Blocks in 80,048 octets: 131,070,000 octets of padding and 1637.393x before, 0 octets and 0.000x after, with all 2,000 frames and 2,000 options still parsed. Worst ratio over five adversarial shapes after the fix is 0.862x, against 1637.393x/960.360x/224.067x before. Zero clamps fire across all six PCAP-NG fixtures (338 options), and 501 truncation levels of `dhcp.pcapng` are byte-identical. 117 tests and 742 subtests pass across the PCAP-NG and contract suites; 9 of the 15 new tests fail on `main` (exit 1 to 0). Fixes #594
…#594) (#676) A PCAP-NG option's length is a 16-bit wire field, so a four-octet option header can declare 65,535 octets of payload. Nothing bounded that against the option area the block frames, and the field layer pads every shortfall inside a 16-bit length unconditionally -- deliberately, so a snapshot-truncated capture still parses -- so repeating such an option across blocks amplified without limit. * `bounded_option()` clamps an option or record payload to the octets its area has left at that field, and warns (`SchemaWarning`) when it does. Applied to all 15 variable-width option and record payloads in the schema. * `bounded_area()` clamps a packet block's option area to the octets the block itself holds, less the trailing Block Total Length. Without it a block could declare 1,000,000 octets while holding 36, size its area from the lie, and synthesise 65,535 octets anyway -- 1,820x. A no-op on well-formed blocks, where the two are equal by construction. * The bound comes from this layer because the field layer cannot see it: what separates the crafted case from a legitimate one is inconsistency with the block's own declared framing, not the shortfall's size. * Clamping, not refusing: a block read has no catch point above `FieldBase.unpack`, so one refusal aborts the whole extraction. It reads only the block's own framing, so it is history-independent. * The negative-remainder guard is load-bearing on the unpack path, where `__length__` can already be past zero: without it a ten-octet area raises `struct.error` from a `'-2s'` template. * Block-level payload fields are left alone; #593 already budgets that 32-bit band, and the five non-packet option areas keep the framing assumption, which #678 is the general fix for. Crafted capture of 2,000 Enhanced Packet Blocks in 80,048 octets: 131,070,000 octets of padding and 1637.393x before, 0 octets and 0.000x after, with all 2,000 frames and 2,000 options still parsed. Worst ratio over five adversarial shapes after the fix is 0.862x, against 1637.393x/960.360x/224.067x before. Zero clamps fire across all six PCAP-NG fixtures (338 options), and 501 truncation levels of `dhcp.pcapng` are byte-identical. 117 tests and 742 subtests pass across the PCAP-NG and contract suites; 9 of the 15 new tests fail on `main` (exit 1 to 0). Fixes #594
…ts sweep claim - The journal export block's bare struct.error, which #699 now fixes too: the block's own 32-bit NUL padding was read as a binary field's name, so every entry of unaligned length raised. Reachable from valid input. - The "no foreign exception" claim scoped to the truncation sweep, with the two families a fuzz still reaches named and measured unchanged either side: #701 (an unassigned block type raising from aenum) and #593's 32-bit band. - #704, the silent loss of every journal field after a binary one, noted as filed rather than fixed. - Where the two remaining ProtocolError levels actually cut: the Interface Description Block's if_tsresol option, not the Section Header Block. `python util/changelog_md.py --check` exits 0.
…ts sweep claim - The journal export block's bare struct.error, which #699 now fixes too: the block's own 32-bit NUL padding was read as a binary field's name, so every entry of unaligned length raised. Reachable from valid input. - The "no foreign exception" claim scoped to the truncation sweep, with the two families a fuzz still reaches named and measured unchanged either side: #701 (an unassigned block type raising from aenum) and #593's 32-bit band. - #704, the silent loss of every journal field after a binary one, noted as filed rather than fixed. - Where the two remaining ProtocolError levels actually cut: the Interface Description Block's if_tsresol option, not the Section Header Block. `python util/changelog_md.py --check` exits 0.
…ts sweep claim - The journal export block's bare struct.error, which #699 now fixes too: the block's own 32-bit NUL padding was read as a binary field's name, so every entry of unaligned length raised. Reachable from valid input. - The "no foreign exception" claim scoped to the truncation sweep, with the two families a fuzz still reaches named and measured unchanged either side: #701 (an unassigned block type raising from aenum) and #593's 32-bit band. - #704, the silent loss of every journal field after a binary one, noted as filed rather than fixed. - Where the two remaining ProtocolError levels actually cut: the Interface Description Block's if_tsresol option, not the Section Header Block. `python util/changelog_md.py --check` exits 0.
…ts sweep claim - The journal export block's bare struct.error, which #699 now fixes too: the block's own 32-bit NUL padding was read as a binary field's name, so every entry of unaligned length raised. Reachable from valid input. - The "no foreign exception" claim scoped to the truncation sweep, with the two families a fuzz still reaches named and measured unchanged either side: #701 (an unassigned block type raising from aenum) and #593's 32-bit band. - #704, the silent loss of every journal field after a binary one, noted as filed rather than fixed. - Where the two remaining ProtocolError levels actually cut: the Interface Description Block's if_tsresol option, not the Section Header Block. `python util/changelog_md.py --check` exits 0.
1 participant
Fixes #573
Root cause
pcapkit/corekit/fields/field.py:472—buffer[:length].rjust(length, b'\x00').#554's fix put a ceiling on that one call (
_MAX_ZERO_PAD_LENGTH = 0x40_000atfield.py:63, guard atfield.py:453), and the ceiling is correct for a single field. It says nothing about how many fields a parse may pad, so the sum had no bound at all: a declared length just under the ceiling is honoured however often it is declared, and nothing in the guard is wrong when it lets each one through.Every zero-pad-on-short-read in the library funnels through that one line. The
unpackoverrides incollections.py,misc.pyandschema.pyeither delegate to it or do not pad (PayloadFieldreads plainly andSchema.unpackhandles it beforefield.unpackis reached); the only otherrjustin the package,SeekableReader.truncateatpcapkit/corekit/io.py:328, has no callers.Re-measured, not taken on trust
The issue's figure reproduces exactly. 200 calls to
UnknownSecrets.unpackwith__length__of 262,142 against two supplied octets, counting a minimal Decryption Secrets Block at its 24 wire octets:The issue notes this was never shown end to end through
Extractor. It can be, and it is worse there — unbounded and linear in the block count. A crafted 80,048-octet.pcapngof 2,000 Enhanced Packet Blocks, each carrying one option whose own 16-bit length declares 65,535 octets against four real ones (UnknownOption.dataatpcapkit/protocols/schema/misc/pcapng.py:494). An option's length is independent of the block total length thatpcapkit/protocols/misc/pcapng.py:968seeks by, which is why many of them fit in a small file where aCustomBlockor a DSB gives only one:All measurements ran with
PYTHONSAFEPATH=1, the worktree root put onsys.pathexplicitly,assert pcapkit.__file__.startswith(<worktree>)before any other import, andRLIMIT_AScapped at 1.5 GiB so a runawayrjustdies rather than taking the host with it.The fix, and why it is shaped the way it is
FieldBase.unpackkeeps a context-local[octets supplied, octets synthesised]ledger and refuses once the synthesised total passes_MAX_ZERO_PAD_LENGTH + _ZERO_PAD_BUDGET_RATIO * supplied. The 200-block scenario now retains 0.2 MiB instead of 50.0 MiB — 54.6x instead of 10,922x per block.A running budget alone is not a safe fix, and that is the substance of this change rather than a footnote. A capture cut short by its snapshot length pads legitimately and must keep parsing — #431, and the counterexample that declined #571 — and it pads far more than it reads. A budget tight enough to matter starts refusing real captures, and worse, refuses them sometimes. Measured with a running budget over all padding at a ratio of 1,024: the same legitimate 54-octet frame declaring an IPv4 total length of 65,535 — what a capture of offload-sized segments truncated to a 54-octet snapshot looks like, padding 65,495 octets per frame from 54 read — parsed to four different results across 40 identical calls, one on 37 of them and three different ones on calls 26, 33 and 39, because whether the shortfall fit depended on what had been parsed before it.
beholderatpcapkit/utilities/decorators.pycatches the refusal and substitutesRaw, so it surfaced as a silently different innermost layer rather than an error. A guard whose answer moves with history is worse than the amplification it bounds.So the guard has two bands, and the lower one is what makes the upper one safe:
_MAX_ZERO_PAD_SHORTFALL(65,536): padded unconditionally, never consulting the ledger and never charged to it. 65,536 is the whole span of a 16-bit wire length — how an IP header, an IPv6 payload, a TCP or IPv4 option and a PCAP-NG option all declare their size — so no shortfall a snapshot-truncated capture, a truncated option area or an over-longihlcan produce is ever refused, on the first frame or the ten-thousandth. The largest legitimate single shortfall found anywhere in measurement was 65,495 octets (that offload frame); the largest from a truncated PCAP-NG option was 64,750. Both are inside it, and so is anything else 16 bits can express. Those shortfalls are not tallied against the budget either: the offload capture pads some 935 octets per octet read, all of it in this band, and charging that would exhaust the budget within a few frames and put the history-dependence straight back._ZERO_PAD_BUDGET_RATIO = 0x10(16). That band is what a 32-bit PCAP-NG block or secrets length reaches, which is where A per-field padding ceiling does not bound their sum: ~10,900x amplification survives the #554 fix #573's amplification lives. One such shortfall is legitimate — it is the final block of a capture truncated at EOF — and the_MAX_ZERO_PAD_LENGTHterm covers any single one of them outright from a cold start, since a shortfall past that ceiling is already refused by Unbounded allocation in FieldBase.unpack: a wire-declared length drives rjust() with no bound #554's guard. Repeating one is not legitimate: truncation cuts the end of a file, so a real capture has one short block, not two hundred.Why a truncated capture still parses
Verified rather than argued, on the tree in this PR against
origin/main:examples/captures/Ethernet(...)andIPv4(...)on a 54-octet offload frame, 100 calls"Identical" here compares the instrumented padding and supplied-octet tallies and each parse's outcome — frame count, or exception type and message — so a cut that changed from parsing to crashing would show up rather than be swallowed by a bare
except. That was a fair criticism of the first version of the harness and it is fixed.What this does not close
Stated plainly because it is the weakest point of the change and I would rather it be reviewed than discovered.
The 16-bit band is deliberately untouched, so a 16-bit-declared length can still be repeated without limit. The 2,000-block Enhanced Packet Block file above amplifies by 1,637x both before this change and after it.
That is not an oversight in the bound. Parsing a bare 40-octet IPv4 header declaring a total length of 65,535 — a legitimate offload-sized frame truncated to its snapshot length — amplifies by the same 1,637x (65,495 padded per 40 supplied). The crafted route and the legitimate one have the same ratio, so no budget at this layer can separate them; that is the measured reason the ratio is not simply lowered. Separating them needs the frame's own
incl_len/orig_len: a crafted block claims nothing was truncated while declaring more than it holds, and a snapshot-truncated frame says so on the wire. Neither is visible anywhere inpcapkit/corekit/fields/, so that fix belongs at the protocol layer and is not in this PR.The budget is not scoped per file.
_zero_pad_budget()exists for a caller that wants one parse bounded on its own, and nothing in the package calls it — the layer that knows where a run ends should, and that is not this one. As shipped the bound is over everything a context has parsed. It is still proportionate to the octets that context was genuinely given, so a long-running service earns more allowance by doing more real work rather than banking an unlimited one, but it is looser than per-file and the docstring now says so.suppliedcounts octets as each field saw them, not octets consumed from the file. A nested schema re-reads its enclosing field's span, andOptionField.unpackpeeks an option's type field then rewinds and parses the same span again — measured at 28 credited octets for a 24-octet IPv4 header carrying four one-octetNOPoptions. The over-count is bounded by nesting depth, so the allowance is somewhat more generous than the ratio alone suggests; it cannot grow without limit, which is the property that matters.Found and deliberately not fixed
Three separate defects turned up alongside this and are left alone to keep the change reviewable:
struct.errorescapes the documented exception hierarchy. A callable field length that resolves negative reachesFieldBase.length→struct.calcsize('-262140s')→struct.error: bad char in struct format. The concrete route is a DSB whoseoptionslength,pkt['length'] - 20 - pkt['secrets_length'] - ...atpcapkit/protocols/schema/misc/pcapng.py:1607, goes negative;OptionField.unpack'swhile length > 0loop never runs, leavesoption_paddingnegative, and that feedspadding_opts: PaddingField(...). A caller catchingpcapkit.utilities.exceptions.*does not catch it. Reproducible from a 24-octet crafted block.Schema.unpacktreats a legitimately declared zero length as falsy.payload_length = length or packet['__length__']atpcapkit/protocols/schema/schema.py:824substitutes the whole remaining budget for acaptured_lenof 0, over-reads, and silently misaligns everything after it. An empty captured packet is legitimate PCAP-NG.MemoryErroron a legitimately truncated real fixture.examples/captures/dhcp_little_endian.pcapngcut to 161 octets raisesMemoryErroratpcapkit/protocols/protocol.py:1016, viaPayloadField.unpack→self._protocol(file, length)→ recursive protocol construction. Confirmed identical before and after this change (both trees, same cut, same line), so it predates the budget and this PR neither causes nor fixes it — but it is the failure class OptionField.unpack never returns for a well-formed HOPOPT header with an SMF_DPD option #431 and corekit: reject short dynamic field buffers #571 exist to prevent, reached by a path this fix does not touch.#591 (
NumberField's latched_need_process) was checked for interaction and there is none: it is a packing defect inpre_process, the amplification here is inunpack, and aNumberFieldpads at most 8 octets — three orders of magnitude below either band. No test in this PR uses aNumberFieldor a callable length.Tests
tests/corekit/test_fields_field.py, 14 new cases inFieldBaseCumulativePaddingBudgetTests, including the amplification bound itself rather than only a smoke test.Against the unfixed tree (
origin/main'sfield.py, same test file) — 8 failed, 17 passed, exit 1, three of them on substance:With the fix — 25 passed, exit 0.
Two of the new cases are regression guards rather than fix demonstrations, and pass either way by design:
test_a_shortfall_within_a_16_bit_length_is_never_refused(400 consecutive 65,536-octet shortfalls, 26.2 MiB, all padded) andtest_the_same_short_read_answers_the_same_whatever_preceded_it(the same 65,495-octet shortfall answering identically 500 times).setUpdeliberately does not read the new constants, so the behavioural cases fail on their assertions rather than on anAttributeErrorraised before any of them runs.Coverage of
pcapkit/corekit/fields/field.pyrises 79% → 84% (statements 101 → 126, missed unchanged at 16, all of them pre-existing);pcapkit/corekit/fields/overall 77% → 78%. Measured withcoverage run -m pytest tests/corekit, scoped rather than over the whole suite.Unit tier: 1193 passed, 5 skipped (
pytest tests --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'), against 1191 before.Hot-path cost, since
unpackruns once per field per packet: parsinghttp.pcapis 41,327unpackcalls, best-of-5 0.5609s before and 0.5759s after, about +2.7% — measured under concurrent load, so noise-dominated at that magnitude.