Merge pull request #58 from glyptodon/password-segfault

GUAC-1264: Fix segfault in password handling

src/common-ssh/guac_sftp.c

@@ -707,8 +707,5 @@ void guac_common_ssh_destroy_sftp_filesystem(guac_object* filesystem) {
     /* Clean up the SFTP filesystem object */
     guac_client_free_object(sftp_data->ssh_session->client, filesystem);
 
-    /* Disconnect SSH session corresponding to the SFTP session */
-    guac_common_ssh_destroy_session(sftp_data->ssh_session);
-
 }
 

src/common-ssh/guac_sftp.h

@@ -110,7 +110,8 @@ guac_object* guac_common_ssh_create_sftp_filesystem(
 
 /**
  * Destroys the given filesystem object, disconnecting from SFTP and freeing
- * and associated resources.
+ * and associated resources. Any associated session or user objects must be
+ * explicitly destroyed.
  *
  * @param object
  *     The filesystem object to destroy.

src/common-ssh/guac_ssh.c

@@ -297,6 +297,13 @@ static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
     char* password = user->password;
     guac_common_ssh_key* key = user->private_key;
 
+    /* Validate username provided */
+    if (username == NULL) {
+        guac_client_abort(client, GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED,
+                "SSH authentication requires a username.");
+        return 1;
+    }
+
     /* Get list of supported authentication methods */
     char* user_authlist = libssh2_userauth_list(session, username,
             strlen(username));
@@ -309,7 +316,8 @@ static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
         /* Check if public key auth is supported on the server */
         if (strstr(user_authlist, "publickey") == NULL) {
             guac_client_abort(client, GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED,
-                               "Public key authentication not supported");
+                    "Public key authentication is not supported by "
+                    "the SSH server");
             return 1;
         }
 
@@ -333,24 +341,35 @@ static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
 
     }
 
-    /* Authenticate with password */
+    /* Authenticate with password, if provided */
-    if (strstr(user_authlist, "password") != NULL) {
+    else if (password != NULL) {
-        guac_client_log(client, GUAC_LOG_DEBUG,
+
-                "Using password authentication method");
+        /* Authenticate with password */
-        return libssh2_userauth_password(session, username, password);
+        if (strstr(user_authlist, "password") != NULL) {
+            guac_client_log(client, GUAC_LOG_DEBUG,
+                    "Using password authentication method");
+            return libssh2_userauth_password(session, username, password);
+        }
+
+        /* Authenticate with password via keyboard-interactive auth */
+        if (strstr(user_authlist, "keyboard-interactive") != NULL) {
+            guac_client_log(client, GUAC_LOG_DEBUG,
+                    "Using keyboard-interactive authentication method");
+            return libssh2_userauth_keyboard_interactive(session, username,
+                    &guac_common_ssh_kbd_callback);
+        }
+
+        /* No known authentication types available */
+        guac_client_abort(client, GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED,
+                "Password and keyboard-interactive authentication are not "
+                "supported by the SSH server");
+        return 1;
+
     }
 
-    /* Authenticate with password via keyboard-interactive auth */
+    /* No credentials provided */
-    if (strstr(user_authlist, "keyboard-interactive") != NULL) {
+    guac_client_abort(client, GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED,
-        guac_client_log(client, GUAC_LOG_DEBUG,
+            "SSH authentication requires either a private key or a password.");
-                "Using keyboard-interactive authentication method");
-        return libssh2_userauth_keyboard_interactive(session, username,
-                &guac_common_ssh_kbd_callback);
-    }
-
-    /* No known authentication types available */
-    guac_client_abort(client, GUAC_PROTOCOL_STATUS_CLIENT_BAD_TYPE,
-            "No known authentication methods");
     return 1;
 
 }
