ssh: key exchange robustness improvements

u3s · u3s · commit 548f1295d86d · 2025-08-27T11:27:40.000+02:00
- reduce untrusted data processing for non-debug logs
- trim badmatch exceptions to avoid processing potentially malicious data
- terminate with kexinit_error when too many algorithms are received in KEX init message
diff --git a/lib/ssh/src/ssh_connection.erl b/lib/ssh/src/ssh_connection.erl
@@ -481,10 +481,9 @@ handle_msg(Msg, Connection, server, Ssh = #ssh{authenticated = false}) ->
     %% respond by disconnecting, preferably with a proper disconnect message
     %% sent to ease troubleshooting.
     MsgFun = fun(M) ->
-                     MaxLogItemLen = ?GET_OPT(max_log_item_len, Ssh#ssh.opts),
                      io_lib:format("Connection terminated. Unexpected message for unauthenticated user."
                                    " Message:  ~w", [M],
-                                   [{chars_limit, MaxLogItemLen}])
+                                   [{chars_limit, ssh_lib:max_log_len(Ssh)}])
              end,
     ?LOG_DEBUG(MsgFun, [Msg]),
     {disconnect, {?SSH_DISCONNECT_PROTOCOL_ERROR, "Connection refused"}, handle_stop(Connection)};
diff --git a/lib/ssh/src/ssh_connection_handler.erl b/lib/ssh/src/ssh_connection_handler.erl
@@ -1182,12 +1182,21 @@ handle_event(info, {Proto, Sock, NewData}, StateName,
                                       {next_event, internal, Msg}
 				    ]}
 	    catch
