From fe627166d51d455d0809f8eb130d83c3e19cd274 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 13 Jun 2026 21:16:24 -0400 Subject: [PATCH] fix(security): close S3 (arg-injection), S4 (presence-name), S1 (panic-slice) Three findings from the first security pass: - S3 (Medium): the peer-supplied screen-share ticket was passed to pixelpass as the first positional CLI arg with no end-of-options guard, so a ticket starting with `-`/`--` could be reinterpreted as a flag (argument injection). New pure `viewer_args()` puts flags first, then a `--` guard, then the ticket positionally; spawn_viewer uses it. +2 tests. - S4 (Medium): peer presence display-names (gossip `Announce`, untrusted and spoofable) were rendered unsanitized/unbounded, unlike the chat path. New `sanitize::sanitize_name` strips bidi/zero-width format chars + control chars, collapses whitespace, and caps at 48 chars; applied at the gossip ingest point so every consumer gets a safe value. +4 tests. - S1 (Low): `&id[..8]` byte-slices could panic on a short/non-ASCII id. New panic-free `short_id()` (char-based take) replaces both slices. +1 test. 158 lib tests (was 151), clippy --all-targets clean. Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 24 +++++++++++-- src/lib.rs | 1 + src/network/gossip.rs | 6 +++- src/sanitize.rs | 82 ++++++++++++++++++++++++++++++++++++++++++ src/screenshare/mod.rs | 41 +++++++++++++++++++-- 5 files changed, 148 insertions(+), 6 deletions(-) create mode 100644 src/sanitize.rs diff --git a/src/app/mod.rs b/src/app/mod.rs index b11ec1a..9f5b109 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -871,6 +871,14 @@ fn format_duration(total_secs: u64) -> String { } } +/// First 8 characters of an id string for compact display. Panic-free: takes +/// chars (not a byte slice), so a short or non-ASCII id can never panic the +/// render (security finding S1) — ids are long ASCII hex today, but this guards +/// the slice regardless. +fn short_id(id: &str) -> String { + id.chars().take(8).collect() +} + /// Max characters kept for a single chat message after sanitizing. const CHAT_MSG_MAX_CHARS: usize = 2000; @@ -1420,7 +1428,7 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { container(text("")).padding(0) }, horizontal_space(), - text(format!("My ID: {}", &state.self_id[..8])) + text(format!("My ID: {}", short_id(&state.self_id))) .size(14) .color(color_subtext), button( @@ -1596,7 +1604,7 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { row![ column![ text(&peer.name).size(16).color(color_text), - text(format!("ID: {}", &peer_id.to_string()[..8])).size(11).color(color_subtext) + text(format!("ID: {}", short_id(&peer_id.to_string()))).size(11).color(color_subtext) ], horizontal_space(), share_el, @@ -2790,6 +2798,18 @@ mod tests { assert_eq!(sanitize_chat(&long).chars().count(), CHAT_MSG_MAX_CHARS); } + #[test] + fn short_id_is_panic_free_on_short_and_unicode_ids() { + use super::short_id; + // Normal long hex id → first 8 chars. + assert_eq!(short_id("abcdef0123456789"), "abcdef01"); + // Shorter-than-8 id must not panic (the old `[..8]` slice would). + assert_eq!(short_id("abc"), "abc"); + assert_eq!(short_id(""), ""); + // Multi-byte chars: take 8 *chars*, never split a byte boundary. + assert_eq!(short_id("héllo 世界 more"), "héllo 世界"); + } + #[test] fn divider_clamps_are_finite_on_a_tiny_window() { // A window smaller than the reserves must not produce NaN/inverted ranges. diff --git a/src/lib.rs b/src/lib.rs index b8077e4..c85a211 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,6 +7,7 @@ pub mod config; pub mod theme; pub mod notify; pub mod screenshare; +pub mod sanitize; use std::path::PathBuf; use std::sync::OnceLock; diff --git a/src/network/gossip.rs b/src/network/gossip.rs index faab78c..23ffb01 100644 --- a/src/network/gossip.rs +++ b/src/network/gossip.rs @@ -144,7 +144,11 @@ impl RoomState for IrohGossipState { crate::log_msg(&format!("Gossip Event::Received from author={:?}, payload={:?}", payload.author, payload.msg)); match payload.msg { - GossipMessage::Announce(state) => { + GossipMessage::Announce(mut state) => { + // Presence names are untrusted (and author- + // spoofable): sanitize at ingest so every + // consumer gets a safe value (security S4). + state.name = crate::sanitize::sanitize_name(&state.name); let (is_new, state_changed) = { let mut peer_map = peers.lock().unwrap(); let is_new = !peer_map.contains_key(&payload.author); diff --git a/src/sanitize.rs b/src/sanitize.rs new file mode 100644 index 0000000..6d42ad9 --- /dev/null +++ b/src/sanitize.rs @@ -0,0 +1,82 @@ +//! Sanitizers for untrusted peer-supplied strings shared across layers. +//! +//! Peer display names ride the gossip presence plane (`PeerState.name`), which is +//! untrusted and spoofable, yet they're rendered directly in the roster. This +//! module cleans a name at the gossip ingest point so every downstream consumer +//! gets a safe value (security finding S4). Chat text has its own sanitizer in +//! the UI layer (`app::sanitize_chat`). + +/// Max characters kept for a peer's display name after sanitizing. Names are +/// short labels, so a tight cap both prevents UI/layout/memory abuse and keeps +/// the roster readable. +pub const NAME_MAX_CHARS: usize = 48; + +/// Unicode *format* characters (`General_Category=Cf`) that can spoof or garble a +/// rendered name even though they are NOT caught by [`char::is_control`]: +/// bidirectional overrides/isolates (text-direction spoofing) and +/// zero-width / BOM characters (invisible, can hide or fake content). Listed +/// explicitly so the sanitizer stays dependency-free (std exposes no category +/// query). Stripped outright rather than replaced. +fn is_spoofing_format_char(c: char) -> bool { + matches!(c, + '\u{200B}'..='\u{200F}' // zero-width space, ZWNJ, ZWJ, LRM, RLM + | '\u{202A}'..='\u{202E}' // LRE, RLE, PDF, LRO, RLO (bidi overrides) + | '\u{2060}'..='\u{2064}' // word joiner .. invisible plus + | '\u{2066}'..='\u{2069}' // LRI, RLI, FSI, PDI (bidi isolates) + | '\u{FEFF}' // BOM / zero-width no-break space + ) +} + +/// Sanitize an untrusted peer display name for safe rendering. Strips bidi / +/// zero-width format characters, turns control characters into spaces, collapses +/// any whitespace run to a single space, trims the ends, and caps the length at +/// [`NAME_MAX_CHARS`]. Returns `""` if nothing usable remains (callers may +/// substitute a placeholder such as a short id). +pub fn sanitize_name(input: &str) -> String { + let cleaned: String = input + .chars() + .filter(|c| !is_spoofing_format_char(*c)) + .map(|c| if c.is_control() { ' ' } else { c }) + .collect(); + let collapsed = cleaned.split_whitespace().collect::>().join(" "); + collapsed.chars().take(NAME_MAX_CHARS).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn keeps_ordinary_names_including_unicode() { + assert_eq!(sanitize_name("Alice"), "Alice"); + assert_eq!(sanitize_name("héllo 世界"), "héllo 世界"); + // Emoji are not format/control chars — kept. + assert_eq!(sanitize_name("🎙 Mike"), "🎙 Mike"); + } + + #[test] + fn strips_control_chars_and_collapses_whitespace() { + // NUL, CR/LF, TAB, and ANSI ESC are control chars → become spaces, then + // collapse; ends trim. + assert_eq!(sanitize_name(" a\u{0}b\r\nc\td\u{1b}[31m "), "a b c d [31m"); + // A name that is only control/whitespace cleans to empty. + assert_eq!(sanitize_name("\u{0}\r\n\t "), ""); + } + + #[test] + fn strips_bidi_and_zero_width_format_chars() { + // Right-to-left override (classic name-spoof char) is removed entirely. + assert_eq!(sanitize_name("admin\u{202E}txt"), "admintxt"); + // Zero-width space / joiner / BOM removed. + assert_eq!(sanitize_name("ab\u{200B}cd"), "abcd"); + assert_eq!(sanitize_name("\u{FEFF}name"), "name"); + // Bidi isolates removed. + assert_eq!(sanitize_name("x\u{2066}y\u{2069}z"), "xyz"); + } + + #[test] + fn caps_length() { + let long = "n".repeat(NAME_MAX_CHARS + 500); + assert_eq!(sanitize_name(&long).chars().count(), NAME_MAX_CHARS); + } +} diff --git a/src/screenshare/mod.rs b/src/screenshare/mod.rs index a30bf37..96db489 100644 --- a/src/screenshare/mod.rs +++ b/src/screenshare/mod.rs @@ -93,6 +93,21 @@ fn json_u32(v: &serde_json::Value, key: &str) -> u32 { v.get(key).and_then(|x| x.as_u64()).unwrap_or(0) as u32 } +/// Build the argv for a pixelpass *viewer*. The `ticket` is peer-supplied (it +/// rides gossip presence, which is untrusted and spoofable), so flags come first +/// and the ticket is passed as a positional **after a `--` end-of-options +/// guard**. That guard means a ticket beginning with `-`/`--` can never be +/// reinterpreted as a pixelpass flag — closing the argument-injection vector +/// (security finding S3). Pure: no I/O. +pub fn viewer_args(ticket: &str) -> Vec { + vec![ + "--output".to_string(), + "json".to_string(), + "--".to_string(), + ticket.to_string(), + ] +} + /// Resolve the pixelpass binary: an explicit config override (used only if it /// points at an existing file), otherwise the first `pixelpass` found on /// `$PATH`. `None` means it isn't installed — a normal, handled state. An @@ -172,9 +187,7 @@ pub async fn spawn_host(bin: &Path) -> std::io::Result<(Child, String)> { /// player window closes (its tunnel ends). pub async fn spawn_viewer(bin: &Path, ticket: &str) -> std::io::Result { let mut child = Command::new(bin) - .arg(ticket) - .arg("--output") - .arg("json") + .args(viewer_args(ticket)) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::null()) @@ -305,6 +318,28 @@ fn spawn_player(bin: &str, args: &[&str], url: &str) -> std::io::Result { mod tests { use super::*; + #[test] + fn viewer_args_guard_neutralizes_flag_like_ticket() { + // A malicious "ticket" that looks like a flag must end up positional, + // i.e. after the `--` end-of-options guard, never parsed as a flag. + let args = viewer_args("--malicious-flag"); + assert_eq!(args.last().unwrap(), "--malicious-flag", "ticket is last"); + let guard = args.iter().position(|a| a == "--").expect("`--` guard present"); + let ticket = args.len() - 1; + assert!(guard < ticket, "ticket must follow the `--` guard"); + // The real flags are parsed before the guard. + let out = args.iter().position(|a| a == "--output").unwrap(); + assert!(out < guard, "flags precede the `--` guard"); + } + + #[test] + fn viewer_args_ordinary_ticket() { + assert_eq!( + viewer_args("abc123"), + vec!["--output", "json", "--", "abc123"] + ); + } + #[test] fn parses_ticket() { assert_eq!(