From 0bcfe6470b66dffa37ba1dfef6b34d6e43fb6954 Mon Sep 17 00:00:00 2001 From: Andy Dixon Date: Tue, 13 Oct 2026 00:42:17 +0100 Subject: [PATCH] fix(ssh): linear host-pattern matching, per-hop forwarding gates, loopback remote forwards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - known_hosts: the @cert-authority pattern matcher was recursive and backtracking; a pattern of a few hundred thousand stars overflowed the connect thread's stack on every handshake and '*a*a*a…b' went exponential. It is now the iterative two-pointer match, and patterns over 1 KiB are refused on import and in Trust a CA. - Agent-forward and remote-forward channels are only honoured on the final hop: a jump host could previously open auth-agent channels and reach remote-forward destinations on the client machine. - Remote forwards with no bind address listen on the server's loopback (as ssh -R does) instead of every interface. - Keyboard-interactive gives up after 10 rounds; dials through jump hosts and run_command have timeouts; the handshake timeout leaves room for the host-key prompt so a slow reader isn't timed out mid-decision. - Known hosts are looked up per key type, so an imported known_hosts with ed25519 and RSA rows no longer produces a false 'key changed' warning. - SFTP download suggests a sanitised base name in the save dialog. --- docs/user-manual.md | 2 +- rust/app/src/sftp.rs | 4 +- rust/core/src/clients/ssh.rs | 62 +++++++++++++++++++-------- rust/core/src/known_hosts.rs | 58 ++++++++++++++++++++++--- rust/core/src/service.rs | 2 +- rust/core/src/sessions/credentials.rs | 2 +- rust/core/src/sessions/mod.rs | 2 +- rust/core/src/storage.rs | 38 ++++++++++++++-- 8 files changed, 139 insertions(+), 31 deletions(-) diff --git a/docs/user-manual.md b/docs/user-manual.md index 5afbd5c..a6749a9 100644 --- a/docs/user-manual.md +++ b/docs/user-manual.md @@ -105,7 +105,7 @@ If you connect to a profile that already has a live session, Nexus Terminal asks - **Key & Passphrase**: choose a key from the suggestions (keys in `~/.ssh`), click **Browse**, or type a full path (`~` is not expanded). The editor checks that the file exists and is a private key. - **Host Verification**: the host key policy (see section 12). - **Jump Hosts**: one or more bastions, each with user, host, port and an optional key. Nexus Terminal tunnels through each hop in order. -- **Port Forwarding**: local and remote forwards, each with a bind address and port, destination, label and an **Active** box to start it on connect. +- **Port Forwarding**: local and remote forwards, each with a bind address and port, destination, label and an **Active** box to start it on connect. A remote forward with no bind address listens on the server's loopback only (as `ssh -R` does); set it to `0.0.0.0` to expose it to the server's network, which also needs `GatewayPorts` on the server. - **Startup**: a startup command and environment variables (`KEY=VALUE`, one per line). - **Login Script**: expect-style steps (see below). - **SSH Agent**: agent authentication and **Forward Agent to Remote** (like `ssh -A`). diff --git a/rust/app/src/sftp.rs b/rust/app/src/sftp.rs index 344d44a..e760beb 100644 --- a/rust/app/src/sftp.rs +++ b/rust/app/src/sftp.rs @@ -393,7 +393,9 @@ impl Sftp { fn download(&mut self, s: &Shared, session: &str, files: Vec) { let targets: Vec<(FileEntry, PathBuf)> = if files.len() == 1 { - let Some(p) = rfd::FileDialog::new().set_file_name(&files[0].name).save_file() else { + // The name comes from the server: keep only a safe base name for the dialog's default. + let suggested = safe_sftp_upload_name(&files[0].name).unwrap_or_else(|_| "download".into()); + let Some(p) = rfd::FileDialog::new().set_file_name(suggested).save_file() else { return; }; vec![(files[0].clone(), p)] diff --git a/rust/core/src/clients/ssh.rs b/rust/core/src/clients/ssh.rs index fe5f807..7e894ab 100644 --- a/rust/core/src/clients/ssh.rs +++ b/rust/core/src/clients/ssh.rs @@ -20,6 +20,16 @@ use zeroize::Zeroizing; const MAX_PRIVATE_KEY_BYTES: u64 = 1 << 20; /// Bounds what `run_command` buffers, so a hostile server can't stream unlimited output into memory. const MAX_COMMAND_OUTPUT_BYTES: usize = 1 << 20; +/// How long `run_command` waits for a command to finish. +const COMMAND_TIMEOUT: Duration = Duration::from_secs(30); +/// Keyboard-interactive rounds answered before giving up on the method. +const MAX_KBD_INTERACTIVE_ROUNDS: usize = 10; +/// Extra handshake time allowed for the host-key prompt (see `credentials::host_key_check`). +const HOST_KEY_PROMPT_SECS: u64 = 60; +/// Remote forwards listen on the server's loopback unless told otherwise, as `ssh -R` does; an +/// empty bind host used to mean every interface, exposing the forwarded service to the server's +/// whole network wherever `GatewayPorts` is on. +const REMOTE_BIND_DEFAULT: &str = "localhost"; /// What a host-key decision needs. The manager's checker consults the known-hosts table and, /// per policy, prompts the user; it returns Err with the reason to reject. @@ -61,6 +71,9 @@ struct Handler { port: u16, check: HostKeyCheck, shared: Arc, + /// Whether this is the final hop. Only the target may open agent-forward and remote-forward + /// channels; a jump host never asked for either and must not get them (as `ssh -J` behaves). + target: bool, /// Certificate authorities trusted for this host (from `@cert-authority` known-hosts rows). cas: Vec, } @@ -123,7 +136,7 @@ impl client::Handler for Handler { _session: &mut client::Session, ) -> std::result::Result<(), Self::Error> { let dest = self.shared.remote_targets.lock().get(&(connected_address.to_string(), connected_port)).cloned(); - let Some((host, port)) = dest else { + let Some((host, port)) = dest.filter(|_| self.target) else { reply.reject(russh::ChannelOpenFailure::AdministrativelyProhibited).await; return Ok(()); }; @@ -143,7 +156,7 @@ impl client::Handler for Handler { reply: client::ChannelOpenHandle, _session: &mut client::Session, ) -> std::result::Result<(), Self::Error> { - if !self.shared.agent_forwarding.load(std::sync::atomic::Ordering::Relaxed) { + if !self.target || !self.shared.agent_forwarding.load(std::sync::atomic::Ordering::Relaxed) { reply.reject(russh::ChannelOpenFailure::AdministrativelyProhibited).await; return Ok(()); } @@ -348,7 +361,9 @@ async fn authenticate(h: &mut Handle, a: Auth<'_>) -> Result<()> { "keyboard-interactive" => { let Some(get) = a.password.clone() else { continue }; let mut resp = h.authenticate_keyboard_interactive_start(a.user, None).await?; - loop { + // A server can keep asking forever (harvesting TOTP codes, parking the connect + // thread on prompts); OpenSSH gives up after a handful of rounds too. + for _ in 0..MAX_KBD_INTERACTIVE_ROUNDS { match resp { client::KeyboardInteractiveAuthResponse::Success => return Ok(()), client::KeyboardInteractiveAuthResponse::Failure { .. } => break, @@ -469,11 +484,14 @@ impl SshClient { host: &str, port: u16, check: &HostKeyCheck, + target: bool, ) -> Result> { let cas = crate::known_hosts::authorities_for(&self.authorities.lock(), host, port); let cfg = config(&self.sc(), !cas.is_empty()); - let handler = Handler { host: host.into(), port, check: check.clone(), shared: self.shared.clone(), cas }; - let timeout = Duration::from_secs(self.profile.connect_timeout.max(1) as u64); + let handler = Handler { host: host.into(), port, check: check.clone(), shared: self.shared.clone(), cas, target }; + // The handshake may stop at the host-key prompt (up to 60 s): don't let the connect + // timeout expire underneath the user while they read a fingerprint. + let timeout = Duration::from_secs(self.profile.connect_timeout.max(1) as u64 + HOST_KEY_PROMPT_SECS); match tokio::time::timeout(timeout, client::connect_stream(cfg, stream, handler)).await { Err(_) => Err(format!("SSH handshake with {host}:{port} timed out").into()), Ok(Ok(h)) => Ok(h), @@ -507,16 +525,16 @@ impl SshClient { }; let mut handle = if sc.jump_hosts.is_empty() { let tcp = dial(&sc, &proxy_password, &p.host, port, timeout).await.map_err(|e| format!("SSH connection failed: {e}"))?; - self.handshake(tcp, &p.host, port, &check).await? + self.handshake(tcp, &p.host, port, &check, true).await? } else { let (prev, h) = self.via_jumps(&sc, &check, passphrase.clone(), timeout).await?; - let ch = prev - .channel_open_direct_tcpip(p.host.clone(), port as u32, "127.0.0.1", 0) + let ch = tokio::time::timeout(timeout, prev.channel_open_direct_tcpip(p.host.clone(), port as u32, "127.0.0.1", 0)) .await + .map_err(|_| format!("failed to dial target {}:{port} through jump hosts: timeout", p.host))? .map_err(|e| format!("failed to dial target {}:{port} through jump hosts: {e}", p.host))?; self.hops.lock().extend(h); self.hops.lock().push(prev); - self.handshake(ch.into_stream(), &p.host, port, &check).await? + self.handshake(ch.into_stream(), &p.host, port, &check, true).await? }; let cert = load_user_certificate(&sc.private_key_path); @@ -600,14 +618,14 @@ impl SshClient { .await .map_err(|_| format!("failed to connect to jump host {i} ({host}:{jport}): timeout"))? .map_err(|e| format!("failed to connect to jump host {i} ({host}:{jport}): {e}"))?; - self.handshake(tcp, host, jport, check).await? + self.handshake(tcp, host, jport, check, false).await? } Some(prev) => { - let ch = prev - .channel_open_direct_tcpip(host.clone(), jport as u32, "127.0.0.1", 0) + let ch = tokio::time::timeout(timeout, prev.channel_open_direct_tcpip(host.clone(), jport as u32, "127.0.0.1", 0)) .await + .map_err(|_| format!("failed to dial through jump host {i} ({host}:{jport}): timeout"))? .map_err(|e| format!("failed to dial through jump host {i} ({host}:{jport}): {e}"))?; - self.handshake(ch.into_stream(), host, jport, check).await? + self.handshake(ch.into_stream(), host, jport, check, false).await? } }; authenticate( @@ -652,11 +670,21 @@ impl SshClient { Ok(start.elapsed().as_millis() as i64) } - /// Runs a command on a separate channel and returns trimmed stdout. + /// Runs a command on a separate channel and returns trimmed stdout, giving up after + /// `COMMAND_TIMEOUT` so a server that never answers can't park the caller's thread. pub fn run_command(&self, command: &str) -> Result { let h = self.handle()?; let command = command.to_string(); rt().block_on(async move { + match tokio::time::timeout(COMMAND_TIMEOUT, Self::run_command_inner(h, command)).await { + Ok(r) => r, + Err(_) => Err("command timed out".into()), + } + }) + } + + async fn run_command_inner(h: Arc>, command: String) -> Result { + { let mut ch = h.channel_open_session().await.map_err(|e| format!("failed to open SSH session: {e}"))?; ch.exec(true, command.as_bytes()).await?; let mut out = vec![]; @@ -673,7 +701,7 @@ impl SshClient { } } Ok(String::from_utf8_lossy(&out).trim().to_string()) - }) + } } pub fn is_connected(&self) -> bool { @@ -733,7 +761,7 @@ impl SshClient { tasks.lock().push(task.abort_handle()); } ForwardDirection::Remote => { - let bind = if rule.bind_host.is_empty() { "0.0.0.0".to_string() } else { rule.bind_host.clone() }; + let bind = if rule.bind_host.is_empty() { REMOTE_BIND_DEFAULT.to_string() } else { rule.bind_host.clone() }; let port = rt() .block_on(h.tcpip_forward(bind.clone(), rule.bind_port as u32)) .map_err(|e| format!("remote forward: server listen on {bind}:{} failed: {e}", rule.bind_port))?; @@ -756,7 +784,7 @@ impl SshClient { t.abort(); } if f.rule.direction == ForwardDirection::Remote { - let bind = if f.rule.bind_host.is_empty() { "0.0.0.0".to_string() } else { f.rule.bind_host.clone() }; + let bind = if f.rule.bind_host.is_empty() { REMOTE_BIND_DEFAULT.to_string() } else { f.rule.bind_host.clone() }; self.shared.remote_targets.lock().retain(|(a, p), _| !(a == &bind && *p == f.rule.bind_port as u32)); if let Ok(h) = self.handle() { let port = f.rule.bind_port as u32; diff --git a/rust/core/src/known_hosts.rs b/rust/core/src/known_hosts.rs index d6c1ef7..5aea50c 100644 --- a/rust/core/src/known_hosts.rs +++ b/rust/core/src/known_hosts.rs @@ -11,6 +11,8 @@ use russh::keys::{HashAlg, PublicKey}; /// `key_type` of a certificate-authority row. pub const CA_KEY_TYPE: &str = "@cert-authority"; +/// Longest host pattern accepted (OpenSSH's own line limit is 8 KiB; real patterns are tiny). +pub const MAX_PATTERN_LEN: usize = 1024; #[derive(Debug, Default, PartialEq)] pub struct Parsed { @@ -35,6 +37,7 @@ pub fn parse(text: &str) -> Parsed { continue; }; match marker { + Some("@cert-authority") if hosts.len() > MAX_PATTERN_LEN => out.skipped += 1, Some("@cert-authority") => out.authorities.push((hosts.to_string(), key)), Some(_) => out.skipped += 1, None if hosts.starts_with('|') => out.skipped += 1, @@ -116,14 +119,34 @@ pub fn pattern_matches(patterns: &str, host: &str, port: u16) -> bool { matched } +/// Wildcard match with `*` and `?`, iterative and linear in `|p| + |s|` per star (the classic +/// two-pointer algorithm), so a pattern of a million stars or `*a*a*a…` can't recurse or +/// backtrack exponentially. Patterns come from imported files and typed input. fn glob(p: &str, s: &str) -> bool { - match (p.chars().next(), s.chars().next()) { - (None, None) => true, - (Some('*'), _) => glob(&p[1..], s) || (!s.is_empty() && glob(p, &s[s.chars().next().unwrap().len_utf8()..])), - (Some('?'), Some(c)) => glob(&p[1..], &s[c.len_utf8()..]), - (Some(a), Some(b)) if a == b => glob(&p[a.len_utf8()..], &s[b.len_utf8()..]), - _ => false, + let (p, s): (Vec, Vec) = (p.chars().collect(), s.chars().collect()); + let (mut pi, mut si) = (0, 0); + let mut star: Option<(usize, usize)> = None; + while si < s.len() { + match p.get(pi) { + Some('*') => { + star = Some((pi, si)); + pi += 1; + } + Some(&c) if c == '?' || c == s[si] => { + pi += 1; + si += 1; + } + _ => match star { + Some((sp, ss)) => { + pi = sp + 1; + si = ss + 1; + star = Some((sp, ss + 1)); + } + None => return false, + }, + } } + p[pi..].iter().all(|&c| c == '*') } /// CA keys trusted for `host:port`. @@ -180,6 +203,29 @@ mod tests { assert!(!pattern_matches(pats, "corp.example", 22)); } + #[test] + fn hostile_patterns_match_in_linear_time() { + // The old recursive matcher overflowed the stack on a long run of stars and went + // exponential on `*a*a*a…b`; both must now be instant (and run on a small stack). + let t = std::thread::Builder::new() + .stack_size(256 << 10) + .spawn(|| { + let start = std::time::Instant::now(); + assert!(pattern_matches(&"*".repeat(200_000), "db.corp", 22)); + assert!(!pattern_matches("*a*a*a*a*a*a*a*a*a*a*b", &"a".repeat(253), 22)); + assert!(pattern_matches("*a*a*a*b?", &format!("{}bx", "a".repeat(253)), 22)); + assert!(!pattern_matches("a*c", "ab", 22) && pattern_matches("a*", "a", 22) && !pattern_matches("a?", "a", 22)); + start.elapsed() + }) + .unwrap() + .join() + .unwrap(); + assert!(t < std::time::Duration::from_millis(500), "{t:?}"); + // Over-long CA patterns in an imported file are skipped rather than stored. + let p = parse(&format!("@cert-authority {} {}\n", "*".repeat(MAX_PATTERN_LEN + 1), ed())); + assert!(p.authorities.is_empty() && p.skipped == 1); + } + #[test] fn exports_what_it_imports() { let ED = ed(); diff --git a/rust/core/src/service.rs b/rust/core/src/service.rs index f68aa46..9ca501a 100644 --- a/rust/core/src/service.rs +++ b/rust/core/src/service.rs @@ -324,7 +324,7 @@ impl Service { /// Trusts an SSH certificate authority for hosts matching `pattern` (OpenSSH syntax). pub fn add_cert_authority(&self, pattern: &str, public_key: &str) -> Result<()> { let pattern = pattern.trim(); - if pattern.is_empty() || pattern.contains(char::is_whitespace) { + if pattern.is_empty() || pattern.contains(char::is_whitespace) || pattern.len() > crate::known_hosts::MAX_PATTERN_LEN { return Err("the host pattern must be one word, e.g. *.example.com".into()); } let key = russh::keys::PublicKey::from_openssh(public_key.trim()).map_err(|e| format!("not an OpenSSH public key: {e}"))?; diff --git a/rust/core/src/sessions/credentials.rs b/rust/core/src/sessions/credentials.rs index 49c4294..569e413 100644 --- a/rust/core/src/sessions/credentials.rs +++ b/rust/core/src/sessions/credentials.rs @@ -108,7 +108,7 @@ impl Manager { pub fn host_key_check(self: &Arc, session: &str, policy: HostKeyPolicy) -> HostKeyCheck { let (me, sid) = (self.clone(), session.to_string()); Arc::new(move |k: &HostKeyInfo| { - let stored = me.store.get_known_host(&k.host, k.port as i64).map_err(|e| format!("failed to query known hosts: {e}"))?; + let stored = me.store.get_known_host(&k.host, k.port as i64, &k.key_type).map_err(|e| format!("failed to query known hosts: {e}"))?; let save = || { let n = now(); me.store diff --git a/rust/core/src/sessions/mod.rs b/rust/core/src/sessions/mod.rs index fbf5c2a..aa097b8 100644 --- a/rust/core/src/sessions/mod.rs +++ b/rust/core/src/sessions/mod.rs @@ -873,7 +873,7 @@ mod tests { } })); let info = |fp: &str| HostKeyInfo { host: "h".into(), port: 22, key_type: "ssh-ed25519".into(), fingerprint: fp.into(), public_key_b64: "AAAA".into() }; - let stored = |m: &Manager| m.store.get_known_host("h", 22).unwrap().map(|k| k.fingerprint); + let stored = |m: &Manager| m.store.get_known_host("h", 22, "ssh-ed25519").unwrap().map(|k| k.fingerprint); // Unknown key: accept once records nothing; accept & save records it. let check = m.host_key_check("s", HostKeyPolicy::Ask); answers.lock().push(HostKeyDecision::AcceptOnce); diff --git a/rust/core/src/storage.rs b/rust/core/src/storage.rs index 37d937f..0662dc5 100644 --- a/rust/core/src/storage.rs +++ b/rust/core/src/storage.rs @@ -273,14 +273,17 @@ impl Store { Ok(n) } - pub fn get_known_host(&self, host: &str, port: i64) -> Result> { + /// The stored key of this type for `host:port`. A host can have one key per type (an + /// imported known_hosts usually brings ed25519, RSA and ECDSA), and comparing the offered + /// key with a row of another type would look like a changed key. + pub fn get_known_host(&self, host: &str, port: i64, key_type: &str) -> Result> { Ok(self .db .lock() .query_row( "SELECT id, host, port, key_type, fingerprint, public_key, first_seen, last_seen, trusted - FROM known_hosts WHERE host = ? AND port = ?", - params![host, port], + FROM known_hosts WHERE host = ? AND port = ? AND key_type = ?", + params![host, port, key_type], scan_known_host, ) .optional()?) @@ -557,4 +560,33 @@ mod tests { self } } + + #[test] + fn known_hosts_are_looked_up_per_key_type() { + let tmp = std::env::temp_dir().join(format!("nexus-kh-{}.db", std::process::id())); + let _ = std::fs::remove_file(&tmp); + let s = Store::open(&tmp).unwrap(); + let n = now(); + for (kt, fp) in [("ssh-ed25519", "SHA256:ed"), ("ssh-rsa", "SHA256:rsa")] { + s.save_known_host(&KnownHost { + id: String::new(), + host: "h".into(), + port: 22, + key_type: kt.into(), + fingerprint: fp.into(), + public_key: "AAAA".into(), + first_seen: n, + last_seen: n, + trusted: true, + }) + .unwrap(); + } + // An imported known_hosts brings several key types per host; the offered key is + // compared with the row of its own type, not whichever row comes first. + assert_eq!(s.get_known_host("h", 22, "ssh-rsa").unwrap().unwrap().fingerprint, "SHA256:rsa"); + assert_eq!(s.get_known_host("h", 22, "ssh-ed25519").unwrap().unwrap().fingerprint, "SHA256:ed"); + assert!(s.get_known_host("h", 22, "ecdsa-sha2-nistp256").unwrap().is_none()); + drop(s); + let _ = std::fs::remove_file(tmp); + } }