From f8f16c44a9cc16372d94bf06a5540a4b15528c2c Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 23 Jul 2015 16:28:12 -0700 Subject: [PATCH 1/4] GUAC-1264: Validate provided credentials. Log if anything is missing. --- src/common-ssh/guac_ssh.c | 53 ++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/common-ssh/guac_ssh.c b/src/common-ssh/guac_ssh.c index bc6e8a0d..24150780 100644 --- a/src/common-ssh/guac_ssh.c +++ b/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 */ - 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, if provided */ + else if (password != NULL) { + + /* Authenticate with 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 */ - 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_BAD_TYPE, - "No known authentication methods"); + /* No credentials provided */ + guac_client_abort(client, GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED, + "SSH authentication requires either a private key or a password."); return 1; } From 49beb7d7fdcdda5785a0b75e5286bc323028bf5e Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 24 Jul 2015 13:20:36 -0700 Subject: [PATCH 2/4] 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; From ec595b9cff135d314fc2cb8f1c0531797576b3cf Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 24 Jul 2015 13:41:20 -0700 Subject: [PATCH 3/4] GUAC-1264: Explicitly free users and sessions within VNC and RDP. --- src/protocols/rdp/client.c | 26 ++++++++++++++++---------- src/protocols/rdp/client.h | 12 ++++++++++++ src/protocols/rdp/guac_handlers.c | 9 +++++++++ src/protocols/vnc/client.c | 25 +++++++++++++++---------- src/protocols/vnc/client.h | 12 ++++++++++++ src/protocols/vnc/guac_handlers.c | 9 +++++++++ 6 files changed, 73 insertions(+), 20 deletions(-) diff --git a/src/protocols/rdp/client.c b/src/protocols/rdp/client.c index 8e2945c5..63fffd71 100644 --- a/src/protocols/rdp/client.c +++ b/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; } @@ -860,7 +861,8 @@ int guac_client_init(guac_client* client, int argc, char** argv) { 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) diff --git a/src/protocols/rdp/client.h b/src/protocols/rdp/client.h index b23f8e79..7bcefe4a 100644 --- a/src/protocols/rdp/client.h +++ b/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. */ diff --git a/src/protocols/rdp/guac_handlers.c b/src/protocols/rdp/guac_handlers.c index a660f34e..86eec9c4 100644 --- a/src/protocols/rdp/guac_handlers.c +++ b/src/protocols/rdp/guac_handlers.c @@ -36,6 +36,7 @@ #ifdef ENABLE_COMMON_SSH #include #include +#include #endif #include @@ -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 diff --git a/src/protocols/vnc/client.c b/src/protocols/vnc/client.c index 426b6ced..874f8579 100644 --- a/src/protocols/vnc/client.c +++ b/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; diff --git a/src/protocols/vnc/client.h b/src/protocols/vnc/client.h index 7ad322dd..5827f7f9 100644 --- a/src/protocols/vnc/client.h +++ b/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. */ diff --git a/src/protocols/vnc/guac_handlers.c b/src/protocols/vnc/guac_handlers.c index 92667628..be380df0 100644 --- a/src/protocols/vnc/guac_handlers.c +++ b/src/protocols/vnc/guac_handlers.c @@ -34,6 +34,7 @@ #ifdef ENABLE_COMMON_SSH #include #include +#include #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 From 6f48ebe7fec7a962e5f7dcc80397c893d2a81cad Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 24 Jul 2015 13:54:39 -0700 Subject: [PATCH 4/4] GUAC-1264: Use proper parameter for SFTP password within RDP. --- src/protocols/rdp/client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/protocols/rdp/client.c b/src/protocols/rdp/client.c index 63fffd71..a2665c7b 100644 --- a/src/protocols/rdp/client.c +++ b/src/protocols/rdp/client.c @@ -857,7 +857,7 @@ 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;