From 865ba07a0fd4fbc7a8203482411aacca3bbfbb9f Mon Sep 17 00:00:00 2001 From: akallabeth Date: Mon, 24 Oct 2022 10:41:55 +0200 Subject: [PATCH] Fixed path validation in drive channel Check that canonical path is a subpath of the shared directory (cherry picked from commit 844c94e6d0438fa7bd8ff8d5513c3f69c3018b85) --- channels/drive/client/drive_file.c | 106 ++++++++++++++++++----------- channels/drive/client/drive_file.h | 8 +-- channels/drive/client/drive_main.c | 8 +-- 3 files changed, 73 insertions(+), 49 deletions(-) diff --git a/channels/drive/client/drive_file.c b/channels/drive/client/drive_file.c index 305438593..1ea4ab9da 100644 --- a/channels/drive/client/drive_file.c +++ b/channels/drive/client/drive_file.c @@ -61,10 +61,14 @@ } while (0) #endif -static void drive_file_fix_path(WCHAR* path) +static BOOL drive_file_fix_path(WCHAR* path, size_t length) { size_t i; - size_t length = _wcslen(path); + + if ((length == 0) || (length > UINT32_MAX)) + return FALSE; + + assert(path); for (i = 0; i < length; i++) { @@ -75,58 +79,82 @@ static void drive_file_fix_path(WCHAR* path) #ifdef WIN32 if ((length == 3) && (path[1] == L':') && (path[2] == L'/')) - return; + return FALSE; #else if ((length == 1) && (path[0] == L'/')) - return; + return FALSE; #endif if ((length > 0) && (path[length - 1] == L'/')) path[length - 1] = L'\0'; + + return TRUE; } static WCHAR* drive_file_combine_fullpath(const WCHAR* base_path, const WCHAR* path, - size_t PathLength) + size_t PathWCharLength) { - WCHAR* fullpath; - size_t base_path_length; + BOOL ok = FALSE; + WCHAR* fullpath = NULL; + size_t length; - if (!base_path || (!path && (PathLength > 0))) - return NULL; + if (!base_path || (!path && (PathWCharLength > 0))) + goto fail; - base_path_length = _wcslen(base_path) * 2; - fullpath = (WCHAR*)calloc(1, base_path_length + PathLength + sizeof(WCHAR)); + const size_t base_path_length = _wcsnlen(base_path, MAX_PATH); + length = base_path_length + PathWCharLength + 1; + fullpath = (WCHAR*)calloc(length, sizeof(WCHAR)); if (!fullpath) + goto fail; + + CopyMemory(fullpath, base_path, base_path_length * sizeof(WCHAR)); + if (path) + CopyMemory(&fullpath[base_path_length], path, PathWCharLength * sizeof(WCHAR)); + + if (!drive_file_fix_path(fullpath, length)) + goto fail; + + /* Ensure the path does not contain sequences like '..' */ + const WCHAR dotdot[] = { '.', '.', '\0' }; + if (_wcsstr(&fullpath[base_path_length], dotdot)) { - WLog_ERR(TAG, "malloc failed!"); - return NULL; + char abuffer[MAX_PATH] = { 0 }; + ConvertFromUnicode(CP_UTF8, 0, &fullpath[base_path_length], -1, (char**)&abuffer, + ARRAYSIZE(abuffer) - 1, NULL, NULL); + + WLog_WARN(TAG, "[rdpdr] received invalid file path '%s' from server, aborting!", + &abuffer[base_path_length]); + goto fail; } - CopyMemory(fullpath, base_path, base_path_length); - if (path) - CopyMemory((char*)fullpath + base_path_length, path, PathLength); - drive_file_fix_path(fullpath); + ok = TRUE; +fail: + if (!ok) + { + free(fullpath); + fullpath = NULL; + } return fullpath; } static BOOL drive_file_remove_dir(const WCHAR* path) { - WIN32_FIND_DATAW findFileData; + WIN32_FIND_DATAW findFileData = { 0 }; BOOL ret = TRUE; - HANDLE dir; - WCHAR* fullpath; - WCHAR* path_slash; - size_t base_path_length; + HANDLE dir = INVALID_HANDLE_VALUE; + WCHAR* fullpath = NULL; + WCHAR* path_slash = NULL; + size_t base_path_length = 0; if (!path) return FALSE; - base_path_length = _wcslen(path) * 2; - path_slash = (WCHAR*)calloc(1, base_path_length + sizeof(WCHAR) * 3); + base_path_length = _wcslen(path); + path_slash = (WCHAR*)calloc(base_path_length + 3, sizeof(WCHAR)); if (!path_slash) { @@ -134,12 +162,11 @@ static BOOL drive_file_remove_dir(const WCHAR* path) return FALSE; } - CopyMemory(path_slash, path, base_path_length); - path_slash[base_path_length / 2] = L'/'; - path_slash[base_path_length / 2 + 1] = L'*'; + CopyMemory(path_slash, path, base_path_length * sizeof(WCHAR)); + path_slash[base_path_length] = L'/'; + path_slash[base_path_length + 1] = L'*'; DEBUG_WSTR("Search in %s", path_slash); dir = FindFirstFileW(path_slash, &findFileData); - path_slash[base_path_length / 2 + 1] = 0; if (dir == INVALID_HANDLE_VALUE) { @@ -149,7 +176,7 @@ static BOOL drive_file_remove_dir(const WCHAR* path) do { - size_t len = _wcslen(findFileData.cFileName); + const size_t len = _wcsnlen(findFileData.cFileName, ARRAYSIZE(findFileData.cFileName)); if ((len == 1 && findFileData.cFileName[0] == L'.') || (len == 2 && findFileData.cFileName[0] == L'.' && findFileData.cFileName[1] == L'.')) @@ -157,7 +184,7 @@ static BOOL drive_file_remove_dir(const WCHAR* path) continue; } - fullpath = drive_file_combine_fullpath(path_slash, findFileData.cFileName, len * 2); + fullpath = drive_file_combine_fullpath(path_slash, findFileData.cFileName, len); DEBUG_WSTR("Delete %s", fullpath); if (findFileData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) @@ -333,13 +360,13 @@ static BOOL drive_file_init(DRIVE_FILE* file) return file->file_handle != INVALID_HANDLE_VALUE; } -DRIVE_FILE* drive_file_new(const WCHAR* base_path, const WCHAR* path, UINT32 PathLength, UINT32 id, - UINT32 DesiredAccess, UINT32 CreateDisposition, UINT32 CreateOptions, - UINT32 FileAttributes, UINT32 SharedAccess) +DRIVE_FILE* drive_file_new(const WCHAR* base_path, const WCHAR* path, UINT32 PathWCharLength, + UINT32 id, UINT32 DesiredAccess, UINT32 CreateDisposition, + UINT32 CreateOptions, UINT32 FileAttributes, UINT32 SharedAccess) { DRIVE_FILE* file; - if (!base_path || (!path && (PathLength > 0))) + if (!base_path || (!path && (PathWCharLength > 0))) return NULL; file = (DRIVE_FILE*)calloc(1, sizeof(DRIVE_FILE)); @@ -359,7 +386,7 @@ DRIVE_FILE* drive_file_new(const WCHAR* base_path, const WCHAR* path, UINT32 Pat file->CreateDisposition = CreateDisposition; file->CreateOptions = CreateOptions; file->SharedAccess = SharedAccess; - drive_file_set_fullpath(file, drive_file_combine_fullpath(base_path, path, PathLength)); + drive_file_set_fullpath(file, drive_file_combine_fullpath(base_path, path, PathWCharLength)); if (!drive_file_init(file)) { @@ -714,13 +741,10 @@ BOOL drive_file_set_information(DRIVE_FILE* file, UINT32 FsInformationClass, UIN return FALSE; fullpath = drive_file_combine_fullpath(file->basepath, (WCHAR*)Stream_Pointer(input), - FileNameLength); + FileNameLength / sizeof(WCHAR)); if (!fullpath) - { - WLog_ERR(TAG, "drive_file_combine_fullpath failed!"); return FALSE; - } #ifdef _WIN32 @@ -759,7 +783,7 @@ BOOL drive_file_set_information(DRIVE_FILE* file, UINT32 FsInformationClass, UIN } BOOL drive_file_query_directory(DRIVE_FILE* file, UINT32 FsInformationClass, BYTE InitialQuery, - const WCHAR* path, UINT32 PathLength, wStream* output) + const WCHAR* path, UINT32 PathWCharLength, wStream* output) { size_t length; WCHAR* ent_path; @@ -773,7 +797,7 @@ BOOL drive_file_query_directory(DRIVE_FILE* file, UINT32 FsInformationClass, BYT if (file->find_handle != INVALID_HANDLE_VALUE) FindClose(file->find_handle); - ent_path = drive_file_combine_fullpath(file->basepath, path, PathLength); + ent_path = drive_file_combine_fullpath(file->basepath, path, PathWCharLength); /* open new search handle and retrieve the first entry */ file->find_handle = FindFirstFileW(ent_path, &file->find_data); free(ent_path); diff --git a/channels/drive/client/drive_file.h b/channels/drive/client/drive_file.h index ed789d6f0..6d3bd7045 100644 --- a/channels/drive/client/drive_file.h +++ b/channels/drive/client/drive_file.h @@ -51,9 +51,9 @@ struct _DRIVE_FILE UINT32 CreateOptions; }; -DRIVE_FILE* drive_file_new(const WCHAR* base_path, const WCHAR* path, UINT32 PathLength, UINT32 id, - UINT32 DesiredAccess, UINT32 CreateDisposition, UINT32 CreateOptions, - UINT32 FileAttributes, UINT32 SharedAccess); +DRIVE_FILE* drive_file_new(const WCHAR* base_path, const WCHAR* path, UINT32 PathWCharLength, + UINT32 id, UINT32 DesiredAccess, UINT32 CreateDisposition, + UINT32 CreateOptions, UINT32 FileAttributes, UINT32 SharedAccess); BOOL drive_file_free(DRIVE_FILE* file); BOOL drive_file_open(DRIVE_FILE* file); @@ -64,6 +64,6 @@ BOOL drive_file_query_information(DRIVE_FILE* file, UINT32 FsInformationClass, w BOOL drive_file_set_information(DRIVE_FILE* file, UINT32 FsInformationClass, UINT32 Length, wStream* input); BOOL drive_file_query_directory(DRIVE_FILE* file, UINT32 FsInformationClass, BYTE InitialQuery, - const WCHAR* path, UINT32 PathLength, wStream* output); + const WCHAR* path, UINT32 PathWCharLength, wStream* output); #endif /* FREERDP_CHANNEL_DRIVE_FILE_H */ diff --git a/channels/drive/client/drive_main.c b/channels/drive/client/drive_main.c index 1b5422522..d3776381c 100644 --- a/channels/drive/client/drive_main.c +++ b/channels/drive/client/drive_main.c @@ -184,8 +184,8 @@ static UINT drive_process_irp_create(DRIVE_DEVICE* drive, IRP* irp) path = (const WCHAR*)Stream_Pointer(irp->input); FileId = irp->devman->id_sequence++; - file = drive_file_new(drive->path, path, PathLength, FileId, DesiredAccess, CreateDisposition, - CreateOptions, FileAttributes, SharedAccess); + file = drive_file_new(drive->path, path, PathLength / sizeof(WCHAR), FileId, DesiredAccess, + CreateDisposition, CreateOptions, FileAttributes, SharedAccess); if (!file) { @@ -636,8 +636,8 @@ static UINT drive_process_irp_query_directory(DRIVE_DEVICE* drive, IRP* irp) irp->IoStatus = STATUS_UNSUCCESSFUL; Stream_Write_UINT32(irp->output, 0); /* Length */ } - else if (!drive_file_query_directory(file, FsInformationClass, InitialQuery, path, PathLength, - irp->output)) + else if (!drive_file_query_directory(file, FsInformationClass, InitialQuery, path, + PathLength / sizeof(WCHAR), irp->output)) { irp->IoStatus = drive_map_windows_err(GetLastError()); } -- 2.37.1