aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAnderson Toshiyuki Sasaki <ansasaki@redhat.com>2019-07-31 15:48:48 +0200
committerJakub Jelen <jjelen@redhat.com>2019-08-08 09:45:08 +0200
commit9e8e5f5cb22a76a66315451414623528f40e2b7c (patch)
tree13339eda1b2f2a39f42debf418a2a1368ad26425
parent80c1dbdb618882c40eb0fb13987705d1d1e6fde3 (diff)
downloadlibssh-9e8e5f5cb22a76a66315451414623528f40e2b7c.tar.gz
libssh-9e8e5f5cb22a76a66315451414623528f40e2b7c.tar.xz
libssh-9e8e5f5cb22a76a66315451414623528f40e2b7c.zip
knownhosts: Use ssh_mkdirs() instead of ssh_mkdir()
Previously, if the path to known_hosts file set through SSH_OPTIONS_KNOWNHOSTS included missing directories, ssh_session_update_known_hosts() would fail. The added test case checks that this is not the case anymore. The logic of checking if the directory is accessible before creating it was replaced by creating the directory if opening the file failed. This is to minimize the risk of TOCTOU race conditions. Fixes: T166 Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com> Reviewed-by: Andreas Schneider <asn@cryptomilk.org> (cherry picked from commit 5b18bcb0ac39c3c366dd769e893af381ddb5deb2)
-rw-r--r--src/known_hosts.c51
-rw-r--r--src/knownhosts.c59
-rw-r--r--tests/client/torture_knownhosts_verify.c42
3 files changed, 107 insertions, 45 deletions
diff --git a/src/known_hosts.c b/src/known_hosts.c
index 80520907..9a09d1c4 100644
--- a/src/known_hosts.c
+++ b/src/known_hosts.c
@@ -496,6 +496,7 @@ int ssh_write_knownhost(ssh_session session) {
FILE *file;
char *buffer;
char *dir;
+ int rc;
if (session->opts.knownhosts == NULL) {
if (ssh_options_apply(session) < 0) {
@@ -504,30 +505,42 @@ int ssh_write_knownhost(ssh_session session) {
}
}
- /* Check if directory exists and create it if not */
- dir = ssh_dirname(session->opts.knownhosts);
- if (dir == NULL) {
- ssh_set_error(session, SSH_FATAL, "%s", strerror(errno));
- return SSH_ERROR;
- }
+ errno = 0;
+ file = fopen(session->opts.knownhosts, "a");
+ if (file == NULL) {
+ if (errno == ENOENT) {
+ dir = ssh_dirname(session->opts.knownhosts);
+ if (dir == NULL) {
+ ssh_set_error(session, SSH_FATAL, "%s", strerror(errno));
+ return SSH_ERROR;
+ }
- if (!ssh_file_readaccess_ok(dir)) {
- if (ssh_mkdir(dir, 0700) < 0) {
- ssh_set_error(session, SSH_FATAL,
- "Cannot create %s directory.", dir);
+ rc = ssh_mkdirs(dir, 0700);
+ if (rc < 0) {
+ ssh_set_error(session, SSH_FATAL,
+ "Cannot create %s directory: %s",
+ dir, strerror(errno));
+ SAFE_FREE(dir);
+ return SSH_ERROR;
+ }
SAFE_FREE(dir);
+
+ errno = 0;
+ file = fopen(session->opts.knownhosts, "a");
+ if (file == NULL) {
+ ssh_set_error(session, SSH_FATAL,
+ "Couldn't open known_hosts file %s"
+ " for appending: %s",
+ session->opts.knownhosts, strerror(errno));
+ return SSH_ERROR;
+ }
+ } else {
+ ssh_set_error(session, SSH_FATAL,
+ "Couldn't open known_hosts file %s for appending: %s",
+ session->opts.knownhosts, strerror(errno));
return SSH_ERROR;
}
}
- SAFE_FREE(dir);
-
- file = fopen(session->opts.knownhosts, "a");
- if (file == NULL) {
- ssh_set_error(session, SSH_FATAL,
- "Couldn't open known_hosts file %s for appending: %s",
- session->opts.knownhosts, strerror(errno));
- return SSH_ERROR;
- }
buffer = ssh_dump_knownhost(session);
if (buffer == NULL) {
diff --git a/src/knownhosts.c b/src/knownhosts.c
index cf9d8a6b..b3587e6c 100644
--- a/src/knownhosts.c
+++ b/src/knownhosts.c
@@ -979,34 +979,41 @@ int ssh_session_update_known_hosts(ssh_session session)
}
}
- /* Check if directory exists and create it if not */
- dir = ssh_dirname(session->opts.knownhosts);
- if (dir == NULL) {
- ssh_set_error(session, SSH_FATAL, "%s", strerror(errno));
- return SSH_ERROR;
- }
-
- rc = ssh_file_readaccess_ok(dir);
- if (rc == 0) {
- rc = ssh_mkdir(dir, 0700);
- } else {
- rc = 0;
- }
-
- if (rc != 0) {
- ssh_set_error(session, SSH_FATAL,
- "Cannot create %s directory.", dir);
- SAFE_FREE(dir);
- return SSH_ERROR;
- }
- SAFE_FREE(dir);
-
+ errno = 0;
fp = fopen(session->opts.knownhosts, "a");
if (fp == NULL) {
- ssh_set_error(session, SSH_FATAL,
- "Couldn't open known_hosts file %s for appending: %s",
- session->opts.knownhosts, strerror(errno));
- return SSH_ERROR;
+ if (errno == ENOENT) {
+ dir = ssh_dirname(session->opts.knownhosts);
+ if (dir == NULL) {
+ ssh_set_error(session, SSH_FATAL, "%s", strerror(errno));
+ return SSH_ERROR;
+ }
+
+ rc = ssh_mkdirs(dir, 0700);
+ if (rc < 0) {
+ ssh_set_error(session, SSH_FATAL,
+ "Cannot create %s directory: %s",
+ dir, strerror(errno));
+ SAFE_FREE(dir);
+ return SSH_ERROR;
+ }
+ SAFE_FREE(dir);
+
+ errno = 0;
+ fp = fopen(session->opts.knownhosts, "a");
+ if (fp == NULL) {
+ ssh_set_error(session, SSH_FATAL,
+ "Couldn't open known_hosts file %s"
+ " for appending: %s",
+ session->opts.knownhosts, strerror(errno));
+ return SSH_ERROR;
+ }
+ } else {
+ ssh_set_error(session, SSH_FATAL,
+ "Couldn't open known_hosts file %s for appending: %s",
+ session->opts.knownhosts, strerror(errno));
+ return SSH_ERROR;
+ }
}
rc = ssh_session_export_known_hosts_entry(session, &entry);
diff --git a/tests/client/torture_knownhosts_verify.c b/tests/client/torture_knownhosts_verify.c
index df8363e2..2a2a6b64 100644
--- a/tests/client/torture_knownhosts_verify.c
+++ b/tests/client/torture_knownhosts_verify.c
@@ -35,6 +35,8 @@
#define BAD_RSA "AAAAB3NzaC1yc2EAAAADAQABAAABAQDXvXuawzaArEwkLIXTz/EWywLOCtqQL3P9yKkrhz6AplXP2PhOh5pyxa1VfGKe453jNeYBJ0ROto3BshXgZXbo86oLXTkbe0gO5xi3r5WjXxjOFvRRTLot5fPLNDOv9+TnsPmkNn0iIeyPnfrcPIyjWt5zSWUfkNC8oNHxsiSshjpbJvTXSDipukpUy41d7jg4uWGuonMTF7yu7HfuHqq7lhb0WlwSpfbqAbfYARBddcdcARyhix4RMWZZqVY20H3Vsjq8bjKC+NJXFce1PRg+qcOWQdlXEei4dkzAvHvfQRx1TjzkrBZ6B6thmZtyeb9IsiB0tg2g0JN2VTAGkxqp"
+const char template[] = "temp_dir_XXXXXX";
+
static int sshd_group_setup(void **state)
{
torture_setup_sshd_server(state, false);
@@ -414,6 +416,43 @@ static void torture_knownhosts_conflict(void **state)
/* session will be freed by session_teardown() */
}
+static void torture_knownhosts_new_file(void **state)
+{
+ struct torture_state *s = *state;
+ ssh_session session = s->ssh.session;
+ enum ssh_known_hosts_e found;
+ int rc;
+
+ char new_known_hosts[256];
+ char *tmp_dir = NULL;
+ ssize_t count = 0;
+
+ /* Create a disposable directory */
+ tmp_dir = torture_make_temp_dir(template);
+ assert_non_null(tmp_dir);
+
+ count = snprintf(new_known_hosts, sizeof(new_known_hosts),
+ "%s/a/b/c/d/known_hosts", tmp_dir);
+ assert_return_code(count, errno);
+
+ rc = ssh_options_set(session, SSH_OPTIONS_KNOWNHOSTS, new_known_hosts);
+ assert_ssh_return_code(session, rc);
+
+ rc = ssh_connect(session);
+ assert_ssh_return_code(session, rc);
+
+ rc = ssh_session_update_known_hosts(session);
+ assert_ssh_return_code(session, rc);
+
+ found = ssh_session_is_known_server(session);
+ assert_int_equal(found, SSH_KNOWN_HOSTS_OK);
+
+ /* Cleanup */
+ torture_rmdirs(tmp_dir);
+
+ SAFE_FREE(tmp_dir);
+}
+
int torture_run_tests(void) {
int rc;
struct CMUnitTest tests[] = {
@@ -438,6 +477,9 @@ int torture_run_tests(void) {
cmocka_unit_test_setup_teardown(torture_knownhosts_duplicate,
session_setup,
session_teardown),
+ cmocka_unit_test_setup_teardown(torture_knownhosts_new_file,
+ session_setup,
+ session_teardown),
};
ssh_init();