Merge branch 'hans/ssh/check_host_user_keys/OTP-14676' into maint

7 files changed, 112 insertions, 52 deletions

diff --git a/lib/ssh/src/ssh.erl b/lib/ssh/src/ssh.erl
index 1a5d48baca..032d87bdad 100644
--- a/lib/ssh/src/ssh.erl
+++ b/lib/ssh/src/ssh.erl
@@ -188,6 +188,7 @@ daemon(Port) ->
 daemon(Socket, UserOptions) when is_port(Socket) ->
     try
         #{} = Options = ssh_options:handle_options(server, UserOptions),
+
         case valid_socket_to_use(Socket, ?GET_OPT(transport,Options)) of
             ok ->
                 {ok, {IP,Port}} = inet:sockname(Socket),
@@ -461,6 +462,9 @@ open_listen_socket(_Host0, Port0, Options0) ->
 %%%----------------------------------------------------------------
 finalize_start(Host, Port, Profile, Options0, F) ->
     try
+        %% throws error:Error if no usable hostkey is found
+        ssh_connection_handler:available_hkey_algorithms(server, Options0),
+
         sshd_sup:start_child(Host, Port, Profile, Options0)
     of
         {error, {already_started, _}} ->
@@ -470,6 +474,8 @@ finalize_start(Host, Port, Profile, Options0, F) ->
         Result = {ok,_} ->
             F(Options0, Result)
     catch
+        error:{shutdown,Err} ->
+            {error,Err};
         exit:{noproc, _} ->
             {error, ssh_not_started}
     end.