-		C:E:ST  ->
-                    MaxLogItemLen = ?GET_OPT(max_log_item_len,SshParams#ssh.opts),
+		Class:Reason0:Stacktrace  ->
+                    Reason = ssh_lib:trim_reason(Reason0),
+                    MsgFun =
+                        fun(debug) ->
+                                io_lib:format("Bad packet: Decrypted, but can't decode~n~p:~p~n~p",
+                                              [Class,Reason,Stacktrace],
+                                              [{chars_limit, ssh_lib:max_log_len(SshParams)}]);
+                           (_) ->
+                                io_lib:format("Bad packet: Decrypted, but can't decode ~p:~p",
+                                              [Class, Reason],
+                                              [{chars_limit, ssh_lib:max_log_len(SshParams)}])
+                        end,
                     {Shutdown, D} =
                         ?send_disconnect(?SSH_DISCONNECT_PROTOCOL_ERROR,
-                                         io_lib:format("Bad packet: Decrypted, but can't decode~n~p:~p~n~p",
-                                                       [C,E,ST], [{chars_limit, MaxLogItemLen}]),
+                                         ?SELECT_MSG(MsgFun),
                                          StateName, D1),
                     {stop, Shutdown, D}
 	    end;
@@ -1217,12 +1226,20 @@ handle_event(info, {Proto, Sock, NewData}, StateName,
                                  StateName, D0),
             {stop, Shutdown, D}
     catch
-	C:E:ST ->
-            MaxLogItemLen = ?GET_OPT(max_log_item_len,SshParams#ssh.opts),
+	Class:Reason0:Stacktrace ->
+            MsgFun =
+                fun(debug) ->
+                        io_lib:format("Bad packet: Couldn't decrypt~n~p:~p~n~p",
+                                      [Class,Reason0,Stacktrace],
+                                      [{chars_limit, ssh_lib:max_log_len(SshParams)}]);
+                   (_) ->
+                        Reason = ssh_lib:trim_reason(Reason0),
+                        io_lib:format("Bad packet: Couldn't decrypt~n~p:~p",
+                                      [Class,Reason],
+                                      [{chars_limit, ssh_lib:max_log_len(SshParams)}])
+                end,
             {Shutdown, D} =
-                ?send_disconnect(?SSH_DISCONNECT_PROTOCOL_ERROR,
-                                 io_lib:format("Bad packet: Couldn't decrypt~n~p:~p~n~p",
-                                               [C,E,ST], [{chars_limit, MaxLogItemLen}]),
+                ?send_disconnect(?SSH_DISCONNECT_PROTOCOL_ERROR, ?SELECT_MSG(MsgFun),
                                  StateName, D0),
             {stop, Shutdown, D}
     end;
diff --git a/lib/ssh/src/ssh_lib.erl b/lib/ssh/src/ssh_lib.erl
@@ -28,7 +28,9 @@
          format_address_port/2, format_address_port/1,
          format_address/1,
          format_time_ms/1,
-         comp/2
+         comp/2,
+         trim_reason/1,
+         max_log_len/1
         ]).
 
 -include("ssh.hrl").
@@ -86,3 +88,14 @@ comp([], [], Truth) ->
 
 comp(_, _, _) ->
     false.
+%% We don't want to process badmatch details, potentially containing
+%% malicious data of unknown size
+trim_reason({badmatch, V}) when is_binary(V) ->
+    badmatch;
+trim_reason(E) ->
+    E.
+
+max_log_len(#ssh{opts = Opts}) ->
+    ?GET_OPT(max_log_item_len, Opts);
+max_log_len(Opts) when is_map(Opts) ->
+    ?GET_OPT(max_log_item_len, Opts).
diff --git a/lib/ssh/src/ssh_message.erl b/lib/ssh/src/ssh_message.erl
@@ -43,7 +43,7 @@
 
 -behaviour(ssh_dbg).
 -export([ssh_dbg_trace_points/0, ssh_dbg_flags/1, ssh_dbg_on/1, ssh_dbg_off/1, ssh_dbg_format/2]).
--define(ALG_NAME_LIMIT, 64).
+-define(ALG_NAME_LIMIT, 64). % RFC4251 sec6
 
 ucl(B) ->
     try unicode:characters_to_list(B) of
@@ -821,23 +821,33 @@ decode_kex_init(<<?BYTE(Bool)>>, Acc, 0) ->
     %% See rfc 4253 7.1
     X = 0,
     list_to_tuple(lists:reverse([X, erl_boolean(Bool) | Acc]));
-decode_kex_init(<<?DEC_BIN(Data,__0), Rest/binary>>, Acc, N) ->
+decode_kex_init(<<?DEC_BIN(Data,__0), Rest/binary>>, Acc, N) when
+      byte_size(Data) < ?MAX_NUM_ALGORITHMS * ?ALG_NAME_LIMIT ->
     BinParts = binary:split(Data, <<$,>>, [global]),
-    Process =
-        fun(<<>>, PAcc) ->
-                PAcc;
-           (Part, PAcc) ->
-                case byte_size(Part) > ?ALG_NAME_LIMIT of
-                    true ->
-                        ?LOG_DEBUG("Ignoring too long name", []),
+    AlgCount = length(BinParts),
+    case AlgCount =< ?MAX_NUM_ALGORITHMS of
+        true ->
+            Process =
+                fun(<<>>, PAcc) ->
                         PAcc;
-                    false ->
-                        Name = binary:bin_to_list(Part),
-                        [Name | PAcc]
-                end
-        end,
-    Names = lists:foldr(Process, [], BinParts),
-    decode_kex_init(Rest, [Names | Acc], N - 1).
+                   (Part, PAcc) ->
+                        case byte_size(Part) =< ?ALG_NAME_LIMIT of
+                            true ->
+                                Name = binary:bin_to_list(Part),
+                                [Name | PAcc];
+                            false ->
+                                ?LOG_DEBUG("Ignoring too long name", []),
+                                PAcc
+                        end
+                end,
+            Names = lists:foldr(Process, [], BinParts),
+            decode_kex_init(Rest, [Names | Acc], N - 1);
+        false ->
+            throw({error, {kexinit_error, N, {alg_count, AlgCount}}})
+    end;
+decode_kex_init(<<?DEC_BIN(Data,__0), _Rest/binary>>, _Acc, N) ->
+    throw({error, {kexinit, N, {string_size, byte_size(Data)}}}).
+
 
 
 %%%================================================================
diff --git a/lib/ssh/src/ssh_transport.erl b/lib/ssh/src/ssh_transport.erl
@@ -403,8 +403,9 @@ handle_kexinit_msg(#ssh_msg_kexinit{} = CounterPart, #ssh_msg_kexinit{} = Own,
 	    key_exchange_first_msg(Algos#alg.kex, 
 				   Ssh#ssh{algorithms = Algos})
     catch
-        Class:Error ->
-            Msg = kexinit_error(Class, Error, client, Own, CounterPart),
+        Class:Reason0 ->
+            Reason = ssh_lib:trim_reason(Reason0),
+            Msg = kexinit_error(Class, Reason, client, Own, CounterPart, Ssh),
             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED, Msg)
         end;
 
@@ -420,31 +421,38 @@ handle_kexinit_msg(#ssh_msg_kexinit{} = CounterPart, #ssh_msg_kexinit{} = Own,
 	Algos ->
             {ok, Ssh#ssh{algorithms = Algos}}
     catch
-        Class:Error ->
-            Msg = kexinit_error(Class, Error, server, Own, CounterPart),
+        Class:Reason0 ->
+            Reason = ssh_lib:trim_reason(Reason0),
+            Msg = kexinit_error(Class, Reason, server, Own, CounterPart, Ssh),
             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED, Msg)
     end.
 
-kexinit_error(Class, Error, Role, Own, CounterPart) ->
+kexinit_error(Class, Error, Role, Own, CounterPart, Ssh) ->
     {Fmt,Args} =
         case {Class,Error} of
             {error, {badmatch,{false,Alg}}} ->
                 {Txt,W,C} = alg_info(Role, Alg),
-                {"No common ~s algorithm,~n"
-                 "  we have:~n    ~s~n"
-                 "  peer have:~n    ~s~n",
-                 [Txt,
-                  lists:join(", ", element(W,Own)),
-                  lists:join(", ", element(C,CounterPart))
-                 ]};
+                MsgFun =
+                    fun(debug) ->
+                            {"No common ~s algorithm,~n"
+                             "  we have:~n    ~s~n"
+                             "  peer have:~n    ~s~n",
+                             [Txt,
+                              lists:join(", ", element(W,Own)),
+                              lists:join(", ", element(C,CounterPart))]};
+                       (_) ->
+                            {"No common ~s algorithm", [Txt]}
+                    end,
+                ?SELECT_MSG(MsgFun);
             _ ->
                 {"Kexinit failed in ~p: ~p:~p", [Role,Class,Error]}
         end,
-    try io_lib:format(Fmt, Args) of
+    try io_lib:format(Fmt, Args, [{chars_limit, ssh_lib:max_log_len(Ssh)}]) of
         R -> R
     catch
         _:_ ->
-            io_lib:format("Kexinit failed in ~p: ~p:~p", [Role, Class, Error])
+            io_lib:format("Kexinit failed in ~p: ~p:~p", [Role, Class, Error],
+                          [{chars_limit, ssh_lib:max_log_len(Ssh)}])
     end.
 
 alg_info(client, Alg) ->
@@ -596,14 +604,19 @@ handle_kexdh_init(#ssh_msg_kexdh_init{e = E},
                                              session_id = sid(Ssh1, H)}};
                 {error,unsupported_sign_alg} ->
                     ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                io_lib:format("Unsupported algorithm ~p", [SignAlg])
-                               )
+                                io_lib:format("Unsupported algorithm ~p", [SignAlg],
+                                              [{chars_limit, ssh_lib:max_log_len(Opts)}]))
             end;
 	true ->
-            ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+            MsgFun =
+                fun(debug) ->
                         io_lib:format("Kexdh init failed, received 'e' out of bounds~n  E=~p~n  P=~p",
-                                      [E,P])
-                       )
+                                      [E,P], [{chars_limit, ssh_lib:max_log_len(Opts)}]);
+                   (_) ->
+                        io_lib:format("Kexdh init failed, received 'e' out of bounds", [],
+                                      [{chars_limit, ssh_lib:max_log_len(Opts)}] )
+                end,
+            ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED, ?SELECT_MSG(MsgFun))
     end.
 
 handle_kexdh_reply(#ssh_msg_kexdh_reply{public_host_key = PeerPubHostKey,
@@ -624,14 +637,15 @@ handle_kexdh_reply(#ssh_msg_kexdh_reply{public_host_key = PeerPubHostKey,
                                                              session_id = sid(Ssh, H)})};
 		Error ->
                     ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                io_lib:format("Kexdh init failed. Verify host key: ~p",[Error])
