From 443ef1a9ccb5745d345d8d11e62a3786e97736e6 Mon Sep 17 00:00:00 2001 From: Jakub Witczak Date: Fri, 27 Feb 2026 12:24:47 +0100 Subject: [PATCH] ssh: Fix path traversal vulnerability in ssh_sftpd root directory validation The is_within_root/2 function used string prefix matching via lists:prefix/2, which allowed access to sibling directories with matching name prefixes (e.g., /tmp/root2/ when root is /tmp/root/). Changed to use path component-based validation with filename:split/1 to ensure proper directory containment checking. Added test cases for sibling directory bypass attempts in access_outside_root/1 test case. Security impact: Prevents authenticated SFTP users from escaping their configured root directory jail via sibling directory access. --- lib/ssh/doc/guides/hardening.md | 25 ++++++++++++++++++ lib/ssh/src/ssh_sftpd.erl | 25 ++++++++++++++---- lib/ssh/test/ssh_sftpd_SUITE.erl | 44 ++++++++++++++++++++++---------- 3 files changed, 76 insertions(+), 18 deletions(-) diff --git a/lib/ssh/doc/guides/hardening.md b/lib/ssh/doc/guides/hardening.md index 1685af86a66e..e0141c844578 100644 --- a/lib/ssh/doc/guides/hardening.md +++ b/lib/ssh/doc/guides/hardening.md @@ -241,3 +241,28 @@ is in milliseconds, and the initial value is infinity. The negotiation (session setup time) time can be limited with the _parameter_ `NegotiationTimeout` in a call establishing an ssh session, for example `ssh:connect/3`. + +## SFTP Security + +### Root Directory Isolation + +The [`root`](`m:ssh_sftpd`) option restricts SFTP users to a +specific directory tree, preventing access to files outside that directory. + +**Example:** + +```erlang +ssh:daemon(Port, [ + {subsystems, [ssh_sftpd:subsystem_spec([{root, "/home/sftpuser"}])]}, + ... +]). +``` + +**Important:** The `root` option is configured per daemon, not per user. All +users connecting to the same daemon share the same root directory. For per-user +isolation, consider running separate daemon instances on different ports or +using OS-level mechanisms (PAM chroot, containers, file permissions). + +**Defense-in-depth:** For high-security deployments, combine the `root` option +with OS-level isolation mechanisms such as chroot jails, containers, or +mandatory access control (SELinux, AppArmor). diff --git a/lib/ssh/src/ssh_sftpd.erl b/lib/ssh/src/ssh_sftpd.erl index e15b5d380b40..ebe7a03dd1b3 100644 --- a/lib/ssh/src/ssh_sftpd.erl +++ b/lib/ssh/src/ssh_sftpd.erl @@ -100,10 +100,15 @@ Options: data provided by the SFTP client. (Note: limitations might be also enforced by underlying operating system) -- **`root`** - Sets the SFTP root directory. Then the user cannot see any files - above this root. If, for example, the root directory is set to `/tmp`, then - the user sees this directory as `/`. If the user then writes `cd /etc`, the - user moves to `/tmp/etc`. +- **`root`** - Sets the SFTP root directory. The user cannot access files + outside this directory tree. If, for example, the root directory is set to + `/tmp`, then the user sees this directory as `/`. If the user then writes + `cd /etc`, the user moves to `/tmp/etc`. + + Note: This provides application-level isolation. For additional security, + consider using OS-level chroot or similar mechanisms. See the + [SFTP Security](hardening.md#sftp-security) section in the Hardening guide + for deployment recommendations. - **`sftpd_vsn`** - Sets the SFTP version to use. Defaults to 5. Version 6 is under development and limited. @@ -926,7 +931,17 @@ relate_file_name(File, #state{cwd = CWD, root = Root}, Canonicalize) -> end. is_within_root(Root, File) -> - lists:prefix(Root, File). + RootParts = filename:split(Root), + FileParts = filename:split(File), + is_prefix_components(RootParts, FileParts). + +%% Verify if request file path is within configured root directory +is_prefix_components([], _) -> + true; +is_prefix_components([H|T1], [H|T2]) -> + is_prefix_components(T1, T2); +is_prefix_components(_, _) -> + false. %% Remove leading slash (/), if any, in order to make the filename %% relative (to the root) diff --git a/lib/ssh/test/ssh_sftpd_SUITE.erl b/lib/ssh/test/ssh_sftpd_SUITE.erl index c7e6094be92d..3d3219682d61 100644 --- a/lib/ssh/test/ssh_sftpd_SUITE.erl +++ b/lib/ssh/test/ssh_sftpd_SUITE.erl @@ -33,8 +33,7 @@ end_per_testcase/2 ]). --export([ - access_outside_root/1, +-export([access_outside_root/1, links/1, mk_rm_dir/1, open_close_dir/1, @@ -161,7 +160,7 @@ init_per_testcase(TestCase, Config) -> RootDir = filename:join(BaseDir, a), CWD = filename:join(RootDir, b), %% Make the directory chain: - ok = filelib:ensure_dir(filename:join(CWD, tmp)), + ok = filelib:ensure_path(CWD), SubSystems = [ssh_sftpd:subsystem_spec([{root, RootDir}, {cwd, CWD}])], ssh:daemon(0, [{subsystems, SubSystems}|Options]); @@ -222,7 +221,12 @@ init_per_testcase(TestCase, Config) -> [{sftp, {Cm, Channel}}, {sftpd, Sftpd }| Config]. end_per_testcase(_TestCase, Config) -> - catch ssh:stop_daemon(proplists:get_value(sftpd, Config)), + try + ssh:stop_daemon(proplists:get_value(sftpd, Config)) + catch + Class:Error:_Stack -> + ?CT_LOG("Class = ~p Error = ~p", [Class, Error]) + end, {Cm, Channel} = proplists:get_value(sftp, Config), ssh_connection:close(Cm, Channel), ssh:close(Cm), @@ -689,33 +693,47 @@ ver6_basic(Config) when is_list(Config) -> access_outside_root(Config) when is_list(Config) -> PrivDir = proplists:get_value(priv_dir, Config), BaseDir = filename:join(PrivDir, access_outside_root), - %% A file outside the tree below RootDir which is BaseDir/a - %% Make the file BaseDir/bad : BadFilePath = filename:join([BaseDir, bad]), ok = file:write_file(BadFilePath, <<>>), + FileInSiblingDir = filename:join([BaseDir, a2, "secret.txt"]), + ok = filelib:ensure_dir(FileInSiblingDir), + ok = file:write_file(FileInSiblingDir, <<"secret">>), + TestFolderStructure = ~""" + PrivDir + |-- access_outside_root (BaseDir) + | |-- a (RootDir folder) + | | +-- b (CWD folder) + | |-- a2 (sibling folder with name prefix equal to RootDir) + | | +-- secret.txt + | +-- bad.txt + """, + ?CT_LOG("TestFolderStructure = ~n~s", [TestFolderStructure]), {Cm, Channel} = proplists:get_value(sftp, Config), - %% Try to access a file parallel to the RootDir: - try_access("/../bad", Cm, Channel, 0), + %% Try to access a file parallel to the RootDir using parent traversal: + try_access("/../bad.txt", Cm, Channel, 0), %% Try to access the same file via the CWD which is /b relative to the RootDir: - try_access("../../bad", Cm, Channel, 1). - + try_access("../../bad.txt", Cm, Channel, 1), + %% Try to access sibling folder name prefixed with root dir + try_access("/../a2/secret.txt", Cm, Channel, 2), + try_access("../../a2/secret.txt", Cm, Channel, 3). try_access(Path, Cm, Channel, ReqId) -> Return = open_file(Path, Cm, Channel, ReqId, ?ACE4_READ_DATA bor ?ACE4_READ_ATTRIBUTES, ?SSH_FXF_OPEN_EXISTING), - ct:log("Try open ~p -> ~p",[Path,Return]), + ?CT_LOG("Try open ~p -> ~w",[Path,Return]), case Return of {ok, <>, _} -> + ?CT_LOG("Got the unexpected ?SSH_FXP_HANDLE",[]), ct:fail("Could open a file outside the root tree!"); {ok, <>, <<>>} -> case Code of ?SSH_FX_FILE_IS_A_DIRECTORY -> - ct:log("Got the expected SSH_FX_FILE_IS_A_DIRECTORY status",[]), + ?CT_LOG("Got the expected SSH_FX_FILE_IS_A_DIRECTORY status",[]), ok; ?SSH_FX_FAILURE -> - ct:log("Got the expected SSH_FX_FAILURE status",[]), + ?CT_LOG("Got the expected SSH_FX_FAILURE status",[]), ok; _ -> case Rest of