Reproducer for memory leak from parsing knonw hosts

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Norbert Pocs <norbertpocs0@gmail.com>

src/gssapi.c

@@ -850,6 +850,10 @@ int ssh_gssapi_client_identity(ssh_session session, gss_OID_set *valid_oids)
     char *ptr = NULL;
     int ret;
 
+    if (session == NULL || session->gssapi == NULL) {
+        return SSH_ERROR;
+    }
+
     if (session->gssapi->client.client_deleg_creds == NULL) {
         if (session->opts.gss_client_identity != NULL) {
             namebuf.value = (void *)session->opts.gss_client_identity;

src/knownhosts.c

@@ -216,11 +216,22 @@ ssh_known_hosts_entries_compare(struct ssh_knownhosts_entry *k1,
     return 0;
 }
 
-/* This method reads the known_hosts file referenced by the path
+/**
+ * @internal
+ *
+ * @brief Read entries from filename to provided list
+ *
+ * This method reads the known_hosts file referenced by the path
  * in  filename  argument, and entries matching the  match  argument
  * will be added to the list in  entries  argument.
  * If the  entries  list is NULL, it will allocate a new list. Caller
  * is responsible to free it even if an error occurs.
+ *
+ * @param match[in]       The host name (with port) to match against
+ * @param filename[in]    The known hosts file to parse
+ * @param entries[in,out] The list of entries to append matching ones
+ * @return                `SSH_OK` on missing file or success parsing,
+ *                        `SSH_ERROR` on error
  */
 static int ssh_known_hosts_read_entries(const char *match,
                                         const char *filename,
@@ -346,6 +357,33 @@ static char *ssh_session_get_host_port(ssh_session session)
 
 /**
  * @internal
+ *
+ * @brief Free known hosts entries list
+ *
+ * @param[in] entry_list The list of ssh_knownhosts_entry items
+ */
+static void ssh_knownhosts_entries_free(struct ssh_list *entry_list)
+{
+    struct ssh_iterator *it = NULL;
+
+    if (entry_list == NULL) {
+        return;
+    }
+
+    for (it = ssh_list_get_iterator(entry_list);
+         it != NULL;
+         it = ssh_list_get_iterator(entry_list)) {
+        struct ssh_knownhosts_entry *entry = NULL;
+
+        entry = ssh_iterator_value(struct ssh_knownhosts_entry *, it);
+        ssh_knownhosts_entry_free(entry);
+        ssh_list_remove(entry_list, it);
+    }
+    ssh_list_free(entry_list);
+}
+/**
+ * @internal
+ *
  * @brief Check which host keys should be preferred for the session.
  *
  * This checks the known_hosts file to find out which algorithms should be
@@ -453,7 +491,7 @@ struct ssh_list *ssh_known_hosts_get_algorithms(ssh_session session)
 
     return list;
 error:
-    ssh_list_free(entry_list);
+    ssh_knownhosts_entries_free(entry_list);
     ssh_list_free(list);
     return NULL;
 }
@@ -505,6 +543,7 @@ static const char *ssh_known_host_sigs_from_hostkey_type(enum ssh_keytypes_e typ
 
 /**
  * @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
@@ -549,7 +588,7 @@ char *ssh_known_hosts_get_algorithms_names(ssh_session session)
                                       &entry_list);
     if (rc != 0) {
         SAFE_FREE(host_port);
-        ssh_list_free(entry_list);
+        ssh_knownhosts_entries_free(entry_list);
         return NULL;
     }
 
@@ -558,7 +597,7 @@ char *ssh_known_hosts_get_algorithms_names(ssh_session session)
                                       &entry_list);
     SAFE_FREE(host_port);
     if (rc != 0) {
-        ssh_list_free(entry_list);
+        ssh_knownhosts_entries_free(entry_list);
         return NULL;
     }
 
@@ -799,7 +838,6 @@ out:
 enum ssh_known_hosts_e ssh_session_has_known_hosts_entry(ssh_session session)
 {
     struct ssh_list *entry_list = NULL;
-    struct ssh_iterator *it = NULL;
     char *host_port = NULL;
     bool global_known_hosts_found = false;
     bool known_hosts_found = false;
@@ -860,7 +898,7 @@ enum ssh_known_hosts_e ssh_session_has_known_hosts_entry(ssh_session session)
                                           &entry_list);
         if (rc != 0) {
             SAFE_FREE(host_port);
-            ssh_list_free(entry_list);
+            ssh_knownhosts_entries_free(entry_list);
             return SSH_KNOWN_HOSTS_ERROR;
         }
     }
@@ -871,7 +909,7 @@ enum ssh_known_hosts_e ssh_session_has_known_hosts_entry(ssh_session session)
                                           &entry_list);
         if (rc != 0) {
             SAFE_FREE(host_port);
-            ssh_list_free(entry_list);
+            ssh_knownhosts_entries_free(entry_list);
             return SSH_KNOWN_HOSTS_ERROR;
         }
     }
@@ -883,16 +921,7 @@ enum ssh_known_hosts_e ssh_session_has_known_hosts_entry(ssh_session session)
         return SSH_KNOWN_HOSTS_UNKNOWN;
     }
 
-    for (it = ssh_list_get_iterator(entry_list);
+    ssh_knownhosts_entries_free(entry_list);
-         it != NULL;
-         it = ssh_list_get_iterator(entry_list)) {
-        struct ssh_knownhosts_entry *entry = NULL;
-
-        entry = ssh_iterator_value(struct ssh_knownhosts_entry *, it);
-        ssh_knownhosts_entry_free(entry);
-        ssh_list_remove(entry_list, it);
-    }
-    ssh_list_free(entry_list);
 
     return SSH_KNOWN_HOSTS_OK;
 }
@@ -1079,13 +1108,13 @@ ssh_known_hosts_check_server_key(const char *hosts_entry,
                                       filename,
                                       &entry_list);
     if (rc != 0) {
-        ssh_list_free(entry_list);
+        ssh_knownhosts_entries_free(entry_list);
         return SSH_KNOWN_HOSTS_UNKNOWN;
     }
 
     it = ssh_list_get_iterator(entry_list);
     if (it == NULL) {
-        ssh_list_free(entry_list);
+        ssh_knownhosts_entries_free(entry_list);
         return SSH_KNOWN_HOSTS_UNKNOWN;
     }
 
@@ -1115,16 +1144,7 @@ ssh_known_hosts_check_server_key(const char *hosts_entry,
         }
     }
 
-    for (it = ssh_list_get_iterator(entry_list);
+    ssh_knownhosts_entries_free(entry_list);
-         it != NULL;
-         it = ssh_list_get_iterator(entry_list)) {
-        struct ssh_knownhosts_entry *entry = NULL;
-
-        entry = ssh_iterator_value(struct ssh_knownhosts_entry *, it);
-        ssh_knownhosts_entry_free(entry);
-        ssh_list_remove(entry_list, it);
-    }
-    ssh_list_free(entry_list);
 
     return found;
 }
@@ -1196,6 +1216,8 @@ ssh_session_get_known_hosts_entry(ssh_session session,
 }
 
 /**
+ * @internal
+ *
  * @brief Get the known_hosts entry for the current connected session
  *        from the given known_hosts file.
  *

tests/unittests/torture_knownhosts_parsing.c

@@ -696,6 +696,82 @@ static void torture_knownhosts_algorithms_global(void **state)
     ssh_free(session);
 }
 
+static int setup_bad_knownhosts_file(void **state)
+{
+    char *tmp_file = NULL;
+    size_t nwritten;
+    FILE *fp = NULL;
+    int rc = 0;
+
+    tmp_file = torture_create_temp_file(TMP_FILE_NAME);
+    assert_non_null(tmp_file);
+
+    *state = tmp_file;
+
+    fp = fopen(tmp_file, "w");
+    assert_non_null(fp);
+
+    nwritten = fwrite(LOCALHOST_DEFAULT_ED25519,
+                      sizeof(char),
+                      strlen(LOCALHOST_DEFAULT_ED25519),
+                      fp);
+    if (nwritten != strlen(LOCALHOST_DEFAULT_ED25519)) {
+        rc = -1;
+        goto close_fp;
+    }
+
+    nwritten = fwrite("\n", sizeof(char), 1, fp);
+    if (nwritten != 1) {
+        rc = -1;
+        goto close_fp;
+    }
+
+#define LOCALHOST_BAD_LINE "localhost \n"
+    nwritten = fwrite(LOCALHOST_BAD_LINE,
+                      sizeof(char),
+                      strlen(LOCALHOST_BAD_LINE),
+                      fp);
+    if (nwritten != strlen(LOCALHOST_BAD_LINE)) {
+        rc = -1;
+        goto close_fp;
+    }
+
+close_fp:
+    fclose(fp);
+
+    return rc;
+}
+
+static void torture_knownhosts_has_entry(void **state)
+{
+    const char *knownhosts_file = *state;
+    enum ssh_known_hosts_e found;
+    ssh_session session;
+    bool process_config = false;
+    struct ssh_knownhosts_entry *entry = NULL;
+
+    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");
+    ssh_options_set(session, SSH_OPTIONS_GLOBAL_KNOWNHOSTS, knownhosts_file);
+
+    /* Error is expected -- this tests the memory is not leaked from this
+     * test case */
+    found = ssh_session_has_known_hosts_entry(session);
+    assert_int_equal(found, SSH_KNOWN_HOSTS_ERROR);
+
+    found = ssh_session_get_known_hosts_entry(session, &entry);
+    assert_int_equal(found, SSH_KNOWN_HOSTS_ERROR);
+    assert_null(entry);
+
+    ssh_free(session);
+}
 #endif /* _WIN32 There is no /dev/null on Windows */
 
 int torture_run_tests(void) {
@@ -738,6 +814,9 @@ int torture_run_tests(void) {
         cmocka_unit_test_setup_teardown(torture_knownhosts_algorithms_global,
                                         setup_knownhosts_file,
                                         teardown_knownhosts_file),
+        cmocka_unit_test_setup_teardown(torture_knownhosts_has_entry,
+                                        setup_bad_knownhosts_file,
+                                        teardown_knownhosts_file),
 #endif
     };