+                                io_lib:format("Kexdh init failed. Verify host key: ~p",[Error],
+                                              [{chars_limit, ssh_lib:max_log_len(Ssh0)}])
                                )
 	    end;
 
 	true ->
             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
                         io_lib:format("Kexdh init failed, received 'f' out of bounds~n  F=~p~n  P=~p",
-                                      [F,P])
+                                      [F,P], [{chars_limit, ssh_lib:max_log_len(Ssh0)}])
                        )
     end.
 
@@ -657,7 +671,8 @@ handle_kex_dh_gex_request(#ssh_msg_kex_dh_gex_request{min = Min0,
 		    }};
 	{error,_} ->
             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                        io_lib:format("No possible diffie-hellman-group-exchange group found",[])
+                        io_lib:format("No possible diffie-hellman-group-exchange group found",[],
+                                      [{chars_limit, ssh_lib:max_log_len(Opts)}])
                        )
     end;
 
@@ -689,8 +704,8 @@ handle_kex_dh_gex_request(#ssh_msg_kex_dh_gex_request_old{n = NBits},
 		    }};
 	{error,_} ->
             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                        io_lib:format("No possible diffie-hellman-group-exchange group found",[])
-                       )
+                        io_lib:format("No possible diffie-hellman-group-exchange group found",[],
+                                      [{chars_limit, ssh_lib:max_log_len(Opts)}]))
     end;
 
 handle_kex_dh_gex_request(_, _) ->
