ssh: Change shell and exec defaults to disabled

lib/kernel/test/shell_test_lib.erl

@@ -182,9 +182,10 @@ setup_tty(Config) ->
                 PrivDir = filename:join(proplists:get_value(priv_dir, Config), "nopubkey"),
                 file:make_dir(PrivDir),
                 SysDir = proplists:get_value(data_dir, Config),
-                {ok, _Sshd} = ssh:daemon(8989, [{system_dir, SysDir},
-                                    {user_dir, PrivDir},
-                                    {password, "bar"}])
+                {ok, _Sshd} = ssh:daemon(8989, [{shell, {shell, start, []}},
+                                                {system_dir, SysDir},
+                                                {user_dir, PrivDir},
+                                                {password, "bar"}])
             end),
             os:cmd(os:find_executable("tmux") ++ " new-window -n " ++ ClientName ++ " -d -- "++
                 "ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null localhost -p 8989 -l foo"),

lib/ssh/doc/guides/hardening.md

@@ -122,7 +122,7 @@ excessive memory consumption.
 
 ### SFTP Server Resource Limits
 
-When running an SFTP server via `ssh_sftpd:subsystem_spec/1`, additional
+When enabling the SFTP subsystem via `ssh_sftpd:subsystem_spec/1`, additional
 resource limits can be configured to protect against resource exhaustion attacks:
 
 #### max_handles
@@ -303,10 +303,9 @@ _exec_ server-side service takes a string provided by the client, evaluates it
 and returns the result. The _shell_ function enables the client to open a shell
 in the shell host.
 
-Those service could - and should - be disabled when they are not needed. The
-options [exec](`t:ssh:exec_daemon_option/0`) and
-[shell](`t:ssh:shell_daemon_option/0`) are enabled per default but could be set
-to `disabled` if not needed. The same options could also install handlers for
+The options [exec](`t:ssh:exec_daemon_option/0`) and
+[shell](`t:ssh:shell_daemon_option/0`) are disabled per default.
+The same options could also install handlers for
 the string(s) passed from the client to the server.
 
 ### The id string
@@ -358,6 +357,10 @@ The negotiation (session setup time) time can be limited with the _parameter_
 
 ## SFTP Security
 
+Note that the SFTP server runs with the file access rights of the OS
+process running the Erlang emulator, regardless of the authenticated
+SSH user. See the [Terminology](terminology.md) section for details.
+
 ### Root Directory Isolation
 
 The `root` option (see `m:ssh_sftpd`) restricts SFTP users to a

lib/ssh/doc/guides/introduction.md

@@ -34,8 +34,9 @@ The `ssh` application is an implementation of the SSH Transport, Connection and
 Authentication Layer Protocols in Erlang. It provides the following:
 
 - API functions to write customized SSH clients and servers applications
-- The Erlang shell available over SSH
-- An SFTP client (`m:ssh_sftp`) and server (`m:ssh_sftpd`)
+- The Erlang shell available over SSH, enabled via the `shell` daemon option
+- An SFTP client (`m:ssh_sftp`) and server (`m:ssh_sftpd`), enabled via the
+  `subsystems` daemon option
 
 ## Prerequisites
 
@@ -114,15 +115,16 @@ the application logic.
 Channels come in the following three flavors:
 
 - _Subsystem_ \- Named services that can be run as part of an SSH server, such
-  as SFTP [(ssh_sftpd)](`m:ssh_sftpd`), that is built into the SSH daemon
-  (server) by default, but it can be disabled. The Erlang `ssh` daemon can be
-  configured to run any Erlang- implemented SSH subsystem.
-- _Shell_ \- Interactive shell. By default the Erlang daemon runs the Erlang
-  shell. The shell can be customized by providing your own read-eval-print loop.
+  as SFTP [(ssh_sftpd)](`m:ssh_sftpd`). No subsystems are enabled by default.
+  The Erlang `ssh` daemon can be configured to run any Erlang-implemented SSH
+  subsystem.
+- _Shell_ \- Interactive shell. By default the Erlang daemon does not expose the Erlang
+  shell. It can be enabled with option `{shell, {shell, start, []}}`
+  The shell can be customized by providing your own read-eval-print loop.
   You can also provide your own Command-Line Interface (CLI) implementation, but
   that is much more work.
-- _Exec_ \- One-time remote execution of commands. See function
-  `ssh_connection:exec/4` for more information.
+- _Exec_ \- By default one-time remote execution of commands is disabled.
+  See function `ssh_connection:exec/4` for more information.
 
 ## Where to Find More Information
 

lib/ssh/doc/guides/terminology.md

@@ -157,8 +157,9 @@ the server's emulator. The rights in that shell is independent of the just
 authenticated user.
 
 In case of an sftp request, an sftp server is started with the rights of the
