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).
This commit is contained in:
Andy Dixon
2026-10-13 06:46:38 +01:00
parent d6f8f3306b
commit 7f3704e890
3 changed files with 87 additions and 12 deletions
+69 -6
View File
@@ -24,7 +24,12 @@ const READ_POLL: Duration = Duration::from_millis(50);
const PING_DELAY: Duration = Duration::from_secs(20); const PING_DELAY: Duration = Duration::from_secs(20);
const PING_TIMEOUT: Duration = Duration::from_secs(60); const PING_TIMEOUT: Duration = Duration::from_secs(60);
const PING_GRACE: Duration = Duration::from_secs(30); 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. /// Users tracked per channel; a hostile server can't grow the client past this.
const MAX_CHANNEL_USERS: usize = 10_000; 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. // IRCv3 capability negotiation; servers without it ignore the command and register as usual.
write(&mut stream, "CAP LS 302")?; 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("PASS", &[&self.cfg.server_pass]))?;
} }
write(&mut stream, &encode("NICK", &[&self.cfg.nick]))?; 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) { 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; return;
} }
let body = if self.cfg.nickserv_account.is_empty() { let body = if self.cfg.nickserv_account.is_empty() {
@@ -781,16 +801,22 @@ impl Client {
fn handle_cap(&self, m: &Message) { fn handle_cap(&self, m: &Message) {
let sub = m.param(1).to_ascii_uppercase(); 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 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() { match sub.as_str() {
// A multi-line LS list ("CAP * LS * :…") continues; act on the last line. // A multi-line LS list ("CAP * LS * :…") continues; act on the last line.
"LS" if m.params.len() >= 4 && m.param(2) == "*" => { "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" => { "LS" => {
let offered: Vec<String> = { let offered: Vec<String> = {
let mut st = self.st(); 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) std::mem::take(&mut st.caps_offered)
}; };
let mut req = vec![]; let mut req = vec![];
@@ -832,6 +858,9 @@ impl Client {
if !me && !self.tracks(&room) { if !me && !self.tracks(&room) {
return; return;
} }
if me && !self.tracks(&room) && self.st().channels.len() >= MAX_CHANNELS {
return;
}
if !from.is_empty() { if !from.is_empty() {
let mut st = self.st(); 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() }); 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::<std::net::IpAddr>().is_ok_and(|ip| ip.is_loopback())
}
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; 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::io::{BufRead, BufReader};
use std::net::TcpListener; use std::net::TcpListener;
+6 -2
View File
@@ -108,6 +108,8 @@ pub(crate) struct TcpConn {
/// A second handle on the socket, so `close` can shut it down without waiting for the /// 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). /// stream lock (a write blocked on a peer that stopped reading holds that lock).
shutdown: parking_lot::Mutex<Option<std::net::TcpStream>>, shutdown: parking_lot::Mutex<Option<std::net::TcpStream>>,
/// 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_error: ErrorFn,
on_close: CloseFn, on_close: CloseFn,
} }
@@ -118,7 +120,7 @@ const WRITE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
impl TcpConn { impl TcpConn {
pub(crate) fn new(on_error: ErrorFn, on_close: CloseFn) -> 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<Self>, stream: std::net::TcpStream, mut on_read: impl FnMut(&[u8]) + Send + 'static) -> crate::Result<()> { pub(crate) fn connect(self: &Arc<Self>, 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()); return Err("already connected".into());
} }
*self.shutdown.lock() = stream.try_clone().ok(); *self.shutdown.lock() = stream.try_clone().ok();
self.closing.store(false, std::sync::atomic::Ordering::SeqCst);
*guard = Some(stream); *guard = Some(stream);
} }
let me = self.clone(); let me = self.clone();
@@ -147,7 +150,7 @@ impl TcpConn {
Err(e) => e.to_string(), Err(e) => e.to_string(),
}; };
// A local close also ends the read; Go reported that as an error too, we don't. // 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.on_error)(format!("read error: {err}"));
me.close(); me.close();
} }
@@ -170,6 +173,7 @@ impl TcpConn {
} }
pub(crate) fn close(&self) { pub(crate) fn close(&self) {
self.closing.store(true, std::sync::atomic::Ordering::SeqCst);
if let Some(s) = self.shutdown.lock().take() { if let Some(s) = self.shutdown.lock().take() {
let _ = s.shutdown(std::net::Shutdown::Both); let _ = s.shutdown(std::net::Shutdown::Both);
} }
+12 -4
View File
@@ -224,10 +224,10 @@ fn strip_terminal_escapes(value: &str) -> String {
s.push_str(&rest[..pos]); s.push_str(&rest[..pos]);
rest = &body[end..]; rest = &body[end..];
} }
None => { // No terminator anywhere after this OSC, so none after any later OSC either:
s.push_str(&rest[..pos + 2]); // keep the rest as is. (Advancing two bytes and searching again was quadratic, and
rest = body; // one hostile 1 MiB line of "\x1b]" froze the UI for minutes.)
} None => break,
} }
} }
s.push_str(rest); s.push_str(rest);
@@ -425,6 +425,14 @@ mod tests {
assert_eq!(plain_text("\x02bold\x02 \x0304red\x03"), "bold red"); 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] #[test]
fn untrusted_terminal_sequences_are_removed() { fn untrusted_terminal_sequences_are_removed() {
assert_eq!(plain_text("before\x1b[31munsafe\x1b]0;title\x07after"), "beforeunsafeafter"); assert_eq!(plain_text("before\x1b[31munsafe\x1b]0;title\x07after"), "beforeunsafeafter");