From 46a28cfc490d8c6bbd30a1cba6496502726f8ec4 Mon Sep 17 00:00:00 2001 From: Diego Roux <9531300-diegoroux@users.noreply.gitlab.com> Date: Wed, 27 Mar 2024 14:31:11 +0000 Subject: [PATCH] log: fixes legacy fallback for multiple sessions. Legacy code in 'ssh_set_callbacks' will fallback to 'ssh_legacy_log_callback' (if the current log cb is NULL) setting the user data to the current session. However, if any other session is created afterwards, it won't update the user data with the new session, potentially leading to a use-after-free. Fixes #238. Signed-off-by: Diego Roux Reviewed-by: Jakub Jelen --- include/libssh/priv.h | 4 ++ src/callbacks.c | 9 ++++ src/log.c | 6 +++ src/session.c | 2 + tests/client/torture_connect.c | 84 ++++++++++++++++++++++++++--- tests/unittests/torture_callbacks.c | 13 +++-- 6 files changed, 108 insertions(+), 10 deletions(-) diff --git a/include/libssh/priv.h b/include/libssh/priv.h index bfef771d..4434d143 100644 --- a/include/libssh/priv.h +++ b/include/libssh/priv.h @@ -274,6 +274,10 @@ void ssh_log_common(struct ssh_common_struct *common, const char *function, const char *format, ...) PRINTF_ATTRIBUTE(4, 5); +void _ssh_remove_legacy_log_cb(void); + +/* log.c */ +void _ssh_reset_log_cb(void); /* ERROR HANDLING */ diff --git a/src/callbacks.c b/src/callbacks.c index 3ed2f11c..cea4301a 100644 --- a/src/callbacks.c +++ b/src/callbacks.c @@ -45,6 +45,15 @@ static void ssh_legacy_log_callback(int priority, log_fn(session, priority, buffer, log_data); } +void +_ssh_remove_legacy_log_cb(void) +{ + if (ssh_get_log_callback() == ssh_legacy_log_callback) { + _ssh_reset_log_cb(); + ssh_set_log_userdata(NULL); + } +} + int ssh_set_callbacks(ssh_session session, ssh_callbacks cb) { if (session == NULL || cb == NULL) { return SSH_ERROR; diff --git a/src/log.c b/src/log.c index 5bae18b8..bef65a84 100644 --- a/src/log.c +++ b/src/log.c @@ -221,6 +221,12 @@ int ssh_set_log_callback(ssh_logging_callback cb) { return SSH_OK; } +void +_ssh_reset_log_cb(void) +{ + ssh_log_cb = NULL; +} + ssh_logging_callback ssh_get_log_callback(void) { return ssh_log_cb; } diff --git a/src/session.c b/src/session.c index 279352b6..b9efc6fa 100644 --- a/src/session.c +++ b/src/session.c @@ -358,6 +358,8 @@ void ssh_free(ssh_session session) } } + _ssh_remove_legacy_log_cb(); + /* burn connection, it could contain sensitive data */ explicit_bzero(session, sizeof(struct ssh_session_struct)); SAFE_FREE(session); diff --git a/tests/client/torture_connect.c b/tests/client/torture_connect.c index f086488d..fd3e3d32 100644 --- a/tests/client/torture_connect.c +++ b/tests/client/torture_connect.c @@ -258,18 +258,90 @@ static void torture_connect_uninitialized(UNUSED_PARAM(void **state)) ssh_free(session); } +static void +internal_log(ssh_session session, + int priority, + const char *message, + void *userdata) +{ + (void)session; + (void)priority; + (void)message; + (void)userdata; + + return; +} + +static void +torture_legacy_callback(void **state) +{ + struct ssh_callbacks_struct cb[2] = {0}; + int rc, verbosity = SSH_LOG_WARNING; + ssh_session session = NULL; + + /* unused. */ + (void)state; + + /* + * Legacy code in 'ssh_set_callbacks' used to + * create the conditions for a use-after-free + * issue, in multi-session programs, by failing + * to update a pointer with the new session. + * + * To verify it won't happen again, this test + * creates two consecutive sessions and frees + * them; if any fault occurs then the pointer + * remained at the previous session, failing + * to be updated. + */ + for (int i = 0; i < 2; i++) { + session = ssh_new(); + assert_non_null(session); + + rc = ssh_options_set(session, SSH_OPTIONS_HOST, TORTURE_SSH_SERVER); + assert_ssh_return_code(session, rc); + + rc = ssh_options_set(session, SSH_OPTIONS_LOG_VERBOSITY, &verbosity); + assert_ssh_return_code(session, rc); + + cb[i].log_function = internal_log; + + ssh_callbacks_init(&cb[i]); + ssh_set_callbacks(session, &cb[i]); + + rc = ssh_connect(session); + assert_ssh_return_code(session, rc); + ssh_disconnect(session); + + ssh_free(session); + } +} + int torture_run_tests(void) { int rc; struct CMUnitTest tests[] = { - cmocka_unit_test_setup_teardown(torture_connect_peer_discon_msg, session_setup, session_teardown), - cmocka_unit_test_setup_teardown(torture_connect_nonblocking, session_setup, session_teardown), - cmocka_unit_test_setup_teardown(torture_connect_ipv6, session_setup, session_teardown), - cmocka_unit_test_setup_teardown(torture_connect_double, session_setup, session_teardown), - cmocka_unit_test_setup_teardown(torture_connect_failure, session_setup, session_teardown), + cmocka_unit_test_setup_teardown(torture_connect_peer_discon_msg, + session_setup, + session_teardown), + cmocka_unit_test_setup_teardown(torture_connect_nonblocking, + session_setup, + session_teardown), + cmocka_unit_test_setup_teardown(torture_connect_ipv6, + session_setup, + session_teardown), + cmocka_unit_test_setup_teardown(torture_connect_double, + session_setup, + session_teardown), + cmocka_unit_test_setup_teardown(torture_connect_failure, + session_setup, + session_teardown), #if 0 cmocka_unit_test_setup_teardown(torture_connect_timeout, session_setup, session_teardown), #endif - cmocka_unit_test_setup_teardown(torture_connect_socket, session_setup, session_teardown), + cmocka_unit_test_setup_teardown(torture_connect_socket, + session_setup, + session_teardown), + cmocka_unit_test(torture_legacy_callback), cmocka_unit_test(torture_connect_uninitialized), }; diff --git a/tests/unittests/torture_callbacks.c b/tests/unittests/torture_callbacks.c index 25111b2f..ac5b4b1b 100644 --- a/tests/unittests/torture_callbacks.c +++ b/tests/unittests/torture_callbacks.c @@ -3,9 +3,10 @@ #define LIBSSH_STATIC #include "torture.h" -#include +#include #include #include +#include static int myauthcallback (const char *prompt, char *buf, size_t len, int echo, int verify, void *userdata) { @@ -249,11 +250,15 @@ static void torture_callbacks_iterate(void **state){ int torture_run_tests(void) { int rc; struct CMUnitTest tests[] = { - cmocka_unit_test_setup_teardown(torture_callbacks_size, setup, teardown), - cmocka_unit_test_setup_teardown(torture_callbacks_exists, setup, teardown), + cmocka_unit_test_setup_teardown(torture_callbacks_size, + setup, + teardown), + cmocka_unit_test_setup_teardown(torture_callbacks_exists, + setup, + teardown), cmocka_unit_test(torture_log_callback), cmocka_unit_test(torture_callbacks_execute_list), - cmocka_unit_test(torture_callbacks_iterate) + cmocka_unit_test(torture_callbacks_iterate), }; ssh_init();