From bed4438695df2f4635617fc45f802d782fdd8479 Mon Sep 17 00:00:00 2001 From: Jakub Jelen Date: Thu, 4 Jul 2024 18:28:43 +0200 Subject: [PATCH] Allow building without the exec() supported ... .. to satisfy restricted environment or fuzzers We are encountering weird issues in the oss-fuzz that the file disappears during coverage build so I assume some corpus sneaked in, that contains some commands that end up being executed as part of the coverage run causing it randomly failing. The solution I propose is to build fuzzers without ability to call arbitrary commands on the filesystem (such as `rm -rf /`) as this is not the point the fuzzers should be testing. This is controlled by the WITH_EXEC CMake option (enabled by default). https://github.com/google/oss-fuzz/issues/10136 Signed-off-by: Jakub Jelen Reviewed-by: Sahana Prasad Reviewed-by: Eshan Kelkar --- .gitlab-ci.yml | 1 + CMakeLists.txt | 1 + DefineOptions.cmake | 5 +++++ config.h.cmake | 7 ++++++- include/libssh/socket.h | 2 +- src/client.c | 9 +++++++-- src/config.c | 31 +++++++++++++++-------------- src/socket.c | 8 +++----- tests/client/torture_proxycommand.c | 18 +++++++++++++++++ tests/fuzz/README.md | 15 +++++++++++++- tests/unittests/torture_config.c | 12 +++++------ tests/unittests/torture_options.c | 6 +++--- 12 files changed, 81 insertions(+), 34 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3d4e9d04..849738e0 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -217,6 +217,7 @@ fedora/openssl_3.0.x/x86_64/minimal: variables: script: - cmake $CMAKE_DEFAULT_OPTIONS + -DWITH_EXEC=OFF -DWITH_SFTP=OFF -DWITH_SERVER=OFF -DWITH_ZLIB=OFF diff --git a/CMakeLists.txt b/CMakeLists.txt index 42cacfdb..d3b46c09 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -226,6 +226,7 @@ message(STATUS "Server support : ${WITH_SERVER}") message(STATUS "GSSAPI support : ${WITH_GSSAPI}") message(STATUS "GEX support : ${WITH_GEX}") message(STATUS "Support insecure none cipher and MAC : ${WITH_INSECURE_NONE}") +message(STATUS "Support exec : ${WITH_EXEC}") message(STATUS "Pcap debugging support : ${WITH_PCAP}") message(STATUS "Build shared library: ${BUILD_SHARED_LIBS}") message(STATUS "Unit testing: ${UNIT_TESTING}") diff --git a/DefineOptions.cmake b/DefineOptions.cmake index ccb277e4..d449ef34 100644 --- a/DefineOptions.cmake +++ b/DefineOptions.cmake @@ -23,6 +23,7 @@ option(WITH_SYMBOL_VERSIONING "Build with symbol versioning" ON) option(WITH_ABI_BREAK "Allow ABI break" OFF) option(WITH_GEX "Enable DH Group exchange mechanisms" ON) option(WITH_INSECURE_NONE "Enable insecure none cipher and MAC algorithms (not suitable for production!)" OFF) +option(WITH_EXEC "Enable libssh to execute arbitrary commands from configuration files or options (match exec, proxy commands and OpenSSH-based proxy-jumps)." ON) option(FUZZ_TESTING "Build with fuzzer for the server and client (automatically enables none cipher!)" OFF) option(PICKY_DEVELOPER "Build with picky developer flags" OFF) @@ -60,3 +61,7 @@ endif (NOT GLOBAL_CLIENT_CONFIG) if (FUZZ_TESTING) set(WITH_INSECURE_NONE ON) endif (FUZZ_TESTING) + +if (WIN32) + set(WITH_EXEC 0) +endif(WIN32) diff --git a/config.h.cmake b/config.h.cmake index b4a44bc3..9dec02bf 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -231,9 +231,14 @@ /* Define to 1 if you want to enable DH group exchange algorithms */ #cmakedefine WITH_GEX 1 -/* Define to 1 if you want to enable none cipher and MAC */ +/* Define to 1 if you want to enable insecure none cipher and MAC */ #cmakedefine WITH_INSECURE_NONE 1 +/* Define to 1 if you want to allow libssh to execute arbitrary commands from + * configuration files or options (match exec, proxy commands and OpenSSH-based + * proxy-jumps). */ +#cmakedefine WITH_EXEC 1 + /* Define to 1 if you want to enable blowfish cipher support */ #cmakedefine WITH_BLOWFISH_CIPHER 1 diff --git a/include/libssh/socket.h b/include/libssh/socket.h index cdd3c837..7c011280 100644 --- a/include/libssh/socket.h +++ b/include/libssh/socket.h @@ -37,8 +37,8 @@ void ssh_socket_set_fd(ssh_socket s, socket_t fd); socket_t ssh_socket_get_fd(ssh_socket s); void ssh_socket_set_connected(ssh_socket s, struct ssh_poll_handle_struct *p); int ssh_socket_unix(ssh_socket s, const char *path); +#if WITH_EXEC void ssh_execute_command(const char *command, socket_t in, socket_t out); -#ifndef _WIN32 int ssh_socket_connect_proxycommand(ssh_socket s, const char *command); #endif void ssh_socket_close(ssh_socket s); diff --git a/src/client.c b/src/client.c index 140ab3e5..e6451ce2 100644 --- a/src/client.c +++ b/src/client.c @@ -589,11 +589,16 @@ int ssh_connect(ssh_session session) session->session_state = SSH_SESSION_STATE_SOCKET_CONNECTED; ssh_socket_set_fd(session->socket, session->opts.fd); ret = SSH_OK; -#ifndef _WIN32 } else if (session->opts.ProxyCommand != NULL) { +#ifdef WITH_EXEC ret = ssh_socket_connect_proxycommand(session->socket, session->opts.ProxyCommand); -#endif +#else + ssh_set_error(session, + SSH_FATAL, + "The libssh is built without support for proxy commands."); + ret = SSH_ERROR; +#endif /* WITH_EXEC */ } else { ret = ssh_socket_connect(session->socket, session->opts.host, diff --git a/src/config.c b/src/config.c index 1c3fd482..4687cce4 100644 --- a/src/config.c +++ b/src/config.c @@ -305,20 +305,8 @@ ssh_config_match(char *value, const char *pattern, bool negate) return result; } -#ifdef _WIN32 -static int -ssh_match_exec(ssh_session session, const char *command, bool negate) -{ - (void) session; - (void) command; - (void) negate; - - SSH_LOG(SSH_LOG_TRACE, "Unsupported 'exec' command on Windows '%s'", - command); - return 0; -} -#else /* _WIN32 */ - +#ifdef WITH_EXEC +/* FIXME reuse the ssh_execute_command() from socket.c */ static int ssh_exec_shell(char *cmd) { @@ -431,7 +419,20 @@ ssh_match_exec(ssh_session session, const char *command, bool negate) free(cmd); return result; } -#endif /* _WIN32 */ +#else +static int +ssh_match_exec(ssh_session session, const char *command, bool negate) +{ + (void)session; + (void)command; + (void)negate; + + SSH_LOG(SSH_LOG_TRACE, + "Unsupported 'exec' command on Windows '%s'", + command); + return 0; +} +#endif /* WITH_EXEC */ /* @brief: Parse the ProxyJump configuration line and if parsing, * stores the result in the configuration option diff --git a/src/socket.c b/src/socket.c index 9dc4cbdd..dc76b788 100644 --- a/src/socket.c +++ b/src/socket.c @@ -881,7 +881,7 @@ int ssh_socket_connect(ssh_socket s, return SSH_OK; } -#ifndef _WIN32 +#ifdef WITH_EXEC /** * @internal * @brief executes a command and redirect input and outputs @@ -910,7 +910,7 @@ ssh_execute_command(const char *command, socket_t in, socket_t out) */ shell = getenv("SHELL"); if (shell == NULL || shell[0] == '\0') { - /* Fall back to the /bin/sh only if the bash is not available. But there are + /* Fall back to the /bin/sh only if the bash is not available. But there are * issues with dash or whatever people tend to link to /bin/sh */ rc = access("/bin/bash", 0); if (rc != 0) { @@ -947,7 +947,6 @@ ssh_execute_command(const char *command, socket_t in, socket_t out) * @returns SSH_OK socket is being connected. * @returns SSH_ERROR error while executing the command. */ - int ssh_socket_connect_proxycommand(ssh_socket s, const char *command) { @@ -987,6 +986,5 @@ ssh_socket_connect_proxycommand(ssh_socket s, const char *command) return SSH_OK; } - -#endif /* _WIN32 */ +#endif /* WITH_EXEC */ /** @} */ diff --git a/tests/client/torture_proxycommand.c b/tests/client/torture_proxycommand.c index 1bad4ccc..1d3798a5 100644 --- a/tests/client/torture_proxycommand.c +++ b/tests/client/torture_proxycommand.c @@ -69,7 +69,9 @@ static void torture_options_set_proxycommand(void **state) char command[255] = {0}; struct stat sb; int rc; +#ifdef WITH_EXEC socket_t fd; +#endif rc = stat(NCAT_EXECUTABLE, &sb); if (rc != 0 || (sb.st_mode & S_IXOTH) == 0) { @@ -88,11 +90,15 @@ static void torture_options_set_proxycommand(void **state) rc = ssh_options_set(session, SSH_OPTIONS_PROXYCOMMAND, command); assert_int_equal(rc, 0); rc = ssh_connect(session); +#ifdef WITH_EXEC assert_ssh_return_code(session, rc); fd = ssh_get_fd(session); assert_true(fd != SSH_INVALID_SOCKET); rc = fcntl(fd, F_GETFL); assert_int_equal(rc & O_RDWR, O_RDWR); +#else + assert_int_equal(rc, SSH_ERROR); +#endif /* WITH_EXEC */ } #else /* NCAT_EXECUTABLE */ @@ -124,7 +130,9 @@ static void torture_options_set_proxycommand_ssh(void **state) const char *address = torture_server_address(AF_INET); char command[255] = {0}; int rc; +#ifdef WITH_EXEC socket_t fd; +#endif rc = snprintf(command, sizeof(command), "ssh -oStrictHostKeyChecking=no -oUserKnownHostsFile=/dev/null -W [%%h]:%%p alice@%s", @@ -134,11 +142,15 @@ static void torture_options_set_proxycommand_ssh(void **state) rc = ssh_options_set(session, SSH_OPTIONS_PROXYCOMMAND, command); assert_int_equal(rc, 0); rc = ssh_connect(session); +#ifdef WITH_EXEC assert_ssh_return_code(session, rc); fd = ssh_get_fd(session); assert_true(fd != SSH_INVALID_SOCKET); rc = fcntl(fd, F_GETFL); assert_int_equal(rc & O_RDWR, O_RDWR); +#else + assert_int_equal(rc, SSH_ERROR); +#endif /* WITH_EXEC */ } static void torture_options_set_proxycommand_ssh_stderr(void **state) @@ -148,7 +160,9 @@ static void torture_options_set_proxycommand_ssh_stderr(void **state) const char *address = torture_server_address(AF_INET); char command[255] = {0}; int rc; +#ifdef WITH_EXEC socket_t fd; +#endif /* The -vvv switches produce the desired output on the standard error */ rc = snprintf(command, sizeof(command), @@ -159,11 +173,15 @@ static void torture_options_set_proxycommand_ssh_stderr(void **state) rc = ssh_options_set(session, SSH_OPTIONS_PROXYCOMMAND, command); assert_int_equal(rc, 0); rc = ssh_connect(session); +#ifdef WITH_EXEC assert_ssh_return_code(session, rc); fd = ssh_get_fd(session); assert_true(fd != SSH_INVALID_SOCKET); rc = fcntl(fd, F_GETFL); assert_int_equal(rc & O_RDWR, O_RDWR); +#else + assert_int_equal(rc, SSH_ERROR); +#endif /* WITH_EXEC */ } static void torture_options_proxycommand_injection(void **state) diff --git a/tests/fuzz/README.md b/tests/fuzz/README.md index 088ce27d..a8f9a1be 100644 --- a/tests/fuzz/README.md +++ b/tests/fuzz/README.md @@ -10,6 +10,8 @@ but they are suitable for debugging. ## Background +### Turn off encryption + Fuzzing ssh protocol is complicated by the way that all the communication between client and server is encrypted and authenticated using keys based on random data, making it impossible to fuzz the actual underlying protocol @@ -17,6 +19,17 @@ as every change in the encrypted data causes integrity errors. For that reason, libssh needs to implement "none" cipher and MAC as described in RFC 4253 and these need to be used during fuzzing to be able to accomplish reproducibility and for fuzzers to be able to progress behind key exchange. +This is enabled with the `WITH_INSECURE_NONE` CMake option. + +### Do not allow filesystem modification + +The OpenSSH configuration files are quite rich and expects users to know what +they do when they write their configuration files. The fuzzer driver is not an +average user so it is very happy to try whatever commands come to its "mind", +including `rm -rf /` and libssh would be very happy to run it by default. This +might remove some parts of the system that are mandatory for fuzzing. +To avoid executing dangerous commands like this, the `WITH_EXEC=OFF` CMake +option prevents invoking any external command through `exec()` syscall. ## Corpus creation @@ -31,7 +44,7 @@ to use none cipher for the key exchange to be plausible. * Compile libssh with support for none cipher and pcap: - cmake -DWITH_INSECURE_NONE=ON -DWITH_PCAP=ON ../ + cmake -DWITH_INSECURE_NONE=ON -DWITH_EXEC=OFF -DWITH_PCAP=ON ../ * Create a configuration file enabling none cipher and mac: diff --git a/tests/unittests/torture_config.c b/tests/unittests/torture_config.c index 83a5f773..004e503e 100644 --- a/tests/unittests/torture_config.c +++ b/tests/unittests/torture_config.c @@ -862,7 +862,7 @@ static void torture_config_match(void **state, } torture_reset_config(session); _parse_config(session, file, string, SSH_OK); -#ifdef _WIN32 +#ifndef WITH_EXEC /* The match exec is not supported on windows at this moment */ assert_string_equal(session->opts.host, "otherhost"); #else @@ -878,7 +878,7 @@ static void torture_config_match(void **state, } torture_reset_config(session); _parse_config(session, file, string, SSH_OK); -#ifdef _WIN32 +#ifndef WITH_EXEC /* The match exec is not supported on windows at this moment */ assert_string_equal(session->opts.host, "otherhost"); #else @@ -894,7 +894,7 @@ static void torture_config_match(void **state, } torture_reset_config(session); _parse_config(session, file, string, SSH_OK); -#ifdef _WIN32 +#ifndef WITH_EXEC /* The match exec is not supported on windows at this moment */ assert_string_equal(session->opts.host, "otherhost"); #else @@ -1855,7 +1855,7 @@ static void torture_config_parser_get_cmd(void **state) { char *p = NULL, *tok = NULL; char data[256]; -#ifdef __unix__ +#ifdef WITH_EXEC FILE *outfile = NULL, *infile = NULL; int pid; char buffer[256] = {0}; @@ -1899,7 +1899,7 @@ static void torture_config_parser_get_cmd(void **state) assert_string_equal(tok, data); assert_int_equal(*p, '\0'); -#ifdef __unix__ +#ifdef WITH_EXEC /* Check if the command would get correctly executed * Use the script file "hello world.sh" to echo the first argument * Run as <= "/workdir/hello world.sh" "hello libssh" => */ @@ -1929,7 +1929,7 @@ static void torture_config_parser_get_cmd(void **state) fclose(outfile); assert_string_equal(buffer, "hello libssh"); -#endif +#endif /* WITH_EXEC */ } /* ssh_config_get_token() should behave as expected diff --git a/tests/unittests/torture_options.c b/tests/unittests/torture_options.c index 40d523eb..22e84863 100644 --- a/tests/unittests/torture_options.c +++ b/tests/unittests/torture_options.c @@ -1147,7 +1147,7 @@ static void torture_options_config_match(void **state) rv = ssh_options_parse_config(session, "test_config"); assert_ssh_return_code(session, rv); -#ifdef _WIN32 +#ifndef WITH_EXEC /* The match exec is not supported on windows at this moment */ assert_int_equal(session->opts.port, 34); #else @@ -1169,7 +1169,7 @@ static void torture_options_config_match(void **state) rv = ssh_options_parse_config(session, "test_config"); assert_ssh_return_code(session, rv); -#ifdef _WIN32 +#ifndef WITH_EXEC /* The match exec is not supported on windows at this moment */ assert_int_equal(session->opts.port, 34); #else @@ -1222,7 +1222,7 @@ static void torture_options_config_match_multi(void **state) rv = ssh_options_parse_config(session, "test_config"); assert_ssh_return_code(session, rv); -#ifdef _WIN32 +#ifndef WITH_EXEC /* The match exec is not supported on windows at this moment */ assert_int_equal(session->opts.port, 34); #else