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();