Merge branch 'hans/ssh/sftpd_fixes/OTP-14225' into maint

2 files changed, 186 insertions, 30 deletions

diff --git a/lib/ssh/src/ssh_sftpd.erl b/lib/ssh/src/ssh_sftpd.erl
index b739955836..9352046795 100644
--- a/lib/ssh/src/ssh_sftpd.erl
+++ b/lib/ssh/src/ssh_sftpd.erl
@@ -664,29 +664,25 @@ open(Vsn, ReqId, Data, State) when Vsn >= 4 ->
     do_open(ReqId, State, Path, Flags).
 
 do_open(ReqId, State0, Path, Flags) ->
-    #state{file_handler = FileMod, file_state = FS0, root = Root, xf = #ssh_xfer{vsn = Vsn}} = State0,
-    XF = State0#state.xf,
-    F = [binary | Flags],
-    {IsDir, _FS1} = FileMod:is_dir(Path, FS0),
+    #state{file_handler = FileMod, file_state = FS0, xf = #ssh_xfer{vsn = Vsn}} = State0,
+    AbsPath = relate_file_name(Path, State0),
+    {IsDir, _FS1} = FileMod:is_dir(AbsPath, FS0),
     case IsDir of 
 	true when Vsn > 5 ->
 	    ssh_xfer:xf_send_status(State0#state.xf, ReqId,
-    				    ?SSH_FX_FILE_IS_A_DIRECTORY, "File is a directory");
+				    ?SSH_FX_FILE_IS_A_DIRECTORY, "File is a directory"),
+	    State0;
 	true ->
 	    ssh_xfer:xf_send_status(State0#state.xf, ReqId,
-    				    ?SSH_FX_FAILURE, "File is a directory");
+				    ?SSH_FX_FAILURE, "File is a directory"),
+	    State0;
 	false ->
-	    AbsPath = case Root of
-			  "" ->
-			      Path;
-			  _ ->
-			      relate_file_name(Path, State0)  
-		      end,
-	    {Res, FS1} = FileMod:open(AbsPath, F, FS0),
+	    OpenFlags = [binary | Flags],
+	    {Res, FS1} = FileMod:open(AbsPath, OpenFlags, FS0),
 	    State1 = State0#state{file_state = FS1},
 	    case Res of
 		{ok, IoDevice} ->
