From efc637801a9dd8b84398331be142f3a0c3ed8ebf Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Apr 2026 02:26:32 +0900 Subject: [PATCH 1/5] feat: add scp.root config and thread sftp/scp chroot through ServerConfig Previously `SftpConfig.root` was declared but never read, and `ScpConfig` had no `root` field at all. Both handler-construction sites in `SshHandler` hard-coded `user_info.home_dir` as the chroot root, so the configured chroot setting was dead code. This commit adds `ScpConfig.root: Option`, plus `sftp_root` and `scp_root` on the builder-based `ServerConfig`, and threads them through `into_server_config()`. SCP falls back to `sftp.root` when `scp.root` is unset, so a single top-level chroot setting governs both subsystems unless admins explicitly want them split. Also adds focused unit tests covering the precedence rules and the no-chroot default. The handler-side wiring lands in a follow-up commit. Refs #186 --- src/server/config/mod.rs | 87 ++++++++++++++++++++++++++++++++++++++ src/server/config/types.rs | 25 +++++++++-- 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/src/server/config/mod.rs b/src/server/config/mod.rs index c50f8a2b..8dd91e60 100644 --- a/src/server/config/mod.rs +++ b/src/server/config/mod.rs @@ -154,6 +154,25 @@ pub struct ServerConfig { #[serde(default = "default_true")] pub scp_enabled: bool, + /// Optional chroot directory for SFTP operations. + /// + /// When `None` (default), SFTP runs without chroot: absolute client paths + /// are used verbatim and relative paths resolve from the user's home + /// directory, matching OpenSSH `sftp-server` semantics. + /// + /// When set, SFTP clients are confined to this directory; absolute paths + /// outside it are rejected with `permission_denied`. + #[serde(default)] + pub sftp_root: Option, + + /// Optional chroot directory for SCP transfers. + /// + /// Has the same semantics as [`Self::sftp_root`]. When `None` and + /// `sftp_root` is set, SCP falls back to `sftp_root`. Configure both + /// fields only if SCP and SFTP need different chroots. + #[serde(default)] + pub scp_root: Option, + /// Time window for counting authentication attempts in seconds. /// /// Default: 300 (5 minutes) @@ -293,6 +312,8 @@ impl Default for ServerConfig { password_auth: PasswordAuthConfigSerde::default(), exec: ExecConfig::default(), scp_enabled: true, + sftp_root: None, + scp_root: None, auth_window_secs: default_auth_window_secs(), ban_time_secs: default_ban_time_secs(), whitelist_ips: Vec::new(), @@ -564,6 +585,25 @@ impl ServerConfigBuilder { self } + /// Set the SFTP chroot directory. + /// + /// When `None`, SFTP runs without chroot (OpenSSH-compatible default). + /// When set, SFTP clients are confined to this directory. + pub fn sftp_root(mut self, root: Option) -> Self { + self.config.sftp_root = root; + self + } + + /// Set the SCP chroot directory. + /// + /// When `None`, SCP falls back to [`sftp_root`](Self::sftp_root) if set, + /// otherwise runs without chroot. Set both fields only when SCP and SFTP + /// need different chroots. + pub fn scp_root(mut self, root: Option) -> Self { + self.config.scp_root = root; + self + } + /// Set the maximum sessions per user. pub fn max_sessions_per_user(mut self, max: usize) -> Self { self.config.max_sessions_per_user = max; @@ -635,6 +675,10 @@ impl ServerFileConfig { blocked_commands: Vec::new(), }, scp_enabled: self.scp.enabled, + // SCP falls back to sftp.root when scp.root is unset so a single + // top-level chroot setting governs both subsystems. + scp_root: self.scp.root.or_else(|| self.sftp.root.clone()), + sftp_root: self.sftp.root, auth_window_secs: self.security.auth_window, ban_time_secs: self.security.ban_time, whitelist_ips: self.security.whitelist_ips, @@ -701,6 +745,49 @@ mod tests { assert!(server_config.allow_password_auth); } + #[test] + fn sftp_root_threads_into_server_config() { + // Setting sftp.root should propagate to ServerConfig.sftp_root, and + // scp_root should fall back to it when scp.root is unset. + let mut file_config = ServerFileConfig::default(); + file_config.server.host_keys = vec![PathBuf::from("/test/key")]; + file_config.sftp.root = Some(PathBuf::from("/srv/sftp")); + + let server_config = file_config.into_server_config(); + + assert_eq!(server_config.sftp_root, Some(PathBuf::from("/srv/sftp"))); + assert_eq!(server_config.scp_root, Some(PathBuf::from("/srv/sftp"))); + } + + #[test] + fn scp_root_overrides_sftp_root_fallback() { + // When scp.root is explicitly set, it takes precedence over the + // sftp.root fallback so admins can split the two chroots. + let mut file_config = ServerFileConfig::default(); + file_config.server.host_keys = vec![PathBuf::from("/test/key")]; + file_config.sftp.root = Some(PathBuf::from("/srv/sftp")); + file_config.scp.root = Some(PathBuf::from("/srv/scp")); + + let server_config = file_config.into_server_config(); + + assert_eq!(server_config.sftp_root, Some(PathBuf::from("/srv/sftp"))); + assert_eq!(server_config.scp_root, Some(PathBuf::from("/srv/scp"))); + } + + #[test] + fn no_chroot_by_default() { + // The new default: both scp_root and sftp_root are None when no + // configuration is provided. This is the OpenSSH-compatible default + // documented for issue #186. + let mut file_config = ServerFileConfig::default(); + file_config.server.host_keys = vec![PathBuf::from("/test/key")]; + + let server_config = file_config.into_server_config(); + + assert!(server_config.sftp_root.is_none()); + assert!(server_config.scp_root.is_none()); + } + #[test] fn test_config_new() { let config = ServerConfig::new(); diff --git a/src/server/config/types.rs b/src/server/config/types.rs index 9a7d07d4..d3228e4a 100644 --- a/src/server/config/types.rs +++ b/src/server/config/types.rs @@ -247,10 +247,18 @@ pub struct SftpConfig { #[serde(default = "default_true")] pub enabled: bool, - /// Root directory for SFTP operations. - /// - /// If set, SFTP clients will be chrooted to this directory. - /// If None, users have access to the entire filesystem (subject to permissions). + /// Optional chroot directory for SFTP operations. + /// + /// When set, SFTP clients are confined to this directory: + /// - Absolute client paths inside `root` are honored as-is. + /// - Absolute client paths outside `root` are rejected with `permission_denied`. + /// - Relative client paths resolve under `root`. + /// - `..` traversal is clamped to `root`. + /// + /// When `None` (default), no chroot is applied. This matches OpenSSH + /// `sftp-server` behavior: absolute paths are used verbatim and relative + /// paths resolve from the user's home directory. Filesystem permissions + /// remain the only access control. pub root: Option, } @@ -263,6 +271,14 @@ pub struct ScpConfig { /// Default: true #[serde(default = "default_true")] pub enabled: bool, + + /// Optional chroot directory for SCP transfers. + /// + /// Has the same semantics as [`SftpConfig::root`]. When `None`, SCP uses + /// `sftp.root` as a fallback so a single `root` setting controls both + /// subsystems. Set this explicitly only when SCP and SFTP need different + /// chroots. + pub root: Option, } /// File transfer filtering configuration. @@ -650,6 +666,7 @@ impl Default for ScpConfig { fn default() -> Self { Self { enabled: default_true(), + root: None, } } } From 01505ceb2cfe336ab9c288b41525725b66532236 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Apr 2026 02:26:53 +0900 Subject: [PATCH 2/5] fix: stop SCP/SFTP path doubling and honor target_is_directory Three related defects in bssh-server's file-transfer subsystems caused SCP and SFTP uploads to fail when the client supplied an absolute destination path. Reported in #186 against the published v2.1.1 build. 1. Path doubling. `ScpHandler::resolve_path` and `SftpHandler::resolve_path_static` unconditionally re-rooted every absolute client path under the user's home directory, so `scp local user@host:/home/work/file.bin` against root `/home/work` produced `/home/work/home/work/file.bin`. The same bug broke `bssh upload local /abs/remote.bin` with `No such file`. Both resolvers now carry an `Option` chroot root and a separate `cwd` for relative paths. With no chroot (the new default), absolute paths are honored verbatim and relative paths resolve from the user's home directory, matching OpenSSH `sftp-server`/`scp`. With chroot, absolute paths inside the chroot are honored verbatim (no doubling); absolute paths outside are rejected with `permission_denied`. Plain `/` keeps mapping to the chroot directory so the `realpath` roundtrip used by interactive SFTP clients still works. Path-traversal via `..` is still clamped to the chroot, and the existing symlink-escape canonicalization is preserved. 2. SCP filename appending. `receive_file` always appended the source filename to the resolved target, so `scp local.bin host:/tmp/dest.bin` wrote to `/tmp/dest.bin/local.bin`. The handler now consults `target_is_directory` (parsed from `-d`/`-r`) and the filesystem state of the resolved target. Single-file destinations write directly to the resolved path; directory destinations (existing directory, `-d`/`-r` flag, or recursive descent) keep the filename-appending behavior. 3. Dead-code chroot config. `SshHandler` now reads `config.sftp_root` and `config.scp_root` and threads them into the handlers, so setting `sftp.root` in the YAML actually changes the chroot. Behavior change: with no `sftp.root`/`scp.root` configured, the server no longer applies an implicit chroot at the user's home directory. Deployments that intentionally wanted that confinement must now set the field explicitly. This matches OpenSSH defaults and is the recommended setup for Backend.AI session containers per the issue. Closes #186 --- src/server/handler.rs | 16 +- src/server/scp.rs | 435 ++++++++++++++++++------ src/server/sftp.rs | 759 ++++++++++++++++++++++++++++-------------- 3 files changed, 868 insertions(+), 342 deletions(-) diff --git a/src/server/handler.rs b/src/server/handler.rs index b5d9f8bc..f4048413 100644 --- a/src/server/handler.rs +++ b/src/server/handler.rs @@ -1001,11 +1001,14 @@ impl russh::server::Handler for SshHandler { let handle_clone = handle.clone(); - // Create SCP handler with user's home directory as root + // Honor the configured chroot. SCP falls back to sftp_root + // (already merged in into_server_config). When None, no + // chroot is applied, matching OpenSSH semantics. let scp_handler = ScpHandler::from_command( &scp_cmd, user_info.clone(), - Some(user_info.home_dir.clone()), + self.config.scp_root.clone(), + user_info.home_dir.clone(), ); // Run SCP in a spawned task so the session loop can process incoming data @@ -1335,8 +1338,13 @@ impl russh::server::Handler for SshHandler { "Starting SFTP session" ); - // Create SFTP handler with user's home directory as root - let sftp_handler = SftpHandler::new(user_info.clone(), Some(user_info.home_dir)); + // Honor the configured chroot. When `sftp_root` is None, + // run without chroot, matching OpenSSH `sftp-server` defaults. + let sftp_handler = SftpHandler::new( + user_info.clone(), + self.config.sftp_root.clone(), + user_info.home_dir, + ); // Run SFTP server on the channel stream russh_sftp::server::run(channel.into_stream(), sftp_handler).await; diff --git a/src/server/scp.rs b/src/server/scp.rs index 5ff02949..5d7b4bb4 100644 --- a/src/server/scp.rs +++ b/src/server/scp.rs @@ -37,11 +37,22 @@ //! use std::path::PathBuf; //! //! let user = UserInfo::new("testuser"); +//! // Without chroot (OpenSSH-compatible behavior): +//! let handler = ScpHandler::new( +//! ScpMode::Sink, +//! PathBuf::from("/tmp/upload"), +//! user.clone(), +//! None, +//! PathBuf::from("/home/testuser"), +//! ); +//! +//! // With chroot: //! let handler = ScpHandler::new( //! ScpMode::Sink, //! PathBuf::from("/tmp/upload"), //! user, -//! Some(PathBuf::from("/home/testuser")), +//! Some(PathBuf::from("/srv/scp")), +//! PathBuf::from("/home/testuser"), //! ); //! ``` @@ -199,6 +210,99 @@ impl ScpCommand { } } +/// Normalize a path's `..` and `.` components without touching the filesystem. +fn normalize_components(path: &Path) -> PathBuf { + let mut out = PathBuf::new(); + for component in path.components() { + match component { + Component::Normal(c) => out.push(c), + Component::CurDir => {} + Component::ParentDir => { + if !out.pop() { + out.push(".."); + } + } + Component::RootDir => out.push("/"), + Component::Prefix(p) => out.push(p.as_os_str()), + } + } + out +} + +/// Resolve a client-supplied SCP path against a chroot root. +/// +/// - Absolute paths inside `root` are honored as-is. +/// - Absolute paths outside `root` are rejected. +/// - Relative paths are joined with `root`; `..` is clamped to `root`. +fn resolve_chroot_scp(requested: &Path, root: &Path, user: &str) -> Result { + if requested.is_absolute() { + // Plain "/" is the client's view of the chroot root (matches what + // `realpath` returns). Map it back to the actual chroot directory. + if requested == Path::new("/") { + return Ok(root.to_path_buf()); + } + let normalized = normalize_components(requested); + if normalized == root || normalized.starts_with(root) { + return Ok(normalized); + } + tracing::warn!( + event = "chroot_escape_blocked", + user = %user, + requested = %requested.display(), + root = %root.display(), + "Security: absolute path outside chroot blocked" + ); + anyhow::bail!("Access denied: path outside root"); + } + + // Relative path under chroot: join, then walk components clamping `..` + // so traversal cannot escape the chroot. + let mut resolved = root.to_path_buf(); + for component in requested.components() { + match component { + Component::Normal(c) => resolved.push(c), + Component::CurDir => {} + Component::ParentDir => { + if resolved != root { + resolved.pop(); + } + if !resolved.starts_with(root) { + resolved = root.to_path_buf(); + } + } + Component::RootDir | Component::Prefix(_) => {} + } + } + + if !resolved.starts_with(root) { + tracing::warn!( + event = "path_traversal_attempt", + user = %user, + requested = %requested.display(), + resolved = %resolved.display(), + root = %root.display(), + "Security: path traversal attempt blocked" + ); + anyhow::bail!("Access denied: path outside root"); + } + + Ok(resolved) +} + +/// Resolve a client-supplied SCP path without a chroot. +/// +/// Absolute paths are used verbatim. Relative paths join with `cwd` +/// (the user's home directory). `..` is normalized but not clamped. +/// This matches OpenSSH `scp -t` behavior on a non-restricted account. +fn resolve_no_chroot_scp(requested: &Path, cwd: &Path) -> PathBuf { + let joined = if requested.is_absolute() { + requested.to_path_buf() + } else { + cwd.join(requested) + }; + normalize_components(&joined) +} + /// SCP server handler. /// /// Implements the SCP protocol for file transfer operations with @@ -210,12 +314,26 @@ pub struct ScpHandler { target_path: PathBuf, /// Current user information. user_info: UserInfo, - /// Root directory for operations (chroot-like behavior). - root_dir: PathBuf, + /// Optional chroot root. + /// + /// `Some(path)` confines all transfers to `path` (chroot semantics). + /// `None` (default) runs without chroot: absolute paths are honored + /// verbatim and relative paths resolve from `cwd`. This matches OpenSSH. + root_dir: Option, + /// Base directory for resolving relative client paths. + /// + /// Set to the chroot root when chroot is enabled, or to the user's home + /// directory when chroot is disabled. + cwd: PathBuf, /// Whether recursive mode is enabled. recursive: bool, /// Whether to preserve times. preserve_times: bool, + /// Whether the client signaled the destination is a directory (`-d`/`-r`). + /// + /// Used by `receive_file` to decide whether to append the source filename + /// to a single-file destination. + target_is_directory: bool, /// Stored times for the next file (mtime, atime). stored_times: Option<(u64, u64)>, } @@ -228,20 +346,24 @@ impl ScpHandler { /// * `mode` - The SCP mode (source or sink) /// * `target_path` - The target path for the operation /// * `user_info` - Information about the authenticated user - /// * `root_dir` - Optional root directory for chroot-like behavior + /// * `root_dir` - Optional chroot root. When `None`, no chroot is applied. + /// * `home_dir` - The user's home directory; used as the base for relative + /// paths when chroot is disabled. pub fn new( mode: ScpMode, target_path: PathBuf, user_info: UserInfo, root_dir: Option, + home_dir: PathBuf, ) -> Self { - let root_dir = root_dir.unwrap_or_else(|| PathBuf::from("/")); + let cwd = root_dir.clone().unwrap_or_else(|| home_dir.clone()); tracing::debug!( user = %user_info.username, mode = %mode, path = %target_path.display(), - root = %root_dir.display(), + chroot = ?root_dir.as_ref().map(|p| p.display().to_string()), + cwd = %cwd.display(), "Creating SCP handler" ); @@ -250,96 +372,72 @@ impl ScpHandler { target_path, user_info, root_dir, + cwd, recursive: false, preserve_times: false, + target_is_directory: false, stored_times: None, } } /// Create a handler from a parsed SCP command. - pub fn from_command(cmd: &ScpCommand, user_info: UserInfo, root_dir: Option) -> Self { - let mut handler = Self::new(cmd.mode, cmd.path.clone(), user_info, root_dir); + pub fn from_command( + cmd: &ScpCommand, + user_info: UserInfo, + root_dir: Option, + home_dir: PathBuf, + ) -> Self { + let mut handler = Self::new(cmd.mode, cmd.path.clone(), user_info, root_dir, home_dir); handler.recursive = cmd.recursive; handler.preserve_times = cmd.preserve_times; + handler.target_is_directory = cmd.target_is_directory; handler } /// Resolve a client path to an absolute filesystem path. /// - /// This method prevents path traversal attacks by: - /// 1. Joining the path with the root directory - /// 2. Normalizing the path (resolving "." and ".." components) - /// 3. Verifying the result is within the root directory - /// 4. If the path exists, canonicalizing to catch symlink attacks + /// Behavior depends on whether a chroot `root_dir` is configured. + /// + /// ## With chroot (`root_dir = Some(root)`): + /// - Absolute client paths inside `root` are honored as-is. + /// - Absolute client paths outside `root` are rejected. + /// - Relative paths are joined with `root`; `..` traversal is clamped. + /// - Existing paths are canonicalized to catch symlink-escape attempts. + /// + /// ## Without chroot (`root_dir = None`): + /// - Absolute paths are used verbatim. + /// - Relative paths are joined with `cwd` (user home directory). + /// - Filesystem permissions remain the access boundary, matching OpenSSH. pub fn resolve_path(&self, path: &Path) -> Result { let path_str = path.to_string_lossy(); - - // Normalize the path manually without following symlinks - let normalized = if path.is_absolute() { - // Strip the leading "/" and join with root - let stripped = path.strip_prefix("/").unwrap_or(path); - self.root_dir.join(stripped) - } else { - self.root_dir.join(path) + let resolved = match self.root_dir.as_deref() { + Some(root) => resolve_chroot_scp(path, root, &self.user_info.username)?, + None => resolve_no_chroot_scp(path, &self.cwd), }; - // Normalize path components (handle ".." and ".") - let mut resolved = PathBuf::new(); - for component in normalized.components() { - match component { - Component::Normal(c) => resolved.push(c), - Component::CurDir => {} // Skip "." - Component::ParentDir => { - // Go up but don't go above root - if resolved.starts_with(&self.root_dir) && resolved != self.root_dir { - resolved.pop(); - } - // If we can't go up, stay at root - if !resolved.starts_with(&self.root_dir) { - resolved = self.root_dir.clone(); - } - } - Component::RootDir => resolved.push("/"), - Component::Prefix(p) => resolved.push(p.as_os_str()), - } - } - - // Ensure the resolved path is within the root - if !resolved.starts_with(&self.root_dir) { - tracing::warn!( - event = "path_traversal_attempt", - user = %self.user_info.username, - requested = %path_str, - resolved = %resolved.display(), - root = %self.root_dir.display(), - "Security: path traversal attempt blocked" - ); - anyhow::bail!("Access denied: path outside root"); - } - - // If the path exists, canonicalize it to catch symlink attacks - // This prevents an attacker from creating symlinks that point outside the root + // Canonicalize existing paths to catch symlink-escape attempts. Only + // enforce the in-root invariant when chroot is enabled; without + // chroot, canonicalization is purely informational. if resolved.exists() { match std::fs::canonicalize(&resolved) { Ok(canonical) => { - if !canonical.starts_with(&self.root_dir) { + if let Some(root) = self.root_dir.as_deref() + && !canonical.starts_with(root) + { tracing::warn!( event = "symlink_escape_attempt", user = %self.user_info.username, requested = %path_str, resolved = %resolved.display(), canonical = %canonical.display(), - root = %self.root_dir.display(), + root = %root.display(), "Security: symlink escape attempt blocked" ); anyhow::bail!("Access denied: symlink target outside root"); } - // Use the canonical path for existing files return Ok(canonical); } Err(e) => { - // If canonicalization fails, proceed with the resolved path - // This handles broken symlinks and permission issues tracing::debug!( path = %resolved.display(), error = %e, @@ -458,10 +556,16 @@ impl ScpHandler { match first_byte { b'C' => { // File: C + // At top level (dir_stack.len() == 1) we honor the + // single-file vs. directory distinction. Inside a + // recursively descended directory, the filename always + // joins to the current directory. + let at_top_level = dir_stack.len() == 1; if let Err(e) = self .receive_file( &line, ¤t_dir, + at_top_level, channel_id, &handle, &mut buffer, @@ -528,10 +632,19 @@ impl ScpHandler { } /// Receive a single file. + /// + /// `at_top_level` indicates the C-record is for the top-level destination + /// supplied by the client, not a file inside a recursively descended + /// directory. At top level, the destination might be either a file + /// (e.g. `scp local.bin host:/tmp/dest.bin`) or a directory (e.g. + /// `scp local.bin host:/tmp/dest/`); the source filename should only be + /// appended in the directory case. + #[allow(clippy::too_many_arguments)] async fn receive_file( &mut self, header: &str, target_dir: &Path, + at_top_level: bool, channel_id: ChannelId, handle: &Handle, buffer: &mut Vec, @@ -574,10 +687,26 @@ impl ScpHandler { anyhow::bail!("File too large"); } - let target_path = target_dir.join(filename); + // Decide whether to treat the resolved target as a directory and + // append the source filename, or write directly to it as a single + // file. This is the fix for the "filename appended to single-file + // destination" bug. + let target_path = + if at_top_level && !self.target_is_directory && !self.recursive && !target_dir.is_dir() + { + // Single-file destination: write directly to the resolved path. + target_dir.to_path_buf() + } else { + // Directory destination (existing dir, `-d`/`-r` flag, or nested + // descent): append the source filename. + target_dir.join(filename) + }; - // Ensure target is within root - if !target_path.starts_with(&self.root_dir) { + // When chroot is configured, ensure the final path stays inside the + // chroot. Without chroot, the OS handles access checks. + if let Some(root) = self.root_dir.as_deref() + && !target_path.starts_with(root) + { anyhow::bail!("Access denied: path outside root"); } @@ -705,8 +834,11 @@ impl ScpHandler { let new_dir = current_dir.join(dirname); - // Ensure target is within root - if !new_dir.starts_with(&self.root_dir) { + // When chroot is configured, ensure the new directory stays inside + // it. Without chroot, the OS enforces access via filesystem perms. + if let Some(root) = self.root_dir.as_deref() + && !new_dir.starts_with(root) + { anyhow::bail!("Access denied: path outside root"); } @@ -1127,16 +1259,33 @@ mod tests { assert!(!ScpCommand::is_scp_command("scpfoo")); } - #[test] - fn test_handler_resolve_path_basic() { + /// Build a handler with chroot enabled at `/home/testuser`. + fn chroot_handler(target_path: PathBuf) -> ScpHandler { let user = UserInfo::new("testuser"); - let handler = ScpHandler::new( + ScpHandler::new( ScpMode::Sink, - PathBuf::from("/tmp"), + target_path, user, Some(PathBuf::from("/home/testuser")), - ); + PathBuf::from("/home/testuser"), + ) + } + + /// Build a handler with no chroot, home dir at `/home/testuser`. + fn no_chroot_handler(target_path: PathBuf) -> ScpHandler { + let user = UserInfo::new("testuser"); + ScpHandler::new( + ScpMode::Sink, + target_path, + user, + None, + PathBuf::from("/home/testuser"), + ) + } + #[test] + fn chroot_relative_path_resolves_under_root() { + let handler = chroot_handler(PathBuf::from("/tmp")); let result = handler .resolve_path(Path::new("documents/file.txt")) .unwrap(); @@ -1144,46 +1293,77 @@ mod tests { } #[test] - fn test_handler_resolve_path_absolute() { - let user = UserInfo::new("testuser"); - let handler = ScpHandler::new( - ScpMode::Sink, - PathBuf::from("/tmp"), - user, - Some(PathBuf::from("/home/testuser")), - ); - + fn chroot_absolute_inside_root_is_returned_verbatim() { + // Bug fix: an absolute client path inside the chroot must NOT be + // re-rooted under itself. /home/testuser/file.bin must resolve to + // /home/testuser/file.bin, not /home/testuser/home/testuser/file.bin. + let handler = chroot_handler(PathBuf::from("/home/testuser/file.bin")); let result = handler - .resolve_path(Path::new("/documents/file.txt")) + .resolve_path(Path::new("/home/testuser/file.bin")) .unwrap(); - assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); + assert_eq!(result, PathBuf::from("/home/testuser/file.bin")); } #[test] - fn test_handler_resolve_path_traversal_blocked() { - let user = UserInfo::new("testuser"); - let handler = ScpHandler::new( - ScpMode::Sink, - PathBuf::from("/tmp"), - user, - Some(PathBuf::from("/home/testuser")), + fn chroot_absolute_outside_root_is_rejected() { + let handler = chroot_handler(PathBuf::from("/etc/passwd")); + let err = handler.resolve_path(Path::new("/etc/passwd")).unwrap_err(); + assert!( + err.to_string().contains("outside root"), + "expected rejection, got: {err}" ); - // Path traversal attempts are clamped to root + let err = handler + .resolve_path(Path::new("/tmp/file.bin")) + .unwrap_err(); + assert!(err.to_string().contains("outside root")); + } + + #[test] + fn chroot_relative_traversal_is_clamped_to_root() { + let handler = chroot_handler(PathBuf::from("/tmp")); let result = handler.resolve_path(Path::new("../etc/passwd")).unwrap(); assert_eq!(result, PathBuf::from("/home/testuser/etc/passwd")); assert!(result.starts_with("/home/testuser")); } #[test] - fn test_handler_from_command() { - let cmd = ScpCommand::parse("scp -rp -t /tmp/upload").unwrap(); - let user = UserInfo::new("testuser"); - let handler = ScpHandler::from_command(&cmd, user, Some(PathBuf::from("/home/testuser"))); + fn no_chroot_absolute_path_used_verbatim() { + // OpenSSH-compatible: absolute paths are not re-rooted. + let handler = no_chroot_handler(PathBuf::from("/home/work/file.bin")); + let result = handler + .resolve_path(Path::new("/home/work/file.bin")) + .unwrap(); + assert_eq!(result, PathBuf::from("/home/work/file.bin")); + + let handler = no_chroot_handler(PathBuf::from("/tmp/file.bin")); + let result = handler.resolve_path(Path::new("/tmp/file.bin")).unwrap(); + assert_eq!(result, PathBuf::from("/tmp/file.bin")); + } + #[test] + fn no_chroot_relative_path_resolves_from_home() { + let handler = no_chroot_handler(PathBuf::from("documents/file.txt")); + let result = handler + .resolve_path(Path::new("documents/file.txt")) + .unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); + } + + #[test] + fn from_command_threads_chroot_and_target_is_directory() { + let cmd = ScpCommand::parse("scp -rp -d -t /tmp/upload").unwrap(); + let user = UserInfo::new("testuser"); + let handler = ScpHandler::from_command( + &cmd, + user, + Some(PathBuf::from("/home/testuser")), + PathBuf::from("/home/testuser"), + ); assert_eq!(handler.mode, ScpMode::Sink); assert!(handler.recursive); assert!(handler.preserve_times); + assert!(handler.target_is_directory); } #[test] @@ -1200,9 +1380,74 @@ mod tests { PathBuf::from("/tmp"), user, Some(PathBuf::from("/home/testuser")), + PathBuf::from("/home/testuser"), ); handler.parse_times("T1234567890 0 1234567800 0").unwrap(); assert_eq!(handler.stored_times, Some((1234567890, 1234567800))); } + + // --- Filesystem-backed tests for receive_file destination semantics ----- + + /// Verify the SCP single-file vs. directory destination decision matches + /// the issue's acceptance criteria. Uses real temp directories so we can + /// exercise both the existing-directory and non-existent-file cases. + #[test] + fn receive_file_destination_decision() { + use tempfile::tempdir; + let dir = tempdir().unwrap(); + + // Case 1: target is a non-existent file (single-file destination). + // Without -d/-r and target_dir is not a directory -> write directly. + let target = dir.path().join("dest.bin"); + assert!(!target.exists()); + let at_top_level = true; + let target_is_directory = false; + let recursive = false; + let resolved = single_file_decision( + &target, + at_top_level, + target_is_directory, + recursive, + "src.bin", + ); + assert_eq!(resolved, target); + + // Case 2: target is an existing directory -> append filename. + let target = dir.path().to_path_buf(); + let resolved = single_file_decision( + &target, + at_top_level, + target_is_directory, + recursive, + "src.bin", + ); + assert_eq!(resolved, target.join("src.bin")); + + // Case 3: target is non-existent but `-d` was supplied -> append. + let target = dir.path().join("dest_dir"); + let resolved = single_file_decision(&target, at_top_level, true, false, "src.bin"); + assert_eq!(resolved, target.join("src.bin")); + + // Case 4: recursive descent (not top level) -> always append. + let target = dir.path().join("subdir"); + let resolved = single_file_decision(&target, false, false, true, "src.bin"); + assert_eq!(resolved, target.join("src.bin")); + } + + /// Mirror of the destination decision logic from `receive_file`. Kept + /// alongside it so the tests cover the same condition the runtime uses. + fn single_file_decision( + target_dir: &Path, + at_top_level: bool, + target_is_directory: bool, + recursive: bool, + filename: &str, + ) -> PathBuf { + if at_top_level && !target_is_directory && !recursive && !target_dir.is_dir() { + target_dir.to_path_buf() + } else { + target_dir.join(filename) + } + } } diff --git a/src/server/sftp.rs b/src/server/sftp.rs index 3820acdd..401c54be 100644 --- a/src/server/sftp.rs +++ b/src/server/sftp.rs @@ -30,7 +30,15 @@ //! use std::path::PathBuf; //! //! let user = UserInfo::new("testuser"); -//! let handler = SftpHandler::new(user, Some(PathBuf::from("/home/testuser"))); +//! // Without chroot (OpenSSH-compatible behavior): +//! let handler = SftpHandler::new(user.clone(), None, PathBuf::from("/home/testuser")); +//! +//! // With chroot: +//! let handler = SftpHandler::new( +//! user, +//! Some(PathBuf::from("/srv/sftp")), +//! PathBuf::from("/home/testuser"), +//! ); //! ``` use std::collections::HashMap; @@ -159,6 +167,150 @@ const MAX_HANDLES: usize = 1000; /// Maximum read buffer size (64KB) to prevent memory exhaustion. const MAX_READ_SIZE: u32 = 65536; +/// Normalize a path's `..` and `.` components without touching the filesystem. +/// +/// This is a logical normalization that does not follow symlinks. Used as +/// a building block for both chrooted and non-chrooted resolution. +fn normalize_components(path: &Path) -> PathBuf { + use std::path::Component; + let mut out = PathBuf::new(); + for component in path.components() { + match component { + Component::Normal(c) => out.push(c), + Component::CurDir => {} + Component::ParentDir => { + // Pop only normal components; never above the root prefix. + if !out.pop() { + out.push(".."); + } + } + Component::RootDir => out.push("/"), + Component::Prefix(p) => out.push(p.as_os_str()), + } + } + out +} + +/// Resolve a client-supplied path against a chroot root. +/// +/// - Plain `/` (the chroot's pseudo-root in the client's view, also returned +/// by `realpath`) maps to `root`. +/// - Absolute paths inside `root` are honored as-is (no doubling). +/// - Absolute paths outside `root` are rejected. +/// - Relative paths are joined with `root` and normalized. +/// - `..` traversal is clamped to `root`. +fn resolve_chroot(requested: &Path, root: &Path) -> Result { + use std::path::Component; + + // Treat empty path the same as "." to keep parity with no-chroot mode. + let requested = if requested.as_os_str().is_empty() { + Path::new(".") + } else { + requested + }; + + if requested.is_absolute() { + // Plain "/" is the client's view of the chroot root (returned by + // `realpath`). Map it back to the actual chroot directory so the + // realpath-roundtrip stays consistent. + if requested == Path::new("/") { + return Ok(root.to_path_buf()); + } + + // Absolute paths inside the chroot are honored verbatim. Anything + // outside is rejected so the chroot enforces a containment boundary + // rather than silently re-rooting the path. + let normalized = normalize_components(requested); + if normalized == root || normalized.starts_with(root) { + tracing::trace!( + requested = %requested.display(), + resolved = %normalized.display(), + "Resolved absolute path inside chroot" + ); + return Ok(normalized); + } + tracing::warn!( + event = "chroot_escape_blocked", + requested = %requested.display(), + root = %root.display(), + "Absolute path outside chroot rejected" + ); + return Err(SftpError::permission_denied( + "Access denied: path outside root", + )); + } + + // Relative path: join with root, then walk components clamping `..` + // so traversal cannot escape the chroot. This preserves the original + // security guarantee. + let mut resolved = root.to_path_buf(); + for component in requested.components() { + match component { + Component::Normal(c) => resolved.push(c), + Component::CurDir => {} + Component::ParentDir => { + if resolved != root { + resolved.pop(); + } + if !resolved.starts_with(root) { + resolved = root.to_path_buf(); + } + } + // Relative paths shouldn't carry these, but ignore safely. + Component::RootDir | Component::Prefix(_) => {} + } + } + + if !resolved.starts_with(root) { + tracing::warn!( + event = "path_traversal_blocked", + requested = %requested.display(), + resolved = %resolved.display(), + root = %root.display(), + "Resolved path escaped chroot" + ); + return Err(SftpError::permission_denied( + "Access denied: path outside root", + )); + } + + tracing::trace!( + requested = %requested.display(), + resolved = %resolved.display(), + "Resolved relative path under chroot" + ); + Ok(resolved) +} + +/// Resolve a client-supplied path without a chroot. +/// +/// - Absolute paths are used verbatim, after normalizing `.` and `..`. +/// - Relative paths join with `cwd` (the user's home directory by default) +/// and are normalized the same way. +/// +/// This matches OpenSSH `sftp-server` semantics: filesystem permissions are +/// the only access boundary. +fn resolve_no_chroot(requested: &Path, cwd: &Path) -> PathBuf { + let requested = if requested.as_os_str().is_empty() { + Path::new(".") + } else { + requested + }; + + let joined = if requested.is_absolute() { + requested.to_path_buf() + } else { + cwd.join(requested) + }; + let normalized = normalize_components(&joined); + tracing::trace!( + requested = %requested.display(), + resolved = %normalized.display(), + "Resolved path (no chroot)" + ); + normalized +} + /// SFTP server handler. /// /// Implements the SFTP protocol for file transfer operations with @@ -167,8 +319,20 @@ pub struct SftpHandler { /// Current user information. user_info: UserInfo, - /// Root directory for SFTP operations (chroot-like behavior). - root_dir: PathBuf, + /// Optional chroot root for SFTP operations. + /// + /// When `Some(path)`, all client paths are confined to this directory. + /// When `None`, the handler runs without chroot (OpenSSH-compatible), + /// using `cwd` as the base for relative paths and honoring absolute + /// client paths verbatim. + root_dir: Option, + + /// Base directory for resolving relative client paths. + /// + /// When `root_dir` is `Some(_)`, this is set to the chroot root. + /// When `root_dir` is `None`, this is the user's home directory and + /// matches OpenSSH's `chdir` behavior on session start. + cwd: PathBuf, /// Open file and directory handles (shared for async access). handles: Arc>>, @@ -183,20 +347,23 @@ impl SftpHandler { /// # Arguments /// /// * `user_info` - Information about the authenticated user - /// * `root_dir` - Optional root directory for chroot-like behavior. - /// If None, defaults to filesystem root ("/"). - pub fn new(user_info: UserInfo, root_dir: Option) -> Self { - let root_dir = root_dir.unwrap_or_else(|| PathBuf::from("/")); + /// * `root_dir` - Optional chroot root. When `None`, no chroot is applied. + /// * `home_dir` - The user's home directory; used as the base for relative + /// paths when chroot is disabled. + pub fn new(user_info: UserInfo, root_dir: Option, home_dir: PathBuf) -> Self { + let cwd = root_dir.clone().unwrap_or_else(|| home_dir.clone()); tracing::debug!( user = %user_info.username, - root = %root_dir.display(), + chroot = ?root_dir.as_ref().map(|p| p.display().to_string()), + cwd = %cwd.display(), "Creating SFTP handler" ); Self { user_info, root_dir, + cwd, handles: Arc::new(Mutex::new(HashMap::new())), handle_counter: 0, } @@ -208,77 +375,61 @@ impl SftpHandler { format!("h{}", self.handle_counter) } - /// Static helper for path resolution (used in symlink validation). - fn resolve_path_static(path: &str, root_dir: &Path) -> Result { - let path = Path::new(path); - - // Normalize the path manually without following symlinks - let normalized = if path.is_absolute() { - // Strip the leading "/" and join with root - let stripped = path.strip_prefix("/").unwrap_or(path); - root_dir.join(stripped) - } else { - root_dir.join(path) - }; - - // Normalize path components (handle ".." and ".") - let mut resolved = PathBuf::new(); - for component in normalized.components() { - use std::path::Component; - match component { - Component::Normal(c) => resolved.push(c), - Component::CurDir => {} // Skip "." - Component::ParentDir => { - // Go up but don't go above root - if resolved.starts_with(root_dir) && resolved != root_dir { - resolved.pop(); - } - // If we can't go up, stay at root - if !resolved.starts_with(root_dir) { - resolved = root_dir.to_path_buf(); - } - } - Component::RootDir => resolved.push("/"), - Component::Prefix(p) => resolved.push(p.as_os_str()), - } - } - - // Ensure the resolved path is within the root - if !resolved.starts_with(root_dir) { - tracing::warn!( - requested = %path.display(), - resolved = %resolved.display(), - root = %root_dir.display(), - "Path traversal attempt detected" - ); - return Err(SftpError::permission_denied( - "Access denied: path outside root", - )); + /// Resolve a client path to an absolute filesystem path. + /// + /// Behavior depends on whether a chroot `root_dir` is configured. + /// + /// ## With chroot (`root_dir = Some(root)`): + /// - Absolute client paths inside `root` are honored as-is. + /// - Absolute client paths outside `root` are rejected with + /// `permission_denied` (matching OpenSSH `ChrootDirectory` semantics). + /// - Relative paths are joined with `root`. + /// - `..` traversal is clamped to `root` (cannot escape). + /// + /// ## Without chroot (`root_dir = None`): + /// - Absolute paths are used verbatim. + /// - Relative paths are joined with `cwd` (the user's home directory). + /// - `..` traversal is normalized but not clamped (filesystem permissions + /// remain the access boundary, matching OpenSSH). + fn resolve_path_static( + path: &str, + root_dir: Option<&Path>, + cwd: &Path, + ) -> Result { + let requested = Path::new(path); + + match root_dir { + Some(root) => resolve_chroot(requested, root), + None => Ok(resolve_no_chroot(requested, cwd)), } - - tracing::trace!( - requested = %path.display(), - resolved = %resolved.display(), - "Resolved path" - ); - - Ok(resolved) } /// Resolve a client path to an absolute filesystem path. /// - /// This method prevents path traversal attacks by: - /// 1. Joining the path with the root directory - /// 2. Normalizing the path (resolving "." and ".." components) - /// 3. Verifying the result is within the root directory + /// See [`Self::resolve_path_static`] for the full semantics. This is the + /// instance-method wrapper used throughout the handler trait impl. + pub fn resolve_path(&self, path: &str) -> Result { + Self::resolve_path_static(path, self.root_dir.as_deref(), &self.cwd) + } + + /// Validate that a symlink's resolved target stays inside the chroot, if + /// chroot is enabled. /// - /// # Security + /// Returns `Ok(())` when: + /// - chroot is disabled (no enforcement applies), or + /// - the resolved target lives under `root_dir`. /// - /// This is critical for security. The resolved path must always - /// start with the root directory to prevent access to files - /// outside the allowed area. - pub fn resolve_path(&self, path: &str) -> Result { - Self::resolve_path_static(path, &self.root_dir) + /// Returns `permission_denied` when the target escapes a configured chroot. + fn ensure_target_in_root( + root_dir: Option<&Path>, + resolved_target: &Path, + ) -> Result<(), SftpError> { + match root_dir { + Some(root) if !resolved_target.starts_with(root) => Err(SftpError::permission_denied( + "Symlink target outside allowed directory", + )), + _ => Ok(()), + } } /// Convert file metadata to SFTP FileAttributes. @@ -378,6 +529,7 @@ impl russh_sftp::server::Handler for SftpHandler { let handle_id = self.new_handle(); let handles = Arc::clone(&self.handles); let root_dir = self.root_dir.clone(); + let cwd = self.cwd.clone(); tracing::debug!( user = %self.user_info.username, @@ -403,27 +555,28 @@ impl russh_sftp::server::Handler for SftpHandler { if let Ok(meta) = metadata && meta.is_symlink() { - // Follow the symlink and ensure target is within root + // Follow the symlink and ensure target is within root (if any) let target = fs::read_link(&path).await?; let resolved_target = if target.is_absolute() { target } else { - // Resolve relative symlink from the symlink's directory - let base = path.parent().unwrap_or(&root_dir); + // Resolve relative symlink from the symlink's directory. + // Fall back to cwd when the parent isn't accessible. + let base = path.parent().unwrap_or(&cwd); let joined = base.join(&target); // Use tokio's canonicalize for async operation tokio::fs::canonicalize(&joined).await.unwrap_or(target) }; - if !resolved_target.starts_with(&root_dir) { + if let Err(e) = + SftpHandler::ensure_target_in_root(root_dir.as_deref(), &resolved_target) + { tracing::warn!( path = %path.display(), target = %resolved_target.display(), "Symlink target outside root directory" ); - return Err(SftpError::permission_denied( - "Symlink target outside allowed directory", - )); + return Err(e); } } @@ -636,17 +789,29 @@ impl russh_sftp::server::Handler for SftpHandler { }); } - // Add ".." entry only if parent is within root + // Add ".." entry. With chroot, only include the parent if it + // remains inside the chroot; otherwise reuse the directory's own + // metadata so the listing doesn't leak the chroot boundary. + // Without chroot, fall back to ordinary parent semantics. if let Some(parent) = resolved_path.parent() { - if parent.starts_with(&root_dir) { + let parent_inside_root = root_dir + .as_ref() + .map(|root| parent.starts_with(root)) + .unwrap_or(true); + let at_root_boundary = root_dir + .as_ref() + .map(|root| resolved_path == *root) + .unwrap_or(false); + + if parent_inside_root { if let Ok(meta) = fs::symlink_metadata(parent).await { entries.push(DirEntryInfo { filename: "..".to_string(), attrs: SftpHandler::metadata_to_attrs(&meta), }); } - } else if resolved_path == root_dir { - // At root boundary, use root's own metadata for ".." + } else if at_root_boundary { + // At chroot boundary, mirror the directory's own metadata. if let Ok(meta) = fs::symlink_metadata(&resolved_path).await { entries.push(DirEntryInfo { filename: "..".to_string(), @@ -745,6 +910,7 @@ impl russh_sftp::server::Handler for SftpHandler { ) -> impl std::future::Future> + Send { let resolved = self.resolve_path(&path); let root_dir = self.root_dir.clone(); + let cwd = self.cwd.clone(); async move { let path = resolved?; @@ -754,24 +920,25 @@ impl russh_sftp::server::Handler for SftpHandler { if symlink_meta.is_symlink() { // Follow the symlink and validate the target is within root + // (when chroot is enabled). let target = fs::read_link(&path).await?; let resolved_target = if target.is_absolute() { target } else { - let base = path.parent().unwrap_or(&root_dir); + let base = path.parent().unwrap_or(&cwd); let joined = base.join(&target); tokio::fs::canonicalize(&joined).await.unwrap_or(target) }; - if !resolved_target.starts_with(&root_dir) { + if let Err(e) = + SftpHandler::ensure_target_in_root(root_dir.as_deref(), &resolved_target) + { tracing::warn!( path = %path.display(), target = %resolved_target.display(), "stat: Symlink target outside root directory" ); - return Err(SftpError::permission_denied( - "Symlink target outside allowed directory", - )); + return Err(e); } } @@ -839,14 +1006,16 @@ impl russh_sftp::server::Handler for SftpHandler { async move { let full_path = resolved?; - // Return path relative to root (as client sees it) - let display_path = if full_path == root_dir { - "/".to_string() - } else { - full_path - .strip_prefix(&root_dir) + // Return path the way the client should see it. With chroot, + // strip the root prefix so the client sees a path rooted at "/". + // Without chroot, expose the resolved absolute path verbatim. + let display_path = match root_dir.as_ref() { + Some(root) if full_path == *root => "/".to_string(), + Some(root) => full_path + .strip_prefix(root) .map(|p| format!("/{}", p.display())) - .unwrap_or_else(|_| full_path.display().to_string()) + .unwrap_or_else(|_| full_path.display().to_string()), + None => full_path.display().to_string(), }; // Get attributes if the path exists @@ -1081,35 +1250,36 @@ impl russh_sftp::server::Handler for SftpHandler { let path = resolved?; let target = fs::read_link(&path).await?; - // If target is absolute and outside root, convert to safe relative path - let safe_target = if target.is_absolute() { - // Resolve the absolute target - let resolved_target = if let Ok(canonical) = tokio::fs::canonicalize(&target).await - { - canonical - } else { - target.clone() - }; - - // If target is outside root, redact it - if !resolved_target.starts_with(&root_dir) { - tracing::warn!( - symlink = %path.display(), - target = %resolved_target.display(), - "readlink: Symlink target outside root, redacting" - ); - // Return a safe placeholder instead of exposing system paths - PathBuf::from("[target outside root]") - } else { - // Convert to path relative to root for safe display - resolved_target - .strip_prefix(&root_dir) - .map(PathBuf::from) - .unwrap_or(target) + // With chroot: redact targets outside the chroot and rewrite + // in-chroot targets to a chroot-relative path. + // Without chroot: pass the link target through verbatim, the same + // way OpenSSH does. + let safe_target = match (root_dir.as_ref(), target.is_absolute()) { + (Some(root), true) => { + let resolved_target = + if let Ok(canonical) = tokio::fs::canonicalize(&target).await { + canonical + } else { + target.clone() + }; + + if !resolved_target.starts_with(root) { + tracing::warn!( + symlink = %path.display(), + target = %resolved_target.display(), + "readlink: Symlink target outside root, redacting" + ); + PathBuf::from("[target outside root]") + } else { + resolved_target + .strip_prefix(root) + .map(PathBuf::from) + .unwrap_or(target) + } } - } else { - // Relative targets are safe to expose as-is - target + // Without chroot, or for relative targets: pass the link + // target through unchanged (matches OpenSSH behavior). + _ => target, }; let attrs = FileAttributes { @@ -1144,85 +1314,82 @@ impl russh_sftp::server::Handler for SftpHandler { let link_resolved = self.resolve_path(&linkpath); let user = self.user_info.username.clone(); let root_dir = self.root_dir.clone(); + let cwd = self.cwd.clone(); async move { let link_path = link_resolved?; - // Validate the target path to prevent symlink attacks + // Validate the symlink target. With chroot, both absolute and + // relative targets must resolve inside the chroot. Without chroot, + // mirror OpenSSH and let the kernel + filesystem permissions + // enforce access; we still create the link with the target as-is. let target = Path::new(&targetpath); - // If target is absolute, ensure it's within root - if target.is_absolute() { - // Resolve target through our path resolution to ensure it's within root - let target_str = target.to_string_lossy(); - let resolved_target = match SftpHandler::resolve_path_static(&target_str, &root_dir) - { - Ok(p) => p, - Err(e) => { + if let Some(root) = root_dir.as_deref() { + if target.is_absolute() { + let resolved_target = resolve_chroot(target, root).inspect_err(|_| { tracing::warn!( user = %user, link = %link_path.display(), target = %targetpath, - "Rejected symlink with target outside root" + "Rejected symlink with absolute target outside chroot" ); - return Err(e); + })?; + if !resolved_target.starts_with(root) { + tracing::warn!( + user = %user, + link = %link_path.display(), + target = %targetpath, + resolved = %resolved_target.display(), + "Symlink target resolves outside chroot" + ); + return Err(SftpError::permission_denied( + "Symlink target must be within root directory", + )); } - }; - - if !resolved_target.starts_with(&root_dir) { - tracing::warn!( - user = %user, - link = %link_path.display(), - target = %targetpath, - resolved = %resolved_target.display(), - "Symlink target resolves outside root" - ); - return Err(SftpError::permission_denied( - "Symlink target must be within root directory", - )); - } - } else { - // For relative targets, verify they resolve within root when combined with link parent - let link_parent = link_path.parent().unwrap_or(&root_dir); - let mut resolved = link_parent.to_path_buf(); - - for component in target.components() { - use std::path::Component; - match component { - Component::Normal(c) => resolved.push(c), - Component::CurDir => {} - Component::ParentDir - if (!resolved.pop() || !resolved.starts_with(&root_dir)) => - { - tracing::warn!( - user = %user, - link = %link_path.display(), - target = %targetpath, - "Relative symlink target escapes root" - ); - return Err(SftpError::permission_denied( - "Symlink target must be within root directory", - )); + } else { + // Relative target: combine with the link's parent directory + // (or fall back to cwd) and ensure the result stays in + // the chroot. + let link_parent = link_path.parent().unwrap_or(&cwd); + let mut resolved = link_parent.to_path_buf(); + for component in target.components() { + use std::path::Component; + match component { + Component::Normal(c) => resolved.push(c), + Component::CurDir => {} + Component::ParentDir + if (!resolved.pop() || !resolved.starts_with(root)) => + { + tracing::warn!( + user = %user, + link = %link_path.display(), + target = %targetpath, + "Relative symlink target escapes chroot" + ); + return Err(SftpError::permission_denied( + "Symlink target must be within root directory", + )); + } + _ => {} } - _ => {} } - } - - if !resolved.starts_with(&root_dir) { - tracing::warn!( - user = %user, - link = %link_path.display(), - target = %targetpath, - resolved = %resolved.display(), - "Relative symlink resolves outside root" - ); - return Err(SftpError::permission_denied( - "Symlink target must be within root directory", - )); + if !resolved.starts_with(root) { + tracing::warn!( + user = %user, + link = %link_path.display(), + target = %targetpath, + resolved = %resolved.display(), + "Relative symlink resolves outside chroot" + ); + return Err(SftpError::permission_denied( + "Symlink target must be within root directory", + )); + } } } - // Create symbolic link (target is used as-is, but validated) + // Create symbolic link (target is stored as-is, validation above) #[cfg(unix)] tokio::fs::symlink(&targetpath, &link_path).await?; @@ -1250,81 +1417,137 @@ impl russh_sftp::server::Handler for SftpHandler { mod tests { use super::*; - fn test_handler() -> SftpHandler { + /// Build a handler with chroot enabled at `/home/testuser`. + fn chroot_handler() -> SftpHandler { let user = UserInfo::new("testuser"); - SftpHandler::new(user, Some(PathBuf::from("/home/testuser"))) + SftpHandler::new( + user, + Some(PathBuf::from("/home/testuser")), + PathBuf::from("/home/testuser"), + ) } - #[test] - fn test_resolve_path_basic() { - let handler = test_handler(); + /// Build a handler with no chroot, home dir at `/home/testuser`. + fn no_chroot_handler() -> SftpHandler { + let user = UserInfo::new("testuser"); + SftpHandler::new(user, None, PathBuf::from("/home/testuser")) + } - // Basic path resolution + // --- Chroot mode tests -------------------------------------------------- + + #[test] + fn chroot_relative_path_resolves_under_root() { + let handler = chroot_handler(); let result = handler.resolve_path("documents/file.txt").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); } #[test] - fn test_resolve_path_absolute() { - let handler = test_handler(); + fn chroot_absolute_inside_root_is_returned_verbatim() { + // The bug fix: an absolute client path inside the chroot must NOT be + // re-rooted (no path doubling). /home/testuser/file.bin must resolve + // to /home/testuser/file.bin, not /home/testuser/home/testuser/file.bin. + let handler = chroot_handler(); + let result = handler.resolve_path("/home/testuser/file.bin").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/file.bin")); + } - // Absolute path should be relative to root - let result = handler.resolve_path("/documents/file.txt").unwrap(); - assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); + #[test] + fn chroot_absolute_at_root_resolves_to_root() { + let handler = chroot_handler(); + let result = handler.resolve_path("/home/testuser").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser")); } #[test] - fn test_resolve_path_traversal_blocked() { - let handler = test_handler(); + fn chroot_absolute_outside_root_is_rejected() { + let handler = chroot_handler(); + let err = handler.resolve_path("/etc/passwd").unwrap_err(); + assert_eq!(err.code, StatusCode::PermissionDenied); - // Path traversal attempts are clamped to root (security measure) - // "../etc/passwd" -> trying to escape, clamped to root, then "etc/passwd" - // Results in "/home/testuser/etc/passwd" - stays within jail + let err = handler.resolve_path("/tmp/file.bin").unwrap_err(); + assert_eq!(err.code, StatusCode::PermissionDenied); + } + + #[test] + fn chroot_relative_traversal_is_clamped_to_root() { + let handler = chroot_handler(); + // "../etc/passwd" tries to escape, gets clamped at root, then etc/passwd let result = handler.resolve_path("../etc/passwd").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser/etc/passwd")); + assert!(result.starts_with("/home/testuser")); - // "documents/../../etc/passwd" -> go into docs, up twice (clamped at root), then etc/passwd - let result = handler.resolve_path("documents/../../etc/passwd").unwrap(); + // Multiple parent refs all get clamped + let result = handler + .resolve_path("../../../../../../../etc/passwd") + .unwrap(); assert_eq!(result, PathBuf::from("/home/testuser/etc/passwd")); - - // All paths stay within the root directory (the security guarantee) assert!(result.starts_with("/home/testuser")); } #[test] - fn test_resolve_path_double_dots() { - let handler = test_handler(); - - // ".." that doesn't escape should work + fn chroot_relative_in_bounds_double_dots_work() { + let handler = chroot_handler(); let result = handler.resolve_path("a/b/../c").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser/a/c")); } #[test] - fn test_resolve_path_root() { - let handler = test_handler(); - - // Root path + fn chroot_root_path_resolves_to_chroot() { + let handler = chroot_handler(); let result = handler.resolve_path("/").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser")); - let result = handler.resolve_path(".").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser")); } + // --- No-chroot mode tests ----------------------------------------------- + #[test] - fn test_resolve_path_many_parent_refs() { - let handler = test_handler(); + fn no_chroot_absolute_path_used_verbatim() { + // OpenSSH-compatible: absolute paths are not re-rooted. + let handler = no_chroot_handler(); + let result = handler.resolve_path("/etc/passwd").unwrap(); + assert_eq!(result, PathBuf::from("/etc/passwd")); - // Many ".." are clamped to stay within root - security is maintained - let result = handler - .resolve_path("../../../../../../../etc/passwd") - .unwrap(); - // All the ".." attempts are stopped at root, then "etc/passwd" is appended - assert_eq!(result, PathBuf::from("/home/testuser/etc/passwd")); + let result = handler.resolve_path("/tmp/file.bin").unwrap(); + assert_eq!(result, PathBuf::from("/tmp/file.bin")); - // The security guarantee: path never escapes root - assert!(result.starts_with("/home/testuser")); + let result = handler.resolve_path("/home/testuser/file.bin").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/file.bin")); + } + + #[test] + fn no_chroot_relative_path_resolves_from_home() { + let handler = no_chroot_handler(); + let result = handler.resolve_path("documents/file.txt").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); + } + + #[test] + fn no_chroot_relative_double_dots_normalize() { + // Without chroot, `..` normalizes against the cwd (home directory) + // exactly as the kernel would. This matches OpenSSH's sftp-server. + let handler = no_chroot_handler(); + let result = handler.resolve_path("../testuser/file.txt").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/file.txt")); + } + + #[test] + fn no_chroot_dot_resolves_to_home() { + let handler = no_chroot_handler(); + let result = handler.resolve_path(".").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser")); + let result = handler.resolve_path("").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser")); + } + + #[test] + fn no_chroot_handler_creates_with_explicit_home() { + let user = UserInfo::new("alice"); + let handler = SftpHandler::new(user, None, PathBuf::from("/home/alice")); + let result = handler.resolve_path("file.txt").unwrap(); + assert_eq!(result, PathBuf::from("/home/alice/file.txt")); } #[test] @@ -1340,7 +1563,7 @@ mod tests { #[test] fn test_new_handle_uniqueness() { - let mut handler = test_handler(); + let mut handler = chroot_handler(); let h1 = handler.new_handle(); let h2 = handler.new_handle(); @@ -1428,7 +1651,7 @@ mod tests { #[test] fn test_resolve_path_empty_string() { - let handler = test_handler(); + let handler = chroot_handler(); // Empty string should resolve to root let result = handler.resolve_path("").unwrap(); @@ -1437,7 +1660,7 @@ mod tests { #[test] fn test_resolve_path_special_characters() { - let handler = test_handler(); + let handler = chroot_handler(); // Path with spaces let result = handler.resolve_path("my documents/file name.txt").unwrap(); @@ -1456,7 +1679,7 @@ mod tests { #[test] fn test_resolve_path_encoded_traversal() { - let handler = test_handler(); + let handler = chroot_handler(); // Encoded patterns should be treated as literal path components // (the path component itself is ".." not percent encoding) @@ -1468,19 +1691,24 @@ mod tests { #[test] fn test_resolve_path_multiple_slashes() { - let handler = test_handler(); + let handler = chroot_handler(); - // Multiple consecutive slashes should be normalized - let result = handler.resolve_path("///documents///file.txt").unwrap(); - // Path normalization in std collapses multiple slashes + // Relative path with consecutive slashes - normalization collapses them. + let result = handler.resolve_path("documents///file.txt").unwrap(); assert!(result.starts_with("/home/testuser")); assert!(result.to_string_lossy().contains("documents")); assert!(result.to_string_lossy().contains("file.txt")); + + // An absolute path with multiple slashes that lands inside the chroot. + let result = handler + .resolve_path("/home/testuser///documents///file.txt") + .unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); } #[test] fn test_resolve_path_dot_only() { - let handler = test_handler(); + let handler = chroot_handler(); // Single dot let result = handler.resolve_path(".").unwrap(); @@ -1493,7 +1721,7 @@ mod tests { #[test] fn test_resolve_path_alternating_dots() { - let handler = test_handler(); + let handler = chroot_handler(); // Alternating . and .. let result = handler.resolve_path("./a/../b/./c/../d").unwrap(); @@ -1555,29 +1783,74 @@ mod tests { } #[test] - fn test_handler_creation_with_default_root() { + fn no_chroot_handler_treats_relative_under_home() { + // With no chroot, relative paths resolve from the home directory. let user = UserInfo::new("testuser"); - let handler = SftpHandler::new(user, None); + let handler = SftpHandler::new(user, None, PathBuf::from("/home/testuser")); - // Should default to filesystem root let result = handler.resolve_path("etc/passwd").unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/etc/passwd")); + + // Absolute paths are honored verbatim. + let result = handler.resolve_path("/etc/passwd").unwrap(); assert_eq!(result, PathBuf::from("/etc/passwd")); } #[test] - fn test_resolve_path_static() { - // Test the static method directly + fn resolve_path_static_chroot_mode() { let root = PathBuf::from("/chroot/jail"); + let cwd = root.clone(); - let result = SftpHandler::resolve_path_static("test.txt", &root).unwrap(); + // Relative path joined with chroot root. + let result = SftpHandler::resolve_path_static("test.txt", Some(&root), &cwd).unwrap(); assert_eq!(result, PathBuf::from("/chroot/jail/test.txt")); - let result = SftpHandler::resolve_path_static("../escape", &root).unwrap(); - // Escape attempt is clamped to root + // Relative `..` clamped to chroot root. + let result = SftpHandler::resolve_path_static("../escape", Some(&root), &cwd).unwrap(); assert_eq!(result, PathBuf::from("/chroot/jail/escape")); - let result = SftpHandler::resolve_path_static("/absolute/path", &root).unwrap(); + // Absolute inside chroot honored as-is. + let result = + SftpHandler::resolve_path_static("/chroot/jail/absolute/path", Some(&root), &cwd) + .unwrap(); assert_eq!(result, PathBuf::from("/chroot/jail/absolute/path")); + + // Absolute outside chroot rejected. + let err = SftpHandler::resolve_path_static("/etc/passwd", Some(&root), &cwd).unwrap_err(); + assert_eq!(err.code, StatusCode::PermissionDenied); + } + + #[test] + fn resolve_path_static_no_chroot_mode() { + let cwd = PathBuf::from("/home/alice"); + + // Relative path joined with cwd. + let result = SftpHandler::resolve_path_static("test.txt", None, &cwd).unwrap(); + assert_eq!(result, PathBuf::from("/home/alice/test.txt")); + + // Absolute path honored verbatim. + let result = SftpHandler::resolve_path_static("/etc/passwd", None, &cwd).unwrap(); + assert_eq!(result, PathBuf::from("/etc/passwd")); + } + + #[test] + fn ensure_target_in_root_allows_no_chroot() { + // No chroot: every target is allowed; filesystem permissions apply. + SftpHandler::ensure_target_in_root(None, Path::new("/anywhere/at/all")).unwrap(); + } + + #[test] + fn ensure_target_in_root_rejects_chroot_escape() { + let root = PathBuf::from("/chroot/jail"); + let err = + SftpHandler::ensure_target_in_root(Some(&root), Path::new("/etc/passwd")).unwrap_err(); + assert_eq!(err.code, StatusCode::PermissionDenied); + } + + #[test] + fn ensure_target_in_root_allows_inside_chroot() { + let root = PathBuf::from("/chroot/jail"); + SftpHandler::ensure_target_in_root(Some(&root), Path::new("/chroot/jail/file")).unwrap(); } #[test] From 94b464c179f82fd4d4fcff1b78a2f73391a04249 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Apr 2026 02:27:23 +0900 Subject: [PATCH 3/5] docs: cover new chroot semantics and add issue #186 integration tests Document the no-chroot default, the chroot semantics (no path doubling inside, rejection outside, `..` clamped at the boundary), and the `scp.root` field that falls back to `sftp.root`. Update the security guide and the server-configuration architecture doc accordingly, and add the migration note to the changelog. Add `tests/scp_sftp_path_resolution_test.rs` covering the Backend.AI reproduction scenarios (absolute SCP/SFTP paths, no doubling, no chroot honors absolute paths, chroot rejects out-of-root paths, `..` clamped, chroot `/` round-trips through `realpath`) plus symlink-escape blocking under chroot. End-to-end tests against a running `bssh-server` with real `scp`/`bssh upload` clients are out of scope for the Rust harness but the path-resolution layer where the bugs lived is covered here. Refs #186 --- CHANGELOG.md | 7 + docs/architecture/server-configuration.md | 10 +- docs/security.md | 28 +- tests/scp_sftp_path_resolution_test.rs | 298 ++++++++++++++++++++++ 4 files changed, 340 insertions(+), 3 deletions(-) create mode 100644 tests/scp_sftp_path_resolution_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f64d341..82f97e7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Internal fork of `russh-sftp` as `crates/bssh-russh-sftp` with a `serde_bytes` performance fix for `SSH_FXP_WRITE` and `SSH_FXP_DATA` packets. The upstream serde derive routes `Vec` through `deserialize_seq` (byte-by-byte), accounting for ~42% of server CPU during 1 GiB SFTP uploads in `perf` profiling. Annotating the `data` fields with `#[serde(with = "serde_bytes")]` and implementing wire-compatible `serialize_bytes` on the SFTP `Serializer` routes through the existing bulk `deserialize_byte_buf`/`try_get_bytes` path. Measured impact on a CPU-bound host (Xeon Silver 4214): 1 GiB SFTP upload throughput improves from 74.8 MiB/s to 96.4 MiB/s (+29%), closing the gap to OpenSSH `sftp-server` from ~26% to ~5%. +- `scp.root` configuration field. SCP transfers now honor a chroot setting separate from SFTP. When unset, SCP falls back to `sftp.root`, so a single top-level chroot setting governs both subsystems unless an admin explicitly wants them split. ### Changed - Switched the top-level `russh-sftp` dependency from crates.io `russh-sftp = "2.1.1"` to `russh-sftp = { package = "bssh-russh-sftp", version = "2.1.1", path = "crates/bssh-russh-sftp" }`. All existing `use russh_sftp::...` imports continue to work unchanged. +- **Default file-transfer behavior is no longer chrooted to the user's home directory.** With `sftp.root`/`scp.root` unset (the default), absolute client paths are honored verbatim and relative paths resolve from the user's home directory, matching OpenSSH `sftp-server`/`scp` defaults. Deployments that intentionally want chroot-at-home-dir must now set `sftp.root: ` (or equivalent) explicitly. (#186) + +### Fixed +- **bssh-server SCP/SFTP path doubling on absolute client paths** (#186). `ScpHandler::resolve_path` and `SftpHandler::resolve_path_static` previously re-rooted every absolute client path under the user's home directory, so `scp local user@host:/home/work/file.bin` wrote to `/home/work/home/work/file.bin` and `bssh upload local /abs/remote.bin` failed with `No such file`. The resolver now treats absolute client paths verbatim when no chroot is configured and rejects out-of-chroot absolute paths with `permission_denied` when one is. Path-traversal and symlink-escape protections continue to apply. +- **SCP single-file destinations no longer have the source filename appended** (#186). `ScpHandler::receive_file` now consults `target_is_directory` (parsed from `-d`/`-r`) and the filesystem state of the resolved target. `scp local.bin user@host:/tmp/dest.bin` now writes to `/tmp/dest.bin` instead of `/tmp/dest.bin/local.bin`. Directory destinations (`/tmp/dir/`, existing directory, or `-d`/`-r` flag) keep the previous filename-appending behavior. +- **Configured `sftp.root` is no longer dead code** (#186). The handler-construction sites in `SshHandler` previously hard-coded `user_info.home_dir` as the chroot root and ignored `config.sftp.root` entirely. Setting `sftp.root` in the YAML configuration now actually changes the SFTP chroot. The same plumbing now exists for `scp.root`. ## [2.1.2] - 2026-04-27 diff --git a/docs/architecture/server-configuration.md b/docs/architecture/server-configuration.md index cf60beb2..63c93571 100644 --- a/docs/architecture/server-configuration.md +++ b/docs/architecture/server-configuration.md @@ -140,12 +140,20 @@ shell: # SFTP subsystem configuration sftp: enabled: true # Default: true - # Optional chroot directory + # Optional chroot directory. + # When unset (default), no chroot: absolute client paths are honored + # verbatim and relative paths resolve from the user's home directory. + # When set, clients are confined to this directory; absolute paths + # outside it are rejected with permission_denied. root: /data/sftp # SCP protocol configuration scp: enabled: true # Default: true + # Optional chroot directory. Same semantics as sftp.root. When unset, + # falls back to sftp.root, so configure scp.root only when SCP and SFTP + # need separate chroots. + root: /data/scp # File transfer filtering filter: diff --git a/docs/security.md b/docs/security.md index 8292823f..45b1961f 100644 --- a/docs/security.md +++ b/docs/security.md @@ -229,12 +229,36 @@ sftp: scp: enabled: false -# Or enable with restrictions +# Or enable with chroot. SCP falls back to sftp.root when scp.root is unset, +# so a single setting governs both subsystems. sftp: enabled: true - root: /data/sftp # Chroot to this directory + root: /data/sftp # Confine SFTP and SCP to this directory. + +# Use scp.root only when SCP and SFTP need separate chroots: +scp: + enabled: true + root: /data/scp ``` +**Chroot semantics.** When `root` is set: + +- Absolute client paths inside `root` are honored as-is. No path doubling. +- Absolute client paths outside `root` are rejected with `permission_denied`. +- Relative client paths resolve under `root`, with `..` clamped at the + chroot boundary. +- The pseudo-root `/` (returned by `realpath`) maps back to the chroot + directory so interactive SFTP clients (`cd /`, `pwd`) still work. +- Path-traversal and symlink-escape protections continue to apply. + +When `root` is unset (default since v2.1.3, per #186), the handler runs +without chroot. Absolute paths are honored verbatim and relative paths +resolve from the user's home directory, matching OpenSSH `sftp-server`. +This is the recommended default for Backend.AI session containers and any +deployment whose clients submit absolute filesystem paths (such as the +WebUI's "Download SSH key / SCP example" snippet, or `bssh upload +/abs/remote.bin`). + ### File Transfer Filtering Block dangerous file types: diff --git a/tests/scp_sftp_path_resolution_test.rs b/tests/scp_sftp_path_resolution_test.rs new file mode 100644 index 00000000..8476d56e --- /dev/null +++ b/tests/scp_sftp_path_resolution_test.rs @@ -0,0 +1,298 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Integration tests for the SCP/SFTP path-resolution and chroot-mode fixes +//! introduced for issue #186. +//! +//! These tests validate the public API of `ScpHandler` and `SftpHandler` +//! against the acceptance criteria in the issue: +//! +//! - Without chroot, absolute client paths are honored verbatim and relative +//! paths resolve from the user's home directory (OpenSSH-compatible). +//! - With chroot, absolute client paths inside the chroot are honored +//! verbatim (no path doubling); paths outside are rejected. +//! - Path-traversal and symlink-escape protections continue to hold under +//! the new logic. +//! +//! End-to-end tests with a running `bssh-server` and a real `scp` or +//! `bssh upload` client are out of scope for this file (they require host +//! key generation and process spawning), but the path-resolution layer +//! covered here is the one the issue identifies as defective. Bug-fix +//! coverage starts here and is supplemented by the unit tests inside the +//! `scp` and `sftp` modules. + +use std::path::{Path, PathBuf}; + +use bssh::server::scp::{ScpHandler, ScpMode}; +use bssh::server::sftp::SftpHandler; +use bssh::shared::auth_types::UserInfo; +use tempfile::tempdir; + +fn user() -> UserInfo { + UserInfo::new("work") +} + +// --------------------------------------------------------------------------- +// SCP path resolution +// --------------------------------------------------------------------------- + +#[test] +fn scp_no_chroot_accepts_absolute_client_path() { + // Reproduction of the Backend.AI bug: client sends `/home/work/file.bin` + // and the server must write to exactly `/home/work/file.bin`. Previously + // the path got doubled to `/home/work/home/work/file.bin`. + let handler = ScpHandler::new( + ScpMode::Sink, + PathBuf::from("/home/work/file.bin"), + user(), + None, // no chroot — the recommended default after this fix. + PathBuf::from("/home/work"), + ); + let resolved = handler + .resolve_path(Path::new("/home/work/file.bin")) + .expect("absolute path inside home dir should resolve"); + assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); +} + +#[test] +fn scp_no_chroot_accepts_path_outside_home() { + // Without chroot, `/tmp/foo` is just `/tmp/foo`. Filesystem permissions + // are the only access boundary, matching OpenSSH `scp`. + let handler = ScpHandler::new( + ScpMode::Sink, + PathBuf::from("/tmp/foo.bin"), + user(), + None, + PathBuf::from("/home/work"), + ); + let resolved = handler + .resolve_path(Path::new("/tmp/foo.bin")) + .expect("absolute path should resolve"); + assert_eq!(resolved, PathBuf::from("/tmp/foo.bin")); +} + +#[test] +fn scp_no_chroot_relative_path_lands_in_home() { + let handler = ScpHandler::new( + ScpMode::Sink, + PathBuf::from("file.bin"), + user(), + None, + PathBuf::from("/home/work"), + ); + let resolved = handler + .resolve_path(Path::new("file.bin")) + .expect("relative path should resolve"); + assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); +} + +#[test] +fn scp_chroot_inside_root_no_doubling() { + let handler = ScpHandler::new( + ScpMode::Sink, + PathBuf::from("/home/work/file.bin"), + user(), + Some(PathBuf::from("/home/work")), + PathBuf::from("/home/work"), + ); + let resolved = handler + .resolve_path(Path::new("/home/work/file.bin")) + .expect("absolute inside chroot should resolve verbatim"); + assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); +} + +#[test] +fn scp_chroot_rejects_paths_outside_root() { + let handler = ScpHandler::new( + ScpMode::Sink, + PathBuf::from("/etc/passwd"), + user(), + Some(PathBuf::from("/home/work")), + PathBuf::from("/home/work"), + ); + let err = handler + .resolve_path(Path::new("/etc/passwd")) + .expect_err("absolute outside chroot must be rejected"); + assert!( + err.to_string().contains("outside root"), + "expected access-denied error, got: {err}" + ); +} + +#[test] +fn scp_chroot_relative_traversal_clamped() { + let handler = ScpHandler::new( + ScpMode::Sink, + PathBuf::from("../etc/passwd"), + user(), + Some(PathBuf::from("/home/work")), + PathBuf::from("/home/work"), + ); + // The traversal-protection invariant: `..` cannot escape the chroot. + let resolved = handler + .resolve_path(Path::new("../etc/passwd")) + .expect("relative traversal should be clamped, not rejected"); + assert!(resolved.starts_with("/home/work")); + assert_eq!(resolved, PathBuf::from("/home/work/etc/passwd")); +} + +// --------------------------------------------------------------------------- +// SFTP path resolution +// --------------------------------------------------------------------------- + +#[test] +fn sftp_no_chroot_accepts_absolute_client_path() { + // Reproduction of the SFTP variant of the Backend.AI bug: `bssh upload` + // sending an absolute path to `bssh-server` previously failed with + // "No such file" because of path doubling. + let handler = SftpHandler::new(user(), None, PathBuf::from("/home/work")); + let resolved = handler + .resolve_path("/home/work/file.bin") + .expect("absolute path inside home dir should resolve"); + assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); +} + +#[test] +fn sftp_no_chroot_accepts_path_outside_home() { + let handler = SftpHandler::new(user(), None, PathBuf::from("/home/work")); + let resolved = handler + .resolve_path("/tmp/foo.bin") + .expect("absolute path should resolve"); + assert_eq!(resolved, PathBuf::from("/tmp/foo.bin")); +} + +#[test] +fn sftp_no_chroot_relative_path_lands_in_home() { + let handler = SftpHandler::new(user(), None, PathBuf::from("/home/work")); + let resolved = handler.resolve_path("file.bin").unwrap(); + assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); +} + +#[test] +fn sftp_chroot_inside_root_no_doubling() { + let handler = SftpHandler::new( + user(), + Some(PathBuf::from("/home/work")), + PathBuf::from("/home/work"), + ); + let resolved = handler.resolve_path("/home/work/file.bin").unwrap(); + assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); +} + +#[test] +fn sftp_chroot_rejects_paths_outside_root() { + let handler = SftpHandler::new( + user(), + Some(PathBuf::from("/home/work")), + PathBuf::from("/home/work"), + ); + let err = handler + .resolve_path("/etc/passwd") + .expect_err("absolute outside chroot must be rejected"); + // The exact code is PermissionDenied; we verify by checking the message. + assert!( + err.to_string().contains("outside root"), + "expected permission-denied, got: {err}" + ); +} + +#[test] +fn sftp_chroot_relative_traversal_clamped() { + let handler = SftpHandler::new( + user(), + Some(PathBuf::from("/home/work")), + PathBuf::from("/home/work"), + ); + let resolved = handler.resolve_path("../../etc/passwd").unwrap(); + assert!(resolved.starts_with("/home/work")); + assert_eq!(resolved, PathBuf::from("/home/work/etc/passwd")); +} + +#[test] +fn sftp_chroot_root_path_returns_chroot() { + // The realpath roundtrip: `realpath(".")` returns "/" to the client, and + // a subsequent client request for "/" must resolve back to the chroot + // directory, not get rejected as "outside root". + let handler = SftpHandler::new( + user(), + Some(PathBuf::from("/home/work")), + PathBuf::from("/home/work"), + ); + let resolved = handler.resolve_path("/").unwrap(); + assert_eq!(resolved, PathBuf::from("/home/work")); +} + +// --------------------------------------------------------------------------- +// Symlink-escape protection +// --------------------------------------------------------------------------- + +/// Create a symlink pointing outside the chroot and ensure the SCP resolver +/// blocks it via canonicalization. +#[test] +#[cfg(unix)] +fn scp_chroot_blocks_symlink_escape() { + let dir = tempdir().expect("tempdir"); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + + // Create a target file outside the chroot and a symlink inside that + // points at it. Resolving the symlink path must canonicalize and reject. + let outside_target = dir.path().join("outside.txt"); + std::fs::write(&outside_target, b"secret").unwrap(); + let escape_link = chroot.join("escape"); + std::os::unix::fs::symlink(&outside_target, &escape_link).unwrap(); + + let handler = ScpHandler::new( + ScpMode::Source, + escape_link.clone(), + user(), + Some(chroot.clone()), + chroot.clone(), + ); + let err = handler + .resolve_path(&escape_link) + .expect_err("symlink escape must be blocked"); + assert!( + err.to_string().contains("symlink target outside root"), + "expected symlink-escape error, got: {err}" + ); +} + +/// Verify that the SCP resolver still rejects a symlink-escape attempt when +/// the user supplies a path *inside* the chroot but the resolved canonical +/// path lands outside, even if the symlink is reached through a relative +/// client path. +#[test] +#[cfg(unix)] +fn scp_chroot_blocks_relative_symlink_escape() { + let dir = tempdir().unwrap(); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + + let outside_target = dir.path().join("outside.txt"); + std::fs::write(&outside_target, b"secret").unwrap(); + std::os::unix::fs::symlink(&outside_target, chroot.join("link")).unwrap(); + + let handler = ScpHandler::new( + ScpMode::Source, + PathBuf::from("link"), + user(), + Some(chroot.clone()), + chroot.clone(), + ); + let err = handler + .resolve_path(Path::new("link")) + .expect_err("relative symlink escape must be blocked"); + assert!(err.to_string().contains("symlink target outside root")); +} From 9903dd81c6fd05966ac9801012df99975974aea0 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Apr 2026 02:48:35 +0900 Subject: [PATCH 4/5] fix(security): block chroot bypass via parent-directory symlinks The chroot resolver only verified lexical containment for paths whose final component did not exist (the typical create/mkdir flow). A symlink inside the chroot pointing to a directory outside the chroot let a client target chroot/escape/newfile and have OpenOptions::open() or create_dir() follow the symlink to operate outside the chroot. ScpHandler::resolve_path and SftpHandler::resolve_path_static now walk up to the closest existing ancestor of the target path, canonicalize both that ancestor and the chroot root, and verify the canonical ancestor stays inside the canonical root. Operator-misconfigured chroots that don't exist on disk fall back to the lexical check. Found during PR #194 review. Adds 6 regression tests covering both SCP and SFTP, absolute and relative client paths, file create and mkdir, plus two sanity checks for legitimate nested operations. --- CHANGELOG.md | 1 + docs/security.md | 5 +- src/server/scp.rs | 59 ++++++++++ src/server/sftp.rs | 66 ++++++++++- tests/scp_sftp_path_resolution_test.rs | 146 +++++++++++++++++++++++++ 5 files changed, 273 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82f97e7d..d51c9d41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **bssh-server SCP/SFTP path doubling on absolute client paths** (#186). `ScpHandler::resolve_path` and `SftpHandler::resolve_path_static` previously re-rooted every absolute client path under the user's home directory, so `scp local user@host:/home/work/file.bin` wrote to `/home/work/home/work/file.bin` and `bssh upload local /abs/remote.bin` failed with `No such file`. The resolver now treats absolute client paths verbatim when no chroot is configured and rejects out-of-chroot absolute paths with `permission_denied` when one is. Path-traversal and symlink-escape protections continue to apply. - **SCP single-file destinations no longer have the source filename appended** (#186). `ScpHandler::receive_file` now consults `target_is_directory` (parsed from `-d`/`-r`) and the filesystem state of the resolved target. `scp local.bin user@host:/tmp/dest.bin` now writes to `/tmp/dest.bin` instead of `/tmp/dest.bin/local.bin`. Directory destinations (`/tmp/dir/`, existing directory, or `-d`/`-r` flag) keep the previous filename-appending behavior. - **Configured `sftp.root` is no longer dead code** (#186). The handler-construction sites in `SshHandler` previously hard-coded `user_info.home_dir` as the chroot root and ignored `config.sftp.root` entirely. Setting `sftp.root` in the YAML configuration now actually changes the SFTP chroot. The same plumbing now exists for `scp.root`. +- **Chroot bypass via intermediate-directory symlink**. The chroot resolver previously checked only lexical containment for paths whose final component did not exist (typical for new-file creates and `mkdir`). A symlink inside the chroot pointing to a directory outside the chroot would let a client target `chroot/escape/newfile` and have `open(...)`/`create_dir(...)` follow the symlink, writing outside the chroot. Both `ScpHandler::resolve_path` and `SftpHandler::resolve_path_static` now canonicalize the closest existing ancestor of the target path and verify it stays inside the canonicalized chroot, blocking the parent-symlink escape. Found during PR #194 review. ## [2.1.2] - 2026-04-27 diff --git a/docs/security.md b/docs/security.md index 45b1961f..dc63e143 100644 --- a/docs/security.md +++ b/docs/security.md @@ -249,7 +249,10 @@ scp: chroot boundary. - The pseudo-root `/` (returned by `realpath`) maps back to the chroot directory so interactive SFTP clients (`cd /`, `pwd`) still work. -- Path-traversal and symlink-escape protections continue to apply. +- Path-traversal and symlink-escape protections continue to apply, + including for paths whose final component does not exist yet: the closest + existing ancestor is canonicalized and verified to stay inside `root`. + This blocks intermediate-directory symlinks pointing outside the chroot. When `root` is unset (default since v2.1.3, per #186), the handler runs without chroot. Absolute paths are honored verbatim and relative paths diff --git a/src/server/scp.rs b/src/server/scp.rs index 5d7b4bb4..4f9a7d60 100644 --- a/src/server/scp.rs +++ b/src/server/scp.rs @@ -289,6 +289,33 @@ fn resolve_chroot_scp(requested: &Path, root: &Path, user: &str) -> Result Option<(PathBuf, PathBuf)> { + let mut cur = path.to_path_buf(); + loop { + if cur.exists() { + if let Ok(canonical) = std::fs::canonicalize(&cur) { + return Some((cur, canonical)); + } + return None; + } + if !cur.pop() { + return None; + } + } +} + /// Resolve a client-supplied SCP path without a chroot. /// /// Absolute paths are used verbatim. Relative paths join with `cwd` @@ -403,6 +430,10 @@ impl ScpHandler { /// - Absolute client paths outside `root` are rejected. /// - Relative paths are joined with `root`; `..` traversal is clamped. /// - Existing paths are canonicalized to catch symlink-escape attempts. + /// - For non-existent paths (typical for new-file creates), the closest + /// existing ancestor is canonicalized and verified to stay inside + /// `root`. This blocks intermediate-directory symlinks pointing outside + /// the chroot. /// /// ## Without chroot (`root_dir = None`): /// - Absolute paths are used verbatim. @@ -445,6 +476,34 @@ impl ScpHandler { ); } } + } else if let Some(root) = self.root_dir.as_deref() { + // Path does not exist (typical for new-file create/upload). Walk + // up to the closest existing ancestor and verify its canonical + // form stays inside the chroot. This catches the case where an + // intermediate parent is a symlink pointing outside the chroot; + // without this check, `OpenOptions::open(...)` would follow the + // symlink and create the file outside the chroot. + // + // Compare canonical-vs-canonical: an unresolved root might itself + // contain symlinks, so we canonicalize both sides. If the chroot + // root does not exist on disk, the operator config is bad and we + // can only fall back to the lexical check above (skip enforcement). + if let Some(canonical_root) = std::fs::canonicalize(root).ok() + && let Some((ancestor, canonical_ancestor)) = closest_existing_canonical(&resolved) + && !canonical_ancestor.starts_with(&canonical_root) + { + tracing::warn!( + event = "symlink_escape_attempt", + user = %self.user_info.username, + requested = %path_str, + resolved = %resolved.display(), + ancestor = %ancestor.display(), + canonical_ancestor = %canonical_ancestor.display(), + canonical_root = %canonical_root.display(), + "Security: parent-directory symlink escape blocked" + ); + anyhow::bail!("Access denied: symlink target outside root"); + } } tracing::trace!( diff --git a/src/server/sftp.rs b/src/server/sftp.rs index 401c54be..433165df 100644 --- a/src/server/sftp.rs +++ b/src/server/sftp.rs @@ -282,6 +282,33 @@ fn resolve_chroot(requested: &Path, root: &Path) -> Result { Ok(resolved) } +/// Find the closest existing ancestor of `path` and return both the ancestor +/// and its canonicalized form. +/// +/// Walks up `path` (popping one component at a time) until a path that exists +/// on the filesystem is found, then canonicalizes it. Used by chroot +/// resolution to detect intermediate-directory symlinks pointing outside the +/// chroot — without this check, `open(...)` / `create_dir(...)` etc. on a +/// non-existent final path would happily follow a parent-symlink and operate +/// outside the chroot. +/// +/// Returns `None` when no ancestor exists or canonicalization fails for every +/// candidate. +fn closest_existing_canonical(path: &Path) -> Option<(PathBuf, PathBuf)> { + let mut cur = path.to_path_buf(); + loop { + if cur.exists() { + if let Ok(canonical) = std::fs::canonicalize(&cur) { + return Some((cur, canonical)); + } + return None; + } + if !cur.pop() { + return None; + } + } +} + /// Resolve a client-supplied path without a chroot. /// /// - Absolute paths are used verbatim, after normalizing `.` and `..`. @@ -398,10 +425,43 @@ impl SftpHandler { ) -> Result { let requested = Path::new(path); - match root_dir { - Some(root) => resolve_chroot(requested, root), - None => Ok(resolve_no_chroot(requested, cwd)), + let resolved = match root_dir { + Some(root) => resolve_chroot(requested, root)?, + None => return Ok(resolve_no_chroot(requested, cwd)), + }; + + // Chroot mode: also verify the closest existing ancestor canonicalizes + // inside the chroot. This catches intermediate-directory symlinks + // pointing outside the chroot. Without this, a chroot-internal symlink + // such as `chroot/escape -> /etc` would let a client target + // `chroot/escape/passwd` and have `open(...)` follow the symlink to + // write `/etc/passwd`. Lexical `starts_with(root)` alone cannot + // detect this; we need filesystem-level canonicalization. + // + // Compare canonical-vs-canonical: an unresolved root might itself + // contain symlinks, so we canonicalize both sides. If the chroot + // root does not exist on disk, the operator config is bad and we + // can only fall back to the lexical check (skip enforcement here). + let root = root_dir.expect("chroot branch implies Some(root)"); + if let Some(canonical_root) = std::fs::canonicalize(root).ok() + && let Some((ancestor, canonical_ancestor)) = closest_existing_canonical(&resolved) + && !canonical_ancestor.starts_with(&canonical_root) + { + tracing::warn!( + event = "symlink_escape_attempt", + requested = %path, + resolved = %resolved.display(), + ancestor = %ancestor.display(), + canonical_ancestor = %canonical_ancestor.display(), + canonical_root = %canonical_root.display(), + "Security: parent-directory symlink escape blocked" + ); + return Err(SftpError::permission_denied( + "Access denied: path outside root", + )); } + + Ok(resolved) } /// Resolve a client path to an absolute filesystem path. diff --git a/tests/scp_sftp_path_resolution_test.rs b/tests/scp_sftp_path_resolution_test.rs index 8476d56e..fe013c77 100644 --- a/tests/scp_sftp_path_resolution_test.rs +++ b/tests/scp_sftp_path_resolution_test.rs @@ -296,3 +296,149 @@ fn scp_chroot_blocks_relative_symlink_escape() { .expect_err("relative symlink escape must be blocked"); assert!(err.to_string().contains("symlink target outside root")); } + +// --------------------------------------------------------------------------- +// Parent-directory symlink escape (issue #186 review-time finding) +// --------------------------------------------------------------------------- +// +// An attacker who can place a symlink inside the chroot pointing to a +// directory outside the chroot must not be able to create files outside the +// chroot by writing through the symlink. Lexical `starts_with(root)` alone +// cannot detect this — the chroot resolver also has to canonicalize the +// closest existing ancestor and compare it against the canonicalized chroot. + +#[test] +#[cfg(unix)] +fn scp_chroot_blocks_parent_symlink_create() { + let dir = tempdir().unwrap(); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir(&outside).unwrap(); + std::os::unix::fs::symlink(&outside, chroot.join("escape")).unwrap(); + + let target = chroot.join("escape").join("newfile.txt"); + let handler = ScpHandler::new( + ScpMode::Sink, + target.clone(), + user(), + Some(chroot.clone()), + chroot.clone(), + ); + + let err = handler + .resolve_path(&target) + .expect_err("parent-symlink escape must be blocked"); + assert!( + err.to_string().contains("outside root"), + "expected access-denied error, got: {err}" + ); +} + +#[test] +#[cfg(unix)] +fn sftp_chroot_blocks_parent_symlink_create() { + let dir = tempdir().unwrap(); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir(&outside).unwrap(); + std::os::unix::fs::symlink(&outside, chroot.join("escape")).unwrap(); + + let handler = SftpHandler::new(user(), Some(chroot.clone()), chroot.clone()); + + let target_str = format!("{}/escape/newfile.txt", chroot.display()); + let err = handler + .resolve_path(&target_str) + .expect_err("parent-symlink escape must be blocked"); + assert!( + err.to_string().contains("outside root"), + "expected permission-denied, got: {err}" + ); +} + +#[test] +#[cfg(unix)] +fn sftp_chroot_blocks_parent_symlink_mkdir() { + let dir = tempdir().unwrap(); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir(&outside).unwrap(); + std::os::unix::fs::symlink(&outside, chroot.join("escape")).unwrap(); + + let handler = SftpHandler::new(user(), Some(chroot.clone()), chroot.clone()); + + let target_str = format!("{}/escape/newdir", chroot.display()); + let err = handler + .resolve_path(&target_str) + .expect_err("parent-symlink mkdir-target must be blocked"); + assert!(err.to_string().contains("outside root")); +} + +#[test] +#[cfg(unix)] +fn scp_chroot_blocks_relative_through_parent_symlink() { + let dir = tempdir().unwrap(); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir(&outside).unwrap(); + std::os::unix::fs::symlink(&outside, chroot.join("escape")).unwrap(); + + let handler = ScpHandler::new( + ScpMode::Sink, + PathBuf::from("escape/newfile.txt"), + user(), + Some(chroot.clone()), + chroot.clone(), + ); + + let err = handler + .resolve_path(Path::new("escape/newfile.txt")) + .expect_err("relative parent-symlink escape must be blocked"); + assert!(err.to_string().contains("outside root")); +} + +#[test] +#[cfg(unix)] +fn scp_chroot_allows_legitimate_nested_create() { + // Sanity: ensure the new check does not over-reject normal nested writes + // through legitimate (in-chroot) directories. + let dir = tempdir().unwrap(); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + std::fs::create_dir(chroot.join("subdir")).unwrap(); + + let target = chroot.join("subdir").join("legit.txt"); + let handler = ScpHandler::new( + ScpMode::Sink, + target.clone(), + user(), + Some(chroot.clone()), + chroot.clone(), + ); + + let resolved = handler + .resolve_path(&target) + .expect("legitimate nested create should resolve"); + assert!(resolved.starts_with(&chroot)); +} + +#[test] +#[cfg(unix)] +fn sftp_chroot_allows_create_in_nonexistent_subdir() { + // The intermediate-symlink check must NOT reject paths whose parents + // simply don't exist (legitimate mkdir-then-create flow). + let dir = tempdir().unwrap(); + let chroot = dir.path().join("chroot"); + std::fs::create_dir(&chroot).unwrap(); + + let handler = SftpHandler::new(user(), Some(chroot.clone()), chroot.clone()); + + let target_str = format!("{}/will-be-created/newfile.txt", chroot.display()); + let resolved = handler + .resolve_path(&target_str) + .expect("nested non-existent subdir should resolve"); + assert!(resolved.starts_with(&chroot)); +} From 3eade1b10831781391f87696b475bf2e333ea970 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 30 Apr 2026 02:54:51 +0900 Subject: [PATCH 5/5] fix: remove dead starts_with check after ParentDir clamping in chroot resolvers In both ScpHandler and SftpHandler, the relative-path chroot resolver initializes `resolved` to `root` and only extends it via Normal pushes. The `ParentDir` arm already guards against popping past `root` with `if resolved != root`, making the subsequent `if !resolved.starts_with(root)` check unreachable dead code. Remove it and document why the single guard is sufficient. Also update docs/man/bssh-server.8: document sftp.root and scp.root in the configuration sections, add the intermediate-directory-symlink chroot protection to SECURITY CONSIDERATIONS, and bump the version to v2.1.3 to reflect the PR's unreleased changes. --- docs/man/bssh-server.8 | 16 +++++++++++++--- src/server/scp.rs | 7 ++++--- src/server/sftp.rs | 7 ++++--- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/docs/man/bssh-server.8 b/docs/man/bssh-server.8 index f6b1fc6e..3946492f 100644 --- a/docs/man/bssh-server.8 +++ b/docs/man/bssh-server.8 @@ -1,6 +1,6 @@ .\" Manpage for bssh-server .\" Contact the maintainers to correct errors or typos. -.TH BSSH-SERVER 8 "April 2026" "v2.1.2" "System Administration Commands" +.TH BSSH-SERVER 8 "April 2026" "v2.1.3" "System Administration Commands" .SH NAME bssh-server \- Backend.AI SSH Server for container environments @@ -173,10 +173,15 @@ Authentication methods and settings (methods, publickey, password) Shell execution settings (default, command_timeout, env) .TP .B sftp -SFTP subsystem settings (enabled, root) +SFTP subsystem settings (\fBenabled\fR, \fBroot\fR). \fBroot\fR sets a chroot +directory that confines SFTP transfers. When unset (default), absolute client +paths are honored verbatim and relative paths resolve from the user's home +directory, matching OpenSSH \fBsftp-server\fR behavior. .TP .B scp -SCP protocol settings (enabled) +SCP protocol settings (\fBenabled\fR, \fBroot\fR). \fBroot\fR has the same +semantics as \fBsftp.root\fR. When \fBscp.root\fR is unset, it falls back to +\fBsftp.root\fR so a single setting governs both subsystems. .TP .B filter File transfer filtering (enabled, rules) @@ -271,6 +276,11 @@ Enable IP allowlists in production to restrict access to trusted networks Configure rate limiting to prevent brute-force attacks .IP \(bu 2 Enable audit logging for security monitoring and compliance +.IP \(bu 2 +When \fBsftp.root\fR or \fBscp.root\fR is set, the chroot resolver +canonicalizes the closest existing ancestor of the requested path and verifies +it stays inside the configured root. This blocks attacks that route writes +outside the chroot through intermediate-directory symlinks inside it. .SH EXIT STATUS .TP diff --git a/src/server/scp.rs b/src/server/scp.rs index 4f9a7d60..30bf6878 100644 --- a/src/server/scp.rs +++ b/src/server/scp.rs @@ -263,12 +263,13 @@ fn resolve_chroot_scp(requested: &Path, root: &Path, user: &str) -> Result resolved.push(c), Component::CurDir => {} Component::ParentDir => { + // Refuse to pop past the chroot root so traversal cannot + // escape. `resolved` is always rooted at `root` (initialized + // to `root`, only extended via `Normal` pushes), so popping + // when `resolved != root` always stays inside or at `root`. if resolved != root { resolved.pop(); } - if !resolved.starts_with(root) { - resolved = root.to_path_buf(); - } } Component::RootDir | Component::Prefix(_) => {} } diff --git a/src/server/sftp.rs b/src/server/sftp.rs index 433165df..b4119b53 100644 --- a/src/server/sftp.rs +++ b/src/server/sftp.rs @@ -249,12 +249,13 @@ fn resolve_chroot(requested: &Path, root: &Path) -> Result { Component::Normal(c) => resolved.push(c), Component::CurDir => {} Component::ParentDir => { + // Refuse to pop past the chroot root so traversal cannot + // escape. `resolved` is always rooted at `root` (initialized + // to `root`, only extended via `Normal` pushes), so popping + // when `resolved != root` always stays inside or at `root`. if resolved != root { resolved.pop(); } - if !resolved.starts_with(root) { - resolved = root.to_path_buf(); - } } // Relative paths shouldn't carry these, but ignore safely. Component::RootDir | Component::Prefix(_) => {}