fix(gbp-sframe): prevent SFrame nonce reuse when encryptors are recreated

crates/gbp-sframe/src/cipher.rs

@@ -1,4 +1,5 @@
 use std::collections::HashMap;
+use std::sync::{Arc, Mutex};
 
 use sframe::frame::{EncryptedFrameView, MediaFrameView, MonotonicCounter, ReplayAttackProtection};
 use sframe::header::KeyId;
@@ -13,15 +14,27 @@ const REPLAY_WINDOW: u64 = 1024;
 
 // ─── SFrameEncryptor ─────────────────────────────────────────────────────────
 
-/// Stateful per-sender SFrame encryptor.
+/// The key + counter for one `(epoch, leaf_index)` KID.
 ///
-/// Holds the derived key for one `(epoch, leaf_index)` KID and an internal
-/// counter that increments on every call to [`encrypt`](Self::encrypt).
-///
-/// Obtain via [`crate::SFrameSession::encryptor`].
-pub struct SFrameEncryptor {
+/// Shared behind an [`Arc<Mutex<_>>`] by every [`SFrameEncryptor`] handle for
+/// that KID, so cloning a handle - or obtaining a new one from
+/// [`crate::SFrameSession::encryptor`] - can never produce a second,
+/// independent counter that would reuse a `(key, KID, CTR)` nonce.
+struct EncryptorState {
     key: EncryptionKey,
     counter: MonotonicCounter,
+}
+
+/// Stateful per-sender SFrame encryptor handle.
+///
+/// Cloning a handle, or requesting another one for the same `(epoch,
+/// leaf_index)` via [`crate::SFrameSession::encryptor`], shares the same
+/// underlying counter - it does **not** create an independent one. This
+/// makes it safe to hold multiple handles (e.g. one per thread) for the same
+/// sender without risking AEAD nonce reuse.
+#[derive(Clone)]
+pub struct SFrameEncryptor {
+    state: Arc<Mutex<EncryptorState>>,
     kid: KeyId,
 }
 
@@ -30,8 +43,10 @@ impl SFrameEncryptor {
         let key = EncryptionKey::derive_from(suite.to_sframe(), kid, base_key)
             .expect("key derivation from a 32-byte base key never fails");
         Self {
-            key,
-            counter: MonotonicCounter::default(),
+            state: Arc::new(Mutex::new(EncryptorState {
+                key,
+                counter: MonotonicCounter::default(),
+            })),
             kid,
         }
     }
@@ -42,9 +57,19 @@ impl SFrameEncryptor {
     /// `extra_aad` is bound into the AEAD tag (e.g. an RTP header) but is **not**
     /// carried in the returned payload; the receiver supplies the same slice to
     /// [`SFrameDecryptor::decrypt`].
+    ///
+    /// Safe to call concurrently from multiple handles sharing this sender's
+    /// state: the counter is allocated under a lock, so concurrent calls
+    /// never allocate the same value twice.
     pub fn encrypt(&mut self, plaintext: &[u8], extra_aad: &[u8]) -> Result<Vec<u8>, SFrameError> {
-        let frame = MediaFrameView::with_meta_data(&mut self.counter, plaintext, extra_aad);
-        let encrypted = frame.encrypt(&self.key).map_err(|_| SFrameError::Encrypt)?;
+        let mut state = self
+            .state
+            .lock()
+            .expect("sframe encryptor state mutex poisoned");
+        let frame = MediaFrameView::with_meta_data(&mut state.counter, plaintext, extra_aad);
+        let encrypted = frame
+            .encrypt(&state.key)
+            .map_err(|_| SFrameError::Encrypt)?;
 
         // sframe serialises `meta_data ‖ header ‖ ciphertext`; strip the
         // metadata prefix so `extra_aad` stays off the wire.
@@ -53,7 +78,11 @@ impl SFrameEncryptor {
 
     /// Current counter value (number of frames encrypted so far).
     pub fn counter(&self) -> u64 {
-        self.counter.current()
+        self.state
+            .lock()
+            .expect("sframe encryptor state mutex poisoned")
+            .counter
+            .current()
     }
 
     /// KID this encryptor was created for.

crates/gbp-sframe/src/lib.rs

@@ -68,17 +68,27 @@ pub use error::SFrameError;
 pub use header::SFrameHeader;
 pub use kdf::{CipherSuite, derive_base_key};
 
+use std::collections::HashMap;
+use std::sync::Mutex;
+
 use gbp_mls::MlsContext;
 
 /// An SFrame session bound to one MLS epoch.
 ///
 /// A new session must be created whenever the MLS group commits (epoch
 /// changes) — the old base key becomes unreachable and all per-sender keys
 /// are rotated automatically.
+///
+/// Owns the per-leaf encryptor state for this epoch: repeated calls to
+/// [`encryptor`](Self::encryptor) for the same `leaf_index` return a handle
+/// to the *same* counter, never a fresh one, so callers cannot accidentally
+/// reuse a `(key, KID, CTR)` nonce by recreating a handle (e.g. one per
+/// thread, or on every request).
 pub struct SFrameSession {
     base_key: [u8; 32],
     epoch: u64,
     suite: CipherSuite,
+    encryptors: Mutex<HashMap<u32, SFrameEncryptor>>,
 }
 
 impl SFrameSession {
@@ -91,6 +101,7 @@ impl SFrameSession {
             base_key,
             epoch,
             suite,
+            encryptors: Mutex::new(HashMap::new()),
         }
     }
 
@@ -121,14 +132,25 @@ impl SFrameSession {
         self.suite
     }
 
-    /// Creates a sender-side encryptor for `leaf_index`.
+    /// Returns a sender-side encryptor handle for `leaf_index`.
     ///
-    /// The returned [`SFrameEncryptor`] owns the derived key+salt for this
-    /// sender and maintains an internal counter.  Create one per sender; do
-    /// **not** share an encryptor across multiple goroutines/threads.
+    /// The returned [`SFrameEncryptor`] holds the derived key+salt for this
+    /// sender and shares its counter with every other handle returned for
+    /// the same `leaf_index` on this session - calling this repeatedly (one
+    /// per thread, one per request, ...) is safe and will never produce two
+    /// independent counters for the same KID.
     pub fn encryptor(&self, leaf_index: u32) -> SFrameEncryptor {
-        let kid = SFrameHeader::kid_from(self.epoch, leaf_index);
-        SFrameEncryptor::new(&self.base_key, kid, self.suite)
+        let mut encryptors = self
+            .encryptors
+            .lock()
+            .expect("sframe session encryptor map mutex poisoned");
+        encryptors
+            .entry(leaf_index)
+            .or_insert_with(|| {
+                let kid = SFrameHeader::kid_from(self.epoch, leaf_index);
+                SFrameEncryptor::new(&self.base_key, kid, self.suite)
+            })
+            .clone()
     }
 
     /// Creates a receiver-side decryptor for this epoch.
@@ -225,4 +247,64 @@ mod tests {
         let payload = enc.encrypt(b"stale", b"").unwrap();
         assert!(dec.decrypt(&payload, b"").is_err());
     }
+
+    #[test]
+    fn repeated_encryptor_calls_share_one_counter_sequence() {
+        // Regression test for a critical nonce-reuse bug: session.encryptor(leaf) used to
+        // return a fresh MonotonicCounter starting at 0 on every call, so e.g. recreating a
+        // handle per request/thread could encrypt multiple frames under the same
+        // (key, KID, CTR).
+        let session = test_session(0);
+
+        let mut first = session.encryptor(0);
+        let mut second = session.encryptor(0); // must reconnect to the same state, not reset it
+
+        assert_eq!(first.counter(), 0);
+        assert_eq!(second.counter(), 0);
+
+        first.encrypt(b"one", b"").unwrap();
+        assert_eq!(first.counter(), 1);
+        // `second` observes the counter `first` already advanced - proof they share state.
+        assert_eq!(second.counter(), 1);
+
+        second.encrypt(b"two", b"").unwrap();
+        assert_eq!(first.counter(), 2);
+    }
+
+    #[test]
+    fn cloned_encryptor_handles_never_duplicate_a_counter_value() {
+        let session = test_session(0);
+        let enc = session.encryptor(0);
+        let mut dec = session.decryptor();
+
+        // Simulate "one encryptor per thread": clone the handle and encrypt concurrently.
+        let handles: Vec<_> = (0..8)
+            .map(|i| {
+                let mut enc = enc.clone();
+                std::thread::spawn(move || enc.encrypt(format!("frame-{i}").as_bytes(), b""))
+            })
+            .collect();
+
+        let payloads: Vec<Vec<u8>> = handles
+            .into_iter()
+            .map(|h| h.join().unwrap().unwrap())
+            .collect();
+
+        // Every counter value in [0, 8) must have been used exactly once: if two threads had
+        // allocated the same CTR, one of these decrypts would fail (duplicate nonce corrupts
+        // the AEAD tag input) - but more importantly, the exact same (key, KID, CTR) must never
+        // have produced two ciphertexts in the first place.
+        let mut counters: Vec<u64> = payloads
+            .iter()
+            .map(|payload| {
+                let (_, _leaf) = dec.decrypt(payload, b"").unwrap();
+                sframe::frame::EncryptedFrameView::try_new(payload.as_slice())
+                    .unwrap()
+                    .header()
+                    .counter()
+            })
+            .collect();
+        counters.sort_unstable();
+        assert_eq!(counters, (0..8).collect::<Vec<_>>());
+    }
 }

crates/gbp/node/src/node.rs

@@ -627,20 +627,18 @@ impl GroupNode {
                     self.transition(NodeState::Active);
                 }
             }
-            ControlOpcode::CapabilitiesAdvertise => {
-                if Self::is_coordinator_claim(&c.args) {
-                    // Coordinator is alive — reset silence timer.
-                    self.note_coordinator_activity();
-                    // Collision resolution (gbp-control-plane §5.1): if we
-                    // also claimed and the remote claimant has a lower
-                    // MemberId, yield the coordinator role to them.
-                    if self.is_coordinator && c.sender_id < self.member_id {
-                        self.is_coordinator = false;
-                    }
-                    self.events.push(Event::CoordinatorClaim {
-                        claimant: c.sender_id,
-                    });
+            ControlOpcode::CapabilitiesAdvertise if Self::is_coordinator_claim(&c.args) => {
+                // Coordinator is alive — reset silence timer.
+                self.note_coordinator_activity();
+                // Collision resolution (gbp-control-plane §5.1): if we
+                // also claimed and the remote claimant has a lower
+                // MemberId, yield the coordinator role to them.
+                if self.is_coordinator && c.sender_id < self.member_id {
+                    self.is_coordinator = false;
                 }
+                self.events.push(Event::CoordinatorClaim {
+                    claimant: c.sender_id,
+                });
             }
             _ => {}
         }

crates/gbp/stack-ffi/src/lib.rs

@@ -218,6 +218,33 @@ fn sframe_encryptors() -> &'static SFrameEncryptorRegistry {
     R.get_or_init(SFrameEncryptorRegistry::new)
 }
 
+/// Shares one [`SFrameEncryptor`] (and therefore one counter) across every
+/// `gbp_sframe_encryptor_create` call for the same `(session_handle,
+/// leaf_index)`.
+///
+/// [`gbp_sframe_encryptor_create`] re-derives a fresh [`SFrameSession`] on
+/// every call (it only borrows an [`MlsContext`], not a persisted session
+/// object), so without this cache each call would otherwise construct an
+/// independent, counter-reset-to-0 encryptor for the same KID - a critical
+/// AEAD nonce-reuse bug. Entries outlive individual `..._free` calls on
+/// purpose, so recreating a handle for the same session+leaf always
+/// reconnects to the same counter instead of resetting it.
+struct SFrameEncryptorDedup {
+    by_session_leaf: Mutex<HashMap<(i32, u32), SFrameEncryptor>>,
+}
+impl SFrameEncryptorDedup {
+    fn new() -> Self {
+        Self {
+            by_session_leaf: Mutex::new(HashMap::new()),
+        }
+    }
+}
+fn sframe_encryptor_dedup() -> &'static SFrameEncryptorDedup {
+    use std::sync::OnceLock;
+    static R: OnceLock<SFrameEncryptorDedup> = OnceLock::new();
+    R.get_or_init(SFrameEncryptorDedup::new)
+}
+
 // ============================================================================
 // Version
 // ============================================================================
@@ -1491,16 +1518,28 @@ pub unsafe extern "C" fn gbp_sframe_session_create(
 }
 
 /// Frees an SFrame session created by [`gbp_sframe_session_create`].
+///
+/// Also releases any encryptor state cached for this session by
+/// [`gbp_sframe_encryptor_create`] - call this on epoch change so a later,
+/// unrelated session cannot be handed a stale cache entry.
 #[unsafe(no_mangle)]
 pub extern "C" fn gbp_sframe_session_free(handle: i32) {
     sframe_sessions().remove(handle);
+    sframe_encryptor_dedup()
+        .by_session_leaf
+        .lock()
+        .unwrap()
+        .retain(|(session_handle, _), _| *session_handle != handle);
 }
 
-/// Creates an encryptor for the local sender (`leaf_index`) within an epoch.
+/// Creates (or reconnects to) an encryptor for the local sender (`leaf_index`)
+/// within an epoch.
 ///
 /// The session handle MUST be the one returned by [`gbp_sframe_session_create`]
-/// for the same epoch.  One encryptor per sender; do **not** share across
-/// threads.
+/// for the same epoch. Calling this more than once for the same
+/// `(session_handle, leaf_index)` - e.g. one call per thread, or after
+/// freeing a previous handle - returns a handle sharing the *same* counter;
+/// it never resets it, so it is always safe against AEAD nonce reuse.
 ///
 /// Returns a positive encryptor handle, or `0` on failure.
 ///
@@ -1537,21 +1576,46 @@ pub unsafe extern "C" fn gbp_sframe_encryptor_create(
         set_last_error("invalid session handle");
         return 0;
     }
+    let dedup_key = (session_handle, leaf_index);
+    if let Some(existing) = sframe_encryptor_dedup()
+        .by_session_leaf
+        .lock()
+        .unwrap()
+        .get(&dedup_key)
+    {
+        return sframe_encryptors().insert(existing.clone());
+    }
+
     let Some(mls_arc) = mls().get(mls_handle) else {
         set_last_error("invalid MLS handle");
         return 0;
     };
     let mls = mls_arc.lock().unwrap();
     match SFrameSession::from_mls(&mls, label, suite) {
-        Ok(session) => sframe_encryptors().insert(session.encryptor(leaf_index)),
+        Ok(session) => {
+            let encryptor = session.encryptor(leaf_index);
+            sframe_encryptor_dedup()
+                .by_session_leaf
+                .lock()
+                .unwrap()
+                .insert(dedup_key, encryptor.clone());
+            sframe_encryptors().insert(encryptor)
+        }
         Err(e) => {
             set_last_error(e);
             0
         }
     }
 }
 
-/// Frees an encryptor created by [`gbp_sframe_encryptor_create`].
+/// Frees an encryptor handle created by [`gbp_sframe_encryptor_create`].
+///
+/// This only releases *this* handle; the underlying per-`(session, leaf)`
+/// counter state is kept alive (by [`sframe_encryptor_dedup`]) for the
+/// lifetime of the session, so a later `gbp_sframe_encryptor_create` call for
+/// the same `(session_handle, leaf_index)` reconnects to it rather than
+/// starting a new counter at `0`. Use [`gbp_sframe_session_free`] to release
+/// it for good, on epoch change.
 #[unsafe(no_mangle)]
 pub extern "C" fn gbp_sframe_encryptor_free(handle: i32) {
     sframe_encryptors().remove(handle);