ssh: refactor packet reception

There was an assymetric relationship between receiving a ssh-packet (decrypting-mac-decompress) and sending one. When sending, most of the work was defined in the ssh_transport module, while at reception the ssh_connection_handler was the one knowing what to do.
This commit moves the reception down to the ssh_transport module where it belongs.

2 files changed, 94 insertions, 129 deletions

diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index 8448218d91..505c6eb181 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -970,57 +970,39 @@ handle_info({Protocol, Socket, Info}, hello,
 		   transport_protocol = Protocol} = State) -> 
     event({info_line, Info}, hello, State);
 
-handle_info({Protocol, Socket, Data}, Statename, 
+handle_info({Protocol, Socket, Data}, StateName, 
 	    #state{socket = Socket,
 		   transport_protocol = Protocol,
-		   ssh_params = #ssh{decrypt_block_size = BlockSize,
-				     recv_mac_size = MacSize} = Ssh0,
-		   decoded_data_buffer = <<>>,
-		   encoded_data_buffer = EncData0} = State0) ->
-
-    %% Implementations SHOULD decrypt the length after receiving the
-    %% first 8 (or cipher block size, whichever is larger) bytes of a
-    %% packet. (RFC 4253: Section 6 - Binary Packet Protocol)
-    case size(EncData0) + size(Data) >= erlang:max(8, BlockSize) of
-	true ->
-	    {Ssh, SshPacketLen, DecData, EncData} = 
-
-		ssh_transport:decrypt_first_block(<<EncData0/binary, 
-						   Data/binary>>, Ssh0),
-	     case SshPacketLen > ?SSH_MAX_PACKET_SIZE of
- 		true ->
-  		    DisconnectMsg = 
-  			#ssh_msg_disconnect{code = 
-					    ?SSH_DISCONNECT_PROTOCOL_ERROR,
-  					    description = "Bad packet length " 
-					    ++ integer_to_list(SshPacketLen),
-  					    language = "en"},
-  		    handle_disconnect(DisconnectMsg, State0);
-  		false ->
-		    RemainingSshPacketLen = 
-			(SshPacketLen + ?SSH_LENGHT_INDICATOR_SIZE) - 
-			BlockSize + MacSize,
-		    State = State0#state{ssh_params = Ssh},
-		    handle_ssh_packet_data(RemainingSshPacketLen, 
-					   DecData, EncData, Statename,
-					   State)
-	     end;
-	false  ->
-	    {next_state, Statename, 
-	     next_packet(State0#state{encoded_data_buffer = 
-				      <<EncData0/binary, Data/binary>>})}
+		   ssh_params = Ssh0,
+		   decoded_data_buffer = DecData0,
+		   encoded_data_buffer = EncData0,
+		   undecoded_packet_length = RemainingSshPacketLen0} = State0) ->
+    Encoded = <<EncData0/binary, Data/binary>>,
+    case ssh_transport:handle_packet_part(DecData0, Encoded, RemainingSshPacketLen0, Ssh0) of
+	{get_more, DecBytes, EncDataRest, RemainingSshPacketLen, Ssh1} ->
+	    {next_state, StateName,
+	     next_packet(State0#state{encoded_data_buffer = EncDataRest,
+				      decoded_data_buffer = DecBytes,
+				      undecoded_packet_length = RemainingSshPacketLen,
+				      ssh_params = Ssh1})};
+	{decoded, MsgBytes, EncDataRest, Ssh1} ->
+	    generate_event(MsgBytes, StateName,
+			   State0#state{ssh_params = Ssh1,
+					%% Important to be set for
+					%% next_packet
+%%% FIXME: the following three seem to always be set in generate_event!
+					decoded_data_buffer = <<>>,
+					undecoded_packet_length = undefined,
+					encoded_data_buffer = EncDataRest},
+			   EncDataRest);
+	{bad_mac, Ssh1} ->
+	    DisconnectMsg = 
+		    #ssh_msg_disconnect{code = ?SSH_DISCONNECT_PROTOCOL_ERROR,
+					description = "Bad mac",
+					language = ""},
+	    handle_disconnect(DisconnectMsg, State0#state{ssh_params=Ssh1})
     end;
-
-handle_info({Protocol, Socket, Data}, Statename, 
-	    #state{socket = Socket,
-		   transport_protocol = Protocol,
-		   decoded_data_buffer = DecData, 
-		   encoded_data_buffer = EncData,
-		   undecoded_packet_length = Len} = 
-	    State) when is_integer(Len) ->
-    handle_ssh_packet_data(Len, DecData, <<EncData/binary, Data/binary>>, 
-			   Statename, State);
-
+		
 handle_info({CloseTag, _Socket}, _StateName, 
 	    #state{transport_close_tag = CloseTag,
 		   ssh_params = #ssh{role = _Role, opts = _Opts}} = State) ->
@@ -1631,57 +1613,6 @@ 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) ->
-    EncSize =  size(EncData), 
-    case RemainingSshPacketLen > EncSize of
-	true ->
-	    {next_state, StateName, 
-	     next_packet(State#state{decoded_data_buffer = DecData,
-				     encoded_data_buffer = EncData,
-				     undecoded_packet_length = 
-				     RemainingSshPacketLen})};
-	false ->
-	    handle_ssh_packet(RemainingSshPacketLen, StateName,
-			      State#state{decoded_data_buffer = DecData,
-					  encoded_data_buffer = EncData})
-    
-    end.    
-
-handle_ssh_packet(Length, StateName, #state{decoded_data_buffer = DecData0,
-					    encoded_data_buffer = EncData0,
-					    ssh_params = Ssh0,
-					    transport_protocol = _Protocol,
-					    socket = _Socket} = State0) ->
-    try 
-	{Ssh1, DecData, EncData, Mac} = 
-	    ssh_transport:unpack(EncData0, Length, Ssh0),
-	SshPacket = <<DecData0/binary, DecData/binary>>,
-	case ssh_transport:is_valid_mac(Mac, SshPacket, Ssh1) of
-	    true ->
-		PacketData = ssh_transport:msg_data(SshPacket),
-		{Ssh1, Msg} = ssh_transport:decompress(Ssh1, PacketData),
-		generate_event(Msg, StateName, 
-			       State0#state{ssh_params = Ssh1,
-					    %% Important to be set for
-					    %% next_packet
-					    decoded_data_buffer = <<>>},
-			       EncData);
-	    false ->
-		DisconnectMsg = 
-		    #ssh_msg_disconnect{code = ?SSH_DISCONNECT_PROTOCOL_ERROR,
-					description = "Bad mac",
-					language = "en"},
-		handle_disconnect(DisconnectMsg, State0)
-	end
-    catch _:_ ->
-	    Disconnect = 
-		#ssh_msg_disconnect{code = ?SSH_DISCONNECT_PROTOCOL_ERROR,
-				    description = "Bad input",
-				    language = "en"},
-	    handle_disconnect(Disconnect, State0) 
-    end.  
-
 
 handle_disconnect(DisconnectMsg, State) ->
     handle_disconnect(own, DisconnectMsg, State).
