From 11bd6e6ad926a38cd7b9f8308a4c2fd8dfd9200c Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Sun, 5 Nov 2023 13:12:47 +0100 Subject: [PATCH 01/12] CVE-2023-6004: torture_config: Allow multiple '@' in usernames Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- tests/unittests/torture_config.c | 44 ++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/tests/unittests/torture_config.c b/tests/unittests/torture_config.c index f91112a9..3a5a74bf 100644 --- a/tests/unittests/torture_config.c +++ b/tests/unittests/torture_config.c @@ -671,24 +671,40 @@ static void torture_config_proxyjump(void **state) { assert_string_equal(session->opts.ProxyCommand, "ssh -W [%h]:%p 2620:52:0::fed"); - /* Try to create some invalid configurations */ - /* Non-numeric port */ + /* Multiple @ is allowed in second jump */ torture_write_file(LIBSSH_TESTCONFIG11, - "Host bad-port\n" - "\tProxyJump jumpbox:22bad22\n" + "Host allowed-hostname\n" + "\tProxyJump localhost,user@principal.com@jumpbox:22\n" ""); torture_reset_config(session); - ssh_options_set(session, SSH_OPTIONS_HOST, "bad-port"); + ssh_options_set(session, SSH_OPTIONS_HOST, "allowed-hostname"); ret = ssh_config_parse_file(session, LIBSSH_TESTCONFIG11); - assert_ssh_return_code_equal(session, ret, SSH_ERROR); + assert_ssh_return_code(session, ret); + assert_string_equal(session->opts.ProxyCommand, + "ssh -J user@principal.com@jumpbox:22 -W [%h]:%p localhost"); - /* Too many @ */ + /* Multiple @ is allowed */ torture_write_file(LIBSSH_TESTCONFIG11, - "Host bad-hostname\n" + "Host allowed-hostname\n" "\tProxyJump user@principal.com@jumpbox:22\n" ""); torture_reset_config(session); - ssh_options_set(session, SSH_OPTIONS_HOST, "bad-hostname"); + ssh_options_set(session, SSH_OPTIONS_HOST, "allowed-hostname"); + ret = ssh_config_parse_file(session, LIBSSH_TESTCONFIG11); + assert_ssh_return_code(session, ret); + assert_string_equal(session->opts.ProxyCommand, + "ssh -l user@principal.com -p 22 -W [%h]:%p jumpbox"); + + /* In this part, we try various other config files and strings. */ + + /* Try to create some invalid configurations */ + /* Non-numeric port */ + torture_write_file(LIBSSH_TESTCONFIG11, + "Host bad-port\n" + "\tProxyJump jumpbox:22bad22\n" + ""); + torture_reset_config(session); + ssh_options_set(session, SSH_OPTIONS_HOST, "bad-port"); ret = ssh_config_parse_file(session, LIBSSH_TESTCONFIG11); assert_ssh_return_code_equal(session, ret, SSH_ERROR); @@ -752,16 +768,6 @@ static void torture_config_proxyjump(void **state) { ret = ssh_config_parse_file(session, LIBSSH_TESTCONFIG11); assert_ssh_return_code_equal(session, ret, SSH_ERROR); - /* Too many @ in second jump */ - torture_write_file(LIBSSH_TESTCONFIG11, - "Host bad-hostname\n" - "\tProxyJump localhost,user@principal.com@jumpbox:22\n" - ""); - torture_reset_config(session); - ssh_options_set(session, SSH_OPTIONS_HOST, "bad-hostname"); - ret = ssh_config_parse_file(session, LIBSSH_TESTCONFIG11); - assert_ssh_return_code_equal(session, ret, SSH_ERROR); - /* Braces mismatch in second jump */ torture_write_file(LIBSSH_TESTCONFIG11, "Host mismatch\n" -- 2.41.0 From c3234e5f94b96d6e29f0c1c82821c1e3ebb181ed Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Wed, 1 Nov 2023 11:24:43 +0100 Subject: [PATCH 02/12] CVE-2023-6004: config_parser: Allow multiple '@' in usernames Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- src/config_parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config_parser.c b/src/config_parser.c index ae2aa2c8..76cca224 100644 --- a/src/config_parser.c +++ b/src/config_parser.c @@ -152,7 +152,7 @@ int ssh_config_parse_uri(const char *tok, } /* Username part (optional) */ - endp = strchr(tok, '@'); + endp = strrchr(tok, '@'); if (endp != NULL) { /* Zero-length username is not valid */ if (tok == endp) { -- 2.41.0 From a5b8bd0d8841296cf71d927824d60f576581243f Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Tue, 31 Oct 2023 09:48:52 +0100 Subject: [PATCH 03/12] CVE-2023-6004: options: Simplify the hostname parsing in ssh_options_set Using ssh_config_parse_uri can simplify the parsing of the host parsing inside the function of ssh_options_set Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- src/options.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/options.c b/src/options.c index b5f951ac..7c03e7ab 100644 --- a/src/options.c +++ b/src/options.c @@ -36,6 +36,7 @@ #include "libssh/session.h" #include "libssh/misc.h" #include "libssh/options.h" +#include "libssh/config_parser.h" #ifdef WITH_SERVER #include "libssh/server.h" #include "libssh/bind.h" @@ -490,33 +491,24 @@ int ssh_options_set(ssh_session session, enum ssh_options_e type, ssh_set_error_invalid(session); return -1; } else { - q = strdup(value); - if (q == NULL) { - ssh_set_error_oom(session); + char *username = NULL, *hostname = NULL, *port = NULL; + rc = ssh_config_parse_uri(value, &username, &hostname, &port); + if (rc != SSH_OK) { return -1; } - p = strchr(q, '@'); - - SAFE_FREE(session->opts.host); - - if (p) { - *p = '\0'; - session->opts.host = strdup(p + 1); - if (session->opts.host == NULL) { - SAFE_FREE(q); - ssh_set_error_oom(session); - return -1; - } - + if (port != NULL) { + SAFE_FREE(username); + SAFE_FREE(hostname); + SAFE_FREE(port); + return -1; + } + if (username != NULL) { SAFE_FREE(session->opts.username); - session->opts.username = strdup(q); - SAFE_FREE(q); - if (session->opts.username == NULL) { - ssh_set_error_oom(session); - return -1; - } - } else { - session->opts.host = q; + session->opts.username = username; + } + if (hostname != NULL) { + SAFE_FREE(session->opts.host); + session->opts.host = hostname; } } break; -- 2.41.0 From efb24b6472e8b87c5832c0590f14e99e82fcdeeb Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Tue, 10 Oct 2023 12:44:16 +0200 Subject: [PATCH 04/12] CVE-2023-6004: misc: Add function to check allowed characters of a hostname The hostname can be a domain name or an ip address. The colon has to be allowed because of IPv6 even it is prohibited in domain names. Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- include/libssh/misc.h | 2 ++ src/misc.c | 68 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/include/libssh/misc.h b/include/libssh/misc.h index 3cc3b113..a5bee930 100644 --- a/include/libssh/misc.h +++ b/include/libssh/misc.h @@ -97,4 +97,6 @@ int ssh_mkdirs(const char *pathname, mode_t mode); int ssh_quote_file_name(const char *file_name, char *buf, size_t buf_len); int ssh_newline_vis(const char *string, char *buf, size_t buf_len); +int ssh_check_hostname_syntax(const char *hostname); + #endif /* MISC_H_ */ diff --git a/src/misc.c b/src/misc.c index 149eb85e..e4239e81 100644 --- a/src/misc.c +++ b/src/misc.c @@ -94,6 +94,8 @@ #define ZLIB_STRING "" #endif +#define ARPA_DOMAIN_MAX_LEN 63 + /** * @defgroup libssh_misc The SSH helper functions. * @ingroup libssh @@ -1734,4 +1736,70 @@ int ssh_newline_vis(const char *string, char *buf, size_t buf_len) return out - buf; } +/** + * @brief Checks syntax of a domain name + * + * The check is made based on the RFC1035 section 2.3.1 + * Allowed characters are: hyphen, period, digits (0-9) and letters (a-zA-Z) + * + * The label should be no longer than 63 characters + * The label should start with a letter and end with a letter or number + * The label in this implementation can start with a number to allow virtual + * URLs to pass. Note that this will make IPv4 addresses to pass + * this check too. + * + * @param hostname The domain name to be checked, has to be null terminated + * + * @return SSH_OK if the hostname passes syntax check + * SSH_ERROR otherwise or if hostname is NULL or empty string + */ +int ssh_check_hostname_syntax(const char *hostname) +{ + char *it = NULL, *s = NULL, *buf = NULL; + size_t it_len; + char c; + + if (hostname == NULL || strlen(hostname) == 0) { + return SSH_ERROR; + } + + /* strtok_r writes into the string, keep the input clean */ + s = strdup(hostname); + if (s == NULL) { + return SSH_ERROR; + } + + it = strtok_r(s, ".", &buf); + /* if the token has 0 length */ + if (it == NULL) { + free(s); + return SSH_ERROR; + } + do { + it_len = strlen(it); + if (it_len > ARPA_DOMAIN_MAX_LEN || + /* the first char must be a letter, but some virtual urls start + * with a number */ + isalnum(it[0]) == 0 || + isalnum(it[it_len - 1]) == 0) { + free(s); + return SSH_ERROR; + } + while (*it != '\0') { + c = *it; + /* the "." is allowed too, but tokenization removes it from the + * string */ + if (isalnum(c) == 0 && c != '-') { + free(s); + return SSH_ERROR; + } + it++; + } + } while ((it = strtok_r(NULL, ".", &buf)) != NULL); + + free(s); + + return SSH_OK; +} + /** @} */ -- 2.41.0 From 234ecdf4d9705efa3727a54dcc1ddfe6377c7bf6 Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Tue, 10 Oct 2023 12:45:28 +0200 Subject: [PATCH 05/12] CVE-2023-6004: torture_misc: Add test for ssh_check_hostname_syntax Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- tests/unittests/torture_misc.c | 73 ++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/tests/unittests/torture_misc.c b/tests/unittests/torture_misc.c index 0a48abbe..d14f4254 100644 --- a/tests/unittests/torture_misc.c +++ b/tests/unittests/torture_misc.c @@ -656,6 +656,78 @@ static void torture_ssh_newline_vis(UNUSED_PARAM(void **state)) assert_string_equal(buffer, "a\\nb\\n"); } +static void torture_ssh_check_hostname_syntax(void **state) +{ + int rc; + (void)state; + + rc = ssh_check_hostname_syntax("duckduckgo.com"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("www.libssh.org"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("Some-Thing.com"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("amazon.a23456789012345678901234567890123456789012345678901234567890123"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("amazon.a23456789012345678901234567890123456789012345678901234567890123.a23456789012345678901234567890123456789012345678901234567890123.ok"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("amazon.a23456789012345678901234567890123456789012345678901234567890123.a23456789012345678901234567890123456789012345678901234567890123.a23456789012345678901234567890123456789012345678901234567890123"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("lavabo-inter.innocentes-manus-meas"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("localhost"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("a"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("a-0.b-b"); + assert_int_equal(rc, SSH_OK); + rc = ssh_check_hostname_syntax("libssh."); + assert_int_equal(rc, SSH_OK); + + rc = ssh_check_hostname_syntax(NULL); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax(""); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("/"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("@"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("["); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("`"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("{"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("&"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("|"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("\""); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("`"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax(" "); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("*the+giant&\"rooks\".c0m"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("!www.libssh.org"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("--.--"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("libssh.a234567890123456789012345678901234567890123456789012345678901234"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("libssh.a234567890123456789012345678901234567890123456789012345678901234.a234567890123456789012345678901234567890123456789012345678901234"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("libssh-"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("fe80::9656:d028:8652:66b6"); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax("."); + assert_int_equal(rc, SSH_ERROR); + rc = ssh_check_hostname_syntax(".."); + assert_int_equal(rc, SSH_ERROR); +} + int torture_run_tests(void) { int rc; struct CMUnitTest tests[] = { @@ -678,6 +750,7 @@ int torture_run_tests(void) { cmocka_unit_test(torture_ssh_newline_vis), cmocka_unit_test(torture_ssh_mkdirs), cmocka_unit_test(torture_ssh_quote_file_name), + cmocka_unit_test(torture_ssh_check_hostname_syntax), }; ssh_init(); -- 2.41.0 From 4d7ae19e9cd8c407012b40f3f2eaf480bfb1da7d Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Tue, 10 Oct 2023 18:33:56 +0200 Subject: [PATCH 06/12] CVE-2023-6004: config_parser: Check for valid syntax of a hostname if it is a domain name This prevents code injection. The domain name syntax checker is based on RFC1035. Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- src/config_parser.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/config_parser.c b/src/config_parser.c index 76cca224..87bac5d4 100644 --- a/src/config_parser.c +++ b/src/config_parser.c @@ -30,6 +30,7 @@ #include "libssh/config_parser.h" #include "libssh/priv.h" +#include "libssh/misc.h" char *ssh_config_get_cmd(char **str) { @@ -139,6 +140,7 @@ int ssh_config_parse_uri(const char *tok, { char *endp = NULL; long port_n; + int rc; /* Sanitize inputs */ if (username != NULL) { @@ -196,6 +198,14 @@ int ssh_config_parse_uri(const char *tok, if (*hostname == NULL) { goto error; } + /* if not an ip, check syntax */ + rc = ssh_is_ipaddr(*hostname); + if (rc == 0) { + rc = ssh_check_hostname_syntax(*hostname); + if (rc != SSH_OK) { + goto error; + } + } } /* Skip also the closing bracket */ if (*endp == ']') { -- 2.41.0 From 8cf4f4bfda968ab526c1a601ea1030bbaccaba17 Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Tue, 10 Oct 2023 10:28:47 +0200 Subject: [PATCH 07/12] CVE-2023-6004: torture_proxycommand: Add test for proxycommand injection Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- tests/client/torture_proxycommand.c | 53 +++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/client/torture_proxycommand.c b/tests/client/torture_proxycommand.c index c04ff2ab..dc17f3d8 100644 --- a/tests/client/torture_proxycommand.c +++ b/tests/client/torture_proxycommand.c @@ -161,6 +161,56 @@ static void torture_options_set_proxycommand_ssh_stderr(void **state) assert_int_equal(rc & O_RDWR, O_RDWR); } +static void torture_options_proxycommand_injection(void **state) +{ + struct torture_state *s = *state; + struct passwd *pwd = NULL; + const char *malicious_host = "`echo foo > mfile`"; + const char *command = "nc %h %p"; + char *current_dir = NULL; + char *malicious_file_path = NULL; + int mfp_len; + int verbosity = torture_libssh_verbosity(); + struct stat sb; + int rc; + + pwd = getpwnam("bob"); + assert_non_null(pwd); + + rc = setuid(pwd->pw_uid); + assert_return_code(rc, errno); + + s->ssh.session = ssh_new(); + assert_non_null(s->ssh.session); + + ssh_options_set(s->ssh.session, SSH_OPTIONS_LOG_VERBOSITY, &verbosity); + // if we would be checking the rc, this should fail + ssh_options_set(s->ssh.session, SSH_OPTIONS_HOST, malicious_host); + + ssh_options_set(s->ssh.session, SSH_OPTIONS_USER, TORTURE_SSH_USER_ALICE); + + rc = ssh_options_set(s->ssh.session, SSH_OPTIONS_PROXYCOMMAND, command); + assert_int_equal(rc, 0); + rc = ssh_connect(s->ssh.session); + assert_ssh_return_code_equal(s->ssh.session, rc, SSH_ERROR); + + current_dir = torture_get_current_working_dir(); + assert_non_null(current_dir); + mfp_len = strlen(current_dir) + 6; + malicious_file_path = malloc(mfp_len); + assert_non_null(malicious_file_path); + rc = snprintf(malicious_file_path, mfp_len, + "%s/mfile", current_dir); + assert_int_equal(rc, mfp_len); + free(current_dir); + rc = stat(malicious_file_path, &sb); + assert_int_not_equal(rc, 0); + + // cleanup + remove(malicious_file_path); + free(malicious_file_path); +} + int torture_run_tests(void) { int rc; struct CMUnitTest tests[] = { @@ -176,6 +226,9 @@ int torture_run_tests(void) { cmocka_unit_test_setup_teardown(torture_options_set_proxycommand_ssh_stderr, session_setup, session_teardown), + cmocka_unit_test_setup_teardown(torture_options_proxycommand_injection, + NULL, + session_teardown), }; -- 2.41.0 From a0dbe0d556e073804cc549802569577bb24757d9 Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Mon, 6 Nov 2023 20:11:38 +0100 Subject: [PATCH 08/12] CVE-2023-6004: torture_misc: Add test for ssh_is_ipaddr Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- tests/unittests/torture_misc.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/unittests/torture_misc.c b/tests/unittests/torture_misc.c index d14f4254..073bc54c 100644 --- a/tests/unittests/torture_misc.c +++ b/tests/unittests/torture_misc.c @@ -728,6 +728,31 @@ static void torture_ssh_check_hostname_syntax(void **state) assert_int_equal(rc, SSH_ERROR); } +static void torture_ssh_is_ipaddr(void **state) { + int rc; + (void)state; + + rc = ssh_is_ipaddr("201.255.3.69"); + assert_int_equal(rc, 1); + rc = ssh_is_ipaddr("::1"); + assert_int_equal(rc, 1); + rc = ssh_is_ipaddr("2001:0db8:85a3:0000:0000:8a2e:0370:7334"); + assert_int_equal(rc, 1); + + rc = ssh_is_ipaddr(".."); + assert_int_equal(rc, 0); + rc = ssh_is_ipaddr(":::"); + assert_int_equal(rc, 0); + rc = ssh_is_ipaddr("1.1.1.1.1"); + assert_int_equal(rc, 0); + rc = ssh_is_ipaddr("1.1"); + assert_int_equal(rc, 0); + rc = ssh_is_ipaddr("caesar"); + assert_int_equal(rc, 0); + rc = ssh_is_ipaddr("::xa:1"); + assert_int_equal(rc, 0); +} + int torture_run_tests(void) { int rc; struct CMUnitTest tests[] = { @@ -751,6 +776,7 @@ int torture_run_tests(void) { cmocka_unit_test(torture_ssh_mkdirs), cmocka_unit_test(torture_ssh_quote_file_name), cmocka_unit_test(torture_ssh_check_hostname_syntax), + cmocka_unit_test(torture_ssh_is_ipaddr), }; ssh_init(); -- 2.41.0 From cdaec0d6273243a03f460cc5ba1a2265b4afb93a Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Tue, 28 Nov 2023 15:26:45 +0100 Subject: [PATCH 09/12] CVE-2023-6004: misc: Add ipv6 link-local check for an ip address Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- src/CMakeLists.txt | 17 ++++++++++------- src/connect.c | 2 +- src/misc.c | 44 ++++++++++++++++++++++++++++++++++++++------ 3 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a576cf71..fc401793 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -9,13 +9,6 @@ set(LIBSSH_LINK_LIBRARIES ${LIBSSH_REQUIRED_LIBRARIES} ) -if (WIN32) - set(LIBSSH_LINK_LIBRARIES - ${LIBSSH_LINK_LIBRARIES} - ws2_32 - ) -endif (WIN32) - if (OPENSSL_CRYPTO_LIBRARIES) set(LIBSSH_PRIVATE_INCLUDE_DIRS ${LIBSSH_PRIVATE_INCLUDE_DIRS} @@ -93,6 +86,16 @@ if (MINGW AND Threads_FOUND) ) endif() +# This needs to be last for mingw to build +# https://gitlab.com/libssh/libssh-mirror/-/issues/84 +if (WIN32) + set(LIBSSH_LINK_LIBRARIES + ${LIBSSH_LINK_LIBRARIES} + iphlpapi + ws2_32 + ) +endif (WIN32) + if (BUILD_STATIC_LIB) set(LIBSSH_STATIC_LIBRARY ssh_static diff --git a/src/connect.c b/src/connect.c index ce4d58df..ca62dcf0 100644 --- a/src/connect.c +++ b/src/connect.c @@ -136,7 +136,7 @@ static int getai(const char *host, int port, struct addrinfo **ai) #endif } - if (ssh_is_ipaddr(host)) { + if (ssh_is_ipaddr(host) == 1) { /* this is an IP address */ SSH_LOG(SSH_LOG_PACKET, "host %s matches an IP address", host); hints.ai_flags |= AI_NUMERICHOST; diff --git a/src/misc.c b/src/misc.c index e4239e81..6f5d2d60 100644 --- a/src/misc.c +++ b/src/misc.c @@ -32,6 +32,7 @@ #include #include #include +#include #endif /* _WIN32 */ @@ -59,6 +60,7 @@ #include #include #include +#include #ifdef HAVE_IO_H #include @@ -216,22 +218,37 @@ int ssh_is_ipaddr_v4(const char *str) { int ssh_is_ipaddr(const char *str) { int rc = SOCKET_ERROR; + char *s = strdup(str); - if (strchr(str, ':')) { + if (s == NULL) { + return -1; + } + if (strchr(s, ':')) { struct sockaddr_storage ss; int sslen = sizeof(ss); + char *network_interface = strchr(s, '%'); - /* TODO link-local (IP:v6:addr%ifname). */ - rc = WSAStringToAddressA((LPSTR) str, + /* link-local (IP:v6:addr%ifname). */ + if (network_interface != NULL) { + rc = if_nametoindex(network_interface + 1); + if (rc == 0) { + free(s); + return 0; + } + *network_interface = '\0'; + } + rc = WSAStringToAddressA((LPSTR) s, AF_INET6, NULL, (struct sockaddr*)&ss, &sslen); if (rc == 0) { + free(s); return 1; } } + free(s); return ssh_is_ipaddr_v4(str); } #else /* _WIN32 */ @@ -335,17 +352,32 @@ int ssh_is_ipaddr_v4(const char *str) { int ssh_is_ipaddr(const char *str) { int rc = -1; + char *s = strdup(str); - if (strchr(str, ':')) { + if (s == NULL) { + return -1; + } + if (strchr(s, ':')) { struct in6_addr dest6; + char *network_interface = strchr(s, '%'); - /* TODO link-local (IP:v6:addr%ifname). */ - rc = inet_pton(AF_INET6, str, &dest6); + /* link-local (IP:v6:addr%ifname). */ + if (network_interface != NULL) { + rc = if_nametoindex(network_interface + 1); + if (rc == 0) { + free(s); + return 0; + } + *network_interface = '\0'; + } + rc = inet_pton(AF_INET6, s, &dest6); if (rc > 0) { + free(s); return 1; } } + free(s); return ssh_is_ipaddr_v4(str); } -- 2.41.0 From 6a8a18c73e73a338283dfbade0a7d83e5cfafe3b Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Tue, 28 Nov 2023 15:27:31 +0100 Subject: [PATCH 10/12] CVE-2023-6004: torture_misc: Add tests for ipv6 link-local Signed-off-by: Norbert Pocs Reviewed-by: Andreas Schneider --- tests/unittests/torture_misc.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/unittests/torture_misc.c b/tests/unittests/torture_misc.c index 073bc54c..f16b766e 100644 --- a/tests/unittests/torture_misc.c +++ b/tests/unittests/torture_misc.c @@ -17,7 +17,14 @@ #include "misc.c" #include "error.c" +#ifdef _WIN32 +#include +#else +#include +#endif + #define TORTURE_TEST_DIR "/usr/local/bin/truc/much/.." +#define TORTURE_IPV6_LOCAL_LINK "fe80::98e1:82ff:fe8d:28b3%%%s" const char template[] = "temp_dir_XXXXXX"; @@ -730,14 +737,27 @@ static void torture_ssh_check_hostname_syntax(void **state) static void torture_ssh_is_ipaddr(void **state) { int rc; + char *interf = malloc(64); + char *test_interf = malloc(128); (void)state; + assert_non_null(interf); + assert_non_null(test_interf); rc = ssh_is_ipaddr("201.255.3.69"); assert_int_equal(rc, 1); rc = ssh_is_ipaddr("::1"); assert_int_equal(rc, 1); rc = ssh_is_ipaddr("2001:0db8:85a3:0000:0000:8a2e:0370:7334"); assert_int_equal(rc, 1); + if_indextoname(1, interf); + assert_non_null(interf); + rc = sprintf(test_interf, TORTURE_IPV6_LOCAL_LINK, interf); + /* the "%%s" is not written */ + assert_int_equal(rc, strlen(interf) + strlen(TORTURE_IPV6_LOCAL_LINK) - 3); + rc = ssh_is_ipaddr(test_interf); + assert_int_equal(rc, 1); + free(interf); + free(test_interf); rc = ssh_is_ipaddr(".."); assert_int_equal(rc, 0); -- 2.41.0 From 72f59157e6ccbd4c0bb806690931413169a0886f Mon Sep 17 00:00:00 2001 From: Jakub Jelen Date: Fri, 22 Dec 2023 10:32:40 +0100 Subject: [PATCH 11/12] Fix regression in IPv6 addresses in hostname parsing Signed-off-by: Jakub Jelen Reviewed-by: Andreas Schneider (cherry picked from commit 4f997aee7c7d7ea346b3e8ba505da0b7601ff318) --- include/libssh/config_parser.h | 11 ++++++++--- src/config.c | 4 ++-- src/config_parser.c | 16 +++++++++++----- src/options.c | 10 ++-------- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/include/libssh/config_parser.h b/include/libssh/config_parser.h index e974917c..ee647bfb 100644 --- a/include/libssh/config_parser.h +++ b/include/libssh/config_parser.h @@ -26,6 +26,8 @@ #ifndef CONFIG_PARSER_H_ #define CONFIG_PARSER_H_ +#include + char *ssh_config_get_cmd(char **str); char *ssh_config_get_token(char **str); @@ -45,13 +47,16 @@ int ssh_config_get_yesno(char **str, int notfound); * be stored or NULL if we do not care about the result. * @param[out] port Pointer to the location, where the new port will * be stored or NULL if we do not care about the result. + * @param[in] ignore_port Set to true if the we should not attempt to parse + * port number. * * @returns SSH_OK if the provided string is in format of SSH URI, * SSH_ERROR on failure */ int ssh_config_parse_uri(const char *tok, - char **username, - char **hostname, - char **port); + char **username, + char **hostname, + char **port, + bool ignore_port); #endif /* LIBSSH_CONFIG_H_ */ diff --git a/src/config.c b/src/config.c index 54ada276..a813568d 100644 --- a/src/config.c +++ b/src/config.c @@ -324,7 +324,7 @@ ssh_config_parse_proxy_jump(ssh_session session, const char *s, bool do_parsing) } if (parse_entry) { /* We actually care only about the first item */ - rv = ssh_config_parse_uri(cp, &username, &hostname, &port); + rv = ssh_config_parse_uri(cp, &username, &hostname, &port, false); /* The rest of the list needs to be passed on */ if (endp != NULL) { next = strdup(endp + 1); @@ -335,7 +335,7 @@ ssh_config_parse_proxy_jump(ssh_session session, const char *s, bool do_parsing) } } else { /* The rest is just sanity-checked to avoid failures later */ - rv = ssh_config_parse_uri(cp, NULL, NULL, NULL); + rv = ssh_config_parse_uri(cp, NULL, NULL, NULL, false); } if (rv != SSH_OK) { goto out; diff --git a/src/config_parser.c b/src/config_parser.c index 87bac5d4..a2da0a62 100644 --- a/src/config_parser.c +++ b/src/config_parser.c @@ -134,9 +134,10 @@ int ssh_config_get_yesno(char **str, int notfound) } int ssh_config_parse_uri(const char *tok, - char **username, - char **hostname, - char **port) + char **username, + char **hostname, + char **port, + bool ignore_port) { char *endp = NULL; long port_n; @@ -182,12 +183,17 @@ int ssh_config_parse_uri(const char *tok, if (endp == NULL) { goto error; } - } else { - /* Hostnames or aliases expand to the last colon or to the end */ + } else if (!ignore_port) { + /* Hostnames or aliases expand to the last colon (if port is requested) + * or to the end */ endp = strrchr(tok, ':'); if (endp == NULL) { endp = strchr(tok, '\0'); } + } else { + /* If no port is requested, expand to the end of line + * (to accommodate the IPv6 addresses) */ + endp = strchr(tok, '\0'); } if (tok == endp) { /* Zero-length hostnames are not valid */ diff --git a/src/options.c b/src/options.c index 7c03e7ab..0890ff2e 100644 --- a/src/options.c +++ b/src/options.c @@ -491,17 +491,11 @@ int ssh_options_set(ssh_session session, enum ssh_options_e type, ssh_set_error_invalid(session); return -1; } else { - char *username = NULL, *hostname = NULL, *port = NULL; - rc = ssh_config_parse_uri(value, &username, &hostname, &port); + char *username = NULL, *hostname = NULL; + rc = ssh_config_parse_uri(value, &username, &hostname, NULL, true); if (rc != SSH_OK) { return -1; } - if (port != NULL) { - SAFE_FREE(username); - SAFE_FREE(hostname); - SAFE_FREE(port); - return -1; - } if (username != NULL) { SAFE_FREE(session->opts.username); session->opts.username = username; -- 2.41.0 From 5dc10ff63ca2e8db91abfdccf1d095f5b4261b8e Mon Sep 17 00:00:00 2001 From: Jakub Jelen Date: Fri, 22 Dec 2023 09:52:18 +0100 Subject: [PATCH 12/12] tests: Increase test coverage for IPv6 address parsing as hostnames This was an issue in cockpit: https://github.com/cockpit-project/cockpit/issues/19772 Signed-off-by: Jakub Jelen Reviewed-by: Andreas Schneider (cherry picked from commit 6f6e453d7b0ad4ee6a6f6a1c96a9a6b27821410d) --- tests/unittests/torture_config.c | 49 +++++++++++++++++++++++++++++++ tests/unittests/torture_options.c | 22 ++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/tests/unittests/torture_config.c b/tests/unittests/torture_config.c index 3a5a74bf..d8097e79 100644 --- a/tests/unittests/torture_config.c +++ b/tests/unittests/torture_config.c @@ -2,6 +2,7 @@ #define LIBSSH_STATIC +#include #include "torture.h" #include "libssh/options.h" #include "libssh/session.h" @@ -997,6 +998,53 @@ static void torture_config_match_pattern(void **state) } +static void torture_config_parse_uri(void **state) +{ + char *username = NULL; + char *hostname = NULL; + char *port = NULL; + int rc; + + (void)state; /* unused */ + + rc = ssh_config_parse_uri("localhost", &username, &hostname, &port, false); + assert_return_code(rc, errno); + assert_null(username); + assert_string_equal(hostname, "localhost"); + SAFE_FREE(hostname); + assert_null(port); + + rc = ssh_config_parse_uri("1.2.3.4", &username, &hostname, &port, false); + assert_return_code(rc, errno); + assert_null(username); + assert_string_equal(hostname, "1.2.3.4"); + SAFE_FREE(hostname); + assert_null(port); + + rc = ssh_config_parse_uri("1.2.3.4:2222", &username, &hostname, &port, false); + assert_return_code(rc, errno); + assert_null(username); + assert_string_equal(hostname, "1.2.3.4"); + SAFE_FREE(hostname); + assert_string_equal(port, "2222"); + SAFE_FREE(port); + + rc = ssh_config_parse_uri("[1:2:3::4]:2222", &username, &hostname, &port, false); + assert_return_code(rc, errno); + assert_null(username); + assert_string_equal(hostname, "1:2:3::4"); + SAFE_FREE(hostname); + assert_string_equal(port, "2222"); + SAFE_FREE(port); + + /* do not want port */ + rc = ssh_config_parse_uri("1:2:3::4", &username, &hostname, NULL, true); + assert_return_code(rc, errno); + assert_null(username); + assert_string_equal(hostname, "1:2:3::4"); + SAFE_FREE(hostname); +} + int torture_run_tests(void) { int rc; @@ -1012,6 +1060,7 @@ int torture_run_tests(void) { cmocka_unit_test(torture_config_rekey), cmocka_unit_test(torture_config_pubkeyacceptedkeytypes), cmocka_unit_test(torture_config_match_pattern), + cmocka_unit_test(torture_config_parse_uri), }; diff --git a/tests/unittests/torture_options.c b/tests/unittests/torture_options.c index d0fdaed1..576ca9cd 100644 --- a/tests/unittests/torture_options.c +++ b/tests/unittests/torture_options.c @@ -59,12 +59,34 @@ static void torture_options_set_host(void **state) { assert_non_null(session->opts.host); assert_string_equal(session->opts.host, "localhost"); + /* IPv4 address */ + rc = ssh_options_set(session, SSH_OPTIONS_HOST, "127.1.1.1"); + assert_true(rc == 0); + assert_non_null(session->opts.host); + assert_string_equal(session->opts.host, "127.1.1.1"); + assert_null(session->opts.username); + + /* IPv6 address */ + rc = ssh_options_set(session, SSH_OPTIONS_HOST, "::1"); + assert_true(rc == 0); + assert_non_null(session->opts.host); + assert_string_equal(session->opts.host, "::1"); + assert_null(session->opts.username); + rc = ssh_options_set(session, SSH_OPTIONS_HOST, "guru@meditation"); assert_true(rc == 0); assert_non_null(session->opts.host); assert_string_equal(session->opts.host, "meditation"); assert_non_null(session->opts.username); assert_string_equal(session->opts.username, "guru"); + + /* more @ in uri is OK -- it should go to the username */ + rc = ssh_options_set(session, SSH_OPTIONS_HOST, "at@login@hostname"); + assert_true(rc == 0); + assert_non_null(session->opts.host); + assert_string_equal(session->opts.host, "hostname"); + assert_non_null(session->opts.username); + assert_string_equal(session->opts.username, "at@login"); } static void torture_options_set_ciphers(void **state) { -- 2.41.0