From 49beb7d7fdcdda5785a0b75e5286bc323028bf5e Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 24 Jul 2015 13:20:36 -0700 Subject: [PATCH] GUAC-1264: Require explicit free for users and sessions. Ensure SSH client data is zeroed upon allocation. --- src/common-ssh/guac_sftp.c | 3 --- src/common-ssh/guac_sftp.h | 3 ++- src/common-ssh/guac_ssh.c | 4 ---- src/common-ssh/guac_ssh.h | 9 ++++++--- src/protocols/ssh/client.c | 4 +--- src/protocols/ssh/client.h | 11 +++++++++++ src/protocols/ssh/guac_handlers.c | 10 ++++++++-- src/protocols/ssh/ssh_client.c | 25 +++++++++++++++---------- 8 files changed, 43 insertions(+), 26 deletions(-) diff --git a/src/common-ssh/guac_sftp.c b/src/common-ssh/guac_sftp.c index 47fb5c13..b5a305ec 100644 --- a/src/common-ssh/guac_sftp.c +++ b/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); - } diff --git a/src/common-ssh/guac_sftp.h b/src/common-ssh/guac_sftp.h index 6fa9f1f6..03c4de15 100644 --- a/src/common-ssh/guac_sftp.h +++ b/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. diff --git a/src/common-ssh/guac_ssh.c b/src/common-ssh/guac_ssh.c index 24150780..68dcf39a 100644 --- a/src/common-ssh/guac_ssh.c +++ b/src/common-ssh/guac_ssh.c @@ -504,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); diff --git a/src/common-ssh/guac_ssh.h b/src/common-ssh/guac_ssh.h index 64c69134..e7a35fab 100644 --- a/src/common-ssh/guac_ssh.h +++ b/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 - * automatically destroyed when this session is destroyed. + * The user to authenticate as, once connected. * * @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. diff --git a/src/protocols/ssh/client.c b/src/protocols/ssh/client.c index d231fa7f..43985b3f 100644 --- a/src/protocols/ssh/client.c +++ b/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; diff --git a/src/protocols/ssh/client.h b/src/protocols/ssh/client.h index 99d3257b..364fca2f 100644 --- a/src/protocols/ssh/client.h +++ b/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. */ diff --git a/src/protocols/ssh/guac_handlers.c b/src/protocols/ssh/guac_handlers.c index 29fcd404..e18c5d5d 100644 --- a/src/protocols/ssh/guac_handlers.c +++ b/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 */ - if (guac_client_data->sftp_filesystem) + /* Clean up the SFTP filesystem object and session */ + 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); diff --git a/src/protocols/ssh/ssh_client.c b/src/protocols/ssh/ssh_client.c index 93797993..6c089252 100644 --- a/src/protocols/ssh/ssh_client.c +++ b/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 */ - else if (client_data->password[0] == 0) { + /* Otherwise, use password */ + else { - guac_terminal_prompt(client_data->term, "Password: ", - client_data->password, sizeof(client_data->password), false); + /* Get password if not provided */ + 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); - if (sftp_ssh_session == NULL) { + client_data->port, client_data->user); + 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;