diff --git a/lib/ssh/src/ssh_transport.erl b/lib/ssh/src/ssh_transport.erl
index 0c999b96cc..f18e4b4d01 100644
--- a/lib/ssh/src/ssh_transport.erl
+++ b/lib/ssh/src/ssh_transport.erl
@@ -31,10 +31,10 @@
 -include("ssh.hrl").
 
 -export([versions/2, hello_version_msg/1]).
--export([next_seqnum/1, decrypt_first_block/2, decrypt_blocks/3,
+-export([next_seqnum/1, 
 	 supported_algorithms/0, supported_algorithms/1,
 	 default_algorithms/0, default_algorithms/1,
-	 is_valid_mac/3,
+	 handle_packet_part/4,
 	 handle_hello_version/1,
 	 key_exchange_init_msg/1,
 	 key_init/3, new_keys_message/1,
@@ -45,9 +45,13 @@
 	 handle_kex_ecdh_init/2,
 	 handle_kex_ecdh_reply/2,
 	 extract_public_key/1,
-	 unpack/3, decompress/2, ssh_packet/2, pack/2, pack/3, msg_data/1,
+	 ssh_packet/2, pack/2,
 	 sign/3, verify/4]).
 
+%%% For test suites
+-export([pack/3]).
+-export([decompress/2,  decrypt_blocks/3, is_valid_mac/3 ]). % FIXME: remove
+
 %%%----------------------------------------------------------------------------
 %%%
 %%% There is a difference between supported and default algorithms. The
@@ -196,12 +200,6 @@ hello_version_msg(Data) ->
 next_seqnum(SeqNum) ->
     (SeqNum + 1) band 16#ffffffff.
 
-decrypt_first_block(Bin, #ssh{decrypt_block_size = BlockSize} = Ssh0) ->
-    <<EncBlock:BlockSize/binary, EncData/binary>> = Bin,
-    {Ssh, <<?UINT32(PacketLen), _/binary>> = DecData} =
-	decrypt(Ssh0, EncBlock),
-    {Ssh, PacketLen, DecData, EncData}.
-    
 decrypt_blocks(Bin, Length, Ssh0) ->
     <<EncBlocks:Length/binary, EncData/binary>> = Bin,
     {Ssh, DecData} = decrypt(Ssh0, EncBlocks),
@@ -938,27 +936,61 @@ pack(Data0, #ssh{encrypt_block_size = BlockSize,
     Ssh = Ssh2#ssh{send_sequence = (SeqNum+1) band 16#ffffffff},
     {Packet, Ssh}.
 
-unpack(EncodedSoFar, ReminingLenght, #ssh{recv_mac_size = MacSize} = Ssh0) ->
-    SshLength = ReminingLenght - MacSize,
-    {NoMac, Mac, Rest} = case MacSize of
-			     0 ->
-				 <<NoMac0:SshLength/binary, 
-				  Rest0/binary>> = EncodedSoFar,
-				 {NoMac0, <<>>, Rest0};
-			     _ ->
-				 <<NoMac0:SshLength/binary, 
-				  Mac0:MacSize/binary,
-				  Rest0/binary>> = EncodedSoFar,
-				 {NoMac0, Mac0, Rest0}
-			 end,
-    {Ssh1, DecData, <<>>} = 
-	case SshLength of
-	    0 ->
-		{Ssh0, <<>>, <<>>};
-	    _ ->
-		decrypt_blocks(NoMac, SshLength, Ssh0)
-	end,
-    {Ssh1, DecData, Rest, Mac}.
+
+handle_packet_part(<<>>, Encoded0, undefined, Ssh0) ->
+    %% New ssh packet
+    case get_length(Encoded0, Ssh0) of
+	get_more ->
+	    %% too short to get the length
+	    {get_more, <<>>, Encoded0, undefined, Ssh0};
+
+	{ok, PacketLen, _DecData, _Encoded1, _Ssh1} when PacketLen > ?SSH_MAX_PACKET_SIZE ->
+	    %% far too long message than expected
+	    throw(#ssh_msg_disconnect{code = ?SSH_DISCONNECT_PROTOCOL_ERROR,
+				      description = "Bad packet length " 
+				      ++ integer_to_list(PacketLen),
+				      language = ""});
+	
+	{ok, PacketLen, DecData, Encoded1, 
+	 #ssh{decrypt_block_size = BlockSize,
+	      recv_mac_size = MacSize} = Ssh1} ->
+	    %% enough bytes so we got the length and can calculate how many
+	    %% more bytes to expect for a full packet
+	    Remaining = (PacketLen + ?SSH_LENGHT_INDICATOR_SIZE) - BlockSize + MacSize,
+	    handle_packet_part(DecData, Encoded1, Remaining, Ssh1)
+    end;
+handle_packet_part(Decoded0, Encoded0, Remaining, Ssh0) 
+  when size(Encoded0) < Remaining ->
+    %% need more bytes to finalize the packet
+    {get_more, Decoded0, Encoded0, Remaining, Ssh0};
+handle_packet_part(Decoded0, Encoded0, Remaining, 
+		   #ssh{recv_mac_size = MacSize} = Ssh0) ->
+    %% enough bytes to decode the packet.
+    SshLengthNotDecoded = Remaining - MacSize,
+    <<PktT:SshLengthNotDecoded/binary, Mac:MacSize/binary, EncRest0/binary>> = Encoded0,
+    {Ssh1, DecData} = decrypt(Ssh0, PktT),
+    MsgBytes = <<Decoded0/binary,DecData/binary>>,
+    case is_valid_mac(Mac, MsgBytes, Ssh1) of
+	false ->
+	    {bad_mac, Ssh1};
+	true ->
+	    {Ssh, DecompressedMsgBytes} = decompress(Ssh1, msg_data(MsgBytes)),
+	    {decoded, DecompressedMsgBytes, EncRest0, Ssh}
+    end.
+    
+    
+get_length(Encoded0, #ssh{decrypt_block_size = BlockSize} = Ssh0) ->
+    case size(Encoded0) >= erlang:max(8, BlockSize) of
+	true ->
+	    <<EncBlock:BlockSize/binary, EncodedRest/binary>> = Encoded0,
+	    {Ssh, Decoded} = decrypt(Ssh0, EncBlock),
+	    <<?UINT32(PacketLen), _/binary>> = Decoded,
+	    {ok, PacketLen, Decoded, EncodedRest, Ssh};
+	false ->
+	    get_more
+    end.
+
+
 
 msg_data(PacketData) ->
     <<Len:32, PaddingLen:8, _/binary>> = PacketData,
@@ -1181,6 +1213,8 @@ decrypt_final(Ssh) ->
 		  decrypt_ctx = undefined,
 		  decrypt_block_size = 8}}.
 
+decrypt(Ssh, <<>>) ->
+    {Ssh, <<>>};
 decrypt(#ssh{decrypt = none} = Ssh, Data) ->
     {Ssh, Data};
 decrypt(#ssh{decrypt = '3des-cbc', decrypt_keys = Keys,