fix(ssh/sftp): harden downloads — path-traversal guard, atomic temp, scoped retry

Three SFTP fixes, all in the download/session path:

- Security (P0): reject server-supplied directory-entry names that aren't a
  single normal path component before using them as a local path component.
  A recursive download built `lpath.join(name)` straight from entry names, so
  a malicious/compromised server could return `..`, `a/b`, or an absolute
  `/etc/...` and escape the destination for arbitrary local file write with
  server-chosen mode bits (CVE-2019-6111 class). New `safe_local_name` guard is
  applied in both the download walker and the `remote_size` pre-pass so the size
  denominator matches what is actually transferred.

- Correctness: download to a per-file `<local>.tty7-download-<rand>` temp then
  rename over the target on success; on error/cancel remove the temp and leave
  any pre-existing target intact. Mirrors the upload temp+rename discipline so a
  failed download never truncates a local file in place. preserve_mode still
  applies to the final file.

- Correctness: `with_session` now retries the one re-opened-session attempt only
  on a transport/channel failure, not on a logical SFTP error (permission
  denied, no such file). A server status code returns directly instead of
  wasting a second identical round-trip.

Adds unit tests for safe_local_name, download_temp_path, and is_transport_failure.

src/daemon/ssh/sftp.rs

@@ -14,8 +14,10 @@
 //!   (`Arc::ptr_eq`). A reconnect (new connection, same key) transparently gets a
 //!   fresh SFTP session.
 //! - One-shot operations run through [`SftpManager::with_session`], which retries
-//!   once on failure with a freshly re-opened session — so a dead subsystem
-//!   channel (while the connection itself lives) is re-opened transparently.
+//!   once with a freshly re-opened session **only** on a transport/channel failure
+//!   — so a dead subsystem channel (while the connection itself lives) is re-opened
+//!   transparently, while a logical SFTP error (permission denied, no such file)
+//!   returns directly without a pointless retry.
 //!
 //! ## Threading
 //! The server's std connection threads call the **sync** methods here
@@ -34,7 +36,7 @@
 
 use std::collections::HashMap;
 use std::future::Future;
-use std::path::Path;
+use std::path::{Component, Path, PathBuf};
 use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
 use std::sync::{Arc, Mutex, OnceLock, Weak};
 use std::time::{Duration, Instant};
@@ -114,6 +116,49 @@ pub fn upload_temp_name(remote: &str) -> String {
     format!("{remote}.tty7-upload-{:x}{:x}", nanos, n)
 }
 
+/// Whether a server-supplied directory-entry `name` is safe to use as a *single*
+/// local path component when building a download destination.
+///
+/// A recursive download turns remote entry names into local path components
+/// (`lpath.join(name)`). A malicious or compromised server can return names like
+/// `..`, `../../etc/foo`, or an absolute `/etc/foo`; `Path::join` with an absolute
+/// component discards the base, and `..` escapes upward — arbitrary local file
+/// write (CVE-2019-6111-class). Accept only a name that is exactly one *normal*
+/// path component: reject empty, `.`, `..`, anything containing a `/` or `\\`
+/// separator, and anything that doesn't resolve to a single `Component::Normal`.
+fn safe_local_name(name: &str) -> bool {
+    if name.is_empty() || name == "." || name == ".." {
+        return false;
+    }
+    // Reject either separator on every platform: a POSIX server name must never
+    // introduce a Windows path separator either.
+    if name.contains('/') || name.contains('\\') {
+        return false;
+    }
+    let mut comps = Path::new(name).components();
+    matches!(
+        (comps.next(), comps.next()),
+        (Some(Component::Normal(_)), None)
+    )
+}
+
+/// The temp path a download writes to before renaming over its target:
+/// `<local>.tty7-download-<rand>`, a sibling in the *same directory* so the
+/// finishing rename is same-filesystem (atomic). Mirrors [`upload_temp_name`].
+fn download_temp_path(lpath: &Path) -> PathBuf {
+    static COUNTER: AtomicU64 = AtomicU64::new(0);
+    let n = COUNTER.fetch_add(1, Ordering::Relaxed);
+    let nanos = std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .map(|d| d.as_nanos() as u64)
+        .unwrap_or(0);
+    // Append to the full path (a sibling with a suffix) so the temp stays in the
+    // destination directory regardless of the file name's own extension.
+    let mut os = lpath.as_os_str().to_os_string();
+    os.push(format!(".tty7-download-{:x}{:x}", nanos, n));
+    PathBuf::from(os)
+}
+
 // ---------------------------------------------------------------------------
 // Entry classification (pure).
 // ---------------------------------------------------------------------------