@@ -716,7 +731,6 @@ handle_kex_dh_gex_group(#ssh_msg_kex_dh_gex_group{p = P, g = G}, Ssh0) ->
     {Public, Private} = generate_key(dh, [P,G,2*Sz]),
     {SshPacket, Ssh1} = 
 	ssh_packet(#ssh_msg_kex_dh_gex_init{e = Public}, Ssh0),	% Pub = G^Priv mod P (def)
-
     {ok, SshPacket, 
      Ssh1#ssh{keyex_key = {{Private, Public}, {G, P}}}}.
 
@@ -747,19 +761,22 @@ handle_kex_dh_gex_init(#ssh_msg_kex_dh_gex_init{e = E},
                                                    }};
                         {error,unsupported_sign_alg} ->
                             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                        io_lib:format("Unsupported algorithm ~p", [SignAlg])
-                                       )
+                                        io_lib:format("Unsupported algorithm ~p", [SignAlg],
+                                                     [{chars_limit, ssh_lib:max_log_len(Opts)}]))
                     end;
 		true ->
                     ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                "Kexdh init failed, received 'k' out of bounds"
-                               )
+                                "Kexdh init failed, received 'k' out of bounds")
 	    end;
 	true ->
-            ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                        io_lib:format("Kexdh gex init failed, received 'e' out of bounds~n  E=~p~n  P=~p",
-                                      [E,P])
-                       )
+            MsgFun =
+                fun(debug) ->
+                        io_lib:format("Kexdh gex init failed, received 'e' out of bounds~n"
+                                      "  E=~p~n  P=~p", [E,P]);
+                   (_) ->
+                        io_lib:format("Kexdh gex init failed, received 'e' out of bounds", [])
+                end,
+            ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED, ?SELECT_MSG(MsgFun))
     end.
 
 handle_kex_dh_gex_reply(#ssh_msg_kex_dh_gex_reply{public_host_key = PeerPubHostKey, 
@@ -784,20 +801,18 @@ handle_kex_dh_gex_reply(#ssh_msg_kex_dh_gex_reply{public_host_key = PeerPubHostK
                                                                      session_id = sid(Ssh, H)})};
                         Error ->
                             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                        io_lib:format("Kexdh gex reply failed. Verify host key: ~p",[Error])
