From d53bd4936297f6310e59f88191014bb5dd0d8b2c Mon Sep 17 00:00:00 2001 From: Hans Nilsson Date: Thu, 13 Oct 2016 13:09:42 +0200 Subject: ssh: property test case for illegal infoline and close This tests an illegal client that sends an info line and closes 'immediatly'. --- lib/ssh/test/Makefile | 3 +- .../property_test/ssh_eqc_client_info_timing.erl | 92 ++++++++++++++++++++++ lib/ssh/test/ssh_eqc_event_handler.erl | 43 ++++++++++ lib/ssh/test/ssh_property_test_SUITE.erl | 7 ++ lib/ssh/test/ssh_test_lib.erl | 25 ++++++ 5 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 lib/ssh/test/property_test/ssh_eqc_client_info_timing.erl create mode 100644 lib/ssh/test/ssh_eqc_event_handler.erl (limited to 'lib') diff --git a/lib/ssh/test/Makefile b/lib/ssh/test/Makefile index 6ce6d6f537..3fca78237c 100644 --- a/lib/ssh/test/Makefile +++ b/lib/ssh/test/Makefile @@ -52,7 +52,8 @@ MODULES= \ ssh_echo_server \ ssh_peername_sockname_server \ ssh_test_cli \ - ssh_relay + ssh_relay \ + ssh_eqc_event_handler HRL_FILES_NEEDED_IN_TEST= \ $(ERL_TOP)/lib/ssh/test/ssh_test_lib.hrl \ diff --git a/lib/ssh/test/property_test/ssh_eqc_client_info_timing.erl b/lib/ssh/test/property_test/ssh_eqc_client_info_timing.erl new file mode 100644 index 0000000000..c07140dc43 --- /dev/null +++ b/lib/ssh/test/property_test/ssh_eqc_client_info_timing.erl @@ -0,0 +1,92 @@ +%% +%% %CopyrightBegin% +%% +%% Copyright Ericsson AB 2004-2016. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%% +%% %CopyrightEnd% +%% +%% + +-module(ssh_eqc_client_info_timing). + +-compile(export_all). + +-proptest(eqc). +-proptest([triq,proper]). + +-ifndef(EQC). +-ifndef(PROPER). +-ifndef(TRIQ). +-define(EQC,true). +%%-define(PROPER,true). +%%-define(TRIQ,true). +-endif. +-endif. +-endif. + +-ifdef(EQC). +-include_lib("eqc/include/eqc.hrl"). +-define(MOD_eqc,eqc). + +-else. +-ifdef(PROPER). +-include_lib("proper/include/proper.hrl"). +-define(MOD_eqc,proper). + +-else. +-ifdef(TRIQ). +-define(MOD_eqc,triq). +-include_lib("triq/include/triq.hrl"). + +-endif. +-endif. +-endif. + + +%%% Properties: + +prop_seq(_Config) -> + {ok,Pid} = ssh_eqc_event_handler:add_report_handler(), + {_, _, Port} = init_daemon(), + numtests(1000, + ?FORALL(Delay, choose(0,100),%% Micro seconds + try + send_bad_sequence(Port, Delay, Pid), + not any_relevant_error_report(Pid) + catch + C:E -> io:format('~p:~p~n',[C,E]), + false + end + )). + +send_bad_sequence(Port, Delay, Pid) -> + {ok,S} = gen_tcp:connect("localhost",Port,[]), + gen_tcp:send(S,"Illegal info-string\r\n"), + ssh_test_lib:sleep_microsec(Delay), + gen_tcp:close(S). + +any_relevant_error_report(Pid) -> + {ok, Reports} = ssh_eqc_event_handler:get_reports(Pid), + lists:any(fun({error_report,_,{_,supervisor_report,L}}) when is_list(L) -> + lists:member({reason,{badmatch,{error,closed}}}, L); + (_) -> + false + end, Reports). + +%%%================================================================ +init_daemon() -> + ok = begin ssh:stop(), ssh:start() end, + ssh_test_lib:daemon([]). + diff --git a/lib/ssh/test/ssh_eqc_event_handler.erl b/lib/ssh/test/ssh_eqc_event_handler.erl new file mode 100644 index 0000000000..233965012a --- /dev/null +++ b/lib/ssh/test/ssh_eqc_event_handler.erl @@ -0,0 +1,43 @@ +-module(ssh_eqc_event_handler). + +-compile(export_all). + +-behaviour(gen_event). + +add_report_handler() -> + error_logger:add_report_handler(?MODULE, [self(),Ref=make_ref()]), + receive + {event_handler_started,HandlerPid,Ref} -> + {ok,HandlerPid} + end. + +get_reports(Pid) -> + Pid ! {get_reports,self(),Ref=make_ref()}, + receive + {reports,Reports,Ref} -> + {ok,Reports} + end. + +%%%================================================================ + +-record(state, { + reports = [] + }). + +%% error_logger:add_report_handler(ssh_eqc_event_handler, [self()]). + +init([CallerPid,Ref]) -> + CallerPid ! {event_handler_started,self(),Ref}, + {ok, #state{}}. + +handle_event(Event, State) -> + {ok, State#state{reports = [Event|State#state.reports]}}. + +handle_info({get_reports,From,Ref}, State) -> + From ! {reports, lists:reverse(State#state.reports), Ref}, + {ok, State#state{reports=[]}}. + +handle_call(_Request, State) -> {ok,reply,State}. +terminate(_Arg, _State) -> stop. + +code_change(_OldVsn, State, _Extra) -> {ok, State}. diff --git a/lib/ssh/test/ssh_property_test_SUITE.erl b/lib/ssh/test/ssh_property_test_SUITE.erl index c8aabcedb7..7ba2732a88 100644 --- a/lib/ssh/test/ssh_property_test_SUITE.erl +++ b/lib/ssh/test/ssh_property_test_SUITE.erl @@ -38,6 +38,7 @@ -include_lib("common_test/include/ct.hrl"). all() -> [{group, messages}, + client_sends_info_timing, {group, client_server} ]. @@ -106,3 +107,9 @@ client_server_parallel_multi(Config) -> ssh_eqc_client_server:prop_parallel_multi(Config), Config ). + +client_sends_info_timing(Config) -> + ct_property_test:quickcheck( + ssh_eqc_client_info_timing:prop_seq(Config), + Config + ). diff --git a/lib/ssh/test/ssh_test_lib.erl b/lib/ssh/test/ssh_test_lib.erl index 6233680dce..c43c6519f9 100644 --- a/lib/ssh/test/ssh_test_lib.erl +++ b/lib/ssh/test/ssh_test_lib.erl @@ -767,3 +767,28 @@ open_port(Arg1, ExtraOpts) -> use_stdio, overlapped_io, hide %only affects windows | ExtraOpts]). + +%%%---------------------------------------------------------------- +%%% Sleeping + +%%% Milli sec +sleep_millisec(Nms) -> receive after Nms -> ok end. + +%%% Micro sec +sleep_microsec(Nus) -> + busy_wait(Nus, erlang:system_time(microsecond)). + +busy_wait(Nus, T0) -> + T = erlang:system_time(microsecond) - T0, + Tleft = Nus - T, + if + Tleft > 2000 -> + sleep_millisec((Tleft-1500) div 1000), % μs -> ms + busy_wait(Nus,T0); + Tleft > 1 -> + busy_wait(Nus, T0); + true -> + T + end. + +%%%---------------------------------------------------------------- -- cgit v1.2.3 From d35017055aa3a138177270221369008d9d263674 Mon Sep 17 00:00:00 2001 From: Hans Nilsson Date: Thu, 13 Oct 2016 15:22:40 +0200 Subject: ssh: ssh_protocol_SUITE test for handling of illegal info_lines --- lib/ssh/test/ssh_protocol_SUITE.erl | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) (limited to 'lib') diff --git a/lib/ssh/test/ssh_protocol_SUITE.erl b/lib/ssh/test/ssh_protocol_SUITE.erl index 4fac1f718a..93d0bc2eb0 100644 --- a/lib/ssh/test/ssh_protocol_SUITE.erl +++ b/lib/ssh/test/ssh_protocol_SUITE.erl @@ -48,6 +48,7 @@ suite() -> all() -> [{group,tool_tests}, + client_info_line, {group,kex}, {group,service_requests}, {group,authentication}, @@ -575,6 +576,36 @@ client_handles_keyboard_interactive_0_pwds(Config) -> ). + +%%%-------------------------------------------------------------------- +client_info_line(_Config) -> + %% A client must not send an info-line. If it does, the server should handle + %% handle this gracefully + {ok,Pid} = ssh_eqc_event_handler:add_report_handler(), + {_, _, Port} = ssh_test_lib:daemon([]), + + %% Fake client: + {ok,S} = gen_tcp:connect("localhost",Port,[]), + gen_tcp:send(S,"An illegal info-string\r\n"), + gen_tcp:close(S), + + %% wait for server to react: + timer:sleep(1000), + + %% check if a badmatch was received: + {ok, Reports} = ssh_eqc_event_handler:get_reports(Pid), + case lists:any(fun({error_report,_,{_,supervisor_report,L}}) when is_list(L) -> + lists:member({reason,{badmatch,{error,closed}}}, L); + (_) -> + false + end, Reports) of + true -> + ct:fail("Bad error report on info_line from client"); + false -> + ok + end. + + %%%================================================================ %%%==== Internal functions ======================================== %%%================================================================ -- cgit v1.2.3 From f6c9b5caaa1ba6c22248ff22cc678b30d89cdd36 Mon Sep 17 00:00:00 2001 From: Hans Nilsson Date: Thu, 13 Oct 2016 10:33:26 +0200 Subject: ssh: Removed matching of 'ok' after send which could cause error reports --- lib/ssh/src/ssh_connection_handler.erl | 50 ++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 24 deletions(-) (limited to 'lib') diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl index abfba4baf1..ced049f0d0 100644 --- a/lib/ssh/src/ssh_connection_handler.erl +++ b/lib/ssh/src/ssh_connection_handler.erl @@ -525,7 +525,7 @@ handle_event(_, _Event, {init_error,Error}, _) -> %% The very first event that is sent when the we are set as controlling process of Socket handle_event(_, socket_control, {hello,_}, D) -> VsnMsg = ssh_transport:hello_version_msg(string_version(D#data.ssh_params)), - ok = send_bytes(VsnMsg, D), + send_bytes(VsnMsg, D), case inet:getopts(Socket=D#data.socket, [recbuf]) of {ok, [{recbuf,Size}]} -> %% Set the socket to the hello text line handling mode: @@ -545,12 +545,13 @@ handle_event(_, {info_line,_Line}, {hello,Role}, D) -> case Role of client -> %% The server may send info lines to the client before the version_exchange + %% RFC4253/4.2 inet:setopts(D#data.socket, [{active, once}]), keep_state_and_data; server -> %% But the client may NOT send them to the server. Openssh answers with cleartext, %% and so do we - ok = send_bytes("Protocol mismatch.", D), + send_bytes("Protocol mismatch.", D), {stop, {shutdown,"Protocol mismatch in version exchange. Client sent info lines."}} end; @@ -565,7 +566,7 @@ handle_event(_, {version_exchange,Version}, {hello,Role}, D) -> {active, once}, {recbuf, D#data.inet_initial_recbuf_size}]), {KeyInitMsg, SshPacket, Ssh} = ssh_transport:key_exchange_init_msg(Ssh1), - ok = send_bytes(SshPacket, D), + send_bytes(SshPacket, D), {next_state, {kexinit,Role,init}, D#data{ssh_params = Ssh, key_exchange_init_msg = KeyInitMsg}}; not_supported -> @@ -583,7 +584,7 @@ handle_event(_, {#ssh_msg_kexinit{}=Kex, Payload}, {kexinit,Role,ReNeg}, Ssh1 = ssh_transport:key_init(peer_role(Role), D#data.ssh_params, Payload), Ssh = case ssh_transport:handle_kexinit_msg(Kex, OwnKex, Ssh1) of {ok, NextKexMsg, Ssh2} when Role==client -> - ok = send_bytes(NextKexMsg, D), + send_bytes(NextKexMsg, D), Ssh2; {ok, Ssh2} when Role==server -> Ssh2 @@ -596,43 +597,43 @@ handle_event(_, {#ssh_msg_kexinit{}=Kex, Payload}, {kexinit,Role,ReNeg}, %%%---- diffie-hellman handle_event(_, #ssh_msg_kexdh_init{} = Msg, {key_exchange,server,ReNeg}, D) -> {ok, KexdhReply, Ssh1} = ssh_transport:handle_kexdh_init(Msg, D#data.ssh_params), - ok = send_bytes(KexdhReply, D), + send_bytes(KexdhReply, D), {ok, NewKeys, Ssh} = ssh_transport:new_keys_message(Ssh1), - ok = send_bytes(NewKeys, D), + send_bytes(NewKeys, D), {next_state, {new_keys,server,ReNeg}, D#data{ssh_params=Ssh}}; handle_event(_, #ssh_msg_kexdh_reply{} = Msg, {key_exchange,client,ReNeg}, D) -> {ok, NewKeys, Ssh} = ssh_transport:handle_kexdh_reply(Msg, D#data.ssh_params), - ok = send_bytes(NewKeys, D), + send_bytes(NewKeys, D), {next_state, {new_keys,client,ReNeg}, D#data{ssh_params=Ssh}}; %%%---- diffie-hellman group exchange handle_event(_, #ssh_msg_kex_dh_gex_request{} = Msg, {key_exchange,server,ReNeg}, D) -> {ok, GexGroup, Ssh} = ssh_transport:handle_kex_dh_gex_request(Msg, D#data.ssh_params), - ok = send_bytes(GexGroup, D), + send_bytes(GexGroup, D), {next_state, {key_exchange_dh_gex_init,server,ReNeg}, D#data{ssh_params=Ssh}}; handle_event(_, #ssh_msg_kex_dh_gex_request_old{} = Msg, {key_exchange,server,ReNeg}, D) -> {ok, GexGroup, Ssh} = ssh_transport:handle_kex_dh_gex_request(Msg, D#data.ssh_params), - ok = send_bytes(GexGroup, D), + send_bytes(GexGroup, D), {next_state, {key_exchange_dh_gex_init,server,ReNeg}, D#data{ssh_params=Ssh}}; handle_event(_, #ssh_msg_kex_dh_gex_group{} = Msg, {key_exchange,client,ReNeg}, D) -> {ok, KexGexInit, Ssh} = ssh_transport:handle_kex_dh_gex_group(Msg, D#data.ssh_params), - ok = send_bytes(KexGexInit, D), + send_bytes(KexGexInit, D), {next_state, {key_exchange_dh_gex_reply,client,ReNeg}, D#data{ssh_params=Ssh}}; %%%---- elliptic curve diffie-hellman handle_event(_, #ssh_msg_kex_ecdh_init{} = Msg, {key_exchange,server,ReNeg}, D) -> {ok, KexEcdhReply, Ssh1} = ssh_transport:handle_kex_ecdh_init(Msg, D#data.ssh_params), - ok = send_bytes(KexEcdhReply, D), + send_bytes(KexEcdhReply, D), {ok, NewKeys, Ssh} = ssh_transport:new_keys_message(Ssh1), - ok = send_bytes(NewKeys, D), + send_bytes(NewKeys, D), {next_state, {new_keys,server,ReNeg}, D#data{ssh_params=Ssh}}; handle_event(_, #ssh_msg_kex_ecdh_reply{} = Msg, {key_exchange,client,ReNeg}, D) -> {ok, NewKeys, Ssh} = ssh_transport:handle_kex_ecdh_reply(Msg, D#data.ssh_params), - ok = send_bytes(NewKeys, D), + send_bytes(NewKeys, D), {next_state, {new_keys,client,ReNeg}, D#data{ssh_params=Ssh}}; @@ -640,9 +641,9 @@ handle_event(_, #ssh_msg_kex_ecdh_reply{} = Msg, {key_exchange,client,ReNeg}, D) handle_event(_, #ssh_msg_kex_dh_gex_init{} = Msg, {key_exchange_dh_gex_init,server,ReNeg}, D) -> {ok, KexGexReply, Ssh1} = ssh_transport:handle_kex_dh_gex_init(Msg, D#data.ssh_params), - ok = send_bytes(KexGexReply, D), + send_bytes(KexGexReply, D), {ok, NewKeys, Ssh} = ssh_transport:new_keys_message(Ssh1), - ok = send_bytes(NewKeys, D), + send_bytes(NewKeys, D), {next_state, {new_keys,server,ReNeg}, D#data{ssh_params=Ssh}}; @@ -650,7 +651,7 @@ handle_event(_, #ssh_msg_kex_dh_gex_init{} = Msg, {key_exchange_dh_gex_init,serv handle_event(_, #ssh_msg_kex_dh_gex_reply{} = Msg, {key_exchange_dh_gex_reply,client,ReNeg}, D) -> {ok, NewKeys, Ssh1} = ssh_transport:handle_kex_dh_gex_reply(Msg, D#data.ssh_params), - ok = send_bytes(NewKeys, D), + send_bytes(NewKeys, D), {next_state, {new_keys,client,ReNeg}, D#data{ssh_params=Ssh1}}; @@ -662,7 +663,7 @@ handle_event(_, #ssh_msg_newkeys{} = Msg, {new_keys,Role,init}, D) -> Ssh = case Role of client -> {MsgReq, Ssh2} = ssh_auth:service_request_msg(Ssh1), - ok = send_bytes(MsgReq, D), + send_bytes(MsgReq, D), Ssh2; server -> Ssh1 @@ -680,7 +681,7 @@ handle_event(_, Msg = #ssh_msg_service_request{name=ServiceName}, StateName = {s "ssh-userauth" -> Ssh0 = #ssh{session_id=SessionId} = D#data.ssh_params, {ok, {Reply, Ssh}} = ssh_auth:handle_userauth_request(Msg, SessionId, Ssh0), - ok = send_bytes(Reply, D), + send_bytes(Reply, D), {next_state, {userauth,server}, D#data{ssh_params = Ssh}}; _ -> @@ -692,7 +693,7 @@ handle_event(_, Msg = #ssh_msg_service_request{name=ServiceName}, StateName = {s handle_event(_, #ssh_msg_service_accept{name = "ssh-userauth"}, {service_request,client}, #data{ssh_params = #ssh{service="ssh-userauth"} = Ssh0} = State) -> {Msg, Ssh} = ssh_auth:init_userauth_request_msg(Ssh0), - ok = send_bytes(Msg, State), + send_bytes(Msg, State), {next_state, {userauth,client}, State#data{auth_user = Ssh#ssh.user, ssh_params = Ssh}}; @@ -709,7 +710,7 @@ handle_event(_, %% Probably the very first userauth_request but we deny unauthorized login {not_authorized, _, {Reply,Ssh}} = ssh_auth:handle_userauth_request(Msg, Ssh0#ssh.session_id, Ssh0), - ok = send_bytes(Reply, D), + send_bytes(Reply, D), {keep_state, D#data{ssh_params = Ssh}}; {"ssh-connection", "ssh-connection", Method} -> @@ -719,7 +720,7 @@ handle_event(_, %% Yepp! we support this method case ssh_auth:handle_userauth_request(Msg, Ssh0#ssh.session_id, Ssh0) of {authorized, User, {Reply, Ssh}} -> - ok = send_bytes(Reply, D), + send_bytes(Reply, D), D#data.starter ! ssh_connected, connected_fun(User, Method, D), {next_state, {connected,server}, @@ -727,11 +728,11 @@ handle_event(_, ssh_params = Ssh#ssh{authenticated = true}}}; {not_authorized, {User, Reason}, {Reply, Ssh}} when Method == "keyboard-interactive" -> retry_fun(User, Reason, D), - ok = send_bytes(Reply, D), + send_bytes(Reply, D), {next_state, {userauth_keyboard_interactive,server}, D#data{ssh_params = Ssh}}; {not_authorized, {User, Reason}, {Reply, Ssh}} -> retry_fun(User, Reason, D), - ok = send_bytes(Reply, D), + send_bytes(Reply, D), {keep_state, D#data{ssh_params = Ssh}} end; false -> @@ -1512,7 +1513,8 @@ send_msg(Msg, State=#data{ssh_params=Ssh0}) when is_tuple(Msg) -> State#data{ssh_params=Ssh}. send_bytes(Bytes, #data{socket = Socket, transport_cb = Transport}) -> - Transport:send(Socket, Bytes). + _ = Transport:send(Socket, Bytes), + ok. handle_version({2, 0} = NumVsn, StrVsn, Ssh0) -> Ssh = counterpart_versions(NumVsn, StrVsn, Ssh0), -- cgit v1.2.3 From 646148180690881b0b8148705a15764a39538924 Mon Sep 17 00:00:00 2001 From: Erlang/OTP Date: Thu, 13 Oct 2016 16:17:10 +0200 Subject: Prepare release --- lib/ssh/doc/src/notes.xml | 17 +++++++++++++++++ lib/ssh/vsn.mk | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/ssh/doc/src/notes.xml b/lib/ssh/doc/src/notes.xml index f6ad8d8dea..773a472818 100644 --- a/lib/ssh/doc/src/notes.xml +++ b/lib/ssh/doc/src/notes.xml @@ -30,6 +30,23 @@ notes.xml +
Ssh 4.3.5 + +
Fixed Bugs and Malfunctions + + +

+ If a client illegaly sends an info-line and then + immediatly closes the TCP-connection, a badmatch + exception was raised.

+

+ Own Id: OTP-13966

+ + +
+ +
+
Ssh 4.3.4
Fixed Bugs and Malfunctions diff --git a/lib/ssh/vsn.mk b/lib/ssh/vsn.mk index 536e559514..7eeeaf796e 100644 --- a/lib/ssh/vsn.mk +++ b/lib/ssh/vsn.mk @@ -1,5 +1,5 @@ #-*-makefile-*- ; force emacs to enter makefile-mode -SSH_VSN = 4.3.4 +SSH_VSN = 4.3.5 APP_VSN = "ssh-$(SSH_VSN)" -- cgit v1.2.3