From ef4b29e5b3d8999538b04cc9a882bb9769e5e7a6 Mon Sep 17 00:00:00 2001 From: Norbert Federa Date: Thu, 3 Mar 2016 16:21:12 +0100 Subject: [PATCH] ConvertFromUnicode fixes and misc hardening - Added missing ConvertFromUnicode checks - If ConvertToUnicode allocates memory, guarantee the null termination similar to ConvertFromUnicode's implementation - Fixed some TestUnicodeConversion.c CTest return values - Added some CTests for ConvertFromUnicode and ConvertToUnicode - Misc code and protocol hardening fixes in the surrounding code regions that have been touched --- channels/cliprdr/client/cliprdr_format.c | 32 ++- channels/cliprdr/server/cliprdr_main.c | 39 ++- channels/echo/server/echo_main.c | 21 +- channels/rdpdr/server/rdpdr_main.c | 51 +++- channels/smartcard/client/smartcard_pack.c | 14 +- client/Windows/wf_rail.c | 41 +++- client/X11/xf_rail.c | 37 ++- libfreerdp/core/gcc.c | 16 +- libfreerdp/core/info.c | 308 +++++++++++++++++++++--- libfreerdp/core/redirection.c | 109 +++++++-- libfreerdp/core/window.c | 29 ++- server/shadow/Win/win_wds.c | 20 +- winpr/libwinpr/crt/test/TestUnicodeConversion.c | 217 ++++++++++++++++- winpr/libwinpr/crt/unicode.c | 35 ++- winpr/libwinpr/file/generic.c | 4 +- 15 files changed, 851 insertions(+), 122 deletions(-) diff --git a/channels/cliprdr/client/cliprdr_format.c b/channels/cliprdr/client/cliprdr_format.c index e686a7c..a7afae5 100644 --- a/channels/cliprdr/client/cliprdr_format.c +++ b/channels/cliprdr/client/cliprdr_format.c @@ -102,16 +102,25 @@ UINT cliprdr_process_format_list(cliprdrPlugin* cliprdr, wStream* s, UINT32 data formats[index].formatName = NULL; + /* According to MS-RDPECLIP 2.2.3.1.1.1 formatName is "a 32-byte block containing + * the *null-terminated* name assigned to the Clipboard Format: (32 ASCII 8 characters + * or 16 Unicode characters)" + * However, both Windows RDSH and mstsc violate this specs as seen in the following + * example of a transferred short format name string: [R.i.c.h. .T.e.x.t. .F.o.r.m.a.t.] + * These are 16 unicode charaters - *without* terminating null ! + */ + if (asciiNames) { szFormatName = (char*) Stream_Pointer(s); if (szFormatName[0]) { + /* ensure null termination */ formats[index].formatName = (char*) malloc(32 + 1); if (!formats[index].formatName) { - WLog_ERR(TAG, "calloc failed!"); + WLog_ERR(TAG, "malloc failed!"); error = CHANNEL_RC_NO_MEMORY; goto error_out; } @@ -125,8 +134,16 @@ UINT cliprdr_process_format_list(cliprdrPlugin* cliprdr, wStream* s, UINT32 data if (wszFormatName[0]) { - ConvertFromUnicode(CP_UTF8, 0, wszFormatName, - 16, &(formats[index].formatName), 0, NULL, NULL); + /* ConvertFromUnicode always returns a null-terminated + * string on success, even if the source string isn't. + */ + if (ConvertFromUnicode(CP_UTF8, 0, wszFormatName, 16, + &(formats[index].formatName), 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert short clipboard format name"); + error = ERROR_INTERNAL_ERROR; + goto error_out; + } } } @@ -185,8 +202,13 @@ UINT cliprdr_process_format_list(cliprdrPlugin* cliprdr, wStream* s, UINT32 data if (formatNameLength) { - ConvertFromUnicode(CP_UTF8, 0, wszFormatName, - -1, &(formats[index].formatName), 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, wszFormatName, -1, + &(formats[index].formatName), 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert long clipboard format name"); + error = ERROR_INTERNAL_ERROR; + goto error_out; + } } Stream_Seek(s, (formatNameLength + 1) * 2); diff --git a/channels/cliprdr/server/cliprdr_main.c b/channels/cliprdr/server/cliprdr_main.c index 24a34c5..ed98170 100644 --- a/channels/cliprdr/server/cliprdr_main.c +++ b/channels/cliprdr/server/cliprdr_main.c @@ -597,8 +597,12 @@ static UINT cliprdr_server_receive_temporary_directory(CliprdrServerContext* con free(cliprdr->temporaryDirectory); cliprdr->temporaryDirectory = NULL; - ConvertFromUnicode(CP_UTF8, 0, wszTempDir, -1, - &(cliprdr->temporaryDirectory), 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, wszTempDir, -1, + &(cliprdr->temporaryDirectory), 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert temporary directory name"); + return ERROR_INVALID_DATA; + } length = strlen(cliprdr->temporaryDirectory); @@ -680,12 +684,21 @@ static UINT cliprdr_server_receive_format_list(CliprdrServerContext* context, wS formats[index].formatName = NULL; + /* According to MS-RDPECLIP 2.2.3.1.1.1 formatName is "a 32-byte block containing + * the *null-terminated* name assigned to the Clipboard Format: (32 ASCII 8 characters + * or 16 Unicode characters)" + * However, both Windows RDSH and mstsc violate this specs as seen in the following + * example of a transferred short format name string: [R.i.c.h. .T.e.x.t. .F.o.r.m.a.t.] + * These are 16 unicode charaters - *without* terminating null ! + */ + if (asciiNames) { szFormatName = (char*) Stream_Pointer(s); if (szFormatName[0]) { + /* ensure null termination */ formats[index].formatName = (char*) malloc(32 + 1); CopyMemory(formats[index].formatName, szFormatName, 32); formats[index].formatName[32] = '\0'; @@ -697,8 +710,16 @@ static UINT cliprdr_server_receive_format_list(CliprdrServerContext* context, wS if (wszFormatName[0]) { - ConvertFromUnicode(CP_UTF8, 0, wszFormatName, - 16, &(formats[index].formatName), 0, NULL, NULL); + /* ConvertFromUnicode always returns a null-terminated + * string on success, even if the source string isn't. + */ + if (ConvertFromUnicode(CP_UTF8, 0, wszFormatName, 16, + &(formats[index].formatName), 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert short clipboard format name"); + error = ERROR_INVALID_DATA; + goto out; + } } } @@ -757,8 +778,13 @@ static UINT cliprdr_server_receive_format_list(CliprdrServerContext* context, wS if (formatNameLength) { - ConvertFromUnicode(CP_UTF8, 0, wszFormatName, - -1, &(formats[index].formatName), 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, wszFormatName, -1, + &(formats[index].formatName), 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert long clipboard format name"); + error = ERROR_INVALID_DATA; + goto out; + } } Stream_Seek(s, (formatNameLength + 1) * 2); @@ -775,6 +801,7 @@ static UINT cliprdr_server_receive_format_list(CliprdrServerContext* context, wS if (error) WLog_ERR(TAG, "ClientFormatList failed with error %lu!", error); +out: for (index = 0; index < formatList.numFormats; index++) { free(formatList.formats[index].formatName); diff --git a/channels/echo/server/echo_main.c b/channels/echo/server/echo_main.c index 58cc812..8c7f921 100644 --- a/channels/echo/server/echo_main.c +++ b/channels/echo/server/echo_main.c @@ -116,12 +116,15 @@ static void* echo_server_thread_func(void* arg) DWORD BytesReturned = 0; echo_server* echo = (echo_server*) arg; UINT error; - DWORD status; + DWORD status; if ((error = echo_server_open_channel(echo))) { - IFCALLRET(echo->context.OpenResult, error, &echo->context, ECHO_SERVER_OPEN_RESULT_NOTSUPPORTED); + UINT error2 = 0; WLog_ERR(TAG, "echo_server_open_channel failed with error %lu!", error); + IFCALLRET(echo->context.OpenResult, error2, &echo->context, ECHO_SERVER_OPEN_RESULT_NOTSUPPORTED); + if (error2) + WLog_ERR(TAG, "echo server's OpenResult callback failed with error %lu", error2); goto out; } @@ -145,14 +148,14 @@ static void* echo_server_thread_func(void* arg) while (1) { - status = WaitForMultipleObjects(nCount, events, FALSE, 100); + status = WaitForMultipleObjects(nCount, events, FALSE, 100); - if (status == WAIT_FAILED) - { - error = GetLastError(); - WLog_ERR(TAG, "WaitForMultipleObjects failed with error %lu", error); - break; - } + if (status == WAIT_FAILED) + { + error = GetLastError(); + WLog_ERR(TAG, "WaitForMultipleObjects failed with error %lu", error); + break; + } if (status == WAIT_OBJECT_0) { diff --git a/channels/rdpdr/server/rdpdr_main.c b/channels/rdpdr/server/rdpdr_main.c index 074d3e4..b29182f 100644 --- a/channels/rdpdr/server/rdpdr_main.c +++ b/channels/rdpdr/server/rdpdr_main.c @@ -152,9 +152,10 @@ static UINT rdpdr_server_receive_client_name_request(RdpdrServerContext* context Stream_Seek_UINT32(s); /* CodePage (4 bytes), MUST be set to zero */ Stream_Read_UINT32(s, ComputerNameLen); /* ComputerNameLen (4 bytes) */ - if (Stream_GetRemainingLength(s) < ComputerNameLen) + + if (UnicodeFlag > 1) /* must be 0x00000000 or 0x00000001 */ { - WLog_ERR(TAG, "not enough data in stream!"); + WLog_ERR(TAG, "invalid UnicodeFlag value: 0x%08X", UnicodeFlag); return ERROR_INVALID_DATA; } @@ -164,6 +165,39 @@ static UINT rdpdr_server_receive_client_name_request(RdpdrServerContext* context * not in characters, including the NULL terminator! */ + if (UnicodeFlag) + { + if ((ComputerNameLen % 2) || ComputerNameLen > 512 || ComputerNameLen < 2) + { + WLog_ERR(TAG, "invalid unicode computer name length: %u", ComputerNameLen); + return ERROR_INVALID_DATA; + } + } + else + { + if (ComputerNameLen > 256 || ComputerNameLen < 1) + { + WLog_ERR(TAG, "invalid ascii computer name length: %u", ComputerNameLen); + return ERROR_INVALID_DATA; + } + } + + if (Stream_GetRemainingLength(s) < ComputerNameLen) + { + WLog_ERR(TAG, "not enough data in stream!"); + return ERROR_INVALID_DATA; + } + + /* ComputerName must be null terminated, check if it really is */ + + if (Stream_Pointer(s)[ComputerNameLen-1] || + (UnicodeFlag && Stream_Pointer(s)[ComputerNameLen-2])) + { + WLog_ERR(TAG, "computer name must be null terminated"); + return ERROR_INVALID_DATA; + } + + if (context->priv->ClientComputerName) { free(context->priv->ClientComputerName); @@ -172,12 +206,21 @@ static UINT rdpdr_server_receive_client_name_request(RdpdrServerContext* context if (UnicodeFlag) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), - -1, &(context->priv->ClientComputerName), 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), -1, + &(context->priv->ClientComputerName), 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert client computer name"); + return ERROR_INVALID_DATA; + } } else { context->priv->ClientComputerName = _strdup((char*) Stream_Pointer(s)); + if (!context->priv->ClientComputerName) + { + WLog_ERR(TAG, "failed to duplicate client computer name"); + return CHANNEL_RC_NO_MEMORY; + } } Stream_Seek(s, ComputerNameLen); diff --git a/channels/smartcard/client/smartcard_pack.c b/channels/smartcard/client/smartcard_pack.c index 421cc61..766648b 100644 --- a/channels/smartcard/client/smartcard_pack.c +++ b/channels/smartcard/client/smartcard_pack.c @@ -707,7 +707,12 @@ void smartcard_trace_list_readers_return(SMARTCARD_DEVICE* smartcard, ListReader if (unicode) { length = ret->cBytes / 2; - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) ret->msz, (int)length, &mszA, 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) ret->msz, (int)length, + &mszA, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "ConvertFromUnicode failed"); + return; + } } else { @@ -1767,7 +1772,12 @@ void smartcard_trace_status_return(SMARTCARD_DEVICE* smartcard, Status_Return* r if (unicode) { length = ret->cBytes / 2; - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) ret->mszReaderNames, (int)length, &mszReaderNamesA, 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) ret->mszReaderNames, (int)length, + &mszReaderNamesA, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "ConvertFromUnicode failed"); + return; + } } else { diff --git a/client/Windows/wf_rail.c b/client/Windows/wf_rail.c index c00a465..9ee65ce 100644 --- a/client/Windows/wf_rail.c +++ b/client/Windows/wf_rail.c @@ -456,14 +456,33 @@ static BOOL wf_rail_window_common(rdpContext* context, WINDOW_ORDER_INFO* orderI { char* title = NULL; - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, - windowState->titleInfo.length / 2, &title, 0, NULL, NULL); + if (windowState->titleInfo.length == 0) + { + if (!(title = _strdup(""))) + { + WLog_ERR(TAG, "failed to duplicate empty window title string"); + /* error handled below */ + } + } + else if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, + windowState->titleInfo.length / 2, &title, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert window title"); + /* error handled below */ + } railWindow->title = title; } else { - railWindow->title = _strdup("RdpRailWindow"); + if (!(railWindow->title = _strdup("RdpRailWindow"))) + WLog_ERR(TAG, "failed to duplicate default window title string"); + } + + if (!railWindow->title) + { + free(railWindow); + return FALSE; } ConvertToUnicode(CP_UTF8, 0, railWindow->title, -1, &titleW, 0); @@ -569,8 +588,20 @@ static BOOL wf_rail_window_common(rdpContext* context, WINDOW_ORDER_INFO* orderI char* title = NULL; WCHAR* titleW = NULL; - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, - windowState->titleInfo.length / 2, &title, 0, NULL, NULL); + if (windowState->titleInfo.length == 0) + { + if (!(title = _strdup(""))) + { + WLog_ERR(TAG, "failed to duplicate empty window title string"); + return FALSE; + } + } + else if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, + windowState->titleInfo.length / 2, &title, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert window title"); + return FALSE; + } free(railWindow->title); railWindow->title = title; diff --git a/client/X11/xf_rail.c b/client/X11/xf_rail.c index 22a941c..6b49d56 100644 --- a/client/X11/xf_rail.c +++ b/client/X11/xf_rail.c @@ -290,15 +290,29 @@ static BOOL xf_rail_window_common(rdpContext* context, WINDOW_ORDER_INFO* orderI { char* title = NULL; - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, - windowState->titleInfo.length / 2, &title, 0, NULL, NULL); + if (windowState->titleInfo.length == 0) + { + if (!(title = _strdup(""))) + { + WLog_ERR(TAG, "failed to duplicate empty window title string"); + /* error handled below */ + } + } + else if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, + windowState->titleInfo.length / 2, &title, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert window title"); + /* error handled below */ + } appWindow->title = title; } else { - appWindow->title = _strdup("RdpRailWindow"); + if (!(appWindow->title = _strdup("RdpRailWindow"))) + WLog_ERR(TAG, "failed to duplicate default window title string"); } + if (!appWindow->title) { free(appWindow); @@ -365,9 +379,20 @@ static BOOL xf_rail_window_common(rdpContext* context, WINDOW_ORDER_INFO* orderI { char* title = NULL; - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, - windowState->titleInfo.length / 2, &title, 0, NULL, NULL); - + if (windowState->titleInfo.length == 0) + { + if (!(title = _strdup(""))) + { + WLog_ERR(TAG, "failed to duplicate empty window title string"); + return FALSE; + } + } + else if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) windowState->titleInfo.string, + windowState->titleInfo.length / 2, &title, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert window title"); + return FALSE; + } free(appWindow->title); appWindow->title = title; } diff --git a/libfreerdp/core/gcc.c b/libfreerdp/core/gcc.c index 7078a1d..e19e019 100644 --- a/libfreerdp/core/gcc.c +++ b/libfreerdp/core/gcc.c @@ -590,7 +590,12 @@ BOOL gcc_read_client_core_data(wStream* s, rdpMcs* mcs, UINT16 blockLength) Stream_Read_UINT32(s, settings->ClientBuild); /* ClientBuild (4 bytes) */ /* clientName (32 bytes, null-terminated unicode, truncated to 15 characters) */ - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), 32 / 2, &str, 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), 32 / 2, + &str, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert client host name"); + return FALSE; + } Stream_Seek(s, 32); free(settings->ClientHostname); settings->ClientHostname = str; @@ -644,10 +649,17 @@ BOOL gcc_read_client_core_data(wStream* s, rdpMcs* mcs, UINT16 blockLength) settings->EarlyCapabilityFlags = (UINT32) earlyCapabilityFlags; blockLength -= 2; + /* clientDigProductId (64 bytes): Contains a value that uniquely identifies the client */ + if (blockLength < 64) break; - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), 64 / 2, &str, 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), 64 / 2, + &str, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert the client product identifier"); + return FALSE; + } Stream_Seek(s, 64); /* clientDigProductId (64 bytes) */ free(settings->ClientProductId); settings->ClientProductId = str; diff --git a/libfreerdp/core/info.c b/libfreerdp/core/info.c index 25fe940..6a918c4 100644 --- a/libfreerdp/core/info.c +++ b/libfreerdp/core/info.c @@ -199,6 +199,7 @@ BOOL rdp_read_extended_info_packet(rdpRdp* rdp, wStream* s) UINT16 cbClientDir; UINT16 cbAutoReconnectLen; rdpSettings* settings = rdp->settings; + WCHAR* wstr; if (Stream_GetRemainingLength(s) < 4) return FALSE; @@ -206,6 +207,17 @@ BOOL rdp_read_extended_info_packet(rdpRdp* rdp, wStream* s) Stream_Read_UINT16(s, clientAddressFamily); /* clientAddressFamily (2 bytes) */ Stream_Read_UINT16(s, cbClientAddress); /* cbClientAddress (2 bytes) */ + /* cbClientAddress is the size in bytes of the character data in the clientAddress field. + * This size includes the length of the mandatory null terminator. + * The maximum allowed value is 80 bytes + */ + + if ((cbClientAddress % 2) || cbClientAddress < 2 || cbClientAddress > 80) + { + WLog_ERR(TAG, "protocol error: invalid cbClientAddress value: %u", cbClientAddress); + return FALSE; + } + settings->IPv6Enabled = (clientAddressFamily == ADDRESS_FAMILY_INET6 ? TRUE : FALSE); if (Stream_GetRemainingLength(s) < cbClientAddress) @@ -217,7 +229,17 @@ BOOL rdp_read_extended_info_packet(rdpRdp* rdp, wStream* s) settings->ClientAddress = NULL; } - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbClientAddress / 2, &settings->ClientAddress, 0, NULL, NULL); + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbClientAddress / 2 - 1]) + { + WLog_ERR(TAG, "protocol error: clientAddress must be null terminated"); + return FALSE; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &settings->ClientAddress, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert client address"); + return FALSE; + } Stream_Seek(s, cbClientAddress); if (Stream_GetRemainingLength(s) < 2) @@ -225,6 +247,17 @@ BOOL rdp_read_extended_info_packet(rdpRdp* rdp, wStream* s) Stream_Read_UINT16(s, cbClientDir); /* cbClientDir (2 bytes) */ + /* cbClientDir is the size in bytes of the character data in the clientDir field. + * This size includes the length of the mandatory null terminator. + * The maximum allowed value is 512 bytes + */ + + if ((cbClientDir % 2) || cbClientDir < 2 || cbClientDir > 512) + { + WLog_ERR(TAG, "protocol error: invalid cbClientDir value: %u", cbClientDir); + return FALSE; + } + if (Stream_GetRemainingLength(s) < cbClientDir) return FALSE; @@ -234,7 +267,17 @@ BOOL rdp_read_extended_info_packet(rdpRdp* rdp, wStream* s) settings->ClientDir = NULL; } - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbClientDir / 2, &settings->ClientDir, 0, NULL, NULL); + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbClientDir / 2 - 1]) + { + WLog_ERR(TAG, "protocol error: clientDir must be null terminated"); + return FALSE; + } + if (ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), -1, &settings->ClientDir, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert client directory"); + return FALSE; + } Stream_Seek(s, cbClientDir); if (!rdp_read_client_time_zone(s, settings)) @@ -337,6 +380,7 @@ BOOL rdp_read_info_packet(rdpRdp* rdp, wStream* s) UINT16 cbWorkingDir; UINT32 CompressionLevel; rdpSettings* settings = rdp->settings; + WCHAR* wstr; if (Stream_GetRemainingLength(s) < 18) return FALSE; @@ -357,6 +401,12 @@ BOOL rdp_read_info_packet(rdpRdp* rdp, wStream* s) settings->CompressionLevel = CompressionLevel; } + if (!(flags & INFO_UNICODE)) + { + WLog_ERR(TAG, "Client without INFO_UNICODE flag: this is currently not supported"); + return FALSE; + } + Stream_Read_UINT16(s, cbDomain); /* cbDomain (2 bytes) */ Stream_Read_UINT16(s, cbUserName); /* cbUserName (2 bytes) */ Stream_Read_UINT16(s, cbPassword); /* cbPassword (2 bytes) */ @@ -368,7 +418,26 @@ BOOL rdp_read_info_packet(rdpRdp* rdp, wStream* s) if (cbDomain > 0) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbDomain / 2, &settings->Domain, 0, NULL, NULL); + /* cbDomain is the size in bytes of the character data in the Domain field. + * This size excludes (!) the length of the mandatory null terminator. + * Maximum value including the mandatory null terminator: 512 + */ + if ((cbDomain % 2) || cbDomain > 512) + { + WLog_ERR(TAG, "protocol error: invalid cbDomain value: %u", cbDomain); + return FALSE; + } + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbDomain / 2]) + { + WLog_ERR(TAG, "protocol error: Domain must be null terminated"); + return FALSE; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &settings->Domain, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert Domain string"); + return FALSE; + } Stream_Seek(s, cbDomain); } Stream_Seek(s, 2); @@ -378,7 +447,26 @@ BOOL rdp_read_info_packet(rdpRdp* rdp, wStream* s) if (cbUserName > 0) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbUserName / 2, &settings->Username, 0, NULL, NULL); + /* cbUserName is the size in bytes of the character data in the UserName field. + * This size excludes (!) the length of the mandatory null terminator. + * Maximum value including the mandatory null terminator: 512 + */ + if ((cbUserName % 2) || cbUserName > 512) + { + WLog_ERR(TAG, "protocol error: invalid cbUserName value: %u", cbUserName); + return FALSE; + } + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbUserName / 2]) + { + WLog_ERR(TAG, "protocol error: UserName must be null terminated"); + return FALSE; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &settings->Username, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert UserName string"); + return FALSE; + } Stream_Seek(s, cbUserName); } Stream_Seek(s, 2); @@ -388,7 +476,26 @@ BOOL rdp_read_info_packet(rdpRdp* rdp, wStream* s) if (cbPassword > 0) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbPassword / 2, &settings->Password, 0, NULL, NULL); + /* cbPassword is the size in bytes of the character data in the Password field. + * This size excludes (!) the length of the mandatory null terminator. + * Maximum value including the mandatory null terminator: 512 + */ + if ((cbPassword % 2) || cbPassword > 512) + { + WLog_ERR(TAG, "protocol error: invalid cbPassword value: %u", cbPassword); + return FALSE; + } + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbPassword / 2]) + { + WLog_ERR(TAG, "protocol error: Password must be null terminated"); + return FALSE; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &settings->Password, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert Password string"); + return FALSE; + } Stream_Seek(s, cbPassword); } Stream_Seek(s, 2); @@ -398,7 +505,26 @@ BOOL rdp_read_info_packet(rdpRdp* rdp, wStream* s) if (cbAlternateShell > 0) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbAlternateShell / 2, &settings->AlternateShell, 0, NULL, NULL); + /* cbAlternateShell is the size in bytes of the character data in the AlternateShell field. + * This size excludes (!) the length of the mandatory null terminator. + * Maximum value including the mandatory null terminator: 512 + */ + if ((cbAlternateShell % 2) || cbAlternateShell > 512) + { + WLog_ERR(TAG, "protocol error: invalid cbAlternateShell value: %u", cbAlternateShell); + return FALSE; + } + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbAlternateShell / 2]) + { + WLog_ERR(TAG, "protocol error: AlternateShell must be null terminated"); + return FALSE; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &settings->AlternateShell, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert AlternateShell string"); + return FALSE; + } Stream_Seek(s, cbAlternateShell); } Stream_Seek(s, 2); @@ -408,7 +534,26 @@ BOOL rdp_read_info_packet(rdpRdp* rdp, wStream* s) if (cbWorkingDir > 0) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbWorkingDir / 2, &settings->ShellWorkingDirectory, 0, NULL, NULL); + /* cbWorkingDir is the size in bytes of the character data in the WorkingDir field. + * This size excludes (!) the length of the mandatory null terminator. + * Maximum value including the mandatory null terminator: 512 + */ + if ((cbWorkingDir % 2) || cbWorkingDir > 512) + { + WLog_ERR(TAG, "protocol error: invalid cbWorkingDir value: %u", cbWorkingDir); + return FALSE; + } + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbWorkingDir / 2]) + { + WLog_ERR(TAG, "protocol error: WorkingDir must be null terminated"); + return FALSE; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &settings->ShellWorkingDirectory, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "failed to convert AlternateShell string"); + return FALSE; + } Stream_Seek(s, cbWorkingDir); } Stream_Seek(s, 2); @@ -664,6 +809,7 @@ BOOL rdp_recv_logon_info_v1(rdpRdp* rdp, wStream* s, logon_info *info) { UINT32 cbDomain; UINT32 cbUserName; + WCHAR* wstr; ZeroMemory(info, sizeof(*info)); @@ -671,35 +817,63 @@ BOOL rdp_recv_logon_info_v1(rdpRdp* rdp, wStream* s, logon_info *info) return FALSE; Stream_Read_UINT32(s, cbDomain); /* cbDomain (4 bytes) */ - if (cbDomain > 52) - return FALSE; - if (cbDomain) + + /* cbDomain is the size of the Unicode character data (including the mandatory + * null terminator) in bytes present in the fixed-length (52 bytes) Domain field + */ + if ((cbDomain % 2) || cbDomain < 2 || cbDomain > 52) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbDomain, &info->domain, 0, NULL, FALSE); - if (!info->domain) - return FALSE; + WLog_ERR(TAG, "protocol error: invalid cbDomain value: %lu", cbDomain); + goto fail; + } + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbDomain / 2 - 1]) + { + WLog_ERR(TAG, "protocol error: Domain must be null terminated"); + goto fail; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &info->domain, 0, NULL, FALSE) < 1) + { + WLog_ERR(TAG, "failed to convert the Domain string"); + goto fail; } - Stream_Seek(s, 52); /* domain (52 bytes) */ + Stream_Read_UINT32(s, cbUserName); /* cbUserName (4 bytes) */ - if (cbUserName > 512) - goto error_username; - if (cbUserName) + + /* cbUserName is the size of the Unicode character data (including the mandatory + * null terminator) in bytes present in the fixed-length (512 bytes) UserName field. + */ + + if ((cbUserName % 2) || cbUserName < 2 || cbUserName > 512) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbUserName, &info->username, 0, NULL, FALSE); - if (!info->username) - goto error_username; + WLog_ERR(TAG, "protocol error: invalid cbUserName value: %lu", cbUserName); + goto fail; + } + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbUserName / 2 - 1]) + { + WLog_ERR(TAG, "protocol error: UserName must be null terminated"); + goto fail; + } + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &info->username, 0, NULL, FALSE) < 1) + { + WLog_ERR(TAG, "failed to convert the UserName string"); + goto fail; } Stream_Seek(s, 512); /* userName (512 bytes) */ Stream_Read_UINT32(s, info->sessionId); /* SessionId (4 bytes) */ - WLog_DBG(TAG, "LogonInfoV1: SessionId: 0x%04X", info->sessionId); + WLog_DBG(TAG, "LogonInfoV1: SessionId: 0x%04X UserName: [%s] Domain: [%s]", + info->sessionId, info->username, info->domain); return TRUE; -error_username: +fail: + free(info->username); + info->username = NULL; free(info->domain); info->domain = NULL; return FALSE; @@ -711,6 +885,7 @@ BOOL rdp_recv_logon_info_v2(rdpRdp* rdp, wStream* s, logon_info *info) UINT32 Size; UINT32 cbDomain; UINT32 cbUserName; + WCHAR* wstr; ZeroMemory(info, sizeof(*info)); @@ -724,32 +899,87 @@ BOOL rdp_recv_logon_info_v2(rdpRdp* rdp, wStream* s, logon_info *info) Stream_Read_UINT32(s, cbUserName); /* cbUserName (4 bytes) */ Stream_Seek(s, 558); /* pad (558 bytes) */ - if (Stream_GetRemainingLength(s) < (cbDomain + cbUserName)) - return FALSE; - if (cbDomain) + /* cbDomain is the size in bytes of the Unicode character data in the Domain field. + * The size of the mandatory null terminator is include in this value. + * Note: Since MS-RDPBCGR 2.2.10.1.1.2 does not mention any size limits we assume + * that the maximum value is 52 bytes, according to the fixed size of the + * Domain field in the Logon Info Version 1 (TS_LOGON_INFO) structure. + */ + + if ((cbDomain % 2) || cbDomain < 2 || cbDomain > 52) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbDomain, &info->domain, 0, NULL, FALSE); - if (!info->domain) - return FALSE; - Stream_Seek(s, cbDomain); /* domain */ + WLog_ERR(TAG, "protocol error: invalid cbDomain value: %lu", cbDomain); + goto fail; } - if (cbUserName) + if (Stream_GetRemainingLength(s) < (size_t) cbDomain) { - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), cbUserName, &info->username, 0, NULL, FALSE); - if (!info->username) - { - free(info->domain); - info->domain = NULL; - return FALSE; - } - Stream_Seek(s, cbUserName); /* userName */ + WLog_ERR(TAG, "insufficient remaining stream length"); + goto fail; + } + + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbDomain / 2 - 1]) + { + WLog_ERR(TAG, "protocol error: Domain field must be null terminated"); + goto fail; } - WLog_DBG(TAG, "LogonInfoV2: SessionId: 0x%04X", info->sessionId); + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &info->domain, 0, NULL, FALSE) < 1) + { + WLog_ERR(TAG, "failed to convert the Domain string"); + goto fail; + } + + Stream_Seek(s, cbDomain); /* domain */ + + + /* cbUserName is the size in bytes of the Unicode character data in the UserName field. + * The size of the mandatory null terminator is include in this value. + * Note: Since MS-RDPBCGR 2.2.10.1.1.2 does not mention any size limits we assume + * that the maximum value is 512 bytes, according to the fixed size of the + * Username field in the Logon Info Version 1 (TS_LOGON_INFO) structure. + */ + + if ((cbUserName % 2) || cbUserName < 2 || cbUserName > 512) + { + WLog_ERR(TAG, "protocol error: invalid cbUserName value: %lu", cbUserName); + goto fail; + } + + if (Stream_GetRemainingLength(s) < (size_t) cbUserName) + { + WLog_ERR(TAG, "insufficient remaining stream length"); + goto fail; + } + + wstr = (WCHAR*) Stream_Pointer(s); + if (wstr[cbUserName / 2 - 1]) + { + WLog_ERR(TAG, "protocol error: UserName field must be null terminated"); + goto fail; + } + + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, &info->username, 0, NULL, FALSE) < 1) + { + WLog_ERR(TAG, "failed to convert the Domain string"); + goto fail; + } + + Stream_Seek(s, cbUserName); /* userName */ + + WLog_DBG(TAG, "LogonInfoV2: SessionId: 0x%04X UserName: [%s] Domain: [%s]", + info->sessionId, info->username, info->domain); return TRUE; + +fail: + free(info->username); + info->username = NULL; + free(info->domain); + info->domain = NULL; + return FALSE; } BOOL rdp_recv_logon_plain_notify(rdpRdp* rdp, wStream* s) diff --git a/libfreerdp/core/redirection.c b/libfreerdp/core/redirection.c index 6d31257..0922902 100644 --- a/libfreerdp/core/redirection.c +++ b/libfreerdp/core/redirection.c @@ -30,7 +30,7 @@ #define TAG FREERDP_TAG("core.redirection") -void rdp_print_redirection_flags(UINT32 flags) +static void rdp_print_redirection_flags(UINT32 flags) { WLog_DBG(TAG, "redirectionFlags = {"); @@ -76,9 +76,10 @@ void rdp_print_redirection_flags(UINT32 flags) WLog_DBG(TAG, "}"); } -BOOL rdp_redirection_read_string(wStream* s, char** str) +static BOOL rdp_redirection_read_unicode_string(wStream* s, char** str, size_t maxLength) { UINT32 length; + WCHAR* wstr = NULL; if (Stream_GetRemainingLength(s) < 4) { @@ -88,13 +89,31 @@ BOOL rdp_redirection_read_string(wStream* s, char** str) Stream_Read_UINT32(s, length); + if ((length % 2) || length < 2 || length > maxLength) + { + WLog_ERR(TAG, "rdp_redirection_read_string failure: invalid unicode string length: %lu", length); + return FALSE; + } + if (Stream_GetRemainingLength(s) < length) { - WLog_ERR(TAG, "rdp_redirection_read_string failure: incorrect length %d", length); + WLog_ERR(TAG, "rdp_redirection_read_string failure: insufficient stream length (%lu bytes required)", length); + return FALSE; + } + + wstr = (WCHAR*) Stream_Pointer(s); + + if (wstr[length / 2 - 1]) + { + WLog_ERR(TAG, "rdp_redirection_read_string failure: unterminated unicode string"); return FALSE; } - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) Stream_Pointer(s), length / 2, str, 0, NULL, NULL); + if (ConvertFromUnicode(CP_UTF8, 0, wstr, -1, str, 0, NULL, NULL) < 1) + { + WLog_ERR(TAG, "rdp_redirection_read_string failure: string conversion failed"); + return FALSE; + } Stream_Seek(s, length); return TRUE; } @@ -173,7 +192,10 @@ int rdp_redirection_apply_settings(rdpRdp* rdp) /* Password may be a cookie without a null terminator */ free(settings->RedirectionPassword); settings->RedirectionPasswordLength = redirection->PasswordLength; - settings->RedirectionPassword = (BYTE*) malloc(settings->RedirectionPasswordLength); + /* For security reasons we'll allocate an additional zero WCHAR at the + * end of the buffer that is not included in RedirectionPasswordLength + */ + settings->RedirectionPassword = (BYTE*) calloc(1, settings->RedirectionPasswordLength + sizeof(WCHAR)); if (!settings->RedirectionPassword) return -1; CopyMemory(settings->RedirectionPassword, redirection->Password, settings->RedirectionPasswordLength); @@ -219,7 +241,7 @@ int rdp_redirection_apply_settings(rdpRdp* rdp) return 0; } -BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) +static BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) { UINT16 flags; UINT16 length; @@ -238,14 +260,33 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) rdp_print_redirection_flags(redirection->flags); + /* Although MS-RDPBCGR does not mention any length constraints limits for the + * variable length null-terminated unicode strings in the RDP_SERVER_REDIRECTION_PACKET + * structure we will use the following limits in bytes including the null terminator: + * + * TargetNetAddress: 80 bytes + * UserName: 512 bytes + * Domain: 52 bytes + * Password(Cookie): 512 bytes + * TargetFQDN: 512 bytes + * TargetNetBiosName: 32 bytes + */ + if (redirection->flags & LB_TARGET_NET_ADDRESS) { - if (!rdp_redirection_read_string(s, &(redirection->TargetNetAddress))) + if (!rdp_redirection_read_unicode_string(s, &(redirection->TargetNetAddress), 80)) return -1; } if (redirection->flags & LB_LOAD_BALANCE_INFO) { + /* See [MSFT-SDLBTS] (a.k.a. TS_Session_Directory.doc) + * load balance info example data: + * 0000 43 6f 6f 6b 69 65 3a 20 6d 73 74 73 3d 32 31 33 Cookie: msts=213 + * 0010 34 30 32 36 34 33 32 2e 31 35 36 32 39 2e 30 30 4026432.15629.00 + * 0020 30 30 0d 0a 00.. + */ + if (Stream_GetRemainingLength(s) < 4) return -1; @@ -265,7 +306,7 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) if (redirection->flags & LB_USERNAME) { - if (!rdp_redirection_read_string(s, &(redirection->Username))) + if (!rdp_redirection_read_unicode_string(s, &(redirection->Username), 512)) return -1; WLog_DBG(TAG, "Username: %s", redirection->Username); @@ -273,7 +314,7 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) if (redirection->flags & LB_DOMAIN) { - if (!rdp_redirection_read_string(s, &(redirection->Domain))) + if (!rdp_redirection_read_unicode_string(s, &(redirection->Domain), 52)) return FALSE; WLog_DBG(TAG, "Domain: %s", redirection->Domain); @@ -281,12 +322,45 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) if (redirection->flags & LB_PASSWORD) { - /* Note: length (hopefully) includes double zero termination */ + /* Note: Password is a variable-length array of bytes containing the + * password used by the user in Unicode format, including a null-terminator + * or (!) or a cookie value that MUST be passed to the target server on + * successful connection. + * Since the format of the password cookie (probably some salted hash) is + * currently unknown we'll treat it as opaque data. All cookies seen so far + * are 120 bytes including \0\0 termination. + * Here is an observed example of a redirection password cookie: + * + * 0000 02 00 00 80 44 53 48 4c 60 ab 69 2f 07 d6 9e 2d ....DSHL`.i/...- + * 0010 f0 3a 97 3b a9 c5 ec 7e 66 bd b3 84 6c b1 ef b9 .:.;...~f...l... + * 0020 b6 82 4e cc 3a df 64 b7 7b 25 04 54 c2 58 98 f8 ..N.:.d.{%.T.X.. + * 0030 97 87 d4 93 c7 c1 e1 5b c2 85 f8 22 49 1f 81 88 .......[..."I... + * 0040 43 44 83 f6 9a 72 40 24 dc 4d 43 cb d9 92 3c 8f CD...r@$.MC...<. + * 0050 3a 37 5c 77 13 a0 72 3c 72 08 64 2a 29 fb dc eb :7\w..rPasswordLength); - redirection->Password = (BYTE*) malloc(redirection->PasswordLength); + + /* [MS-RDPBCGR] specifies 512 bytes as the upper limit for the password length + * including the null terminatior(s). This should also be enough for the unknown + * password cookie format (see previous comment). + */ + + if (Stream_GetRemainingLength(s) < redirection->PasswordLength) + return -1; + + if (redirection->PasswordLength > 512) + return -1; + + redirection->Password = (BYTE*) calloc(1, redirection->PasswordLength + sizeof(WCHAR)); if (!redirection->Password) return -1; Stream_Read(s, redirection->Password, redirection->PasswordLength); @@ -297,7 +371,7 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) if (redirection->flags & LB_TARGET_FQDN) { - if (!rdp_redirection_read_string(s, &(redirection->TargetFQDN))) + if (!rdp_redirection_read_unicode_string(s, &(redirection->TargetFQDN), 512)) return -1; WLog_DBG(TAG, "TargetFQDN: %s", redirection->TargetFQDN); @@ -305,7 +379,7 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) if (redirection->flags & LB_TARGET_NETBIOS_NAME) { - if (!rdp_redirection_read_string(s, &(redirection->TargetNetBiosName))) + if (!rdp_redirection_read_unicode_string(s, &(redirection->TargetNetBiosName), 32)) return -1; WLog_DBG(TAG, "TargetNetBiosName: %s", redirection->TargetNetBiosName); @@ -352,7 +426,7 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) for (i = 0; i < (int) count; i++) { - if (!rdp_redirection_read_string(s, &(redirection->TargetNetAddresses[i]))) + if (!rdp_redirection_read_unicode_string(s, &(redirection->TargetNetAddresses[i]), 80)) return FALSE; WLog_DBG(TAG, "TargetNetAddresses[%d]: %s", i, redirection->TargetNetAddresses[i]); @@ -371,13 +445,6 @@ BOOL rdp_recv_server_redirection_pdu(rdpRdp* rdp, wStream* s) return 1; } -int rdp_recv_redirection_packet(rdpRdp* rdp, wStream* s) -{ - int status = 0; - status = rdp_recv_server_redirection_pdu(rdp, s); - return status; -} - int rdp_recv_enhanced_security_redirection_packet(rdpRdp* rdp, wStream* s) { int status = 0; diff --git a/libfreerdp/core/window.c b/libfreerdp/core/window.c index e31fe45..e197b76 100644 --- a/libfreerdp/core/window.c +++ b/libfreerdp/core/window.c @@ -371,6 +371,7 @@ BOOL update_recv_window_info_order(rdpUpdate* update, wStream* s, WINDOW_ORDER_I { rdpContext* context = update->context; rdpWindowUpdate* window = update->window; + BOOL result = TRUE; if (Stream_GetRemainingLength(s) < 4) return FALSE; @@ -382,20 +383,20 @@ BOOL update_recv_window_info_order(rdpUpdate* update, wStream* s, WINDOW_ORDER_I if (!update_read_window_icon_order(s, orderInfo, &window->window_icon)) return FALSE; WLog_Print(update->log, WLOG_DEBUG, "WindowIcon"); - IFCALL(window->WindowIcon, context, orderInfo, &window->window_icon); + IFCALLRET(window->WindowIcon, result, context, orderInfo, &window->window_icon); } else if (orderInfo->fieldFlags & WINDOW_ORDER_CACHED_ICON) { if (!update_read_window_cached_icon_order(s, orderInfo, &window->window_cached_icon)) return FALSE; WLog_Print(update->log, WLOG_DEBUG, "WindowCachedIcon"); - IFCALL(window->WindowCachedIcon, context, orderInfo, &window->window_cached_icon); + IFCALLRET(window->WindowCachedIcon, result, context, orderInfo, &window->window_cached_icon); } else if (orderInfo->fieldFlags & WINDOW_ORDER_STATE_DELETED) { update_read_window_delete_order(s, orderInfo); WLog_Print(update->log, WLOG_DEBUG, "WindowDelete"); - IFCALL(window->WindowDelete, context, orderInfo); + IFCALLRET(window->WindowDelete, result, context, orderInfo); } else { @@ -405,16 +406,16 @@ BOOL update_recv_window_info_order(rdpUpdate* update, wStream* s, WINDOW_ORDER_I if (orderInfo->fieldFlags & WINDOW_ORDER_STATE_NEW) { WLog_Print(update->log, WLOG_DEBUG, "WindowCreate"); - IFCALL(window->WindowCreate, context, orderInfo, &window->window_state); + IFCALLRET(window->WindowCreate, result, context, orderInfo, &window->window_state); } else { WLog_Print(update->log, WLOG_DEBUG, "WindowUpdate"); - IFCALL(window->WindowUpdate, context, orderInfo, &window->window_state); + IFCALLRET(window->WindowUpdate, result, context, orderInfo, &window->window_state); } } - return TRUE; + return result; } BOOL update_read_notification_icon_state_order(wStream* s, WINDOW_ORDER_INFO* orderInfo, NOTIFY_ICON_STATE_ORDER* notify_icon_state) @@ -470,6 +471,7 @@ BOOL update_recv_notification_icon_info_order(rdpUpdate* update, wStream* s, WIN { rdpContext* context = update->context; rdpWindowUpdate* window = update->window; + BOOL result = TRUE; if (Stream_GetRemainingLength(s) < 8) return FALSE; @@ -481,7 +483,7 @@ BOOL update_recv_notification_icon_info_order(rdpUpdate* update, wStream* s, WIN { update_read_notification_icon_delete_order(s, orderInfo); WLog_Print(update->log, WLOG_DEBUG, "NotifyIconDelete"); - IFCALL(window->NotifyIconDelete, context, orderInfo); + IFCALLRET(window->NotifyIconDelete, result, context, orderInfo); } else { @@ -491,16 +493,16 @@ BOOL update_recv_notification_icon_info_order(rdpUpdate* update, wStream* s, WIN if (orderInfo->fieldFlags & WINDOW_ORDER_STATE_NEW) { WLog_Print(update->log, WLOG_DEBUG, "NotifyIconCreate"); - IFCALL(window->NotifyIconCreate, context, orderInfo, &window->notify_icon_state); + IFCALLRET(window->NotifyIconCreate, result, context, orderInfo, &window->notify_icon_state); } else { WLog_Print(update->log, WLOG_DEBUG, "NotifyIconUpdate"); - IFCALL(window->NotifyIconUpdate, context, orderInfo, &window->notify_icon_state); + IFCALLRET(window->NotifyIconUpdate, result, context, orderInfo, &window->notify_icon_state); } } - return TRUE; + return result; } BOOL update_read_desktop_actively_monitored_order(wStream* s, WINDOW_ORDER_INFO* orderInfo, MONITORED_DESKTOP_ORDER* monitored_desktop) @@ -560,22 +562,23 @@ BOOL update_recv_desktop_info_order(rdpUpdate* update, wStream* s, WINDOW_ORDER_ { rdpContext* context = update->context; rdpWindowUpdate* window = update->window; + BOOL result = TRUE; if (orderInfo->fieldFlags & WINDOW_ORDER_FIELD_DESKTOP_NONE) { update_read_desktop_non_monitored_order(s, orderInfo); WLog_Print(update->log, WLOG_DEBUG, "NonMonitoredDesktop"); - IFCALL(window->NonMonitoredDesktop, context, orderInfo); + IFCALLRET(window->NonMonitoredDesktop, result, context, orderInfo); } else { if (!update_read_desktop_actively_monitored_order(s, orderInfo, &window->monitored_desktop)) return FALSE; WLog_Print(update->log, WLOG_DEBUG, "ActivelyMonitoredDesktop"); - IFCALL(window->MonitoredDesktop, context, orderInfo, &window->monitored_desktop); + IFCALLRET(window->MonitoredDesktop, result, context, orderInfo, &window->monitored_desktop); } - return TRUE; + return result; } BOOL update_recv_altsec_window_order(rdpUpdate* update, wStream* s) diff --git a/server/shadow/Win/win_wds.c b/server/shadow/Win/win_wds.c index 44a12d2..2bfc423 100644 --- a/server/shadow/Win/win_wds.c +++ b/server/shadow/Win/win_wds.c @@ -744,7 +744,7 @@ int win_shadow_wds_init(winShadowSubsystem* subsystem) return -1; } - subsystem->pInvitation->lpVtbl->get_ConnectionString(subsystem->pInvitation, &bstrConnectionString); + hr = subsystem->pInvitation->lpVtbl->get_ConnectionString(subsystem->pInvitation, &bstrConnectionString); if (FAILED(hr)) { @@ -752,10 +752,24 @@ int win_shadow_wds_init(winShadowSubsystem* subsystem) return -1; } + status = ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) bstrConnectionString, + ((UINT32*) bstrConnectionString)[-1], &(file->ConnectionString2), 0, NULL, NULL); + + SysFreeString(bstrConnectionString); + + if (status < 1) + { + WLog_ERR(TAG, "failed to convert connection string"); + return -1; + } + file = subsystem->pAssistanceFile = freerdp_assistance_file_new(); - ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) bstrConnectionString, - ((UINT32*) bstrConnectionString)[-1], &(file->ConnectionString2), 0, NULL, NULL); + if (!file) + { + WLog_ERR(TAG, "freerdp_assistance_file_new() failed"); + return -1; + } status = freerdp_assistance_parse_connection_string2(file); diff --git a/winpr/libwinpr/crt/test/TestUnicodeConversion.c b/winpr/libwinpr/crt/test/TestUnicodeConversion.c index 559c919..08497bf 100644 --- a/winpr/libwinpr/crt/test/TestUnicodeConversion.c +++ b/winpr/libwinpr/crt/test/TestUnicodeConversion.c @@ -260,7 +260,7 @@ int convert_utf16_to_utf8(BYTE* lpWideCharStr, BYTE* expected_lpMultiByteStr, in return length; } -int test_unicode_uppercasing(BYTE* lower, BYTE* upper) +BOOL test_unicode_uppercasing(BYTE* lower, BYTE* upper) { WCHAR* lowerW = NULL; int lowerLength; @@ -280,13 +280,182 @@ int test_unicode_uppercasing(BYTE* lower, BYTE* upper) printf("Uppercase String:\n"); string_hexdump((BYTE*) upperW, upperLength * 2); - return -1; + return FALSE; } free(lowerW); free(upperW); - return 0; + printf("success\n\n"); + return TRUE; +} + +BOOL test_ConvertFromUnicode_wrapper() +{ + /* 00 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 18 */ + WCHAR src1[] = { 'R','I','C','H',' ','T','E','X','T',' ','F','O','R','M','A','T','@','@','@' }; + WCHAR src2[] = { 'R','I','C','H',' ','T','E','X','T',' ','F','O','R','M','A','T', 0 }; + CHAR cmp0[] = { 'R','I','C','H',' ','T','E','X','T',' ','F','O','R','M','A','T', 0 }; + CHAR* dst = NULL; + int i; + + /* Test unterminated unicode string: + * ConvertFromUnicode must always null-terminate, even if the src string isn't + */ + + printf("Input UTF16 String:\n"); + string_hexdump((BYTE*) src1, 19 * sizeof(WCHAR)); + + i = ConvertFromUnicode(CP_UTF8, 0, src1, 16, &dst, 0, NULL, NULL); + if (i != 16) + { + fprintf(stderr, "ConvertFromUnicode failure A1: unexpectedly returned %d instead of 16\n", i); + goto fail; + } + if (dst == NULL) + { + fprintf(stderr, "ConvertFromUnicode failure A2: destination ist NULL\n"); + goto fail; + } + if ((i = strlen(dst)) != 16) + { + fprintf(stderr, "ConvertFromUnicode failure A3: dst length is %d instead of 16\n", i); + goto fail; + } + if (strcmp(dst, cmp0)) + { + fprintf(stderr, "ConvertFromUnicode failure A4: data mismatch\n"); + goto fail; + } + printf("Output UTF8 String:\n"); + string_hexdump((BYTE*) dst, i + 1); + + free(dst); + dst = NULL; + + /* Test null-terminated string */ + + printf("Input UTF16 String:\n"); + string_hexdump((BYTE*) src2, (_wcslen(src2) + 1 ) * sizeof(WCHAR)); + + i = ConvertFromUnicode(CP_UTF8, 0, src2, -1, &dst, 0, NULL, NULL); + if (i != 17) + { + fprintf(stderr, "ConvertFromUnicode failure B1: unexpectedly returned %d instead of 17\n", i); + goto fail; + } + if (dst == NULL) + { + fprintf(stderr, "ConvertFromUnicode failure B2: destination ist NULL\n"); + goto fail; + } + if ((i = strlen(dst)) != 16) + { + fprintf(stderr, "ConvertFromUnicode failure B3: dst length is %d instead of 16\n", i); + goto fail; + } + if (strcmp(dst, cmp0)) + { + fprintf(stderr, "ConvertFromUnicode failure B: data mismatch\n"); + goto fail; + } + printf("Output UTF8 String:\n"); + string_hexdump((BYTE*) dst, i + 1); + + free(dst); + dst = NULL; + + printf("success\n\n"); + + return TRUE; + +fail: + free(dst); + return FALSE; +} + +BOOL test_ConvertToUnicode_wrapper() +{ + /* 00 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 18 */ + CHAR src1[] = { 'R','I','C','H',' ','T','E','X','T',' ','F','O','R','M','A','T','@','@','@' }; + CHAR src2[] = { 'R','I','C','H',' ','T','E','X','T',' ','F','O','R','M','A','T', 0 }; + WCHAR cmp0[] = { 'R','I','C','H',' ','T','E','X','T',' ','F','O','R','M','A','T', 0 }; + WCHAR* dst = NULL; + int i; + + /* Test unterminated unicode string: + * ConvertToUnicode must always null-terminate, even if the src string isn't + */ + + printf("Input UTF8 String:\n"); + string_hexdump((BYTE*) src1, 19); + + i = ConvertToUnicode(CP_UTF8, 0, src1, 16, &dst, 0); + if (i != 16) + { + fprintf(stderr, "ConvertToUnicode failure A1: unexpectedly returned %d instead of 16\n", i); + goto fail; + } + if (dst == NULL) + { + fprintf(stderr, "ConvertToUnicode failure A2: destination ist NULL\n"); + goto fail; + } + if ((i = _wcslen(dst)) != 16) + { + fprintf(stderr, "ConvertToUnicode failure A3: dst length is %d instead of 16\n", i); + goto fail; + } + if (_wcscmp(dst, cmp0)) + { + fprintf(stderr, "ConvertToUnicode failure A4: data mismatch\n"); + goto fail; + } + printf("Output UTF16 String:\n"); + string_hexdump((BYTE*) dst, (i + 1) * sizeof(WCHAR)); + + free(dst); + dst = NULL; + + /* Test null-terminated string */ + + printf("Input UTF8 String:\n"); + string_hexdump((BYTE*) src2, strlen(src2) + 1); + + i = ConvertToUnicode(CP_UTF8, 0, src2, -1, &dst, 0); + if (i != 17) + { + fprintf(stderr, "ConvertToUnicode failure B1: unexpectedly returned %d instead of 17\n", i); + goto fail; + } + if (dst == NULL) + { + fprintf(stderr, "ConvertToUnicode failure B2: destination ist NULL\n"); + goto fail; + } + if ((i = _wcslen(dst)) != 16) + { + fprintf(stderr, "ConvertToUnicode failure B3: dst length is %d instead of 16\n", i); + goto fail; + } + if (_wcscmp(dst, cmp0)) + { + fprintf(stderr, "ConvertToUnicode failure B: data mismatch\n"); + goto fail; + } + printf("Output UTF16 String:\n"); + string_hexdump((BYTE*) dst, (i + 1) * 2); + + free(dst); + dst = NULL; + + printf("success\n\n"); + + return TRUE; + +fail: + free(dst); + return FALSE; } int TestUnicodeConversion(int argc, char* argv[]) @@ -375,7 +544,47 @@ int TestUnicodeConversion(int argc, char* argv[]) printf("Uppercasing\n"); - test_unicode_uppercasing(ru_Administrator_lower, ru_Administrator_upper); + if (!test_unicode_uppercasing(ru_Administrator_lower, ru_Administrator_upper)) + return -1; + + /* ConvertFromUnicode */ + + printf("ConvertFromUnicode\n"); + + if (!test_ConvertFromUnicode_wrapper()) + return -1; + + /* ConvertToUnicode */ + + printf("ConvertToUnicode\n"); + if (!test_ConvertToUnicode_wrapper()) + return -1; +/* + + printf("----------------------------------------------------------\n\n"); + + if (0) + { + BYTE src[] = { 'R',0,'I',0,'C',0,'H',0,' ',0, 'T',0,'E',0,'X',0,'T',0,' ',0,'F',0,'O',0,'R',0,'M',0,'A',0,'T',0,'@',0,'@',0 }; + //BYTE src[] = { 'R',0,'I',0,'C',0,'H',0,' ',0, 0,0, 'T',0,'E',0,'X',0,'T',0,' ',0,'F',0,'O',0,'R',0,'M',0,'A',0,'T',0,'@',0,'@',0 }; + //BYTE src[] = { 0,0,'R',0,'I',0,'C',0,'H',0,' ',0, 'T',0,'E',0,'X',0,'T',0,' ',0,'F',0,'O',0,'R',0,'M',0,'A',0,'T',0,'@',0,'@',0 }; + char* dst = NULL; + int num; + num = ConvertFromUnicode(CP_UTF8, 0, (WCHAR*) src, 16, &dst, 0, NULL, NULL); + printf("ConvertFromUnicode returned %d dst=[%s]\n", num, dst); + string_hexdump((BYTE*)dst, num+1); + } + if (1) + { + char src[] = "RICH TEXT FORMAT@@@@@@"; + WCHAR *dst = NULL; + int num; + num = ConvertToUnicode(CP_UTF8, 0, src, 16, &dst, 0); + printf("ConvertToUnicode returned %d dst=%p\n", num, dst); + string_hexdump((BYTE*)dst, num * 2 + 2); + + } +*/ return 0; } diff --git a/winpr/libwinpr/crt/unicode.c b/winpr/libwinpr/crt/unicode.c index fc5134f..0816fe0 100644 --- a/winpr/libwinpr/crt/unicode.c +++ b/winpr/libwinpr/crt/unicode.c @@ -276,6 +276,19 @@ int WideCharToMultiByte(UINT CodePage, DWORD dwFlags, LPCWSTR lpWideCharStr, int #endif +/** + * ConvertToUnicode is a convenience wrapper for MultiByteToWideChar: + * + * If the lpWideCharStr prarameter for the converted string points to NULL + * or if the cchWideChar parameter is set to 0 this function will automatically + * allocate the required memory which is guaranteed to be null-terminated + * after the conversion, even if the source c string isn't. + * + * If the cbMultiByte parameter is set to -1 the passed lpMultiByteStr must + * be null-terminated and the required length for the converted string will be + * calculated accordingly. + */ + int ConvertToUnicode(UINT CodePage, DWORD dwFlags, LPCSTR lpMultiByteStr, int cbMultiByte, LPWSTR* lpWideCharStr, int cchWideChar) { @@ -305,7 +318,7 @@ int ConvertToUnicode(UINT CodePage, DWORD dwFlags, LPCSTR lpMultiByteStr, if (allocate) { - *lpWideCharStr = (LPWSTR) calloc(cchWideChar, sizeof(WCHAR)); + *lpWideCharStr = (LPWSTR) calloc(cchWideChar + 1, sizeof(WCHAR)); if (!(*lpWideCharStr)) { @@ -317,11 +330,31 @@ int ConvertToUnicode(UINT CodePage, DWORD dwFlags, LPCSTR lpMultiByteStr, status = MultiByteToWideChar(CodePage, dwFlags, lpMultiByteStr, cbMultiByte, *lpWideCharStr, cchWideChar); if (status != cchWideChar) + { + if (allocate) + { + free(*lpWideCharStr); + *lpWideCharStr = NULL; + } status = 0; + } return status; } +/** + * ConvertFromUnicode is a convenience wrapper for WideCharToMultiByte: + * + * If the lpMultiByteStr parameter for the converted string points to NULL + * or if the cbMultiByte parameter is set to 0 this function will automatically + * allocate the required memory which is guaranteed to be null-terminated + * after the conversion, even if the source unicode string isn't. + * + * If the cchWideChar parameter is set to -1 the passed lpWideCharStr must + * be null-terminated and the required length for the converted string will be + * calculated accordingly. + */ + int ConvertFromUnicode(UINT CodePage, DWORD dwFlags, LPCWSTR lpWideCharStr, int cchWideChar, LPSTR* lpMultiByteStr, int cbMultiByte, LPCSTR lpDefaultChar, LPBOOL lpUsedDefaultChar) { diff --git a/winpr/libwinpr/file/generic.c b/winpr/libwinpr/file/generic.c index 367d282..71d4756 100644 --- a/winpr/libwinpr/file/generic.c +++ b/winpr/libwinpr/file/generic.c @@ -274,7 +274,7 @@ HANDLE CreateFileW(LPCWSTR lpFileName, DWORD dwDesiredAccess, DWORD dwShareMode, LPSTR lpFileNameA = NULL; HANDLE hdl; - if (ConvertFromUnicode(CP_UTF8, 0, lpFileName, -1, &lpFileNameA, 0, NULL, NULL)) + if (ConvertFromUnicode(CP_UTF8, 0, lpFileName, -1, &lpFileNameA, 0, NULL, NULL) < 1) return NULL; hdl= CreateFileA(lpFileNameA, dwDesiredAccess, dwShareMode, lpSecurityAttributes, @@ -296,7 +296,7 @@ BOOL DeleteFileW(LPCWSTR lpFileName) LPSTR lpFileNameA = NULL; BOOL rc = FALSE; - if (ConvertFromUnicode(CP_UTF8, 0, lpFileName, -1, &lpFileNameA, 0, NULL, NULL)) + if (ConvertFromUnicode(CP_UTF8, 0, lpFileName, -1, &lpFileNameA, 0, NULL, NULL) < 1) return FALSE; rc = DeleteFileA(lpFileNameA); free (lpFileNameA); -- 2.7.4