@@ -485,10 +504,6 @@ void guac_common_ssh_destroy_session(guac_common_ssh_session* session) {
     libssh2_session_disconnect(session->session, "Bye");
     libssh2_session_free(session->session);
 
-    /* Destroy associated user */
-    if (session->user)
-        guac_common_ssh_destroy_user(session->user);
-
     /* Free all other data */
     free(session);
 

src/common-ssh/guac_ssh.h

@@ -80,6 +80,9 @@ void guac_common_ssh_uninit();
  * Connects to the SSH server running at the given hostname and port, and
  * authenticates as the given user. If an error occurs while connecting or
  * authenticating, the Guacamole client will automatically and fatally abort.
+ * The user object provided must eventually be explicitly destroyed, but should
+ * not be destroyed until this session is destroyed, assuming the session is
+ * successfully created.
  *
  * @param client
  *     The Guacamole client that will be using SSH.
@@ -91,8 +94,7 @@ void guac_common_ssh_uninit();
  *     The port to connect to on the given hostname.
  *
  * @param user
- *     The user to authenticate as, once connected. This user will be
+ *     The user to authenticate as, once connected.
- *     automatically destroyed when this session is destroyed.
  *
  * @return
  *     A new SSH session if the connection and authentication succeed, or NULL
@@ -103,7 +105,8 @@ guac_common_ssh_session* guac_common_ssh_create_session(guac_client* client,
 
 /**
  * Disconnects and destroys the given SSH session, freeing all associated
- * resources.
+ * resources. Any associated user must be explicitly destroyed, and will not
+ * be destroyed automatically.
  *
  * @param session
  *     The SSH session to destroy.

src/protocols/rdp/client.c

@@ -831,7 +831,8 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
         if (sftp_username[0] == '\0' && settings->username != NULL)
             sftp_username = settings->username;
 
-        guac_common_ssh_user* user = guac_common_ssh_create_user(sftp_username);
+        guac_client_data->sftp_user =
+            guac_common_ssh_create_user(sftp_username);
 
         /* Import private key, if given */
         if (argv[IDX_SFTP_PRIVATE_KEY][0] != '\0') {
@@ -840,10 +841,10 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
                     "Authenticating with private key.");
 
             /* Abort if private key cannot be read */
-            if (guac_common_ssh_user_import_key(user,
+            if (guac_common_ssh_user_import_key(guac_client_data->sftp_user,
                         argv[IDX_SFTP_PRIVATE_KEY],
                         argv[IDX_SFTP_PASSPHRASE])) {
-                guac_common_ssh_destroy_user(user);
+                guac_common_ssh_destroy_user(guac_client_data->sftp_user);
                 return 1;
             }
 
@@ -856,11 +857,12 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
                     "Authenticating with password.");
 
             /* Parse password - use RDP password by default */
-            const char* sftp_password = argv[IDX_SFTP_USERNAME];
+            const char* sftp_password = argv[IDX_SFTP_PASSWORD];
             if (sftp_password[0] == '\0' && settings->password != NULL)
                 sftp_password = settings->password;
 
-            guac_common_ssh_user_set_password(user, sftp_password);
+            guac_common_ssh_user_set_password(guac_client_data->sftp_user,
+                    sftp_password);
 
         }
 
@@ -875,24 +877,28 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
             sftp_port = "22";
 
         /* Attempt SSH connection */
-        guac_common_ssh_session* session =
+        guac_client_data->sftp_session =
             guac_common_ssh_create_session(client, sftp_hostname, sftp_port,
-                    user);
+                    guac_client_data->sftp_user);
 
         /* Fail if SSH connection does not succeed */
-        if (session == NULL) {
+        if (guac_client_data->sftp_session == NULL) {
             /* Already aborted within guac_common_ssh_create_session() */
-            guac_common_ssh_destroy_user(user);
+            guac_common_ssh_destroy_user(guac_client_data->sftp_user);
             return 1;
         }
 
         /* Load and expose filesystem */
         guac_client_data->sftp_filesystem =
-            guac_common_ssh_create_sftp_filesystem(session, "/");
+            guac_common_ssh_create_sftp_filesystem(
+                    guac_client_data->sftp_session, "/");
 
         /* Abort if SFTP connection fails */
-        if (guac_client_data->sftp_filesystem == NULL)
+        if (guac_client_data->sftp_filesystem == NULL) {
+            guac_common_ssh_destroy_session(guac_client_data->sftp_session);
+            guac_common_ssh_destroy_user(guac_client_data->sftp_user);
             return 1;
+        }
 
         /* Use SFTP for basic uploads, if drive not enabled */
         if (!settings->drive_enabled)

src/protocols/rdp/client.h

@@ -35,6 +35,8 @@
 
 #ifdef ENABLE_COMMON_SSH
 #include "guac_sftp.h"
+#include "guac_ssh.h"
+#include "guac_ssh_user.h"
 #endif
 
 #ifdef HAVE_FREERDP_DISPLAY_UPDATE_SUPPORT
@@ -162,6 +164,16 @@ typedef struct rdp_guac_client_data {
     guac_rdp_fs* filesystem;
 
 #ifdef ENABLE_COMMON_SSH
+    /**
+     * The user and credentials used to authenticate for SFTP.
+     */
+    guac_common_ssh_user* sftp_user;
+
+    /**
+     * The SSH session used for SFTP.
+     */
+    guac_common_ssh_session* sftp_session;
+
     /**
      * The exposed filesystem object, implemented with SFTP.
      */

src/protocols/rdp/guac_handlers.c

@@ -36,6 +36,7 @@
 #ifdef ENABLE_COMMON_SSH
 #include <guac_sftp.h>
 #include <guac_ssh.h>
+#include <guac_ssh_user.h>
 #endif
 
 #include <freerdp/cache/cache.h>
@@ -99,6 +100,14 @@ int rdp_guac_client_free_handler(guac_client* client) {
     if (guac_client_data->sftp_filesystem)
         guac_common_ssh_destroy_sftp_filesystem(guac_client_data->sftp_filesystem);
 
+    /* Free SFTP session */
+    if (guac_client_data->sftp_session)
+        guac_common_ssh_destroy_session(guac_client_data->sftp_session);
+
+    /* Free SFTP user */
+    if (guac_client_data->sftp_user)
+        guac_common_ssh_destroy_user(guac_client_data->sftp_user);
+
     guac_common_ssh_uninit();
 #endif
 

src/protocols/ssh/client.c

@@ -120,11 +120,10 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
 
     guac_socket* socket = client->socket;
 
-    ssh_guac_client_data* client_data = malloc(sizeof(ssh_guac_client_data));
+    ssh_guac_client_data* client_data = calloc(1, sizeof(ssh_guac_client_data));
 
     /* Init client data */
     client->data = client_data;
-    client_data->term_channel = NULL;
 
     if (argc != SSH_ARGS_COUNT) {
         guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Wrong number of arguments");
@@ -159,7 +158,6 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
 
     /* Parse SFTP enable */
     client_data->enable_sftp = strcmp(argv[IDX_ENABLE_SFTP], "true") == 0;
-    client_data->sftp_filesystem = NULL;
 
 #ifdef ENABLE_SSH_AGENT
     client_data->enable_agent = strcmp(argv[IDX_ENABLE_AGENT], "true") == 0;

src/protocols/ssh/client.h

@@ -27,6 +27,7 @@
 #include "config.h"
 
 #include "guac_ssh.h"
+#include "guac_ssh_user.h"
 #include "sftp.h"
 #include "terminal.h"
 
@@ -109,11 +110,21 @@ typedef struct ssh_guac_client_data {
      */
     pthread_t client_thread;
 
+    /**
+     * The user and credentials to use for all SSH sessions.
+     */
+    guac_common_ssh_user* user;
+
     /**
      * SSH session, used by the SSH client thread.
      */
     guac_common_ssh_session* session;
 
+    /**
+     * SFTP session, used by the SFTP client/filesystem.
+     */
+    guac_common_ssh_session* sftp_session;
+
     /**
      * The filesystem object exposed for the SFTP session.
      */

src/protocols/ssh/guac_handlers.c

@@ -102,14 +102,20 @@ int ssh_guac_client_free_handler(guac_client* client) {
     /* Free channels */
     libssh2_channel_free(guac_client_data->term_channel);
 
-    /* Clean up the SFTP filesystem object */
+    /* Clean up the SFTP filesystem object and session */
-    if (guac_client_data->sftp_filesystem)
+    if (guac_client_data->sftp_filesystem) {
         guac_common_ssh_destroy_sftp_filesystem(guac_client_data->sftp_filesystem);
+        guac_common_ssh_destroy_session(guac_client_data->sftp_session);
+    }
 
     /* Free session */
     if (guac_client_data->session != NULL)
         guac_common_ssh_destroy_session(guac_client_data->session);
 
+    /* Free user */
+    if (guac_client_data->user != NULL)
+        guac_common_ssh_destroy_user(guac_client_data->user);
+
     /* Free generic data struct */
     free(client->data);
 

src/protocols/ssh/ssh_client.c

@@ -130,12 +130,16 @@ static guac_common_ssh_user* guac_ssh_get_user(guac_client* client) {
 
     } /* end if key given */
 
-    /* Otherwise, get password if not provided */
+    /* Otherwise, use password */
-    else if (client_data->password[0] == 0) {
+    else {
 
-        guac_terminal_prompt(client_data->term, "Password: ",
+        /* Get password if not provided */
-                client_data->password, sizeof(client_data->password), false);
+        if (client_data->password[0] == 0)
+            guac_terminal_prompt(client_data->term, "Password: ",
+                    client_data->password, sizeof(client_data->password),
+                    false);
 
+        /* Set provided password */
         guac_common_ssh_user_set_password(user, client_data->password);
 
     }
@@ -181,7 +185,7 @@ void* ssh_client_thread(void* data) {
         return NULL;
 
     /* Get user and credentials */
-    guac_common_ssh_user* user = guac_ssh_get_user(client);
+    client_data->user = guac_ssh_get_user(client);
 
     /* Send new name */
     char name[1024];
@@ -191,7 +195,7 @@ void* ssh_client_thread(void* data) {
 
     /* Open SSH session */
     client_data->session = guac_common_ssh_create_session(client,
-            client_data->hostname, client_data->port, user);
+            client_data->hostname, client_data->port, client_data->user);
     if (client_data->session == NULL) {
         /* Already aborted within guac_common_ssh_create_session() */
         return NULL;
@@ -229,17 +233,18 @@ void* ssh_client_thread(void* data) {
 
         /* Create SSH session specific for SFTP */
         guac_client_log(client, GUAC_LOG_DEBUG, "Reconnecting for SFTP...");
-        guac_common_ssh_session* sftp_ssh_session =
+        client_data->sftp_session =
             guac_common_ssh_create_session(client, client_data->hostname,
-                    client_data->port, user);
+                    client_data->port, client_data->user);
-        if (sftp_ssh_session == NULL) {
+        if (client_data->sftp_session == NULL) {
             /* Already aborted within guac_common_ssh_create_session() */
             return NULL;
         }
 
         /* Request SFTP */
         client_data->sftp_filesystem =
-            guac_common_ssh_create_sftp_filesystem(sftp_ssh_session, "/");
+            guac_common_ssh_create_sftp_filesystem(
+                    client_data->sftp_session, "/");
 
         /* Set generic (non-filesystem) file upload handler */
         client->file_handler = guac_sftp_file_handler;

src/protocols/vnc/client.c

@@ -372,7 +372,7 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
         guac_client_log(client, GUAC_LOG_DEBUG,
                 "Connecting via SSH for SFTP filesystem access.");
 
-        guac_common_ssh_user* user =
+        guac_client_data->sftp_user =
             guac_common_ssh_create_user(argv[IDX_SFTP_USERNAME]);
 
         /* Import private key, if given */
@@ -382,10 +382,10 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
                     "Authenticating with private key.");
 
             /* Abort if private key cannot be read */
-            if (guac_common_ssh_user_import_key(user,
+            if (guac_common_ssh_user_import_key(guac_client_data->sftp_user,
                         argv[IDX_SFTP_PRIVATE_KEY],
                         argv[IDX_SFTP_PASSPHRASE])) {
-                guac_common_ssh_destroy_user(user);
+                guac_common_ssh_destroy_user(guac_client_data->sftp_user);
                 return 1;
             }
 
@@ -395,7 +395,8 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
         else {
             guac_client_log(client, GUAC_LOG_DEBUG,
                     "Authenticating with password.");
-            guac_common_ssh_user_set_password(user, argv[IDX_SFTP_PASSWORD]);
+            guac_common_ssh_user_set_password(guac_client_data->sftp_user,
+                    argv[IDX_SFTP_PASSWORD]);
         }
 
         /* Parse hostname - use VNC hostname by default */
@@ -409,24 +410,28 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
             sftp_port = "22";
 
         /* Attempt SSH connection */
-        guac_common_ssh_session* session =
+        guac_client_data->sftp_session =
             guac_common_ssh_create_session(client, sftp_hostname, sftp_port,
-                    user);
+                    guac_client_data->sftp_user);
 
         /* Fail if SSH connection does not succeed */
-        if (session == NULL) {
+        if (guac_client_data->sftp_session == NULL) {
             /* Already aborted within guac_common_ssh_create_session() */
-            guac_common_ssh_destroy_user(user);
+            guac_common_ssh_destroy_user(guac_client_data->sftp_user);
             return 1;
         }
 
         /* Load and expose filesystem */
         guac_client_data->sftp_filesystem =
-            guac_common_ssh_create_sftp_filesystem(session, "/");
+            guac_common_ssh_create_sftp_filesystem(
+                    guac_client_data->sftp_session, "/");
 
         /* Abort if SFTP connection fails */
-        if (guac_client_data->sftp_filesystem == NULL)
+        if (guac_client_data->sftp_filesystem == NULL) {
+            guac_common_ssh_destroy_session(guac_client_data->sftp_session);
+            guac_common_ssh_destroy_user(guac_client_data->sftp_user);
             return 1;
+        }
 
         /* Set file handler for basic uploads */
         client->file_handler = guac_vnc_sftp_file_handler;

src/protocols/vnc/client.h

@@ -38,6 +38,8 @@
 
 #ifdef ENABLE_COMMON_SSH
 #include "guac_sftp.h"
+#include "guac_ssh.h"
+#include "guac_ssh_user.h"
 #endif
 
 /**
@@ -191,6 +193,16 @@ typedef struct vnc_guac_client_data {
     guac_common_surface* default_surface;
 
 #ifdef ENABLE_COMMON_SSH
+    /**
+     * The user and credentials used to authenticate for SFTP.
+     */
+    guac_common_ssh_user* sftp_user;
+
+    /**
+     * The SSH session used for SFTP.
+     */
+    guac_common_ssh_session* sftp_session;
+
     /**
      * The exposed filesystem object, implemented with SFTP.
      */

src/protocols/vnc/guac_handlers.c

@@ -34,6 +34,7 @@
 #ifdef ENABLE_COMMON_SSH
 #include <guac_sftp.h>
 #include <guac_ssh.h>
+#include <guac_ssh_user.h>
 #endif
 
 #ifdef ENABLE_PULSE
@@ -145,6 +146,14 @@ int vnc_guac_client_free_handler(guac_client* client) {
     if (guac_client_data->sftp_filesystem)
         guac_common_ssh_destroy_sftp_filesystem(guac_client_data->sftp_filesystem);
 
+    /* Free SFTP session */
+    if (guac_client_data->sftp_session)
+        guac_common_ssh_destroy_session(guac_client_data->sftp_session);
+
+    /* Free SFTP user */
+    if (guac_client_data->sftp_user)
+        guac_common_ssh_destroy_user(guac_client_data->sftp_user);
+
     guac_common_ssh_uninit();
 #endif