From 865ba07a0fd4fbc7a8203482411aacca3bbfbb9f Mon Sep 17 00:00:00 2001

From: akallabeth <akallabeth@posteo.net>

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

@@ -34,6 +34,7 @@

 #include <stdlib.h>

 #include <string.h>

 #include <time.h>

+#include <assert.h>

 #include <winpr/wtypes.h>

 #include <winpr/crt.h>

@@ -61,10 +62,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