Merge branch 'ia/ssh/error-handling' into maint

* ia/ssh/error-handling:
  Prevent client hanging. (OTP-8111)
  Added checks of API input
  Improve check so that we will not try to read ssh packet length indicator if not sure we have enough data.
  Improved error handling

1 files changed, 148 insertions, 105 deletions

diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index 9079089d5d..5b3d1b8a1b 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -41,7 +41,7 @@
 -export([hello/2, kexinit/2, key_exchange/2, new_keys/2,
 	 userauth/2, connected/2]).
 
--export([init/1, state_name/3, handle_event/3,
+-export([init/1, handle_event/3,
 	 handle_sync_event/4, handle_info/3, terminate/3, code_change/4]).
 
 %% spawn export
@@ -106,22 +106,28 @@ peer_address(ConnectionHandler) ->
 %% initialize. 
 %%--------------------------------------------------------------------
 init([Role, Manager, Socket, SshOpts]) ->
+    process_flag(trap_exit, true),
     {NumVsn, StrVsn} = ssh_transport:versions(Role, SshOpts),
     ssh_bits:install_messages(ssh_transport:transport_messages(NumVsn)),
     {Protocol, Callback, CloseTag} = 
 	proplists:get_value(transport, SshOpts, {tcp, gen_tcp, tcp_closed}),
-    Ssh = init_ssh(Role, NumVsn, StrVsn, SshOpts, Socket),
-    {ok, hello, #state{ssh_params = 
-		       Ssh#ssh{send_sequence = 0, recv_sequence = 0}, 
-		       socket = Socket,
-		       decoded_data_buffer = <<>>,
-		       encoded_data_buffer = <<>>,
-		       transport_protocol = Protocol, 
-		       transport_cb = Callback,
-		       transport_close_tag = CloseTag,
-		       manager = Manager,
-		       opts = SshOpts
-		      }}.
+    try init_ssh(Role, NumVsn, StrVsn, SshOpts, Socket) of
+	Ssh ->
+	    {ok, hello, #state{ssh_params =
+				   Ssh#ssh{send_sequence = 0, recv_sequence = 0},
+			       socket = Socket,
+			       decoded_data_buffer = <<>>,
+			       encoded_data_buffer = <<>>,
+			       transport_protocol = Protocol,
+			       transport_cb = Callback,
+			       transport_close_tag = CloseTag,
+			       manager = Manager,
+			       opts = SshOpts
+			      }}
+    catch
+	exit:Reason ->
+	    {stop, {shutdown, Reason}}
+    end.
 %%--------------------------------------------------------------------
 %% Function: 
 %% state_name(Event, State) -> {next_state, NextStateName, NextState}|