@@ -409,8 +454,12 @@ impl SftpManager {
     // --- session cache -----------------------------------------------------
 
     /// Run `f` against the pane's cached SFTP session, retrying once with a
-    /// freshly re-opened session if the first attempt fails — so a dead subsystem
-    /// channel (while the connection lives) is transparently re-established.
+    /// freshly re-opened session **only** when the first attempt failed for a
+    /// transport/channel reason (the cached subsystem channel died while the
+    /// connection lives). A logical SFTP failure — a server status like permission
+    /// denied or no-such-file — returns directly, never re-opening the session (a
+    /// retry would just fail identically and waste a round-trip). See
+    /// [`is_transport_failure`].
     async fn with_session<T, F, Fut>(&self, conn: &Arc<SshConnection>, f: F) -> Result<T, String>
     where
         F: Fn(Arc<SftpSession>) -> Fut,
@@ -419,11 +468,12 @@ impl SftpManager {
         let sftp = self.session_for(conn).await?;
         match f(sftp).await {
             Ok(v) => Ok(v),
-            Err(_) => {
+            Err(e) if is_transport_failure(&e) => {
                 self.invalidate(conn.key());
                 let sftp = self.session_for(conn).await?;
                 f(sftp).await
             }
+            Err(e) => Err(e),
         }
     }
 
@@ -459,6 +509,26 @@ impl SftpManager {
     }
 }
 
+/// Whether a stringified SFTP op error looks like a *transport/channel* failure
+/// (the subsystem channel died) rather than a logical server status (permission
+/// denied, no such file, …). Only the former is worth re-opening the session for.
+///
+/// `russh_sftp` renders channel/IO failures with these markers; a server status
+/// code renders as `<code>: <message>` and matches none of them — so an unmatched
+/// (logical) error is not retried. Conservative by design: an unrecognized error
+/// is treated as logical and returned directly.
+fn is_transport_failure(msg: &str) -> bool {
+    const MARKERS: &[&str] = &[
+        "I/O:",           // russh_sftp `Error::IO` — the channel stream failed
+        "Unexpected EOF", // the stream closed mid-message
+        "Timeout",        // no response — the subsystem/channel is wedged
+        "Unexpected packet",
+        "SendError", // the channel task's receiver is gone
+        "RecvError", // the channel task ended before replying
+    ];
+    MARKERS.iter().any(|m| msg.contains(m))
+}
+
 /// Open a fresh SFTP subsystem channel on `conn` and hand back a session.
 async fn open_sftp(conn: &Arc<SshConnection>) -> Result<Arc<SftpSession>, String> {
     let channel = conn
@@ -657,6 +727,15 @@ async fn download(sftp: &SftpSession, spec: &SftpTransferSpec, job: &Job) -> Res
                 if name == "." || name == ".." {
                     continue;
                 }
+                // Guard against a hostile server returning a traversing name
+                // (`..`, `a/b`, `/abs`): it would become a local path component
+                // via `lpath.join`, escaping the destination. Skip unsafe names.
+                if !safe_local_name(&name) {
+                    log::warn!(
+                        "sftp download: skipping remote entry with unsafe name {name:?} under {rpath}"
+                    );
+                    continue;
+                }
                 stack.push((remote_join(&rpath, &name), lpath.join(&name)));
             }
         } else {
@@ -677,29 +756,48 @@ async fn download_file(
     if let Some(parent) = lpath.parent() {
         let _ = tokio::fs::create_dir_all(parent).await;
     }
-    let mut remote = sftp
-        .open(rpath.to_string())
-        .await
-        .map_err(|e| format!("{e}"))?;
-    let mut local = tokio::fs::File::create(lpath)
-        .await
-        .map_err(|e| format!("create {}: {e}", lpath.display()))?;
-    let mut buf = vec![0u8; CHUNK];
-    loop {
-        if job.is_cancelled() {
-            return Err(cancelled());
-        }
-        let n = remote.read(&mut buf).await.map_err(|e| format!("{e}"))?;
-        if n == 0 {
-            break;
-        }
-        local
-            .write_all(&buf[..n])
+    // Download to a per-file temp in the destination dir, then rename over the
+    // target on success — mirroring the upload temp+rename discipline so a failed
+    // or cancelled download never truncates a pre-existing local file in place.
+    let temp = download_temp_path(lpath);
+    let result: Result<(), String> = async {
+        let mut remote = sftp
+            .open(rpath.to_string())
             .await
-            .map_err(|e| format!("write local: {e}"))?;
-        job.add_bytes(n as u64);
+            .map_err(|e| format!("{e}"))?;
+        let mut local = tokio::fs::File::create(&temp)
+            .await
+            .map_err(|e| format!("create {}: {e}", temp.display()))?;
+        let mut buf = vec![0u8; CHUNK];
+        loop {
+            if job.is_cancelled() {
+                return Err(cancelled());
+            }
+            let n = remote.read(&mut buf).await.map_err(|e| format!("{e}"))?;
+            if n == 0 {
+                break;
+            }
+            local
+                .write_all(&buf[..n])
+                .await
+                .map_err(|e| format!("write local: {e}"))?;
+            job.add_bytes(n as u64);
+        }
+        local.flush().await.ok();
+        Ok(())
+    }
+    .await;
+
+    if let Err(e) = result {
+        // Best effort: drop the partial temp, leaving any pre-existing target intact.
+        let _ = tokio::fs::remove_file(&temp).await;
+        return Err(e);
+    }
+    // Swap the completed temp over the target.
+    if let Err(e) = tokio::fs::rename(&temp, lpath).await {
+        let _ = tokio::fs::remove_file(&temp).await;
+        return Err(format!("rename into {}: {e}", lpath.display()));
     }
-    local.flush().await.ok();
     // Preserve the executable/permission bits where sane (unix only, low 12 bits).
     preserve_mode(lpath, mode);
     Ok(())
@@ -819,6 +917,11 @@ async fn remote_size(
                     if name == "." || name == ".." {
                         continue;
                     }
+                    // Skip the same unsafe names the download walker skips so the
+                    // size denominator matches what is actually transferred.
+                    if !safe_local_name(&name) {
+                        continue;
+                    }
                     stack.push(remote_join(&path, &name));
                 }
             }
@@ -916,6 +1019,56 @@ mod tests {
         assert_ne!(a, b);
     }
 
+    #[test]
+    fn safe_local_name_rejects_traversal_and_accepts_plain_names() {
+        // Rejected: empty, dot, dotdot, embedded/leading separators, absolute.
+        assert!(!safe_local_name(""));
+        assert!(!safe_local_name("."));
+        assert!(!safe_local_name(".."));
+        assert!(!safe_local_name("a/b"));
+        assert!(!safe_local_name("/abs"));
+        assert!(!safe_local_name("../../.ssh/authorized_keys"));
+        assert!(!safe_local_name("a\\b"));
+        // Accepted: ordinary single components, including Unicode and dotted names.
+        assert!(safe_local_name("file.txt"));
+        assert!(safe_local_name("项目"));
+        assert!(safe_local_name("a.tar.gz"));
+        assert!(safe_local_name(".hidden"));
+    }
+
+    #[test]
+    fn download_temp_path_is_sibling_and_distinct() {
+        let target = Path::new("/dest/dir/file.bin");
+        let a = download_temp_path(target);
+        let b = download_temp_path(target);
+        // Same directory as the target (so the finishing rename is same-filesystem).
+        assert_eq!(a.parent(), target.parent());
+        assert!(
+            a.file_name()
+                .unwrap()
+                .to_string_lossy()
+                .starts_with("file.bin.tty7-download-")
+        );
+        // Two temps for the same target differ (counter component).
+        assert_ne!(a, b);
+    }
+
+    #[test]
+    fn is_transport_failure_distinguishes_channel_from_logical_errors() {
+        // Transport/channel failures → retry.
+        assert!(is_transport_failure("I/O: broken pipe"));
+        assert!(is_transport_failure("rename failed: I/O: connection reset"));
+        assert!(is_transport_failure("Unexpected EOF on stream"));
+        assert!(is_transport_failure("Timeout"));
+        assert!(is_transport_failure("SendError: channel closed"));
+        // Logical server statuses → no retry.
+        assert!(!is_transport_failure("3: Permission denied"));
+        assert!(!is_transport_failure("2: No such file or directory"));
+        assert!(!is_transport_failure(
+            "remote path is a directory (enable recursive)"
+        ));
+    }
+
     #[test]
     fn classify_prefers_symlink_over_regular_bit() {
         // S_IFLNK carries the S_IFREG bit too; symlink must win.