From b0fb7d1527b65b86c99906db7029b78d88585abb Mon Sep 17 00:00:00 2001 From: "Artyom V. Poptsov" Date: Sun, 15 Aug 2021 22:50:33 +0300 Subject: [PATCH 1/3] channels: Fix segfaults when the channel data is freed Calling some channel procedures on a freed channel is always resulting in segmentation fault errors. The reason is that when a channel is freed with 'ssh_channel_do_free' procedure, its 'session' field is set to NULL; then when a channel procedure tries to access any field of 'channel->session' structure it is effectively dereferencing a NULL pointer. The change fixes that behavior by adding a check which ensures that a channel state is not SSH_CHANNEL_FLAG_FREED_LOCAL before accessing its parent session. Also the test suite is updated to check for the fixed errors, and the Doxygen documentation updated accordingly. Signed-off-by: Artyom V. Poptsov --- src/channels.c | 15 ++-- tests/client/torture_session.c | 151 +++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 5 deletions(-) diff --git a/src/channels.c b/src/channels.c index 8d8124775..1e5105a02 100644 --- a/src/channels.c +++ b/src/channels.c @@ -878,7 +878,7 @@ SSH_PACKET_CALLBACK(channel_rcv_request) { #else SSH_LOG(SSH_LOG_WARNING, "Unhandled channel request %s", request); #endif - + SAFE_FREE(request); return SSH_PACKET_USED; @@ -3086,6 +3086,8 @@ int ssh_channel_read_nonblocking(ssh_channel channel, * * @return The number of bytes available for reading, 0 if nothing * is available or SSH_ERROR on error. + * When a channel is freed the function returns + * SSH_ERROR immediately. * * @warning When the channel is in EOF state, the function returns SSH_EOF. * @@ -3094,7 +3096,7 @@ int ssh_channel_read_nonblocking(ssh_channel channel, int ssh_channel_poll(ssh_channel channel, int is_stderr){ ssh_buffer stdbuf; - if(channel == NULL) { + if ((channel == NULL) || (channel->state == SSH_CHANNEL_FLAG_FREED_LOCAL)) { return SSH_ERROR; } @@ -3140,6 +3142,7 @@ int ssh_channel_poll(ssh_channel channel, int is_stderr){ * SSH_ERROR on error. * * @warning When the channel is in EOF state, the function returns SSH_EOF. + * When a channel is freed the function returns SSH_ERROR immediately. * * @see ssh_channel_is_eof() */ @@ -3151,7 +3154,7 @@ int ssh_channel_poll_timeout(ssh_channel channel, int timeout, int is_stderr) size_t len; int rc; - if (channel == NULL) { + if ((channel == NULL) || (channel->state == SSH_CHANNEL_FLAG_FREED_LOCAL)) { return SSH_ERROR; } @@ -3233,6 +3236,8 @@ static int ssh_channel_exit_status_termination(void *c){ * (yet), or SSH_ERROR on error. * @warning This function may block until a timeout (or never) * if the other side is not willing to close the channel. + * When a channel is freed the function returns + * SSH_ERROR immediately. * * If you're looking for an async handling of this register a callback for the * exit status. @@ -3241,7 +3246,7 @@ static int ssh_channel_exit_status_termination(void *c){ */ int ssh_channel_get_exit_status(ssh_channel channel) { int rc; - if(channel == NULL) { + if ((channel == NULL) || (channel->state == SSH_CHANNEL_FLAG_FREED_LOCAL)) { return SSH_ERROR; } rc = ssh_handle_packets_termination(channel->session, @@ -3590,7 +3595,7 @@ error: * forward the content of a socket to the channel. You still have to * use channel_read and channel_write for this. */ -int ssh_channel_open_x11(ssh_channel channel, +int ssh_channel_open_x11(ssh_channel channel, const char *orig_addr, int orig_port) { ssh_session session; ssh_buffer payload = NULL; diff --git a/tests/client/torture_session.c b/tests/client/torture_session.c index 2fa2ae334..665da7bc1 100644 --- a/tests/client/torture_session.c +++ b/tests/client/torture_session.c @@ -258,6 +258,145 @@ static void torture_channel_delayed_close(void **state) } +/* Ensure that calling 'ssh_channel_poll' on a freed channel does not lead to + * segmentation faults. */ +static void torture_freed_channel_poll(void **state) +{ + struct torture_state *s = *state; + ssh_session session = s->ssh.session; + ssh_channel channel; + + char request[256]; + char buff[256] = {0}; + int rc; + + snprintf(request, 256, + "dd if=/dev/urandom of=/tmp/file bs=64000 count=2; hexdump -C /tmp/file"); + + channel = ssh_channel_new(session); + assert_non_null(channel); + + rc = ssh_channel_open_session(channel); + assert_ssh_return_code(session, rc); + + /* Make the request, read parts with close */ + rc = ssh_channel_request_exec(channel, request); + assert_ssh_return_code(session, rc); + + /* do { */ + /* rc = ssh_channel_read(channel, buff, 256, 0); */ + /* } while(rc > 0); */ + /* assert_ssh_return_code(session, rc); */ + + ssh_channel_free(channel); + + rc = ssh_channel_poll(channel, 0); + assert_int_equal(rc, SSH_ERROR); +} + +/* Ensure that calling 'ssh_channel_poll_timeout' on a freed channel does not + * lead to segmentation faults. */ +static void torture_freed_channel_poll_timeout(void **state) +{ + struct torture_state *s = *state; + ssh_session session = s->ssh.session; + ssh_channel channel; + + char request[256]; + char buff[256] = {0}; + int rc; + + snprintf(request, 256, + "dd if=/dev/urandom of=/tmp/file bs=64000 count=2; hexdump -C /tmp/file"); + + channel = ssh_channel_new(session); + assert_non_null(channel); + + rc = ssh_channel_open_session(channel); + assert_ssh_return_code(session, rc); + + /* Make the request, read parts with close */ + rc = ssh_channel_request_exec(channel, request); + assert_ssh_return_code(session, rc); + + do { + rc = ssh_channel_read(channel, buff, 256, 0); + } while(rc > 0); + assert_ssh_return_code(session, rc); + + ssh_channel_free(channel); + + rc = ssh_channel_poll_timeout(channel, 500, 0); + assert_int_equal(rc, SSH_ERROR); +} + +/* Ensure that calling 'ssh_channel_read_nonblocking' on a freed channel does + * not lead to segmentation faults. */ +static void torture_freed_channel_read_nonblocking(void **state) +{ + struct torture_state *s = *state; + ssh_session session = s->ssh.session; + ssh_channel channel; + + char request[256]; + char buff[256] = {0}; + int rc; + + snprintf(request, 256, + "dd if=/dev/urandom of=/tmp/file bs=64000 count=2; hexdump -C /tmp/file"); + + channel = ssh_channel_new(session); + assert_non_null(channel); + + rc = ssh_channel_open_session(channel); + assert_ssh_return_code(session, rc); + + /* Make the request, read parts with close */ + rc = ssh_channel_request_exec(channel, request); + assert_ssh_return_code(session, rc); + + ssh_channel_free(channel); + + rc = ssh_channel_read_nonblocking(channel, buff, 256, 0); + assert_ssh_return_code_equal(session, rc, SSH_ERROR); +} + +/* Ensure that calling 'ssh_channel_get_exit_status' on a freed channel does not + * lead to segmentation faults. */ +static void torture_freed_channel_get_exit_status(void **state) +{ + struct torture_state *s = *state; + ssh_session session = s->ssh.session; + ssh_channel channel; + + char request[256]; + char buff[256] = {0}; + int rc; + + snprintf(request, 256, + "dd if=/dev/urandom of=/tmp/file bs=64000 count=2; hexdump -C /tmp/file"); + + channel = ssh_channel_new(session); + assert_non_null(channel); + + rc = ssh_channel_open_session(channel); + assert_ssh_return_code(session, rc); + + /* Make the request, read parts with close */ + rc = ssh_channel_request_exec(channel, request); + assert_ssh_return_code(session, rc); + + do { + rc = ssh_channel_read(channel, buff, 256, 0); + } while(rc > 0); + assert_ssh_return_code(session, rc); + + ssh_channel_free(channel); + + rc = ssh_channel_get_exit_status(channel); + assert_ssh_return_code_equal(session, rc, SSH_ERROR); +} + int torture_run_tests(void) { int rc; struct CMUnitTest tests[] = { @@ -276,6 +415,18 @@ int torture_run_tests(void) { cmocka_unit_test_setup_teardown(torture_channel_delayed_close, session_setup, session_teardown), + cmocka_unit_test_setup_teardown(torture_freed_channel_poll, + session_setup, + session_teardown), + cmocka_unit_test_setup_teardown(torture_freed_channel_poll_timeout, + session_setup, + session_teardown), + cmocka_unit_test_setup_teardown(torture_freed_channel_read_nonblocking, + session_setup, + session_teardown), + cmocka_unit_test_setup_teardown(torture_freed_channel_get_exit_status, + session_setup, + session_teardown), }; ssh_init(); -- GitLab From 143d9490fcfa96d81cf9d30ed8049aad708ca21f Mon Sep 17 00:00:00 2001 From: "Artyom V. Poptsov" Date: Sun, 22 Aug 2021 13:51:31 +0300 Subject: [PATCH 2/3] tests/client/torture_session: Fix failing tests tests/client/torture_session.c: Fix errors in 'torture_freed_channel_poll' due to an unused variable. --- tests/client/torture_session.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/client/torture_session.c b/tests/client/torture_session.c index 665da7bc1..27e8fc863 100644 --- a/tests/client/torture_session.c +++ b/tests/client/torture_session.c @@ -267,7 +267,6 @@ static void torture_freed_channel_poll(void **state) ssh_channel channel; char request[256]; - char buff[256] = {0}; int rc; snprintf(request, 256, @@ -283,11 +282,6 @@ static void torture_freed_channel_poll(void **state) rc = ssh_channel_request_exec(channel, request); assert_ssh_return_code(session, rc); - /* do { */ - /* rc = ssh_channel_read(channel, buff, 256, 0); */ - /* } while(rc > 0); */ - /* assert_ssh_return_code(session, rc); */ - ssh_channel_free(channel); rc = ssh_channel_poll(channel, 0); -- GitLab From 3c74ba5e94ba24560cc1d797e2bf780383be4a02 Mon Sep 17 00:00:00 2001 From: "Artyom V. Poptsov" Date: Tue, 14 Sep 2021 20:10:06 +0300 Subject: [PATCH 3/3] channels: Bugfix: Check channel flags instead of the state There was a bug introduced in b0fb7d15: 'ssh_channel_poll', 'ssh_channel_poll_timeout' and 'ssh_channel_get_exit_status' would compare the channel state to the 'SSH_CHANNEL_FLAG_FREED_LOCAL' constant to check if the channel is alive. But the procedures must check the channel flags for the presence of 'SSH_CHANNEL_FLAG_FREED_LOCAL' bits instead. This change fixes the bug. Signed-off-by: Artyom V. Poptsov --- src/channels.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/channels.c b/src/channels.c index 1e5105a02..6e9fcfc1c 100644 --- a/src/channels.c +++ b/src/channels.c @@ -3096,7 +3096,7 @@ int ssh_channel_read_nonblocking(ssh_channel channel, int ssh_channel_poll(ssh_channel channel, int is_stderr){ ssh_buffer stdbuf; - if ((channel == NULL) || (channel->state == SSH_CHANNEL_FLAG_FREED_LOCAL)) { + if ((channel == NULL) || (channel->flags & SSH_CHANNEL_FLAG_FREED_LOCAL)) { return SSH_ERROR; } @@ -3154,7 +3154,7 @@ int ssh_channel_poll_timeout(ssh_channel channel, int timeout, int is_stderr) size_t len; int rc; - if ((channel == NULL) || (channel->state == SSH_CHANNEL_FLAG_FREED_LOCAL)) { + if ((channel == NULL) || (channel->flags & SSH_CHANNEL_FLAG_FREED_LOCAL)) { return SSH_ERROR; } @@ -3246,7 +3246,7 @@ static int ssh_channel_exit_status_termination(void *c){ */ int ssh_channel_get_exit_status(ssh_channel channel) { int rc; - if ((channel == NULL) || (channel->state == SSH_CHANNEL_FLAG_FREED_LOCAL)) { + if ((channel == NULL) || (channel->flags & SSH_CHANNEL_FLAG_FREED_LOCAL)) { return SSH_ERROR; } rc = ssh_handle_packets_termination(channel->session, -- GitLab