From 10cf8f2d1c156dc23048c14205d94dbd588eb1de Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 30 Sep 2024 14:50:00 +0100 Subject: [PATCH] Remove xrdp_sec_in_mcs_data() function THe function xrdp_sec_in_mcs_data() parses data within the TS_UD_CS_CORE struct which could just as easily be parsed when xrdp_sec_process_mcs_data_CS_CORE() is called. Currently the contents of the MSC Connect Initial PDU are stored within the client_mcs_data member of the xrdp_sec struct. This stream is parsed once by xrdp_sec_process_mcs_data() and then separately by xrdp_sec_in_mcs_data(). There is no reason not to perform the parse in a single pass through the stream. This commit folds the functionality in xrdp_sec_in_mcs_data() into xrdp_sec_process_mcs_data_CS_CORE() and removes xrdp_sec_in_mcs_data() --- libxrdp/xrdp_sec.c | 119 ++++++++------------------------------------- 1 file changed, 19 insertions(+), 100 deletions(-) diff --git a/libxrdp/xrdp_sec.c b/libxrdp/xrdp_sec.c index 779e4b96c..07f61c421 100644 --- a/libxrdp/xrdp_sec.c +++ b/libxrdp/xrdp_sec.c @@ -1488,7 +1488,6 @@ xrdp_sec_process_mcs_data_CS_CORE(struct xrdp_sec *self, struct stream *s) int highColorDepth; int supportedColorDepths; int earlyCapabilityFlags; - char clientName[INFO_CLIENT_NAME_BYTES / 2] = { '\0' }; UNUSED_VAR(version); struct xrdp_client_info *client_info = &self->rdp_layer->client_info; @@ -1516,8 +1515,8 @@ xrdp_sec_process_mcs_data_CS_CORE(struct xrdp_sec *self, struct stream *s) break; } in_uint8s(s, 2); /* SASSequence */ - in_uint8s(s, 4); /* keyboardLayout */ - in_uint8s(s, 4); /* clientBuild */ + in_uint32_le(s, client_info->keylayout); + in_uint32_le(s, client_info->build); /* clientName * @@ -1525,26 +1524,33 @@ xrdp_sec_process_mcs_data_CS_CORE(struct xrdp_sec *self, struct stream *s) * isn't by ignoring the last two bytes and treating them as a * terminator anyway */ in_utf16_le_fixed_as_utf8(s, (INFO_CLIENT_NAME_BYTES - 2) / 2, - clientName, sizeof(clientName)); + client_info->hostname, + sizeof(client_info->hostname)); in_uint8s(s, 2); /* See above */ - LOG(LOG_LEVEL_INFO, "Connected client computer name: %s", clientName); - in_uint8s(s, 4); /* keyboardType */ - in_uint8s(s, 4); /* keyboardSubType */ + LOG(LOG_LEVEL_INFO, "Connected client computer name: %s", + client_info->hostname); + in_uint32_le(s, client_info->keyboard_type); /* [MS-RDPBCGR] TS_UD_CS_CORE keyboardType */ + in_uint32_le(s, client_info->keyboard_subtype); /* [MS-RDPBCGR] TS_UD_CS_CORE keyboardSubType */ in_uint8s(s, 4); /* keyboardFunctionKey */ in_uint8s(s, 64); /* imeFileName */ LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_UD_CS_CORE " " version %08x, desktopWidth %d, " "desktopHeight %d, colorDepth %s, SASSequence (ignored), " - "keyboardLayout (ignored), clientBuild (ignored), " - "clientName %s, keyboardType (ignored), " - "keyboardSubType (ignored), keyboardFunctionKey (ignored), " + "keyboardLayout 0x%8.8x, clientBuild %d, " + "clientName %s, keyboardType 0x%8.8x, " + "keyboardSubType 0x%8.8x, keyboardFunctionKey (ignored), " "imeFileName (ignored)", version, client_info->display_sizes.session_width, client_info->display_sizes.session_height, - (colorDepth == 0xca00 ? "RNS_UD_COLOR_4BPP" : - colorDepth == 0xca01 ? "RNS_UD_COLOR_8BPP" : "unknown"), - clientName); + (colorDepth == RNS_UD_COLOR_4BPP ? "RNS_UD_COLOR_4BPP" : + colorDepth == RNS_UD_COLOR_8BPP ? "RNS_UD_COLOR_8BPP" : + "unknown"), + client_info->keylayout, + client_info->build, + client_info->hostname, + client_info->keyboard_type, + client_info->keyboard_subtype); /* TS_UD_CS_CORE optional fields */ if (!s_check_rem(s, 2)) @@ -2180,87 +2186,6 @@ xrdp_sec_process_mcs_data(struct xrdp_sec *self) return 0; } -/*****************************************************************************/ -/* Process the mcs client data [ITU T.124] ConferenceCreateRequest userData field - as a [MS-RDPBCGR] TS_UD_CS_CORE struct */ -/* TODO: why does this method parse the strust from back to front (resetting - after each field) instead of from front to back like the rest of the parsing - code? */ -/* TODO: why does this method exist when the same struct is parsed in - xrdp_sec_process_mcs_data_CS_CORE and there does not seem to be any - dependencies preventing a call to that function. */ -/* TODO: this is a brittle function that assumes field offsets in the stream - instead of parsing the variable length fields of [ITU T.124] ConferenceCreateRequest */ -static int -xrdp_sec_in_mcs_data(struct xrdp_sec *self) -{ - struct stream *s = (struct stream *)NULL; - struct xrdp_client_info *client_info = (struct xrdp_client_info *)NULL; - - client_info = &(self->rdp_layer->client_info); - s = &(self->client_mcs_data); - /* get hostname, it's unicode */ - s->p = s->data; - if (!s_check_rem_and_log(s, 47, "Parsing [ITU T.124] ConferenceCreateRequest")) - { - return 1; - } - in_uint8s(s, 47); /* skip [ITU T.124] ConferenceCreateRequest up to the - userData field, and skip [MS-RDPBCGR] TS_UD_CS_CORE - up to the clientName field */ - if (!s_check_rem_and_log(s, INFO_CLIENT_NAME_BYTES, - "Parsing [MS-RDPBCGR] TS_UD_CS_CORE clientName")) - { - return 1; - } - in_utf16_le_fixed_as_utf8(s, (INFO_CLIENT_NAME_BYTES - 2) / 2, - client_info->hostname, - sizeof(client_info->hostname)); - in_uint8s(s, 2); /* Ignored - terminator for full-size clientName */ - - /* get build */ - s->p = s->data; - if (!s_check_rem_and_log(s, 43 + 4, "Parsing [MS-RDPBCGR] TS_UD_CS_CORE clientBuild")) - { - return 1; - } - in_uint8s(s, 43); - in_uint32_le(s, client_info->build); /* [MS-RDPBCGR] TS_UD_CS_CORE clientBuild */ - /* get keylayout */ - s->p = s->data; - if (!s_check_rem_and_log(s, 39 + 4, "Parsing [MS-RDPBCGR] TS_UD_CS_CORE keyboardLayout")) - { - return 1; - } - in_uint8s(s, 39); - in_uint32_le(s, client_info->keylayout); /* [MS-RDPBCGR] TS_UD_CS_CORE keyboardLayout */ - /* get keyboard type / subtype */ - s->p = s->data; - if (!s_check_rem_and_log(s, 79 + 8, "Parsing [MS-RDPBCGR] TS_UD_CS_CORE keyboardType")) - { - return 1; - } - in_uint8s(s, 79); - in_uint32_le(s, client_info->keyboard_type); /* [MS-RDPBCGR] TS_UD_CS_CORE keyboardType */ - in_uint32_le(s, client_info->keyboard_subtype); /* [MS-RDPBCGR] TS_UD_CS_CORE keyboardSubType */ - LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_UD_CS_CORE " - " version (ignored), desktopWidth (ignored), " - "desktopHeight (ignored), colorDepth (ignored), SASSequence (ignored), " - "keyboardLayout 0x%8.8x, clientBuild %d, " - "clientName %s, keyboardType 0x%8.8x, " - "keyboardSubType 0x%8.8x, keyboardFunctionKey (ignored), " - "imeFileName (ignored)", - client_info->keylayout, - client_info->build, - client_info->hostname, - client_info->keyboard_type, - client_info->keyboard_subtype); - - s->p = s->data; - - return 0; -} - /*****************************************************************************/ /* returns error */ static int @@ -2445,12 +2370,6 @@ xrdp_sec_incoming(struct xrdp_sec *self) self->server_mcs_data.data, (int)(self->server_mcs_data.end - self->server_mcs_data.data)); - if (xrdp_sec_in_mcs_data(self) != 0) - { - LOG(LOG_LEVEL_ERROR, "xrdp_sec_incoming: xrdp_sec_in_mcs_data failed"); - return 1; - } - return 0; }