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 <diegoroux04@protonmail.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>

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 */
 

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;

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;
 }

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

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_peer_discon_msg,
-        cmocka_unit_test_setup_teardown(torture_connect_nonblocking, session_setup, session_teardown),
+                                        session_setup,
-        cmocka_unit_test_setup_teardown(torture_connect_ipv6, session_setup, session_teardown),
+                                        session_teardown),
-        cmocka_unit_test_setup_teardown(torture_connect_double, session_setup, session_teardown),
+        cmocka_unit_test_setup_teardown(torture_connect_nonblocking,
-        cmocka_unit_test_setup_teardown(torture_connect_failure, session_setup, session_teardown),
+                                        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),
     };
 

tests/unittests/torture_callbacks.c

@@ -3,9 +3,10 @@
 #define LIBSSH_STATIC
 
 #include "torture.h"
-#include <libssh/priv.h>
+#include <errno.h>
 #include <libssh/callbacks.h>
 #include <libssh/misc.h>
+#include <libssh/priv.h>
 
 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_size,
-        cmocka_unit_test_setup_teardown(torture_callbacks_exists, setup, teardown),
+                                        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();