fix(security): address Codex Tier C audit — F-01 regression + cheap closes

Follow-up to 5c11947 after Codex's adversarial audit (tier-c-audit-2026-06-23.md). Fixes a regression the roster cap introduced and closes F-01's two cheap unbounded-growth vectors. No wire/protocol change, no new deps. - Regression (cap × reconnect): a peer reconnecting from a transient drop sits in `disconnected_peers`, not the live roster, so the new cap could reject it as "new" at a full 32-peer roster — and the eager `disconnected_peers.remove()` (before the cap check) then orphaned its recovery state so a later signed Leave skipped cleanup. Now reconnecting (and existing) peers are exempt from the cap via the pure `announce_subject_to_cap`, and the disconnect marker is cleared only after admission. PeerJoined semantics for reconnects are preserved. - F-01 replay map: `state_mutations_seen` was uncapped, so signed Leaves from unlimited generated keys grew it for the room's lifetime. Prune entries older than the freshness window once past a soft cap (`prune_stale_mutations`) — stale entries can't gate an in-window message (verify_gossip rejects the replay first), so replay protection is intact; the map is now bounded to ~authors-seen-per-window. - F-01 address lookup: a signed Leave now calls `remove_endpoint_info`, so cycling identities through Announce→Leave can't grow the iroh lookup without bound. Re-announce re-populates it. - F-03 test: added a forced same-hash/different-bytes ByteLru test (via a hash-injectable inner seam) so collision-safety is regression-tested, not just code-reviewed. Deferred follow-ups from the audit (logged): recovery/known_peers identity cap (needs a design pass, touches reconnect-resilience), F-02 result-cache LRU, and the (author,id)-vs-id attachment aliasing integrity bug. 416 lib tests (+3), clippy --all-targets clean, release build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-23 15:51:41 -04:00
parent 5c11947bd7
commit f927567105
2 changed files with 123 additions and 9 deletions
@@ -217,8 +217,13 @@ impl<V: Clone> ByteLru<V> {
        use std::hash::{Hash, Hasher};
        let mut hasher = std::collections::hash_map::DefaultHasher::new();
        bytes.hash(&mut hasher);
-        let hash = hasher.finish();
+        self.get_or_insert_hashed(hasher.finish(), bytes, build)
+    }

