ERROR reading clientDir with macOS MS Remote Desktop.app with devel #2853
Comments
|
Like the source and spec says this should have a null term so disabling the check shouldn't be it. |
|
@matt335672 Can you look into this |
|
Looks like this is a client issue. iOS MS RDP does the same thing and MS RDP Android did (or does?) the same. So maybe disabling the check or doing something "extra" if len_ldd is zero might be desired until MS fixes their own clients? |
|
Thanks for raising this @derekschrock This appears to be a regression introduced by #2794. It doesn't seem to affect v0.9.x which is probably why we haven't seen it before. The UTF16 conversion code there is adding 2 back on to the values which have had 2 subtracted from them. Unterminated strings will cause an error, but missing strings won't. There's a bit of length checking missing from this code. Also seems to be no reason to parse fields we are not using. I've made a few changes to address this, including using unsigned variables. Derek - can you try the attached patch? I don't have easy access to a client which exhibits this behaviour. I've reproduced the patch below as well --- a/common/ms-rdpbcgr.h
+++ b/common/ms-rdpbcgr.h
@@ -131,6 +131,9 @@
#define RDP_LOGON_LEAVE_AUDIO 0x2000
#define RDP_LOGON_RAIL 0x8000
+/* Extended Info Packet: clientAddress (2.2.1.11.1.1.1) */
+#define EXTENDED_INFO_MAX_CLIENT_ADDR_LENGTH 80
+
/* Extended Info Packet: performanceFlags (2.2.1.11.1.1.1) */
/* TODO: to be renamed */
#define RDP5_DISABLE_NOTHING 0x00
diff --git a/libxrdp/xrdp_sec.c b/libxrdp/xrdp_sec.c
index 231176cd..20665dfc 100644
--- a/libxrdp/xrdp_sec.c
+++ b/libxrdp/xrdp_sec.c
@@ -833,18 +833,15 @@ static int
xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s)
{
int flags = 0;
- int len_domain = 0;
- int len_user = 0;
- int len_password = 0;
- int len_program = 0;
- int len_directory = 0;
- int len_ip = 0;
- int len_dll = 0;
- char tmpdata[256];
+ unsigned int len_domain = 0;
+ unsigned int len_user = 0;
+ unsigned int len_password = 0;
+ unsigned int len_program = 0;
+ unsigned int len_directory = 0;
+ unsigned int len_clnt_addr = 0;
+ unsigned int len_clnt_dir = 0;
const char *sep;
- /* initialize (zero out) local variables */
- g_memset(tmpdata, 0, sizeof(char) * 256);
if (!s_check_rem_and_log(s, 8, "Parsing [MS-RDPBCGR] TS_INFO_PACKET"))
{
return 1;
@@ -1097,22 +1094,33 @@ xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s)
}
/* TS_EXTENDED_INFO_PACKET required fields */
in_uint8s(s, 2); /* clientAddressFamily */
- in_uint16_le(s, len_ip);
- if (ts_info_utf16_in(s, len_ip - 2, tmpdata, sizeof(tmpdata)) != 0)
+ in_uint16_le(s, len_clnt_addr);
+ if (len_clnt_addr > EXTENDED_INFO_MAX_CLIENT_ADDR_LENGTH ||
+ !s_check_rem(s, len_clnt_addr))
{
- LOG(LOG_LEVEL_ERROR, "ERROR reading ip");
+ LOG(LOG_LEVEL_ERROR, "clientAddress is too long (%u bytes)",
+ len_clnt_addr);
return 1;
}
+ // The clientAddress is currently unused. [MS-RDPBCGR] requires
+ // a mandatory null terminator, but some clients set
+ // len_clnt_addr == 0 if this field is missing. Allow for this
+ // in any future implementation.
+ in_uint8s(s, len_clnt_addr); // Skip Unicode clientAddress
+
if (!s_check_rem_and_log(s, 2, "Parsing [MS-RDPBCGR] TS_EXTENDED_INFO_PACKET clientDir"))
{
return 1;
}
- in_uint16_le(s, len_dll);
- if (ts_info_utf16_in(s, len_dll - 2, tmpdata, sizeof(tmpdata)) != 0)
+ in_uint16_le(s, len_clnt_dir);
+ if (len_clnt_dir > INFO_CLIENT_MAX_CB_LEN ||
+ !s_check_rem(s, len_clnt_dir))
{
- LOG(LOG_LEVEL_ERROR, "ERROR reading clientDir");
+ LOG(LOG_LEVEL_ERROR, "clientDir is too long (%u bytes)", len_clnt_dir);
return 1;
}
+ in_uint8s(s, len_clnt_dir); // Skip Unicode clientDir
+
LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_EXTENDED_INFO_PACKET "
"<Required Fields> clientAddressFamily (ignored), "
"cbClientAddress (ignored), clientAddress (ignored), " |
|
Looks good tested from macOS and iOS. Thanks. |
xrdp version
0.9.80
Detailed xrdp version, build options
Operating system & version
FreeBSD 13.2
Installation method
git clone & make install
Which backend do you use?
xorgxrdp
What desktop environment do you use?
fvwm3
Environment xrdp running on
FreeBSD 13.2 jail
What's your client?
macOS Microsoft Remote Desktop
Area(s) with issue?
No response
Steps to reproduce
RDP to the server with macOS MS Remote Desktop.app (Version 10.9.4 (2161))
✔️ Expected Behavior
To be presented with the RDP login screen.
❌ Actual Behavior
With macOS MS RDP application and devel 50cff2e fails to present the login screen with the following error in xrdp.log:
It appears if ts_info_utf16_in is called when src_bytes is 0 uni_len is -2... this didn't seem correct. Added a check to not perform term check if src_bytes is <= 0. This appears to allow the macOS MS RDP client to login however I'm not sure if this is a client bug or possible issue with ts_info_utf16_in or previous processing of ip data from the stream.
Anything else?
No response
The text was updated successfully, but these errors were encountered: