fix: SCP/SFTP path resolution and dead chroot config (#186) (#194)

* 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<PathBuf>`, 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

* 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<PathBuf>` 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

* 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

* 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.

* 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.

Author: inureyes

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<u8>` 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: <home dir>` (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
 

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:

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

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:

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<PathBuf>,
+
+    /// 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<PathBuf>,
+
     /// 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<PathBuf>) -> 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<PathBuf>) -> 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();

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<PathBuf>,
 }
 
@@ -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<PathBuf>,
 }
 
 /// File transfer filtering configuration.
@@ -650,6 +666,7 @@ impl Default for ScpConfig {
     fn default() -> Self {
         Self {
             enabled: default_true(),
+            root: None,
         }
     }
 }

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;