From 7eb44bd54c68bbbca68a7d2e56810e7868ff02ed Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 21 Apr 2022 11:19:20 +0100 Subject: [PATCH 1/4] Update channel logging --- libxrdp/xrdp_mcs.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/libxrdp/xrdp_mcs.c b/libxrdp/xrdp_mcs.c index 4d86aee0ae..c5d7b33ec8 100644 --- a/libxrdp/xrdp_mcs.c +++ b/libxrdp/xrdp_mcs.c @@ -718,6 +718,8 @@ xrdp_mcs_recv_cjrq(struct xrdp_mcs *self) { int opcode; struct stream *s; + int initiator; + int channel_id; s = libxrdp_force_read(self->iso_layer->trans); if (s == 0) @@ -753,8 +755,8 @@ xrdp_mcs_recv_cjrq(struct xrdp_mcs *self) return 1; } - in_uint8s(s, 4); /* initiator (2 bytes) - channelId (2 bytes) */ + in_uint16_be(s, initiator); + in_uint16_be(s, channel_id); /* * [MS-RDPBCGR] 2.2.1.8 says that the mcsAUrq field is 5 bytes (which have @@ -772,8 +774,9 @@ xrdp_mcs_recv_cjrq(struct xrdp_mcs *self) } LOG_DEVEL(LOG_LEVEL_TRACE, "Received [ITU-T T.125] ChannelJoinRequest " - "initiator (ignored), channelId (ignored), " + "initiator %d, channelId %d, " "nonStandard (%s)", + initiator, channel_id, (opcode & 2) ? "present" : "not present"); if (!s_check_end_and_log(s, "MCS protocol error [ITU-T T.125] ChannelJoinRequest")) From 28da2045d9d7dca741fb0a5cfad12fe244f370d4 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 12 May 2022 16:10:53 +0100 Subject: [PATCH 2/4] Send MS-compatible user channel The Windows 10 RDS sets the user channel ID to be one more than the ID of the last allocated static virtual channel. Currently we set it to 1002 (0x03ea) which is allocated to the server channel. This change makes xrdp emulate RDS more closely. --- libxrdp/xrdp_mcs.c | 19 +++++++++++-------- libxrdp/xrdp_sec.c | 17 ++++++++++++++++- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/libxrdp/xrdp_mcs.c b/libxrdp/xrdp_mcs.c index c5d7b33ec8..f90a7f30a1 100644 --- a/libxrdp/xrdp_mcs.c +++ b/libxrdp/xrdp_mcs.c @@ -109,7 +109,7 @@ xrdp_mcs_send_cjcf(struct xrdp_mcs *self, int userid, int chanid) out_uint16_be(s, chanid); /* channelId (OPTIONAL) */ s_mark_end(s); - LOG_DEVEL(LOG_LEVEL_TRACE, "Sening [ITU-T T.125] ChannelJoinConfirm " + LOG_DEVEL(LOG_LEVEL_TRACE, "Sending [ITU-T T.125] ChannelJoinConfirm " "result SUCCESS, initiator %d, requested %d, " "channelId %d", userid, chanid, chanid); if (xrdp_iso_send(self->iso_layer, s) != 0) @@ -714,12 +714,11 @@ xrdp_mcs_send_aucf(struct xrdp_mcs *self) * * returns error */ static int -xrdp_mcs_recv_cjrq(struct xrdp_mcs *self) +xrdp_mcs_recv_cjrq(struct xrdp_mcs *self, int *channel_id) { int opcode; struct stream *s; int initiator; - int channel_id; s = libxrdp_force_read(self->iso_layer->trans); if (s == 0) @@ -756,7 +755,7 @@ xrdp_mcs_recv_cjrq(struct xrdp_mcs *self) } in_uint16_be(s, initiator); - in_uint16_be(s, channel_id); + in_uint16_be(s, *channel_id); /* * [MS-RDPBCGR] 2.2.1.8 says that the mcsAUrq field is 5 bytes (which have @@ -776,7 +775,7 @@ xrdp_mcs_recv_cjrq(struct xrdp_mcs *self) LOG_DEVEL(LOG_LEVEL_TRACE, "Received [ITU-T T.125] ChannelJoinRequest " "initiator %d, channelId %d, " "nonStandard (%s)", - initiator, channel_id, + initiator, *channel_id, (opcode & 2) ? "present" : "not present"); if (!s_check_end_and_log(s, "MCS protocol error [ITU-T T.125] ChannelJoinRequest")) @@ -1237,18 +1236,22 @@ xrdp_mcs_incoming(struct xrdp_mcs *self) return 1; } + /* + * Expect a channel join request PDU for each of the static virtual channels, plus + * the user channel (self->chanid) and the I/O channel (MCS_GLOBAL_CHANNEL) + */ for (index = 0; index < self->channel_list->count + 2; index++) { + int channel_id; LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence] receive channel join request"); - if (xrdp_mcs_recv_cjrq(self) != 0) + if (xrdp_mcs_recv_cjrq(self, &channel_id) != 0) { LOG(LOG_LEVEL_ERROR, "[MCS Connection Sequence] receive channel join request failed"); return 1; } LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence] send channel join confirm"); - if (xrdp_mcs_send_cjcf(self, self->userid, - self->userid + MCS_USERCHANNEL_BASE + index) != 0) + if (xrdp_mcs_send_cjcf(self, self->userid, channel_id) != 0) { LOG(LOG_LEVEL_ERROR, "[MCS Connection Sequence] send channel join confirm failed"); return 1; diff --git a/libxrdp/xrdp_sec.c b/libxrdp/xrdp_sec.c index 885b5a3d47..1795dcfff0 100644 --- a/libxrdp/xrdp_sec.c +++ b/libxrdp/xrdp_sec.c @@ -2267,6 +2267,7 @@ xrdp_sec_process_mcs_data_channels(struct xrdp_sec *self, struct stream *s) int index; struct xrdp_client_info *client_info; struct mcs_channel_item *channel_item; + int next_mcs_channel_id; client_info = &(self->rdp_layer->client_info); /* this is an option set in xrdp.ini */ @@ -2288,6 +2289,13 @@ xrdp_sec_process_mcs_data_channels(struct xrdp_sec *self, struct stream *s) "max 31, received %d", num_channels); return 1; } + + /* GOTCHA: When adding a channel the MCS channel ID is set to + * MCS_GLOBAL_CHANNEL + (index + 1). This is assumed by + * xrdp_channel_process(), when mapping an incoming PDU into an + * entry in this array */ + next_mcs_channel_id = MCS_GLOBAL_CHANNEL + 1; + for (index = 0; index < num_channels; index++) { channel_item = g_new0(struct mcs_channel_item, 1); @@ -2304,7 +2312,7 @@ xrdp_sec_process_mcs_data_channels(struct xrdp_sec *self, struct stream *s) LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] " "TS_UD_CS_NET.CHANNEL_DEF %d, name %s, options 0x%8.8x", index, channel_item->name, channel_item->flags); - channel_item->chanid = MCS_GLOBAL_CHANNEL + (index + 1); + channel_item->chanid = next_mcs_channel_id++; list_add_item(self->mcs_layer->channel_list, (intptr_t) channel_item); LOG(LOG_LEVEL_DEBUG, @@ -2322,6 +2330,13 @@ xrdp_sec_process_mcs_data_channels(struct xrdp_sec *self, struct stream *s) g_free(channel_item); } } + + /* Set the user channel as well */ + self->mcs_layer->chanid = next_mcs_channel_id++; + self->mcs_layer->userid = self->mcs_layer->chanid - MCS_USERCHANNEL_BASE; + LOG_DEVEL(LOG_LEVEL_DEBUG, "MCS user is %d, channel id is %d", + self->mcs_layer->userid, self->mcs_layer->chanid); + return 0; } From 683864349440ab0b90193df5bc6d1a863056ab36 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Wed, 11 May 2022 13:39:41 +0100 Subject: [PATCH 3/4] Log connected client version --- libxrdp/xrdp_sec.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libxrdp/xrdp_sec.c b/libxrdp/xrdp_sec.c index 1795dcfff0..bde7d3face 100644 --- a/libxrdp/xrdp_sec.c +++ b/libxrdp/xrdp_sec.c @@ -1944,6 +1944,7 @@ xrdp_sec_send_fastpath(struct xrdp_sec *self, struct stream *s) static int xrdp_sec_process_mcs_data_CS_CORE(struct xrdp_sec *self, struct stream *s) { + int version; int colorDepth; int postBeta2ColorDepth; int highColorDepth; @@ -1951,8 +1952,10 @@ xrdp_sec_process_mcs_data_CS_CORE(struct xrdp_sec *self, struct stream *s) int earlyCapabilityFlags; char clientName[INFO_CLIENT_NAME_BYTES / 2] = { '\0' }; + UNUSED_VAR(version); + /* TS_UD_CS_CORE requiered fields */ - in_uint8s(s, 4); /* version */ + in_uint32_le(s, version); in_uint16_le(s, self->rdp_layer->client_info.display_sizes.session_width); in_uint16_le(s, self->rdp_layer->client_info.display_sizes.session_height); in_uint16_le(s, colorDepth); @@ -1975,12 +1978,13 @@ xrdp_sec_process_mcs_data_CS_CORE(struct xrdp_sec *self, struct stream *s) in_uint8s(s, 4); /* keyboardFunctionKey */ in_uint8s(s, 64); /* imeFileName */ LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_UD_CS_CORE " - " version (ignored), desktopWidth %d, " + " version %08x, desktopWidth %d, " "desktopHeight %d, colorDepth %s, SASSequence (ingored), " "keyboardLayout (ignored), clientBuild (ignored), " "clientName %s, keyboardType (ignored), " "keyboardSubType (ignored), keyboardFunctionKey (ignored), " - "imeFileName (ignroed)", + "imeFileName (ignored)", + version, self->rdp_layer->client_info.display_sizes.session_width, self->rdp_layer->client_info.display_sizes.session_height, (colorDepth == 0xca00 ? "RNS_UD_COLOR_4BPP" : From 8fdc1ba21685ae35d6d6a4eaa189299518f246ee Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 12 May 2022 16:04:54 +0100 Subject: [PATCH 4/4] Relaxed Channel Join PDU requirements for non-TLS Windows 10 RDS is quite relaxed about missing channel join PDUs, whereas we have to adhere quite tightly to the specification to make sure we get a TLS "Client hello" where appropriate. This makes us incompatible with older RDP clients. For example, the Wyse sx0 thin client does not send a channel join PDU for the user channel. Older, non-TLS versions of xrdp supported these devices. This commit re-implements the xrdp v0.6.1 behaviour for non-TLS connections only, allowing system administrators to use these devices on trusted networks. These devices are in any case too old to establish a modern TLS connection. --- libxrdp/libxrdp.h | 3 + libxrdp/xrdp_mcs.c | 252 ++++++++++++++++++++++++++++++++------------- 2 files changed, 184 insertions(+), 71 deletions(-) diff --git a/libxrdp/libxrdp.h b/libxrdp/libxrdp.h index b124a85bcc..19ba95d1e8 100644 --- a/libxrdp/libxrdp.h +++ b/libxrdp/libxrdp.h @@ -67,6 +67,9 @@ struct xrdp_mcs struct stream *client_mcs_data; struct stream *server_mcs_data; struct list *channel_list; + /* This boolean is set to indicate we're expecing channel join + * requests as part of the connect sequence */ + int expecting_channel_join_requests; }; /* fastpath */ diff --git a/libxrdp/xrdp_mcs.c b/libxrdp/xrdp_mcs.c index f90a7f30a1..0f23e473ca 100644 --- a/libxrdp/xrdp_mcs.c +++ b/libxrdp/xrdp_mcs.c @@ -27,6 +27,11 @@ #include "ms-rdpbcgr.h" #include "log.h" +/* Forward references */ +static int +handle_non_tls_client_channel_join_requests(struct xrdp_mcs *self, + struct stream *s, int *appid); + /*****************************************************************************/ struct xrdp_mcs * xrdp_mcs_create(struct xrdp_sec *owner, struct trans *trans, @@ -123,6 +128,34 @@ xrdp_mcs_send_cjcf(struct xrdp_mcs *self, int userid, int chanid) return 0; } +/*****************************************************************************/ +/* Reads the header of a DomainMCSPDU and gets the appid + * + * @return 0 for success */ +static int +get_domain_mcs_pdu_header(struct xrdp_mcs *self, struct stream *s, int *appid) +{ + int opcode; + if (xrdp_iso_recv(self->iso_layer, s) != 0) + { + LOG(LOG_LEVEL_ERROR, "xrdp_mcs_recv: xrdp_iso_recv failed"); + return 1; + } + + if (!s_check_rem_and_log(s, 1, "Parsing [ITU-T T.125] DomainMCSPDU")) + { + return 1; + } + + /* The DomainMCSPDU choice index is a 6-bit int with the 2 least + significant bits of the byte as padding */ + in_uint8(s, opcode); + *appid = opcode >> 2; /* 2-bit padding */ + LOG_DEVEL(LOG_LEVEL_TRACE, + "Received [ITU-T T.125] DomainMCSPDU choice index %d", *appid); + return 0; +} + /*****************************************************************************/ /* * Processes an [ITU-T T.125] DomainMCSPDU message. @@ -136,70 +169,32 @@ int xrdp_mcs_recv(struct xrdp_mcs *self, struct stream *s, int *chan) { int appid; - int opcode; int len; - int userid; - int chanid; - - while (1) + if (get_domain_mcs_pdu_header(self, s, &appid) != 0) { - if (xrdp_iso_recv(self->iso_layer, s) != 0) - { - LOG(LOG_LEVEL_ERROR, "xrdp_mcs_recv: xrdp_iso_recv failed"); - return 1; - } + return 1; + } - if (!s_check_rem_and_log(s, 1, "Parsing [ITU-T T.125] DomainMCSPDU")) + if (self->expecting_channel_join_requests) + { + if (handle_non_tls_client_channel_join_requests(self, s, &appid) != 0) { return 1; } + LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence] completed"); + self->expecting_channel_join_requests = 0; + } - /* The DomainMCSPDU choice index is a 6-bit int with the 2 least - significant bits of the byte as padding */ - in_uint8(s, opcode); - appid = opcode >> 2; /* 2-bit padding */ + if (appid == MCS_DPUM) /* Disconnect Provider Ultimatum */ + { LOG_DEVEL(LOG_LEVEL_TRACE, - "Received [ITU-T T.125] DomainMCSPDU choice index %d", appid); - - if (appid == MCS_DPUM) /* Disconnect Provider Ultimatum */ - { - LOG_DEVEL(LOG_LEVEL_TRACE, "Received [ITU-T T.125] DisconnectProviderUltimatum"); - LOG(LOG_LEVEL_DEBUG, "Recieved disconnection request"); - return 1; - } - - /* MCS_CJRQ: Channel Join ReQuest - this is channels getting added from the client */ - if (appid == MCS_CJRQ) - { - if (!s_check_rem_and_log(s, 4, "Parsing [ITU-T T.125] ChannelJoinRequest")) - { - return 1; - } - - in_uint16_be(s, userid); - in_uint16_be(s, chanid); - LOG_DEVEL(LOG_LEVEL_TRACE, "Received [ITU-T T.125] ChannelJoinRequest " - "initiator 0x%4.4x, channelId 0x%4.4x", userid, chanid); - - if (xrdp_mcs_send_cjcf(self, userid, chanid) != 0) - { - LOG(LOG_LEVEL_WARNING, "[ITU-T T.125] Channel join sequence: failed"); - } - - s = libxrdp_force_read(self->iso_layer->trans); - if (s == 0) - { - LOG(LOG_LEVEL_ERROR, "xrdp_mcs_recv: libxrdp_force_read failed"); - return 1; - } - - continue; - } - break; + "Received [ITU-T T.125] DisconnectProviderUltimatum"); + LOG(LOG_LEVEL_DEBUG, "Recieved disconnection request"); + return 1; } + if (appid != MCS_SDRQ) { LOG(LOG_LEVEL_ERROR, "Received [ITU-T T.125] DomainMCSPDU " @@ -1176,6 +1171,127 @@ xrdp_mcs_send_connect_response(struct xrdp_mcs *self) return 0; } +/*****************************************************************************/ +/* Handle all client channel join requests for a non-TLS connection + * + * @param self MCS structure + * @param s Input stream + * @param[in,out] appid Type of the MCS PDU whose header has just been read. + * @return 0 for success + * + * For non-TLS connections, the channel join MCS PDUs are followed by + * another MCS PDU. This not the case for TLS connections. + * + * Called when an MCS PDU header has been read, but the PDU has not + * been processed. + * + * If the PDU is a channel join request, it is processed, and the next + * PDU header is read. When we've exhausted all the channel join requests, + * the type of the next PDU is passed back to the caller for the caller + * to process. + * + * In order to cater for older clients which may not conform exactly to + * the specification, we simply take all the join requests which come in, + * and respond to them. + * + * See :- + * - https://github.com/neutrinolabs/xrdp/issues/2166 + * - [MS-RDPBCGR] 3.2.5.3.8 and 3.2.5.3.8 + */ +static int +handle_non_tls_client_channel_join_requests(struct xrdp_mcs *self, + struct stream *s, int *appid) +{ + int rv = 0; + while (*appid == MCS_CJRQ) + { + int userid; + int chanid; + + if (!s_check_rem_and_log(s, 4, "Parsing [ITU-T T.125] " + "ChannelJoinRequest")) + { + rv = 1; + break; + } + + in_uint16_be(s, userid); + in_uint16_be(s, chanid); + LOG_DEVEL(LOG_LEVEL_TRACE, + "Received [ITU-T T.125] ChannelJoinRequest " + "initiator 0x%4.4x, channelId 0x%4.4x", userid, chanid); + + if (xrdp_mcs_send_cjcf(self, userid, chanid) != 0) + { + LOG(LOG_LEVEL_WARNING, + "[ITU-T T.125] Channel join sequence: failed"); + } + + /* Get the next PDU header */ + s = libxrdp_force_read(self->iso_layer->trans); + if (s == 0) + { + LOG(LOG_LEVEL_ERROR, "xrdp_mcs_recv: libxrdp_force_read failed"); + rv = 1; + break; + } + if (get_domain_mcs_pdu_header(self, s, appid) != 0) + { + rv = 1; + break; + } + } + + return rv; +} + +/*****************************************************************************/ +/* Handle all client channel join requests for a TLS connection + * + * @param self MCS structure + * @return 0 for success + * + * When were are about to negotiate a TLS connection, it is important that + * we agree on the exact number of client join request / client join confirm + * PDUs, so that we get the TLS 'client hello' message exactly when + * expected. + * + * See [MS-RDPBCGR] 3.2.5.3.8 and 3.2.5.3.8 + */ +static int +handle_tls_client_channel_join_requests(struct xrdp_mcs *self) +{ + int index; + int rv = 0; + + static const char *tag = "[MCS Connection Sequence (TLS)]"; + /* + * Expect a channel join request PDU for each of the static virtual + * channels, plus the user channel (self->chanid) and the I/O channel + * (MCS_GLOBAL_CHANNEL) */ + for (index = 0; index < self->channel_list->count + 2; index++) + { + int channel_id; + LOG(LOG_LEVEL_DEBUG, "%s receive channel join request", tag); + if (xrdp_mcs_recv_cjrq(self, &channel_id) != 0) + { + LOG(LOG_LEVEL_ERROR, "%s receive channel join request failed", tag); + rv = 1; + break; + } + + LOG(LOG_LEVEL_DEBUG, "%s send channel join confirm", tag); + if (xrdp_mcs_send_cjcf(self, self->userid, channel_id) != 0) + { + LOG(LOG_LEVEL_ERROR, "%s send channel join confirm failed", tag); + rv = 1; + break; + } + } + + return rv; +} + /*****************************************************************************/ /* Process and send the MCS messages for the RDP Connection Sequence * [MS-RDPBCGR] 1.3.1.1 @@ -1185,8 +1301,6 @@ xrdp_mcs_send_connect_response(struct xrdp_mcs *self) int xrdp_mcs_incoming(struct xrdp_mcs *self) { - int index; - LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence] receive connection request"); if (xrdp_mcs_recv_connect_initial(self) != 0) { @@ -1236,29 +1350,25 @@ xrdp_mcs_incoming(struct xrdp_mcs *self) return 1; } - /* - * Expect a channel join request PDU for each of the static virtual channels, plus - * the user channel (self->chanid) and the I/O channel (MCS_GLOBAL_CHANNEL) - */ - for (index = 0; index < self->channel_list->count + 2; index++) + if (self->iso_layer->selectedProtocol > PROTOCOL_RDP) { - int channel_id; - LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence] receive channel join request"); - if (xrdp_mcs_recv_cjrq(self, &channel_id) != 0) + /* TLS connection. Client and server have to agree on MCS channel + * join messages, and these have to be processed before the TLS + * client hello */ + if (handle_tls_client_channel_join_requests(self) != 0) { - LOG(LOG_LEVEL_ERROR, "[MCS Connection Sequence] receive channel join request failed"); return 1; } - LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence] send channel join confirm"); - if (xrdp_mcs_send_cjcf(self, self->userid, channel_id) != 0) - { - LOG(LOG_LEVEL_ERROR, "[MCS Connection Sequence] send channel join confirm failed"); - return 1; - } + LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence (TLS)] completed"); + } + else + { + /* Non-TLS connection - channel joins handled in MCS PDU + * processing loop */ + self->expecting_channel_join_requests = 1; } - LOG(LOG_LEVEL_DEBUG, "[MCS Connection Sequence] completed"); return 0; }