From b040856ccfde1a5d4c21791f46ca6ee00c21a47b Mon Sep 17 00:00:00 2001 From: Anderson Toshiyuki Sasaki Date: Tue, 2 Jul 2019 10:21:15 +0200 Subject: [PATCH 1/6] knownhosts: Fix possible memory leak The memory allocated for host_port can leak if the global knownhosts file is unaccessible. Found by address sanitizer build in CI. Signed-off-by: Anderson Toshiyuki Sasaki Reviewed-by: Jakub Jelen (cherry picked from commit fe248414fec1e654e4ee1259927d68777dd870ae) --- src/knownhosts.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/knownhosts.c b/src/knownhosts.c index 8a4a8ba7..9383cc97 100644 --- a/src/knownhosts.c +++ b/src/knownhosts.c @@ -706,13 +706,15 @@ enum ssh_known_hosts_e ssh_session_has_known_hosts_entry(ssh_session session) rc = ssh_known_hosts_read_entries(host_port, session->opts.global_knownhosts, &entry_list); - SAFE_FREE(host_port); if (rc != 0) { + SAFE_FREE(host_port); ssh_list_free(entry_list); return SSH_KNOWN_HOSTS_ERROR; } } + SAFE_FREE(host_port); + if (ssh_list_count(entry_list) == 0) { ssh_list_free(entry_list); return SSH_KNOWN_HOSTS_UNKNOWN; -- 2.21.0 From 7ff0af75436ee6e549907dd563e968b92f7f8db2 Mon Sep 17 00:00:00 2001 From: Anderson Toshiyuki Sasaki Date: Fri, 28 Jun 2019 13:19:51 +0200 Subject: [PATCH 2/6] tests: Check if known_hosts works with single unaccessible file Make sure known hosts check works when local known_hosts file is unaccessible, but the host is present in global known_hosts file. Remove double return value check in previous existing test. Signed-off-by: Anderson Toshiyuki Sasaki Reviewed-by: Jakub Jelen (cherry picked from commit ad68de7271e6ccda261df4d9fc827321e7d90fd0) --- tests/unittests/torture_knownhosts_parsing.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/tests/unittests/torture_knownhosts_parsing.c b/tests/unittests/torture_knownhosts_parsing.c index ac8d7f31..a087caef 100644 --- a/tests/unittests/torture_knownhosts_parsing.c +++ b/tests/unittests/torture_knownhosts_parsing.c @@ -384,22 +384,19 @@ static void torture_knownhosts_host_exists(void **state) /* This makes sure the system's known_hosts are not used */ ssh_options_set(session, SSH_OPTIONS_GLOBAL_KNOWNHOSTS, "/dev/null"); - found = ssh_session_has_known_hosts_entry(session); assert_int_equal(found, SSH_KNOWN_HOSTS_OK); - assert_true(found == SSH_KNOWN_HOSTS_OK); /* This makes sure the check will not fail when the system's known_hosts is * not accessible*/ ssh_options_set(session, SSH_OPTIONS_GLOBAL_KNOWNHOSTS, "./unaccessible"); - found = ssh_session_has_known_hosts_entry(session); assert_int_equal(found, SSH_KNOWN_HOSTS_OK); - assert_true(found == SSH_KNOWN_HOSTS_OK); + /* This makes sure the check will fail for an unknown host */ ssh_options_set(session, SSH_OPTIONS_HOST, "wurstbrot"); found = ssh_session_has_known_hosts_entry(session); - assert_true(found == SSH_KNOWN_HOSTS_UNKNOWN); + assert_int_equal(found, SSH_KNOWN_HOSTS_UNKNOWN); ssh_free(session); } @@ -414,17 +411,23 @@ static void torture_knownhosts_host_exists_global(void **state) assert_non_null(session); ssh_options_set(session, SSH_OPTIONS_HOST, "localhost"); + ssh_options_set(session, SSH_OPTIONS_GLOBAL_KNOWNHOSTS, knownhosts_file); + /* This makes sure the user's known_hosts are not used */ ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, "/dev/null"); - ssh_options_set(session, SSH_OPTIONS_GLOBAL_KNOWNHOSTS, knownhosts_file); + found = ssh_session_has_known_hosts_entry(session); + assert_int_equal(found, SSH_KNOWN_HOSTS_OK); + /* This makes sure the check will not fail when the user's known_hosts is + * not accessible*/ + ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, "./unaccessible"); found = ssh_session_has_known_hosts_entry(session); assert_int_equal(found, SSH_KNOWN_HOSTS_OK); - assert_true(found == SSH_KNOWN_HOSTS_OK); + /* This makes sure the check will fail for an unknown host */ ssh_options_set(session, SSH_OPTIONS_HOST, "wurstbrot"); found = ssh_session_has_known_hosts_entry(session); - assert_true(found == SSH_KNOWN_HOSTS_UNKNOWN); + assert_int_equal(found, SSH_KNOWN_HOSTS_UNKNOWN); ssh_free(session); } -- 2.21.0 From b9530cedbeb169762307096dfeb485ab94e09740 Mon Sep 17 00:00:00 2001 From: Anderson Toshiyuki Sasaki Date: Fri, 28 Jun 2019 13:27:34 +0200 Subject: [PATCH 3/6] knownhosts: Read knownhosts file only if found Avoid trying to open the files if they are not accessible. This was already treated as a non-error, but with this we save one function call. Signed-off-by: Anderson Toshiyuki Sasaki Reviewed-by: Jakub Jelen (cherry picked from commit e5a64a3d6b1b601cbaf207468a6658d1a4fa0031) --- src/knownhosts.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/knownhosts.c b/src/knownhosts.c index 9383cc97..8040a0c0 100644 --- a/src/knownhosts.c +++ b/src/knownhosts.c @@ -691,7 +691,7 @@ enum ssh_known_hosts_e ssh_session_has_known_hosts_entry(ssh_session session) return SSH_KNOWN_HOSTS_ERROR; } - if (session->opts.knownhosts != NULL) { + if (known_hosts_found) { rc = ssh_known_hosts_read_entries(host_port, session->opts.knownhosts, &entry_list); @@ -702,7 +702,7 @@ enum ssh_known_hosts_e ssh_session_has_known_hosts_entry(ssh_session session) } } - if (session->opts.global_knownhosts != NULL) { + if (global_known_hosts_found) { rc = ssh_known_hosts_read_entries(host_port, session->opts.global_knownhosts, &entry_list); -- 2.21.0 From aaa978ad06ebdeff88a39b9a894696254263162c Mon Sep 17 00:00:00 2001 From: Anderson Toshiyuki Sasaki Date: Fri, 28 Jun 2019 22:35:38 +0200 Subject: [PATCH 4/6] token: Added function to remove duplicates Added a function to remove duplicates from lists. This function is used in a new provided function to append lists removing duplicates. Signed-off-by: Anderson Toshiyuki Sasaki Reviewed-by: Jakub Jelen (cherry picked from commit 548753b3389518ebce98a7ddbf0640db3ad72de8) --- include/libssh/token.h | 6 +- src/token.c | 152 ++++++++++++++++++++++++++++++- tests/unittests/torture_tokens.c | 122 +++++++++++++++++++++++++ 3 files changed, 278 insertions(+), 2 deletions(-) diff --git a/include/libssh/token.h b/include/libssh/token.h index 7b244189..9896fb06 100644 --- a/include/libssh/token.h +++ b/include/libssh/token.h @@ -38,7 +38,11 @@ void ssh_tokens_free(struct ssh_tokens_st *tokens); char *ssh_find_matching(const char *available_d, const char *preferred_d); - char *ssh_find_all_matching(const char *available_d, const char *preferred_d); + +char *ssh_remove_duplicates(const char *list); + +char *ssh_append_without_duplicates(const char *list, + const char *appended_list); #endif /* TOKEN_H_ */ diff --git a/src/token.c b/src/token.c index aee235ac..0924d3bd 100644 --- a/src/token.c +++ b/src/token.c @@ -26,6 +26,7 @@ #include #include +#include #include "libssh/priv.h" #include "libssh/token.h" @@ -175,7 +176,7 @@ char *ssh_find_matching(const char *available_list, for (i = 0; p_tok->tokens[i]; i++) { for (j = 0; a_tok->tokens[j]; j++) { - if (strcmp(a_tok->tokens[j], p_tok->tokens[i]) == 0){ + if (strcmp(a_tok->tokens[j], p_tok->tokens[i]) == 0) { ret = strdup(a_tok->tokens[j]); goto out; } @@ -260,3 +261,152 @@ out: ssh_tokens_free(p_tok); return ret; } + +/** + * @internal + * + * @brief Given a string containing a list of elements, remove all duplicates + * and return in a newly allocated string. + * + * @param[in] list The list to be freed of duplicates + * + * @return A newly allocated copy of the string free of duplicates; NULL in + * case of error. + */ +char *ssh_remove_duplicates(const char *list) +{ + struct ssh_tokens_st *tok = NULL; + + size_t i, j, num_tokens, max_len; + char *ret = NULL; + bool *should_copy = NULL, need_comma = false; + + if (list == NULL) { + return NULL; + } + + /* The maximum number of tokens is the size of the list */ + max_len = strlen(list); + if (max_len == 0) { + return NULL; + } + + /* Add space for ending '\0' */ + max_len++; + + tok = ssh_tokenize(list, ','); + if ((tok == NULL) || (tok->tokens == NULL) || (tok->tokens[0] == NULL)) { + goto out; + } + + should_copy = calloc(1, max_len); + if (should_copy == NULL) { + goto out; + } + + if (strlen(tok->tokens[0]) > 0) { + should_copy[0] = true; + } + + for (i = 1; tok->tokens[i]; i++) { + for (j = 0; j < i; j++) { + if (strcmp(tok->tokens[i], tok->tokens[j]) == 0) { + /* Found a duplicate; do not copy */ + should_copy[i] = false; + break; + } + } + + /* No matching token before */ + if (j == i) { + /* Only copy if it is not an empty string */ + if (strlen(tok->tokens[i]) > 0) { + should_copy[i] = true; + } else { + should_copy[i] = false; + } + } + } + + num_tokens = i; + + ret = calloc(1, max_len); + if (ret == NULL) { + goto out; + } + + for (i = 0; i < num_tokens; i++) { + if (should_copy[i]) { + if (need_comma) { + strncat(ret, ",", (max_len - strlen(ret) - 1)); + } + strncat(ret, tok->tokens[i], (max_len - strlen(ret) - 1)); + need_comma = true; + } + } + + /* If no comma is needed, nothing was copied */ + if (!need_comma) { + SAFE_FREE(ret); + } + +out: + SAFE_FREE(should_copy); + ssh_tokens_free(tok); + return ret; +} + +/** + * @internal + * + * @brief Given two strings containing lists of tokens, return a newly + * allocated string containing all the elements of the first list appended with + * all the elements of the second list, without duplicates. The order of the + * elements will be preserved. + * + * @param[in] list The first list + * @param[in] appended_list The list to be appended + * + * @return A newly allocated copy list containing all the elements of the + * kept_list appended with the elements of the appended_list without duplicates; + * NULL in case of error. + */ +char *ssh_append_without_duplicates(const char *list, + const char *appended_list) +{ + size_t concat_len = 0; + char *ret = NULL, *concat = NULL; + + if (list != NULL) { + concat_len = strlen(list); + } + + if (appended_list != NULL) { + concat_len += strlen(appended_list); + } + + if (concat_len == 0) { + return NULL; + } + + /* Add room for ending '\0' and for middle ',' */ + concat_len += 2; + concat = calloc(1, concat_len); + if (concat == NULL) { + return NULL; + } + + if (list != NULL) { + strcpy(concat, list); + strncat(concat, ",", concat_len - strlen(concat) - 1); + } + if (appended_list != NULL) { + strncat(concat, appended_list, concat_len - strlen(concat) - 1); + } + + ret = ssh_remove_duplicates(concat); + + SAFE_FREE(concat); + + return ret; +} diff --git a/tests/unittests/torture_tokens.c b/tests/unittests/torture_tokens.c index e192842f..6b52b847 100644 --- a/tests/unittests/torture_tokens.c +++ b/tests/unittests/torture_tokens.c @@ -146,6 +146,126 @@ static void torture_tokens_sanity(UNUSED_PARAM(void **state)) tokenize_compare_expected(",", single_colon, 1); } +static void torture_remove_duplicate(UNUSED_PARAM(void **state)) +{ + + const char *simple[] = {"a,a,b,b,c,c", + "a,b,c,a,b,c", + "a,b,c,c,b,a", + "a,a,,b,b,,c,c", + ",a,a,b,b,c,c", + "a,a,b,b,c,c,"}; + const char *empty[] = {"", + ",,,,,,,,,", + NULL}; + char *ret = NULL; + int i; + + for (i = 0; i < 6; i++) { + ret = ssh_remove_duplicates(simple[i]); + assert_non_null(ret); + assert_string_equal("a,b,c", ret); + printf("simple[%d] resulted in '%s'\n", i, ret); + SAFE_FREE(ret); + } + + for (i = 0; i < 3; i++) { + ret = ssh_remove_duplicates(empty[i]); + if (ret != NULL) { + printf("empty[%d] resulted in '%s'\n", i, ret); + } + assert_null(ret); + } + + ret = ssh_remove_duplicates("a"); + assert_non_null(ret); + assert_string_equal("a", ret); + SAFE_FREE(ret); +} + +static void torture_append_without_duplicate(UNUSED_PARAM(void **state)) +{ + const char *s1[] = {"a,a,b,b,c,c", + "a,b,c,a,b,c", + "a,b,c,c,b,a", + "a,a,,b,b,,c,c", + ",a,a,b,b,c,c", + "a,a,b,b,c,c,"}; + const char *s2[] = {"a,a,b,b,c,c,d,d", + "a,b,c,d,a,b,c,d", + "a,b,c,d,d,c,b,a", + "a,a,,b,b,,c,c,,d,d", + ",a,a,b,b,c,c,d,d", + "a,a,b,b,c,c,d,d,", + "d"}; + const char *empty[] = {"", + ",,,,,,,,,", + NULL, + NULL}; + char *ret = NULL; + int i, j; + + ret = ssh_append_without_duplicates("a", "a"); + assert_non_null(ret); + assert_string_equal("a", ret); + SAFE_FREE(ret); + + ret = ssh_append_without_duplicates("a", "b"); + assert_non_null(ret); + assert_string_equal("a,b", ret); + SAFE_FREE(ret); + + ret = ssh_append_without_duplicates("a", NULL); + assert_non_null(ret); + assert_string_equal("a", ret); + SAFE_FREE(ret); + + ret = ssh_append_without_duplicates(NULL, "b"); + assert_non_null(ret); + assert_string_equal("b", ret); + SAFE_FREE(ret); + + for (i = 0; i < 6; i++) { + for (j = 0; j < 7; j++) { + ret = ssh_append_without_duplicates(s1[i], s2[j]); + assert_non_null(ret); + printf("s1[%d] + s2[%d] resulted in '%s'\n", i, j, ret); + assert_string_equal("a,b,c,d", ret); + SAFE_FREE(ret); + } + } + + for (i = 0; i < 6; i++) { + for (j = 0; j < 3; j++) { + ret = ssh_append_without_duplicates(s1[i], empty[j]); + assert_non_null(ret); + printf("s1[%d] + empty[%d] resulted in '%s'\n", i, j, ret); + assert_string_equal("a,b,c", ret); + SAFE_FREE(ret); + } + } + + for (i = 0; i < 3; i++) { + for (j = 0; j < 6; j++) { + ret = ssh_append_without_duplicates(empty[i], s1[j]); + assert_non_null(ret); + printf("empty[%d] + s1[%d] resulted in '%s'\n", i, j, ret); + assert_string_equal("a,b,c", ret); + SAFE_FREE(ret); + } + } + for (i = 0; i < 4; i++) { + for (j = 0; j < 4; j++) { + ret = ssh_append_without_duplicates(empty[i], empty[j]); + if (ret != NULL) { + printf("empty[%d] + empty[%d] resulted in '%s'\n", i, j, ret); + } + assert_null(ret); + } + } +} + + int torture_run_tests(void) { int rc; @@ -153,6 +273,8 @@ int torture_run_tests(void) cmocka_unit_test(torture_tokens_sanity), cmocka_unit_test(torture_find_matching), cmocka_unit_test(torture_find_all_matching), + cmocka_unit_test(torture_remove_duplicate), + cmocka_unit_test(torture_append_without_duplicate), }; ssh_init(); -- 2.21.0 From fa3caa61fdb39269cd78c4919df515b71991d231 Mon Sep 17 00:00:00 2001 From: Anderson Toshiyuki Sasaki Date: Tue, 2 Jul 2019 13:48:17 +0200 Subject: [PATCH 5/6] knownhosts: Introduced ssh_known_hosts_get_algorithms_names() The added internal function obtain a newly allocated string containing a list of the signature types that can be generated by the keys present in the known_hosts files, separated by commas. Signed-off-by: Anderson Toshiyuki Sasaki Reviewed-by: Jakub Jelen (cherry picked from commit 65a38759ca872e8bec0158ab3676e74b6afd336f) --- include/libssh/knownhosts.h | 1 + src/knownhosts.c | 141 +++++++++++++++++++ tests/unittests/torture_knownhosts_parsing.c | 28 ++++ 3 files changed, 170 insertions(+) diff --git a/include/libssh/knownhosts.h b/include/libssh/knownhosts.h index dcaa6c24..44e434c0 100644 --- a/include/libssh/knownhosts.h +++ b/include/libssh/knownhosts.h @@ -23,6 +23,7 @@ #define SSH_KNOWNHOSTS_H_ struct ssh_list *ssh_known_hosts_get_algorithms(ssh_session session); +char *ssh_known_hosts_get_algorithms_names(ssh_session session); enum ssh_known_hosts_e ssh_session_get_known_hosts_entry_file(ssh_session session, const char *filename, diff --git a/src/knownhosts.c b/src/knownhosts.c index 8040a0c0..cf9d8a6b 100644 --- a/src/knownhosts.c +++ b/src/knownhosts.c @@ -42,6 +42,7 @@ #include "libssh/pki.h" #include "libssh/dh.h" #include "libssh/knownhosts.h" +#include "libssh/token.h" /** * @addtogroup libssh_session @@ -451,6 +452,146 @@ error: return NULL; } +/** + * @internal + * + * @brief Returns a static string containing a list of the signature types the + * given key type can generate. + * + * @returns A static cstring containing the signature types the key is able to + * generate separated by commas; NULL in case of error + */ +static const char *ssh_known_host_sigs_from_hostkey_type(enum ssh_keytypes_e type) +{ + switch (type) { + case SSH_KEYTYPE_RSA: + return "rsa-sha2-512,rsa-sha2-256,ssh-rsa"; + case SSH_KEYTYPE_ED25519: + return "ssh-ed25519"; +#ifdef HAVE_DSA + case SSH_KEYTYPE_DSS: + return "ssh-dss"; +#endif +#ifdef HAVE_ECDH + case SSH_KEYTYPE_ECDSA_P256: + return "ecdsa-sha2-nistp256"; + case SSH_KEYTYPE_ECDSA_P384: + return "ecdsa-sha2-nistp384"; + case SSH_KEYTYPE_ECDSA_P521: + return "ecdsa-sha2-nistp521"; +#endif + case SSH_KEYTYPE_UNKNOWN: + default: + SSH_LOG(SSH_LOG_WARN, "The given type %d is not a base private key type " + "or is unsupported", type); + return NULL; + } +} + +/** + * @internal + * @brief Get the host keys algorithms identifiers from the known_hosts files + * + * This expands the signatures types that can be generated from the keys types + * present in the known_hosts files + * + * @param[in] session The ssh session to use. + * + * @return A newly allocated cstring containing a list of signature algorithms + * that can be generated by the host using the keys listed in the known_hosts + * files, NULL on error. + */ +char *ssh_known_hosts_get_algorithms_names(ssh_session session) +{ + char methods_buffer[256 + 1] = {0}; + struct ssh_list *entry_list = NULL; + struct ssh_iterator *it = NULL; + char *host_port = NULL; + size_t count; + bool needcomma = false; + char *names; + + int rc; + + if (session->opts.knownhosts == NULL || + session->opts.global_knownhosts == NULL) { + if (ssh_options_apply(session) < 0) { + ssh_set_error(session, + SSH_REQUEST_DENIED, + "Can't find a known_hosts file"); + + return NULL; + } + } + + host_port = ssh_session_get_host_port(session); + if (host_port == NULL) { + return NULL; + } + + rc = ssh_known_hosts_read_entries(host_port, + session->opts.knownhosts, + &entry_list); + if (rc != 0) { + SAFE_FREE(host_port); + ssh_list_free(entry_list); + return NULL; + } + + rc = ssh_known_hosts_read_entries(host_port, + session->opts.global_knownhosts, + &entry_list); + SAFE_FREE(host_port); + if (rc != 0) { + ssh_list_free(entry_list); + return NULL; + } + + if (entry_list == NULL) { + return NULL; + } + + count = ssh_list_count(entry_list); + if (count == 0) { + ssh_list_free(entry_list); + return NULL; + } + + for (it = ssh_list_get_iterator(entry_list); + it != NULL; + it = ssh_list_get_iterator(entry_list)) + { + struct ssh_knownhosts_entry *entry = NULL; + const char *algo = NULL; + + entry = ssh_iterator_value(struct ssh_knownhosts_entry *, it); + algo = ssh_known_host_sigs_from_hostkey_type(entry->publickey->type); + if (algo == NULL) { + continue; + } + + if (needcomma) { + strncat(methods_buffer, + ",", + sizeof(methods_buffer) - strlen(methods_buffer) - 1); + } + + strncat(methods_buffer, + algo, + sizeof(methods_buffer) - strlen(methods_buffer) - 1); + needcomma = true; + + ssh_knownhosts_entry_free(entry); + ssh_list_remove(entry_list, it); + } + + ssh_list_free(entry_list); + + names = ssh_remove_duplicates(methods_buffer); + + return names; +} + /** * @brief Parse a line from a known_hosts entry into a structure * diff --git a/tests/unittests/torture_knownhosts_parsing.c b/tests/unittests/torture_knownhosts_parsing.c index a087caef..6952e858 100644 --- a/tests/unittests/torture_knownhosts_parsing.c +++ b/tests/unittests/torture_knownhosts_parsing.c @@ -369,6 +369,31 @@ static void torture_knownhosts_read_file(void **state) ssh_list_free(entry_list); } +static void torture_knownhosts_get_algorithms_names(void **state) +{ + const char *knownhosts_file = *state; + ssh_session session; + const char *expect = "ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa"; + char *names = NULL; + bool process_config = false; + + session = ssh_new(); + assert_non_null(session); + + /* This makes sure the global configuration file is not processed */ + ssh_options_set(session, SSH_OPTIONS_PROCESS_CONFIG, &process_config); + + ssh_options_set(session, SSH_OPTIONS_HOST, "localhost"); + ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, knownhosts_file); + + names = ssh_known_hosts_get_algorithms_names(session); + assert_non_null(names); + assert_string_equal(names, expect); + + SAFE_FREE(names); + ssh_free(session); +} + #ifndef _WIN32 /* There is no /dev/null on Windows */ static void torture_knownhosts_host_exists(void **state) { @@ -510,6 +535,9 @@ int torture_run_tests(void) { cmocka_unit_test_setup_teardown(torture_knownhosts_read_file, setup_knownhosts_file_duplicate, teardown_knownhosts_file), + cmocka_unit_test_setup_teardown(torture_knownhosts_get_algorithms_names, + setup_knownhosts_file, + teardown_knownhosts_file), #ifndef _WIN32 cmocka_unit_test_setup_teardown(torture_knownhosts_host_exists, setup_knownhosts_file, -- 2.21.0 From 1fd68ec732214a12ba3f59ca23f80463411e22dd Mon Sep 17 00:00:00 2001 From: Anderson Toshiyuki Sasaki Date: Mon, 1 Jul 2019 19:39:07 +0200 Subject: [PATCH 6/6] kex: Do not ignore keys in known_hosts files Previously, if the SSH_OPTIONS_HOSTKEYS option was set by any mean, including the client configuration file, the keys in known_hosts files wouldn't be considered before advertising the list of wanted host keys. This could result in the client requesting the server to provide a signature using a key not present in the known_hosts files (e.g. when the first wanted algorithm in SSH_OPTIONS_HOSTKEYS is not present in the known_hosts files), causing a host key mismatch and possible key rejection. Now, the keys present in the known_hosts files are prioritized over the other wanted keys. This do not change the fact that only keys of types present in the list set in SSH_OPTIONS_HOSTKEYS will be accepted and prioritized following the order defined by such list. The new wanted list of hostkeys is given by: - The keys present in known_hosts files, ordered by preference defined in SSH_OPTIONS_HOSTKEYS. If the option is not set, a default order of preference is used. - The other keys present in the same option are appended without adding duplicates. If the option is not set, the default list of keys is used. Fixes: T156 Signed-off-by: Anderson Toshiyuki Sasaki Reviewed-by: Jakub Jelen (cherry picked from commit f18a7cc17e399ae7bc92f707da3a676c52fd948e) --- src/kex.c | 165 +++++++++---------- tests/unittests/torture_knownhosts_parsing.c | 148 ++++++++++++++++- 2 files changed, 223 insertions(+), 90 deletions(-) diff --git a/src/kex.c b/src/kex.c index 6ea5e8ba..0d4cad6d 100644 --- a/src/kex.c +++ b/src/kex.c @@ -561,103 +561,94 @@ void ssh_list_kex(struct ssh_kex_struct *kex) { /** * @internal + * * @brief selects the hostkey mechanisms to be chosen for the key exchange, - * as some hostkey mechanisms may be present in known_hosts file and preferred + * as some hostkey mechanisms may be present in known_hosts files. + * * @returns a cstring containing a comma-separated list of hostkey methods. * NULL if no method matches */ char *ssh_client_select_hostkeys(ssh_session session) { - char methods_buffer[128]={0}; - char tail_buffer[128]={0}; + const char *wanted = NULL; + char *wanted_without_certs = NULL; + char *known_hosts_algorithms = NULL; + char *known_hosts_ordered = NULL; char *new_hostkeys = NULL; - static const char *preferred_hostkeys[] = { - "ssh-ed25519", - "ecdsa-sha2-nistp521", - "ecdsa-sha2-nistp384", - "ecdsa-sha2-nistp256", - "rsa-sha2-512", - "rsa-sha2-256", - "ssh-rsa", -#ifdef HAVE_DSA - "ssh-dss", -#endif - NULL - }; - struct ssh_list *algo_list = NULL; - struct ssh_iterator *it = NULL; - size_t algo_count; - int needcomma = 0; - size_t i, len; - - algo_list = ssh_known_hosts_get_algorithms(session); - if (algo_list == NULL) { - return NULL; + char *fips_hostkeys = NULL; + + wanted = session->opts.wanted_methods[SSH_HOSTKEYS]; + if (wanted == NULL) { + if (ssh_fips_mode()) { + wanted = ssh_kex_get_fips_methods(SSH_HOSTKEYS); + } else { + wanted = ssh_kex_get_default_methods(SSH_HOSTKEYS); + } } - algo_count = ssh_list_count(algo_list); - if (algo_count == 0) { - ssh_list_free(algo_list); + /* This removes the certificate types, unsupported for now */ + wanted_without_certs = ssh_find_all_matching(HOSTKEYS, wanted); + if (wanted_without_certs == NULL) { + SSH_LOG(SSH_LOG_WARNING, + "List of allowed host key algorithms is empty or contains only " + "unsupported algorithms"); return NULL; } - for (i = 0; preferred_hostkeys[i] != NULL; ++i) { - bool found = false; - /* This is a signature type: We list also the SHA2 extensions */ - enum ssh_keytypes_e base_preferred = - ssh_key_type_from_signature_name(preferred_hostkeys[i]); - - for (it = ssh_list_get_iterator(algo_list); - it != NULL; - it = it->next) { - const char *algo = ssh_iterator_value(const char *, it); - /* This is always key type so we do not have to care for the - * SHA2 extension */ - enum ssh_keytypes_e base_algo = ssh_key_type_from_name(algo); - - if (base_preferred == base_algo) { - /* Matching the keys already verified it is a known type */ - if (needcomma) { - strncat(methods_buffer, - ",", - sizeof(methods_buffer) - strlen(methods_buffer) - 1); - } - strncat(methods_buffer, - preferred_hostkeys[i], - sizeof(methods_buffer) - strlen(methods_buffer) - 1); - needcomma = 1; - found = true; - } - } - /* Collect the rest of the algorithms in other buffer, that will - * follow the preferred buffer. This will signalize all the algorithms - * we are willing to accept. - */ - if (!found) { - snprintf(tail_buffer + strlen(tail_buffer), - sizeof(tail_buffer) - strlen(tail_buffer), - ",%s", preferred_hostkeys[i]); - } + SSH_LOG(SSH_LOG_DEBUG, + "Order of wanted host keys: \"%s\"", + wanted_without_certs); + + known_hosts_algorithms = ssh_known_hosts_get_algorithms_names(session); + if (known_hosts_algorithms == NULL) { + SSH_LOG(SSH_LOG_DEBUG, + "No key found in known_hosts; " + "changing host key method to \"%s\"", + wanted_without_certs); + + return wanted_without_certs; } - ssh_list_free(algo_list); - if (strlen(methods_buffer) == 0) { + SSH_LOG(SSH_LOG_DEBUG, + "Algorithms found in known_hosts files: \"%s\"", + known_hosts_algorithms); + + /* Filter and order the keys from known_hosts according to wanted list */ + known_hosts_ordered = ssh_find_all_matching(known_hosts_algorithms, + wanted_without_certs); + SAFE_FREE(known_hosts_algorithms); + if (known_hosts_ordered == NULL) { SSH_LOG(SSH_LOG_DEBUG, - "No supported kex method for existing key in known_hosts file"); - return NULL; + "No key found in known_hosts is allowed; " + "changing host key method to \"%s\"", + wanted_without_certs); + + return wanted_without_certs; } - /* Append the supported list to the preferred. - * The length is maximum 128 + 128 + 1, which will not overflow - */ - len = strlen(methods_buffer) + strlen(tail_buffer) + 1; - new_hostkeys = malloc(len); + /* Append the other supported keys after the preferred ones + * This function tolerates NULL pointers in parameters */ + new_hostkeys = ssh_append_without_duplicates(known_hosts_ordered, + wanted_without_certs); + SAFE_FREE(known_hosts_ordered); + SAFE_FREE(wanted_without_certs); if (new_hostkeys == NULL) { ssh_set_error_oom(session); return NULL; } - snprintf(new_hostkeys, len, - "%s%s", methods_buffer, tail_buffer); + + if (ssh_fips_mode()) { + /* Filter out algorithms not allowed in FIPS mode */ + fips_hostkeys = ssh_keep_fips_algos(SSH_HOSTKEYS, new_hostkeys); + SAFE_FREE(new_hostkeys); + if (fips_hostkeys == NULL) { + SSH_LOG(SSH_LOG_WARNING, + "None of the wanted host keys or keys in known_hosts files " + "is allowed in FIPS mode."); + return NULL; + } + new_hostkeys = fips_hostkeys; + } SSH_LOG(SSH_LOG_DEBUG, "Changing host key method to \"%s\"", @@ -672,7 +663,7 @@ char *ssh_client_select_hostkeys(ssh_session session) */ int ssh_set_client_kex(ssh_session session) { - struct ssh_kex_struct *client= &session->next_crypto->client_kex; + struct ssh_kex_struct *client = &session->next_crypto->client_kex; const char *wanted; char *kex = NULL; char *kex_tmp = NULL; @@ -687,14 +678,22 @@ int ssh_set_client_kex(ssh_session session) } memset(client->methods, 0, KEX_METHODS_SIZE * sizeof(char **)); - /* first check if we have specific host key methods */ - if (session->opts.wanted_methods[SSH_HOSTKEYS] == NULL) { - /* Only if no override */ - session->opts.wanted_methods[SSH_HOSTKEYS] = - ssh_client_select_hostkeys(session); - } + /* Set the list of allowed algorithms in order of preference, if it hadn't + * been set yet. */ for (i = 0; i < KEX_METHODS_SIZE; i++) { + if (i == SSH_HOSTKEYS) { + /* Set the hostkeys in the following order: + * - First: keys present in known_hosts files ordered by preference + * - Next: other wanted algorithms ordered by preference */ + client->methods[i] = ssh_client_select_hostkeys(session); + if (client->methods[i] == NULL) { + ssh_set_error_oom(session); + return SSH_ERROR; + } + continue; + } + wanted = session->opts.wanted_methods[i]; if (wanted == NULL) { if (ssh_fips_mode()) { diff --git a/tests/unittests/torture_knownhosts_parsing.c b/tests/unittests/torture_knownhosts_parsing.c index 6952e858..1c2ccc10 100644 --- a/tests/unittests/torture_knownhosts_parsing.c +++ b/tests/unittests/torture_knownhosts_parsing.c @@ -28,6 +28,8 @@ #define TMP_FILE_NAME "/tmp/known_hosts_XXXXXX" +const char template[] = "temp_dir_XXXXXX"; + static int setup_knownhosts_file(void **state) { char *tmp_file = NULL; @@ -394,6 +396,115 @@ static void torture_knownhosts_get_algorithms_names(void **state) ssh_free(session); } +static void torture_knownhosts_algorithms_wanted(void **state) +{ + const char *knownhosts_file = *state; + char *algo_list = NULL; + ssh_session session; + bool process_config = false; + const char *wanted = "ecdsa-sha2-nistp384,ecdsa-sha2-nistp256," + "rsa-sha2-256,ecdsa-sha2-nistp521"; + const char *expect = "rsa-sha2-256,ecdsa-sha2-nistp384," + "ecdsa-sha2-nistp256,ecdsa-sha2-nistp521"; + int verbose = 4; + + session = ssh_new(); + assert_non_null(session); + + ssh_options_set(session, SSH_OPTIONS_LOG_VERBOSITY, &verbose); + + /* This makes sure the global configuration file is not processed */ + ssh_options_set(session, SSH_OPTIONS_PROCESS_CONFIG, &process_config); + + /* Set the wanted list of hostkeys, ordered by preference */ + ssh_options_set(session, SSH_OPTIONS_HOSTKEYS, wanted); + + ssh_options_set(session, SSH_OPTIONS_HOST, "localhost"); + ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, knownhosts_file); + + algo_list = ssh_client_select_hostkeys(session); + assert_non_null(algo_list); + assert_string_equal(algo_list, expect); + free(algo_list); + + ssh_free(session); +} + +static void torture_knownhosts_algorithms_negative(UNUSED_PARAM(void **state)) +{ + const char *wanted = NULL; + const char *expect = NULL; + + char *algo_list = NULL; + + char *cwd = NULL; + char *tmp_dir = NULL; + + bool process_config = false; + int verbose = 4; + int rc = 0; + + ssh_session session; + /* Create temporary directory */ + cwd = torture_get_current_working_dir(); + assert_non_null(cwd); + + tmp_dir = torture_make_temp_dir(template); + assert_non_null(tmp_dir); + + rc = torture_change_dir(tmp_dir); + assert_int_equal(rc, 0); + + session = ssh_new(); + assert_non_null(session); + + ssh_options_set(session, SSH_OPTIONS_LOG_VERBOSITY, &verbose); + ssh_options_set(session, SSH_OPTIONS_PROCESS_CONFIG, &process_config); + ssh_options_set(session, SSH_OPTIONS_HOST, "localhost"); + + /* Test with unknown key type in known_hosts */ + wanted = "rsa-sha2-256"; + ssh_options_set(session, SSH_OPTIONS_HOSTKEYS, wanted); + torture_write_file("unknown_key_type", "localhost unknown AAAABBBBCCCC"); + ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, "unknown_key_type"); + algo_list = ssh_client_select_hostkeys(session); + assert_non_null(algo_list); + assert_string_equal(algo_list, wanted); + SAFE_FREE(algo_list); + + /* Test with unsupported, but existing types */ + wanted = "rsa-sha2-256-cert-v01@openssh.com," + "rsa-sha2-512-cert-v01@openssh.com"; + ssh_options_set(session, SSH_OPTIONS_HOSTKEYS, wanted); + algo_list = ssh_client_select_hostkeys(session); + assert_null(algo_list); + + /* In FIPS mode, test filtering keys not allowed */ + if (ssh_fips_mode()) { + wanted = "ssh-ed25519,rsa-sha2-256,ssh-rsa"; + expect = "rsa-sha2-256"; + ssh_options_set(session, SSH_OPTIONS_HOSTKEYS, wanted); + torture_write_file("no_fips", LOCALHOST_DEFAULT_ED25519); + ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, "no_fips"); + algo_list = ssh_client_select_hostkeys(session); + assert_non_null(algo_list); + assert_string_equal(algo_list, expect); + SAFE_FREE(algo_list); + } + + ssh_free(session); + + /* Teardown */ + rc = torture_change_dir(cwd); + assert_int_equal(rc, 0); + + rc = torture_rmdirs(tmp_dir); + assert_int_equal(rc, 0); + + SAFE_FREE(tmp_dir); + SAFE_FREE(cwd); +} + #ifndef _WIN32 /* There is no /dev/null on Windows */ static void torture_knownhosts_host_exists(void **state) { @@ -457,12 +568,12 @@ static void torture_knownhosts_host_exists_global(void **state) ssh_free(session); } -static void -torture_knownhosts_algorithms(void **state) +static void torture_knownhosts_algorithms(void **state) { const char *knownhosts_file = *state; char *algo_list = NULL; ssh_session session; + bool process_config = false; const char *expect = "ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa," "ecdsa-sha2-nistp521,ecdsa-sha2-nistp384," "ecdsa-sha2-nistp256" @@ -470,10 +581,15 @@ torture_knownhosts_algorithms(void **state) ",ssh-dss" #endif ; + const char *expect_fips = "rsa-sha2-512,rsa-sha2-256,ecdsa-sha2-nistp521," + "ecdsa-sha2-nistp384,ecdsa-sha2-nistp256"; session = ssh_new(); assert_non_null(session); + /* This makes sure the global configuration file is not processed */ + ssh_options_set(session, SSH_OPTIONS_PROCESS_CONFIG, &process_config); + ssh_options_set(session, SSH_OPTIONS_HOST, "localhost"); ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, knownhosts_file); /* This makes sure the system's known_hosts are not used */ @@ -481,18 +597,22 @@ torture_knownhosts_algorithms(void **state) algo_list = ssh_client_select_hostkeys(session); assert_non_null(algo_list); - assert_string_equal(algo_list, expect); + if (ssh_fips_mode()) { + assert_string_equal(algo_list, expect_fips); + } else { + assert_string_equal(algo_list, expect); + } free(algo_list); ssh_free(session); } -static void -torture_knownhosts_algorithms_global(void **state) +static void torture_knownhosts_algorithms_global(void **state) { const char *knownhosts_file = *state; char *algo_list = NULL; ssh_session session; + bool process_config = false; const char *expect = "ssh-ed25519,rsa-sha2-512,rsa-sha2-256,ssh-rsa," "ecdsa-sha2-nistp521,ecdsa-sha2-nistp384," "ecdsa-sha2-nistp256" @@ -500,10 +620,15 @@ torture_knownhosts_algorithms_global(void **state) ",ssh-dss" #endif ; + const char *expect_fips = "rsa-sha2-512,rsa-sha2-256,ecdsa-sha2-nistp521," + "ecdsa-sha2-nistp384,ecdsa-sha2-nistp256"; session = ssh_new(); assert_non_null(session); + /* This makes sure the global configuration file is not processed */ + ssh_options_set(session, SSH_OPTIONS_PROCESS_CONFIG, &process_config); + ssh_options_set(session, SSH_OPTIONS_HOST, "localhost"); /* This makes sure the current-user's known hosts are not used */ ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, "/dev/null"); @@ -511,12 +636,17 @@ torture_knownhosts_algorithms_global(void **state) algo_list = ssh_client_select_hostkeys(session); assert_non_null(algo_list); - assert_string_equal(algo_list, expect); + if (ssh_fips_mode()) { + assert_string_equal(algo_list, expect_fips); + } else { + assert_string_equal(algo_list, expect); + } free(algo_list); ssh_free(session); } -#endif + +#endif /* _WIN32 There is no /dev/null on Windows */ int torture_run_tests(void) { int rc; @@ -538,6 +668,10 @@ int torture_run_tests(void) { cmocka_unit_test_setup_teardown(torture_knownhosts_get_algorithms_names, setup_knownhosts_file, teardown_knownhosts_file), + cmocka_unit_test_setup_teardown(torture_knownhosts_algorithms_wanted, + setup_knownhosts_file, + teardown_knownhosts_file), + cmocka_unit_test(torture_knownhosts_algorithms_negative), #ifndef _WIN32 cmocka_unit_test_setup_teardown(torture_knownhosts_host_exists, setup_knownhosts_file, -- 2.21.0