Merge branch 'hans/ssh/auth_review/OTP-12787' into maint

Before this merge the ssh application was stateless after
the key-exchange phase.  To prevent the application to act
on messages received in the wrong state more states are
introduced.  They take care of service requests and later
authorization.

* hans/ssh/auth_review/OTP-12787:
  ssh: remove unused filed #ssh.kb_data
  ssh: new states for keyboard-interactive
  ssh: new state - service_request

3 files changed, 79 insertions, 66 deletions

diff --git a/lib/ssh/src/ssh.hrl b/lib/ssh/src/ssh.hrl
index 462c98f503..da64e4abf9 100644
--- a/lib/ssh/src/ssh.hrl
+++ b/lib/ssh/src/ssh.hrl
@@ -133,7 +133,6 @@
 	  userauth_supported_methods,       %  string() eg "keyboard-interactive,password"
 	  userauth_methods,                 %  list( string() )  eg ["keyboard-interactive", "password"]
 	  kb_tries_left = 0,                %  integer(), num tries left for "keyboard-interactive"
-	  kb_data,
 	  userauth_preference,
 	  available_host_keys,
 	  authenticated = false
diff --git a/lib/ssh/src/ssh_auth.erl b/lib/ssh/src/ssh_auth.erl
index a91b8c200e..726f52132f 100644
--- a/lib/ssh/src/ssh_auth.erl
+++ b/lib/ssh/src/ssh_auth.erl
@@ -153,7 +153,7 @@ userauth_request_msg(#ssh{userauth_methods = Methods,
 		not_ok ->
 		    userauth_request_msg(Ssh);
 		Result ->
-		    Result
+		    {Pref,Result}
 	    end;
 	false ->
 	    userauth_request_msg(Ssh)
@@ -299,8 +299,7 @@ handle_userauth_request(#ssh_msg_userauth_request{user = User,
 							>>
 						},
 	    {not_authorized, {User, undefined}, 
-	     ssh_transport:ssh_packet(Msg, Ssh#ssh{user = User,
-						   kb_data = Msg
+	     ssh_transport:ssh_packet(Msg, Ssh#ssh{user = User
 						  })}
     end;
 
@@ -313,6 +312,8 @@ handle_userauth_request(#ssh_msg_userauth_request{user = User,
        #ssh_msg_userauth_failure{authentications = Methods,
 				 partial_success = false}, Ssh)}.
 
+
+
 handle_userauth_info_request(
   #ssh_msg_userauth_info_request{name = Name,
 				 instruction = Instr,
@@ -330,36 +331,19 @@ handle_userauth_info_request(
 handle_userauth_info_response(#ssh_msg_userauth_info_response{num_responses = 1,
 							      data = <<?UINT32(Sz), Password:Sz/binary>>},
 			      #ssh{opts = Opts,
-				   kb_tries_left = KbTriesLeft0,
-				   kb_data = InfoMsg,
+				   kb_tries_left = KbTriesLeft,
 				   user = User,
 				   userauth_supported_methods = Methods} = Ssh) ->
