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.

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 <poptsov.artyom@gmail.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
This commit is contained in:
Artyom V. Poptsov
2021-08-15 22:50:33 +03:00
committed by Jakub Jelen
parent 76b7e0e9b5
commit 1ab2340644
2 changed files with 155 additions and 5 deletions

View File

@@ -258,6 +258,139 @@ 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];
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_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 +409,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();