From 226e77ef7162b0fc043d99a5f68f5dcc891fb093 Mon Sep 17 00:00:00 2001 From: Hans Nilsson Date: Thu, 19 Nov 2015 17:16:05 +0100 Subject: 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. --- lib/ssh/src/ssh_connection_handler.erl | 129 ++++++++------------------------- lib/ssh/src/ssh_transport.erl | 94 ++++++++++++++++-------- 2 files changed, 94 insertions(+), 129 deletions(-) (limited to 'lib/ssh') 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(<>, 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 = - <>})} + ssh_params = Ssh0, + decoded_data_buffer = DecData0, + encoded_data_buffer = EncData0, + undecoded_packet_length = RemainingSshPacketLen0} = State0) -> + Encoded = <>, + 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, <>, - 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 = <>, - 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) -> - <> = Bin, - {Ssh, <> = DecData} = - decrypt(Ssh0, EncBlock), - {Ssh, PacketLen, DecData, EncData}. - decrypt_blocks(Bin, Length, Ssh0) -> <> = 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 -> - <> = EncodedSoFar, - {NoMac0, <<>>, Rest0}; - _ -> - <> = 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, + <> = Encoded0, + {Ssh1, DecData} = decrypt(Ssh0, PktT), + MsgBytes = <>, + 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 -> + <> = Encoded0, + {Ssh, Decoded} = decrypt(Ssh0, EncBlock), + <> = Decoded, + {ok, PacketLen, Decoded, EncodedRest, Ssh}; + false -> + get_more + end. + + msg_data(PacketData) -> <> = 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, -- cgit v1.2.3