OSS-Fuzz: Fix blocking of ssh mock session

Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
Merge-Request: <https://gitlab.com/libssh/libssh-mirror/-/merge_requests/782>

tests/fuzz/CMakeLists.txt

@@ -3,6 +3,9 @@ project(fuzzing CXX)
 # Build SSH server mock helper as object library
 add_library(ssh_server_mock_obj OBJECT ssh_server_mock.c)
 target_link_libraries(ssh_server_mock_obj PRIVATE ${TORTURE_LINK_LIBRARIES})
+if (WITH_COVERAGE)
+    append_coverage_compiler_flags_to_target(ssh_server_mock_obj)
+endif (WITH_COVERAGE)
 
 macro(fuzzer name)
     add_executable(${name} ${name}.c)
@@ -22,10 +25,16 @@ macro(fuzzer name)
                               PROPERTIES
                                   COMPILE_FLAGS "-fsanitize=fuzzer"
                                   LINK_FLAGS "-fsanitize=fuzzer")
+        # Pick up <name>.dict if present
+        if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${name}.dict")
+            set(_DICT_ARG "-dict=${CMAKE_CURRENT_SOURCE_DIR}/${name}.dict")
+        else()
+            set(_DICT_ARG "")
+        endif()
         # Run the fuzzer to make sure it works
-        add_test(${name} ${CMAKE_CURRENT_BINARY_DIR}/${name} -runs=1)
+        add_test(${name} ${CMAKE_CURRENT_BINARY_DIR}/${name} ${_DICT_ARG} -runs=1)
         # Run the fuzzer with nalloc to make sure it works
-        add_test(${name}_nalloc ${CMAKE_CURRENT_BINARY_DIR}/${name} -runs=1)
+        add_test(${name}_nalloc ${CMAKE_CURRENT_BINARY_DIR}/${name} ${_DICT_ARG} -runs=1)
         set_property(TEST ${name}_nalloc PROPERTY ENVIRONMENT NALLOC_FREQ 32)
     else()
         target_sources(${name} PRIVATE fuzzer.c)

tests/fuzz/ssh_scp_fuzzer.c

@@ -16,11 +16,13 @@
 
 #include <assert.h>
 #include <pthread.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/socket.h>
+#include <sys/time.h>
 #include <unistd.h>
 
 #define LIBSSH_STATIC 1
@@ -45,12 +47,104 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
     return 0;
 }
 
