ssh: Change shell and exec defaults to disabled - #10733
Conversation
CT Test Results 3 files 92 suites 1h 36m 28s ⏱️ For more details on these failures, see this check. Results for commit 3dbfc21. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
8a3ec45 to
51d5259
Compare
Mikaka27
left a comment
There was a problem hiding this comment.
Minor formatting problems and one question.
51d5259 to
954d2cf
Compare
Change the shell and exec daemon options to default to disabled
instead of enabling the Erlang shell and Erlang term evaluation
respectively. This implements the "secure by default" principle,
preventing authenticated users from executing arbitrary Erlang code
unless explicitly configured.
The new erlang_eval exec option enables Erlang term evaluation via
exec requests, replacing the previous implicit behavior.
Applications requiring these services must now explicitly enable them:
%% Enable Erlang shell
ssh:daemon(Port, [{shell, {shell, start, []}} | Options])
%% Enable Erlang term evaluation via exec
ssh:daemon(Port, [{exec, erlang_eval} | Options])
Also fix a client-side crash in ssh:shell/1 where the missing exec
key in options caused channel_cb_init_args/1 to append undefined to
the callback init args. Both erlang_eval and undefined are now
filtered out, as neither is a callback init argument.
954d2cf to
dda3f20
Compare
ff8b09f to
b7ed446
Compare
The subsystems daemon option now defaults to an empty list instead
of enabling the SFTP subsystem via ssh_sftpd:subsystem_spec([]).
This extends the "secure by default" principle to also cover SFTP,
complementing the shell and exec default changes.
Previously, all authenticated SSH users had access to the entire
file system visible to the Erlang VM process through the default
SFTP subsystem, with no path restrictions.
Changes:
- Change subsystems default from [ssh_sftpd:subsystem_spec([])] to []
- Update subsystem_spec doc string in ssh.hrl
- Remove dead code in ssh_connection:check_subsystem/2
- Update test suites to explicitly enable SFTP where needed
- Update documentation (hardening.md, introduction.md, using_ssh.md,
ssh_app.md, terminology.md)
Applications requiring SFTP must now explicitly enable it:
ssh:daemon(Port, [{subsystems, [ssh_sftpd:subsystem_spec([])]}
| Options])
b7ed446 to
3dbfc21
Compare
Mikaka27
left a comment
There was a problem hiding this comment.
Besides the added comments change looks ok me. It would be great if the comment in the PR description was updated to reflect the current state of changes before/after this is merged.
| deduced_encoding, | ||
| group, | ||
| shell, | ||
| exec = erlang_eval, |
There was a problem hiding this comment.
Why have this value as default if exec is disabled by default?
| replace_options_enable_services(Config) when is_list(Config) -> | ||
| PrivDir = proplists:get_value(priv_dir, Config), | ||
| UserDir = filename:join(PrivDir, nopubkey), | ||
| file:make_dir(UserDir), | ||
| SysDir = proplists:get_value(data_dir, Config), | ||
| {Pid, Host, Port} = ssh_test_lib:daemon([{system_dir, SysDir}, | ||
| {user_dir, UserDir}, | ||
| {password, "morot"}]), | ||
| ConnOpts = [{silently_accept_hosts, true}, | ||
| {user, "foo"}, | ||
| {password, "morot"}, | ||
| {user_dir, UserDir}], | ||
|
|
||
| %% Verify all services disabled before replace | ||
| C1 = ssh_test_lib:connect(Host, Port, ConnOpts), | ||
| ?CT_LOG("Checking shell is disabled before replace", []), | ||
| test_shell_is_disabled(C1), | ||
| ?CT_LOG("Checking exec is disabled before replace", []), | ||
| test_exec_is_disabled(C1), | ||
| ?CT_LOG("Checking SFTP is unavailable before replace", []), | ||
| {error, _} = ssh_sftp:start_channel(C1), | ||
| ?CT_LOG("All services confirmed disabled", []), | ||
| ssh:close(C1), | ||
|
|
||
| %% Enable shell, exec and SFTP | ||
| ?CT_LOG("Replacing options: enabling shell, exec and SFTP", []), | ||
| {ok, Pid} = ssh:daemon_replace_options(Pid, | ||
| [{shell, {shell, start, []}}, | ||
| {exec, erlang_eval}, | ||
| {subsystems, [ssh_sftpd:subsystem_spec([])]}]), | ||
|
|
||
| %% Verify all services work after replace | ||
| C2 = ssh_test_lib:connect(Host, Port, ConnOpts), | ||
| ?CT_LOG("Checking shell is enabled after replace", []), | ||
| test_shell_is_enabled(C2), | ||
| ?CT_LOG("Checking exec is enabled after replace", []), | ||
| test_exec_is_enabled(C2), | ||
| ?CT_LOG("Checking SFTP is available after replace", []), | ||
| {ok, SftpPid} = ssh_sftp:start_channel(C2), | ||
| ssh_sftp:stop_channel(SftpPid), | ||
| ?CT_LOG("All services confirmed enabled after replace", []), | ||
|
|
||
| ssh:close(C2), | ||
| ssh:stop_daemon(Pid). | ||
|
|
There was a problem hiding this comment.
This test fails on my machine (line 2114) with following error:
ssh_connection_SUITE:2075 Got unexpected {#Ref<0.1007350121.354418689.14699>,
#{meta =>
#{error_logger =>
#{tag => info_report,
type => progress,
report_cb =>
fun supervisor:format_log/1},
line => 2323,
pid => <0.18567.0>,
time => 1775469388246684,
file => "supervisor.erl",
gl => <0.18564.0>,
domain => [otp,sasl],
logger_formatter =>
#{title => "PROGRESS REPORT"},
mfa =>
{supervisor,report_progress,3},
report_cb =>
fun supervisor:format_log/2},
msg =>
{report,
#{label => {supervisor,progress},
report =>
[{supervisor,
{local,sshd_sup}},
{started,
[{pid,<0.18568.0>},
{id,ssh_lsocket_sup},
{mfargs,
{ssh_lsocket_sup,
start_link,[]}},
{restart_type,permanent},
{significant,false},
{shutdown,infinity},
{child_type,
supervisor}]}]}},
level => info}}
Expect: {ssh_cm,<0.18575.0>,{data,0,'0|1',<<"Prohibited.">>}}
There was a problem hiding this comment.
should be fixed in other PR, once I push
|
close in favor of PR-10970 |
The shell and exec daemon options now default to disabled instead of enabling the Erlang shell and Erlang term evaluation respectively. This improves security by following the "secure by default" principle.
Previously, all authenticated SSH users could execute arbitrary Erlang code via the default shell or exec services. This violated the principle of least privilege and created unnecessary security exposure.
Changes:
Applications requiring shell or exec functionality must now explicitly enable them:
%% Enable Erlang shell
ssh:daemon(Port, [{shell, {shell, start, []}} | Options])
%% Enable Erlang term evaluation via exec
ssh:daemon(Port, [{exec, undefined} | Options])
SFTP and other subsystems are unaffected by this change.