-    KbTriesLeft = KbTriesLeft0 - 1,
     case check_password(User, unicode:characters_to_list(Password), Opts) of
 	true ->
 	    {authorized, User,
 	     ssh_transport:ssh_packet(#ssh_msg_userauth_success{}, Ssh)};
-	false when KbTriesLeft > 0 ->
-	    UserAuthInfoMsg = 
-		InfoMsg#ssh_msg_userauth_info_request{
-		  name = "",
-		  instruction = 
-		      lists:concat(
-			["Bad user or password, try again. ",
-			 integer_to_list(KbTriesLeft),
-			 " tries left."])
-		 },
-	    {not_authorized, {User, undefined}, 
-	     ssh_transport:ssh_packet(UserAuthInfoMsg,
-				      Ssh#ssh{kb_tries_left = KbTriesLeft})};
-	     
 	false ->
 	    {not_authorized, {User, {error,"Bad user or password"}}, 
 	     ssh_transport:ssh_packet(#ssh_msg_userauth_failure{
 					 authentications = Methods,
 					 partial_success = false}, 
-				      Ssh#ssh{kb_data = undefined,
-					      kb_tries_left = 0}
+				      Ssh#ssh{kb_tries_left = max(KbTriesLeft-1, 0)}
 				     )}
     end;
 
diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index fcd66b80c0..646f787874 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -49,7 +49,10 @@
 -export([hello/2, kexinit/2, key_exchange/2, 
 	 key_exchange_dh_gex_init/2, key_exchange_dh_gex_reply/2,
 	 new_keys/2,
-	 userauth/2, connected/2,
+	 service_request/2, connected/2,
+	 userauth/2,
+	 userauth_keyboard_interactive/2,
+	 userauth_keyboard_interactive_info_response/2,
 	 error/2]).
 
 -export([init/1, handle_event/3,
@@ -82,7 +85,12 @@
 	  recbuf
 	 }). 
 
--type state_name()           :: hello | kexinit | key_exchange | new_keys | userauth | connection.
+-type state_name()           :: hello | kexinit | key_exchange | key_exchange_dh_gex_init |
+				key_exchange_dh_gex_reply | new_keys | service_request | 
+				userauth | userauth_keyboard_interactive |
+				userauth_keyboard_interactive_info_response |
+				connection.
+
 -type gen_fsm_state_return() :: {next_state, state_name(), term()} |
 				{next_state, state_name(), term(), timeout()} |
 				{stop, term(), term()}.
@@ -474,28 +482,30 @@ new_keys(#ssh_msg_newkeys{} = Msg, #state{ssh_params = Ssh0} = State0) ->
     after_new_keys(next_packet(State0#state{ssh_params = Ssh})).
 
 %%--------------------------------------------------------------------
--spec userauth(#ssh_msg_service_request{} | #ssh_msg_service_accept{} |
-	       #ssh_msg_userauth_request{} | #ssh_msg_userauth_info_request{} |
-	       #ssh_msg_userauth_info_response{} | #ssh_msg_userauth_success{} |
-	       #ssh_msg_userauth_failure{} | #ssh_msg_userauth_banner{},
-	       #state{}) -> gen_fsm_state_return().
+-spec service_request(#ssh_msg_service_request{} | #ssh_msg_service_accept{},
+		      #state{}) -> gen_fsm_state_return().
 %%--------------------------------------------------------------------
-
-userauth(#ssh_msg_service_request{name = "ssh-userauth"} = Msg, 
+service_request(#ssh_msg_service_request{name = "ssh-userauth"} = Msg, 
 	 #state{ssh_params = #ssh{role = server, 
 				  session_id = SessionId} = Ssh0} = State) ->
     {ok, {Reply, Ssh}} = ssh_auth:handle_userauth_request(Msg, SessionId, Ssh0),
     send_msg(Reply, State),
     {next_state, userauth, next_packet(State#state{ssh_params = Ssh})};
 
-userauth(#ssh_msg_service_accept{name = "ssh-userauth"},  
-	 #state{ssh_params = #ssh{role = client,
-				  service = "ssh-userauth"} = Ssh0} = 
-	 State) ->
+service_request(#ssh_msg_service_accept{name = "ssh-userauth"},  
+		#state{ssh_params = #ssh{role = client,
+					 service = "ssh-userauth"} = Ssh0} = 
+		    State) ->
     {Msg, Ssh} = ssh_auth:init_userauth_request_msg(Ssh0),
     send_msg(Msg, State),
-    {next_state, userauth, next_packet(State#state{auth_user = Ssh#ssh.user, ssh_params = Ssh})};
+    {next_state, userauth, next_packet(State#state{auth_user = Ssh#ssh.user, ssh_params = Ssh})}.
 
+%%--------------------------------------------------------------------
+-spec userauth(#ssh_msg_userauth_request{} | #ssh_msg_userauth_info_request{} |
+	       #ssh_msg_userauth_info_response{} | #ssh_msg_userauth_success{} |
+	       #ssh_msg_userauth_failure{} | #ssh_msg_userauth_banner{},
+	       #state{}) -> gen_fsm_state_return().
+%%--------------------------------------------------------------------
 userauth(#ssh_msg_userauth_request{service = "ssh-connection",
                                   method = "none"} = Msg, 
         #state{ssh_params = #ssh{session_id = SessionId, role = server, 
@@ -521,6 +531,10 @@ userauth(#ssh_msg_userauth_request{service = "ssh-connection",
 		    connected_fun(User, Address, Method, Opts),
 		    {next_state, connected, 
 		     next_packet(State#state{auth_user = User, ssh_params = Ssh})};
+		{not_authorized, {User, Reason}, {Reply, Ssh}} when Method == "keyboard-interactive" ->
+		    retry_fun(User, Address, Reason, Opts),
+		    send_msg(Reply, State),
+		    {next_state, userauth_keyboard_interactive, next_packet(State#state{ssh_params = Ssh})};
 		{not_authorized, {User, Reason}, {Reply, Ssh}} ->
 		    retry_fun(User, Address, Reason, Opts),
 		    send_msg(Reply, State),
@@ -530,30 +544,6 @@ userauth(#ssh_msg_userauth_request{service = "ssh-connection",
 	    userauth(Msg#ssh_msg_userauth_request{method="none"}, State)
     end;
 
-userauth(#ssh_msg_userauth_info_request{} = Msg, 
-	 #state{ssh_params = #ssh{role = client, 
-				  io_cb = IoCb} = Ssh0} = State) ->
-    {ok, {Reply, Ssh}} = ssh_auth:handle_userauth_info_request(Msg, IoCb, Ssh0),
-    send_msg(Reply, State),
-    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})};
-
-userauth(#ssh_msg_userauth_info_response{} = Msg, 
-	 #state{ssh_params = #ssh{role = server,
-				  peer = {_, Address}} = Ssh0,
-		opts = Opts, starter = Pid} = State) ->
-    case ssh_auth:handle_userauth_info_response(Msg, Ssh0) of
-	{authorized, User, {Reply, Ssh}} ->
-	    send_msg(Reply, State),
-	    Pid ! ssh_connected,
-	    connected_fun(User, Address, "keyboard-interactive", Opts),
-	    {next_state, connected, 
-	     next_packet(State#state{auth_user = User, ssh_params = Ssh})};
-	{not_authorized, {User, Reason}, {Reply, Ssh}} ->
-	    retry_fun(User, Address, Reason, Opts),
-	    send_msg(Reply, State),
-	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})} 
-    end;
-			
 userauth(#ssh_msg_userauth_success{}, #state{ssh_params = #ssh{role = client} = Ssh,
 					     starter = Pid} = State) ->
     Pid ! ssh_connected,
@@ -580,19 +570,25 @@ userauth(#ssh_msg_userauth_failure{authentications = Methodes},
 	{disconnect, DisconnectMsg, {Msg, Ssh}} ->
 	    send_msg(Msg, State),
 	    handle_disconnect(DisconnectMsg, State#state{ssh_params = Ssh});
-	{Msg, Ssh} ->
+	{"keyboard-interactive", {Msg, Ssh}} ->
+	    send_msg(Msg, State),
+	    {next_state, userauth_keyboard_interactive, next_packet(State#state{ssh_params = Ssh})};
+	{_Method, {Msg, Ssh}} ->
 	    send_msg(Msg, State),
 	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})}
     end;
 
 %% The prefered authentication method failed try next method 
-userauth(#ssh_msg_userauth_failure{},  
+userauth(#ssh_msg_userauth_failure{},
 	 #state{ssh_params = #ssh{role = client} = Ssh0} = State) ->
     case ssh_auth:userauth_request_msg(Ssh0) of
 	{disconnect,  DisconnectMsg,{Msg, Ssh}} ->
 	    send_msg(Msg, State),
 	    handle_disconnect(DisconnectMsg, State#state{ssh_params = Ssh}); 
-	{Msg, Ssh} ->	  
+	{"keyboard-interactive", {Msg, Ssh}} ->
+	    send_msg(Msg, State),
+	    {next_state, userauth_keyboard_interactive, next_packet(State#state{ssh_params = Ssh})};
+	{_Method, {Msg, Ssh}} ->
 	    send_msg(Msg, State),
 	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})}
     end;
@@ -607,6 +603,40 @@ userauth(#ssh_msg_userauth_banner{message = Msg},
     io:format("~s", [Msg]),
     {next_state, userauth, next_packet(State)}.
 
+
+
+userauth_keyboard_interactive(#ssh_msg_userauth_info_request{} = Msg, 
+			      #state{ssh_params = #ssh{role = client, 
+						       io_cb = IoCb} = Ssh0} = State) ->
+    {ok, {Reply, Ssh}} = ssh_auth:handle_userauth_info_request(Msg, IoCb, Ssh0),
+    send_msg(Reply, State),
+    {next_state, userauth_keyboard_interactive_info_response, next_packet(State#state{ssh_params = Ssh})};
+
+userauth_keyboard_interactive(#ssh_msg_userauth_info_response{} = Msg, 
+			      #state{ssh_params = #ssh{role = server,
+						       peer = {_, Address}} = Ssh0,
+				     opts = Opts, starter = Pid} = State) ->
+    case ssh_auth:handle_userauth_info_response(Msg, Ssh0) of
+	{authorized, User, {Reply, Ssh}} ->
+	    send_msg(Reply, State),
+	    Pid ! ssh_connected,
+	    connected_fun(User, Address, "keyboard-interactive", Opts),
+	    {next_state, connected, 
+	     next_packet(State#state{auth_user = User, ssh_params = Ssh})};
+	{not_authorized, {User, Reason}, {Reply, Ssh}} ->
+	    retry_fun(User, Address, Reason, Opts),
+	    send_msg(Reply, State),
+	    {next_state, userauth, next_packet(State#state{ssh_params = Ssh})} 
+    end.
+			
+
+
+userauth_keyboard_interactive_info_response(Msg=#ssh_msg_userauth_failure{}, State) ->
+    userauth(Msg, State);
+
+userauth_keyboard_interactive_info_response(Msg=#ssh_msg_userauth_success{}, State) ->
+    userauth(Msg, State).
+
 %%--------------------------------------------------------------------
 -spec connected({#ssh_msg_kexinit{}, binary()}, %%| %% #ssh_msg_kexdh_init{},
 		 #state{}) -> gen_fsm_state_return().
@@ -1563,10 +1593,10 @@ after_new_keys(#state{renegotiate = false,
 		      ssh_params = #ssh{role = client} = Ssh0} = State) ->
     {Msg, Ssh} = ssh_auth:service_request_msg(Ssh0),
     send_msg(Msg, State),
-    {next_state, userauth, State#state{ssh_params = Ssh}};
+    {next_state, service_request, State#state{ssh_params = Ssh}};
 after_new_keys(#state{renegotiate = false,  
 		      ssh_params = #ssh{role = server}} = State) ->
-    {next_state, userauth, State}.
+    {next_state, service_request, State}.
 
 after_new_keys_events({sync, _Event, From}, {stop, _Reason, _StateData}=Terminator) ->
     gen_fsm:reply(From, {error, closed}),