-		    add_handle(State1, XF, ReqId, file, {Path,IoDevice});
+		    add_handle(State1, State0#state.xf, ReqId, file, {Path,IoDevice});
 		{error, Error} ->
 		    ssh_xfer:xf_send_status(State1#state.xf, ReqId,
 					    ssh_xfer:encode_erlang_status(Error)),
@@ -742,6 +738,10 @@ resolve_symlinks_2([], State, _LinkCnt, AccPath) ->
     {{ok, AccPath}, State}.
 
 
+%% The File argument is always in a user visible file system, i.e.
+%% is under Root and is relative to CWD or Root, if starts with "/".
+%% The result of the function is always an absolute path in a
+%% "backend" file system.
 relate_file_name(File, State) ->
     relate_file_name(File, State, _Canonicalize=true).
 
@@ -749,19 +749,20 @@ relate_file_name(File, State, Canonicalize) when is_binary(File) ->
     relate_file_name(unicode:characters_to_list(File), State, Canonicalize);
 relate_file_name(File, #state{cwd = CWD, root = ""}, Canonicalize) ->
     relate_filename_to_path(File, CWD, Canonicalize);
-relate_file_name(File, #state{root = Root}, Canonicalize) ->
-    case is_within_root(Root, File) of
-	true ->
-	    File;
-	false ->
-	    RelFile = make_relative_filename(File),
-	    NewFile = relate_filename_to_path(RelFile, Root, Canonicalize),
-	    case is_within_root(Root, NewFile) of
-		true ->
-		    NewFile;
-		false ->
-		    Root
-	    end
+relate_file_name(File, #state{cwd = CWD, root = Root}, Canonicalize) ->
+    CWD1 = case is_within_root(Root, CWD) of
+	       true  -> CWD;
+	       false -> Root
+	   end,
+    AbsFile = case make_relative_filename(File) of
+		  File ->
+		       relate_filename_to_path(File, CWD1, Canonicalize);
+		  RelFile ->
+		       relate_filename_to_path(RelFile, Root, Canonicalize)
+	      end,
+    case is_within_root(Root, AbsFile) of
+	true  -> AbsFile;
+	false -> Root
     end.
 
 is_within_root(Root, File) ->
diff --git a/lib/ssh/test/ssh_sftpd_SUITE.erl b/lib/ssh/test/ssh_sftpd_SUITE.erl
index 52a26110c4..6d18a980ee 100644
--- a/lib/ssh/test/ssh_sftpd_SUITE.erl
+++ b/lib/ssh/test/ssh_sftpd_SUITE.erl
@@ -65,7 +65,12 @@ all() ->
      ver3_open_flags,
      relpath, 
      sshd_read_file,
-     ver6_basic].
+     ver6_basic,
+     access_outside_root,
+     root_with_cwd,
+     relative_path,
+     open_file_dir_v5,
+     open_file_dir_v6].
 
 groups() -> 
     [].
@@ -117,6 +122,31 @@ init_per_testcase(TestCase, Config) ->
 		      ver6_basic ->
 			  SubSystems = [ssh_sftpd:subsystem_spec([{sftpd_vsn, 6}])],
 			  ssh:daemon(0, [{subsystems, SubSystems}|Options]);
+                      access_outside_root ->
+                          %% Build RootDir/access_outside_root/a/b and set Root and CWD
+                          BaseDir = filename:join(PrivDir, access_outside_root),
+                          RootDir = filename:join(BaseDir, a),
+                          CWD     = filename:join(RootDir, b),
+                          %% Make the directory chain:
+                          ok = filelib:ensure_dir(filename:join(CWD, tmp)),
+                          SubSystems = [ssh_sftpd:subsystem_spec([{root, RootDir},
+                                                                  {cwd, CWD}])],
+                          ssh:daemon(0, [{subsystems, SubSystems}|Options]);
+		      root_with_cwd ->
+			  RootDir = filename:join(PrivDir, root_with_cwd),
+			  CWD     = filename:join(RootDir, home),
+			  SubSystems = [ssh_sftpd:subsystem_spec([{root, RootDir}, {cwd, CWD}])],
+			  ssh:daemon(0, [{subsystems, SubSystems}|Options]);
+		      relative_path ->
+			  SubSystems = [ssh_sftpd:subsystem_spec([{cwd, PrivDir}])],
+			  ssh:daemon(0, [{subsystems, SubSystems}|Options]);
+		      open_file_dir_v5 ->
+			  SubSystems = [ssh_sftpd:subsystem_spec([{cwd, PrivDir}])],
+			  ssh:daemon(0, [{subsystems, SubSystems}|Options]);
+		      open_file_dir_v6 ->
+			  SubSystems = [ssh_sftpd:subsystem_spec([{cwd, PrivDir},
+								  {sftpd_vsn, 6}])],
+			  ssh:daemon(0, [{subsystems, SubSystems}|Options]);
 		      _ ->
 			  SubSystems = [ssh_sftpd:subsystem_spec([])],
 			  ssh:daemon(0, [{subsystems, SubSystems}|Options])
@@ -646,6 +676,133 @@ ver6_basic(Config) when is_list(Config) ->
 	open_file(PrivDir, Cm, Channel, ReqId,
 		  ?ACE4_READ_DATA  bor ?ACE4_READ_ATTRIBUTES,
 		  ?SSH_FXF_OPEN_EXISTING).
+
+%%--------------------------------------------------------------------
+access_outside_root() ->
+    [{doc, "Try access files outside the tree below RootDir"}].
+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, <<>>),
+    {Cm, Channel} = proplists:get_value(sftp, Config),
+    %% Try to access a file parallell to the RootDir:
+    try_access("/../bad",   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(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]),
+    case Return of
+        {ok, <<?SSH_FXP_HANDLE, ?UINT32(ReqId), _Handle0/binary>>, _} ->
+            ct:fail("Could open a file outside the root tree!");
+        {ok, <<?SSH_FXP_STATUS, ?UINT32(ReqId), ?UINT32(Code), Rest/binary>>, <<>>} ->
+            case Code of
+                ?SSH_FX_FILE_IS_A_DIRECTORY ->
+                    ct:pal("Got the expected SSH_FX_FILE_IS_A_DIRECTORY status",[]),
+                    ok;
+                ?SSH_FX_FAILURE ->
+                    ct:pal("Got the expected SSH_FX_FAILURE status",[]),
+                    ok;
+                _ ->
+                    case Rest of
+                        <<?UINT32(Len), Txt:Len/binary, _/binary>> ->
+                            ct:fail("Got unexpected SSH_FX_code: ~p (~p)",[Code,Txt]);
+                        _ ->
+                            ct:fail("Got unexpected SSH_FX_code: ~p",[Code])
+                    end
+            end;
+        _ ->
+            ct:fail("Completly unexpected return: ~p", [Return])
+    end.
+
+%%--------------------------------------------------------------------
+root_with_cwd() ->
+    [{doc, "Check if files are found, if the CWD and Root are specified"}].
+root_with_cwd(Config) when is_list(Config) ->
+    PrivDir =  proplists:get_value(priv_dir, Config),
+    RootDir = filename:join(PrivDir, root_with_cwd),
+    CWD     = filename:join(RootDir, home),
+    FileName = "root_with_cwd.txt",
+    FilePath = filename:join(CWD, FileName),
+    ok = filelib:ensure_dir(FilePath),
+    ok = file:write_file(FilePath ++ "0", <<>>),
+    ok = file:write_file(FilePath ++ "1", <<>>),
+    ok = file:write_file(FilePath ++ "2", <<>>),
+    {Cm, Channel} = proplists:get_value(sftp, Config),
+    ReqId0 = 0,
+    {ok, <<?SSH_FXP_HANDLE, ?UINT32(ReqId0), _Handle0/binary>>, _} =
+	open_file(FileName ++ "0", Cm, Channel, ReqId0,
+		  ?ACE4_READ_DATA  bor ?ACE4_READ_ATTRIBUTES,
+		  ?SSH_FXF_OPEN_EXISTING),
+    ReqId1 = 1,
+    {ok, <<?SSH_FXP_HANDLE, ?UINT32(ReqId1), _Handle1/binary>>, _} =
+	open_file("./" ++ FileName ++ "1", Cm, Channel, ReqId1,
+		  ?ACE4_READ_DATA  bor ?ACE4_READ_ATTRIBUTES,
+		  ?SSH_FXF_OPEN_EXISTING),
+    ReqId2 = 2,
+    {ok, <<?SSH_FXP_HANDLE, ?UINT32(ReqId2), _Handle2/binary>>, _} =
+	open_file("/home/" ++ FileName ++ "2", Cm, Channel, ReqId2,
+		  ?ACE4_READ_DATA  bor ?ACE4_READ_ATTRIBUTES,
+		  ?SSH_FXF_OPEN_EXISTING).
+
+%%--------------------------------------------------------------------
+relative_path() ->
+    [{doc, "Test paths relative to CWD when opening a file handle."}].
+relative_path(Config) when is_list(Config) ->
+    PrivDir =  proplists:get_value(priv_dir, Config),
+    FileName = "test_relative_path.txt",
+    FilePath = filename:join(PrivDir, FileName),
+    ok = filelib:ensure_dir(FilePath),
+    ok = file:write_file(FilePath, <<>>),
+    {Cm, Channel} = proplists:get_value(sftp, Config),
+    ReqId = 0,
+    {ok, <<?SSH_FXP_HANDLE, ?UINT32(ReqId), _Handle/binary>>, _} =
+        open_file(FileName, Cm, Channel, ReqId,
+                  ?ACE4_READ_DATA  bor ?ACE4_READ_ATTRIBUTES,
+                  ?SSH_FXF_OPEN_EXISTING).
+
+%%--------------------------------------------------------------------
+open_file_dir_v5() ->
+    [{doc, "Test if open_file fails when opening existing directory."}].
+open_file_dir_v5(Config) when is_list(Config) ->
+    PrivDir =  proplists:get_value(priv_dir, Config),
+    FileName = "open_file_dir_v5",
+    FilePath = filename:join(PrivDir, FileName),
+    ok = filelib:ensure_dir(FilePath),
+    ok = file:make_dir(FilePath),
+    {Cm, Channel} = proplists:get_value(sftp, Config),
+    ReqId = 0,
+    {ok, <<?SSH_FXP_STATUS, ?UINT32(ReqId),
+	   ?UINT32(?SSH_FX_FAILURE), _/binary>>, _} =
+        open_file(FileName, Cm, Channel, ReqId,
+                  ?ACE4_READ_DATA  bor ?ACE4_READ_ATTRIBUTES,
+                  ?SSH_FXF_OPEN_EXISTING).
+
+%%--------------------------------------------------------------------
+open_file_dir_v6() ->
+    [{doc, "Test if open_file fails when opening existing directory."}].
+open_file_dir_v6(Config) when is_list(Config) ->
+    PrivDir =  proplists:get_value(priv_dir, Config),
+    FileName = "open_file_dir_v6",
+    FilePath = filename:join(PrivDir, FileName),
+    ok = filelib:ensure_dir(FilePath),
+    ok = file:make_dir(FilePath),
+    {Cm, Channel} = proplists:get_value(sftp, Config),
+    ReqId = 0,
+    {ok, <<?SSH_FXP_STATUS, ?UINT32(ReqId),
+	   ?UINT32(?SSH_FX_FILE_IS_A_DIRECTORY), _/binary>>, _} =
+        open_file(FileName, Cm, Channel, ReqId,
+                  ?ACE4_READ_DATA  bor ?ACE4_READ_ATTRIBUTES,
+                  ?SSH_FXF_OPEN_EXISTING).
+
 %%--------------------------------------------------------------------
 %% Internal functions ------------------------------------------------
 %%--------------------------------------------------------------------
@@ -688,9 +845,7 @@ reply(Cm, Channel, RBuf) ->
 	30000 -> ct:fail("timeout ~p:~p",[?MODULE,?LINE])
     end.
 
-
 open_file(File, Cm, Channel, ReqId, Access, Flags) ->
-
     Data = list_to_binary([?uint32(ReqId),
 			   ?binary(list_to_binary(File)),
 			   ?uint32(Access),