From a9d1cfa9e233601269f1ca20a952f7b9504d1a5c Mon Sep 17 00:00:00 2001 From: Gauravsingh Sisodia Date: Mon, 19 Feb 2024 12:45:23 +0000 Subject: [PATCH] feat: Handle hostkeys like OpenSSH fix: memory leak fix: add defaults after parsing fix: set defaults in ssh_bind_listen tests: add test for checking default hostkey paths remove: null check for hostkey paths, can't happen since we set defaults now examples: ssh_server remove "no default keys", default hostkeys set in ssh_bind_listen Signed-off-by: Gauravsingh Sisodia Reviewed-by: Sahana Prasad --- examples/ssh_server.c | 55 +-------------------------- src/bind.c | 37 +++++++++--------- tests/unittests/CMakeLists.txt | 11 ++---- tests/unittests/torture_unit_server.c | 39 ++++++++++++++++++- 4 files changed, 62 insertions(+), 80 deletions(-) diff --git a/examples/ssh_server.c b/examples/ssh_server.c index 4b91807e..3e9f344b 100644 --- a/examples/ssh_server.c +++ b/examples/ssh_server.c @@ -45,32 +45,10 @@ The goal is to show the API in action. #define BUF_SIZE 1048576 #endif -#ifndef KEYS_FOLDER -#ifdef _WIN32 -#define KEYS_FOLDER -#else -#define KEYS_FOLDER "/etc/ssh/" -#endif -#endif - #define SESSION_END (SSH_CLOSED | SSH_CLOSED_ERROR) #define SFTP_SERVER_PATH "/usr/lib/sftp-server" #define AUTH_KEYS_MAX_LINE_SIZE 2048 -static void set_default_keys(ssh_bind sshbind, - int rsa_already_set, - int ecdsa_already_set) { - if (!rsa_already_set) { - ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, - KEYS_FOLDER "ssh_host_rsa_key"); - } - if (!ecdsa_already_set) { - ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, - KEYS_FOLDER "ssh_host_ecdsa_key"); - } - ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, - KEYS_FOLDER "ssh_host_ed25519_key"); -} #define DEF_STR_SIZE 1024 char authorizedkeys[DEF_STR_SIZE] = {0}; char username[128] = "myuser"; @@ -145,14 +123,6 @@ static struct argp_option options[] = { .doc = "Set expected password.", .group = 0 }, - { - .name = "no-default-keys", - .key = 'n', - .arg = NULL, - .flags = 0, - .doc = "Do not set default key locations.", - .group = 0 - }, { .name = "verbose", .key = 'v', @@ -169,30 +139,19 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state) { /* Get the input argument from argp_parse, which we * know is a pointer to our arguments structure. */ ssh_bind sshbind = state->input; - static int no_default_keys = 0; - static int rsa_already_set = 0, ecdsa_already_set = 0; switch (key) { - case 'n': - no_default_keys = 1; - break; case 'p': ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_BINDPORT_STR, arg); break; case 'k': ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, arg); - /* We can't track the types of keys being added with this - option, so let's ensure we keep the keys we're adding - by just not setting the default keys */ - no_default_keys = 1; break; case 'r': ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, arg); - rsa_already_set = 1; break; case 'e': ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, arg); - ecdsa_already_set = 1; break; case 'a': strncpy(authorizedkeys, arg, DEF_STR_SIZE-1); @@ -219,13 +178,6 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state) { /* Not enough arguments. */ argp_usage (state); } - - if (!no_default_keys) { - set_default_keys(sshbind, - rsa_already_set, - ecdsa_already_set); - } - break; default: return ARGP_ERR_UNKNOWN; @@ -242,10 +194,8 @@ static int parse_opt(int argc, char **argv, ssh_bind sshbind) { int ecdsa_already_set = 0; int key; - while((key = getopt(argc, argv, "a:e:k:np:P:r:u:v")) != -1) { - if (key == 'n') { - no_default_keys = 1; - } else if (key == 'p') { + while((key = getopt(argc, argv, "a:e:k:p:P:r:u:v")) != -1) { + if (key == 'p') { ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_BINDPORT_STR, optarg); } else if (key == 'k') { ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, optarg); @@ -281,7 +231,6 @@ static int parse_opt(int argc, char **argv, ssh_bind sshbind) { " -e, --ecdsakey=FILE Set the ecdsa key (deprecated alias for 'k').\n" " -k, --hostkey=FILE Set a host key. Can be used multiple times.\n" " Implies no default keys.\n" - " -n, --no-default-keys Do not set default key locations.\n" " -p, --port=PORT Set the port to bind.\n" " -P, --pass=PASSWORD Set expected password.\n" " -r, --rsakey=FILE Set the rsa key (deprecated alias for 'k').\n" diff --git a/src/bind.c b/src/bind.c index 6580b776..c2917865 100644 --- a/src/bind.c +++ b/src/bind.c @@ -149,14 +149,6 @@ ssh_bind ssh_bind_new(void) { static int ssh_bind_import_keys(ssh_bind sshbind) { int rc; - if (sshbind->ecdsakey == NULL && - sshbind->rsakey == NULL && - sshbind->ed25519key == NULL) { - ssh_set_error(sshbind, SSH_FATAL, - "ECDSA, ED25519, or RSA host key file must be set"); - return SSH_ERROR; - } - #ifdef HAVE_ECC if (sshbind->ecdsa == NULL && sshbind->ecdsakey != NULL) { rc = ssh_pki_import_privkey_file(sshbind->ecdsakey, @@ -225,12 +217,28 @@ static int ssh_bind_import_keys(ssh_bind sshbind) { return SSH_OK; } -int ssh_bind_listen(ssh_bind sshbind) -{ - const char *host = NULL; +int ssh_bind_listen(ssh_bind sshbind) { + const char *host; socket_t fd; int rc; + /* Apply global bind configurations, if it hasn't been applied before */ + rc = ssh_bind_options_parse_config(sshbind, NULL); + if (rc != 0) { + ssh_set_error(sshbind, SSH_FATAL,"Could not parse global config"); + return SSH_ERROR; + } + + /* Set default hostkey paths if no hostkey was found before */ + if (sshbind->ecdsakey == NULL && + sshbind->rsakey == NULL && + sshbind->ed25519key == NULL) { + + sshbind->ecdsakey = strdup("/etc/ssh/ssh_host_ecdsa_key"); + sshbind->rsakey = strdup("/etc/ssh/ssh_host_rsa_key"); + sshbind->ed25519key = strdup("/etc/ssh/ssh_host_ed25519_key"); + } + /* Apply global bind configurations, if it hasn't been applied before */ rc = ssh_bind_options_parse_config(sshbind, NULL); if (rc != 0) { @@ -424,13 +432,6 @@ int ssh_bind_accept_fd(ssh_bind sshbind, ssh_session session, socket_t fd) return SSH_ERROR; } - /* Apply global bind configurations, if it hasn't been applied before */ - rc = ssh_bind_options_parse_config(sshbind, NULL); - if (rc != 0) { - ssh_set_error(sshbind, SSH_FATAL,"Could not parse global config"); - return SSH_ERROR; - } - session->server = 1; /* Copy options from bind to session */ diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index e4ee846a..c053e5b8 100644 --- a/tests/unittests/CMakeLists.txt +++ b/tests/unittests/CMakeLists.txt @@ -84,13 +84,10 @@ if (UNIX AND NOT WIN32) torture_threads_pki_rsa ) if (WITH_SERVER) - # Not working correctly - # add_cmocka_test(torture_server_x11 torture_server_x11.c ${TEST_TARGET_LIBRARIES}) - # the signals are not testable under cmocka - # set(LIBSSH_THREAD_UNIT_TESTS - # ${LIBSSH_THREAD_UNIT_TESTS} - # torture_unit_server - # ) + set(LIBSSH_THREAD_UNIT_TESTS + ${LIBSSH_THREAD_UNIT_TESTS} + torture_unit_server + ) endif (WITH_SERVER) endif (UNIX AND NOT WIN32) diff --git a/tests/unittests/torture_unit_server.c b/tests/unittests/torture_unit_server.c index 3e7e69f7..2fd4be72 100644 --- a/tests/unittests/torture_unit_server.c +++ b/tests/unittests/torture_unit_server.c @@ -10,11 +10,13 @@ #include #include +#include #include "torture.h" #include "torture_key.h" #define TEST_SERVER_PORT 2222 +#if 0 struct test_state { const char *hostkey; char *hostkey_path; @@ -107,7 +109,7 @@ static void test_ssh_accept_interrupt(void **state) struct test_state *ts = (struct test_state *)*state; int rc; pthread_t client_pthread, interrupt_pthread; - ssh_bind sshbind; + ssh_bind sshbind = NULL; ssh_session server; /* Create server */ @@ -145,14 +147,47 @@ static void test_ssh_accept_interrupt(void **state) rc = pthread_join(client_pthread, NULL); assert_int_equal(rc, 0); } +#endif + + +static void test_default_hostkey_paths(void **state) +{ + int rc; + ssh_bind sshbind = NULL; + + /* state not used */ + (void)state; + + /* Create server */ + rc = ssh_init(); + assert_int_equal(rc, 0); + + sshbind = ssh_bind_new(); + assert_non_null(sshbind); + + /* This will fail because we don't have permission to import keys unless we run as root + * TODO: Implement some filesystem wrapper, that would allow this check to pass by + * reading the keys from some accessible test location */ + ssh_bind_listen(sshbind); + + assert_string_equal(sshbind->rsakey, "/etc/ssh/ssh_host_rsa_key"); + assert_string_equal(sshbind->ecdsakey, "/etc/ssh/ssh_host_ecdsa_key"); + assert_string_equal(sshbind->ed25519key, "/etc/ssh/ssh_host_ed25519_key"); + + /* Cleanup */ + ssh_bind_free(sshbind); + ssh_finalize(); +} int torture_run_tests(void) { int rc; const struct CMUnitTest tests[] = { + cmocka_unit_test(test_default_hostkey_paths), + /* Not working correctly the signals are not testable under cmocka cmocka_unit_test_setup_teardown(test_ssh_accept_interrupt, setup, - teardown) + teardown) */ }; rc = cmocka_run_group_tests(tests, NULL, NULL);