-/* Helper function to test one cipher/HMAC combination */
+static const char *const k_ciphers[] = {
+    "none",
+    "aes128-ctr",
+    "aes256-ctr",
+    "aes128-cbc",
+};
+
+static const char *const k_hmacs[] = {
+    "none",
+    "hmac-sha1",
+    "hmac-sha2-256",
+};
+
+/*
+ * Wrap fuzzer bytes as a valid SCP server record stream so the client
+ * survives ssh_scp_init's initial-ACK gate and reaches the deeper SCP
+ * code paths (record parsing, accept/deny, transfer flow, recursive
+ * push). Without this wrapping the fuzzer almost never satisfies the
+ * "\x00C<mode> <size> <name>\n" prefix the SCP layer expects, and
+ * coverage stalls in the early-protocol record-parser rejection paths.
+ *
+ * The 4 envelope bytes consumed here are still fuzzer-controlled, so
+ * libFuzzer mutations explore C/D/T/E variants, mismatched sizes, and
+ * unusual modes from inside the wrap:
+ *
+ *   data[0..1]  SCP mode (12 bits)
+ *   data[2]     bit 0-1: variant select (0=C, 1=D, 2=T, 3=E)
+ *               bit 2:   optional trailing server-ACK after payload
+ *   data[3]     declared transfer size in the C-record header
+ *   data[4..]   raw payload bytes appended after the SCP header
+ *
+ * Coverage of invalid SCP record parsing is NOT given up by this
+ * shaping: ssh_server_fuzzer and ssh_client_fuzzer pump unstructured
+ * bytes through the SSH transport and reach the SCP record parser's
+ * rejection paths from that direction.
+ */
+static size_t
+scp_wrap(const uint8_t *data, size_t size, uint8_t *out, size_t out_cap)
+{
+    uint16_t mode;
+    uint8_t variant;
+    uint8_t declared_size;
+    size_t payload_sz;
+    size_t cap_left;
+    size_t total;
+    int n;
+
+    if (size < 4 || out_cap == 0) {
+        return 0;
+    }
+    mode = ((uint16_t)data[0] << 8 | data[1]) & 07777;
+    variant = data[2] & 0x03;
+    declared_size = data[3];
+
+    switch (variant) {
+    case 0:
+        n = snprintf((char *)out,
+                     out_cap,
+                     "%cC%04o %u f\n",
+                     0,
+                     mode,
+                     declared_size);
+        break;
+    case 1:
+        n = snprintf((char *)out, out_cap, "%cD%04o 0 d\n", 0, mode);
+        break;
+    case 2:
+        n = snprintf((char *)out, out_cap, "%cT0 0 0 0\n", 0);
+        break;
+    default:
+        n = snprintf((char *)out, out_cap, "%cE\n", 0);
+        break;
+    }
+    if (n < 0 || (size_t)n >= out_cap) {
+        return 0;
+    }
+
+    payload_sz = size - 4;
+    cap_left = out_cap - (size_t)n;
+    if (payload_sz > cap_left) {
+        payload_sz = cap_left;
+    }
+    memcpy(out + n, data + 4, payload_sz);
+    total = (size_t)n + payload_sz;
+    /* Optional server final ACK; bit chosen by fuzzer to cover both paths */
+    if (variant == 0 && (data[2] & 0x04) && total < out_cap) {
+        out[total++] = '\x00';
+    }
+    return total;
+}
+
+/* Run one SCP fuzzing iteration against the mock server */
 static int test_scp_with_cipher(const uint8_t *data,
                                 size_t size,
                                 const char *cipher,
                                 const char *hmac)
 {
+    bool thread_started = false;
     int socket_fds[2] = {-1, -1};
     ssh_session client_session = NULL;
     ssh_scp scp = NULL, scp_recursive = NULL;
@@ -70,22 +164,31 @@ static int test_scp_with_cipher(const uint8_t *data,
         .client_socket = -1,
         .server_ready = false,
         .server_error = false,
+        .shutdown_requested = false,
     };
 
     if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_fds) != 0) {
         goto cleanup;
     }
 
+    /* Set socket timeouts to prevent indefinite blocking */
+    struct timeval tv = {.tv_sec = 2, .tv_usec = 0};
+    setsockopt(socket_fds[0], SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
+    setsockopt(socket_fds[0], SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv));
+    setsockopt(socket_fds[1], SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
+    setsockopt(socket_fds[1], SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv));
+
     server_config.server_socket = socket_fds[0];
     server_config.client_socket = socket_fds[1];
 
     if (ssh_mock_server_start(&server_config, &srv_thread) != 0) {
         goto cleanup;
     }
+    thread_started = true;
 
     client_session = ssh_new();
     if (client_session == NULL) {
-        goto cleanup_thread;
+        goto cleanup;
     }
 
     /* Configure client with specified cipher/HMAC */
