diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f64d341..d51c9d41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,17 @@ 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`. +- **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/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/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/docs/security.md b/docs/security.md index 8292823f..dc63e143 100644 --- a/docs/security.md +++ b/docs/security.md @@ -229,12 +229,39 @@ 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, + 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 +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/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, } } } 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..30bf6878 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,127 @@ 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 => { + // 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(); + } + } + 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) +} + +/// 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, `OpenOptions::open(...)` on a non-existent +/// final path would happily follow a parent-symlink and write 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 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 +342,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 +374,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 +400,76 @@ 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. + /// - 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. + /// - 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, @@ -347,6 +477,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!( @@ -458,10 +616,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 +692,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 +747,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 +894,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 +1319,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 +1353,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 +1440,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..b4119b53 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,178 @@ 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 => { + // 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(); + } + } + // 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) +} + +/// 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 `..`. +/// - 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 +347,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 +375,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 +403,94 @@ 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) + /// 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); + + let resolved = match root_dir { + Some(root) => resolve_chroot(requested, root)?, + None => return Ok(resolve_no_chroot(requested, cwd)), }; - // 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) { + // 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!( - requested = %path.display(), + event = "symlink_escape_attempt", + requested = %path, resolved = %resolved.display(), - root = %root_dir.display(), - "Path traversal attempt detected" + 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", )); } - 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 +590,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 +616,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 +850,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 +971,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 +981,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 +1067,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 +1311,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 +1375,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 absolute target outside chroot" + ); + })?; + if !resolved_target.starts_with(root) { tracing::warn!( user = %user, link = %link_path.display(), target = %targetpath, - "Rejected symlink with target outside root" + resolved = %resolved_target.display(), + "Symlink target resolves outside chroot" ); - return Err(e); + 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 +1478,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); + + let err = handler.resolve_path("/tmp/file.bin").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 + #[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 +1624,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 +1712,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 +1721,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 +1740,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 +1752,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 +1782,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 +1844,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] diff --git a/tests/scp_sftp_path_resolution_test.rs b/tests/scp_sftp_path_resolution_test.rs new file mode 100644 index 00000000..fe013c77 --- /dev/null +++ b/tests/scp_sftp_path_resolution_test.rs @@ -0,0 +1,444 @@ +// 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")); +} + +// --------------------------------------------------------------------------- +// 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)); +}