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.
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