ssh: Receiving SSH_MSG_USERAUTH_BANNER causes a protocol error

[alexandrejbr]

Describe the bug
RFC4252 (https://www.ietf.org/rfc/rfc4252.txt) states that:
"The SSH server may send an SSH_MSG_USERAUTH_BANNER message at any
time after this authentication protocol starts and before
authentication is successful."

While the most common use case, and the only one that openssh daemon uses, is to send the banner as a response to the first userauth message we manage to find some implementations that use this phrasing of the RFC to send a banner just before the authentication terminates in order to notify the user that the password is about to expiry.

OpenSSH accepts this without a problem but the erlang ssh client implementation doesn't handle this and we end up in the catch all clause of the ssh_connection_handler which will terminate the connection with protocol error.

To Reproduce
In order to reproduce this one can change the erlang server implementation by applying the following patch:

diff --git a/lib/ssh/src/ssh_fsm_userauth_server.erl b/lib/ssh/src/ssh_fsm_userauth_server.erl
index 8d0710d..2455c0f 100644
--- a/lib/ssh/src/ssh_fsm_userauth_server.erl
+++ b/lib/ssh/src/ssh_fsm_userauth_server.erl
@@ -129,7 +129,10 @@ handle_event(internal,
 handle_event(internal, #ssh_msg_userauth_info_response{} = Msg, {userauth_keyboard_interactive, server}, D0) ->
     case ssh_auth:handle_userauth_info_response(Msg, D0#data.ssh_params) of
        {authorized, User, {Reply, Ssh1}} ->
-            D = connected_state(Reply, Ssh1, User, "keyboard-interactive", D0),
+            Banner = #ssh_msg_userauth_banner{message = <<"Real programmers don't eat quiche">>,
+                                              language = <<"EN">>},
+            D1 = ssh_connection_handler:send_msg(Banner, D0#data{ssh_params = Ssh1}),
+            D = connected_state(Reply, D1#data.ssh_params, User, "keyboard-interactive", D1),
             {next_state, {connected,server}, D,
              start_alive(D, [set_max_initial_idle_timeout(D),
               {change_callback_module,ssh_connection_handler}
@@ -147,7 +150,10 @@ handle_event(internal, #ssh_msg_userauth_info_response{} = Msg, {userauth_keyboa
 handle_event(internal, #ssh_msg_userauth_info_response{} = Msg, {userauth_keyboard_interactive_extra, server}, D0) ->
     {authorized, User, {Reply, Ssh1}} =
         ssh_auth:handle_userauth_info_response({extra,Msg}, D0#data.ssh_params),
-    D = connected_state(Reply, Ssh1, User, "keyboard-interactive", D0),
+    Banner = #ssh_msg_userauth_banner{message = <<"Real programmers don't eat quiche">>,
+                                      language = <<"EN">>},
+    D1 = ssh_connection_handler:send_msg(Banner, D0#data{ssh_params = Ssh1}),
+    D = connected_state(Reply, D1#data.ssh_params, User, "keyboard-interactive", D1),
     {next_state, {connected,server}, D,
      start_alive(D, [set_max_initial_idle_timeout(D),
       {change_callback_module,ssh_connection_handler}

Then if you force the erlang client and server to user keyboard-interactive as authentication method you'll see the protocol error.

Expected behavior
I believe it's not harmful for the client to just process any banner during userauth so something like this could make sense:

diff --git a/lib/ssh/src/ssh_fsm_userauth_client.erl b/lib/ssh/src/ssh_fsm_userauth_client.erl
index 0cf9508..3570eea 100644
--- a/lib/ssh/src/ssh_fsm_userauth_client.erl
+++ b/lib/ssh/src/ssh_fsm_userauth_client.erl
@@ -108,7 +108,7 @@ handle_event(internal, #ssh_msg_userauth_failure{authentications = Methods}, Sta
     end;
 
 %%---- banner to client
-handle_event(internal, #ssh_msg_userauth_banner{message = Msg}, {userauth,client}, D) ->
+handle_event(internal, #ssh_msg_userauth_banner{message = Msg}, {_,_}, D) ->
     case D#data.ssh_params#ssh.userauth_quiet_mode of
        false -> io:format("~s", [Msg]);
        true -> ok

If you want to be more conservative one can just process the banner in a slimmer set of states.

Affected versions
At least from OTP 22 until OTP 27, probably earlier

[alexandrejbr]

In order to make this easier to test one could add an undocumented feature to the server role to allow that a banner can be sent in the situations of the reproduction. Maybe in the future could be helpful for somebody and if so we could document it.

[u3s]

we planned looking on that password expiry scenario.
thanks for the report.

have you used some other ssh server implementation for verifying this? which implementations are sending this banner and are on your list?

asking so that we can use it as a reference during development and testing.

[alexandrejbr]

I don't know the technologies behind it, but we have reports of this from devices from Huawei and Affirmed. We have the openssh log from those devices and it's something like this:

debug1: Next authentication method: keyboard-interactive
debug2: userauth_kbdint
debug3: send packet: type 50
debug2: we sent a keyboard-interactive packet, wait for reply
debug3: receive packet: type 60
debug2: input_userauth_info_req: entering
debug2: input_userauth_info_req: num_prompts 1
([email protected]) Password:
debug3: send packet: type 61
debug3: receive packet: type 60
debug2: input_userauth_info_req: entering
debug2: input_userauth_info_req: num_prompts 0
debug3: send packet: type 61
debug3: receive packet: type 53
debug3: input_userauth_banner: entering
Password will expire in 14 days
Please change your password at https://blablabla.com
debug3: receive packet: type 52
Authenticated to 0.0.0.0 ([0.0.0.0]:22) using "keyboard-
interactive". 

I confirmed with the openssh client that the patched Erlang server works good. So the hacked erlang server is doing something reasonable.

I've seen that golang has a proposal of an implementation to allow the server to send banners in more scenarios, but I haven't explored if that proposal would be usable for reproduction.

As I mentioned before, I think the easiest way of having tests for this is to have the erlang server sending userauth banners. It could be a callback called auth_successful_banner. I understand that you'd like to validate this with other implementations but unfortunately I haven't found any so I can't help with that. I can help with the implementation of the fix and the testing, as long as we agree on what we want/need.

[u3s]

I can't help with the implementation of the fix and the testing, as long as we agree on what we want/need.

? can or can't ? :-)
give us some time for figuring out what we need in our sidetrack activities. i will come back with information.

[alexandrejbr]

"can" of course :D. I will edit the original message to make it easier for future readers.

[u3s]

If you want to be more conservative one can just process the banner in a slimmer set of states.

Yes we would like to be more conservative and check explicitly for state name (I guess it is only keyboard-interactive missing). Also including client atom would be wanted.
PR on that would be very welcome.

Can you capture ssh_dbg output for devices causing problems and attach it to the issue?

For testing, I'm not yet sure if it is worth having undocumented option just for that. Let us pause this for a while.
Instead maybe you could have a look on ssh_trpt_test_lib module, to see if this can be used for preparing a testcase (maybe in ssh_protocol_SUITE?

[alexandrejbr]

That’s how I ended up implementing internally. I’ll open the PR shortly.

I don’t have access to any of thoses devices unfortunately so I can’t provide you any more than the openssh debug output.

With regards to the option to send a banner I have realised we have an internal patch to be able to send a banner at the start of the authentication (just like openssh can do). I am happy to contribute that feature and augment it with the banner at the end of the authentication. It’s perhaps better if I just open a draft PR with it so you can have a look if it’s interesting, it’s quite simple.

I will explore that alternative that you suggested for testing of the PR I am opening to solve this issue.

[u3s]

With regards to the option to send a banner I have realised we have an internal patch to be able to send a banner at the start of the authentication (just like openssh can do). I am happy to contribute that feature and augment it with the banner at the end of the authentication. It’s perhaps better if I just open a draft PR with it so you can have a look if it’s interesting, it’s quite simple.

Yes. If you have code at hand, it would be great to have a look and decide then. Typically we're interested with aligning functionality with OpenSSH.

I will explore that alternative that you suggested for testing of the PR I am opening to solve this issue.

ssh_trpt_test_lib is a bit hairy at first. but allows verifying custom scenarios.

[alexandrejbr]

@u3s I have opened #9149 that adds enables the server to send a SSH_MSG_USERAUTH_BANNER at the beginning of the authentication. I realised our implementation was not 100% correct so I add to re-implement the feature.

If you want I am happy to extend it with the possibility of sending the SSH_MSG_USERAUTH_BANNER at the end of the authentication. I would use 2 separate functions for it, pre_userauth_bannerfun and post_userauth_bannerfun.

[alexandrejbr]

I will now look at ssh_trpt_test_lib to see if I can create a test for #9139