Fix protocol violations during rekeying

In RFC 4253, sections 7.1 & 9 describe rekeying with
special attention to the protocol messages that may be
received and may not be sent during rekeying.
This patch fixes a number of problems during rekeying
caused by data & requests received from the network, and/or data & requests sent by the user.

1 files changed, 146 insertions, 91 deletions

diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index 9b11cadab6..65208ae158 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -71,6 +71,7 @@
 	  key_exchange_init_msg,   %  #ssh_msg_kexinit{}
 	  renegotiate = false,     %  boolean() 
 	  last_size_rekey = 0,
+	  event_queue = [],
 	  connection_queue,
 	  address,
 	  port,
@@ -83,6 +84,11 @@
 				{next_state, state_name(), term(), timeout()} |
 				{stop, term(), term()}.
 
+-type gen_fsm_sync_return() :: {next_state, state_name(), term()} |
+			       {next_state, state_name(), term(), timeout()} |
+			       {reply, term(), state_name(), term()} |
+			       {stop, term(), term(), term()}.
+
 %%====================================================================
 %% Internal application API
 %%====================================================================
@@ -433,9 +439,7 @@ key_exchange(#ssh_msg_kex_dh_gex_reply{} = Msg,
 
 new_keys(#ssh_msg_newkeys{} = Msg, #state{ssh_params = Ssh0} = State0) ->
     {ok, Ssh} = ssh_transport:handle_new_keys(Msg, Ssh0),
-    {NextStateName, State} =
-	after_new_keys(State0#state{ssh_params = Ssh}),
-    {next_state, NextStateName, next_packet(State)}.
+    after_new_keys(next_packet(State0#state{ssh_params = Ssh})).
 
 %%--------------------------------------------------------------------
 -spec userauth(#ssh_msg_service_request{} | #ssh_msg_service_accept{} |
@@ -594,33 +598,6 @@ handle_event(#ssh_msg_debug{always_display = Display, message = DbgMsg, language
 handle_event(#ssh_msg_unimplemented{}, StateName, State) ->
     {next_state, StateName, next_packet(State)};
 
-handle_event({adjust_window, ChannelId, Bytes}, StateName,
-	     #state{connection_state =
-			#connection{channel_cache = Cache}} = State0) ->
-    State =
-	case ssh_channel:cache_lookup(Cache, ChannelId) of
-	    #channel{recv_window_size = WinSize, remote_id = Id} = Channel ->
-		ssh_channel:cache_update(Cache, Channel#channel{recv_window_size =
-					       WinSize + Bytes}),
-		Msg = ssh_connection:channel_adjust_window_msg(Id, Bytes),
-		send_replies([{connection_reply, Msg}], State0);
-	undefined ->
-	    State0
-	end,
-    {next_state, StateName, next_packet(State)};
-
-handle_event({reply_request, success, ChannelId}, StateName,
-		  #state{connection_state =
-			     #connection{channel_cache = Cache}} = State0) ->
-    State = case ssh_channel:cache_lookup(Cache, ChannelId) of
-		#channel{remote_id = RemoteId} ->
-		    Msg = ssh_connection:channel_success_msg(RemoteId),
-		    send_replies([{connection_reply, Msg}], State0);
-		undefined ->
-		    State0
-	    end,
-    {next_state, StateName, State};
-
 handle_event(renegotiate, connected, #state{ssh_params = Ssh0} 
 	     = State) ->
     {KeyInitMsg, SshPacket, Ssh} = ssh_transport:key_exchange_init_msg(Ssh0),
@@ -632,8 +609,7 @@ handle_event(renegotiate, connected, #state{ssh_params = Ssh0}
 			     renegotiate = true})};
 
 handle_event(renegotiate, StateName, State) ->
-    timer:apply_after(?REKEY_TIMOUT, gen_fsm, send_all_state_event, [self(), renegotiate]),
-    %% Allready in keyexcahange so ignore
+    %% Already in key-exchange so safe to ignore
     {next_state, StateName, State};
 
 %% Rekey due to sent data limit reached?
@@ -655,6 +631,38 @@ handle_event(data_size, connected, #state{ssh_params = Ssh0} = State) ->
 	    {next_state, connected, next_packet(State)}
     end;
 handle_event(data_size, StateName, State) ->
+    %% Already in key-exchange so safe to ignore
+    {next_state, StateName, State};
+
+handle_event(Event, StateName, State) when StateName /= connected ->
+    Events = [{event, Event} | State#state.event_queue],
+    {next_state, StateName, State#state{event_queue = Events}};
+
+handle_event({adjust_window, ChannelId, Bytes}, StateName,
+	     #state{connection_state =
+			#connection{channel_cache = Cache}} = State0) ->
+    State =
+	case ssh_channel:cache_lookup(Cache, ChannelId) of
+	    #channel{recv_window_size = WinSize, remote_id = Id} = Channel ->
+		ssh_channel:cache_update(Cache, Channel#channel{recv_window_size =
+					       WinSize + Bytes}),
+		Msg = ssh_connection:channel_adjust_window_msg(Id, Bytes),
+		send_replies([{connection_reply, Msg}], State0);
+	undefined ->
+	    State0
+	end,
+    {next_state, StateName, next_packet(State)};
+
+handle_event({reply_request, success, ChannelId}, StateName,
+		  #state{connection_state =
+			     #connection{channel_cache = Cache}} = State0) ->
+    State = case ssh_channel:cache_lookup(Cache, ChannelId) of
+		#channel{remote_id = RemoteId} ->
+		    Msg = ssh_connection:channel_success_msg(RemoteId),
+		    send_replies([{connection_reply, Msg}], State0);
+		undefined ->
+		    State0
+	    end,
     {next_state, StateName, State};
 
 handle_event({request, ChannelPid, ChannelId, Type, Data}, StateName, State0) ->
@@ -685,8 +693,65 @@ handle_event({unknown, Data}, StateName, State) ->
 					   sockname]} | {channel_info, channel_id(), [recv_window |
 										      send_window]} |
 			{close, channel_id()} | stop, term(), state_name(), #state{})
-		       -> gen_fsm_state_return().
+		       -> gen_fsm_sync_return().
 %%--------------------------------------------------------------------
+handle_sync_event(get_print_info, _From, StateName, State) ->
+    Reply =
+	try
+	    {inet:sockname(State#state.socket),
+	     inet:peername(State#state.socket)
+	    }
+	of
+	    {{ok,Local}, {ok,Remote}} -> {{Local,Remote},io_lib:format("statename=~p",[StateName])};
+	    _ -> {{"-",0},"-"}
+	catch
+	    _:_ -> {{"?",0},"?"}
+	end,
+    {reply, Reply, StateName, State};
+
+handle_sync_event({connection_info, Options}, _From, StateName, State) ->
+    Info = ssh_info(Options, State, []),
+    {reply, Info, StateName, State};
+
+handle_sync_event({channel_info, ChannelId, Options}, _From, StateName,
+		  #state{connection_state = #connection{channel_cache = Cache}} = State) ->
+    case ssh_channel:cache_lookup(Cache, ChannelId) of
+       #channel{} = Channel ->
+	    Info = ssh_channel_info(Options, Channel, []),
+	    {reply, Info, StateName, State};
+	undefined ->
+	    {reply, [], StateName, State}
+    end;
+
+handle_sync_event({info, ChannelPid}, _From, StateName,
+	    #state{connection_state =
+		       #connection{channel_cache = Cache}} = State) ->
+    Result = ssh_channel:cache_foldl(
+	       fun(Channel, Acc) when ChannelPid == all;
+				      Channel#channel.user == ChannelPid ->
+		       [Channel | Acc];
+		  (_, Acc) ->
+		       Acc
+	       end, [], Cache),
+    {reply, {ok, Result}, StateName, State};
+
+handle_sync_event(stop, _, _StateName, #state{connection_state = Connection0,
+					     role = Role,
+					     opts = Opts} = State0) ->
+    {disconnect, Reason, {{replies, Replies}, Connection}} =
+	ssh_connection:handle_msg(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_BY_APPLICATION,
+						      description = "User closed down connection",
+						      language = "en"}, Connection0, Role),
+    State = send_replies(Replies, State0),
+    SSHOpts = proplists:get_value(ssh_opts, Opts),
+    disconnect_fun(Reason, SSHOpts),
+    {stop, normal, ok, State#state{connection_state = Connection}};
+
+
+handle_sync_event(Event, From, StateName, State) when StateName /= connected ->
+    Events = [{sync, Event, From} | State#state.event_queue],
+    {next_state, StateName, State#state{event_queue = Events}};
+
 handle_sync_event({request, ChannelPid, ChannelId, Type, Data, Timeout}, From,  StateName, State0) ->
     {{replies, Replies}, State1} = handle_request(ChannelPid,
 						 ChannelId, Type, Data,
@@ -789,46 +854,6 @@ handle_sync_event({recv_window, ChannelId}, _From, StateName,
 	    end,
     {reply, Reply, StateName, next_packet(State)};
 
-handle_sync_event(get_print_info, _From, StateName, State) ->
-    Reply =
-	try
-	    {inet:sockname(State#state.socket),
-	     inet:peername(State#state.socket)
-	    }
-	of
-	    {{ok,Local}, {ok,Remote}} -> {{Local,Remote},io_lib:format("statename=~p",[StateName])};
-	    _ -> {{"-",0},"-"}
-	catch
-	    _:_ -> {{"?",0},"?"}
-	end,
-    {reply, Reply, StateName, State};
-
-handle_sync_event({connection_info, Options}, _From, StateName, State) ->
-    Info = ssh_info(Options, State, []),
-    {reply, Info, StateName, State};
-
-handle_sync_event({channel_info, ChannelId, Options}, _From, StateName,
-		  #state{connection_state = #connection{channel_cache = Cache}} = State) ->
-    case ssh_channel:cache_lookup(Cache, ChannelId) of
-       #channel{} = Channel ->
-	    Info = ssh_channel_info(Options, Channel, []),
-	    {reply, Info, StateName, State};
-	undefined ->
-	    {reply, [], StateName, State}
-    end;
-
-handle_sync_event({info, ChannelPid}, _From, StateName,
-	    #state{connection_state =
-		       #connection{channel_cache = Cache}} = State) ->
-    Result = ssh_channel:cache_foldl(
-	       fun(Channel, Acc) when ChannelPid == all;
-				      Channel#channel.user == ChannelPid ->
-		       [Channel | Acc];
-		  (_, Acc) ->
-		       Acc
-	       end, [], Cache),
-    {reply, {ok, Result}, StateName, State};
-
 handle_sync_event({close, ChannelId}, _, StateName,
 		  #state{connection_state =
 			     #connection{channel_cache = Cache}} = State0) ->
@@ -843,19 +868,7 @@ handle_sync_event({close, ChannelId}, _, StateName,
 	    undefined ->
 		State0
 	end,
-    {reply, ok, StateName, next_packet(State)};
-
-handle_sync_event(stop, _, _StateName, #state{connection_state = Connection0,
-					     role = Role,
-					     opts = Opts} = State0) ->
-    {disconnect, Reason, {{replies, Replies}, Connection}} =
-	ssh_connection:handle_msg(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_BY_APPLICATION,
-						      description = "User closed down connection",
-						      language = "en"}, Connection0, Role),
-    State = send_replies(Replies, State0),
-    SSHOpts = proplists:get_value(ssh_opts, Opts),
-    disconnect_fun(Reason, SSHOpts),
-    {stop, normal, ok, State#state{connection_state = Connection}}.
+    {reply, ok, StateName, next_packet(State)}.
 
 %%--------------------------------------------------------------------
 -spec handle_info({atom(), port(), binary()} | {atom(), port()} |
@@ -1284,8 +1297,17 @@ generate_event(<<?BYTE(Byte), _/binary>> = Msg, StateName,
     ConnectionMsg =  ssh_message:decode(Msg),
     State1 = generate_event_new_state(State0, EncData),
     try ssh_connection:handle_msg(ConnectionMsg, Connection0, Role) of
-	{{replies, Replies}, Connection} ->
-	    State = send_replies(Replies,  State1#state{connection_state = Connection}),
+	{{replies, Replies0}, Connection} ->
+	    if StateName == connected ->
+		    Replies = Replies0,
+		    State2  = State1;
+	       true ->
+		    {ConnReplies, Replies} =
+			lists:splitwith(fun not_connected_filter/1, Replies0),
+		    Q = State1#state.event_queue ++ ConnReplies,
+		    State2  = State1#state{ event_queue = Q }
+	    end,
+	    State = send_replies(Replies,  State2#state{connection_state = Connection}),
 	    {next_state, StateName, next_packet(State)};
 	{noreply, Connection} ->
 	    {next_state, StateName, next_packet(State1#state{connection_state = Connection})};
@@ -1458,15 +1480,43 @@ next_packet(#state{socket = Socket} = State) ->
     State.
 
 after_new_keys(#state{renegotiate = true} = State) ->
-    {connected, State#state{renegotiate = false}};
+    State1 = State#state{renegotiate = false, event_queue = []},
+    lists:foldr(fun after_new_keys_events/2, {next_state, connected, State1}, State#state.event_queue);
 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),
-    {userauth, State#state{ssh_params = Ssh}};
+    {next_state, userauth, State#state{ssh_params = Ssh}};
 after_new_keys(#state{renegotiate = false,  
 		      ssh_params = #ssh{role = server}} = State) ->
-    {userauth, State}.
+    {next_state, userauth, State}.
+
+after_new_keys_events({sync, _Event, From}, {stop, _Reason, _StateData}=Terminator) ->
+    gen_fsm:reply(From, {error, closed}),
+    Terminator;
+after_new_keys_events(_, {stop, _Reason, _StateData}=Terminator) ->
+    Terminator;
+after_new_keys_events({sync, Event, From}, {next_state, StateName, StateData}) ->
+    case handle_sync_event(Event, From, StateName, StateData) of
+	{reply, Reply, NextStateName, NewStateData} ->
+	    gen_fsm:reply(From, Reply),
+	    {next_state, NextStateName, NewStateData};
+	{next_state, NextStateName, NewStateData}->
+	    {next_state, NextStateName, NewStateData};
+	{stop, Reason, Reply, NewStateData} ->
+	    gen_fsm:reply(From, Reply),
+	    {stop, Reason, NewStateData}
+    end;
+after_new_keys_events({event, Event}, {next_state, StateName, StateData}) ->
+    case handle_event(Event, StateName, StateData) of
+	{next_state, NextStateName, NewStateData}->
+	    {next_state, NextStateName, NewStateData};
+	{stop, Reason, NewStateData} ->
+	    {stop, Reason, NewStateData}
+    end;
+after_new_keys_events({connection_reply, _Data} = Reply, {StateName, State}) ->
+    NewState = send_replies([Reply], State),
+    {next_state, StateName, NewState}.
 
 handle_ssh_packet_data(RemainingSshPacketLen, DecData, EncData, StateName, 
 		       State) ->
@@ -1627,6 +1677,11 @@ log_error(Reason) ->
     error_logger:error_report(Report),
     "Internal error".
 
+not_connected_filter({connection_reply, _Data}) ->
+    true;
+not_connected_filter(_) ->
+    false.
+
 send_replies([], State) ->
     State;
 send_replies([{connection_reply, Data} | Rest], #state{ssh_params = Ssh0} = State) ->