From 5c11947bd7ecf603ad3d3af4cac931c807d1a23d Mon Sep 17 00:00:00 2001 From: Mollusk Date: Tue, 23 Jun 2026 15:10:08 -0400 Subject: [PATCH] fix(security): Tier C F-01/F-02/F-03 insider resource-exhaustion caps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A room ticket holder is an authenticated insider; signatures only prove keypair ownership, not a distinct human. Previously such a member could exhaust a victim's memory/tasks/dials without bound. Add caps + dedup at the gossip/core/UI boundaries (no wire/protocol change, no new deps): F-01 (gossip): cap the roster at MAX_ACTIVE_PEERS (32) — new authors are rejected when full, existing peers' updates always pass; sanitize each announced EndpointAddr (<=8 addrs, relay-URL <=256 bytes, drop Custom); replace (set_endpoint_info) instead of unioning attacker address history. F-02 (core): gate chat image auto-fetch — only roster authors qualify, (author, attachment_id) is deduped, and a 4-permit pool bounds concurrent detached fetch tasks (RAII AutoFetchGuard releases permit + dedup marker). Chat text is still shown (already sanitized); the user-initiated "Save" fetch is unchanged. Non-roster sock-puppet chat can no longer spawn tasks. F-03 (app): replace the unbounded AVATAR_HANDLE_CACHE map with a bounded, byte-equality-keyed LRU (avatar::ByteLru, cap 64) — fixes both unbounded growth from an endless stream of distinct valid avatars and the prior 64-bit-hash-collision-shows-wrong-avatar bug. Pure seams (sanitize_endpoint_addr, admit_into_roster, should_auto_fetch, ByteLru) + 6 adversarial/unit tests. 413 lib tests, clippy --all-targets clean, release build green. Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 33 +++++----- src/avatar.rs | 99 ++++++++++++++++++++++++++++++ src/core/mod.rs | 129 +++++++++++++++++++++++++++++++++++---- src/network/gossip.rs | 138 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 364 insertions(+), 35 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 1ed7cc5..46febab 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -5526,30 +5526,35 @@ fn avatar_badge<'a>(name: &str, key: &str, size: f32) -> Element<'a, AppMessage> .into() } +/// Maximum distinct avatar images we keep handles for. Each avatar is bounded to +/// 48 KiB / 256×256 at ingest, so a 64-entry LRU caps this cache at a few MB +/// regardless of how many distinct avatars peers publish over time (Tier C F-03). +const AVATAR_CACHE_CAP: usize = 64; + thread_local! { - /// Cache of avatar image handles, keyed by a hash of the PNG bytes, so the - /// SAME `image::Handle` (and thus the same GPU texture id) is reused across + /// Bounded cache of avatar image handles, keyed by PNG content, so the SAME + /// `image::Handle` (and thus the same GPU texture id) is reused across /// redraws. `image::Handle::from_bytes` mints a fresh *unique* id on every /// call, so building handles inline in `view()` made iced re-upload the /// texture on every repaint — including the redraws fired on each mouse move — - /// which showed up as constant flicker. Lives on the (single) UI thread. - static AVATAR_HANDLE_CACHE: std::cell::RefCell> = - std::cell::RefCell::new(HashMap::new()); + /// which showed up as constant flicker. A peer can publish an unbounded stream + /// of distinct valid avatars over a session, so the cache is an LRU (bounded + + /// byte-equality keyed) rather than a plain map (Tier C F-03). Lives on the + /// (single) UI thread. + static AVATAR_HANDLE_CACHE: + std::cell::RefCell> = + std::cell::RefCell::new(crate::avatar::ByteLru::new(AVATAR_CACHE_CAP)); } -/// A stable image handle for these exact PNG bytes (cached by content hash), so -/// it keeps the same id across redraws — see [`AVATAR_HANDLE_CACHE`]. +/// A stable image handle for these exact PNG bytes (cached by content), so it +/// keeps the same id across redraws — see [`AVATAR_HANDLE_CACHE`]. fn cached_image_handle(bytes: bytes::Bytes) -> iced::widget::image::Handle { - use std::hash::{Hash, Hasher}; - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - bytes.as_ref().hash(&mut hasher); - let key = hasher.finish(); AVATAR_HANDLE_CACHE.with(|cache| { cache .borrow_mut() - .entry(key) - .or_insert_with(|| iced::widget::image::Handle::from_bytes(bytes)) - .clone() + .get_or_insert(bytes.as_ref(), || { + iced::widget::image::Handle::from_bytes(bytes.clone()) + }) }) } diff --git a/src/avatar.rs b/src/avatar.rs index fc85f1f..0b4df60 100644 --- a/src/avatar.rs +++ b/src/avatar.rs @@ -185,10 +185,109 @@ pub fn initials(name: &str) -> String { } } +/// A small content-addressed LRU cache mapping image bytes to a built value +/// (e.g. an `iced` image handle), so the SAME value is reused across redraws +/// instead of rebuilt every frame. Two Tier C F-03 properties beyond a plain +/// hash map: +/// +/// 1. **Bounded** — at most `cap` entries, evicting the least-recently-used on +/// overflow, so a peer can't grow the cache without limit by publishing an +/// endless stream of distinct valid avatars. +/// 2. **Collision-safe** — a hit requires full byte equality, not just a matching +/// 64-bit hash, so a hash collision can never return a different image's value. +/// +/// Linear scan; intended for small `cap` (tens of entries). +pub struct ByteLru { + cap: usize, + /// `(content hash, content bytes, value)`; back = most recently used. + entries: Vec<(u64, Vec, V)>, +} + +impl ByteLru { + /// Create an LRU holding at most `cap` entries (`cap` is clamped to >= 1). + pub fn new(cap: usize) -> Self { + Self { cap: cap.max(1), entries: Vec::new() } + } + + /// Return the cached value for these exact `bytes`, building and inserting it + /// on a miss (evicting the least-recently-used entry once over `cap`). A hit + /// verifies full byte equality, so a 64-bit hash collision never returns the + /// wrong value. A hit also refreshes the entry's recency. + pub fn get_or_insert(&mut self, bytes: &[u8], build: impl FnOnce() -> V) -> V { + use std::hash::{Hash, Hasher}; + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + bytes.hash(&mut hasher); + let hash = hasher.finish(); + + if let Some(idx) = self + .entries + .iter() + .position(|(h, b, _)| *h == hash && b.as_slice() == bytes) + { + // LRU touch: move the hit entry to the back (most recent). + let entry = self.entries.remove(idx); + let val = entry.2.clone(); + self.entries.push(entry); + return val; + } + + let val = build(); + if self.entries.len() >= self.cap { + self.entries.remove(0); // evict least-recently-used + } + self.entries.push((hash, bytes.to_vec(), val.clone())); + val + } + + #[cfg(test)] + fn len(&self) -> usize { + self.entries.len() + } +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn byte_lru_reuses_value_for_identical_bytes() { + let mut lru: ByteLru = ByteLru::new(4); + let mut next = 0u32; + let mut build = |lru: &mut ByteLru, b: &[u8]| { + lru.get_or_insert(b, || { + next += 1; + next + }) + }; + // Same bytes → same value, built only once. + assert_eq!(build(&mut lru, b"alice"), 1); + assert_eq!(build(&mut lru, b"alice"), 1); + // Different bytes → a freshly built value. + assert_eq!(build(&mut lru, b"bob"), 2); + assert_eq!(lru.len(), 2); + } + + #[test] + fn byte_lru_evicts_least_recently_used() { + let mut lru: ByteLru = ByteLru::new(2); + let mut n = 0u32; + let mut ins = |lru: &mut ByteLru, b: &[u8]| { + lru.get_or_insert(b, || { + n += 1; + n + }) + }; + ins(&mut lru, b"a"); // -> 1 + ins(&mut lru, b"b"); // -> 2, cache = [a, b] + ins(&mut lru, b"a"); // touch a, cache = [b, a] + ins(&mut lru, b"c"); // evicts LRU (b), cache = [a, c] + assert_eq!(lru.len(), 2); + // `a` survived (recently touched) → still value 1, not rebuilt. + assert_eq!(ins(&mut lru, b"a"), 1); + // `b` was evicted → rebuilt with a new value. + assert_eq!(ins(&mut lru, b"b"), 4); + } + #[test] fn initials_takes_first_two_words() { assert_eq!(initials("Alice"), "A"); diff --git a/src/core/mod.rs b/src/core/mod.rs index a801078..86e3f96 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -744,20 +744,63 @@ async fn build_net_stack( }) } +/// Maximum number of *automatic* chat-attachment fetches in flight at once. +/// +/// Auto-fetch (inline image preview) is triggered by an untrusted peer's chat +/// message, and each fetch is a detached task that can spend up to ~60s dialing +/// and reading. Without a bound, a room insider could spam attachment-carrying +/// chat to accumulate arbitrary pending tasks/dials (Tier C F-02). When the bound +/// is reached we simply skip the auto-fetch; the descriptor still renders and the +/// user can fetch it on demand (which is not rate-limited here). +const MAX_INFLIGHT_ATTACHMENT_FETCHES: usize = 4; + +/// In-flight `(author, attachment_id)` markers for bounded, deduplicated auto- +/// fetches (Tier C F-02). Bounded by [`MAX_INFLIGHT_ATTACHMENT_FETCHES`]. +type InflightAttachments = + Arc>>; + +/// RAII bookkeeping for one bounded auto-fetch: holds the concurrency permit for +/// the task's lifetime and clears the in-flight `(author, id)` marker when the +/// fetch finishes (success OR failure), so the same image can be retried later. +struct AutoFetchGuard { + _permit: tokio::sync::OwnedSemaphorePermit, + inflight: InflightAttachments, + key: (EndpointId, crate::files::AttachmentId), +} + +impl Drop for AutoFetchGuard { + fn drop(&mut self) { + self.inflight.lock().unwrap().remove(&self.key); + } +} + +/// Whether to AUTO-fetch a chat image attachment. Only authenticated roster +/// authors qualify (closing the non-roster injection vector), and a `(author, +/// id)` already being fetched is skipped (dedup). The concurrency bound itself is +/// enforced separately by the permit. Pure → unit-testable (Tier C F-02). +fn should_auto_fetch(is_image: bool, author_in_roster: bool, already_inflight: bool) -> bool { + is_image && author_in_roster && !already_inflight +} + /// Fetch a chat attachment's bytes from `from` over the file plane in a detached /// task, then report the result to the UI via [`UiEvent::AttachmentReady`] / /// [`UiEvent::AttachmentFailed`] keyed by the attachment id. For images /// (`is_image`) the bytes are defensively re-validated (decodable + within pixel /// limits) before being handed to the renderer; an "Image" that doesn't decode is -/// reported as a failure rather than rendered. +/// reported as a failure rather than rendered. `guard` is `Some` for bounded +/// auto-fetches and `None` for user-initiated fetches; it is dropped when the +/// task ends, releasing the concurrency permit and the dedup marker. fn spawn_attachment_fetch( transport: Arc, ui_tx: mpsc::Sender, from: EndpointId, att: crate::files::ChatAttachment, is_image: bool, + guard: Option, ) { tokio::spawn(async move { + // Held for the whole fetch; dropped here on completion (Tier C F-02). + let _guard = guard; match transport.fetch_attachment(from, &att).await { Ok(data) => { if is_image && crate::files::validate_image_bytes(&data).is_none() { @@ -1813,12 +1856,24 @@ async fn run_core_loop( // presence scheduler can reach them later. let friends_events = friends.clone(); let friends_read_only_events = friends_read_only; + // Bounded, deduplicated auto-fetch of chat image attachments (Tier C + // F-02): the permit pool caps concurrent fetch tasks; the in-flight + // set dedups identical (author, id) pairs. + let attachment_limiter = + Arc::new(tokio::sync::Semaphore::new(MAX_INFLIGHT_ATTACHMENT_FETCHES)); + let inflight_attachments: InflightAttachments = + Arc::new(std::sync::Mutex::new(HashSet::new())); let event_task = tokio::spawn(async move { + // The authenticated roster for this room, maintained from the + // same sequential event stream. Only its members may trigger an + // automatic attachment fetch (Tier C F-02). + let mut roster: HashSet = HashSet::new(); while let Some(event) = room_events.recv().await { match event { RoomEvent::PeerJoined(peer_id, state) => { // A (re)join means the peer is back — cancel any // pending reconnect grace timer before re-adding it. + roster.insert(peer_id); cancel_grace_timer(&grace_timers_events, &peer_id); recovery_events.cancel(peer_id); transport_events.admit_audio_sender(peer_id); @@ -1862,6 +1917,7 @@ async fn run_core_loop( } RoomEvent::PeerLeft(peer_id) => { // Graceful leave — evict immediately. + roster.remove(&peer_id); cancel_grace_timer(&grace_timers_events, &peer_id); seen_connected_events.lock().unwrap().remove(&peer_id); // A signed Leave cancels background recovery and @@ -1913,16 +1969,47 @@ async fn run_core_loop( // without a click; non-image files wait for an explicit // FetchAttachment (the "Save" chip). The descriptor was // already filename-sanitized + size-capped on ingest. - if let Some(att) = attachment.clone() - && att.kind == crate::files::AttachmentKind::Image - { - spawn_attachment_fetch( - transport_events.clone(), - ui_tx_events.clone(), - from, - att, - true, - ); + // + // The auto path is an untrusted-peer-triggered detached + // task, so it is gated (Tier C F-02): only roster authors + // qualify, identical (author,id) pairs are deduped, and a + // permit pool caps concurrent fetch tasks. The chat TEXT + // is always forwarded (it's sanitized at the UI edge); + // only the fetch is bounded. + if let Some(att) = attachment.clone() { + let is_image = att.kind == crate::files::AttachmentKind::Image; + let key = (from, att.id); + let already_inflight = + inflight_attachments.lock().unwrap().contains(&key); + if should_auto_fetch(is_image, roster.contains(&from), already_inflight) { + // Reserve the dedup slot, then a permit. If the + // pool is exhausted, drop the auto-fetch (and the + // dedup marker) — the descriptor still shows and + // the user can fetch on demand. + inflight_attachments.lock().unwrap().insert(key); + match attachment_limiter.clone().try_acquire_owned() { + Ok(permit) => { + spawn_attachment_fetch( + transport_events.clone(), + ui_tx_events.clone(), + from, + att, + true, + Some(AutoFetchGuard { + _permit: permit, + inflight: inflight_attachments.clone(), + key, + }), + ); + } + Err(_) => { + inflight_attachments.lock().unwrap().remove(&key); + crate::log_msg( + "Chat attachment auto-fetch limit reached; skipping (fetch on demand)", + ); + } + } + } } let _ = ui_tx_events.send(UiEvent::ChatMessage { from: from.to_string(), @@ -2467,12 +2554,15 @@ async fn run_core_loop( CoreCommand::FetchAttachment { from, attachment } => { if let Some(session) = &active_session { let is_image = attachment.kind == crate::files::AttachmentKind::Image; + // User-initiated (the "Save" chip): not bounded here — a human + // click rate-limits it. The auto path (F-02) passes a guard. spawn_attachment_fetch( session.transport.clone(), ui_tx.clone(), from, attachment, is_image, + None, ); } } @@ -2576,10 +2666,23 @@ async fn run_core_loop( mod tests { use super::{ apply_volume, audio_datagram_len_ok, frame_level, mix_frames, mix_stereo_frames, - next_game_change, stereo_to_mono, KnownPeers, MicLevelMeter, PeerSpeakTicket, - MAX_OPUS_PAYLOAD, MIC_LEVEL_REPORT_SAMPLES, + next_game_change, should_auto_fetch, stereo_to_mono, KnownPeers, MicLevelMeter, + PeerSpeakTicket, MAX_OPUS_PAYLOAD, MIC_LEVEL_REPORT_SAMPLES, }; + #[test] + fn auto_fetch_only_for_roster_images_not_already_inflight() { + // The happy path: a roster author's brand-new image attachment. + assert!(should_auto_fetch(true, true, false)); + // A non-image (generic file) never auto-fetches — it waits for "Save". + assert!(!should_auto_fetch(false, true, false)); + // A non-roster author (e.g. a sock puppet that never announced) is rejected, + // closing the F-02 unbounded-task vector. + assert!(!should_auto_fetch(true, false, false)); + // An identical (author,id) already being fetched is deduped. + assert!(!should_auto_fetch(true, true, true)); + } + #[test] fn retained_peers_are_shared_by_topic_across_restamped_tickets() { let topic_id = [23u8; 32]; diff --git a/src/network/gossip.rs b/src/network/gossip.rs index c017a8a..8f6c3ec 100644 --- a/src/network/gossip.rs +++ b/src/network/gossip.rs @@ -1,11 +1,11 @@ use crate::network::{RoomState, NetError, PeerState, RoomEvent, PeerSpeakTicket}; -use iroh::{Endpoint, EndpointAddr, EndpointId, SecretKey, Signature}; +use iroh::{Endpoint, EndpointAddr, EndpointId, SecretKey, Signature, TransportAddr}; use iroh_gossip::net::Gossip; use iroh_gossip::proto::TopicId; use tokio::sync::mpsc; use tokio::sync::mpsc::Receiver; use std::sync::{Arc, Mutex}; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeSet, HashMap, HashSet}; use async_trait::async_trait; use tokio_stream::StreamExt; use serde::{Serialize, Deserialize}; @@ -131,6 +131,57 @@ fn admit_state_mutation( true } +/// 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 +/// proves ownership of the generated keypair it was made with, not that the +/// author is a distinct human. A malicious member can therefore mint many valid +/// signed identities. Voice is full-mesh (each peer dials every other), so a real +/// room is realistically well under this bound; the cap exists purely so a flood +/// of sock-puppet `Announce`s can't grow our peer map / audio supervisors / dials +/// without limit (Tier C F-01). +const MAX_ACTIVE_PEERS: usize = 32; + +/// Maximum transport addresses we retain from a single peer announce. iroh +/// normally advertises a handful (a few LAN/WAN IP candidates plus one home +/// relay); the cap stops an insider stuffing a large unique address set into each +/// announce to inflate the address lookup and the dialer's candidate list. +const MAX_PEER_ADDRS: usize = 8; + +/// Maximum byte length of a relay URL we accept inside a peer address. A relay +/// URL is normal-length; anything longer is dropped rather than retained. +const MAX_RELAY_URL_LEN: usize = 256; + +/// Bound an untrusted peer's advertised address set before we retain it / hand it +/// to the address lookup and dialer (Tier C F-01). Drops transport kinds we never +/// use (`Custom`) and over-long relay URLs, then truncates to at most +/// [`MAX_PEER_ADDRS`] addresses. `BTreeSet` iteration is deterministic, so the +/// kept subset is stable. Pure → unit-testable. +fn sanitize_endpoint_addr(addr: &EndpointAddr) -> EndpointAddr { + let addrs: BTreeSet = addr + .addrs + .iter() + .filter(|a| match a { + TransportAddr::Relay(url) => url.as_str().len() <= MAX_RELAY_URL_LEN, + TransportAddr::Ip(_) => true, + // `TransportAddr` is #[non_exhaustive]; we only speak IP + relay, so + // anything else (Custom / future kinds) is dropped, not retained. + _ => false, + }) + .take(MAX_PEER_ADDRS) + .cloned() + .collect(); + 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). +/// Pure → unit-testable. +fn admit_into_roster(roster_len: usize, is_new: bool, max_peers: usize) -> bool { + !is_new || roster_len < max_peers +} + fn peer_state_for_log(state: &PeerState) -> String { format!( "name={:?}, muted={}, addr_id={}, addrs={}, sharing={}, game={:?}", @@ -436,15 +487,34 @@ impl RoomState for IrohGossipState { let cleaned = crate::sanitize::sanitize_game_label(&g); (!cleaned.is_empty()).then_some(cleaned) }); + // Bound an insider's advertised address set + // 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); - let (is_new, state_changed) = { + let admitted = { let mut peer_map = peers.lock().unwrap(); let is_new = !peer_map.contains_key(&payload.author); - let state_changed = peer_map.get(&payload.author) != Some(&state); - if is_new || state_changed { - peer_map.insert(payload.author, state.clone()); + // 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) { + None + } else { + let state_changed = peer_map.get(&payload.author) != Some(&state); + if is_new || state_changed { + peer_map.insert(payload.author, state.clone()); + } + Some((is_new, state_changed)) } - (is_new, state_changed) + }; + let Some((is_new, state_changed)) = admitted else { + crate::log_msg(&format!( + "Gossip roster full ({MAX_ACTIVE_PEERS}); rejecting new peer {}", + crate::short_id(&payload.author.to_string()) + )); + continue; }; if is_new { @@ -453,7 +523,12 @@ impl RoomState for IrohGossipState { crate::short_id(&payload.author.to_string()), peer_state_for_log(&state) )); - address_lookup.add_endpoint_info(state.addr.clone()); + // Replace (not union) the lookup's record for + // this id with the authenticated, sanitized + // address set, so leave/re-announce cycles + // can't accumulate attacker-supplied history + // (Tier C F-01). + let _ = address_lookup.set_endpoint_info(state.addr.clone()); let _ = event_tx.send(RoomEvent::PeerJoined(payload.author, state)).await; } else if state_changed { crate::log_msg(&format!( @@ -768,6 +843,53 @@ mod tests { assert!(!bootstrap.contains(&me)); } + #[test] + fn admit_into_roster_caps_new_authors_but_not_updates() { + // New authors are admitted while there's room... + assert!(admit_into_roster(0, true, 3)); + assert!(admit_into_roster(2, true, 3)); + // ...rejected once the roster is full... + assert!(!admit_into_roster(3, true, 3)); + assert!(!admit_into_roster(10, true, 3)); + // ...but an existing peer's update always passes, even at/over the cap. + assert!(admit_into_roster(3, false, 3)); + assert!(admit_into_roster(99, false, 3)); + } + + #[test] + fn sanitize_endpoint_addr_caps_address_count() { + use std::net::SocketAddr; + let id = fresh_id(); + // An insider stuffs far more addresses than MAX_PEER_ADDRS into one announce. + let many: Vec = (0..(MAX_PEER_ADDRS as u16 + 50)) + .map(|i| TransportAddr::Ip(SocketAddr::from(([127, 0, 0, 1], 1000 + i)))) + .collect(); + let addr = EndpointAddr::from_parts(id, many); + let out = sanitize_endpoint_addr(&addr); + assert_eq!(out.id, id); + assert_eq!(out.addrs.len(), MAX_PEER_ADDRS); + } + + #[test] + fn sanitize_endpoint_addr_drops_overlong_relay_url() { + use std::str::FromStr; + let id = fresh_id(); + let short = iroh::RelayUrl::from_str("https://relay.example/").unwrap(); + let long = iroh::RelayUrl::from_str(&format!( + "https://relay.example/{}", + "a".repeat(MAX_RELAY_URL_LEN) + )) + .unwrap(); + assert!(long.as_str().len() > MAX_RELAY_URL_LEN); + let addr = EndpointAddr::from_parts( + id, + [TransportAddr::Relay(short.clone()), TransportAddr::Relay(long)], + ); + let out = sanitize_endpoint_addr(&addr); + let relays: Vec<_> = out.relay_urls().cloned().collect(); + assert_eq!(relays, vec![short], "over-long relay URL must be dropped"); + } + #[test] fn test_gossip_message_leave_round_trip() { let original = GossipMessage::Leave;