From 38444354ee848d714c0b834cba78dca95b756bf6 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Wed, 2 Sep 2026 17:50:25 +1000 Subject: [PATCH 1/4] docs(security): document the ByteStorage decompression bound (LAB-2504) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The threat-model bullet claimed "size limits + ratio validation" with no numbers and no statement of reach, which left three things a reader has to rediscover from source: what the limits actually are, that original_size is attacker-controlled and deliberately not trusted, and that xxHash3-64 is unkeyed and therefore not a control against forgery at all. Also states the ceiling's blast radius honestly. 512 MiB assumes a host that can absorb a 512 MiB allocation; a Workers isolate has ~128 MiB, so a payload well inside these limits can still OOM it. Sizing is LAB-2505's call, so this records the constraint rather than changing it. Records that compression_bomb runs in the quick-fuzz matrix on every push and PR, not as an ad hoc script — the guard is only worth citing if a reader can tell it gates merges. --- SECURITY.md | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/SECURITY.md b/SECURITY.md index 48dcd83..ee62f54 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -49,7 +49,7 @@ This crate protects against: - **Data tampering**: GCM authentication tags (when encryption enabled); xxHash3 detects accidental corruption only - **Data disclosure**: AES-256-GCM encryption (when enabled) - **Key compromise isolation**: HKDF domain separation per tenant -- **Decompression bombs**: Size limits + ratio validation +- **Decompression bombs**: Size limits + ratio validation (see [Decompression limits](#decompression-limits)) - **Memory disclosure**: `zeroize` on drop for key material This crate does **not** protect against: @@ -59,6 +59,51 @@ This crate does **not** protect against: - Denial of service via resource exhaustion (partial protection only) - Attacks requiring physical access +### Decompression limits + +`StorageEnvelope::extract` bounds LZ4 decompression **before** calling +`lz4_flex::decompress`, so a forged envelope cannot expand without limit: + +| Limit | Value | Enforced on | +|:------|:------|:------------| +| `MAX_COMPRESSED_SIZE` | 512 MiB | `compressed_data.len()` | +| `MAX_UNCOMPRESSED_SIZE` | 512 MiB | declared `original_size` | +| `MAX_COMPRESSION_RATIO` | 1000:1 | `original_size` vs `compressed_data.len()` | + +The ratio check uses integer arithmetic (`checked_mul`, overflow treated as a +bomb) so a floating-point rounding bypass is not available, and zero-length +compressed data with a non-zero `original_size` is rejected outright. The +allocation is sized from `original_size` and `lz4_flex` returns +`OutputTooSmall` rather than growing past it, so the decompressed output is +bounded by `min(512 MiB, 1000 × compressed_len)` regardless of what the +envelope claims. `extract` re-checks the produced length afterwards. + +Two properties of this bound are worth stating explicitly, because both are +easy to assume and wrong: + +- **`original_size` is not trusted.** It is attacker-controlled on any + backend an attacker can write to. It sizes the allocation only *after* the + absolute and ratio checks have already constrained it. +- **xxHash3-64 is not a control here.** It is unkeyed, so anyone who can + forge an envelope recomputes it. It detects accidental corruption, not + forgery. Authentication comes from AES-256-GCM, and only for secure caches. + +**The ceiling is server-class.** 512 MiB assumes a host that can absorb a +512 MiB allocation. It does *not* prevent an out-of-memory kill in a +constrained runtime — a Cloudflare Workers isolate has ~128 MiB, so a payload +well inside these limits can still exhaust it. Deployments on constrained +runtimes must bound payload size at the caller. Making these constants +environment-aware or configurable is tracked separately (LAB-2505); do not +treat the current values as tuned for anything but a server. + +These properties are pinned by the `compression_bomb` fuzz target +(`fuzz/fuzz_targets/compression_bomb.rs`), which asserts that `extract` never +panics, never emits more than 512 MiB, and fails with `DecompressionBomb` or +`InputTooLarge`. It runs in CI on every push and pull request as part of the +`quick-fuzz` matrix in `.github/workflows/security.yml` (corpus-only, 120 s per +target), plus a weekly deep-fuzz run — it is a merge-time guard, not an ad hoc +script. + ### Dependencies Security-critical dependencies are audited via `cargo-deny`: From 91829cb304d86270fc4b4c85a830d549376335e5 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Wed, 2 Sep 2026 18:07:10 +1000 Subject: [PATCH 2/4] docs(security): correct the CI-coverage and fuzz-contract claims (LAB-2504) Expert-panel findings on the previous commit. Three claims were wrong or overstated and a security doc that overstates its own guarantees is worse than one that says nothing. CI coverage: 'runs on every push and pull request' was false for pushes -- security.yml is on: push: branches: [main], so feature-branch pushes never run quick-fuzz. Worse, fuzz/.gitignore excludes corpus/*/, so on a fresh CI checkout the corpus is empty and 'cargo fuzz run -runs=0' generates nothing: at PR time the job proves the target BUILDS and exercises no input. The real input coverage is the weekly deep-fuzz run plus the unit tests and Kani proofs, so those are what the section now points at. The 120 s figure is gone; it was a timeout, not a measure of coverage. Fuzz-target contract: the target also accepts DecompressionFailed, and its compressed_size is a u16 -- so it caps compressed input at 64 KiB and cannot reach the 512 MiB boundary the old text cited. It exercises the ratio bound, not the absolute one. 'original_size is not trusted' was too strong and papered over the interesting part. It is not trusted as a BOUND, but it does size the allocation within that bound, so a forged envelope can still make a reader allocate up to 1000x its wire size before the LZ4 stream is validated -- an eager memory.grow on wasm32 needing no valid stream behind it. That is the LAB-2505 sizing question, now named as such instead of hidden behind a reassuring phrase. --- SECURITY.md | 56 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index ee62f54..d356cbc 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -70,20 +70,20 @@ This crate does **not** protect against: | `MAX_UNCOMPRESSED_SIZE` | 512 MiB | declared `original_size` | | `MAX_COMPRESSION_RATIO` | 1000:1 | `original_size` vs `compressed_data.len()` | -The ratio check uses integer arithmetic (`checked_mul`, overflow treated as a -bomb) so a floating-point rounding bypass is not available, and zero-length -compressed data with a non-zero `original_size` is rejected outright. The -allocation is sized from `original_size` and `lz4_flex` returns -`OutputTooSmall` rather than growing past it, so the decompressed output is -bounded by `min(512 MiB, 1000 × compressed_len)` regardless of what the -envelope claims. `extract` re-checks the produced length afterwards. - -Two properties of this bound are worth stating explicitly, because both are -easy to assume and wrong: - -- **`original_size` is not trusted.** It is attacker-controlled on any - backend an attacker can write to. It sizes the allocation only *after* the - absolute and ratio checks have already constrained it. +The ratio product is computed in `u64` via `checked_mul` (overflow is treated +as a bomb), and zero-length compressed data with a non-zero `original_size` is +rejected outright. `lz4_flex` returns `OutputTooSmall` rather than growing past +the allocation, so the decompressed output is bounded by +`min(512 MiB, 1000 × compressed_len)` regardless of what the envelope claims, +and `extract` re-checks the produced length afterwards — a decompressor's +size argument sizes a buffer, it never asserts the decoded length. + +- **`original_size` does not act as a bound.** It is attacker-controlled on any + backend an attacker can write to. It *does* size the allocation, but only + within the absolute and ratio limits already checked above — so a forged + envelope can still make a reader allocate up to `1000 ×` its wire size before + the LZ4 stream is validated. That allocation amplification is the sizing + question in LAB-2505, not a bypass of the bound. - **xxHash3-64 is not a control here.** It is unkeyed, so anyone who can forge an envelope recomputes it. It detects accidental corruption, not forgery. Authentication comes from AES-256-GCM, and only for secure caches. @@ -91,18 +91,26 @@ easy to assume and wrong: **The ceiling is server-class.** 512 MiB assumes a host that can absorb a 512 MiB allocation. It does *not* prevent an out-of-memory kill in a constrained runtime — a Cloudflare Workers isolate has ~128 MiB, so a payload -well inside these limits can still exhaust it. Deployments on constrained -runtimes must bound payload size at the caller. Making these constants -environment-aware or configurable is tracked separately (LAB-2505); do not -treat the current values as tuned for anything but a server. +well inside these limits can still exhaust it, and on `wasm32` the allocation +is an eager `memory.grow` that needs no valid LZ4 stream behind it. Deployments +on constrained runtimes must bound payload size at the caller. Making these +constants environment-aware or configurable is tracked in LAB-2505. -These properties are pinned by the `compression_bomb` fuzz target +These properties are exercised by the `compression_bomb` fuzz target (`fuzz/fuzz_targets/compression_bomb.rs`), which asserts that `extract` never -panics, never emits more than 512 MiB, and fails with `DecompressionBomb` or -`InputTooLarge`. It runs in CI on every push and pull request as part of the -`quick-fuzz` matrix in `.github/workflows/security.yml` (corpus-only, 120 s per -target), plus a weekly deep-fuzz run — it is a merge-time guard, not an ad hoc -script. +panics and never emits more than 512 MiB, and that rejections are one of +`DecompressionBomb`, `InputTooLarge`, or `DecompressionFailed`. Its +`compressed_size` is a `u16`, so it caps compressed input at 64 KiB and +therefore exercises the **ratio** bound, not the 512 MiB absolute one. + +Be precise about how much CI coverage that buys: the `quick-fuzz` matrix in +`.github/workflows/security.yml` runs on pull requests and on pushes to `main` +(not on feature-branch pushes), and `fuzz/.gitignore` excludes `corpus/*/`, so +on a fresh checkout the corpus is empty and `cargo fuzz run … -runs=0` +generates no inputs. At PR time the job therefore proves the target still +**builds**; the input coverage comes from the weekly deep-fuzz run, and from +the unit tests and Kani proofs in `src/byte_storage.rs`, which do assert the +bound directly. ### Dependencies From a5f051a7a46a0173ca59c781895a478eec6d707d Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Wed, 2 Sep 2026 20:20:20 +1000 Subject: [PATCH 3/4] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20correct=20four=20overstated=20claims=20in=20the=20d?= =?UTF-8?q?ecompression-limits=20section?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four CodeRabbit findings verified against source and applied; an expert-panel pass on the result found five more of the same class, also applied. Corrected here: - zero-length compressed data is rejected unconditionally, not only when original_size is non-zero (byte_storage.rs:122 has no original_size term) - the compression_bomb target reaches MAX_UNCOMPRESSED_SIZE's rejection branch (u32 original_size) but never MAX_COMPRESSED_SIZE (u16 compressed_size) - its 512 MiB output assertion is vacuous for the same reason, and its error-variant assertions are guarded, not universal - -runs=0 executes libFuzzer's newline seed before the run-limit check; it does no mutation rather than no execution - the Kani harnesses are tautologies (assert_eq!(P, P)) and run only on schedule/dispatch, so they are neither verification of the bound nor a merge-time gate Also stated: deep-fuzz does not persist its corpus, validate() reaches the full extract allocation, the pre-deserialization bound lives in ByteStorage rather than on the public StorageEnvelope, the OutputTooSmall bound depends on lz4_flex's default safe-decode, and wasm32 linear memory never shrinks. CodeRabbit-Resolved: SECURITY.md:75:Document the unconditional zero-length rej CodeRabbit-Resolved: SECURITY.md:104:Correct the fuzz coverage boundary claim CodeRabbit-Resolved: SECURITY.md:110:Describe the -runs=0 coverage accurately CodeRabbit-Resolved: SECURITY.md:113:Qualify the Kani coverage statement --- SECURITY.md | 99 ++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 79 insertions(+), 20 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index d356cbc..9970685 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -70,20 +70,38 @@ This crate does **not** protect against: | `MAX_UNCOMPRESSED_SIZE` | 512 MiB | declared `original_size` | | `MAX_COMPRESSION_RATIO` | 1000:1 | `original_size` vs `compressed_data.len()` | +The same `MAX_COMPRESSED_SIZE` constant also bounds the *serialized* envelope +before MessagePack deserialization — but that check lives in +`ByteStorage::retrieve` and `ByteStorage::validate`, not on `StorageEnvelope`. +`StorageEnvelope` is public with public fields, so a caller who deserializes it +directly gets no such bound and must impose one. + The ratio product is computed in `u64` via `checked_mul` (overflow is treated -as a bomb), and zero-length compressed data with a non-zero `original_size` is -rejected outright. `lz4_flex` returns `OutputTooSmall` rather than growing past -the allocation, so the decompressed output is bounded by -`min(512 MiB, 1000 × compressed_len)` regardless of what the envelope claims, +as a bomb — though the 512 MiB compressed-size check above already puts the +product near 2^39, so that branch is belt-and-braces rather than a live +defense), and zero-length compressed data is rejected outright regardless of +what `original_size` claims — the check is unconditional, so an envelope +declaring `original_size == 0` is rejected on the same branch rather than +decompressing to an empty result. `lz4_flex` returns `OutputTooSmall` rather +than growing past the allocation, so the decompressed output is bounded by +`min(512 MiB, 1000 × compressed_data.len())` regardless of what the envelope +claims — a property of `lz4_flex`'s default `safe-decode` path, which this +crate must not opt out of (`default-features = false` moves bounds enforcement +into the separate `checked-decode` feature and swaps the fixed-length buffer +for `with_capacity` + `set_len`), and `extract` re-checks the produced length afterwards — a decompressor's size argument sizes a buffer, it never asserts the decoded length. - **`original_size` does not act as a bound.** It is attacker-controlled on any backend an attacker can write to. It *does* size the allocation, but only within the absolute and ratio limits already checked above — so a forged - envelope can still make a reader allocate up to `1000 ×` its wire size before - the LZ4 stream is validated. That allocation amplification is the sizing - question in LAB-2505, not a bypass of the bound. + envelope can still make a reader allocate up to `1000 × compressed_data.len()` + before the LZ4 stream is validated. That allocation amplification is the + sizing question in LAB-2505, not a bypass of the bound. Note that + `ByteStorage::validate()` reaches the same allocation — it calls `extract()` + and discards the result — so despite its name and its "validate envelope + without extracting data" doc comment it is *not* a cheap structural + pre-screen for untrusted envelopes. - **xxHash3-64 is not a control here.** It is unkeyed, so anyone who can forge an envelope recomputes it. It detects accidental corruption, not forgery. Authentication comes from AES-256-GCM, and only for secure caches. @@ -92,25 +110,66 @@ size argument sizes a buffer, it never asserts the decoded length. 512 MiB allocation. It does *not* prevent an out-of-memory kill in a constrained runtime — a Cloudflare Workers isolate has ~128 MiB, so a payload well inside these limits can still exhaust it, and on `wasm32` the allocation -is an eager `memory.grow` that needs no valid LZ4 stream behind it. Deployments -on constrained runtimes must bound payload size at the caller. Making these -constants environment-aware or configurable is tracked in LAB-2505. +is an eager `memory.grow` that needs no valid LZ4 stream behind it. On `wasm32` +that is worse than a spike: linear memory never shrinks, so a single large +extract permanently raises the isolate's floor for every subsequent request it +serves. Deployments on constrained runtimes must bound payload size at the +caller. Making these constants environment-aware or configurable is tracked in +LAB-2505. These properties are exercised by the `compression_bomb` fuzz target -(`fuzz/fuzz_targets/compression_bomb.rs`), which asserts that `extract` never -panics and never emits more than 512 MiB, and that rejections are one of -`DecompressionBomb`, `InputTooLarge`, or `DecompressionFailed`. Its -`compressed_size` is a `u16`, so it caps compressed input at 64 KiB and -therefore exercises the **ratio** bound, not the 512 MiB absolute one. +(`fuzz/fuzz_targets/compression_bomb.rs`) — but read its assertions before +crediting them: + +- It asserts `extract` never panics. That one is unconditional and real. +- It asserts the output never exceeds 512 MiB. Vacuous in this target: with + `compressed_size` a `u16`, compressed input caps at 64 KiB, so the ratio + bound already holds output under ~62.5 MiB. The assertion cannot fire. +- It asserts rejections are one of `DecompressionBomb`, `InputTooLarge`, or + `DecompressionFailed` — but **only** for inputs that already violate the + declared-size or ratio limit. Both variant checks sit behind guards. Every + other rejection is unconstrained, and `extract`'s two remaining failure + variants, `ChecksumMismatch` and `SizeValidationFailed`, are never asserted + against at all. + +On which bounds it reaches: `compressed_size` is a `u16`, so the 512 MiB +`MAX_COMPRESSED_SIZE` boundary is never approached. `original_size` is a `u32`, +which does range past 512 MiB, so the target does reach `MAX_UNCOMPRESSED_SIZE`'s +*rejection* branch. It exercises the **ratio** bound and that rejection path — +not the compressed-size limit, and not the output bound. Be precise about how much CI coverage that buys: the `quick-fuzz` matrix in `.github/workflows/security.yml` runs on pull requests and on pushes to `main` (not on feature-branch pushes), and `fuzz/.gitignore` excludes `corpus/*/`, so -on a fresh checkout the corpus is empty and `cargo fuzz run … -runs=0` -generates no inputs. At PR time the job therefore proves the target still -**builds**; the input coverage comes from the weekly deep-fuzz run, and from -the unit tests and Kani proofs in `src/byte_storage.rs`, which do assert the -bound directly. +on a fresh checkout the corpus is empty. libFuzzer seeds an empty corpus with a +single newline input and executes it before the run-limit check, so `-runs=0` +is not literally zero executions — but it performs no mutation, so the job +generates no inputs of its own. At PR time it therefore proves little beyond +the target still **building**. + +Input coverage comes from the weekly deep-fuzz run — though that job does not +persist its corpus either (it uploads `fuzz/artifacts/` only, and the cache key +covers `fuzz/target/`), so each week restarts cold from the same seed and +coverage does not accumulate — and from the unit tests in `src/byte_storage.rs`, +which call `extract` directly. Those unit tests are the only thing in this repo +that executes the bound at merge time. + +**The Kani proofs are weaker than they look, and are not a merge-time gate.** +The `kani` job runs only on `schedule` and `workflow_dispatch`, never on a pull +request. More importantly, three of the four size/ratio harnesses assign the +same predicate to two bindings and assert the two are equal — for example +`let exceeds_limit = size > MAX_UNCOMPRESSED_SIZE; let should_reject = size > +MAX_UNCOMPRESSED_SIZE; assert_eq!(exceeds_limit, should_reject);`. That is a +tautology: it holds for any predicate, and would still pass if the comparison +were inverted or the constant were wrong. `verify_decompression_bomb_protection` +additionally assumes `compressed_size <= 1000`, which makes `checked_mul` +infallible and leaves its overflow branch unreachable and stubbed `assert!(true)`. + +What Kani does buy is its default check set — no panic, no arithmetic overflow — +over the harness bodies. What it does not buy is any evidence that the predicates +are the *right* ones, and it never executes `StorageEnvelope::extract`, so it +cannot catch a divergence between the modelled predicate and the shipped one. +Treat these as smoke checks, not as verification of the bound. ### Dependencies From 39ae394c995996bf3c6d8742f4dcd8e0f7e2e3ff Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Wed, 2 Sep 2026 20:54:24 +1000 Subject: [PATCH 4/4] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20sharpen=20the=20quick-fuzz=20and=20Kani=20descripti?= =?UTF-8?q?ons?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from CodeRabbit's re-review of a5f051a, both correct. quick-fuzz: "proves little beyond building" undersold it. The seed does execute, so the job is single-seed smoke coverage — it catches a build break or a panic on that one input. Says so, and points at the unit tests as the merge-time boundary coverage. Kani: my previous text said three of four harnesses assign the same predicate to two bindings. Only two do that literally. The other two are equally vacuous by different routes — checked_mul compared against the same product under an assume that makes overflow impossible, and a branch that restates its own definition. All four are now described by their actual pattern rather than lumped under one. CodeRabbit-Resolved: SECURITY.md:148:Describe the pull-request fuzz job as sin CodeRabbit-Resolved: SECURITY.md:164:Correct the Kani harness count and patter --- SECURITY.md | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 9970685..a1ca487 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -144,8 +144,10 @@ Be precise about how much CI coverage that buys: the `quick-fuzz` matrix in on a fresh checkout the corpus is empty. libFuzzer seeds an empty corpus with a single newline input and executes it before the run-limit check, so `-runs=0` is not literally zero executions — but it performs no mutation, so the job -generates no inputs of its own. At PR time it therefore proves little beyond -the target still **building**. +generates no inputs of its own. At PR time it is therefore **single-seed smoke +coverage**: it will catch a build break or a panic on that one newline input, +and nothing else. It produces no boundary coverage; the merge-time coverage for +the boundary cases is the unit tests below. Input coverage comes from the weekly deep-fuzz run — though that job does not persist its corpus either (it uploads `fuzz/artifacts/` only, and the cache key @@ -156,14 +158,22 @@ that executes the bound at merge time. **The Kani proofs are weaker than they look, and are not a merge-time gate.** The `kani` job runs only on `schedule` and `workflow_dispatch`, never on a pull -request. More importantly, three of the four size/ratio harnesses assign the -same predicate to two bindings and assert the two are equal — for example -`let exceeds_limit = size > MAX_UNCOMPRESSED_SIZE; let should_reject = size > -MAX_UNCOMPRESSED_SIZE; assert_eq!(exceeds_limit, should_reject);`. That is a -tautology: it holds for any predicate, and would still pass if the comparison -were inverted or the constant were wrong. `verify_decompression_bomb_protection` -additionally assumes `compressed_size <= 1000`, which makes `checked_mul` -infallible and leaves its overflow branch unreachable and stubbed `assert!(true)`. +request. More importantly, none of the four size/ratio harnesses can fail on a +wrong predicate — though they get there by two different routes: + +- `verify_input_size_limits` and `verify_compressed_size_limits` are literal + self-comparisons: `let exceeds_limit = size > MAX_UNCOMPRESSED_SIZE; let + should_reject = size > MAX_UNCOMPRESSED_SIZE; assert_eq!(exceeds_limit, + should_reject);`. That holds for any predicate, and would still pass with the + comparison inverted or the constant wrong. +- `verify_decompression_bomb_protection` compares the `checked_mul` result + against the same product computed directly — equal by construction, and its + `kani::assume(compressed_size <= 1000)` makes the multiplication infallible, + so the overflow branch it exists to check is unreachable and stubbed + `assert!(true)`. +- `verify_compression_ratio_calculation_safety` derives `is_bomb = original_size + > max_allowed` and then asserts that same comparison in both branches of + `if original_size <= max_allowed`, which restates its own definition. What Kani does buy is its default check set — no panic, no arithmetic overflow — over the harness bodies. What it does not buy is any evidence that the predicates