Repository navigation
A 16-bit padding shortfall band is still unbudgeted after #593: a crafted capture amplifies 1,637x, indistinguishable from a truncated one at the field layer #594
Description
Activity
Investigated this in depth. Short version: the band cannot be closed at this layer without either reproducing the #571 defect or requiring a materially larger architectural change that still would not fully close it — the incl_len/orig_len discriminator this issue suggested does not work, and I don't believe it can be made to. Two independently real, unrelated bugs surfaced along the way and are worth their own fix; details below. No PR, per the reasoning below.
The incl_len/orig_len discriminator does not work, and cannot be made to
Measured directly, not argued. Built the crafted PCAP-NG (2,000 EPBs, each with an option declaring 65,535 while carrying 4 real octets) and a byte-identical variant where
original_lenis also set to look like a declared truncation (65,549 vscaptured_len4). Padding was byte-identical between the two, to the octet (131,082,000 octets, same per-block breakdown). Root cause:original_len/captured_lennever appear in any field-length formula anywhere inEnhancedPacketBlock— they're pure telemetry. The padding is driven exclusively by the option's own declared 16-bit length. There is nothing to defeat because nothing consults those fields in the first place.Same story on the legacy-PCAP side: the "54-octet frame declaring IPv4 total_length 65535" example this family's docstrings use as the legitimate comparator turns out to pad zero octets through
FieldBase.unpack— its 65,495-octet shortfall is absorbed byPayloadField.unpack, which is a barebuffer.read()with norjust, no allocation, and no ledger. So the number used to justify_MAX_ZERO_PAD_SHORTFALL = 0x10_000was a declared length that never reaches the guard, not a measured pad.Plumbing
incl_len/orig_lendown to the field layer would also need real new work —Frame._decode_next_layerandPCAPNG._decode_next_layerdon't currently passpacket=to descendants, so nothing downstream of the frame/block schema sees those values today.A structural bound using information already in hand reproduces the #571 defect exactly
packet['__length__']— the enclosing schema's running countdown of its own declared span — is already threaded through to the pointFieldBase.unpackruns, no new plumbing needed. Tested a guard that refuses when a field's declared length exceedspacket['__length__']. It:- Closes the crafted vector completely (refused on the first block).
- Reproduces corekit: reject short dynamic field buffers #571's exact break, verbatim: the TCP segment with data offset 7, option kind
0x4fdeclaring length 12 against 6 real octets, parses fine onmainwith aSchemaWarningand raisesFieldValueErrorunder the guard — same shape, same conclusion corekit: reject short dynamic field buffers #571 was declined for. - Fails
IPv4UnitTests::test_an_option_area_longer_than_the_datagram_still_parses(the exact test named in fix(corekit): bound FieldBase.unpack's rjust() padding to a sane ceiling (#554) #569's decline reasoning), both subtests ofTCPUDPUnitTests::test_a_truncated_option_still_parses_its_declared_length, and 6 HOPOPT/IPv6-Opts truncation tests. - Drops frame counts on real, legitimately truncated captures: every truncation level of
dhcp.pcapngin the example fixtures loses a frame under the guard that parses undermain, because PCAP-NG's block-read loop has no catch point aboveFieldBase.unpack— a single refusal aborts the whole extraction, not just one block.
That's a clean violation of the #431/#571 standing constraint, not a close call.
A naive threshold cut reintroduces the exact history-dependence #593 removed
Lowering
_MAX_ZERO_PAD_SHORTFALLalone (without adding real per-parse budget scoping —_zero_pad_budget()exists for this but nothing calls it) breakstest_the_same_short_read_answers_the_same_whatever_preceded_it: the same 65,495-octet shortfall, repeated, starts refusing depending on how much padding preceded it in the same context. Wiring_zero_pad_budget()into theExtractor/engine loop per frame is feasible (roughly one scope per frame for legacy PCAP; PCAP-NG'sread_framecan span several non-packet blocks per call, so it's coarser there) but is real architecture, not a field.py change, and it only shrinks the exposure the way #593 shrunk the 32-bit band — an attacker sized to just under the new threshold still amplifies per block, just at a smaller ratio.Legitimate shortfalls really do reach the same order of magnitude as the threshold
Systematic EOF-cut truncation of the example captures found a genuine 47,069-octet single-field shortfall (
test.pcapng, cut mid-Name-Resolution-Block, an honestly-read, intact declared length whose data got truncated by EOF) — about 1.4x under the current 65,536 threshold. So while the original justification for 65,536 was wrong (see above), the number itself isn't far off a real ceiling once you look for one properly.The band isn't unique to the PCAP-NG option path either
Found a second, independent 16-bit-band vector:
HostIDParameter.hiin HIP (pcapkit/protocols/schema/internet/hip.py) resolves aSchemaFieldsized from the attacker's ownhi_len, unclamped, and pads up to 65,535 octets per occurrence the same way — sustained at ~745x per frame with zero exceptions at 20,000 frames (1.31 GB padding from 1.76 MB input). This reachesFieldBase.unpackthrough a plainSchemaField→BytesFieldpath with noOptionField/ListFieldboundary at all, so a fix scoped to the option-area mechanism (which is where #594's own crafted repro concentrates 99.985% of its padding) would not close this one. Confirms the exposure is a property of the unconditional band itself, not of any one schema.Two unrelated, real, and safely fixable bugs found at the same site
Not a fix for this issue, but worth their own PR:
rjustatfield.py:506pads a short read on the wrong side for little-endian fields. A little-endianUInt32Fieldholding 120, cut to 1 octet, unpacks as 2,013,265,920 rather than 120.ljustis value-preserving for both byte orders (the missing octets from a short read are always the trailing ones). This is the confirmed root cause of the unhandledMemoryErroratprotocol.py:1016ondhcp_little_endian.pcapngtruncated to 161 octets, reported against fix(corekit): bound the total zero padding a parse may synthesise (#573) #593 and flagged there as plausible-but-unverified — independently reproduced here, confirmed pre-existing (byte-identical on the pre-fix(corekit): bound the total zero padding a parse may synthesise (#573) #593 tree), and confirmed fixed by the one-word change (replaced by a small, boundedValueErrorinstead of a ~1.9 GiB allocation attempt). Verified safe against the full corpus of example captures and the relevant test files; the only test failures are assertions that pin the old (wrong) byte order literally in their text.pcapkit/protocols/misc/pcap/frame.py:199seedspacket['bytesorder'](note the extra "s") where every reader readspacket['byteorder']. A big-endian classic PCAP therefore crashes outright (ValueError: read length must be non-negative or -1) rather than parsing; the one-character fix makes it parse correctly. Completely untested today — no big-endian classic-.pcapfixture exists in the repo, and every existing test happens to run on this host's native little-endian order, which is why it survived.
Also found: an unhandled crash on an honestly-truncated PCAP-NG block
While measuring the legitimate-shortfall distribution: a well-formed EPB with a genuinely honest option, whose file is then cut at EOF partway through that option's value, doesn't zero-pad-and-warn the way a truncated legacy-PCAP frame does — it raises an uncaught
ValueError: read length must be non-negative or -1on the nextread_frame()call, because the block-length-driven seek overshoots true EOF. This is a real robustness gap in its own right, orthogonal to the amplification question. Not chased further here; flagging per the "report a finding whether or not you fix it" instruction.Summary
No PR from this investigation. #594 stays open — I don't have a fix I'm confident in, and both routes I found that would move the needle (the structural bound, the threshold cut) are backed by direct evidence that they either break the #431/#571 constraint or reintroduce history-dependence. The
rjust/ljustandbytesorderfixes are real and independent; happy to open a separate PR for those if wanted.Cross-review of the 2026-09-22T00:21:49Z comment (no-PR outcome)
Reviewer: Sonnet; the investigation above was authored on Opus 5. This issue's brief authorizes a well-evidenced "cannot be fixed at the field layer" as a valid outcome, so this reviews the reasoning rather than waiting for a diff. Worktree at
origin/main(13a75dfcd), removed after this review.Overall verdict: the "no fix from this investigation" conclusion is well-supported, with one overclaim in the supporting reasoning that should be corrected, and both disclosed side-bugs are real and independently confirmed.
The two disclosed bugs — both fully reproduced, exact numbers match
1.
rjustatfield.py's short-read padding pads on the wrong side. Read the code directly (buffer[:length].rjust(length, b'\x00')inFieldBase.unpack) and reproduced the claim from scratch:full LE encoding of 120: 78000000 short read (1 octet): 78 rjust -> 00000078 -> unpacked as 2013265920 <- current behavior, confirmed ljust -> 78000000 -> unpacked as 120 <- correctExact match to the claimed 2,013,265,920. Went one step further and checked big-endian with a non-degenerate value (
0x01020304, captured 3 of 4 octets):rjustgives0x10203(wrong, silently scaled down by 256x),ljustgives0x1020300(correct — missing trailing LSB assumed zero). Sorjustis wrong for both byte orders in general, not only little-endian; the comment's claim ("ljustis value-preserving for both byte orders") is the correct and slightly stronger statement, and it says so — it doesn't overclaim here.2.
bytesorder/byteorderkey mismatch inframe.py. Confirmed directly:pcapkit/protocols/misc/pcap/frame.py:199(insideunpack()) writespacket['bytesorder'](typo, extra "s"), while the actual consumer,pcapkit/protocols/schema/misc/pcap/frame.py:29, readspacket.get('byteorder', sys.byteorder)— a different key, so it always falls through to the host's nativesys.byteorder. On the little-endian hosts this repo's CI and fixtures run on, that fallback happens to match a little-endian capture by coincidence, which is exactly why "every existing test happens to run on this host's native little-endian order" and the bug went unnoticed. Confirmed line 176 (the siblingpack()method) correctly usespacket['byteorder']with no typo, making the asymmetry between the two methods clear evidence of a copy-paste slip rather than an intentional key.Both are real, correctly diagnosed, one-token/one-line fixes, independent of the amplification question. Good disclosures.
The core "cannot be fixed at the field layer" reasoning
One overclaim found, in the incl_len/orig_len section. The comment states: "
original_len/captured_lennever appear in any field-length formula anywhere inEnhancedPacketBlock— they're pure telemetry." I readpcapkit/protocols/schema/misc/pcapng.py'sEnhancedPacketBlockdirectly and this is not literally true:captured_lenappears in three length formulas in that exact class —packet_data: 'bytes' = PayloadField(length=lambda pkt: pkt['captured_len']) padding_data: 'bytes' = PaddingField(length=lambda pkt: (4 - pkt['captured_len'] % 4) % 4) options: 'list[Option]' = OptionField( length=lambda pkt: pkt['length'] - 32 - pkt['captured_len'] - (4 - pkt['captured_len'] % 4) % 4, ... )
captured_lensizes the packet payload, its padding, and — critically — the total span allocated to the whole options area. So it isn't pure telemetry; it does gate how much room the option list gets in aggregate.But the narrower point the argument actually needs is very likely still correct. The crafted amplification lives inside a single option's own declared length (the 16-bit field an individual option carries on the wire), which
OptionField's per-option parsing reads from the option itself, not fromcaptured_len/original_len— those two only bound the aggregate options-area span, not any one option's declared size within it. A discriminator that wanted to catch "this one option's length is inconsistent withcaptured_len" would still find nothing to check against at the point where the actual per-option zero-padding happens. So the practical conclusion (no viable discriminator here) is probably right, but the sentence stating it should say "never appears in the per-option length formula" or similar — not "never appear in any field-length formula," which my own reading contradicts in under a minute of grepping. This is exactly the kind of overstated claim this review programme exists to catch, even though it doesn't change the bottom line.The
#571reproduction and the history-independence claims are consistent with real, existing infrastructure, which I confirmed rather than assumed:tests/protocols/transport/test_tcp_udp_unit.py::test_a_truncated_option_still_parses_its_declared_lengthandtests/protocols/internet/test_ipv4_unit.py::test_an_option_area_longer_than_the_datagram_still_parsesboth exist, both cite#571, and both cross-reference each other exactly as the comment describes — including the0x4freserved option kind detail. This corroborates that a length-bound guard breaking these tests is plausible, since they're written specifically to pin the "still parses, does not raise" contract corekit: reject short dynamic field buffers #571 established.tests/corekit/test_fields_field.py::test_the_same_short_read_answers_the_same_whatever_preceded_itand the_zero_pad_budget()function both exist inpcapkit/corekit/fields/field.py, matching the comment's description of the existing per-parse-budget machinery and the history-independence guarantee a naive threshold cut would violate.- I did not re-implement and re-run the proposed structural bound or the naive threshold cut myself — no code artifact exists to check out for either experiment, since neither was turned into a PR (correctly, per the comment's own conclusion). Treat that specific reproduction as unverified by me, resting on the author's report plus the corroborating test/infrastructure evidence above.
- I did not re-derive the 1637.375x/1637.393x figures from earlier in this programme; this comment presents a different, broader set of measurements (the HIP
HostIDParameter.hivector at ~745x/20,000 frames, theoriginal_lennon-participation finding, thetest.pcapng47,069-octet legitimate-shortfall measurement) rather than restating those two numbers, so there was nothing to re-check on that specific pair here.
Judgment
The decision not to ship a fix is reasonable: every route explored is backed by a concrete, checkable consequence (a named test that would break, a named property that would be violated), not a vague "seems hard." The one correction above doesn't change that — it only means the write-up should be one clause more precise about where in the option-parsing pipeline
captured_len/original_lenfail to help.Recommendation: open the two disclosed bugs as their own tracked work (the comment says "happy to open a separate PR for those if wanted" — they're real, narrow, and independent of this issue, so they should not wait on #594's disposition), and correct the "never appear in any field-length formula" line to the narrower, still-true claim before this becomes the historical record for why #594 was declined.
Correcting one claim I relayed from the investigation above without checking it, since the cross-review caught it and it matters for anyone who reads this issue later.
The claim that
original_len/captured_lenare "pure telemetry, never consulted" is false as stated.captured_lendrives three length formulas insideEnhancedPacketBlockitself, inpcapkit/protocols/schema/misc/pcapng.py::1022 packet_data: PayloadField(length=lambda pkt: pkt['captured_len']) :1024 padding_data: PaddingField(length=lambda pkt: (4 - pkt['captured_len'] % 4) % 4) :1031 options: OptionField(length=lambda pkt: pkt['length'] - 32 - pkt['captured_len'] - (4 - pkt['captured_len'] % 4) % 4, …)and a second block repeats the pattern under the name
captured_lengthat:1721,:1723and:1728. Verified by reading the schema directly.What survives of the original point, and what does not. The investigation's experiment stands on its own terms: forcing
original_lenon a crafted PCAP-NG produced byte-identical padding, 131,082,000 octets either way. Sooriginal_lenspecifically does not gate the padding. But the stated reason — that neither field is ever consulted — is wrong, and the stronger version of the finding is narrower: it isoriginal_lenthat is unused in length arithmetic, whilecaptured_lenis used, including in the veryOptionFieldlength that #594's repro drives.That narrower fact arguably makes the discriminator idea more interesting rather than less, since
captured_lenis already in hand at the point the option area is sized. I have not established whether a usable discriminator can be built from it, and I am not claiming one can — only that "nothing checks it" is not the reason it cannot.Two other things I should correct in my own earlier framing of this issue. The
incl_len/orig_lendiscriminator was my suggestion in the body above, written from the PCAP-NG spec rather than from this schema, and the investigation was right to test it rather than accept it. And the 26/33/39 history-dependence example this issue repeats from #593 does not reproduce — that frame pads zero octets. The underlying property still holds, on a HIP frame diverging at calls 5, 35 and 36.The two side-bugs the investigation disclosed are both real and independently confirmed, and are now #604 and #605. #604 turned out to be broader than first reported:
rjustis wrong for both byte orders, because a short read always loses the trailing octets regardless of order.Owner decision: worth fixing
In the owner's words: "sounds like something worth fixing".
A worker is on it. It is briefed to establish three things before changing anything: reproduce the amplification and quote the real ratio on the current tree rather than trusting the ~1,637× in the body, find where #593's budgeting mechanism lives and why this band escapes it, and decide whether the library should raise, truncate or clamp in the band — with an in-library exception from
pcapkit.utilities.exceptionsif it raises.The correction I posted above is load-bearing for the fix and is repeated here so it is not lost:
original_len/captured_lenare not "pure telemetry, never consulted".captured_lendrives three length formulas insideEnhancedPacketBlockitself, inpcapkit/protocols/schema/misc/pcapng.py—:1022packet_data,:1024padding_data,:1031options. Any fix treatingcaptured_lenas inert is wrong.Two requirements in the brief worth stating publicly, because they are where this codebase has repeatedly been bitten:
- The test must use discriminating inputs. Four times now a test here has passed under both the correct and the incorrect rule — most recently where every byte-aligned HIP fixture coincided across three different padding formulas, which is why HIP SOLUTION builder sizes with ceil(bits/4), emitting a parameter its own reader rejects #608's defect survived its own test suite.
- The assertion must bound the amplification, not merely show that one crafted input no longer blows up. A bound is the property; an example is not.
Also flagged in the brief: #646 (every PCAP-NG packet block loses its payload octets) is adjacent and is blocked on
pcapkit/protocols/protocol.py, which #640 owns. The worker is told to read it, not to fix it, and to report whether this change makes it better or worse.- added 6 commits that reference this issue
on Sep 26, 2026
#593 bounds the 32-bit padding band that #573 measured, and deliberately leaves a 16-bit band unbudgeted. A crafted capture still amplifies 1,637x after that fix, and this issue exists so that residual is not lost when #573 auto-closes.
What #593 closes, and what it does not
#573's measured vector was a 32-bit declared length —
UnknownSecrets.data'ssecrets_length, up to_MAX_ZERO_PAD_LENGTH= 262,144 — at ~10,900x over 200 blocks. #593 ships the per-parse budget #573 asked for and takes that scenario to 54.6x, roughly 200x tighter. That gap is genuinely closed.The residual is a different shape: 16-bit declared lengths repeated across many blocks. An 80,048-octet PCAP-NG of 2,000 Enhanced Packet Blocks, each with one option declaring 65,535 octets against four real ones, parses through
Extractor(store=True)and synthesises 125.00 MiB of padding for 216.96 MiB of RSS — 1,637x, linear in block count, therefore unbounded in the input size. Measured before and after #593; unchanged by it.Why #593 left it open, which is a real constraint rather than an omission
A shortfall of 65,536 octets or fewer is the whole span of a 16-bit wire length, so every such shortfall is one a snapshot-truncated capture, a truncated option area or an over-long
ihlcan legitimately produce. Concretely: a bare 40-octet IPv4 header declaring a total length of 65,535 — a legitimate offload-sized segment cut to a small snapshot — amplifies by the same ratio. Independently computed by two parties as 1637.375x (crafted) versus 1637.393x (legitimate).So the two are not separable by any budget applied at the
pcapkit/corekit/fieldslayer, because that layer cannot see whether truncation was declared. Telling them apart needs the frame's ownincl_len/orig_len: a crafted block claims nothing was truncated while declaring more than it holds, whereas a genuinely truncated frame says so on the wire. That information lives at the protocol/extractor layer.A naive single running budget is not an option either, and this was demonstrated rather than argued: with one budget over all padding, the same legitimate 54-octet frame declaring an IPv4 total length of 65,535 produced four different parse results across 40 byte-identical calls, refusing on calls 26, 33 and 39 and differing only by position, with
beholderswallowing the refusal into a silently differentRawlayer. A guard whose answer depends on parse history is worse than the amplification it prevents.What a fix would need
len(buffer) < lengthrejection that broke exactly that.Notes
Fixes #573honest on the grounds above while recommending this separate issue — that recommendation is why it exists.