-user of the Erlang emulator's OS process. So with sftp the authenticated user
-does not influence the rights.
+user of the Erlang emulator's OS process, provided the SFTP subsystem is
+enabled. So with sftp, the file access rights are those of the OS process
+running the Erlang emulator, regardless of the authenticated SSH user.
 
 So after an authentication, the user name is not used anymore and has no
 influence.

lib/ssh/doc/guides/using_ssh.md

@@ -83,7 +83,8 @@ _Step 3._ Start the Erlang `ssh` daemon:
 1> ssh:start().
 ok
 2> {ok, Sshd} = ssh:daemon(8989, [{system_dir, "/tmp/ssh_daemon"},
-                                  {user_dir, "/tmp/otptest_user/.ssh"}]).
+                                  {user_dir, "/tmp/otptest_user/.ssh"},
+                                  {shell, {shell, start, []}}]).
 {ok,<0.54.0>}
 3>
 ```
@@ -432,7 +433,8 @@ and `ClientAddress`). See the
 
 ## SFTP Server
 
-Start the Erlang `ssh` daemon with the SFTP subsystem:
+The SFTP subsystem is not enabled by default. To start an SSH daemon with
+SFTP, configure the `subsystems` option explicitly:
 
 ```erlang
 1> ssh:start().

lib/ssh/doc/ssh_app.md

@@ -28,8 +28,9 @@ SSH File Transfer Protocol (SFTP) client and server.
 
 The `ssh` application is an implementation of the SSH protocol in Erlang. `ssh`
 offers API functions to write customized SSH clients and servers as well as
-making the Erlang shell available over SSH. An SFTP client, `ssh_sftp`, and
-server, `ssh_sftpd`, are also included.
+making the Erlang shell available over SSH. An SFTP client (`ssh_sftp`) and
+server (`ssh_sftpd`) are also included. The SFTP server can be enabled via
+the `subsystems` daemon option.
 
 ## Dependencies
 

lib/ssh/src/ssh.hrl

@@ -36,8 +36,6 @@
 
 -define(DEFAULT_TRANSPORT,  {tcp, gen_tcp, tcp_closed} ).
 
--define(DEFAULT_SHELL, {shell, start, []} ).
-
 -define(DEFAULT_TIMEOUT, 5000).
 
 -define(MAX_RND_PADDING_LEN, 15).
