fix(crypto): commit replay window only after AEAD verification#60
Merged
Conversation
The anti-replay window advanced its high-water mark before AEAD verification: a forged frame carrying an arbitrary large counter slid latest forward and wiped the bitmap, so subsequent legitimate packets were rejected as too-old — an off-path session-killing DoS (the counter is in the clear on the raw path). Split ReplayWindow::check_and_set into a read-only check() and a mutating commit(); open()/open_into() now check() before AEAD and commit() only after it verifies, matching WireGuard's post-decrypt window advance. AEAD wire bytes unchanged (both snow KATs still pass).
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
The anti-replay window advanced its high-water mark before AEAD verification, so a forged frame carrying an arbitrary counter could slide the window forward and starve the session of legitimate traffic — an off-path, session-killing DoS. This reorders the receive path to WireGuard's actual behaviour: check (read-only) → AEAD verify → commit.
The bug
ReplayWindow::check_and_setdidcheck + mutateatomically, andSession::open/open_intocalled it beforeopen_in_place:When
counter > latest,check_and_setunconditionally setslatest = counterand rewrites the bitmap. A forged frame at, say,latest + 1_000_000therefore slides the window far forward and wipes it. Subsequent legitimate packets then hitdiff = latest - counter ≥ 64and are rejected as too old.Exploitability: on the default raw-UDP path the counter is on the wire in the clear, so an off-path attacker who can inject a UDP packet to the peer's socket controls it. AEAD rejects the forgery, but the window has already moved. (The
obf_pskpath is unaffected — the counter rides inside the keystream-XOR envelope, so controlling it requires the obf key.)This is a DoS surface only — not a confidentiality or integrity break. Forged frames still never decrypt.
The fix
Split
ReplayWindow::check_and_setinto:check(&self, counter) -> bool— read-only; would this counter be accepted?commit(&mut self, counter)— advance/record; caller guaranteescheck(and AEAD) passed first.open/open_intonowcheck()before AEAD andcommit()only after it verifies.check_and_setis retained as a#[cfg(test)]helper for the sliding-window unit tests. Single-threaded&mut selfreceive path, so there's no check→commit TOCTOU.Testing
forged_frame_does_not_advance_replay_windowand itsopen_intotwin) inject a forged far-future counter and assert legitimate frames still open afterwards. Both fail onmain, pass here.seal_is_byte_identical_across_a_reference_session,session_seal_is_byte_identical_to_snow_write_message_both_directions) still pass.yip-cryptosuite (20 tests) green; clippy clean;yipdbuilds.Credit
Surfaced while re-auditing the receive path in response to public scrutiny of the nonce construction (the nonce itself is the standard Noise §11.4 / RFC 8439 construction and is correct as-is — this is the separate, real issue that audit turned up).