Skip to content

fix(pg-core,security): bounds-check malformed ciphertext input#150

Open
dobby-coder[bot] wants to merge 3 commits intomainfrom
fix/pg-core-panic-on-malformed
Open

fix(pg-core,security): bounds-check malformed ciphertext input#150
dobby-coder[bot] wants to merge 3 commits intomainfrom
fix/pg-core-panic-on-malformed

Conversation

@dobby-coder
Copy link
Copy Markdown
Contributor

@dobby-coder dobby-coder Bot commented Apr 23, 2026

Summary

Addresses the three findings in #138. Several code paths in pg-core would panic on truncated or attacker-crafted ciphertext. This PR:

  • In-memory unsealer (client/rust/mod.rs, client/web/mod.rs): replaced unchecked slice::split_at with a new util::try_split_at helper that returns Error::FormatViolation when mid > b.len().
  • Stream unsealer extract_policy (client/rust/stream.rs, client/web/stream.rs): added explicit buf.len() < POL_SIZE_SIZE and buf.len() < POL_SIZE_SIZE + pol_len checks, plus checked_add for the policy-length addition so a crafted pol_len cannot overflow.
  • Stream unsealer segment verification: replaced debug_assert!(seg.len() > SIG_BYTES) with runtime Err returns, so release builds no longer wrap/underflow when a decrypted segment is too short for its trailing signature. Applied in both Rust (verify_segment) and WASM (two sites around the final_plain/per-segment signature checks).

Tests

Three new unit tests exercise the in-memory unsealer against malformed input:

  • test_unseal_rejects_empty_input — empty slice must not panic.
  • test_unseal_rejects_truncated_after_preamble — a valid preamble + 1 byte must return FormatViolation, not panic.
  • test_unseal_rejects_garbage_input — 1 KiB of zeros must return some Err, not panic.

```
$ cargo test -p pg-core --features test,stream,rust
test result: ok. 34 passed; 0 failed; 0 ignored
```

Notes

  • The three new bounds checks on the stream unsealer exercise paths only reachable after an AEAD decryption succeeds — i.e. an attacker who holds a valid USK and has crafted plaintext with a malformed policy/segment header. Still worth being defensive: the Rust type system guarantees no panic now.
  • WASM compilation was not tested from this worktree (pre-existing Error::FuturesIO issue on the wasm32-unknown-unknown target unrelated to this change).
  • Did not touch util.rs::preamble_checked — it already bounds-checks correctly.

Reviewer quickstart

```
git fetch origin && git checkout fix/pg-core-panic-on-malformed && cargo test -p pg-core --features test,stream,rust
```

Closes #138

Several code paths in pg-core would panic on truncated or attacker-
crafted ciphertext. Replaced unchecked slice::split_at calls with a new
try_split_at helper that returns FormatViolation, added explicit length
checks in extract_policy and verify_segment (both Rust and WASM
variants), and replaced debug_assert! on seg.len() > SIG_BYTES with
runtime Err returns so release builds reject truncated segments
cleanly.

Added 3 unit tests covering empty input, truncated-after-preamble, and
garbage input. All 34 pg-core tests pass.

Closes #138
@rubenhensen
Copy link
Copy Markdown
Contributor

/dobby add some tests.

@dobby-coder
Copy link
Copy Markdown
Contributor Author

dobby-coder Bot commented Apr 24, 2026

Dobby is on it, sir! Dobby sees Master Ruben wants more tests for the malformed ciphertext fixes. Dobby will pull the branch, study the existing tests, and add tests covering the stream unsealer bounds checks too. Dobby will report back very soon!

Adds unit tests for the try_split_at helper and preamble_checked, plus
more Unsealer::new boundary cases on both the in-memory and stream paths:
each truncation point (preamble, header, sig length, sig bytes) surfaces
the expected FormatViolation/NotPostGuard instead of panicking.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@dobby-coder
Copy link
Copy Markdown
Contributor Author

dobby-coder Bot commented Apr 24, 2026

Pushed a new commit with expanded test coverage (67aa349..864d1b5):

pg-core/src/util.rs — new tests module:

  • try_split_at_* — 5 tests covering exact boundary, middle split, zero-length split, out-of-bounds (asserts FormatViolation carries label + requested length), and empty-slice-with-nonzero-mid.
  • preamble_checked_rejects_short_input, preamble_checked_rejects_wrong_prelude — lock in the two early-return paths.

pg-core/src/client/rust/mod.rs — four new boundary tests for the in-memory unsealer, one per try_split_at call site:

  • test_unseal_rejects_input_shorter_than_preamble — preamble split
  • test_unseal_rejects_truncated_inside_header — header split
  • test_unseal_rejects_truncated_before_sig_lenSIG_SIZE_SIZE split (uses preamble_checked to locate the exact cut)
  • test_unseal_rejects_truncated_inside_sig_bytes — header-signature split
  • test_unseal_rejects_wrong_prelude — flipped prelude byte must surface as NotPostGuard

pg-core/src/client/rust/stream.rs — four new async tests for the stream unsealer:

  • test_stream_unseal_rejects_empty_input, test_stream_unseal_rejects_truncated_preamble, test_stream_unseal_rejects_garbage_input, test_stream_unseal_rejects_flipped_prelude

Each test asserts the specific error variant/message it expects (not just is_err), so a regression that swallowed the wrong error or panicked anywhere under the unsealer would fail the suite.

$ cargo test -p pg-core --features test,stream,rust
test result: ok. 50 passed; 0 failed; 0 ignored

Note: I did not add direct tests for the extract_policy / segment-length checks in stream.rs because those paths only run after an AEAD decryption succeeds — exercising them needs either (a) a crafted USK + plaintext, or (b) refactoring the inner fns out of unseal. Happy to do the refactor-and-test pass as a follow-up if you want that covered too.

@rubenhensen
Copy link
Copy Markdown
Contributor

/dobby make sure you pass the ci checks.

@dobby-coder
Copy link
Copy Markdown
Contributor Author

dobby-coder Bot commented Apr 24, 2026

Dobby hears Master rubenhensen! Dobby will make sure the CI checks pass on PR #150! Dobby is checking the failing checks right now and will push fixes! 🧦

@dobby-coder dobby-coder Bot marked this pull request as ready for review April 24, 2026 08:02
@dobby-coder
Copy link
Copy Markdown
Contributor Author

dobby-coder Bot commented Apr 24, 2026

CI is green. All 20 checks pass (the 4 Format workspace (core/ffi/pkg/cli) jobs were failing because two test blocks in pg-core/src/client/rust/mod.rs and pg-core/src/util.rs weren't cargo fmt-clean — all four workspace fmt jobs run over pg-core files). Pushed 1f9708f with cargo fmt --all applied and marked the PR ready for review.

@dobby-coder dobby-coder Bot requested a review from rubenhensen April 24, 2026 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pg-core: panics on malformed ciphertext input instead of returning errors

1 participant