@@ -179,7 +185,12 @@ kexinit({#ssh_msg_kexinit{} = Kex, Payload},
 	     next_packet(State#state{ssh_params = Ssh})}
     catch
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+						  description = Desc,
+						  language = "en"}, State)
     end.
     
 key_exchange(#ssh_msg_kexdh_init{} = Msg, 
@@ -192,7 +203,12 @@ key_exchange(#ssh_msg_kexdh_init{} = Msg,
 	    {next_state, new_keys, next_packet(State#state{ssh_params = Ssh})}
     catch
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	      handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+						  description = Desc,
+						  language = "en"}, State)
     end;
   
 key_exchange(#ssh_msg_kexdh_reply{} = Msg, 
@@ -203,7 +219,12 @@ key_exchange(#ssh_msg_kexdh_reply{} = Msg,
 	    {next_state, new_keys, next_packet(State#state{ssh_params = Ssh})}
     catch
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 
 key_exchange(#ssh_msg_kex_dh_gex_group{} = Msg, 
@@ -216,7 +237,12 @@ key_exchange(#ssh_msg_kex_dh_gex_group{} = Msg,
 	    {next_state, new_keys, next_packet(State#state{ssh_params = Ssh})}
     catch
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 
 key_exchange(#ssh_msg_kex_dh_gex_request{} = Msg, 
@@ -227,7 +253,12 @@ key_exchange(#ssh_msg_kex_dh_gex_request{} = Msg,
 	    {next_state, new_keys, next_packet(State#state{ssh_params = Ssh})}
     catch
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 key_exchange(#ssh_msg_kex_dh_gex_reply{} = Msg, 
 	     #state{ssh_params = #ssh{role = client} = Ssh0} = State) ->
@@ -237,7 +268,12 @@ key_exchange(#ssh_msg_kex_dh_gex_reply{} = Msg,
 	    {next_state, new_keys, next_packet(State#state{ssh_params = Ssh})}
     catch
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+						  description = Desc,
+						  language = "en"}, State)
     end.
 
 new_keys(#ssh_msg_newkeys{} = Msg, #state{ssh_params = Ssh0} = State0) ->
@@ -248,8 +284,12 @@ new_keys(#ssh_msg_newkeys{} = Msg, #state{ssh_params = Ssh0} = State0) ->
 	    {next_state, NextStateName, next_packet(State)}
     catch
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State0),
-	    {stop, normal, State0}
+	    handle_disconnect(DisconnectMsg, State0);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+						  description = Desc,
+						  language = "en"}, State0)
     end.
 
 userauth(#ssh_msg_service_request{name = "ssh-userauth"} = Msg, 
@@ -262,7 +302,12 @@ userauth(#ssh_msg_service_request{name = "ssh-userauth"} = Msg,
 	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})}
     catch 	
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_SERVICE_NOT_AVAILABLE,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 
 userauth(#ssh_msg_service_accept{name = "ssh-userauth"},  
@@ -284,7 +329,12 @@ userauth(#ssh_msg_userauth_request{service = "ssh-connection",
            {next_state, userauth, next_packet(State#state{ssh_params = Ssh})}
     catch 
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_SERVICE_NOT_AVAILABLE,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 
 userauth(#ssh_msg_userauth_request{service = "ssh-connection",
@@ -307,7 +357,12 @@ userauth(#ssh_msg_userauth_request{service = "ssh-connection",
 	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})} 
     catch 	
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	  handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_SERVICE_NOT_AVAILABLE,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 
 userauth(#ssh_msg_userauth_info_request{} = Msg, 
@@ -319,7 +374,12 @@ userauth(#ssh_msg_userauth_info_request{} = Msg,
 	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})}
     catch 	
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_SERVICE_NOT_AVAILABLE,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 
 userauth(#ssh_msg_userauth_info_response{} = Msg, 
@@ -330,7 +390,12 @@ userauth(#ssh_msg_userauth_info_response{} = Msg,
 	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})}
     catch 	
 	#ssh_msg_disconnect{} = DisconnectMsg ->
-	    handle_disconnect(DisconnectMsg, State)
+	    handle_disconnect(DisconnectMsg, State);
+	_:Error ->
+	    Desc = log_error(Error),
+	    handle_disconnect(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_SERVICE_NOT_AVAILABLE,
+						  description = Desc,
+						  language = "en"}, State)
     end;
 			
 userauth(#ssh_msg_userauth_success{}, #state{ssh_params = #ssh{role = client},
@@ -361,24 +426,11 @@ userauth(#ssh_msg_userauth_failure{authentications = Methodes},
 
 %% The prefered authentication method failed try next method 
 userauth(#ssh_msg_userauth_failure{},  
-	 #state{ssh_params = #ssh{role = client} = Ssh0,
-		manager = Pid} = State) ->
+	 #state{ssh_params = #ssh{role = client} = Ssh0} = State) ->
     case ssh_auth:userauth_request_msg(Ssh0) of
-	{disconnect, Event, {Msg, _}} ->
-	    try 
-		send_msg(Msg, State),
-		ssh_connection_manager:event(Pid, Event)
-	    catch
-		exit:{noproc, _Reason} ->
-		    Report = io_lib:format("Connection Manager terminated: ~p~n",
-					   [Pid]),
-		    error_logger:info_report(Report);
-		  exit:Exit ->
-		    Report = io_lib:format("Connection Manager returned:~n~p~n~p~n",
-					   [Msg, Exit]),
-		    error_logger:info_report(Report)
-	    end,
-	    {stop, normal, State};
+	{disconnect,  DisconnectMsg,{Msg, Ssh}} ->
+	    send_msg(Msg, State),
+	    handle_disconnect(DisconnectMsg, State#state{ssh_params = Ssh}); 
 	{Msg, Ssh} ->	  
 	    send_msg(Msg, State),
 	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})}
@@ -398,25 +450,6 @@ connected({#ssh_msg_kexinit{}, _Payload} = Event, State) ->
     kexinit(Event, State#state{renegotiate = true}).
 
 %%--------------------------------------------------------------------
-%% Function:
-%% state_name(Event, From, State) -> {next_state, NextStateName, NextState} |
-%%                                   {next_state, NextStateName, 
-%%                                     NextState, Timeout} |
-%%                                   {reply, Reply, NextStateName, NextState}|
-%%                                   {reply, Reply, NextStateName, 
-%%                                    NextState, Timeout} |
-%%                                   {stop, Reason, NewState}|
-%%                                   {stop, Reason, Reply, NewState}
-%% Description: There should be one instance of this function for each
-%% possible state name. Whenever a gen_fsm receives an event sent using
-%% gen_fsm:sync_send_event/2,3, the instance of this function with the same
-%% name as the current state name StateName is called to handle the event.
-%%--------------------------------------------------------------------
-state_name(_Event, _From, State) ->
-    Reply = ok,
-    {reply, Reply, state_name, State}.
-
-%%--------------------------------------------------------------------
 %% Function: 
 %% handle_event(Event, StateName, State) -> {next_state, NextStateName, 
 %%						  NextState} |
@@ -566,10 +599,18 @@ handle_info({Protocol, Socket, Data}, Statename,
 			   Statename, State);
 
 handle_info({CloseTag, _Socket}, _StateName, 
-	    #state{transport_close_tag = CloseTag, %%manager = Pid,
+	    #state{transport_close_tag = CloseTag,
 		   ssh_params = #ssh{role = _Role, opts = _Opts}} = State) ->
-    %%ok = ssh_connection_manager:delivered(Pid),
-    {stop, normal, State};
+    DisconnectMsg =
+	#ssh_msg_disconnect{code = ?SSH_DISCONNECT_CONNECTION_LOST,
+			    description = "Connection Lost",
+			    language = "en"},
+    {stop, {shutdown, DisconnectMsg}, State};
+
+%%% So that terminate will be run when supervisor is shutdown
+handle_info({'EXIT', _Sup, Reason}, _StateName, State) ->
+    {stop, Reason, State};
+
 handle_info(UnexpectedMessage, StateName, #state{ssh_params = SshParams} = State) ->
     Msg = lists:flatten(io_lib:format(
            "Unexpected message '~p' received in state '~p'\n"
@@ -581,7 +622,6 @@ handle_info(UnexpectedMessage, StateName, #state{ssh_params = SshParams} = State
     error_logger:info_report(Msg),
     {next_state, StateName, State}.
 
-
 %%--------------------------------------------------------------------
 %% Function: terminate(Reason, StateName, State) -> void()
 %% Description:This function is called by a gen_fsm when it is about
@@ -596,22 +636,31 @@ terminate(normal, _, #state{transport_cb = Transport,
     (catch Transport:close(Socket)),
     ok;
 
-terminate(shutdown, _, State) ->
+%% Terminated as manager terminated
+terminate(shutdown, StateName, #state{ssh_params = Ssh0} = State) ->
     DisconnectMsg = 
 	#ssh_msg_disconnect{code = ?SSH_DISCONNECT_BY_APPLICATION,
-			    description = "Application disconnect",
+			    description = "Application shutdown",
 			    language = "en"},
-    handle_disconnect(DisconnectMsg, State);
+    {SshPacket, Ssh} = ssh_transport:ssh_packet(DisconnectMsg, Ssh0),
+    send_msg(SshPacket, State),
+    terminate(normal, StateName, State#state{ssh_params = Ssh});
 
-terminate(Reason, _, State) ->
-    Desc = io_lib:format("Erlang ssh connection handler failed with reason: "
-			 "~p , please report this to [email protected] \n", 
-			 [Reason]),
+terminate({shutdown, #ssh_msg_disconnect{} = Msg}, StateName, #state{ssh_params = Ssh0, manager = Pid} = State) ->
+    {SshPacket, Ssh} = ssh_transport:ssh_packet(Msg, Ssh0),
+    send_msg(SshPacket, State),
+    ssh_connection_manager:event(Pid, Msg),
+    terminate(normal, StateName, State#state{ssh_params = Ssh});
+terminate(Reason, StateName, #state{ssh_params = Ssh0, manager = Pid} = State) ->
+    log_error(Reason),
     DisconnectMsg = 
-	#ssh_msg_disconnect{code = ?SSH_DISCONNECT_CONNECTION_LOST,
-			    description = Desc,
+	#ssh_msg_disconnect{code = ?SSH_DISCONNECT_BY_APPLICATION,
+			    description = "Internal error",
 			    language = "en"},
-    handle_disconnect(DisconnectMsg, State).
+    {SshPacket, Ssh} = ssh_transport:ssh_packet(DisconnectMsg, Ssh0),
+    ssh_connection_manager:event(Pid, DisconnectMsg),
+    send_msg(SshPacket, State),
+    terminate(normal, StateName, State#state{ssh_params = Ssh}).
 
 %%--------------------------------------------------------------------
 %% Function:
@@ -757,11 +806,8 @@ generate_event(<<?BYTE(Byte), _/binary>> = Msg, StateName,
 	next_packet(State),
 	{next_state, StateName, State}
     catch
-	exit:{noproc, _Reason} ->
-	    Report = io_lib:format("~p Connection Handler terminated: ~p~n",
-				   [self(), Pid]),
-	    error_logger:info_report(Report),
-	    {stop, normal, State0}
+	exit:{noproc, Reason} ->
+	    {stop, {shutdown, Reason}, State0}
     end;
 generate_event(Msg, StateName, State0, EncData) ->
     Event = ssh_bits:decode(Msg),
@@ -786,13 +832,18 @@ generate_event_new_state(#state{ssh_params =
 
 next_packet(#state{decoded_data_buffer = <<>>,
 		   encoded_data_buffer = Buff,
+		   ssh_params = #ssh{decrypt_block_size = BlockSize},
 		   socket = Socket,
-		   transport_protocol = Protocol} = 
-	    State) when Buff =/= <<>> andalso size(Buff) >= 8 ->
-    %% More data from the next packet has been received
-    %% Fake a socket-recive message so that the data will be processed
-    inet:setopts(Socket, [{active, once}]),
-    self() ! {Protocol, Socket, <<>>},
+		   transport_protocol = Protocol} = State) when Buff =/= <<>> ->
+    case  size(Buff) >= erlang:max(8, BlockSize) of
+	true ->
+	    %% Enough data from the next packet has been received to
+	    %% decode the length indicator, fake a socket-recive
+	    %% message so that the data will be processed
+	    self() ! {Protocol, Socket, <<>>};
+	false ->
+	    inet:setopts(Socket, [{active, once}])
+    end,
     State;
 
 next_packet(#state{socket = Socket} = State) ->
@@ -853,24 +904,8 @@ handle_ssh_packet(Length, StateName, #state{decoded_data_buffer = DecData0,
 	    handle_disconnect(DisconnectMsg, State0)
     end.
 
-handle_disconnect(#ssh_msg_disconnect{} = Msg, 
-		  #state{ssh_params = Ssh0, manager = Pid} = State) ->
-    {SshPacket, Ssh} = ssh_transport:ssh_packet(Msg, Ssh0),
-    try 
-	send_msg(SshPacket, State),
- 	ssh_connection_manager:event(Pid, Msg)
-    catch
-	exit:{noproc, _Reason} ->
-	    Report = io_lib:format("~p Connection Manager terminated: ~p~n",
-				   [self(), Pid]),
-	    error_logger:info_report(Report);
-	exit:Exit ->
-	    Report = io_lib:format("Connection Manager returned:~n~p~n~p~n",
-				   [Msg, Exit]),
-	    error_logger:info_report(Report)
-    end,
-    (catch ssh_userreg:delete_user(Pid)),
-    {stop, normal, State#state{ssh_params = Ssh}}.
+handle_disconnect(#ssh_msg_disconnect{} = Msg, State) ->
+    {stop, {shutdown, Msg}, State}.
 
 counterpart_versions(NumVsn, StrVsn, #ssh{role = server} = Ssh) ->
     Ssh#ssh{c_vsn = NumVsn , c_version = StrVsn};
@@ -928,3 +963,11 @@ ssh_info([peer | Rest], #ssh{peer = Peer} = SshParams, Acc) ->
 
 ssh_info([ _ | Rest], SshParams, Acc) ->
     ssh_info(Rest, SshParams, Acc).
+
+log_error(Reason) ->
+    Report = io_lib:format("Erlang ssh connection handler failed with reason: "
+			   "~p ~n, Stacktace: ~p ~n"
+			   "please report this to [email protected] \n",
+			   [Reason,  erlang:get_stacktrace()]),
+    error_logger:error_report(Report),
+    "Internal error".