+    /// Inner seam with the content `hash` supplied explicitly. Production callers
+    /// use [`get_or_insert`]; tests use this to force a hash collision (different
+    /// bytes, same hash) and exercise the byte-equality guard.
+    fn get_or_insert_hashed(&mut self, hash: u64, bytes: &[u8], build: impl FnOnce() -> V) -> V {
        if let Some(idx) = self
            .entries
            .iter()
@@ -288,6 +293,21 @@ mod tests {
        assert_eq!(ins(&mut lru, b"b"), 4);
    }

+    #[test]
+    fn byte_lru_byte_equality_survives_a_hash_collision() {
+        // Force the SAME 64-bit hash for two DIFFERENT byte strings (the case a
+        // bare-hash cache would alias — Tier C F-03 collision bug).
+        let mut lru: ByteLru<u32> = ByteLru::new(4);
+        assert_eq!(lru.get_or_insert_hashed(42, b"alice", || 1), 1);
+        // `bob` collides on the hash but differs in bytes → a MISS, built fresh,
+        // NOT aliased to alice's value.
+        assert_eq!(lru.get_or_insert_hashed(42, b"bob", || 2), 2);
+        // Both coexist; each re-lookup returns its own value (build closure unused).
+        assert_eq!(lru.get_or_insert_hashed(42, b"alice", || 99), 1);
+        assert_eq!(lru.get_or_insert_hashed(42, b"bob", || 99), 2);
+        assert_eq!(lru.len(), 2);
+    }
+
    #[test]
    fn initials_takes_first_two_words() {
        assert_eq!(initials("Alice"), "A");
@@ -131,6 +131,27 @@ fn admit_state_mutation(
    true
 }

+/// Size at which we prune stale entries from the replay-tracking map (Tier C
+/// F-01 audit). `admit_state_mutation` records `(author, kind)` for every signed
+/// mutation, so an insider sending validly signed `Leave`s from unlimited
+/// generated keys would otherwise grow it for the room's lifetime. A mutation
+/// older than the freshness window can never be the deciding `last_ts` for an
+/// in-window message — `verify_gossip`'s timestamp check rejects such a replay
+/// first — so dropping those entries cannot weaken replay protection; it bounds
+/// the map to roughly the authors seen within one freshness window.
+const STATE_MUTATIONS_SOFT_CAP: usize = 256;
+
+/// Drop replay-tracking entries whose timestamp is older than `window_ms` before
+/// `now_ms` (see [`STATE_MUTATIONS_SOFT_CAP`]). Pure → unit-testable.
+fn prune_stale_mutations(
+    seen: &mut HashMap<(EndpointId, StateMutationKind), u64>,
+    now_ms: u64,
+    window_ms: u64,
+) {
+    let floor = now_ms.saturating_sub(window_ms);
+    seen.retain(|_, last_ts| *last_ts >= floor);
+}
+
 /// Maximum number of distinct peers we hold in a room roster at once.
 ///
 /// Everyone with the room ticket is an authenticated *insider*: a signature only
@@ -174,12 +195,23 @@ fn sanitize_endpoint_addr(addr: &EndpointAddr) -> EndpointAddr {
    EndpointAddr { id: addr.id, addrs }
 }

-/// Whether an `Announce` may enter the roster. A NEW author is admitted only
-/// while the roster is below [`MAX_ACTIVE_PEERS`]; updates to an already-present
-/// peer always pass (so a full room's members can keep changing mute/avatar/etc).
+/// Whether an `Announce` may enter the roster. Only a brand-new author
+/// (`subject_to_cap`) is gated by [`MAX_ACTIVE_PEERS`]; updates to an
+/// already-present peer AND re-announces from a peer mid-reconnect (which
+/// already held a slot) always pass — exempting reconnects keeps a full room
+/// from rejecting a legitimately reconnecting member and orphaning its recovery
+/// state (Tier C F-01 audit). Pure → unit-testable.
+fn admit_into_roster(roster_len: usize, subject_to_cap: bool, max_peers: usize) -> bool {
+    !subject_to_cap || roster_len < max_peers
+}
+
+/// Whether a received `Announce`'s author is gated by the roster cap. A peer
+/// already in the roster (`is_new == false`, an ordinary update) or one
+/// mid-reconnect (`is_reconnecting`, it already held a slot) is exempt; only a
+/// brand-new author counts against [`MAX_ACTIVE_PEERS`] (Tier C F-01 audit).
 /// Pure → unit-testable.
-fn admit_into_roster(roster_len: usize, is_new: bool, max_peers: usize) -> bool {
-    !is_new || roster_len < max_peers
+fn announce_subject_to_cap(is_new: bool, is_reconnecting: bool) -> bool {
+    is_new && !is_reconnecting
 }

 fn peer_state_for_log(state: &PeerState) -> String {
@@ -441,6 +473,18 @@ impl RoomState for IrohGossipState {
                                    continue;
                                }

+                                // Keep the replay-tracking map bounded: prune entries
+                                // older than the freshness window once it grows past the
+                                // soft cap (Tier C F-01 audit). Stale entries can't gate
+                                // an in-window message, so this never weakens replay
+                                // protection.
+                                if state_mutations_seen.len() > STATE_MUTATIONS_SOFT_CAP {
+                                    prune_stale_mutations(
+                                        &mut state_mutations_seen,
+                                        now_millis(),
+                                        GOSSIP_FRESHNESS_MS,
+                                    );
+                                }
                                if !admit_state_mutation(
                                    &mut state_mutations_seen,
                                    payload.author,
@@ -491,15 +535,25 @@ impl RoomState for IrohGossipState {
                                        // before we retain it / hand it to the dialer
                                        // (Tier C F-01).
                                        state.addr = sanitize_endpoint_addr(&state.addr);
-                                        disconnected_peers.lock().unwrap().remove(&payload.author);
+                                        // A peer reconnecting from a transient drop sits
+                                        // in `disconnected_peers` (not the live roster);
+                                        // it already held a slot, so it must be re-admitted
+                                        // regardless of the cap, and its disconnect marker
+                                        // cleared ONLY once re-admitted — clearing it before
+                                        // a possible reject would orphan its recovery state
+                                        // (Tier C F-01 audit).
+                                        let is_reconnecting =
+                                            disconnected_peers.lock().unwrap().contains(&payload.author);
                                        let admitted = {
                                            let mut peer_map = peers.lock().unwrap();
                                            let is_new = !peer_map.contains_key(&payload.author);
                                            // Cap the roster so a flood of signed
                                            // sock-puppet identities can't grow our
                                            // memory/tasks/dials without bound (Tier C
-                                            // F-01). Existing peers' updates always pass.
-                                            if !admit_into_roster(peer_map.len(), is_new, MAX_ACTIVE_PEERS) {
+                                            // F-01). Existing-peer updates and reconnects
+                                            // are exempt; only brand-new authors are gated.
+                                            let subject_to_cap = announce_subject_to_cap(is_new, is_reconnecting);
+                                            if !admit_into_roster(peer_map.len(), subject_to_cap, MAX_ACTIVE_PEERS) {
                                                None
                                            } else {
                                                let state_changed = peer_map.get(&payload.author) != Some(&state);
@@ -516,6 +570,10 @@ impl RoomState for IrohGossipState {
                                            ));
                                            continue;
                                        };
+                                        // Admitted — now it is safe to clear any reconnect
+                                        // marker (a rejected announce above leaves it intact
+                                        // so a later signed Leave still cleans up).
+                                        disconnected_peers.lock().unwrap().remove(&payload.author);

                                        if is_new {
                                            crate::log_msg(&format!(
@@ -541,6 +599,11 @@ impl RoomState for IrohGossipState {
                                    }
                                    GossipMessage::Leave => {
                                        crate::log_msg(&format!("Gossip peer leave request from author={:?}", payload.author));
+                                        // Drop this id's address-lookup entry so cycling
+                                        // distinct identities through Announce→Leave can't
+                                        // grow the lookup for the room's lifetime (Tier C
+                                        // F-01 audit). Re-announce re-populates it.
+                                        let _ = address_lookup.remove_endpoint_info(payload.author);
                                        let removed = peers.lock().unwrap().remove(&payload.author).is_some();
                                        let was_disconnected = disconnected_peers
                                            .lock()
@@ -856,6 +919,37 @@ mod tests {
        assert!(admit_into_roster(99, false, 3));
    }

+    #[test]
+    fn reconnecting_and_existing_peers_are_exempt_from_the_cap() {
+        // A brand-new author counts against the cap...
+        assert!(announce_subject_to_cap(/* is_new */ true, /* is_reconnecting */ false));
+        // ...but an ordinary update from an in-roster peer does not...
+        assert!(!announce_subject_to_cap(false, false));
+        // ...and neither does a re-announce from a peer mid-reconnect, even
+        // though it was removed from the live roster (the F-01-audit fix: a full
+        // room must not reject a legitimately reconnecting member).
+        assert!(!announce_subject_to_cap(true, true));
+        // Combined with admit_into_roster: a reconnecting author passes at a full
+        // roster, a brand-new one does not.
+        assert!(admit_into_roster(3, announce_subject_to_cap(true, true), 3));
+        assert!(!admit_into_roster(3, announce_subject_to_cap(true, false), 3));
+    }
+
+    #[test]
+    fn prune_stale_mutations_drops_only_out_of_window_entries() {
+        let a = fresh_id();
+        let b = fresh_id();
+        let mut seen = HashMap::new();
+        seen.insert((a, StateMutationKind::Announce), 10_000u64);
+        seen.insert((b, StateMutationKind::Leave), 250_000u64);
+        // now = 300_000, window = 120_000 → floor 180_000. The 10_000 entry is
+        // stale (and could never gate an in-window message), the 250_000 is live.
+        prune_stale_mutations(&mut seen, 300_000, GOSSIP_FRESHNESS_MS);
+        assert_eq!(seen.len(), 1);
+        assert!(seen.contains_key(&(b, StateMutationKind::Leave)));
+        assert!(!seen.contains_key(&(a, StateMutationKind::Announce)));
+    }
+
    #[test]
    fn sanitize_endpoint_addr_caps_address_count() {
        use std::net::SocketAddr;