diff --git a/lib/ssh/src/ssh_auth.erl b/lib/ssh/src/ssh_auth.erl
index ac64a7bf14..894877f8bf 100644
--- a/lib/ssh/src/ssh_auth.erl
+++ b/lib/ssh/src/ssh_auth.erl
@@ -145,14 +145,17 @@ get_public_key(SigAlg, #ssh{opts = Opts}) ->
     case KeyCb:user_key(KeyAlg, [{key_cb_private,KeyCbOpts}|UserOpts]) of
         {ok, PrivKey} ->
             try
+                %% Check the key - the KeyCb may be a buggy plugin
+                true = ssh_transport:valid_key_sha_alg(PrivKey, KeyAlg),
                 Key = ssh_transport:extract_public_key(PrivKey),
                 public_key:ssh_encode(Key, ssh2_pubkey)
             of
                 PubKeyBlob -> {ok,{PrivKey,PubKeyBlob}}
             catch
                 _:_ -> 
-		    not_ok
+                    not_ok
             end;
+
 	_Error ->
 	    not_ok
     end.
diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
index 4158a52a27..802bf62570 100644
--- a/lib/ssh/src/ssh_connection_handler.erl
+++ b/lib/ssh/src/ssh_connection_handler.erl
@@ -46,6 +46,7 @@
 
 %%% Internal application API
 -export([start_connection/4,
+         available_hkey_algorithms/2,
 	 open_channel/6,
 	 request/6, request/7,
 	 reply_request/3, 
@@ -432,13 +433,12 @@ init_ssh_record(Role, Socket, Opts) ->
     init_ssh_record(Role, Socket, PeerAddr, Opts).
 
 init_ssh_record(Role, _Socket, PeerAddr, Opts) ->
-    KeyCb = ?GET_OPT(key_cb, Opts),
     AuthMethods = ?GET_OPT(auth_methods, Opts),
     S0 = #ssh{role = Role,
-	      key_cb = KeyCb,
+	      key_cb = ?GET_OPT(key_cb, Opts),
 	      opts = Opts,
 	      userauth_supported_methods = AuthMethods,
-	      available_host_keys = supported_host_keys(Role, KeyCb, Opts),
+	      available_host_keys = available_hkey_algorithms(Role, Opts),
 	      random_length_padding = ?GET_OPT(max_random_length_padding, Opts)
 	   },
 
@@ -1544,44 +1544,42 @@ peer_role(client) -> server;
 peer_role(server) -> client.
 
 %%--------------------------------------------------------------------
-supported_host_keys(client, _, Options) ->
-    try
-        find_sup_hkeys(Options)
-    of
-	[] ->
+available_hkey_algorithms(Role, Options) ->
+    KeyCb = ?GET_OPT(key_cb, Options),
+    case [A || A <- available_hkey_algos(Options),
+               (Role==client) orelse available_host_key(KeyCb, A, Options)
+         ] of
+        
+        [] when Role==client ->
 	    error({shutdown, "No public key algs"});
-	Algs ->
-	    [atom_to_list(A) || A<-Algs]
-    catch
-	exit:Reason ->
-	    error({shutdown, Reason})
-    end;
-supported_host_keys(server, KeyCb, Options) ->
-    [atom_to_list(A) || A <- find_sup_hkeys(Options),
-			available_host_key(KeyCb, A, Options)
-    ].
 
+        [] when Role==server ->
+	    error({shutdown, "No host key available"});
 
-find_sup_hkeys(Options) ->
-    case proplists:get_value(public_key,
-                             ?GET_OPT(preferred_algorithms,Options)
-                            )
-    of
-        undefined ->
-            ssh_transport:default_algorithms(public_key);
-        L ->
-            NonSupported =  L--ssh_transport:supported_algorithms(public_key),
-            L -- NonSupported
+	Algs ->
+	    [atom_to_list(A) || A<-Algs]
     end.
 
 
+available_hkey_algos(Options) ->
+    SupAlgos = ssh_transport:supported_algorithms(public_key),
+    HKeys = proplists:get_value(public_key,
+                                ?GET_OPT(preferred_algorithms,Options)
+                               ),
+    NonSupported =  HKeys -- SupAlgos,
+    AvailableAndSupported = HKeys -- NonSupported,
+    AvailableAndSupported.
+
 
 %% Alg :: atom()
 available_host_key({KeyCb,KeyCbOpts}, Alg, Opts) ->
     UserOpts = ?GET_OPT(user_options, Opts),
     case KeyCb:host_key(Alg, [{key_cb_private,KeyCbOpts}|UserOpts]) of
-        {ok,_} -> true;
-        _ -> false
+        {ok,Key} ->
+            %% Check the key - the KeyCb may be a buggy plugin
+            ssh_transport:valid_key_sha_alg(Key, Alg);
+        _ ->
+            false
     end.
 
 
diff --git a/lib/ssh/src/ssh_transport.erl b/lib/ssh/src/ssh_transport.erl
index e92c727559..d8f7a96c15 100644
--- a/lib/ssh/src/ssh_transport.erl
+++ b/lib/ssh/src/ssh_transport.erl
@@ -795,8 +795,14 @@ get_host_key(SSH, SignAlg) ->
     #ssh{key_cb = {KeyCb,KeyCbOpts}, opts = Opts} = SSH,
     UserOpts = ?GET_OPT(user_options, Opts),
     case KeyCb:host_key(SignAlg, [{key_cb_private,KeyCbOpts}|UserOpts]) of
-	{ok, PrivHostKey} ->  PrivHostKey;
-	Result -> exit({error, {Result, unsupported_key_type}})
+	{ok, PrivHostKey} ->
+            %% Check the key - the KeyCb may be a buggy plugin
+            case valid_key_sha_alg(PrivHostKey, SignAlg) of
+                true -> PrivHostKey;
+                false -> exit({error, bad_hostkey})
+            end;
+	Result ->
+            exit({error, {Result, unsupported_key_type}})
     end.
 
 extract_public_key(#'RSAPrivateKey'{modulus = N, publicExponent = E}) ->
@@ -1830,11 +1836,14 @@ valid_key_sha_alg(#'RSAPrivateKey'{}, 'ssh-rsa'     ) -> true;
 valid_key_sha_alg({_, #'Dss-Parms'{}}, 'ssh-dss') -> true;
 valid_key_sha_alg(#'DSAPrivateKey'{},  'ssh-dss') -> true;
 
-valid_key_sha_alg({#'ECPoint'{},{namedCurve,OID}},                Alg) -> sha(OID) == sha(Alg);
-valid_key_sha_alg(#'ECPrivateKey'{parameters = {namedCurve,OID}}, Alg) -> sha(OID) == sha(Alg);
+valid_key_sha_alg({#'ECPoint'{},{namedCurve,OID}},                Alg) -> valid_key_sha_alg_ec(OID, Alg);
+valid_key_sha_alg(#'ECPrivateKey'{parameters = {namedCurve,OID}}, Alg) -> valid_key_sha_alg_ec(OID, Alg);
 valid_key_sha_alg(_, _) -> false.
     
-
+valid_key_sha_alg_ec(OID, Alg) -> 
+    Curve = public_key:oid2ssh_curvename(OID),
+    Alg == list_to_atom("ecdsa-sha2-" ++ binary_to_list(Curve)).
+    
 
 public_algo(#'RSAPublicKey'{}) ->   'ssh-rsa';  % FIXME: Not right with draft-curdle-rsa-sha2
 public_algo({_, #'Dss-Parms'{}}) -> 'ssh-dss';
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
index c07140dc43..19e2754eba 100644
--- a/lib/ssh/test/property_test/ssh_eqc_client_info_timing.erl
+++ b/lib/ssh/test/property_test/ssh_eqc_client_info_timing.erl
@@ -57,9 +57,9 @@
 
 %%% Properties:
 
-prop_seq(_Config) ->
+prop_seq(Config) ->
     {ok,Pid} = ssh_eqc_event_handler:add_report_handler(),
-    {_, _, Port} = init_daemon(),
+    {_, _, Port} = init_daemon(Config),
     numtests(1000,
 	     ?FORALL(Delay, choose(0,100),%% Micro seconds
 		     try 
@@ -86,7 +86,8 @@ any_relevant_error_report(Pid) ->
 	      end, Reports).
 
 %%%================================================================
-init_daemon() ->
+init_daemon(Config) ->
     ok = begin ssh:stop(), ssh:start() end,
-    ssh_test_lib:daemon([]).
+    DataDir = proplists:get_value(data_dir, Config),
+    ssh_test_lib:daemon([{system_dir,DataDir}]).
 
diff --git a/lib/ssh/test/ssh_basic_SUITE.erl b/lib/ssh/test/ssh_basic_SUITE.erl
index db2ae241e5..202b0afe57 100644
--- a/lib/ssh/test/ssh_basic_SUITE.erl
+++ b/lib/ssh/test/ssh_basic_SUITE.erl
@@ -46,6 +46,7 @@
 	 exec_key_differs2/1,
 	 exec_key_differs3/1,
 	 exec_key_differs_fail/1,
+         fail_daemon_start/1,
 	 idle_time_client/1,
 	 idle_time_server/1,
 	 inet6_option/1,
@@ -105,6 +106,7 @@ all() ->
      {group, host_user_key_differs},
      {group, key_cb},
      {group, internal_error},
+     {group, rsa_host_key_is_actualy_ecdsa},
      daemon_already_started,
      double_close,
      daemon_opt_fd,
@@ -121,6 +123,7 @@ groups() ->
      {ecdsa_sha2_nistp256_key, [], basic_tests()},
      {ecdsa_sha2_nistp384_key, [], basic_tests()},
      {ecdsa_sha2_nistp521_key, [], basic_tests()},
+     {rsa_host_key_is_actualy_ecdsa, [], [fail_daemon_start]},
      {host_user_key_differs, [], [exec_key_differs1,
 				  exec_key_differs2,
 				  exec_key_differs3,
@@ -180,6 +183,31 @@ init_per_group(rsa_key, Config) ->
 	false ->
 	    {skip, unsupported_pub_key}
     end;
+init_per_group(rsa_host_key_is_actualy_ecdsa, Config) ->
+    case 
+        lists:member('ssh-rsa',
+		      ssh_transport:default_algorithms(public_key)) and
+        lists:member('ecdsa-sha2-nistp256',
+                     ssh_transport:default_algorithms(public_key))
+    of
+	true ->
+            DataDir = proplists:get_value(data_dir, Config),
+            PrivDir = proplists:get_value(priv_dir, Config),
+	    ssh_test_lib:setup_ecdsa("256", DataDir, PrivDir),
+            %% The following sets up bad rsa keys:
+            begin
+                UserDir = PrivDir,
+                System = filename:join(UserDir, "system"),
+                file:copy(filename:join(DataDir, "id_rsa"), filename:join(UserDir, "id_rsa")),
+                file:rename(filename:join(System, "ssh_host_ecdsa_key"), filename:join(System, "ssh_host_rsa_key")),
+                file:rename(filename:join(System, "ssh_host_ecdsa_key.pub"), filename:join(System, "ssh_host_rsa_key.pub")),
+                ssh_test_lib:setup_rsa_known_host(DataDir, UserDir),
+                ssh_test_lib:setup_rsa_auth_keys(DataDir, UserDir)
+            end,
+            Config;
+	false ->
+	    {skip, unsupported_pub_key}
+    end;
 init_per_group(ecdsa_sha2_nistp256_key, Config) ->
     case lists:member('ecdsa-sha2-nistp256',
 		      ssh_transport:default_algorithms(public_key)) of
@@ -304,7 +332,8 @@ init_per_group(internal_error, Config) ->
     DataDir = proplists:get_value(data_dir, Config),
     PrivDir = proplists:get_value(priv_dir, Config),
     ssh_test_lib:setup_dsa(DataDir, PrivDir),
-    file:delete(filename:join(PrivDir, "system/ssh_host_dsa_key")),
+    %% In the test case the key will be deleted after the daemon start:
+    %% ... file:delete(filename:join(PrivDir, "system/ssh_host_dsa_key")),
     Config;
 init_per_group(dir_options, Config) ->
     PrivDir = proplists:get_value(priv_dir, Config),
@@ -868,12 +897,17 @@ key_callback_options(Config) when is_list(Config) ->
 %%% Test that client does not hang if disconnects due to internal error
 internal_error(Config) when is_list(Config) ->
     process_flag(trap_exit, true),
-    SystemDir = filename:join(proplists:get_value(priv_dir, Config), system),
+    PrivDir = proplists:get_value(priv_dir, Config),
     UserDir = proplists:get_value(priv_dir, Config),
+    SystemDir = filename:join(PrivDir, system),
     
     {Pid, Host, Port} = ssh_test_lib:daemon([{system_dir, SystemDir},
                                              {user_dir, UserDir},
                                              {failfun, fun ssh_test_lib:failfun/2}]),
+
+    %% Now provoke an error in the following connect:
+    file:delete(filename:join(PrivDir, "system/ssh_host_dsa_key")), 
+
     {error, Error} =
         ssh:connect(Host, Port, [{silently_accept_hosts, true},
                                  {user_dir, UserDir},
@@ -902,6 +936,17 @@ send(Config) when is_list(Config) ->
 
 
 %%--------------------------------------------------------------------
+%%%
+fail_daemon_start(Config) when is_list(Config) ->
+    process_flag(trap_exit, true),
+    SystemDir = filename:join(proplists:get_value(priv_dir, Config), system),
+    UserDir = proplists:get_value(priv_dir, Config),
+
+    {error,_} = ssh_test_lib:daemon([{system_dir, SystemDir},
+                                     {user_dir, UserDir},
+                                     {failfun, fun ssh_test_lib:failfun/2}]).
+
+%%--------------------------------------------------------------------
 %%% Test ssh:connection_info([peername, sockname])
 peername_sockname(Config) when is_list(Config) ->
     process_flag(trap_exit, true),
@@ -1300,14 +1345,11 @@ shell_exit_status(Config) when is_list(Config) ->
 %%--------------------------------------------------------------------
 %% Due to timing the error message may or may not be delivered to
 %% the "tcp-application" before the socket closed message is recived
-check_error("Invalid state") ->
-    ok;
-check_error("Connection closed") ->
-    ok;
-check_error("Selection of key exchange algorithm failed"++_) ->
-    ok;
-check_error(Error) ->
-    ct:fail(Error).
+check_error("Invalid state") -> ok;
+check_error("Connection closed") -> ok;
+check_error("Selection of key exchange algorithm failed"++_) -> ok;
+check_error("No host key available") -> ok;
+check_error(Error) -> ct:fail(Error).
 
 basic_test(Config) ->
     ClientOpts = proplists:get_value(client_opts, Config),
diff --git a/lib/ssh/test/ssh_protocol_SUITE.erl b/lib/ssh/test/ssh_protocol_SUITE.erl
index 74f802cf57..3e3e151781 100644
--- a/lib/ssh/test/ssh_protocol_SUITE.erl
+++ b/lib/ssh/test/ssh_protocol_SUITE.erl
@@ -630,11 +630,12 @@ client_handles_keyboard_interactive_0_pwds(Config) ->
 
 
 %%%--------------------------------------------------------------------
-client_info_line(_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([]),
+    DataDir = proplists:get_value(data_dir, Config),
+    {_, _, Port} = ssh_test_lib:daemon([{system_dir,DataDir}]),
 
     %% Fake client:
     {ok,S} = gen_tcp:connect("localhost",Port,[]),