Merge branch 'kuba/ssh/kex_robustness_fix/OTP-19741' into maint-28

Erlang/OTP · Erlang/OTP · commit 634fb34a26e6 · 2025-09-10T15:15:43.000+02:00
* kuba/ssh/kex_robustness_fix/OTP-19741:
  ssh: refactor select_all function
  ssh: key exchange robustness improvements
diff --git a/lib/ssh/src/ssh_connection.erl b/lib/ssh/src/ssh_connection.erl
@@ -766,10 +766,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
@@ -1188,12 +1188,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;
@@ -1223,12 +1232,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
@@ -33,7 +33,9 @@
          format_time_ms/1,
          comp/2,
          set_label/1,
-         set_label/2
+         set_label/2,
+         trim_reason/1,
+         max_log_len/1
         ]).
 
 -include("ssh.hrl").
@@ -99,3 +101,17 @@ set_label(client, Details) ->
     proc_lib:set_label({sshc, Details});
 set_label(server, Details) ->
     proc_lib:set_label({sshd, Details}).
+
+%% 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
@@ -46,7 +46,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
@@ -827,23 +827,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
diff --git a/lib/ssh/test/ssh_connection_SUITE.erl b/lib/ssh/test/ssh_connection_SUITE.erl
