From f3babdda6af5bc43b034f2c9e526924df307f9a4 Mon Sep 17 00:00:00 2001 From: Sion Kang Date: Mon, 6 Jul 2026 21:09:03 +0900 Subject: [PATCH 1/4] fix(sftp): resolve chroot client absolute paths relative to the root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With `sftp.root` set, listing worked but every path resolution failed: `cd subdir`, `get file`, and even `get` of a file at the chroot root were rejected as `path outside root` / `not found` — only bare `/` and readdir worked, making the chroot feature unusable. Under chroot the client's coordinate space is rooted at `/` (that's what `realpath` reports and what the client sends back), but `resolve_chroot` treated an absolute client path like `/subdir` as a *host* path and rejected anything not starting with the host root, so `/subdir` (= `/subdir`) never matched. A prior "avoid path doubling" change caused this, but real clients never send the host path — they send chroot-relative absolute paths. Resolve both absolute and relative client paths relative to `root`: ignore a leading `/`, drop `.`, and clamp `..` so traversal cannot escape. Containment is unchanged — `..` stays pinned at the root and a client `/etc/passwd` maps to `/etc/passwd`, never the host's. Verified with the OpenSSH sftp client (cd/get/root files/Unicode work; escape attempts confined) and updated unit tests. Fixes #214. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 3 ++ src/server/sftp.rs | 132 +++++++++++++++++++++++++-------------------- 2 files changed, 76 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cf8e7e7..e04f150c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **Make `sftp.root` chroot actually usable — resolve client absolute paths relative to the chroot root** (#214). With `sftp.root` configured, directory listing worked but every path resolution failed: `cd subdir`, `get file`, and even `get` of a file sitting directly at the chroot root were rejected as `path outside root` or `not found`, so only bare `/` and `readdir` functioned. Root cause: under chroot the client's coordinate space is rooted at `/` (this is what `realpath` reports and what the client sends back), but `resolve_chroot` treated a client absolute path such as `/subdir` as a *host* path and rejected anything not starting with the host root — so `/subdir` (meaning `/subdir`) never matched. A prior change had introduced this to avoid "path doubling", but real SFTP clients never send the host path; they send chroot-relative absolute paths, so the guard broke the normal case. `resolve_chroot` now interprets both absolute and relative client paths relative to `root`, ignoring a leading `/`, dropping `.`, and clamping `..` so traversal still cannot escape. Containment is unchanged and verified: `..` stays pinned at the root and a client `/etc/passwd` maps to `/etc/passwd` (a likely-nonexistent path inside the jail), never the host's `/etc/passwd`. Verified end-to-end with the OpenSSH `sftp` client (`cd`/`get`/root-level files/Unicode names all work; escape attempts stay confined) and covered by updated unit tests. + ## [2.2.3] - 2026-05-25 ### Security diff --git a/src/server/sftp.rs b/src/server/sftp.rs index afea5112..342efca0 100644 --- a/src/server/sftp.rs +++ b/src/server/sftp.rs @@ -225,40 +225,17 @@ fn resolve_chroot(requested: &Path, root: &Path) -> Result { 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. + // Under chroot the client's coordinate space is rooted at "/" == root: + // `realpath` reports paths relative to the chroot (e.g. "/subdir"), and the + // client sends them straight back that way. So BOTH absolute ("/subdir", + // "/hello.txt") and relative ("subdir") client paths are interpreted + // relative to `root` — a leading "/" denotes the chroot root, NOT a + // host-absolute path. (The previous code treated an absolute client path as + // a host path and rejected anything not starting with `root`, so every + // `cd`/`get`/`open` failed while only bare `/` and `readdir` worked; see + // issue #214.) Walk the components starting from `root`, ignoring any + // RootDir/Prefix, dropping `.`, and clamping `..` so traversal can never + // escape the chroot. let mut resolved = root.to_path_buf(); for component in requested.components() { match component { @@ -273,7 +250,8 @@ fn resolve_chroot(requested: &Path, root: &Path) -> Result { resolved.pop(); } } - // Relative paths shouldn't carry these, but ignore safely. + // A leading "/" is the client's view of the chroot root; relative + // paths shouldn't carry a prefix. Ignore both safely. Component::RootDir | Component::Prefix(_) => {} } } @@ -1520,30 +1498,52 @@ mod tests { } #[test] - 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. + fn chroot_absolute_path_is_reanchored_under_root() { + // Under chroot the client's "/" IS the chroot root, so an absolute + // client path like "/file.bin" means "/file.bin" and must be + // re-anchored under the root — not treated as a host path. Treating it + // as a host path (and rejecting anything not under root) is exactly what + // broke `cd`/`get` for real clients; see issue #214. let handler = chroot_handler(); - let result = handler.resolve_path("/home/testuser/file.bin").unwrap(); + let result = handler.resolve_path("/file.bin").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser/file.bin")); + + 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() { + // The client's view of the chroot root is bare "/". let handler = chroot_handler(); - let result = handler.resolve_path("/home/testuser").unwrap(); + let result = handler.resolve_path("/").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser")); } #[test] - fn chroot_absolute_outside_root_is_rejected() { + fn chroot_absolute_host_path_is_confined_not_escaped() { + // A client cannot reach the host filesystem: an absolute path is + // confined under the chroot, so "/etc/passwd" maps to + // "/etc/passwd" (which likely does not exist) rather than the + // host's /etc/passwd. This is the containment guarantee — the previous + // "reject absolute outside root" behavior was both wrong (broke #214) + // and unnecessary for confinement. 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); + assert_eq!( + handler.resolve_path("/etc/passwd").unwrap(), + PathBuf::from("/home/testuser/etc/passwd") + ); + assert_eq!( + handler.resolve_path("/tmp/file.bin").unwrap(), + PathBuf::from("/home/testuser/tmp/file.bin") + ); + // `..` cannot climb above the root even when prefixed with the host path. + assert_eq!( + handler + .resolve_path("/home/testuser/../../etc/passwd") + .unwrap(), + PathBuf::from("/home/testuser/etc/passwd") + ); } #[test] @@ -1776,10 +1776,9 @@ mod tests { 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(); + // An absolute (chroot-relative) path with multiple slashes collapses + // them and re-anchors under the chroot root. + let result = handler.resolve_path("/documents///file.txt").unwrap(); assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); } @@ -1886,15 +1885,30 @@ mod tests { let result = SftpHandler::resolve_path_static("../escape", Some(&root), &cwd).unwrap(); assert_eq!(result, PathBuf::from("/chroot/jail/escape")); - // 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")); + // Bare "/" is the client's view of the chroot root. + let result = SftpHandler::resolve_path_static("/", Some(&root), &cwd).unwrap(); + assert_eq!(result, PathBuf::from("/chroot/jail")); - // Absolute outside chroot rejected. - let err = SftpHandler::resolve_path_static("/etc/passwd", Some(&root), &cwd).unwrap_err(); - assert_eq!(err.code, StatusCode::PermissionDenied); + // Absolute client paths are chroot-relative ("/" == root), so they are + // re-anchored under the chroot rather than treated as host paths. + // Regression test for #214: `cd /subdir`, `get /hello.txt` must resolve + // to `/subdir` and `/hello.txt`, not be rejected. + let result = SftpHandler::resolve_path_static("/subdir/file", Some(&root), &cwd).unwrap(); + assert_eq!(result, PathBuf::from("/chroot/jail/subdir/file")); + + let result = SftpHandler::resolve_path_static("/hello.txt", Some(&root), &cwd).unwrap(); + assert_eq!(result, PathBuf::from("/chroot/jail/hello.txt")); + + // A host-looking absolute path cannot escape: it is confined under the + // chroot (the client's `/etc/passwd` maps to `/etc/passwd`, never + // the host's /etc/passwd). + let result = SftpHandler::resolve_path_static("/etc/passwd", Some(&root), &cwd).unwrap(); + assert_eq!(result, PathBuf::from("/chroot/jail/etc/passwd")); + + // `..` escape attempts stay clamped to the root, absolute or not. + let result = + SftpHandler::resolve_path_static("/../../etc/passwd", Some(&root), &cwd).unwrap(); + assert_eq!(result, PathBuf::from("/chroot/jail/etc/passwd")); } #[test] From a2203bd7dfee259080e6a30f2a315a384bafa0fd Mon Sep 17 00:00:00 2001 From: Sion Kang Date: Tue, 7 Jul 2026 16:40:26 +0900 Subject: [PATCH 2/4] test(sftp): update chroot integration tests for re-anchoring semantics The `tests/scp_sftp_path_resolution_test.rs` SFTP chroot tests still encoded the old behavior (absolute client path honored verbatim / rejected outside root), so `cargo test --tests` failed in CI after the resolver change. - `sftp_chroot_inside_root_no_doubling` -> `..._absolute_path_reanchored_under_root`: a client "/file.bin" now resolves to "/file.bin". - `sftp_chroot_rejects_paths_outside_root` -> `..._absolute_host_path_confined_under_root`: "/etc/passwd" maps to "/etc/passwd" (confined) rather than being rejected. - `sftp_chroot_blocks_parent_symlink_{create,mkdir}`: send the chroot-relative path ("/escape/...") the real client would send instead of the host-absolute path; the canonicalized-ancestor check still blocks the symlink escape. Only the SFTP resolver changed; the SCP tests are untouched. Co-Authored-By: Claude Opus 4.8 --- tests/scp_sftp_path_resolution_test.rs | 32 ++++++++++++++------------ 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/tests/scp_sftp_path_resolution_test.rs b/tests/scp_sftp_path_resolution_test.rs index fe013c77..b2f29a38 100644 --- a/tests/scp_sftp_path_resolution_test.rs +++ b/tests/scp_sftp_path_resolution_test.rs @@ -180,31 +180,31 @@ fn sftp_no_chroot_relative_path_lands_in_home() { } #[test] -fn sftp_chroot_inside_root_no_doubling() { +fn sftp_chroot_absolute_path_reanchored_under_root() { + // Under chroot the client's "/" IS the chroot root, so an absolute client + // path is re-anchored under the root, not treated as a host path. The + // client sends "/file.bin" meaning "/file.bin". (#214) let handler = SftpHandler::new( user(), Some(PathBuf::from("/home/work")), PathBuf::from("/home/work"), ); - let resolved = handler.resolve_path("/home/work/file.bin").unwrap(); + let resolved = handler.resolve_path("/file.bin").unwrap(); assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); } #[test] -fn sftp_chroot_rejects_paths_outside_root() { +fn sftp_chroot_absolute_host_path_confined_under_root() { + // A client "/etc/passwd" is confined under the chroot, mapping to + // /etc/passwd — never the host's /etc/passwd. (Rejecting absolute + // paths outright, as before, also broke every legitimate path; #214.) 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}" - ); + let resolved = handler.resolve_path("/etc/passwd").unwrap(); + assert_eq!(resolved, PathBuf::from("/home/work/etc/passwd")); } #[test] @@ -347,9 +347,11 @@ fn sftp_chroot_blocks_parent_symlink_create() { let handler = SftpHandler::new(user(), Some(chroot.clone()), chroot.clone()); - let target_str = format!("{}/escape/newfile.txt", chroot.display()); + // The client sends a chroot-relative path that traverses the `escape` + // parent symlink; canonicalization of the closest existing ancestor must + // still detect that it lands outside the chroot. let err = handler - .resolve_path(&target_str) + .resolve_path("/escape/newfile.txt") .expect_err("parent-symlink escape must be blocked"); assert!( err.to_string().contains("outside root"), @@ -369,9 +371,9 @@ fn sftp_chroot_blocks_parent_symlink_mkdir() { let handler = SftpHandler::new(user(), Some(chroot.clone()), chroot.clone()); - let target_str = format!("{}/escape/newdir", chroot.display()); + // Chroot-relative mkdir target traversing the `escape` parent symlink. let err = handler - .resolve_path(&target_str) + .resolve_path("/escape/newdir") .expect_err("parent-symlink mkdir-target must be blocked"); assert!(err.to_string().contains("outside root")); } From e5dfc04218535e7589cd3c5f242fa6fe8328fb0e Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 16 Jul 2026 18:24:55 +0900 Subject: [PATCH 3/4] fix(server): confine absolute SFTP symlink targets and unify SCP chroot Follow-up hardening and consistency for the chroot re-rooting fix (#214). Security: once resolve_chroot stopped rejecting out-of-root absolute paths, the SFTP symlink handler's containment guard became a no-op, so a chrooted client could create a link whose on-disk target was an absolute host path (e.g. `symlink /link /etc/passwd`). With a virtual chroot and no chroot(2) that link resolved to the real host filesystem. Absolute symlink targets are now re-anchored under the chroot before the link is written; relative targets keep OpenSSH-compatible verbatim storage. SCP: resolve_chroot_scp still rejected out-of-root absolute client paths, diverging from the new sftp.root behavior. It now re-anchors absolute and relative client paths under the chroot root the same way SFTP does, so the documented "same semantics as sftp.root" holds. The existing per-request canonicalization keeps symlink-escape containment intact. Docs: update the chroot description everywhere it was still stated as "absolute outside root is rejected" to the re-rooting model, and note the symlink confinement: resolve_chroot / resolve_path_static / SftpConfig.root / ScpConfig doc comments, docs/security.md, bssh-server.yaml, docs/architecture/server-configuration.md, and the bssh-server.8 man page. Tests: add an async unit test asserting an absolute SFTP symlink target is stored re-anchored under the chroot (not the host path); rewrite the SCP chroot unit and integration tests for re-anchoring and to drive symlink escape checks through chroot-relative client paths. --- CHANGELOG.md | 5 +- docs/architecture/server-configuration.md | 6 +- docs/man/bssh-server.8 | 10 +- docs/security.md | 28 +++--- src/server/config/types.rs | 14 ++- src/server/scp.rs | 95 +++++++++--------- src/server/sftp.rs | 111 ++++++++++++++-------- tests/scp_sftp_path_resolution_test.rs | 40 +++++--- 8 files changed, 189 insertions(+), 120 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e04f150c..83968e71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed -- **Make `sftp.root` chroot actually usable — resolve client absolute paths relative to the chroot root** (#214). With `sftp.root` configured, directory listing worked but every path resolution failed: `cd subdir`, `get file`, and even `get` of a file sitting directly at the chroot root were rejected as `path outside root` or `not found`, so only bare `/` and `readdir` functioned. Root cause: under chroot the client's coordinate space is rooted at `/` (this is what `realpath` reports and what the client sends back), but `resolve_chroot` treated a client absolute path such as `/subdir` as a *host* path and rejected anything not starting with the host root — so `/subdir` (meaning `/subdir`) never matched. A prior change had introduced this to avoid "path doubling", but real SFTP clients never send the host path; they send chroot-relative absolute paths, so the guard broke the normal case. `resolve_chroot` now interprets both absolute and relative client paths relative to `root`, ignoring a leading `/`, dropping `.`, and clamping `..` so traversal still cannot escape. Containment is unchanged and verified: `..` stays pinned at the root and a client `/etc/passwd` maps to `/etc/passwd` (a likely-nonexistent path inside the jail), never the host's `/etc/passwd`. Verified end-to-end with the OpenSSH `sftp` client (`cd`/`get`/root-level files/Unicode names all work; escape attempts stay confined) and covered by updated unit tests. +- **Make `sftp.root`/`scp.root` chroot usable by re-anchoring client paths under the chroot root** (#214). With a chroot configured, directory listing worked but every path resolution failed: `cd subdir`, `get file`, and even `get` of a file sitting directly at the chroot root were rejected as `path outside root` or `not found`, so only bare `/` and `readdir` functioned. Under chroot the client's coordinate space is rooted at `/` (this is what `realpath` reports and what the client sends back), but `resolve_chroot` treated a client absolute path such as `/subdir` as a *host* path and rejected anything not starting with the host root, so `/subdir` (meaning `/subdir`) never matched. A prior change had introduced this to avoid "path doubling", but real clients never send the host path; they send chroot-relative absolute paths, so the guard broke the normal case. The SFTP and SCP resolvers now both interpret absolute and relative client paths relative to `root` (OpenSSH `ChrootDirectory` re-rooting), ignoring a leading `/`, dropping `.`, and clamping `..` so traversal still cannot escape. Containment is verified: `..` stays pinned at the root and a client `/etc/passwd` maps to `/etc/passwd` inside the jail, never the host file. SCP previously kept the old "reject absolute outside root" behavior, so it is unified here to match `sftp.root`. Verified end-to-end with the OpenSSH `sftp` client (`cd`/`get`/root-level files/Unicode names all work; escape attempts stay confined) and covered by updated unit and integration tests. + +### Security +- **Confine absolute SFTP symlink targets to the chroot** (#214). Once `resolve_chroot` stopped rejecting out-of-root absolute paths, the SFTP `symlink` handler's containment guard became a no-op, so a chrooted client could create a link whose on-disk target was an absolute *host* path (for example `symlink /link /etc/passwd`). Because bssh uses a virtual chroot with no `chroot(2)`, that link resolved to the real host filesystem. Absolute symlink targets are now re-anchored under the chroot before the link is written, so a created link can never point outside the jail; relative targets keep OpenSSH-compatible verbatim storage. ## [2.2.3] - 2026-05-25 diff --git a/docs/architecture/server-configuration.md b/docs/architecture/server-configuration.md index 63c93571..d9b45a04 100644 --- a/docs/architecture/server-configuration.md +++ b/docs/architecture/server-configuration.md @@ -143,8 +143,10 @@ sftp: # 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. + # When set, clients are confined to this directory; the client's / is the + # chroot root, so absolute and relative paths alike are re-anchored under it + # (a host-looking /etc/passwd is confined to /etc/passwd) and .. is + # clamped to the chroot. root: /data/sftp # SCP protocol configuration diff --git a/docs/man/bssh-server.8 b/docs/man/bssh-server.8 index 92651054..74734376 100644 --- a/docs/man/bssh-server.8 +++ b/docs/man/bssh-server.8 @@ -174,9 +174,13 @@ Shell execution settings (default, command_timeout, env) .TP .B sftp 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. +directory that confines SFTP transfers. When set, the client's \fB/\fR is the +chroot root: absolute and relative client paths are both re-anchored under it +(OpenSSH \fBChrootDirectory\fR semantics), so a host-looking \fB/etc/passwd\fR +is confined to \fB/etc/passwd\fR and \fB..\fR cannot escape. 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 (\fBenabled\fR, \fBroot\fR). \fBroot\fR has the same diff --git a/docs/security.md b/docs/security.md index dc63e143..6d7285f0 100644 --- a/docs/security.md +++ b/docs/security.md @@ -241,18 +241,22 @@ scp: 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. +**Chroot semantics.** When `root` is set, the client's `/` is the chroot root +(OpenSSH `ChrootDirectory` re-rooting semantics). SFTP and SCP behave the same +way: + +- Both absolute and relative client paths are re-anchored under `root`, so + `/subdir` resolves to `/subdir` and plain `/` maps to the chroot + directory (interactive clients' `cd /`, `pwd` keep working). +- A host-looking path such as `/etc/passwd` is confined to `/etc/passwd`, + never the host file. +- `..` traversal is clamped at the chroot boundary and can never escape. +- Path-traversal and symlink-escape protections apply, including for paths + whose final component does not exist yet: the closest existing ancestor is + canonicalized and verified to stay inside `root`, which blocks + intermediate-directory symlinks pointing outside the chroot. An absolute + SFTP symlink target is itself re-anchored under `root`, so a created link can + never point at the host filesystem. When `root` is unset (default since v2.1.3, per #186), the handler runs without chroot. Absolute paths are honored verbatim and relative paths diff --git a/src/server/config/types.rs b/src/server/config/types.rs index d3228e4a..b08d4694 100644 --- a/src/server/config/types.rs +++ b/src/server/config/types.rs @@ -249,11 +249,15 @@ pub struct SftpConfig { /// 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 set, SFTP clients are confined to this directory. The client's `/` + /// is the chroot root (OpenSSH `ChrootDirectory` re-rooting semantics): + /// - Both absolute and relative client paths are re-anchored under `root`, + /// so `/subdir` resolves to `/subdir`. + /// - A host-looking path such as `/etc/passwd` is confined to + /// `/etc/passwd`, never the host file. + /// - `..` traversal is clamped to `root` (it can never escape), and + /// intermediate-directory symlinks pointing outside the chroot are + /// rejected. /// /// When `None` (default), no chroot is applied. This matches OpenSSH /// `sftp-server` behavior: absolute paths are used verbatim and relative diff --git a/src/server/scp.rs b/src/server/scp.rs index 30bf6878..d991f399 100644 --- a/src/server/scp.rs +++ b/src/server/scp.rs @@ -231,32 +231,19 @@ fn normalize_components(path: &Path) -> PathBuf { /// 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`. +/// Under chroot the client's `/` is the chroot root (OpenSSH `ChrootDirectory` +/// re-rooting semantics, matching `sftp.root`): +/// +/// - Both absolute and relative client paths are re-anchored under `root`, so +/// `/subdir` maps to `/subdir` and plain `/` maps to `root`. +/// - A host-looking path such as `/etc/passwd` is confined to +/// `/etc/passwd`, never the host file. +/// - `..` traversal is clamped to `root` (it can never escape). 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. + // Re-anchor the client path under `root`: a leading "/" denotes the chroot + // root (not a host-absolute path), so absolute and relative paths alike + // walk from `root`, dropping "." and clamping ".." so traversal cannot + // escape the chroot. let mut resolved = root.to_path_buf(); for component in requested.components() { match component { @@ -427,9 +414,12 @@ impl ScpHandler { /// 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. + /// - The client's `/` is the chroot root, so both absolute and relative + /// client paths are re-anchored under `root` (OpenSSH `ChrootDirectory` + /// semantics, matching `sftp.root`): `/subdir` resolves to + /// `/subdir`, and a host-looking `/etc/passwd` is confined to + /// `/etc/passwd`. + /// - `..` traversal is clamped to `root` (cannot escape). /// - 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 @@ -1353,30 +1343,45 @@ mod tests { } #[test] - 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")); + fn chroot_absolute_path_is_reanchored_under_root() { + // Under chroot the client's "/" IS the chroot root, so an absolute + // client path like "/file.bin" means "/file.bin" and is + // re-anchored under the root (matching sftp.root and OpenSSH + // ChrootDirectory), not treated as a host path. + let handler = chroot_handler(PathBuf::from("/file.bin")); + let result = handler.resolve_path(Path::new("/file.bin")).unwrap(); + assert_eq!(result, PathBuf::from("/home/testuser/file.bin")); + let result = handler - .resolve_path(Path::new("/home/testuser/file.bin")) + .resolve_path(Path::new("/documents/file.txt")) .unwrap(); - assert_eq!(result, PathBuf::from("/home/testuser/file.bin")); + assert_eq!(result, PathBuf::from("/home/testuser/documents/file.txt")); } #[test] - fn chroot_absolute_outside_root_is_rejected() { + fn chroot_absolute_host_path_is_confined_not_escaped() { + // A client cannot reach the host filesystem: an absolute path is + // confined under the chroot, so "/etc/passwd" maps to + // "/etc/passwd" (which does not exist) rather than the host's + // /etc/passwd. Rejecting absolute paths outright, as before, also broke + // every legitimate SCP path under chroot; re-anchoring both fixes that + // and unifies SCP with sftp.root. 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}" + assert_eq!( + handler.resolve_path(Path::new("/etc/passwd")).unwrap(), + PathBuf::from("/home/testuser/etc/passwd") + ); + assert_eq!( + handler.resolve_path(Path::new("/tmp/file.bin")).unwrap(), + PathBuf::from("/home/testuser/tmp/file.bin") + ); + // `..` cannot climb above the root even when prefixed with the host path. + assert_eq!( + handler + .resolve_path(Path::new("/home/testuser/../../etc/passwd")) + .unwrap(), + PathBuf::from("/home/testuser/etc/passwd") ); - - let err = handler - .resolve_path(Path::new("/tmp/file.bin")) - .unwrap_err(); - assert!(err.to_string().contains("outside root")); } #[test] diff --git a/src/server/sftp.rs b/src/server/sftp.rs index 342efca0..20cc3105 100644 --- a/src/server/sftp.rs +++ b/src/server/sftp.rs @@ -209,12 +209,17 @@ fn normalize_components(path: &Path) -> PathBuf { /// 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`. +/// Under chroot the client's coordinate space is rooted at `/` == `root` +/// (this is what `realpath` reports and what the client sends back), so both +/// absolute and relative client paths are interpreted relative to `root` +/// (OpenSSH `ChrootDirectory` re-rooting semantics): +/// +/// - A leading `/` denotes the chroot root, not a host-absolute path, so +/// `/subdir` maps to `/subdir` and plain `/` maps to `root`. +/// - Relative paths are joined with `root`. +/// - `..` traversal is clamped to `root` (it can never escape), and a +/// host-looking path such as `/etc/passwd` is confined to `/etc/passwd` +/// rather than reaching the host filesystem. fn resolve_chroot(requested: &Path, root: &Path) -> Result { use std::path::Component; @@ -402,11 +407,15 @@ impl SftpHandler { /// 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`. + /// - The client's `/` is the chroot root, so both absolute and relative + /// client paths are re-anchored under `root` (OpenSSH `ChrootDirectory` + /// semantics): `/subdir` resolves to `/subdir`. + /// - A host-looking path such as `/etc/passwd` is confined to + /// `/etc/passwd`, never the host file. /// - `..` traversal is clamped to `root` (cannot escape). + /// - The closest existing ancestor is canonicalized and verified to stay + /// inside `root`, blocking intermediate-directory symlinks that point + /// outside the chroot. /// /// ## Without chroot (`root_dir = None`): /// - Absolute paths are used verbatim. @@ -1374,34 +1383,29 @@ impl russh_sftp::server::Handler for SftpHandler { async move { let link_path = link_resolved?; - // 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. + // Validate the symlink target and decide what to store on disk. + // With chroot, both absolute and relative targets must resolve + // inside the chroot. Without chroot, mirror OpenSSH and let the + // kernel + filesystem permissions enforce access. + // + // `link_target` is what actually gets written as the link's target: + // the client value verbatim for relative / no-chroot links, or the + // re-anchored in-jail path for an absolute target under chroot. let target = Path::new(&targetpath); + let mut link_target = PathBuf::from(&targetpath); 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, - resolved = %resolved_target.display(), - "Symlink target resolves outside chroot" - ); - return Err(SftpError::permission_denied( - "Symlink target must be within root directory", - )); - } + // The client's absolute target is chroot-relative ("/" == + // root). Re-anchor it under `root` (clamping `..`) and store + // the re-anchored path, NOT the raw client string. Otherwise + // `symlink /link /etc/passwd` would write an on-disk link to + // the host's real `/etc/passwd`: since bssh has no kernel + // `chroot(2)`, a raw absolute target escapes the virtual + // jail. Re-anchoring confines it to `/etc/passwd`. + let resolved_target = resolve_chroot(target, root)?; + Self::ensure_target_in_root(root_dir.as_deref(), &resolved_target)?; + link_target = resolved_target; } else { // Relative target: combine with the link's parent directory // (or fall back to cwd) and ensure the result stays in @@ -1444,9 +1448,11 @@ impl russh_sftp::server::Handler for SftpHandler { } } - // Create symbolic link (target is stored as-is, validation above) + // Create the symbolic link. `link_target` is the client target + // verbatim for relative / no-chroot links, or the re-anchored + // in-jail path for an absolute target under chroot (validated above). #[cfg(unix)] - tokio::fs::symlink(&targetpath, &link_path).await?; + tokio::fs::symlink(&link_target, &link_path).await?; #[cfg(not(unix))] return Err(SftpError::not_supported()); @@ -1454,7 +1460,7 @@ impl russh_sftp::server::Handler for SftpHandler { tracing::info!( user = %user, link = %link_path.display(), - target = %targetpath, + target = %link_target.display(), "Created symbolic link" ); @@ -1968,4 +1974,35 @@ mod tests { assert!(attrs.mtime.is_some()); assert!(attrs.atime.is_some()); } + + #[tokio::test] + #[cfg(unix)] + async fn chroot_symlink_absolute_target_is_reanchored_not_host() { + // Security guard (#214 follow-up): under chroot an absolute symlink + // target is chroot-relative ("/" == root) and must be stored re-anchored + // under the root. Storing the raw client target would make + // `symlink /link /etc/passwd` write an on-disk link to the host's real + // /etc/passwd, escaping the virtual jail (bssh has no `chroot(2)`). + use russh_sftp::server::Handler; + use tempfile::tempdir; + + let dir = tempdir().unwrap(); + let chroot = dir.path().to_path_buf(); + let mut handler = SftpHandler::new( + UserInfo::new("testuser"), + Some(chroot.clone()), + chroot.clone(), + ); + + handler + .symlink(1, "/link".to_string(), "/etc/passwd".to_string()) + .await + .expect("symlink creation should succeed"); + + let on_disk = std::fs::read_link(chroot.join("link")).unwrap(); + // The stored target is confined under the chroot, never the host path. + assert_eq!(on_disk, chroot.join("etc/passwd")); + assert!(on_disk.starts_with(&chroot)); + assert_ne!(on_disk, PathBuf::from("/etc/passwd")); + } } diff --git a/tests/scp_sftp_path_resolution_test.rs b/tests/scp_sftp_path_resolution_test.rs index b2f29a38..d2439f04 100644 --- a/tests/scp_sftp_path_resolution_test.rs +++ b/tests/scp_sftp_path_resolution_test.rs @@ -20,8 +20,9 @@ //! //! - 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. +//! - With chroot, the client's `/` is the chroot root, so both absolute and +//! relative client paths are re-anchored under it (SFTP and SCP alike), with +//! `..` clamped and host-looking paths confined inside the root (#214). //! - Path-traversal and symlink-escape protections continue to hold under //! the new logic. //! @@ -98,22 +99,29 @@ fn scp_no_chroot_relative_path_lands_in_home() { } #[test] -fn scp_chroot_inside_root_no_doubling() { +fn scp_chroot_absolute_path_reanchored_under_root() { + // Under chroot the client's "/" IS the chroot root (matching sftp.root), so + // an absolute client path is re-anchored under the root: "/file.bin" means + // "/file.bin". The client sends chroot-relative paths, not host paths. let handler = ScpHandler::new( ScpMode::Sink, - PathBuf::from("/home/work/file.bin"), + PathBuf::from("/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"); + .resolve_path(Path::new("/file.bin")) + .expect("absolute chroot-relative path should resolve"); assert_eq!(resolved, PathBuf::from("/home/work/file.bin")); } #[test] -fn scp_chroot_rejects_paths_outside_root() { +fn scp_chroot_absolute_host_path_confined_under_root() { + // A client "/etc/passwd" is confined under the chroot, mapping to + // /etc/passwd, never the host's /etc/passwd. (Rejecting absolute + // paths outright, as before, also broke every legitimate path and diverged + // from sftp.root; #214.) let handler = ScpHandler::new( ScpMode::Sink, PathBuf::from("/etc/passwd"), @@ -121,13 +129,10 @@ fn scp_chroot_rejects_paths_outside_root() { Some(PathBuf::from("/home/work")), PathBuf::from("/home/work"), ); - let err = handler + let resolved = 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}" - ); + .expect("absolute host-looking path should be confined, not rejected"); + assert_eq!(resolved, PathBuf::from("/home/work/etc/passwd")); } #[test] @@ -260,8 +265,10 @@ fn scp_chroot_blocks_symlink_escape() { Some(chroot.clone()), chroot.clone(), ); + // The client sends the chroot-relative path "/escape"; canonicalizing the + // symlink must detect the target lands outside the chroot. let err = handler - .resolve_path(&escape_link) + .resolve_path(Path::new("/escape")) .expect_err("symlink escape must be blocked"); assert!( err.to_string().contains("symlink target outside root"), @@ -326,8 +333,11 @@ fn scp_chroot_blocks_parent_symlink_create() { chroot.clone(), ); + // The client sends a chroot-relative path traversing the `escape` parent + // symlink; canonicalization of the closest existing ancestor must still + // detect that it lands outside the chroot. let err = handler - .resolve_path(&target) + .resolve_path(Path::new("/escape/newfile.txt")) .expect_err("parent-symlink escape must be blocked"); assert!( err.to_string().contains("outside root"), From 9133836ad1e8742ebb817434006488d09f79c82c Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 16 Jul 2026 18:34:37 +0900 Subject: [PATCH 4/4] docs(server): fix stale chroot doc on runtime ServerConfig.sftp_root The runtime `ServerConfig.sftp_root` field (which actually drives the SFTP/SCP resolvers via handler.rs) still documented the old "absolute paths outside root are rejected" behavior. Align it with the re-rooting model already applied to `SftpConfig.root` and the rest of the docs. `scp_root` inherits the corrected wording through its "same semantics as sftp_root" reference. --- src/server/config/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/server/config/mod.rs b/src/server/config/mod.rs index 8dd91e60..e140a020 100644 --- a/src/server/config/mod.rs +++ b/src/server/config/mod.rs @@ -160,8 +160,10 @@ pub struct ServerConfig { /// 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`. + /// When set, SFTP clients are confined to this directory. The client's `/` + /// is the chroot root, so absolute and relative paths are re-anchored under + /// it (a host-looking `/etc/passwd` is confined to `/etc/passwd`) and + /// `..` cannot escape. #[serde(default)] pub sftp_root: Option,