Conversation
Stop callable field lengths from padding beyond the bytes that were read. Preserve the existing fixed-width padding behavior used by higher-level parser diagnostics.
|
Thanks for this, and for spotting that a naive The blocker: a truncated option stops parsingDiscriminating on whether the length came from packet context doesn't separate the attack from the legitimate short read, because both are wire-derived. A capture cut short by the snapshot length declares an over-long option length in exactly the same way a hostile one does.
A TCP segment with data offset 7, unassigned option kind That warning is the library noting the shortfall and carrying on, which is what #431 established. On this branch the same segment raises The full suite stays green either way, which is why this wasn't obvious — that path has no test at all. I've filed that gap as #572, independently of this PR. What's rightYour exception choice is correct: Where this leaves itI'm going with #569, which bounds on magnitude instead: it rejects only when a declared length exceeds both the buffer and 262,144 octets (libpcap's Two smaller notes for next time: tests for a Please do send more. |
…ing (#554) length is frequently wire-derived -- resolved by a Field's _length_callback against the packet under parse, or built by a schema selector from a value it just read off the wire (e.g. DecryptionSecretsBlock's secrets_data field) -- and so attacker- or corruption-controlled. buffer[:length].rjust(length, b'\x00') zero-padded straight up to that length regardless of how little data buffer actually held, so a ~40-octet PCAP-NG Decryption Secrets Block with a bogus inner length could force a multi-gigabyte allocation. - Raise FieldValueError when a declared length exceeds both the buffer's actual size and a 262144-octet (0x40_000) ceiling -- libpcap's own MAXIMUM_SNAPLEN, and this package's own default snaplen -- before the rjust() that would otherwise allocate and zero-fill on the packet's own say-so. FieldValueError, not BoolError ("must be a bool"), matches the precedent for an invalid/insufficient length in ListField.unpack. - The ceiling is deliberate rather than "any shortfall is bogus": ListField.unpack and OptionField.unpack (collections.py) depend on a short, sometimes empty, tail read past a truncated option area decoding as zero, so an over-long ihl or a capture cut short by its snapshot length reads as end-of-option-list/Pad1 instead of raising (#431). An unconditional reject broke 15 existing tests exercising that mechanism; every one of those fields is a handful of octets, far under the ceiling, and is untouched. - Add tests/corekit/test_fields_field.py: small-field short reads still zero-pad, the ceiling boundary in both directions, a full buffer past the ceiling is accepted, and a 16 GiB declared length against a 2-octet buffer is rejected under a 5s deadline without allocating. Verified against three mutants (loosened comparison, shifted ceiling operator, wrong constant) to confirm the bound is exact. - Update docs/source/changelog/1.5.0.rst and regenerate CHANGELOG.md. Build: pytest tests -- 1271 passed, 17 skipped, 2850 subtests passed, 0 failed. Spell the error message's field name plain rather than quoted, matching the 8/8 precedent across `pcapkit/corekit/` including `field.py:207` five lines away. Adopted from @lux-liang's #571, which proposed the same correction.
…ing (#554) (#569) length is frequently wire-derived -- resolved by a Field's _length_callback against the packet under parse, or built by a schema selector from a value it just read off the wire (e.g. DecryptionSecretsBlock's secrets_data field) -- and so attacker- or corruption-controlled. buffer[:length].rjust(length, b'\x00') zero-padded straight up to that length regardless of how little data buffer actually held, so a ~40-octet PCAP-NG Decryption Secrets Block with a bogus inner length could force a multi-gigabyte allocation. - Raise FieldValueError when a declared length exceeds both the buffer's actual size and a 262144-octet (0x40_000) ceiling -- libpcap's own MAXIMUM_SNAPLEN, and this package's own default snaplen -- before the rjust() that would otherwise allocate and zero-fill on the packet's own say-so. FieldValueError, not BoolError ("must be a bool"), matches the precedent for an invalid/insufficient length in ListField.unpack. - The ceiling is deliberate rather than "any shortfall is bogus": ListField.unpack and OptionField.unpack (collections.py) depend on a short, sometimes empty, tail read past a truncated option area decoding as zero, so an over-long ihl or a capture cut short by its snapshot length reads as end-of-option-list/Pad1 instead of raising (#431). An unconditional reject broke 15 existing tests exercising that mechanism; every one of those fields is a handful of octets, far under the ceiling, and is untouched. - Add tests/corekit/test_fields_field.py: small-field short reads still zero-pad, the ceiling boundary in both directions, a full buffer past the ceiling is accepted, and a 16 GiB declared length against a 2-octet buffer is rejected under a 5s deadline without allocating. Verified against three mutants (loosened comparison, shifted ceiling operator, wrong constant) to confirm the bound is exact. - Update docs/source/changelog/1.5.0.rst and regenerate CHANGELOG.md. Build: pytest tests -- 1271 passed, 17 skipped, 2850 subtests passed, 0 failed. Spell the error message's field name plain rather than quoted, matching the 8/8 precedent across `pcapkit/corekit/` including `field.py:207` five lines away. Adopted from @lux-liang's #571, which proposed the same correction.
#572) (#578) - Add TCP coverage (test_tcp_udp_unit.py): a segment with an unassigned option kind declaring length 12 (and 32), with only 6 data octets behind it, still parses -- FieldBase.unpack left-pads the short read with zero octets rather than raising. - Add the IPv4 equivalent (test_ipv4_unit.py), which needs the option area declared larger than the octets physically present, since IPv4._read_ipv4_options gates on the sum of *declared* option lengths where TCP gates on what was actually consumed. - Only the empty-tail half of #431 had coverage before this (test_an_option_area_longer_than_the_{segment,datagram}_still_parses); a candidate fix for #554 (#571) regressed the truncated-data half while the whole suite stayed green, which is the gap #572 tracks. - Add a changelog entry and regenerate CHANGELOG.md. Verified the new tests fail against a deliberately over-strict guard in FieldBase.unpack (raise whenever len(buffer) < length) with the same FieldValueError message #571 would produce, then pass again once the guard is reverted. No source file is touched in this commit. Build/test: full unit tier (pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py') -- 1118 passed, 8 skipped, 2670 subtests passed.
|
Thanks for the detailed analysis. I agree that snapshot truncation and hostile lengths are indistinguishable at that point in the wire-derived input, so the discriminator in #571 cannot be made sound without breaking the established short-read padding behavior. I’ll leave the bounded-magnitude approach in #569 to address the original issue, and I’ll keep an eye on #572 for the missing truncated-option coverage. I’ve also noted the test-file and generated-changelog guidance for future field changes. Thanks again for the careful review — and I’ll send more focused fixes. |
|
Following up now that #554 is actually fixed: the defect you reported is closed, by a different mechanism than this PR proposed. Two commits did it:
Your instinct was right and your specific guard was not, and it is worth being precise about the difference. A What we did not know then, and now do, is that the obvious next design — a single running budget over all padding — also fails, and for a subtler reason. Measured: with one budget, the same legitimate 54-octet frame declaring an IPv4 total length of 65,535 parsed to four different results across 40 byte-identical calls, refusing on calls 26, 33 and 39 and differing only by position in the run, with The part your PR was reaching for that remains genuinely open is now #594: a 16-bit shortfall band is still padded unconditionally, so a crafted capture amplifies 1,637x — and a legitimately truncated offload frame amplifies by the same ratio, 1637.375x versus 1637.393x, so nothing at the field layer can tell them apart. Separating them needs the frame's own So this PR can be closed as superseded. Thank you for raising it — it is the reason the family got looked at properly, and four issues came out of it. |
JarryShaw
added
fix
2 participants
Fixes #554.
Summary
Why
FieldBase.unpack()padded every short read to the declared field length. For acallable length derived from packet data, that could create bytes that never
arrived. The parser now raises the existing
FieldValueErrorbefore padding ashort dynamically sized field.
Verification
python -m compileallon both changed filesgit diff --check