fix(gbp-sframe): prevent SFrame nonce reuse when encryptors are recreated - #64
Merged
Conversation
…ated SFrameSession::encryptor(leaf_index) returned a fresh SFrameEncryptor with its own MonotonicCounter starting at 0 on every call, while every encryptor for a given leaf derives the same key from the same (base_key, KID). Recreating a handle - one per thread, one per request, after a free/reacquire cycle - could therefore reuse the same (key, KID, CTR) nonce as an earlier handle, breaking AES-GCM confidentiality and authenticity. SFrameEncryptor is now a cheap Clone handle around an Arc<Mutex<..>> holding the derived key and counter. SFrameSession caches one handle per leaf_index, so repeated encryptor() calls reconnect to the same counter instead of resetting it, and concurrent encrypt() calls from cloned handles allocate the counter under a lock (no two calls can ever observe the same value). The FFI and WASM bindings had the same bug one layer up: both re-derive a fresh SFrameSession on every encryptor-creation call (they only hold a borrowed MLS context, not a persisted session), which would silently undo the fix above by handing out a brand-new SFrameSession - and therefore a brand-new counter - each time. Added a matching cache in each binding, keyed by (session handle, leaf) for FFI and by leaf for WASM, so repeated creation calls reconnect to the same counter there too; freeing an FFI encryptor handle releases only that handle; the counter is released for good by gbp_sframe_session_free (on epoch change). Addresses the "Required immediate fix" items 1-4 and 6, and the "Temporary mitigation" of #62. Full library-owned sender-state architecture (distinct KID per stream, generation rollover, crash-safe persistence, typed conflict errors) is left to the follow-up in #63. Closes #62
Unrelated to this branch's sframe fix, but CI's clippy (running a
newer version than locally available) flags collapsible_match here,
blocking the build. Applies clippy's own suggested rewrite; behavior
is unchanged (a false condition falls through to the existing `_ => {}`
arm either way).
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
Closes #62.
SFrameSession::encryptor(leaf_index)returned a freshSFrameEncryptorwith its ownMonotonicCounterstarting at0on every call, while every encryptor for a given leaf derives the same key from the same(base_key, KID). Recreating a handle - one per thread, one per request, after a free/reacquire cycle - could therefore reuse the same(key, KID, CTR)nonce as an earlier handle, breaking AES-GCM confidentiality and authenticity.Changes
gbp-sframe:SFrameEncryptoris now a cheapClonehandle around anArc<Mutex<..>>holding the derived key and counter.SFrameSessioncaches one handle perleaf_index, so repeatedencryptor()calls reconnect to the same counter instead of resetting it, and concurrentencrypt()calls from cloned handles allocate the counter under a lock - no two calls can ever observe the same value.gbp-stack-ffi:gbp_sframe_encryptor_createre-derives a freshSFrameSessionon every call (it only borrows anMlsContext, not a persisted session), which would silently undo the fix above. Added a cache keyed by(session_handle, leaf_index)so repeated create calls reconnect to the same counter.gbp_sframe_encryptor_freeonly releases that specific handle; the underlying counter is released for good bygbp_sframe_session_free(call on epoch change).gbp-stack-wasm: same issue inSFrameSession::createEncryptor(re-derives the session from MLS every call) - added an equivalent per-leafIndexcache.Scope
This addresses the "Required immediate fix" items 1-4 and 6, and the "Temporary mitigation" section of #62 - it is no longer possible, through any of the Rust, FFI, or WASM APIs, to obtain two independent counters for the same
(session, leaf). It does not implement the full architecture from #63 (distinct KID per logical stream, generation rollover on counter exhaustion, crash-safe persistence across process restarts, typed conflict/rotation errors, C#/Python bindings) - that's a substantially larger, separately-tracked follow-up.Counter exhaustion itself (
u64::MAXwraparound) is still bounded by the upstreamsframecrate's current behavior, tracked separately by #61/#96/#97.Test plan
cargo test -p gbp-sframe -p gbp-stack-ffi -p gbp-stack- all passcargo test --workspace --exclude gbp-stack-wasm- all passwasm-pack test --node(gbp-stack-wasm) - 39/39 passcargo clippy -p gbp-sframe -p gbp-stack-ffi -p gbp-stack-wasm --all-targets- cleancargo fmt --check- cleangbp-sframe:repeated_encryptor_calls_share_one_counter_sequence,cloned_encryptor_handles_never_duplicate_a_counter_value(8 threads racing onencrypt(), decrypts each and asserts all 8 counter values 0..8 were used exactly once)gbp-stack-wasm:sframe_repeated_create_encryptor_shares_counter_state(twocreateEncryptorcalls for the same leaf; both frames must still decrypt - a reset counter would make the second a replay-window duplicate)