@@ -210,10 +208,13 @@ The `channel_callback` is the module that implements the `m:ssh_server_channel`
 [Creating a Subsystem](using_ssh.md#usersguide_creating_a_subsystem) in the
 User's Guide for more information and an example.
 
-If the subsystems option is not present, the value of
-`ssh_sftpd:subsystem_spec([])` is used. This enables the sftp subsystem by
-default. The option can be set to the empty list if you do not want the daemon
-to run any subsystems.
+If the subsystems option is not present, the default is an empty list
+and no subsystems are enabled.
+
+To enable the SFTP subsystem:
+```
+ssh:daemon(Port, [{subsystems, [ssh_sftpd:subsystem_spec([])]} | Options])
+```
 """.
 -doc(#{group => <<"Daemon Options">>}).
 -type subsystem_spec()        :: {Name::string(), mod_args()} .
@@ -842,6 +843,14 @@ risk.
 -type shell_daemon_option()     :: {shell, shell_spec()} .
 -doc(#{group => <<"Daemon Options">>}).
 -type shell_spec() :: mod_fun_args() | shell_fun() | disabled .
+-doc """
+The default is `disabled`.
+
+To enable the Erlang shell (the behavior from OTP versions prior to OTP @OTP-19969@):
+```
+ssh:daemon(Port, [{shell, {shell, start, []}} | Options])
+```
+""".
 -doc(#{group => <<"Daemon Options">>,
        equiv => 'shell_fun/2'/0}).
 -type shell_fun() :: 'shell_fun/1'()  | 'shell_fun/2'() .
@@ -850,7 +859,7 @@ risk.
 -type 'shell_fun/1'() :: fun((User::string()) -> pid()) .
 -doc """
 Defines the read-eval-print loop used in a daemon when a shell is requested by
-the client. The default is to use the Erlang shell: `{shell, start, []}`
+the client.
 
 See the option [`exec-option`](`t:exec_daemon_option/0`) for a description of
 how the daemon executes shell-requests and exec-requests depending on the shell-
@@ -862,7 +871,23 @@ and exec-options.
 -doc(#{group => <<"Daemon Options">>}).
 -type exec_daemon_option()      :: {exec, exec_spec()} .
 -doc(#{group => <<"Daemon Options">>}).
--type exec_spec()               :: {direct, exec_fun()} | disabled | deprecated_exec_opt().
+-type exec_spec()               :: {direct, exec_fun()} | disabled | deprecated_exec_opt() | erlang_eval.
+-doc """
+The default is `disabled`.
+
+Value `erlang_eval` enables evaluation of Erlang terms via exec requests.
+This works when the shell option is either `disabled` (no shell) or
+`{shell, start, []}` (Erlang shell). It does not work with custom shells.
+
+To restore the behavior from OTP versions prior to OTP @OTP-19969@, configure:
+```
+ssh:daemon(Port, [{shell, {shell, start, []}},
+                  {exec, erlang_eval}
+                  | Options])
+```
+
+For new code, consider using `{direct, Fun}` for more controlled exec handling.
+""".
 -doc(#{group => <<"Daemon Options">>}).
 -type exec_fun()                :: 'exec_fun/1'() | 'exec_fun/2'() | 'exec_fun/3'().
 -doc(#{group => <<"Daemon Options">>}).

lib/ssh/src/ssh_cli.erl

@@ -41,19 +41,20 @@
 
 %% state
 -record(state, {
-	  cm,
-	  channel,
-	  pty,
-          encoding,
-          deduced_encoding, % OpenSSH sometimes lies about its encodeing. This variable
-                            % is for the process of guessing the peer encoding, taylord
-                            % after the behaviour of openssh.  If it says latin1 it is so.
-                            % It there arrives characters encoded in latin1 it is so. Otherwise
-                            % assume utf8 until otherwise is proved.
-	  group,
-	  shell,
-	  exec,
-      tty
+                cm,
+                channel,
+                pty,
+                encoding,
+                %% OpenSSH sometimes lies about its encodeing. This variable
+                %% is for the process of guessing the peer encoding, taylord
+                %% after the behaviour of openssh.  If it says latin1 it is so.
+                %% It there arrives characters encoded in latin1 it is so. Otherwise
+                %% assume utf8 until otherwise is proved.
+                deduced_encoding,
+                group,
+                shell,
+                exec = erlang_eval,
+                tty
 
 	 }).
 
@@ -212,16 +213,16 @@ handle_ssh_msg({ssh_cm, ConnectionHandler,  {exec, ChannelId, WantReply, Cmd0}},
                 %% The standard I/O is directed from/to the channel ChannelId.
                 exec_direct(ConnectionHandler, ChannelId, Cmd, F, WantReply, S1);
 
-            undefined when S0#state.shell == ?DEFAULT_SHELL ; 
-                           S0#state.shell == disabled ->
-                %% Exec called and the shell is the default shell (= Erlang shell).
-                %% To be exact, eval the term as an Erlang term (but not using the
-                %% ?DEFAULT_SHELL directly). This disables banner, prompts and such.
+            erlang_eval when S0#state.shell == disabled;
+                             S0#state.shell == {shell, start, []} ->
+                %% Exec called and the shell is the Erlang shell or disabled.
+                %% To be exact, eval the term as an Erlang term
+                %% This disables banner, prompts and such.
                 %% The standard I/O is directed from/to the channel ChannelId.
                 exec_in_erlang_default_shell(ConnectionHandler, ChannelId, Cmd, WantReply, S1);
 
-            undefined ->
-                %% Exec called, but the a shell other than the default shell is defined.
+            erlang_eval ->
+                %% Exec called, but the shell is custom (not Erlang shell).
                 %% No new exec shell is defined, so don't execute!
                 %% We don't know if it is intended to use the new shell or not.
                 {"Prohibited.", ?EXEC_ERROR_STATUS, 1};

lib/ssh/src/ssh_client_channel.erl

@@ -420,7 +420,11 @@ init([Options]) ->
 
 channel_cb_init_args(Options) ->
     case proplists:get_value(exec, Options) of
+        erlang_eval ->
+            %% erlang_eval is a mode flag for ssh_cli, not a cb init arg
+            proplists:get_value(init_args, Options);
         undefined ->
+            %% exec key absent from Options (client-side ssh:shell/1)
             proplists:get_value(init_args, Options);
         Exec ->
             proplists:get_value(init_args, Options) ++ [Exec]

lib/ssh/src/ssh_connection.erl

@@ -50,7 +50,6 @@ these messages are handled by
 -include("ssh.hrl").
 -include("ssh_connect.hrl").
 -include("ssh_transport.hrl").
-
 %% API
 -export([session_channel/2, session_channel/4,
 	 exec/4, shell/2, subsystem/4, send/3, send/4, send/5, 
@@ -1497,43 +1496,29 @@ start_cli(#connection{options = Options,
         no_cli ->
             {error, cli_disabled};
         {CbModule, Args} ->
-            ssh_connection_sup:start_channel(server, ConnectionSup, self(), CbModule, ChannelId, Args, Exec, Options)
+            ssh_connection_sup:start_channel(server, ConnectionSup, self(),
+                                             CbModule, ChannelId, Args, Exec, Options)
     end.
 
 
 start_subsystem(BinName, #connection{options = Options,
                                      connection_supervisor = ConnectionSup},
 	       #channel{local_id = ChannelId}, _ReplyMsg) ->
     Name = binary_to_list(BinName),
-    case check_subsystem(Name, Options) of
+    Subsystems = ?GET_OPT(subsystems, Options),
+    case proplists:get_value(Name, Subsystems, {none, []}) of
 	{Callback, Opts} when is_atom(Callback), Callback =/= none ->
-            ssh_connection_sup:start_channel(server, ConnectionSup, self(), Callback, ChannelId, Opts, undefined, Options);
+            %% Exec is not used by subsystems; undefined is filtered
+            %% out by channel_cb_init_args/1 so it won't be appended
+            %% to the callback's init args.
+            ssh_connection_sup:start_channel(server, ConnectionSup, self(),
+                                             Callback, ChannelId, Opts, undefined, Options);
         {none, _} ->
             {error, bad_subsystem};
 	{_, _} ->
 	    {error, legacy_option_not_supported}
     end.
 
-
-%%% Helpers for starting cli/subsystems
-check_subsystem("sftp"= SsName, Options) ->
-    case ?GET_OPT(subsystems, Options) of
-	no_subsys -> 	% FIXME: Can 'no_subsys' ever be matched?
-	    {SsName, {Cb, Opts}} = ssh_sftpd:subsystem_spec([]),
-	    {Cb, Opts};
-	SubSystems ->
-	    proplists:get_value(SsName, SubSystems, {none, []})
-    end;
-
-check_subsystem(SsName, Options) ->
-    Subsystems = ?GET_OPT(subsystems, Options),
-    case proplists:get_value(SsName, Subsystems, {none, []}) of
-	Fun when is_function(Fun) ->
-	    {Fun, []};
-	{_, _} = Value ->
-	    Value
-    end.
-
 %%%----------------------------------------------------------------
 %%%
 %%% Send-window handling

lib/ssh/src/ssh_options.erl

@@ -405,7 +405,7 @@ default(server) ->
     (default(common))
         #{
       subsystems =>
-          #{default => [ssh_sftpd:subsystem_spec([])],
+          #{default => [],
             chk => fun(L) ->
                            is_list(L) andalso
                                lists:all(fun(SubSystem = {Name,{CB,Args}}) ->
@@ -421,7 +421,7 @@ default(server) ->
            },
 
       shell =>
-          #{default => ?DEFAULT_SHELL,
+          #{default => disabled,
             chk => fun({M,F,A}) -> is_atom(M) andalso is_atom(F) andalso is_list(A);
                       (disabled) -> true;
                       (V) -> check_function1(V) orelse
@@ -431,9 +431,10 @@ default(server) ->
            },
 
       exec =>
-          #{default => undefined,
+          #{default => disabled,
             chk => fun({direct, V}) ->  check_function1(V) orelse check_function2(V) orelse check_function3(V);
                       (disabled) -> true;
+                      (erlang_eval) -> true; % Enable Erlang term evaluation
                       %% Compatibility (undocumented):
                       ({M,F,A}) -> is_atom(M) andalso is_atom(F) andalso is_list(A);
                       (V) -> check_function1(V) orelse check_function2(V) orelse check_function3(V)

lib/ssh/src/ssh_sftp.erl

@@ -172,6 +172,8 @@ start_channel(Cm, UserOptions0) when is_pid(Cm) ->
     PacketSize = proplists:get_value(packet_size, ChanOpts, ?XFER_PACKET_SIZE),
     case ssh_connection:session_channel(Cm, WindowSize, PacketSize, Timeout) of
 	{ok, ChannelId} ->
+            %% Exec is not used by SFTP channels; undefined is filtered
+            %% out by channel_cb_init_args/1.
             case ssh_connection_handler:start_channel(Cm, ?MODULE, ChannelId,
                                                       [Cm,ChannelId,SftpOpts], undefined) of
 		{ok, Pid} ->

lib/ssh/test/ssh_algorithms_SUITE.erl

@@ -231,7 +231,9 @@ init_per_group(Tag, Algs, Alg, PA, Config) ->
             end;
 
         _ ->
-            start_std_daemon([PrefAlgs],
+            start_std_daemon([{exec, erlang_eval},
+                              {subsystems, [ssh_sftpd:subsystem_spec([])]}
+                              | [PrefAlgs]],
                              [{pref_algs,PrefAlgs},
                               {tag_alg,{Tag,[Alg]}}
                               | Config])