From b6d7593aca7834c995bb86921b2393fcbb4735c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cons=20T=20=C3=85hs?= Date: Tue, 21 Sep 2021 08:13:52 +0200 Subject: [ssh] Add better error handling in connect/2,3,4 Fix issues where incorrect arguments might result in a function_clause exception and potential information leakage in a stack trace. * Add tests with incorrectly typed arguments and verify that an informative error is returned rather than an exception. * Add error detection of arguments to connect/2,3,4. * Add better abstraction and collection of errors. --- lib/ssh/src/ssh.erl | 88 +++++++++++---- lib/ssh/test/ssh_connection_SUITE.erl | 195 +++++++++++++++++++++++++++++++--- 2 files changed, 248 insertions(+), 35 deletions(-) diff --git a/lib/ssh/src/ssh.erl b/lib/ssh/src/ssh.erl index bf354c7ef6..467e429755 100644 --- a/lib/ssh/src/ssh.erl +++ b/lib/ssh/src/ssh.erl @@ -1,7 +1,7 @@ % %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2004-2020. All Rights Reserved. +%% Copyright Ericsson AB 2004-2021. All Rights Reserved. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. @@ -126,25 +126,35 @@ stop() -> %%-------------------------------------------------------------------- %% Description: Starts an ssh connection. %%-------------------------------------------------------------------- --spec connect(OpenTcpSocket, Options) -> {ok,connection_ref()} | {error,term()} when +-define(IS_VALID_OPTIONS(Options), is_list(Options)). +-define(IS_VALID_PORT(Port), (is_integer(Port) andalso Port > 0)). +-define(IS_VALID_TIMEOUT(Timeout), + (Timeout == infinity + orelse (is_integer(Timeout) + andalso Timeout >= 0))). + +-spec connect(OpenTcpSocket, Options) + -> {ok, connection_ref()} + | {error, term()} when OpenTcpSocket :: open_socket(), Options :: client_options(). -connect(OpenTcpSocket, Options) when is_list(Options) -> - connect(OpenTcpSocket, Options, infinity). - +connect(OpenTcpSocket, Options) when ?IS_VALID_OPTIONS(Options) -> + connect(OpenTcpSocket, Options, infinity); +connect(_OpenTcpSocket, Options) -> + bad_arg([{options, Options}]). -spec connect(open_socket(), client_options(), timeout()) -> - {ok,connection_ref()} | {error,term()} + {ok, connection_ref()} | {error, term()} ; (host(), inet:port_number(), client_options()) -> - {ok,connection_ref()} | {error,term()}. + {ok, connection_ref()} | {error, term()}. -connect(Host, Port, Options) when is_integer(Port), - Port>0, - is_list(Options) -> +connect(Host, Port, Options) when ?IS_VALID_PORT(Port), + ?IS_VALID_OPTIONS(Options) -> connect(Host, Port, Options, infinity); - -connect(Socket, UserOptions, NegotiationTimeout) when is_list(UserOptions) -> +connect(Socket, UserOptions, NegotiationTimeout) + when ?IS_VALID_OPTIONS(UserOptions), + ?IS_VALID_TIMEOUT(NegotiationTimeout) -> case ssh_options:handle_options(client, UserOptions) of {error, Error} -> {error, Error}; @@ -156,26 +166,30 @@ connect(Socket, UserOptions, NegotiationTimeout) when is_list(UserOptions) -> {error,SockError} -> {error,SockError} end - end. - + end; +connect(_HostOrSocket, PortOrOptions, OptionsOrTimeout) -> + bad_arg(PortOrOptions, OptionsOrTimeout). --spec connect(Host, Port, Options, NegotiationTimeout) -> {ok,connection_ref()} | {error,term()} when +-spec connect(Host, Port, Options, NegotiationTimeout) + -> {ok, connection_ref()} + | {error, term()} when Host :: host(), Port :: inet:port_number(), Options :: client_options(), NegotiationTimeout :: timeout(). -connect(Host0, Port, UserOptions, NegotiationTimeout) when is_integer(Port), - Port>0, - is_list(UserOptions) -> +connect(Host0, Port, UserOptions, NegotiationTimeout) + when ?IS_VALID_PORT(Port), + ?IS_VALID_OPTIONS(UserOptions), + ?IS_VALID_TIMEOUT(NegotiationTimeout) -> case ssh_options:handle_options(client, UserOptions) of {error, Reason} -> {error, Reason}; - + Options -> SocketOpts = [{active,false} | ?GET_OPT(socket_options,Options)], Host = mangle_connect_address(Host0, Options), - try + try transport_connect(Host, Port, SocketOpts, Options) of {ok, Socket} -> @@ -188,8 +202,42 @@ connect(Host0, Port, UserOptions, NegotiationTimeout) when is_integer(Port), error:Error -> {error,Error}; Class:Error -> {error, {Class,Error}} end + end; +connect(_Host, Port, UserOptions, NegotiationTimeout) -> + bad_arg([{port, Port}, + {options, UserOptions}, + {timeout, NegotiationTimeout}]). + +bad_arg(Args) -> + hd(bad_args(Args)). + +%% Special handling for finding the incorrect args for connect/3, +%% which has two distinctly different signatures. +bad_arg(Arg2, Arg3) -> + E0 = bad_args([{port, Arg2}, {options, Arg3}]), + E1 = bad_args([{options, Arg2}, {timeout, Arg3}]), + %% Select the case with only one error + case {E0, E1} of + {[Error], _} -> Error; + {_, [Error]} -> Error; + {[Error, _], _} -> Error end. +%% Return list of errors +-spec bad_args([{'options' | 'port' | 'timeout', any()}]) -> + [{'error', term()}]. +bad_args(Args) -> + IsErr = fun(true, _) -> false; + (false, Error) -> {true, {error, Error}} + end, + Check = + fun({options, Arg}) -> IsErr(?IS_VALID_OPTIONS(Arg), invalid_options); + ({timeout, Arg}) -> IsErr(?IS_VALID_TIMEOUT(Arg), invalid_timeout); + ({port, Arg}) -> IsErr(?IS_VALID_PORT(Arg), invalid_port) + end, + + lists:filtermap(Check, Args). + %%%---------------- continue_connect(Socket, Options0, NegTimeout) -> {ok, {SockHost,SockPort}} = inet:sockname(Socket), diff --git a/lib/ssh/test/ssh_connection_SUITE.erl b/lib/ssh/test/ssh_connection_SUITE.erl index dbc0c45fff..cda08d47a9 100644 --- a/lib/ssh/test/ssh_connection_SUITE.erl +++ b/lib/ssh/test/ssh_connection_SUITE.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2008-2020. All Rights Reserved. +%% Copyright Ericsson AB 2008-2021. All Rights Reserved. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. @@ -43,6 +43,20 @@ big_cat/1, connect_sock_not_passive/1, connect_sock_not_tcp/1, + connect2_invalid_options/1, + connect_invalid_port/1, + connect_invalid_timeout_0/1, + connect_invalid_timeout_1/1, + connect_invalid_options/1, + connect3_invalid_port/1, + connect3_invalid_options/1, + connect3_invalid_timeout_0/1, + connect3_invalid_timeout_1/1, + connect3_invalid_both/1, + connect4_invalid_two_0/1, + connect4_invalid_two_1/1, + connect4_invalid_two_2/1, + connect4_invalid_three/1, daemon_sock_not_passive/1, daemon_sock_not_tcp/1, do_interrupted_send/3, @@ -132,6 +146,20 @@ all() -> start_shell_sock_daemon_exec_multi, encode_decode_pty_opts, connect_sock_not_tcp, + connect2_invalid_options, + connect_invalid_port, + connect_invalid_options, + connect_invalid_timeout_0, + connect_invalid_timeout_1, + connect3_invalid_port, + connect3_invalid_options, + connect3_invalid_timeout_0, + connect3_invalid_timeout_1, + connect3_invalid_both, + connect4_invalid_two_0, + connect4_invalid_two_1, + connect4_invalid_two_2, + connect4_invalid_three, daemon_sock_not_tcp, gracefull_invalid_version, gracefull_invalid_start, @@ -239,6 +267,139 @@ simple_exec_two_socks(_Config) -> {Pid2,ok} -> ok end. +%%-------------------------------------------------------------------- +connect2_invalid_options(_Config) -> + {error, invalid_options} = ssh:connect(bogus_socket, {bad, option}). + +connect3_invalid_port(_Config) -> + {error, invalid_port} = ssh:connect(bogus_host, noport, [{key, value}]). + +connect3_invalid_options(_Config) -> + {error, invalid_options} = ssh:connect(bogus_host, 1337, bad_options). + +connect3_invalid_timeout_0(_Config) -> + {error, invalid_timeout} = + ssh:connect(bogus_socket, [{key, value}], short). + +connect3_invalid_timeout_1(_Config) -> + {error, invalid_timeout} = + ssh:connect(bogus_socket, [{key, value}], -1). + +connect3_invalid_both(_Config) -> + %% The actual reason is implementation dependent. + {error, _Reason} = + ssh:connect(bogus, no_list_or_port, no_list_or_timeout). + +connect_invalid_port(Config) -> + {Pid, Host, _Port, UserDir} = daemon_start(Config), + + {error, invalid_port} = + ssh:connect(Host, undefined, + [{silently_accept_hosts, true}, + {user, "foo"}, + {password, "morot"}, + {user_interaction, false}, + {user_dir, UserDir}], + infinity), + + ssh:stop_daemon(Pid). + +connect_invalid_timeout_0(Config) -> + {Pid, Host, Port, UserDir} = daemon_start(Config), + + {error, invalid_timeout} = + ssh:connect(Host, Port, + [{silently_accept_hosts, true}, + {user, "foo"}, + {password, "morot"}, + {user_interaction, false}, + {user_dir, UserDir}], + longer), + + ssh:stop_daemon(Pid). + +connect_invalid_timeout_1(Config) -> + {Pid, Host, Port, UserDir} = daemon_start(Config), + + {error, invalid_timeout} = + ssh:connect(Host, Port, + [{silently_accept_hosts, true}, + {user, "foo"}, + {password, "morot"}, + {user_interaction, false}, + {user_dir, UserDir}], + -1), + + ssh:stop_daemon(Pid). + +connect_invalid_options(Config) -> + {Pid, Host, Port, _UserDir} = daemon_start(Config), + + {error, invalid_options} = + ssh:connect(Host, Port, + {user, "foo"}, + infinity), + + ssh:stop_daemon(Pid). + +%% Two out three arguments incorrect. Three possibilities. +connect4_invalid_two_0(Config) -> + {Pid, Host, _Port, _UserDir} = daemon_start(Config), + + %% Actual error implementation dependent + {error, _} = + ssh:connect(Host, noport, + {user, "foo"}, + infinity), + + ssh:stop_daemon(Pid). + +connect4_invalid_two_1(Config) -> + {Pid, Host, Port, _UserDir} = daemon_start(Config), + + %% Actual error implementation dependent + {error, _} = + ssh:connect(Host, Port, + {user, "foo"}, + short), + + ssh:stop_daemon(Pid). + +connect4_invalid_two_2(Config) -> + {Pid, Host, Port, _UserDir} = daemon_start(Config), + + %% Actual error implementation dependent + {error, _} = + ssh:connect(Host, newport, + [{user, "foo"}], + -1), + + ssh:stop_daemon(Pid). + +%% All three args incorrect +connect4_invalid_three(Config) -> + {Pid, Host, Port, _UserDir} = daemon_start(Config), + + %% Actual error implementation dependent + {error, _} = + ssh:connect(Host, teleport, + {user, "foo"}, + fortnight), + + ssh:stop_daemon(Pid). + +daemon_start(Config) -> + PrivDir = proplists:get_value(priv_dir, Config), + UserDir = filename:join(PrivDir, nopubkey), % to make sure we don't use public-key-auth + 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"}, + {exec, fun ssh_exec_echo/1}]), + {Pid, Host, Port, UserDir}. + %%-------------------------------------------------------------------- connect_sock_not_tcp(_Config) -> {ok,Sock} = gen_udp:open(0, []), @@ -1239,7 +1400,8 @@ kex_error(Config) -> stop_listener(Config) when is_list(Config) -> PrivDir = proplists:get_value(priv_dir, Config), - UserDir = filename:join(PrivDir, nopubkey), % to make sure we don't use public-key-auth + %% to make sure we don't use public-key-auth + UserDir = filename:join(PrivDir, nopubkey), file:make_dir(UserDir), SysDir = proplists:get_value(data_dir, Config), @@ -1248,13 +1410,14 @@ stop_listener(Config) when is_list(Config) -> {password, "morot"}, {exec, fun ssh_exec_echo/1}]), - ConnectionRef0 = ssh_test_lib:connect(Host, Port, [{silently_accept_hosts, true}, - {user, "foo"}, - {password, "morot"}, - {user_interaction, false}, - {user_dir, UserDir}]), - - {ok, ChannelId0} = ssh_connection:session_channel(ConnectionRef0, infinity), + ConnectionRef0 = ssh_test_lib:connect(Host, Port, + [{silently_accept_hosts, true}, + {user, "foo"}, + {password, "morot"}, + {user_interaction, false}, + {user_dir, UserDir}]), + {ok, ChannelId0} = + ssh_connection:session_channel(ConnectionRef0, infinity), ssh:stop_listener(Host, Port), @@ -1268,7 +1431,8 @@ stop_listener(Config) when is_list(Config) -> success = ssh_connection:exec(ConnectionRef0, ChannelId0, "testing", infinity), receive - {ssh_cm, ConnectionRef0, {data, ChannelId0, 0, <<"echo testing\n">>}} -> + {ssh_cm, ConnectionRef0, + {data, ChannelId0, 0, <<"echo testing\n">>}} -> ok after 5000 -> ct:fail("Exec Timeout") @@ -1279,11 +1443,12 @@ stop_listener(Config) when is_list(Config) -> {password, "potatis"}, {exec, fun ssh_exec_echo/1}]) of {Pid1, Host, Port} -> - ConnectionRef1 = ssh_test_lib:connect(Host, Port, [{silently_accept_hosts, true}, - {user, "foo"}, - {password, "potatis"}, - {user_interaction, true}, - {user_dir, UserDir}]), + ConnectionRef1 = + ssh_test_lib:connect(Host, Port, [{silently_accept_hosts, true}, + {user, "foo"}, + {password, "potatis"}, + {user_interaction, true}, + {user_dir, UserDir}]), {error, _} = ssh:connect(Host, Port, [{silently_accept_hosts, true}, {save_accepted_host, false}, {user, "foo"}, -- cgit v1.2.1