diff options
author | Dirkjan Bussink <d.bussink@gmail.com> | 2020-12-10 13:20:34 +0000 |
---|---|---|
committer | Jakub Jelen <jjelen@redhat.com> | 2021-08-17 15:46:53 +0200 |
commit | 0a5b93e4791ae1bf3b0429fc80293c412a26e3bb (patch) | |
tree | 0891ef65c2da797d6d18a96935f209da25d4ea4e | |
parent | 761a4d5fa2577eda4cde699f8d71c1bbc40dc94d (diff) | |
download | libssh-0a5b93e4791ae1bf3b0429fc80293c412a26e3bb.tar.gz libssh-0a5b93e4791ae1bf3b0429fc80293c412a26e3bb.tar.xz libssh-0a5b93e4791ae1bf3b0429fc80293c412a26e3bb.zip |
Ignore request success and failure message if they are not expected
In https://gitlab.com/libssh/libssh-mirror/-/merge_requests/145#note_463232084
behavior in libssh was identified where it diverges from how for example
OpenSSH behaves. In OpenSSH if a request success of failure message is
received, apart from it being treated as a keepalive message, it is
ignored otherwise.
Libssh does handle the unexpected message and triggers an error
condition internally. This means that with the Dropbear behavior where
it replies to a hostkeys-00@openssh.com message even with a want_reply
= 0 (arguably a bug), libssh enters an error state.
This change makes the libssh behavior match OpenSSH to ignore these
messages. The spec is a bit unclear on whether Dropbear is buggy here or
not, but let's be liberal with the input accepted here in libssh.
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit f6a2f6190c2aa047d901547720ae6d1729e1e2c0)
-rw-r--r-- | src/packet.c | 34 | ||||
-rw-r--r-- | tests/unittests/torture_packet_filter.c | 96 |
2 files changed, 112 insertions, 18 deletions
diff --git a/src/packet.c b/src/packet.c index e9ae5648..0b784828 100644 --- a/src/packet.c +++ b/src/packet.c @@ -688,10 +688,12 @@ static enum ssh_packet_filter_result_e ssh_packet_incoming_filter(ssh_session se /* * States required: * - session_state == SSH_SESSION_STATE_AUTHENTICATED - * - session->global_req_state == SSH_CHANNEL_REQ_STATE_PENDING * * Transitions: - * - session->global_req_state == SSH_CHANNEL_REQ_STATE_ACCEPTED + * - From channel->request_state == SSH_CHANNEL_REQ_STATE_PENDING + * - To channel->request_state = SSH_CHANNEL_REQ_STATE_ACCEPTED + * + * If not in a pending state, message is ignored in the callback handler. * */ if (session->session_state != SSH_SESSION_STATE_AUTHENTICATED) { @@ -699,21 +701,18 @@ static enum ssh_packet_filter_result_e ssh_packet_incoming_filter(ssh_session se break; } - if (session->global_req_state != SSH_CHANNEL_REQ_STATE_PENDING) { - rc = SSH_PACKET_DENIED; - break; - } - rc = SSH_PACKET_ALLOWED; break; case SSH2_MSG_REQUEST_FAILURE: // 82 /* * States required: * - session_state == SSH_SESSION_STATE_AUTHENTICATED - * - session->global_req_state == SSH_CHANNEL_REQ_STATE_PENDING * * Transitions: - * - session->global_req_state == SSH_CHANNEL_REQ_STATE_DENIED + * - From channel->request_state == SSH_CHANNEL_REQ_STATE_PENDING + * - To channel->request_state = SSH_CHANNEL_REQ_STATE_ACCEPTED + * + * If not in a pending state, message is ignored in the callback handler. * */ if (session->session_state != SSH_SESSION_STATE_AUTHENTICATED) { @@ -721,11 +720,6 @@ static enum ssh_packet_filter_result_e ssh_packet_incoming_filter(ssh_session se break; } - if (session->global_req_state != SSH_CHANNEL_REQ_STATE_PENDING) { - rc = SSH_PACKET_DENIED; - break; - } - rc = SSH_PACKET_ALLOWED; break; case SSH2_MSG_CHANNEL_OPEN: // 90 @@ -878,10 +872,12 @@ static enum ssh_packet_filter_result_e ssh_packet_incoming_filter(ssh_session se /* * States required: * - session_state == SSH_SESSION_STATE_AUTHENTICATED - * - channel->request_state == SSH_CHANNEL_REQ_STATE_PENDING * * Transitions: - * - channel->request_state = SSH_CHANNEL_REQ_STATE_ACCEPTED + * - From channel->request_state == SSH_CHANNEL_REQ_STATE_PENDING + * - To channel->request_state = SSH_CHANNEL_REQ_STATE_ACCEPTED + * + * If not in a pending state, message is ignored in the callback handler. * */ if (session->session_state != SSH_SESSION_STATE_AUTHENTICATED) { @@ -895,10 +891,12 @@ static enum ssh_packet_filter_result_e ssh_packet_incoming_filter(ssh_session se /* * States required: * - session_state == SSH_SESSION_STATE_AUTHENTICATED - * - channel->request_state == SSH_CHANNEL_REQ_STATE_PENDING * * Transitions: - * - channel->request_state = SSH_CHANNEL_REQ_STATE_DENIED + * - From channel->request_state == SSH_CHANNEL_REQ_STATE_PENDING + * - To channel->request_state = SSH_CHANNEL_REQ_STATE_ACCEPTED + * + * If not in a pending state, message is ignored in the callback handler. * */ if (session->session_state != SSH_SESSION_STATE_AUTHENTICATED) { diff --git a/tests/unittests/torture_packet_filter.c b/tests/unittests/torture_packet_filter.c index 85fb5c1b..ffa49602 100644 --- a/tests/unittests/torture_packet_filter.c +++ b/tests/unittests/torture_packet_filter.c @@ -517,12 +517,108 @@ static void torture_packet_filter_check_channel_open(void **state) assert_int_equal(rc, 0); } +static void torture_packet_filter_check_channel_success(void **state) +{ + int rc; + + /* The only condition to accept a CHANNEL_SUCCESS is to be authenticated */ + global_state accepted[] = { + { + .flags = COMPARE_SESSION_STATE, + .session = SSH_SESSION_STATE_AUTHENTICATED, + } + }; + + int accepted_count = 1; + + /* Unused */ + (void) state; + + rc = check_message_in_all_states(accepted, accepted_count, + SSH2_MSG_CHANNEL_SUCCESS); + + assert_int_equal(rc, 0); +} + +static void torture_packet_filter_check_channel_failure(void **state) +{ + int rc; + + /* The only condition to accept a CHANNEL_FAILURE is to be authenticated */ + global_state accepted[] = { + { + .flags = COMPARE_SESSION_STATE, + .session = SSH_SESSION_STATE_AUTHENTICATED, + } + }; + + int accepted_count = 1; + + /* Unused */ + (void) state; + + rc = check_message_in_all_states(accepted, accepted_count, + SSH2_MSG_CHANNEL_FAILURE); + + assert_int_equal(rc, 0); +} + +static void torture_packet_filter_check_request_success(void **state) +{ + int rc; + + /* The only condition to accept a REQUEST_SUCCESS is to be authenticated */ + global_state accepted[] = { + { + .flags = COMPARE_SESSION_STATE, + .session = SSH_SESSION_STATE_AUTHENTICATED, + } + }; + + int accepted_count = 1; + + /* Unused */ + (void) state; + + rc = check_message_in_all_states(accepted, accepted_count, + SSH2_MSG_REQUEST_SUCCESS); + + assert_int_equal(rc, 0); +} + +static void torture_packet_filter_check_request_failure(void **state) +{ + int rc; + + /* The only condition to accept a REQUEST_FAILURE is to be authenticated */ + global_state accepted[] = { + { + .flags = COMPARE_SESSION_STATE, + .session = SSH_SESSION_STATE_AUTHENTICATED, + } + }; + + int accepted_count = 1; + + /* Unused */ + (void) state; + + rc = check_message_in_all_states(accepted, accepted_count, + SSH2_MSG_REQUEST_FAILURE); + + assert_int_equal(rc, 0); +} + int torture_run_tests(void) { int rc; struct CMUnitTest tests[] = { cmocka_unit_test(torture_packet_filter_check_auth_success), cmocka_unit_test(torture_packet_filter_check_channel_open), + cmocka_unit_test(torture_packet_filter_check_channel_success), + cmocka_unit_test(torture_packet_filter_check_channel_failure), + cmocka_unit_test(torture_packet_filter_check_request_success), + cmocka_unit_test(torture_packet_filter_check_request_failure), cmocka_unit_test(torture_packet_filter_check_unfiltered), cmocka_unit_test(torture_packet_filter_check_msg_ext_info) }; |