-                                       )
+                                        io_lib:format("Kexdh gex reply failed. Verify host key: ~p",
+                                                      [Error], [{chars_limit, ssh_lib:max_log_len(Ssh0)}]))
 		    end;
 
 		true ->
                     ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                "Kexdh gex init failed, 'K' out of bounds"
-                               )
+                                "Kexdh gex init failed, 'K' out of bounds")
 	    end;
 	true ->
             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
                         io_lib:format("Kexdh gex init failed, received 'f' out of bounds~n  F=~p~n  P=~p",
-                                      [F,P])
-                       )
+                                      [F,P], [{chars_limit, ssh_lib:max_log_len(Ssh0)}]))
     end.
 
 %%%----------------------------------------------------------------
@@ -831,17 +846,25 @@ handle_kex_ecdh_init(#ssh_msg_kex_ecdh_init{q_c = PeerPublic},
                                              session_id = sid(Ssh1, H)}};
                 {error,unsupported_sign_alg} ->
                     ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                io_lib:format("Unsupported algorithm ~p", [SignAlg])
-                               )
+                                io_lib:format("Unsupported algorithm ~p", [SignAlg],
+                                             [{chars_limit, ssh_lib:max_log_len(Opts)}]))
             end
     catch
-        Class:Error ->
-            ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
+        Class:Reason0 ->
+            Reason = ssh_lib:trim_reason(Reason0),
+            MsgFun =
+                fun(debug) ->
                         io_lib:format("ECDH compute key failed in server: ~p:~p~n"
                                       "Kex: ~p, Curve: ~p~n"
                                       "PeerPublic: ~p",
-                                      [Class,Error,Kex,Curve,PeerPublic])
-                       )
+                                      [Class,Reason,Kex,Curve,PeerPublic],
+                                      [{chars_limit, ssh_lib:max_log_len(Ssh0)}]);
+                   (_) ->
+                        io_lib:format("ECDH compute key failed in server: ~p:~p",
+                                      [Class,Reason],
+                                      [{chars_limit, ssh_lib:max_log_len(Ssh0)}])
+                end,
+            ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED, ?SELECT_MSG(MsgFun))
     end.
 
 handle_kex_ecdh_reply(#ssh_msg_kex_ecdh_reply{public_host_key = PeerPubHostKey,
@@ -864,15 +887,14 @@ handle_kex_ecdh_reply(#ssh_msg_kex_ecdh_reply{public_host_key = PeerPubHostKey,
                                                              session_id = sid(Ssh, H)})};
 		Error ->
                     ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
-                                io_lib:format("ECDH reply failed. Verify host key: ~p",[Error])
-                               )
+                                io_lib:format("ECDH reply failed. Verify host key: ~p",[Error],
+                                              [{chars_limit, ssh_lib:max_log_len(Ssh0)}]))
 	    end
     catch
         Class:Error ->
             ?DISCONNECT(?SSH_DISCONNECT_KEY_EXCHANGE_FAILED,
                         io_lib:format("Peer ECDH public key seem invalid: ~p:~p",
-                                      [Class,Error])
-                       )
+                                      [Class,Error], [{chars_limit, ssh_lib:max_log_len(Ssh0)}]))
     end.
 
 
diff --git a/lib/ssh/test/ssh_connection_SUITE.erl b/lib/ssh/test/ssh_connection_SUITE.erl