@@ -102,20 +205,20 @@ static int test_scp_with_cipher(const uint8_t *data,
     ssh_options_set(client_session, SSH_OPTIONS_TIMEOUT, &timeout);
 
     if (ssh_connect(client_session) != SSH_OK) {
-        goto cleanup_thread;
+        goto cleanup;
     }
 
     if (ssh_userauth_none(client_session, NULL) != SSH_AUTH_SUCCESS) {
-        goto cleanup_thread;
+        goto cleanup;
     }
 
     scp = ssh_scp_new(client_session, SSH_SCP_READ, "/tmp/fuzz");
     if (scp == NULL) {
-        goto cleanup_thread;
+        goto cleanup;
     }
 
     if (ssh_scp_init(scp) != SSH_OK) {
-        goto cleanup_thread;
+        goto cleanup;
     }
 
     if (size > 0) {
@@ -150,10 +253,17 @@ static int test_scp_with_cipher(const uint8_t *data,
         }
     }
 
-cleanup_thread:
-    pthread_join(srv_thread, NULL);
-
 cleanup:
+    /* Signal server thread to exit */
+    server_config.shutdown_requested = true;
+
+    /* Close sockets */
+    if (socket_fds[0] >= 0)
+        close(socket_fds[0]);
+    if (socket_fds[1] >= 0)
+        close(socket_fds[1]);
+
+    /* Cleanup client objects */
     if (scp_recursive != NULL) {
         ssh_scp_close(scp_recursive);
         ssh_scp_free(scp_recursive);
@@ -166,39 +276,47 @@ cleanup:
         ssh_disconnect(client_session);
         ssh_free(client_session);
     }
-    if (socket_fds[0] >= 0)
-        close(socket_fds[0]);
-    if (socket_fds[1] >= 0)
-        close(socket_fds[1]);
+
+    /* Server thread exits via shutdown_requested + 2s socket timeout */
+    if (thread_started) {
+        pthread_join(srv_thread, NULL);
+    }
 
     return 0;
 }
 
