docs(security): record the bounded LZ4 path and the unbounded Arrow one (LAB-2504) - #275
Open
27Bslash6 wants to merge 2 commits into
Open
docs(security): record the bounded LZ4 path and the unbounded Arrow one (LAB-2504)#27527Bslash6 wants to merge 2 commits into
27Bslash6 wants to merge 2 commits into
Conversation
…ne (LAB-2504) Two findings from the LAB-2504 cross-SDK audit of the decompression paths. The default read path is sound and now says so: ByteStorage.retrieve is a pass-through to cachekit-core's extract(), which bounds output at min(512 MiB, 1000 x compressed_len) before decompressing. Worth stating explicitly because the msgpack caps sit on already-decompressed bytes, so a reader can easily assume those caps are the protection when they are in fact downstream of it. ArrowSerializer is the exception and gets a warning. Its read path never touches extract(): deserialize() hands the body to pa.ipc.open_file(...).read_all(), which decompresses zstd with no size or ratio limit. Measured 2,570 bytes -> 64 MiB (26,112:1), a ratio core rejects at 1000:1. Neither existing control covers it: the xxHash3-64 prefix is unkeyed so a backend-write attacker recomputes it, and max_value_size is enforced on serialize only, making it a producer-side quota rather than a check on what comes back off the wire. Documented rather than patched because pyarrow exposes no sound bound -- no read-side size cap, no allocation-limiting pool, and write_table emits a single record batch so per-batch accumulation misses the common case, while a post-read_all() check runs after the allocation it should prevent. The real fix reads each buffer's uncompressed-length prefix from the record-batch Flatbuffers metadata, which is a new parser on untrusted input and wants its own review: LAB-2730. Until then the warning names the mitigations that do work today.
… (LAB-2504) Expert-panel findings on the previous commit. The listed mitigation 'use compression=None' was FALSE and actively harmful -- deserialize() never reads self.compression; pa.ipc.open_file decompresses according to the stored stream's own BodyCompression metadata, so an attacker's forged envelope declares zstd regardless of the reader's setting. Anyone who followed that advice would have believed they were protected. Now stated as a non-mitigation, explicitly. Exposure was wrong in both directions. Narrower: @cache.io is the CachekitIO BACKEND preset and does not select Arrow -- that needs an explicit serializer='arrow' plus the [data] extra. Wider: deserialize() also accepts raw ARROW1 bodies with no checksum at all via the legacy integrity-off branch, so an attacker need not recompute the unkeyed xxh3 prefix the old text said they would. 'A sound bound requires a Flatbuffers walk' was wrong. Uncompressed Arrow IPC allocates in proportion to its own length (measured ratio 1.000), so refusing bodies that declare BodyCompression makes len(body) a real pre-decompression bound in three lines. It costs the compression feature, which is a wire-size and L1-footprint call for the owner rather than a drive-by fix -- so the section now says a bound exists and what it costs, instead of implying none does. The Flatbuffers walk is only needed to KEEP compression. Also records the second measured data point (8,714 -> 256 MiB, 30,805:1) and trims the call-chain symbol names, which would rot on any refactor.
|
Warning Review limit reachedNext included review available in 56 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 96 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Comment |
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs-only. Part of the LAB-2504 cross-SDK decompression-bomb audit. Depends on cachekit-core#75 — the new
[core-decompress]link targets an anchor that PR adds, so merge core first.Two findings, opposite verdicts
The default read path is sound.
ByteStorage.retrieveinrust/src/python_bindings.rsis a pass-through to cachekit-core'sextract(), which caps output atmin(512 MiB, 1000 × compressed_len)before decompressing. Worth stating because the MessagePack size caps sit on already-decompressed bytes — a reader can easily assume those caps are the protection when they are in fact downstream of it.ArrowSerializeris not.deserialize()hands the body topa.ipc.open_file(...).read_all(), which decompresses zstd with no size or ratio limit and never touchesextract(). Reproduced end-to-end through the real serializer:cachekit-core rejects at 1000:1. Neither existing control covers it: the
[8-byte xxh3][Arrow IPC]prefix is unkeyed, andmax_value_sizeis enforced on the write path only (cache_handler.py), making it a producer-side quota rather than a check on bytes coming back off the wire.Documented rather than patched — the fix is a wire-size/L1-footprint decision, filed as LAB-2730 with the PoC and the ruled-out approaches. Mitigations that work today are named in the doc.
What this PR corrects about its own first draft
The expert panel caught three errors in the initial commit:
compression=None" was false and actively harmful.deserialize()never readsself.compression; the reader decompresses according to the stored stream's ownBodyCompressionmetadata, so an attacker's forged envelope declares zstd regardless of the reader's setting. Anyone who followed that advice would have believed they were protected. Now stated explicitly as a non-mitigation.@cache.iois the CachekitIO backend preset and does not select Arrow — that needs an explicitserializer="arrow"plus the[data]extra. Wider:deserialize()also accepts rawARROW1bodies with no checksum at all via the legacy integrity-off branch, so an attacker need not even recompute the unkeyed prefix.BodyCompressionmakeslen(body)a real pre-decompression bound in three lines. It costs the compression feature, which is why it is still an owner call — but the doc now says a bound exists and what it costs, rather than implying none does. The Flatbuffers walk is only needed to keep compression.Scope
No code. The Arrow fix is LAB-2730; core's constants are LAB-2505.
Closes LAB-2504 for this repo.