Bugs in ssh regnotiation
"Simon Cornish" <[email protected]> Tue, 28 Apr 2015 07:08:08 -0700
| Newsgroups | gmane.comp.lang.erlang.bugs |
|---|---|
| Message-ID | <[email protected]> |
Hi, There are two bugs surrounding the OTP ssh implementation of key rengotiation. The first is the handling of the sent data statistics. The port stats are not accumulated so that once rekey_limit bytes (by default, 1GB) have been transmitted the connection will be rekeyed every minute, not after the next 1GB. The attached patch, rekey_limit.diff, provides a simple solution. In the second bug, ssh_connection_handler processes channel data requests from either the application or the network during renegotiation. This is in violation of the RFC and causes the peer to disconnect (observed with sshd on Solaris 10 and SLES 11). The solution is to queue those events for processing until after renegotiation is complete. The second attached patch is in that direction. It's not particularly complete hence the reason this is an email to erlang-bugs and not a pull request. This gist can recreate the fault fairly dependably by ensuring a constant flow of data. https://gist.github.com/dotsimon/ef41296db307561d8f94 /Simon _______________________________________________ erlang-bugs mailing list [email protected] http://erlang.org/mailman/listinfo/erlang-bugs
rekey_limit.diff
(text/plain, 1.4 KB)
diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index b7ccb02..59e80e6 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -68,6 +68,7 @@
undecoded_packet_length, % integer()
key_exchange_init_msg, % #ssh_msg_kexinit{}
renegotiate = false, % boolean()
+ last_size_rekey = 0,
connection_queue,
address,
port,
@@ -596,7 +597,8 @@ handle_event(renegotiate, StateName, State) ->
%% Rekey due to sent data limit reached?
handle_event(data_size, connected, #state{ssh_params = Ssh0} = State) ->
- {ok, [{send_oct,Sent}]} = inet:getstat(State#state.socket, [send_oct]),
+ {ok, [{send_oct,Sent0}]} = inet:getstat(State#state.socket, [send_oct]),
+ Sent = Sent0 - State#state.last_size_rekey,
MaxSent = proplists:get_value(rekey_limit, State#state.opts, 1024000000),
timer:apply_after(?REKEY_DATA_TIMOUT, gen_fsm, send_all_state_event, [self(), data_size]),
case Sent >= MaxSent of
@@ -606,7 +608,8 @@ handle_event(data_size, connected, #state{ssh_params = Ssh0} = State) ->
{next_state, kexinit,
next_packet(State#state{ssh_params = Ssh,
key_exchange_init_msg = KeyInitMsg,
- renegotiate = true})};
+ renegotiate = true,
+ last_size_rekey = Sent0})};
_ ->
{next_state, connected, next_packet(State)}
end;
queue_data.patch
(application/octet-stream, 4.2 KB)
diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index 59e80e6..dedb2e6 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -69,7 +69,8 @@
key_exchange_init_msg, % #ssh_msg_kexinit{}
renegotiate = false, % boolean()
last_size_rekey = 0,
- connection_queue,
+ kexinit_reply_queue = [],
+ kexinit_event_queue = [],
address,
port,
opts
@@ -379,9 +380,12 @@ 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)}.
+ case after_new_keys(State0#state{ssh_params = Ssh}) of
+ {stop, _Reason, _NewStateData} = Terminator ->
+ Terminator;
+ {NextStateName, State} ->
+ {next_state, NextStateName, next_packet(State)}
+ end.
%%--------------------------------------------------------------------
-spec userauth(#ssh_msg_service_request{} | #ssh_msg_service_accept{} |
@@ -675,6 +679,11 @@ handle_sync_event({global_request, Pid, _, _, _} = Request, From, StateName,
State = add_request(true, Channel#channel.local_id, From, State1),
{next_state, StateName, next_packet(State)};
+handle_sync_event({data, ChannelId, Type, Data, Timeout}, From, StateName, State)
+ when StateName == kexinit orelse State#state.renegotiate == true ->
+ Events = [{{data, ChannelId, Type, Data, Timeout}, From} | State#state.kexinit_event_queue],
+ {next_state, StateName, State#state{kexinit_event_queue = Events}};
+
handle_sync_event({data, ChannelId, Type, Data, Timeout}, From, StateName,
#state{connection_state = #connection{channel_cache = _Cache}
= Connection0} = State0) ->
@@ -1185,7 +1194,12 @@ generate_event(<<?BYTE(Byte), _/binary>> = Msg, StateName,
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}),
+ if Renegotiation == true ->
+ NewReplies = State1#state.kexinit_reply_queue ++ Replies,
+ State = State1#state{connection_state = Connection, kexinit_reply_queue = NewReplies };
+ true ->
+ State = send_replies(Replies, State1#state{connection_state = Connection})
+ end,
{next_state, StateName, next_packet(State)};
{noreply, Connection} ->
{next_state, StateName, next_packet(State1#state{connection_state = Connection})};
@@ -1348,7 +1362,11 @@ next_packet(#state{socket = Socket} = State) ->
State.
after_new_keys(#state{renegotiate = true} = State) ->
- {connected, State#state{renegotiate = false}};
+ State1 = State#state{renegotiate = false},
+ {NewStateName, State2} =
+ lists:foldr(fun after_new_keys_sync_event/2, {connected, State1}, State#state.kexinit_event_queue),
+ State3 = send_replies(State#state.kexinit_reply_queue, State2),
+ {NewStateName, State3#state{kexinit_reply_queue = [], kexinit_event_queue = []}};
after_new_keys(#state{renegotiate = false,
ssh_params = #ssh{role = client} = Ssh0} = State) ->
{Msg, Ssh} = ssh_auth:service_request_msg(Ssh0),
@@ -1358,6 +1376,21 @@ after_new_keys(#state{renegotiate = false,
ssh_params = #ssh{role = server}} = State) ->
{userauth, State}.
+after_new_keys_sync_event({_Event, From}, {stop, _Reason, _StateData}=Terminator) ->
+ gen_fsm:reply(From, {error, closed}),
+ Terminator;
+after_new_keys_sync_event({Event, From}, {StateName, StateData}) ->
+ case handle_sync_event(Event, From, StateName, StateData) of
+ {reply, Reply, NextStateName, NewStateData} ->
+ gen_fsm:reply(From, Reply),
+ {NextStateName, NewStateData};
+ {next_state, NextStateName, NewStateData}->
+ {NextStateName, NewStateData};
+ {stop, Reason, Reply, NewStateData} ->
+ gen_fsm:reply(From, Reply),
+ {stop, Reason, NewStateData}
+ end.
+
handle_ssh_packet_data(RemainingSshPacketLen, DecData, EncData, StateName,
State) ->
EncSize = size(EncData),