+/*
+ * Fuzzer input layout (not passed directly to the SSH transport; it is
+ * decoded here, then fed through scp_wrap):
+ *
+ *   data[0]   cipher index, taken modulo length of k_ciphers
+ *   data[1]   HMAC index,   taken modulo length of k_hmacs
+ *   data[2..] handed to scp_wrap, which uses the first 4 bytes as the
+ *             SCP envelope (mode, variant, declared size, optional ACK
+ *             toggle) and the rest as the file-content payload.
+ *
+ * Inputs shorter than 6 bytes are rejected so every iteration has at
+ * least the two selector bytes plus one full envelope for scp_wrap.
+ */
 int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
 {
+    uint8_t wrapped[4096];
+    size_t wrapped_size = 0;
+    const char *cipher = NULL;
+    const char *hmac = NULL;
+
+    if (size < 6) {
+        return 0;
+    }
+
     assert(nalloc_start(data, size) > 0);
 
-    /* Test all cipher/HMAC combinations exhaustively */
-    const char *ciphers[] = {
-        "none",
-        "aes128-ctr",
-        "aes256-ctr",
-        "aes128-cbc",
-    };
+    cipher = k_ciphers[data[0] % (sizeof(k_ciphers) / sizeof(k_ciphers[0]))];
+    hmac = k_hmacs[data[1] % (sizeof(k_hmacs) / sizeof(k_hmacs[0]))];
 
-    const char *hmacs[] = {
-        "none",
-        "hmac-sha1",
-        "hmac-sha2-256",
-    };
-
-    int num_ciphers = sizeof(ciphers) / sizeof(ciphers[0]);
-    int num_hmacs = sizeof(hmacs) / sizeof(hmacs[0]);
-
-    for (int i = 0; i < num_ciphers; i++) {
-        for (int j = 0; j < num_hmacs; j++) {
-            test_scp_with_cipher(data, size, ciphers[i], hmacs[j]);
-        }
+    wrapped_size = scp_wrap(data + 2, size - 2, wrapped, sizeof(wrapped));
+    if (wrapped_size > 0) {
+        test_scp_with_cipher(wrapped, wrapped_size, cipher, hmac);
     }
 
     nalloc_end();

tests/fuzz/ssh_scp_fuzzer.dict

@@ -0,0 +1,46 @@
+"\x00C0644 "
+"\x00C0755 "
+"\x00C0600 "
+"\x00D0755 "
+"\x00D0777 "
+"\x00T0 0 0 0\x0a"
+"\x00E\x0a"
+" f\x0a"
+" d\x0a"
+"ssh-ed25519"
+"ssh-rsa"
+"rsa-sha2-256"
+"rsa-sha2-512"
+"ecdsa-sha2-nistp256"
+"ecdh-sha2-nistp256"
+"curve25519-sha256"
+"curve25519-sha256@libssh.org"
+"aes128-ctr"
+"aes192-ctr"
+"aes256-ctr"
+"aes128-cbc"
+"hmac-sha1"
+"hmac-sha2-256"
+"chacha20-poly1305@openssh.com"
+"zlib"
+"zlib@openssh.com"
+"none"
+"SSH-2.0-"
+"OpenSSH"
+"exec"
+"shell"
+"subsystem"
+"pty-req"
+"exit-status"
+"exit-signal"
+"ext-info-c"
+"ext-info-s"
+"server-sig-algs"
+"ping@openssh.com"
+"hostkeys-00@openssh.com"
+"hostkeys-prove-00@openssh.com"
+"\x00\x00\x00\x01"
+"\x00\x00\x00\x05"
+"\x00\x00\x00\x07"
+"\x00\x00\x00\x10"
+"\x00\x00\x00\x20"

tests/fuzz/ssh_server_mock.c

@@ -16,6 +16,7 @@
 
 #include "ssh_server_mock.h"
 
+#include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -98,16 +99,34 @@ static int mock_channel_subsystem(ssh_session session,
     return SSH_OK;
 }
 
+/* Consolidated cleanup for the server thread */
+struct server_resources {
+    ssh_bind sshbind;
+    ssh_session session;
+    ssh_event event;
+};
+
+static void cleanup_server_resources(void *arg)
+{
+    struct server_resources *res = (struct server_resources *)arg;
+    ssh_event_free(res->event);
+    if (res->session) {
+        ssh_disconnect(res->session);
+        ssh_free(res->session);
+    }
+    ssh_bind_free(res->sshbind);
+}
+
 /* Server thread implementation */
 static void *server_thread_func(void *arg)
 {
     struct ssh_mock_server_config *config =
         (struct ssh_mock_server_config *)arg;
-    ssh_bind sshbind = NULL;
-    ssh_session session = NULL;
-    ssh_event event = NULL;
     struct mock_session_data sdata = {0};
     sdata.config = config;
+    int rc;
+
+    struct server_resources res = {NULL, NULL, NULL};
 
     struct ssh_server_callbacks_struct server_cb = {
         .userdata = &sdata,
@@ -123,57 +142,57 @@ static void *server_thread_func(void *arg)
 
     bool no = false;
 
-    sshbind = ssh_bind_new();
-    if (sshbind == NULL) {
+    res.sshbind = ssh_bind_new();
+    if (res.sshbind == NULL) {
         config->server_error = true;
-        return NULL;
+        goto cleanup;
     }
 
-    session = ssh_new();
-    if (session == NULL) {
-        ssh_bind_free(sshbind);
+    res.session = ssh_new();
+    if (res.session == NULL) {
         config->server_error = true;
-        return NULL;
+        goto cleanup;
     }
 
     const char *cipher = config->cipher ? config->cipher : "aes128-ctr";
     const char *hmac = config->hmac ? config->hmac : "hmac-sha1";
 
-    ssh_bind_options_set(sshbind,
+    ssh_bind_options_set(res.sshbind,
                          SSH_BIND_OPTIONS_HOSTKEY,
                          SSH_MOCK_HOSTKEY_PATH);
-    ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_CIPHERS_C_S, cipher);
-    ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_CIPHERS_S_C, cipher);
-    ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HMAC_C_S, hmac);
-    ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HMAC_S_C, hmac);
-    ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_PROCESS_CONFIG, &no);
+    ssh_bind_options_set(res.sshbind, SSH_BIND_OPTIONS_CIPHERS_C_S, cipher);
+    ssh_bind_options_set(res.sshbind, SSH_BIND_OPTIONS_CIPHERS_S_C, cipher);
+    ssh_bind_options_set(res.sshbind, SSH_BIND_OPTIONS_HMAC_C_S, hmac);
+    ssh_bind_options_set(res.sshbind, SSH_BIND_OPTIONS_HMAC_S_C, hmac);
+    ssh_bind_options_set(res.sshbind, SSH_BIND_OPTIONS_PROCESS_CONFIG, &no);
 
-    ssh_set_auth_methods(session, SSH_AUTH_METHOD_NONE);
+    ssh_set_auth_methods(res.session, SSH_AUTH_METHOD_NONE);
     ssh_callbacks_init(&server_cb);
-    ssh_set_server_callbacks(session, &server_cb);
+    ssh_set_server_callbacks(res.session, &server_cb);
 
-    if (ssh_bind_accept_fd(sshbind, session, config->server_socket) != SSH_OK) {
-        ssh_free(session);
-        ssh_bind_free(sshbind);
+    /* Bound libssh's internal poll in ssh_handle_key_exchange */
+    long server_timeout = 1;
+    ssh_options_set(res.session, SSH_OPTIONS_TIMEOUT, &server_timeout);
+
+    rc = ssh_bind_accept_fd(res.sshbind, res.session, config->server_socket);
+    if (rc != SSH_OK) {
         config->server_error = true;
-        return NULL;
+        goto cleanup;
     }
 
     config->server_ready = true;
 
-    event = ssh_event_new();
-    if (event == NULL) {
-        ssh_disconnect(session);
-        ssh_free(session);
-        ssh_bind_free(sshbind);
-        return NULL;
+    res.event = ssh_event_new();
+    if (res.event == NULL) {
+        goto cleanup;
     }
 
-    if (ssh_handle_key_exchange(session) == SSH_OK) {
-        ssh_event_add_session(event, session);
+    if (ssh_handle_key_exchange(res.session) == SSH_OK) {
+        ssh_event_add_session(res.event, res.session);
 
-        for (int i = 0; i < 50 && !sdata.channel; i++) {
-            ssh_event_dopoll(event, 1);
+        for (int i = 0; i < 50 && !sdata.channel && !config->shutdown_requested;
+             i++) {
+            ssh_event_dopoll(res.event, 1);
         }
 
         if (sdata.channel) {
@@ -183,23 +202,18 @@ static void *server_thread_func(void *arg)
             int max_iterations = 30;
             for (int iter = 0; iter < max_iterations &&
                                !ssh_channel_is_closed(sdata.channel) &&
-                               !ssh_channel_is_eof(sdata.channel);
+                               !ssh_channel_is_eof(sdata.channel) &&
+                               !config->shutdown_requested;
                  iter++) {
-                if (ssh_event_dopoll(event, 100) == SSH_ERROR) {
+                if (ssh_event_dopoll(res.event, 100) == SSH_ERROR) {
                     break;
                 }
             }
         }
     }
 
-    if (event)
-        ssh_event_free(event);
-    if (session) {
-        ssh_disconnect(session);
-        ssh_free(session);
-    }
-    if (sshbind)
-        ssh_bind_free(sshbind);
+cleanup:
+    cleanup_server_resources(&res);
 
     return NULL;
 }
@@ -223,10 +237,15 @@ int ssh_mock_server_start(struct ssh_mock_server_config *config,
         usleep(100);
     }
 
-    return config->server_error ? -1 : 0;
+    if (config->server_error) {
+        pthread_join(*thread, NULL);
+        return -1;
+    }
+
+    return 0;
 }
 
-/* Generic protocol callback - sends raw fuzzer data for any protocol */
+/* Generic protocol callback */
 int ssh_mock_send_raw_data(void *channel,
                            const void *data,
                            size_t size,
@@ -236,7 +255,7 @@ int ssh_mock_send_raw_data(void *channel,
 
     ssh_channel target_channel = (ssh_channel)channel;
 
-    /* Send raw fuzzer data - let protocol parser interpret it */
+    /* Send raw fuzzer data */
     if (size > 0) {
         ssh_channel_write(target_channel, data, size);
     }

tests/fuzz/ssh_server_mock.h

@@ -38,8 +38,9 @@ struct ssh_mock_server_config {
     const char *hmac;
     int server_socket;
     int client_socket;
-    bool server_ready;
-    bool server_error;
+    volatile bool server_ready;
+    volatile bool server_error;
+    volatile bool shutdown_requested;
 };
 
 /* Public API functions */