File 0126-ssh-Add-better-error-handling-in-connect-2... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 0126-ssh-Add-better-error-handling-in-connect-2-3-4.patch of Package erlang (Revision 5b89568f64d9961d3b4b7c988ecff390)

Currently displaying revision 5b89568f64d9961d3b4b7c988ecff390 , Show latest

From b6d7593aca7834c995bb86921b2393fcbb4735c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cons=20T=20=C3=85hs?= <cons@erlang.org>
Date: Tue, 21 Sep 2021 08:13:52 +0200
Subject: [PATCH] [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"},
-- 
2.31.1

Places

File 0126-ssh-Add-better-error-handling-in-connect-2-3-4.patch of Package erlang (Revision 5b89568f64d9961d3b4b7c988ecff390)

Places