fix: reject replayed MPC session initiations from the coordinator (#241) - #474
Merged
Marvy247 merged 1 commit intoAug 29, 2026
Merged
Conversation
…tEmPoka#241) services/node's sessions map is never pruned, so a coordinator (or an attacker replaying a captured request) could POST /session/:id/shares again for a session_id that had already reached Complete. Because receive_share_fragment() unconditionally set status back to SharesReceived, this silently reopened a finished session: a subsequent /generate call could then overwrite the already-delivered proof for that session_id with a different one — a real tampering vector for a poker MPC system, not just wasted work. - Added NodeState.finalized_sessions: a HashSet<String> recording every session_id that has reached SessionStatus::Complete, populated in post_generate's background task at the same point status is set to Complete. Kept independent of the `sessions` map itself so the check still holds even if `sessions` entries were ever pruned in the future. - post_shares now rejects (409 Conflict) any share submission whose session_id is in finalized_sessions, before touching the sessions map at all. - post_shares also now rejects (409 Conflict) a share submission for a session that exists but has moved past SessionStatus::SharesReceived (WitnessGenerating/ProofGenerating/Failed) — this catches an in-flight replay attempt that the finalized_sessions check alone wouldn't, since that check only covers sessions that reached Complete. Tests (services/node/src/api.rs, replay_protection_tests module): - a fresh session_id is still accepted (no false positive) - a session_id already in finalized_sessions is rejected with 409 and the expected message - a session that's been moved to WitnessGenerating (simulating what post_generate does) rejects a further share submission with 409 - marking one session_id finalized doesn't affect an unrelated session_id Verification: no local Rust toolchain available in this environment (link.exe fails compiling proc-macro2/quote build scripts — the same limitation hit on other Rust work this session), so `cargo test -p mpc-node` could not be run here. Verified by manual review: the new guards are two early-return checks with no interaction with the rest of post_shares's logic, and the existing post_shares/post_generate control flow (circuit-name mismatch check, capacity check, background proof generation) is otherwise untouched.
|
@Davidemulo Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
Summary
services/node'ssessionsmap is never pruned, so a coordinator (or an attacker replaying a captured request) couldPOST /session/:id/sharesagain for asession_idthat had already reachedComplete. Becausereceive_share_fragment()unconditionally set status back toSharesReceived, this silently reopened a finished session: a subsequent/generatecall could then overwrite the already-delivered proof for thatsession_idwith a different one — a real tampering vector for a poker MPC system, not just wasted work.Fix
NodeState.finalized_sessions: aHashSet<String>recording everysession_idthat has reachedSessionStatus::Complete, populated inpost_generate's background task at the same point status is set toComplete. Kept independent of thesessionsmap itself so the check still holds even ifsessionsentries were ever pruned in the future.post_sharesnow rejects (409 Conflict) any share submission whosesession_idis infinalized_sessions, before touching the sessions map at all.post_sharesalso now rejects (409 Conflict) a share submission for a session that exists but has moved pastSessionStatus::SharesReceived(WitnessGenerating/ProofGenerating/Failed) — this catches an in-flight replay attempt that thefinalized_sessionscheck alone wouldn't, since that check only covers sessions that reachedComplete.Tests
services/node/src/api.rs,replay_protection_testsmodule:session_idis still accepted (no false positive)session_idalready infinalized_sessionsis rejected with 409 and the expected messageWitnessGenerating(simulating whatpost_generatedoes) rejects a further share submission with 409session_idfinalized doesn't affect an unrelatedsession_idVerification
No local Rust toolchain is available in this environment (
link.exefails compilingproc-macro2/quotebuild scripts).cargo test -p mpc-nodecould not be run here. Verified by manual review: the new guards are two early-return checks with no interaction with the rest ofpost_shares's logic, and the existingpost_shares/post_generatecontrol flow (circuit-name mismatch check, capacity check, background proof generation) is otherwise untouched. Would appreciate CI/a reviewer confirmingcargo test -p mpc-nodelocally.Closes #241