fix: bound stream signature length - #228
Conversation
The streaming Unsealer read a length prefix from the input and used it to size a buffer allocation before any validation. Add a MAX_SIG_SIZE bound (8 KiB) in consts.rs and reject oversized values with Error::ConstraintViolation in both the native and web streaming unsealers, mirroring the existing MAX_HEADER_SIZE check in preamble_checked. Adds a regression test that an oversized length prefix is rejected cleanly instead of driving an allocation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Rules + Review gatekeeper — cycle 1
The security bound itself is correct and well-tested: the MAX_SIG_SIZE (8 KiB) check lands before both allocations, mirrors the existing MAX_HEADER_SIZE check, returns the right error, and the regression test exercises the rejection path cleanly. Full native suite (54 tests) + wasm check + cargo fmt are green, and the happy path stays covered by the existing stream round-trip tests. PR title/body, commit, branch name, and issue #227 (masked advisory link, redacted comments) are all appropriately generic.
One item to address before sign-off, plus a nit:
1. Embargo hygiene (blocking). This is a public repo and the driving advisory is still in draft. A few comments — the two fix comments, the doc comment on the new constant (which gets published in the crate docs), and the regression-test comment — describe the issue's mechanism/impact more specifically than the (correctly neutral) PR body does. While the embargo holds, please genericize those to a neutral rationale, e.g. "bound the length prefix to a sane maximum before allocation, mirroring MAX_HEADER_SIZE", and restore the full detail once the advisory is published. Keep the code fix and test logic byte-identical. Inline notes mark each spot.
2. Nit (non-blocking). web/stream.rs uses Err(Error::ConstraintViolation.into()) where the native side uses a bare Err(Error::ConstraintViolation); since new() returns Result<Self, Error> the .into() is an identity conversion — drop it for consistency.
Keeping the PR in draft while the advisory is embargoed (not flipping to ready). Routing back for a quick fix pass.
Reword the MAX_SIG_SIZE doc comment, the two bound-check comments, and the regression-test comment to neutral rationale (mirrors MAX_HEADER_SIZE) while the advisory is still in draft; code is unchanged. Also drop a redundant identity .into() on the web unsealer error path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Rules + Review gatekeeper — cycle 2
Clean. Both cycle-1 items are resolved:
1. Embargo hygiene (was blocking) — fixed. The two bound-check comments, the MAX_SIG_SIZE doc comment, and the regression-test comment were all genericized to neutral rationale ("bound the length prefix to a sane maximum before allocation, mirroring MAX_HEADER_SIZE") in commit 8310ba96, with the code/test logic left byte-identical. A tree-wide sweep confirms no draft-advisory id or vulnerability detail leaks into any public artifact: PR title/body, both commit messages, branch name, and issue #227 (masked advisory link, redacted comments) are all appropriately generic.
2. Nit — fixed. The redundant identity .into() on the web unsealer error path was dropped; it now matches the native side (return Err(Error::ConstraintViolation)).
The security bound itself remains correct and well-tested: the check lands before both allocations in rust/stream.rs and web/stream.rs, mirrors the existing MAX_HEADER_SIZE guard, returns the right error, and the regression test exercises the rejection path cleanly while the happy path stays covered by the existing round-trip tests. Native suite (54 tests) + wasm check + cargo fmt are green per the PR notes.
Staying in draft until the associated advisory is published — this is a public repo and the driving advisory is still embargoed, so I'm not flipping to ready-for-review even though the gate is clean. Signing off; no further changes needed.
Summary
The streaming
Unsealerread a length prefix from the input stream and used it directly to size a buffer allocation before the corresponding data was validated. This adds an explicit upper bound so a malformed/oversized length prefix is rejected cleanly instead of driving an allocation.Changes
MAX_SIG_SIZE(8 KiB) constant inpg-core/src/consts.rs. A serialized header signature holds a single signature plus its policy and fits comfortably in a few KiB.MAX_SIG_SIZEwithError::ConstraintViolationin both streaming unsealers:pg-core/src/client/rust/stream.rs(native)pg-core/src/client/web/stream.rs(wasm)MAX_HEADER_SIZEcheck inpreamble_checked.test_stream_unseal_rejects_oversized_sig_len) confirming an oversized length prefix is rejected withConstraintViolationrather than allocating.Testing
cargo test -p pg-core --features "test,rust,stream"— all 54 tests pass (incl. the new regression) + doc-tests.cargo fmt -p pg-core -- --check— clean.cargo check -p pg-core --no-default-features --features "web,stream" --target wasm32-unknown-unknown— clean.Closes #227.
Kept generic while the associated security advisory is embargoed.