From 7f3704e8908ee3fdea5d9c01d03c435e99801cd8 Mon Sep 17 00:00:00 2001 From: Andy Dixon Date: Tue, 13 Oct 2026 06:46:38 +0100 Subject: [PATCH] fix(irc): linear OSC stripping, bounded joins and CAP state, no cleartext passwords - ircformat: an unterminated OSC made the stripper advance two bytes and rescan the rest, so one hostile 1 MiB line of ESC ] froze the UI thread for minutes. It now stops at the first OSC with no terminator (none can follow), which is linear. - Server lines are capped at 64 KiB (512 bytes + 8 KiB of tags is the real ceiling) instead of 1 MiB. - Spoofed self-JOINs can't grow channel state past 500 rooms (they were also persisted and auto-joined next time); CAP LS is ignored after registration and its gathered list is capped. - PASS, SASL PLAIN and NickServ IDENTIFY are only sent over TLS, an SSH tunnel or a loopback server; otherwise the Server tab says why. - TcpConn: a local close no longer races the reader into reporting a spurious read error (regression from 00f110f, which shut the socket down before taking the stream). --- rust/core/src/clients/irc.rs | 75 +++++++++++++++++++++++++++++++++--- rust/core/src/clients/mod.rs | 8 +++- rust/core/src/ircformat.rs | 16 ++++++-- 3 files changed, 87 insertions(+), 12 deletions(-) diff --git a/rust/core/src/clients/irc.rs b/rust/core/src/clients/irc.rs index acd97d9..68afce8 100644 --- a/rust/core/src/clients/irc.rs +++ b/rust/core/src/clients/irc.rs @@ -24,7 +24,12 @@ const READ_POLL: Duration = Duration::from_millis(50); const PING_DELAY: Duration = Duration::from_secs(20); const PING_TIMEOUT: Duration = Duration::from_secs(60); const PING_GRACE: Duration = Duration::from_secs(30); -const MAX_LINE_BYTES: usize = 1 << 20; +/// IRC lines are 512 bytes plus up to 8 KiB of IRCv3 tags; anything far longer is hostile. +const MAX_LINE_BYTES: usize = 64 << 10; +/// Channels the client keeps state for; a server can't grow it past this with spoofed self-JOINs. +const MAX_CHANNELS: usize = 500; +/// Capability names gathered from a multi-line CAP LS. +const MAX_CAPS: usize = 1000; /// Users tracked per channel; a hostile server can't grow the client past this. const MAX_CHANNEL_USERS: usize = 10_000; @@ -563,7 +568,7 @@ impl Client { }; // IRCv3 capability negotiation; servers without it ignore the command and register as usual. write(&mut stream, "CAP LS 302")?; - if !self.cfg.server_pass.is_empty() { + if !self.cfg.server_pass.is_empty() && self.credentials_allowed("server password") { write(&mut stream, &encode("PASS", &[&self.cfg.server_pass]))?; } write(&mut stream, &encode("NICK", &[&self.cfg.nick]))?; @@ -720,8 +725,23 @@ impl Client { } } + /// Passwords (PASS, SASL PLAIN, NickServ) only go over an encrypted or local connection: + /// TLS, an SSH tunnel, or a loopback server. Otherwise they're held back and the Server tab + /// says why, rather than putting the password on the wire in the clear. + fn credentials_allowed(&self, what: &str) -> bool { + if sends_credentials_safely(&self.cfg) { + return true; + } + self.emit(Event { + kind: "error".into(), + text: format!("{what} not sent: the connection isn't encrypted (turn on TLS or an SSH tunnel)"), + ..Default::default() + }); + false + } + fn identify_with_nickserv(&self) { - if !self.cfg.nickserv_identify || self.cfg.nickserv_password.is_empty() { + if !self.cfg.nickserv_identify || self.cfg.nickserv_password.is_empty() || !self.credentials_allowed("NickServ identify") { return; } let body = if self.cfg.nickserv_account.is_empty() { @@ -781,16 +801,22 @@ impl Client { fn handle_cap(&self, m: &Message) { let sub = m.param(1).to_ascii_uppercase(); let caps: Vec<&str> = m.last().split_whitespace().map(|c| c.split('=').next().unwrap_or(c)).collect(); - let want_sasl = !self.cfg.sasl_account.is_empty(); + let want_sasl = !self.cfg.sasl_account.is_empty() && self.credentials_allowed("SASL login"); + if self.st().connected && sub == "LS" { + return; // negotiation is over; a late LS can only grow state + } match sub.as_str() { // A multi-line LS list ("CAP * LS * :…") continues; act on the last line. "LS" if m.params.len() >= 4 && m.param(2) == "*" => { - self.st().caps_offered.extend(caps.iter().map(|c| c.to_string())); + let mut st = self.st(); + let room = MAX_CAPS.saturating_sub(st.caps_offered.len()); + st.caps_offered.extend(caps.iter().take(room).map(|c| c.to_string())); } "LS" => { let offered: Vec = { let mut st = self.st(); - st.caps_offered.extend(caps.iter().map(|c| c.to_string())); + let room = MAX_CAPS.saturating_sub(st.caps_offered.len()); + st.caps_offered.extend(caps.iter().take(room).map(|c| c.to_string())); std::mem::take(&mut st.caps_offered) }; let mut req = vec![]; @@ -832,6 +858,9 @@ impl Client { if !me && !self.tracks(&room) { return; } + if me && !self.tracks(&room) && self.st().channels.len() >= MAX_CHANNELS { + return; + } if !from.is_empty() { let mut st = self.st(); let ch = st.channels.entry(fold(&room)).or_insert_with(|| Channel { name: room.clone(), topic: String::new(), users: Vec::new() }); @@ -1155,9 +1184,43 @@ pub fn fuzz_lines(data: &[u8]) { } } +/// See [`Client::credentials_allowed`]. +fn sends_credentials_safely(cfg: &Config) -> bool { + let h = cfg.server.trim_start_matches('[').trim_end_matches(']'); + cfg.use_tls || cfg.connector.is_some() || h.eq_ignore_ascii_case("localhost") || h.parse::().is_ok_and(|ip| ip.is_loopback()) +} + #[cfg(test)] mod tests { use super::*; + + #[test] + fn credentials_need_tls_a_tunnel_or_loopback() { + let c = |server: &str, use_tls: bool| Config { server: server.into(), use_tls, ..Default::default() }; + assert!(!sends_credentials_safely(&c("irc.libera.chat", false))); + assert!(sends_credentials_safely(&c("irc.libera.chat", true))); + assert!( + sends_credentials_safely(&c("127.0.0.1", false)) && sends_credentials_safely(&c("localhost", false)) && sends_credentials_safely(&c("::1", false)) + ); + let tunnelled = Config { connector: Some(Arc::new(|| Err("unused".into()))), ..c("irc.example", false) }; + assert!(sends_credentials_safely(&tunnelled)); + } + + #[test] + fn spoofed_self_joins_and_cap_floods_are_bounded() { + let client = Client::new(Config::default()); + client.st().nick = "me".into(); + let feed = |l: &str| client.handle_message(&parse_line(l).unwrap()); + for i in 0..(MAX_CHANNELS + 200) { + feed(&format!(":me!u@h JOIN #r{i}")); + } + assert_eq!(client.st().channels.len(), MAX_CHANNELS); + assert_eq!(client.st().room_names.len(), MAX_CHANNELS); + for _ in 0..50 { + feed(&format!(":srv CAP * LS * :{}", "x ".repeat(100))); + } + assert!(client.st().caps_offered.len() <= MAX_CAPS); + } use std::io::{BufRead, BufReader}; use std::net::TcpListener; diff --git a/rust/core/src/clients/mod.rs b/rust/core/src/clients/mod.rs index fad40db..eab7ae7 100644 --- a/rust/core/src/clients/mod.rs +++ b/rust/core/src/clients/mod.rs @@ -108,6 +108,8 @@ pub(crate) struct TcpConn { /// A second handle on the socket, so `close` can shut it down without waiting for the /// stream lock (a write blocked on a peer that stopped reading holds that lock). shutdown: parking_lot::Mutex>, + /// Set before the socket is shut down, so the reader doesn't report our own close as an error. + closing: std::sync::atomic::AtomicBool, on_error: ErrorFn, on_close: CloseFn, } @@ -118,7 +120,7 @@ const WRITE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); impl TcpConn { pub(crate) fn new(on_error: ErrorFn, on_close: CloseFn) -> TcpConn { - TcpConn { stream: parking_lot::Mutex::new(None), shutdown: parking_lot::Mutex::new(None), on_error, on_close } + TcpConn { stream: parking_lot::Mutex::new(None), shutdown: parking_lot::Mutex::new(None), closing: Default::default(), on_error, on_close } } pub(crate) fn connect(self: &Arc, stream: std::net::TcpStream, mut on_read: impl FnMut(&[u8]) + Send + 'static) -> crate::Result<()> { @@ -130,6 +132,7 @@ impl TcpConn { return Err("already connected".into()); } *self.shutdown.lock() = stream.try_clone().ok(); + self.closing.store(false, std::sync::atomic::Ordering::SeqCst); *guard = Some(stream); } let me = self.clone(); @@ -147,7 +150,7 @@ impl TcpConn { Err(e) => e.to_string(), }; // A local close also ends the read; Go reported that as an error too, we don't. - if me.is_connected() { + if me.is_connected() && !me.closing.load(std::sync::atomic::Ordering::SeqCst) { (me.on_error)(format!("read error: {err}")); me.close(); } @@ -170,6 +173,7 @@ impl TcpConn { } pub(crate) fn close(&self) { + self.closing.store(true, std::sync::atomic::Ordering::SeqCst); if let Some(s) = self.shutdown.lock().take() { let _ = s.shutdown(std::net::Shutdown::Both); } diff --git a/rust/core/src/ircformat.rs b/rust/core/src/ircformat.rs index 681bd82..116c9a0 100644 --- a/rust/core/src/ircformat.rs +++ b/rust/core/src/ircformat.rs @@ -224,10 +224,10 @@ fn strip_terminal_escapes(value: &str) -> String { s.push_str(&rest[..pos]); rest = &body[end..]; } - None => { - s.push_str(&rest[..pos + 2]); - rest = body; - } + // No terminator anywhere after this OSC, so none after any later OSC either: + // keep the rest as is. (Advancing two bytes and searching again was quadratic, and + // one hostile 1 MiB line of "\x1b]" froze the UI for minutes.) + None => break, } } s.push_str(rest); @@ -425,6 +425,14 @@ mod tests { assert_eq!(plain_text("\x02bold\x02 \x0304red\x03"), "bold red"); } + #[test] + fn unterminated_osc_runs_are_linear() { + let start = std::time::Instant::now(); + let out = plain_text(&"\x1b]".repeat(500_000)); + assert!(start.elapsed() < std::time::Duration::from_secs(2), "{:?}", start.elapsed()); + assert!(!out.contains('\x1b')); + } + #[test] fn untrusted_terminal_sequences_are_removed() { assert_eq!(plain_text("before\x1b[31munsafe\x1b]0;title\x07after"), "beforeunsafeafter");