feat(permutation)!: oblivious key generation via sort-by-random-key - #282
Conversation
4364a7b to
434d845
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Security review follow-up
The initial review analysis was performed by GPT-5.6 Sol. After discussion with @coderdan, the collision-timing claim has been refined to distinguish whole-batch rejection from the current seeded architecture.
-
Zeroize the extracted permutation buffer (
packages/permutation/src/shuffle.rs, around lines 138–142)The plain
out: [u8; N]temporary contains the complete permutation. ForPermutationKey<128>in an optimized x86-64 build, it can remain on the stack after being copied into protected storage; the later wipe only clearsw. This leaves an unwiped copy even after the protected key is dropped. Wrap the extraction buffer inZeroizingand transfer it using a zeroing replacement, or construct it directly in protected storage. -
Refined collision-handling finding (
random_permutation/PermutationKey::from_seed)Discarding an entire colliding batch does not reveal the eventual accepted permutation. With independent random batches, the rejection count is independent of the accepted permutation, so whole-batch rejection itself is sound.
The narrower issue is that the current implementation does not discard the seed.
from_seedinitializes one deterministic RNG, andrandom_permutationadvances that same RNG and sorts another batch after a collision. Runtime therefore reveals whether that retained seed caused an initial collision: a predicate of the seed, although not of the final permutation. This event is extraordinarily rare (about2^-43forN = 128) and is not plausibly exploitable in the usual setting, but it conflicts with a strict constant-time-with-respect-to-seed claim.The intended seed lifecycle discussed with @coderdan removes that concern:
- generate a fresh independent seed;
- attempt exactly one batch;
- on collision, destroy the seed and generate another;
- retain only a seed whose first batch succeeds.
Under that architecture, provisioning time reveals only how many independent rejected seeds preceded the accepted seed. It reveals nothing about the retained seed beyond the already-known fact that it belongs to the collision-free set. To enforce this lifecycle, seeded derivation should return a collision error after one batch rather than retrying from the same RNG stream; the seed-generation layer can then retry with a new seed. Rejected seeds must not be reused, while caller-supplied fixed seeds should fail on collision.
Conclusion: treat this as an architecture/API mismatch, not a general objection to whole-batch rejection. Once the fresh-seed rejection lifecycle is enforced, fixed-work collision handling is unnecessary for this threat model.
416225e to
bfa83d9
Compare
|
Both findings from the security review follow-up are addressed in bfa83d9:
Also from the Claude review pass: the random-word sorting test now covers every supported length (N=32 previously had no sorting-correctness coverage at all). |
bfa83d9 to
fa8f6ba
Compare
fa8f6ba to
3ddf7d1
Compare
|
Rebased onto the re-based #281 ( |
✅ No CRAP threshold violations619 function(s) analyzed · threshold 30 |
🧬 Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 67 | 0 | 40 | 0 |
✅ Every mutant in the changed lines was caught by a test.
|
CI was red on the first Rust-gate run this branch has ever had (it predates #319). Fixed in 6a6aff5, all in
Local: 100 mutants on |
6a6aff5 to
6ed88a8
Compare
6ed88a8 to
54fc8df
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive collision policy contradicts the stated redraw behavior, and the constant-time documentation currently overstates the implementation guarantee.
Pull request overview
Replaces Fisher–Yates permutation generation with a sorting-network-based oblivious shuffle.
Changes:
- Adds compile-time Batcher sorting networks and validation tests.
- Integrates oblivious generation and collision rejection.
- Updates deterministic examples and removes
paste.
File summaries
| File | Description |
|---|---|
.cargo/mutants.toml |
Excludes an equivalent network-generator mutant. |
Cargo.lock |
Removes paste from the lockfile. |
packages/permutation/Cargo.toml |
Removes the unused paste dependency. |
packages/permutation/README.md |
Updates seed-derived outputs. |
packages/permutation/src/key.rs |
Uses the oblivious shuffle for key generation. |
packages/permutation/src/lib.rs |
Registers the shuffle module. |
packages/permutation/src/shuffle.rs |
Implements and tests the sorting-network shuffle. |
packages/random/src/lib.rs |
Adds the seed-rejection error variant. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f648899 to
7a666cf
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two new documentation claims incorrectly promise exact uniformity despite the bounded sampler’s documented statistical bias.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The cryptographic constant-time guarantees require the planned assembly and timing verification before final approval.
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Local assembly inspection of the shuffle pathCopilot's last pass deferred on the constant-time claims pending the #280 follow-ups. As an interim check I inspected the release codegen for the whole Method. A throwaway example with two Sort loop (N = 128: loop counter runs 2942 bytes / 2 = 1471 gates, matching the closed form):
Collision scan. LLVM rewrote Every conditional branch in the function, both targets:
Confirmed baseline caveat. On the accept path the permutation is Net: codegen matches the module doc's claim (trace is a function of N only, except the accept/reject branch) on both targets at the current tip. Still worth landing the #280 CI check so this holds across rustc upgrades. |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The oblivious permutation implementation itself matches amended #280: the fixed network, branchless compare-exchange, collision handling, protected output path, supported sizes, and verification are convincing. This layer also contains independently breaking RNG APIs and fixed-seed behavior changes that are not required by #280 and should be split into their own compatibility boundary. The password documentation must also be brought into line with the newly documented bounded-reduction bias.
| /// The type of the value drawn. This is the bound's own type for `u32` | ||
| /// and [`Protected<u32>`], and [`Protected<u32>`] for a | ||
| /// [`Protected<NonZeroU32>`] bound, since `0` is a valid draw. | ||
| type Output; |
There was a problem hiding this comment.
This associated type breaks every downstream BoundedRng implementation, but issue #280 is scoped to oblivious permutation generation and does not require reshaping this public trait. Please split the associated-output and Protected<NonZeroU32> API into its own issue/PR so this security fix has a reviewable compatibility boundary.
| // Because a 0 would be an invalid value we must try again (rejection sampling) | ||
| Self::random(rng) | ||
| } | ||
| let offset = rng.next_below(u32::from(u16::MAX)) as u16; |
There was a problem hiding this comment.
This changes fixed-seed output and stream consumption for NonZeroU16; the accompanying password changes are likewise independent of the oblivious shuffle requested by #280. Please move these behavior changes to a separately specified layer rather than coupling another seed-compatibility break to permutation generation.
There was a problem hiding this comment.
Moved to #336 with the password draw-loop refactor and the bias-bound docs. This branch no longer touches generatable.rs or vitaminc-password; the only vitaminc-random change left is the RandomError::SeedRejected variant the shuffle needs.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Re-review at head 873335f: the previously requested split is not present in the PR's merge diff. Despite the updated body and replies saying the RNG/password work moved to #336, this head still includes the breaking BoundedRng::Output/Protected<NonZeroU32> changes, the NonZeroU16 fixed-seed change, and the password refactor/docs; the password README still makes the conflicting equal-probability claim. The permutation implementation itself remains sound, but please push/rebase the advertised permutation-only branch before re-review.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Re-review at the still-current head 873335f. The exact merge diff continues to include the breaking RNG-trait, NonZeroU16, and password changes that the PR body says were moved to #336, and the password README mismatch remains. The permutation work itself is ready, but the advertised permutation-only rebase has not reached this PR. Please update the remote head before requesting another re-review.
873335f to
1362ac8
Compare
Fisher-Yates key generation addressed memory with the secret draw (`key.swap(i, j)` where `j` derives from the secret-seeded RNG), so key derivation had a secret-dependent memory trace even after the bounded draw was made fixed-count. The permute/depermute paths were already hardened with ConditionallySelectable scans; key generation was the remaining gap. Replace the algorithm with the djbsort/NTRU Prime construction: pack `(next_u64() << 8) | i`, sort through a Batcher odd-even mergesort network with a branchless subtle compare-exchange, and strip the low bytes. The gate schedule is a compile-time constant per length (Knuth 5.3.4 iterative form), so instruction trace, memory trace, and latency are functions of N only. Colliding random keys discard the whole batch (p ~ N^2/2^57): whole-batch restart preserves exact uniformity (a tie broken by the packed index would bias toward identity order) and obliviousness (patching colliding keys would leak which positions collided); restart timing depends only on discarded randomness. Tests: gate counts against the closed form (k^2 - k + 4)*2^(k-2) - 1, exhaustive zero-one verification at N=8 and N=16, randomized u64 sort checks at N=64/128, permutation validity for every IsPermutable size, seed determinism, and the existing position-matrix chi-squared uniformity test now exercising the new path. BREAKING CHANGE: PermutationKey::from_seed and from_controlled_seed derive different keys from the same seed than previous releases (the key-generation algorithm changed). Landing alongside the bounded-draw change so seed-derived keys break once, not twice. Closes #280 Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
Key collisions occur with probability ~2^-43, so the discard branch was unreachable by any test driving the RNG. Extract the sort + collision check + strip into permutation_from_words and test it with crafted words: equal sort keys (distinct payloads) must reject the batch, and distinct descending keys must reverse the payloads. Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
Two review findings (Claude review + GPT-5.6 Sol/Codex, refined on the PR): 1. Seed lifecycle. random_permutation retried a colliding batch by drawing more words from the same RNG. For an entropy-seeded RNG that is benign, but the intended architecture derives permutations from a long-term seed handed to clients: a seed whose first batch collides can never yield a stable derivation, and in-stream retry makes runtime reveal whether the *retained* seed collided — a predicate of the secret seed, not of discardable randomness. Now exactly one batch is attempted; a collision returns the new RandomError::SeedRejected, and the caller must discard the seed and provision a fresh one. Retry moves to the seed-generation layer, where timing reveals only how many independent, rejected seeds preceded the accepted one. 2. Zeroization. The extracted permutation — itself secret key material — was built and returned in plain [u8; N] stack arrays; only the packed u64 words were wiped. The output buffer now lives in Zeroizing end to end, leaving only the unavoidable move into the Protected container. Also extends the random-word network test to every supported length: zero-one covers 8/16 exhaustively, but N=32 previously had no sorting- correctness test at all, so a length-specific transcription bug would have produced silently biased keys. BREAKING CHANGE: PermutationKey::random / from_seed can now return RandomError::SeedRejected (probability ~2^-43 per seed). Treat it as "seed unusable": discard the seed and generate a fresh one; never retain a seed whose first derivation failed. In vitaminc-random, RandomError gains the SeedRejected variant, so an exhaustive match on RandomError must add an arm. Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
…rom IsPermutable Two review cleanups on the shuffle internals: - batcher_gate_count and batcher_schedule duplicated the same 4-deep loop nest, so the subtle merge guard existed in two copies that could drift apart — and the gates == G assert only catches drift that changes the count. Both are now thin wrappers over one batcher_fill (counting pass passes an empty slice; gates beyond out.len() are counted but not stored). - The gate schedule is a property of the array length alone, but lived as an associated const on the crate-wide IsPermutable marker, forcing every element type to carry a schedule no code reads and requiring the paste proc macro just to name the per-length consts. The schedules now live privately in shuffle.rs behind a const fn length lookup referenced through inline const blocks, so an unsupported length is still a compile-time error. IsPermutable reverts to a pure marker and nothing uses the paste macro any more; the dependency is dropped in the following commit. Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
`paste` is archived and flagged by cargo-audit as RUSTSEC-2024-0436. The previous commit removed vitaminc-permutation's only use of it, so it comes off the manifest and out of the lockfile; downstream `cargo audit` runs no longer attribute the advisory to this crate. Claude-Session: https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm
…P and mutants First run of the Rust gates on this branch (it predates #319, so only the Go workflow had ever run) found one CRAP violation and three surviving mutants, all in `shuffle.rs`. `schedule` scored 56: complexity 7 from one match arm per size, and 0% coverage, because production only evaluates it inside `const` blocks and llvm-cov cannot see compile-time execution. The bounds test now calls it at runtime, and a `should_panic` test covers the unsupported length arm. Same production code, 100% covered, CRAP 7. `batcher_fill`'s inner guard `i + j + k < n` had a surviving `<=` mutant: every shipped size is a power of two, where a partial final merge block never occurs, so nothing exercised it. A zero-one-principle test now builds the schedule at runtime for every n from 2 to 16 and checks sorting and gate bounds; odd n reach the guard and the mutant dies with an out-of-bounds gate. The outer merge loop's bound has an equivalent mutant: one extra iteration whose every inner step the guard rejects, unobservable by construction. It is rewritten as `n > j + k` so its mutant name differs from the inner guard's, and that one name is excluded in .cargo/mutants.toml with the reasoning. The inner guard stays mutated. Packing `(random << 8) | index` had an equivalent `^` mutant since the low byte is zero after the shift. `+` produces the same word and has no equivalent mutant. Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
…izes must be powers of two
Two review findings on the shuffle internals:
The supported lengths lived in two lists: the `IsPermutable` impls in
lib.rs and the `match N` in `schedule`. Nothing but this crate's own tests
kept them aligned, and a length with an impl but no schedule compiled
here and failed as a post-monomorphization const-eval panic in the
*downstream* crate's build. The schedule is now an associated const on
`IsPermutable`, built with `batcher_schedule::<{ batcher_gate_count(N) }>`
directly in the impl, so a missing network is E0046 when this crate
compiles. The `paste` dependency that motivated splitting the two apart
was never needed for this shape: a `macro_rules!` over the lengths does
it. The wider element types alias the `[u8; N]` schedule, since the
network depends on the length alone. The runtime `schedule` lookup, its
`_ => panic!` arm and `should_panic` test, and the duplicate `N <= 256`
assert are gone.
`batcher_fill`'s inner `i + j + k < n` guard is dead for every shipped
size: for a power of two every merge block is full, and emitting the gate
list with and without it is byte-identical at 8 through 128. The function
now asserts `n.is_power_of_two()` (every caller is const, so a violation
is a compile error) and the loops are written in their natural form. That
removes the reshaped `n > j + k` bound, its comment, the odd-length test
that existed only to exercise the guard, and the unanchored `>`/`>=`
exclusion in .cargo/mutants.toml that would have silently excluded any
later `>` written in the function.
The exhaustive zero-one test now runs over every power of two up to 16,
and at 8 and 16 asserts the runtime-built gate list is byte-for-byte the
compile-time `SCHEDULE`, which subsumes the separate binary-input test.
The `is_sorted` helper is std's since 1.82.
Claude-Session: https://claude.ai/code/session_014zPfvCbCsGsFwF547vgoEt
… slot `PermutationKey::random` took the `Zeroizing<[u8; N]>` the shuffle returned and handed `generate(|| *out)` a `Copy` of it, so a plain `[u8; N]` holding the whole key sat in the closure's return slot and the `f()` temporary with nothing to wipe it. On main the only plain-array temporary in this path was the non-secret identity. `random_permutation` now takes `&mut [u8; N]` and the key passes its own `Protected` slot through `inner_mut`, so no unwiped copy of the permutation exists at any point; on collision the zeroed slot drops and wipes like any key. `compare_exchange` copied both words into locals before selecting. It now uses `subtle`'s `conditional_swap`, whose only transient is the masked xor of the two words, never a copy of either. Docs brought in line with the code: the module doc claimed instruction trace was a function of `N` only, but `permutation_from_words` branches on the collision predicate by design, so the doc names that one exception instead of contradicting the function-level doc. `from_seed` gave the collision probability as "≈ 2⁻⁴³" unconditionally; that is the N = 128 worst case of N²/2⁵⁷, and it now says so. `random_permutation` also tells an entropy-seeded caller what to do on `SeedRejected`: build a fresh generator and call again. Claude-Session: https://claude.ai/code/session_014zPfvCbCsGsFwF547vgoEt
Each sample in `key_position_uniformity` is a permutation matrix, not N independent categorical draws, so the raw Pearson sum over the N² cells is not χ²((N − 1)²): its mean is N/(N − 1) times that. With the 85.35 threshold read off χ²(49), the true false-alarm rate for a correct generator was about 1%, not the 0.1% the comment claimed, so any re-roll of the seed or sample count had a one-in-a-hundred chance of failing on correct code. The statistic is now scaled by (N − 1)/N, which recovers χ²(49) (simulated over uniform permutations: mean 49.0, 0.1% above the threshold), and the comment says what "deterministic" does and does not buy. Claude-Session: https://claude.ai/code/session_014zPfvCbCsGsFwF547vgoEt
…twork Removing the dead `i + j + k < n` guard left the merge loop's `<` bound with an equivalent `<=` mutant: the extra iteration is now absorbed by the division guard instead, so it emits no gate and no test can see it. An `assert!` on the invariant the loop bounds imply, `i + j + k < n`, turns that off-by-one into a compile-time failure (every caller is const) and makes the mutant killable, with no exclusion in .cargo/mutants.toml. Verified with `cargo mutants --re batcher_fill`: 25 caught, 0 missed. Claude-Session: https://claude.ai/code/session_014zPfvCbCsGsFwF547vgoEt
1362ac8 to
adda436
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The rebase resolves the prior scope blockers: this merge diff is now limited to the oblivious permutation implementation, its documentation/dependency cleanup, and the required SeedRejected error. The Batcher schedule, constant-time compare-exchange, collision handling, protected output path, zeroized working state, and verification remain intact. No remaining spec or repository-standards concerns; CI is green.
Closes #280. Stacked on #281 (merge that first; this PR targets its branch). The
vitaminc-random/vitaminc-passwordchanges that came out of reviewing this PR now live in #336, a sibling on #281; this PR carries only the permutation change.What
Replaces Fisher–Yates in
PermutationKey::randomwith a constant-time oblivious shuffle: sort an array of random keys through a fixed Batcher odd-even mergesort network (the djbsort / NTRU Prime construction).Even with #281's fixed-count draw, Fisher–Yates still executes
key.swap(i, j)wherejderives from the secret-seeded RNG — a secret-dependent memory access pattern.permute/depermutewere already hardened withConditionallySelectablescans; key generation was the remaining gap flagged in #198's "constant-time note".Construction (
packages/permutation/src/shuffle.rs)w[i] = (rng.next_u64() << 8) | i— high 56 bits random sort key, low byte carries the index.const fn(Knuth 5.3.4 iterative form), one&'static [(u8, u8)]perIsPermutablelength (8–128, hung off the sealed trait so every length is compile-time guaranteed a schedule). Compare-exchange is branchlesssubtle(ct_gt+conditional_select): both slots read and written on every gate, gate indices always public.Choicemask. Exactly one batch is attempted: a collision returnsRandomError::SeedRejectedand the seed must be discarded. Retrying from the same stream would make runtime reveal whether the retained seed collided (a predicate of the secret), and such a seed can never derive a stable key anyway. Entropy-seeded callers build a freshSafeRandand call again. Whole-batch rejection keeps exact uniformity (index-broken ties bias toward identity) and obliviousness (patching would leak which positions collided). p ≈ N²/2⁵⁷, ≤ 2⁻⁴³ at N=128.Working state is wrapped in
Zeroizingand the permutation is written straight into the key'sProtectedslot. Instruction trace, memory trace, and per-instruction latency are functions ofNonly, except the single accept/reject branch on the collision predicate.Verification
(k² − k + 4)·2^(k−2) − 1for n = 2…128sort_unstableIsPermutablesize incl. new N=128 invert/complement coverageFollow-ups tracked in #280: asm inspection in CI (
cmov/csel, no branches on masks),dudecttiming tests, symbolic trace verification.Breaking changes
PermutationKey::from_seed/from_controlled_seedderive different keys from the same seed (algorithm change). Stacked on fix(random)!: replace rejection sampling with fixed-count Lemire draw #281 so seed-derived keys break once, not twice.PermutationKey::random/from_seedcan returnRandomError::SeedRejected;RandomErrorgains that variant, so exhaustive matches need an arm.https://claude.ai/code/session_012cUAnQ2qY9RD5TaAKyZpUm