From e5c1147cf6ebcc150d4a352d5245547dc2027b06 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 12 Nov 2018 15:19:33 -0800 Subject: [PATCH] GUACAMOLE-637: Replace usages of strncpy() with guac_strlcpy(). --- src/common-ssh/sftp.c | 26 +++++++++++----- src/common/clipboard.c | 4 +-- src/protocols/rdp/guac_svc/svc_service.c | 3 +- src/protocols/rdp/rdp_fs.c | 38 +++++++++++------------- src/protocols/rdp/rdp_settings.c | 9 +++--- src/protocols/rdp/rdp_stream.c | 5 ++-- src/protocols/rdp/rdp_svc.c | 14 ++++----- src/protocols/rdp/rdp_svc.h | 7 +++-- 8 files changed, 58 insertions(+), 48 deletions(-) diff --git a/src/common-ssh/sftp.c b/src/common-ssh/sftp.c index 8a53b264..51fa7cd5 100644 --- a/src/common-ssh/sftp.c +++ b/src/common-ssh/sftp.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -74,10 +75,15 @@ static int guac_common_ssh_sftp_normalize_path(char* fullpath, path++; /* Copy path into component data for parsing */ - strncpy(path_component_data, path, sizeof(path_component_data) - 1); + int length = guac_strlcpy(path_component_data, path, + sizeof(path_component_data)); + + /* Fail if input path was truncated */ + if (length >= sizeof(path_component_data)) + return 1; /* Find path components within path */ - for (i = 0; i < sizeof(path_component_data) - 1; i++) { + for (i = 0; i < sizeof(path_component_data); i++) { /* If current character is a path separator, parse as component */ char c = path_component_data[i]; @@ -114,9 +120,6 @@ static int guac_common_ssh_sftp_normalize_path(char* fullpath, return 1; } - /* Ensure last component is null-terminated */ - path_component_data[i] = 0; - /* Convert components back into path */ for (; path_depth > 0; path_depth--) { @@ -830,8 +833,17 @@ static int guac_common_ssh_sftp_get_handler(guac_user* user, list_state->directory = dir; list_state->filesystem = filesystem; - strncpy(list_state->directory_name, name, - sizeof(list_state->directory_name) - 1); + + int length = guac_strlcpy(list_state->directory_name, name, + sizeof(list_state->directory_name)); + + /* Bail out if directory name is too long to store */ + if (length >= sizeof(list_state->directory_name)) { + guac_user_log(user, GUAC_LOG_INFO, "Unable to read directory " + "\"%s\": Path too long", fullpath); + free(list_state); + return 0; + } /* Allocate stream for body */ guac_stream* stream = guac_user_alloc_stream(user); diff --git a/src/common/clipboard.c b/src/common/clipboard.c index eb2f5480..e02a7ca4 100644 --- a/src/common/clipboard.c +++ b/src/common/clipboard.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -131,8 +132,7 @@ void guac_common_clipboard_reset(guac_common_clipboard* clipboard, clipboard->length = 0; /* Assign given mimetype */ - strncpy(clipboard->mimetype, mimetype, sizeof(clipboard->mimetype) - 1); - clipboard->mimetype[sizeof(clipboard->mimetype) - 1] = '\0'; + guac_strlcpy(clipboard->mimetype, mimetype, sizeof(clipboard->mimetype)); pthread_mutex_unlock(&(clipboard->lock)); diff --git a/src/protocols/rdp/guac_svc/svc_service.c b/src/protocols/rdp/guac_svc/svc_service.c index 4a38cb38..c40c6c5f 100644 --- a/src/protocols/rdp/guac_svc/svc_service.c +++ b/src/protocols/rdp/guac_svc/svc_service.c @@ -29,6 +29,7 @@ #include #include #include +#include #ifdef ENABLE_WINPR #include @@ -53,7 +54,7 @@ int VirtualChannelEntry(PCHANNEL_ENTRY_POINTS pEntryPoints) { guac_rdp_svc* svc = (guac_rdp_svc*) entry_points_ex->pExtendedData; /* Init channel def */ - strncpy(svc_plugin->plugin.channel_def.name, svc->name, + guac_strlcpy(svc_plugin->plugin.channel_def.name, svc->name, GUAC_RDP_SVC_MAX_LENGTH); svc_plugin->plugin.channel_def.options = CHANNEL_OPTION_INITIALIZED diff --git a/src/protocols/rdp/rdp_fs.c b/src/protocols/rdp/rdp_fs.c index ab48cc2f..db58c77e 100644 --- a/src/protocols/rdp/rdp_fs.c +++ b/src/protocols/rdp/rdp_fs.c @@ -39,6 +39,7 @@ #include #include #include +#include #include guac_rdp_fs* guac_rdp_fs_alloc(guac_client* client, const char* drive_path, @@ -607,11 +608,11 @@ const char* guac_rdp_fs_read_dir(guac_rdp_fs* fs, int file_id) { int guac_rdp_fs_normalize_path(const char* path, char* abs_path) { int i; - int path_depth = 0; + int path_depth = 1; char path_component_data[GUAC_RDP_FS_MAX_PATH]; - const char* path_components[64]; + const char* path_components[64] = { "" }; - const char** current_path_component = &(path_components[0]); + const char** current_path_component = &(path_components[1]); const char* current_path_component_data = &(path_component_data[0]); /* If original path is not absolute, normalization fails */ @@ -622,10 +623,15 @@ int guac_rdp_fs_normalize_path(const char* path, char* abs_path) { path++; /* Copy path into component data for parsing */ - strncpy(path_component_data, path, sizeof(path_component_data) - 1); + int length = guac_strlcpy(path_component_data, path, + sizeof(path_component_data)); + + /* Fail if input path was truncated */ + if (length >= sizeof(path_component_data)) + return 1; /* Find path components within path */ - for (i = 0; i < sizeof(path_component_data) - 1; i++) { + for (i = 0; i < sizeof(path_component_data); i++) { /* If current character is a path separator, parse as component */ char c = path_component_data[i]; @@ -666,9 +672,6 @@ int guac_rdp_fs_normalize_path(const char* path, char* abs_path) { return 0; } - /* Ensure last component is null-terminated */ - path_component_data[i] = 0; - /* Convert components back into path */ for (; path_depth > 0; path_depth--) { @@ -691,26 +694,19 @@ int guac_rdp_fs_normalize_path(const char* path, char* abs_path) { int guac_rdp_fs_convert_path(const char* parent, const char* rel_path, char* abs_path) { - int i; + int length; char combined_path[GUAC_RDP_FS_MAX_PATH]; - char* current = combined_path; /* Copy parent path */ - for (i=0; i #include +#include #include #ifdef ENABLE_WINPR @@ -1242,11 +1243,11 @@ void guac_rdp_push_settings(guac_client* client, /* Client name */ if (guac_settings->client_name != NULL) { #ifdef LEGACY_RDPSETTINGS - strncpy(rdp_settings->client_hostname, guac_settings->client_name, - RDP_CLIENT_HOSTNAME_SIZE - 1); + guac_strlcpy(rdp_settings->client_hostname, guac_settings->client_name, + RDP_CLIENT_HOSTNAME_SIZE); #else - strncpy(rdp_settings->ClientHostname, guac_settings->client_name, - RDP_CLIENT_HOSTNAME_SIZE - 1); + guac_strlcpy(rdp_settings->ClientHostname, guac_settings->client_name, + RDP_CLIENT_HOSTNAME_SIZE); #endif } diff --git a/src/protocols/rdp/rdp_stream.c b/src/protocols/rdp/rdp_stream.c index 5dae3667..533ff07e 100644 --- a/src/protocols/rdp/rdp_stream.c +++ b/src/protocols/rdp/rdp_stream.c @@ -32,6 +32,7 @@ #include #include #include +#include #ifdef HAVE_FREERDP_CLIENT_CLIPRDR_H #include @@ -504,8 +505,8 @@ int guac_rdp_download_get_handler(guac_user* user, guac_object* object, rdp_stream->type = GUAC_RDP_LS_STREAM; rdp_stream->ls_status.fs = fs; rdp_stream->ls_status.file_id = file_id; - strncpy(rdp_stream->ls_status.directory_name, name, - sizeof(rdp_stream->ls_status.directory_name) - 1); + guac_strlcpy(rdp_stream->ls_status.directory_name, name, + sizeof(rdp_stream->ls_status.directory_name)); /* Allocate stream for body */ guac_stream* stream = guac_user_alloc_stream(user); diff --git a/src/protocols/rdp/rdp_svc.c b/src/protocols/rdp/rdp_svc.c index 83537a86..0a6eb246 100644 --- a/src/protocols/rdp/rdp_svc.c +++ b/src/protocols/rdp/rdp_svc.c @@ -25,6 +25,7 @@ #include #include +#include #ifdef ENABLE_WINPR #include @@ -33,7 +34,6 @@ #endif #include -#include guac_rdp_svc* guac_rdp_alloc_svc(guac_client* client, char* name) { @@ -44,16 +44,14 @@ guac_rdp_svc* guac_rdp_alloc_svc(guac_client* client, char* name) { svc->plugin = NULL; svc->output_pipe = NULL; + /* Init name */ + int name_length = guac_strlcpy(svc->name, name, GUAC_RDP_SVC_MAX_LENGTH); + /* Warn about name length */ - if (strnlen(name, GUAC_RDP_SVC_MAX_LENGTH+1) > GUAC_RDP_SVC_MAX_LENGTH) + if (name_length >= GUAC_RDP_SVC_MAX_LENGTH) guac_client_log(client, GUAC_LOG_INFO, "Static channel name \"%s\" exceeds maximum of %i characters " - "and will be truncated", - name, GUAC_RDP_SVC_MAX_LENGTH); - - /* Init name */ - strncpy(svc->name, name, GUAC_RDP_SVC_MAX_LENGTH); - svc->name[GUAC_RDP_SVC_MAX_LENGTH] = '\0'; + "and will be truncated", name, GUAC_RDP_SVC_MAX_LENGTH - 1); return svc; } diff --git a/src/protocols/rdp/rdp_svc.h b/src/protocols/rdp/rdp_svc.h index 322c4d9a..ebb3a13f 100644 --- a/src/protocols/rdp/rdp_svc.h +++ b/src/protocols/rdp/rdp_svc.h @@ -27,9 +27,10 @@ #include /** - * The maximum number of characters to allow for each channel name. + * The maximum number of bytes to allow within each channel name, including + * null terminator. */ -#define GUAC_RDP_SVC_MAX_LENGTH 7 +#define GUAC_RDP_SVC_MAX_LENGTH 8 /** * Structure describing a static virtual channel, and the corresponding @@ -50,7 +51,7 @@ typedef struct guac_rdp_svc { /** * The name of the RDP channel in use, and the name to use for each pipe. */ - char name[GUAC_RDP_SVC_MAX_LENGTH+1]; + char name[GUAC_RDP_SVC_MAX_LENGTH]; /** * The output pipe, opened when the RDP server receives a connection to