fix(core): wire-varint cross-checks (closes #58 + #59) - #75
Merged
Conversation
Closes #58. `PakEntryHeader::read_encoded` reads `compressed_size` from a (potentially u64-width) wire varint and stores it on the parsed header — but the multi-block path never cross-checks it against the actual sum of per-block sizes. The cursor walk uses per-block u32 sizes for boundaries; `compressed_size` is structurally orphaned. An attacker can craft `compressed_size = u64::MAX - 1` (via bit-29 cleared on the wire to widen the varint) while the per-block sizes sum to a few KiB, and the lie propagates to `PakEntryHeader::compressed_size()` and any downstream consumer reporting it (the CLI `list` command, future JSON/GUI surfaces). Fix: in `read_encoded`'s multi-block path, accumulate the unaligned per-block size sum during the cursor walk, then assert it equals the wire `compressed_size` claim. New variant `IndexParseFault::EncodedCompressedSizeMismatch { claimed, computed, path }` carries both numbers (so operator-visible errors show the divergence) and `path: Option<String>` so the FDI walk's existing `with_index_path` enrichment fills in the virtual path. The single-block trivial path is structurally exempt — its block is constructed FROM `compressed_size`, so they agree by construction. Two regression tests: - `read_encoded_rejects_compressed_size_block_sum_mismatch` — pins the new check fires on the documented attack shape. - `read_encoded_single_block_compressed_size_unchecked` — pins that the single-block path keeps accepting valid entries (i.e. the new check doesn't accidentally generalize too far). Adds `#[allow(clippy::too_many_lines)]` to `read_encoded` (was implicit on the function before; now explicit because the new check pushed it past the 100-line threshold). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-1 silent-failure-hunter on PR #75 surfaced four sibling silent-failure shapes adjacent to the original #58 fix. After user sign-off on scope expansion, this commit addresses three of them (closes #58 and #59 together): 1. **uncompressed_size unchecked** (issue #58 sibling): added a `BoundsExceeded { field: "uncompressed_size", limit: block_count * compression_block_size }` check after the multi-block walk. Without it, a malicious archive could claim `uncompressed_size = MAX_UNCOMPRESSED_ENTRY_BYTES` (8 GiB) on a payload that fits in 4 KiB, and consumers reading the wire field (CLI list, JSON output, alloc estimators) would see the lie. 2. **block_count == 0 with compression_method != None** (closes issue #59): `read_encoded` now rejects this structurally nonsensical shape with `InvariantViolated`. The pre-existing `read_encoded_zero_blocks_with_compression_slot` test was actively pinning the silent-accept; renamed to `read_encoded_rejects_zero_blocks_with_compression_slot` and inverted to assert rejection. New companion `read_encoded_zero_blocks_no_compression` pins that the legitimate (uncompressed) zero-block shape still parses. 3. **`offset + compressed_size > file_size` not checked at open time**: added an open-time iteration in `PakReader::open` that walks all index entries and rejects any whose claim implies on-disk bytes past EOF. This covers single-block + multi-block + zero-block uniformly and closes the lie-leak via consumers that surface header fields without extracting (CLI list, JSON). Pre-existing per-entry-read check at `open_entry_into` stays as defense-in-depth. Plus the round-1 code-reviewer's nits: - Display-pin tests for `EncodedCompressedSizeMismatch` (Some + None branches), matching the per-variant pinning convention in error.rs. - Fixed comment arithmetic: `65 535 * u32::MAX ≈ 256 TiB`, not 280 GiB (off by ~1000×). Fixed in two places. - Renamed test `read_encoded_single_block_compressed_size_unchecked` → `read_encoded_single_block_path_skips_cross_check` for accuracy ("unchecked" implied a missing check; "skips_cross_check" is what actually happens — the single-block path's block IS compressed_size by construction). - Added an AES-alignment caveat comment near the unaligned-sum reasoning, noting verification is against repak only (no first-party UE encrypted v10/v11 fixtures yet). Per project memory on empirical wire-format verification. Updated existing integration test `read_entry_rejects_index_offset_past_eof` → `open_rejects_index_offset_past_eof`: rejection moves from `read_entry`-time to `open`-time per the new check. Tests: 267 → 271 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
r6e
changed the title
Round-2 reviewer findings on PR #75: **Code-reviewer (Important):** - Hoisted `uncompressed_size <= block_count * compression_block_size` cap from the multi-block branch to apply to the single-block trivial path too. The lie shape is the same on both paths; prior scoping missed the single-block case. - Guarded the parse-time cap with `compression_method != None` to avoid false-positives on encrypted-uncompressed entries (which typically write `compression_block_size = 0` and would otherwise reject any non-zero `uncompressed_size`). - New boundary tests: - `read_encoded_accepts_uncompressed_size_at_block_capacity` pins `uncompressed_size == cap` accepts (cap uses `>`, not `>=`; a regression flipping the operator would silently reject valid full-block entries). - `read_encoded_skips_uncompressed_cap_for_uncompressed_method` pins the false-positive guard. **Silent-failure-hunter (HIGH + LOW):** - Added `MAX_UNCOMPRESSED_ENTRY_BYTES` (8 GiB) check to the open-time loop in `PakReader::open` (Finding A option b). Closes the structural weakness from the deferred `compression_block_size` cap: an attacker-controlled `compression_block_size = u32::MAX` lifts the parse-time cap to ~256 TiB, but the open-time backstop bounds it at the same ceiling `verify_entry`/`read_entry` already use. Also closes Finding D (pre-existing inline v3-9 path's `uncompressed_size` was unchecked at parse time) — the open-time iteration covers both inline and encoded uniformly. - Added `tracing::warn!` calls before each open-time return (Finding C), matching the warn-then-return pattern at the surrounding rejection sites. Updated existing integration tests: - `read_entry_rejects_oversized_uncompressed_size` → `open_rejects_oversized_uncompressed_size`: cap now fires at open, not at read. - `cap_uncompressed_size_boundary_text_couples_both_sides`: MAX+1 trips at open (was at read); MAX boundary may trip open's payload-end-vs-file_size check (synthetic pak's uncompressed claim vastly exceeds payload region) but must not contain the cap text. Test handles both Open-fails and Open-succeeds-Read-fails paths. Tests: 271 → 273 passing. Deferred (small follow-ups, not blocking): - silent-failure-hunter Finding B: `InvariantViolated` for #59 carries no path or context. Would require introducing a typed variant or extending `InvariantViolated` with `Option<String>`. Diagnostic-quality, not silent failure. - code-reviewer suggestion: `OverflowSite::EncodedBlockEnd` reused at two distinct sites. Diagnostic precision, not correctness. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-3 silent-failure-hunter polish for PR #75 (both LOW, both non-blocking but trivial enough to fold in): - Add `warn!` calls to the read-time `MAX_UNCOMPRESSED_ENTRY_BYTES` backstops in `stream_entry_to` and `read_entry`, mirroring the warn-then-return pattern just added at the open-time check. These backstops are mostly unreachable for `PakReader::open`-loaded archives (open-time loop already filtered them) but remain reachable as defense-in-depth and should log consistently. - Document the cross-module invariant in the encoded uncompressed cap's skip-when-None comment: the skip relies on `PakReader::open` enforcing `MAX_UNCOMPRESSED_ENTRY_BYTES` on every entry. A future contributor adding a code path that constructs `PakEntryHeader` outside of `open` must add the backstop at the call site or route through `open`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Closes #58 and #59 (security/bug bundle).
Background
PR scope was expanded after silent-failure-hunter round-1 surfaced four sibling silent-failure shapes adjacent to the originally-tracked #58 fix. Per user direction, this PR now addresses the fixable subset (#58 itself + sibling
uncompressed_sizelie + open-time payload-end-vs-EOF lie + #59) in one bundle. Two further hardening items (compression_block_sizeupper bound,is_encrypted/compression_methodtyped errors) are deferred to follow-up issues — they're independent and less critical.Changes
crates/paksmith-core/src/container/pak/index/entry_header.rs—read_encoded:block_count == 0 && compression_method != None(closes bug(core): block_count==0 with compression_method!=None accepted silently; no short-write guard in stream_zlib_to #59) — structurally nonsensical: no blocks back thecompressed_sizeclaim.compressed_size == sum(unaligned per-block sizes)after the multi-block walk (bug(core): wire-format compressed_size never cross-checked against sum of per-block sizes #58 original).uncompressed_size <= block_count * compression_block_size(bug(core): wire-format compressed_size never cross-checked against sum of per-block sizes #58 sibling).build_partial_entry, but no first-party UE encrypted v10/v11 fixtures exist to confirm UE writes the same.65 535 * u32::MAX ≈ 256 TiB, was280 GiB).crates/paksmith-core/src/container/pak/mod.rs—PakReader::open:offset + compressed_size > file_size. Covers single-block, multi-block, and zero-block paths uniformly through one site. Catches lies that pre-bug(core): wire-format compressed_size never cross-checked against sum of per-block sizes #58 would only surface atread_entrytime, leaking topaksmith list-style consumers that never extract.crates/paksmith-core/src/error.rs— newIndexParseFault::EncodedCompressedSizeMismatch { claimed, computed, path: Option<String> }variant. Display arm + addition toset_path_if_unset's enriching arm (so PR #74's FDI-walkwith_index_pathpopulates path automatically).Tests (267 → 271):
read_encoded_rejects_compressed_size_block_sum_mismatch— pins bug(core): wire-format compressed_size never cross-checked against sum of per-block sizes #58's primary fix.read_encoded_rejects_uncompressed_size_exceeding_block_capacity— pins the bug(core): wire-format compressed_size never cross-checked against sum of per-block sizes #58 sibling.read_encoded_rejects_zero_blocks_with_compression_slot(renamed/inverted fromread_encoded_zero_blocks_with_compression_slot) — pins bug(core): block_count==0 with compression_method!=None accepted silently; no short-write guard in stream_zlib_to #59 rejection.read_encoded_zero_blocks_no_compression— pins the legitimate uncompressed-zero-block shape still parses.read_encoded_single_block_path_skips_cross_check(renamed for accuracy) — pins single-block exemption.EncodedCompressedSizeMismatch(Some + None branches).open_rejects_index_offset_past_eof(renamed fromread_entry_rejects_...) — rejection moves from read-time to open-time.Reviewer-deferred items (filed as follow-ups recommended)
compression_block_sizeupper bound: the 5-bit wire field can be0x3f(sentinel) + a u32 escape, which lets an attacker writeu32::MAXand amplify into mis-categorizedDecompressionerrors at read time. Independent fix.is_encrypted/compression_methodtyped errors: theUnknownslot resolution currently conflates "out-of-range slot" with "slot at index N is None". Independent typed-error refinement.Test plan
cargo test --workspace --all-features— 271 passedcargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— clean🤖 Generated with Claude Code