From c0f3a96089613ada8cb97de84ce071aac1efe9d6 Mon Sep 17 00:00:00 2001 From: Jon Simons Date: Wed, 24 Apr 2019 10:41:19 -0700 Subject: [PATCH] server: fix queued USERAUTH_SUCCESS rekey bug Fix a bug with server-side rekeying where the session state at hand has been toggled to SSH_SESSION_STATE_AUTHENTICATED before performing the packet send of the SSH2_MSG_USERAUTH_SUCCESS message. Before this change, what can happen is that during the packet send, the SSH2_MSG_USERAUTH_SUCCESS message can end up being queued due to a small rekey data limit value. libssh server will then proceed to attempt to send KEX-related rekeying messages to the client before the client has received USERAUTH_SUCCESS. OpenSSH clients do not expect to undergo rekeying before having been authenticated, and so will exit with error when this happens. The behavior before and after can be observed with the pkd test making use of its new --rekey flag: ./pkd_hello -t torture_pkd_openssh_rsa_rsa_default -i1 --rekey=16 -v -v -v A new CMake test entry is added for the above variation and can be run with: ARGS="-R pkd_hello_rekey" make test Before the fix, the test will fail; after, the test succeeds while performing rekeying once every 16 bytes. Signed-off-by: Jon Simons Reviewed-by: Andreas Schneider --- src/server.c | 12 +++++++++--- tests/pkd/CMakeLists.txt | 5 +++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/server.c b/src/server.c index 91de94a5..2d7e91f1 100644 --- a/src/server.c +++ b/src/server.c @@ -895,9 +895,6 @@ int ssh_auth_reply_success(ssh_session session, int partial) return ssh_auth_reply_default(session, partial); } - session->session_state = SSH_SESSION_STATE_AUTHENTICATED; - session->flags |= SSH_SESSION_FLAG_AUTHENTICATED; - r = ssh_buffer_add_u8(session->out_buffer,SSH2_MSG_USERAUTH_SUCCESS); if (r < 0) { return SSH_ERROR; @@ -905,6 +902,15 @@ int ssh_auth_reply_success(ssh_session session, int partial) r = ssh_packet_send(session); + /* + * Consider the session as having been authenticated only after sending + * the USERAUTH_SUCCESS message. Setting these flags after ssh_packet_send + * ensures that a rekey is not triggered prematurely, causing the message + * to be queued. + */ + session->session_state = SSH_SESSION_STATE_AUTHENTICATED; + session->flags |= SSH_SESSION_FLAG_AUTHENTICATED; + crypto = ssh_packet_get_current_crypto(session, SSH_DIRECTION_OUT); if (crypto != NULL && crypto->delayed_compress_out) { SSH_LOG(SSH_LOG_PROTOCOL, "Enabling delayed compression OUT"); diff --git a/tests/pkd/CMakeLists.txt b/tests/pkd/CMakeLists.txt index 259d83ab..71355b08 100644 --- a/tests/pkd/CMakeLists.txt +++ b/tests/pkd/CMakeLists.txt @@ -38,6 +38,11 @@ target_link_libraries(pkd_hello ${pkd_libs}) # add_test(pkd_hello_i1 ${CMAKE_CURRENT_BINARY_DIR}/pkd_hello -e -o -i1 -w /tmp/pkd_socket_wrapper_XXXXXX) +# +# pkd_hello_rekey is used to test server-side implementation of rekeying. +# +add_test(pkd_hello_rekey ${CMAKE_CURRENT_BINARY_DIR}/pkd_hello -t torture_pkd_openssh_rsa_rsa_default -i1 --rekey=16 -v -v -v -w /tmp/pkd_socket_wrapper_XXXXXX) + # # Configure environment for cwrap socket wrapper. #