From 5cd36c511cab76ae4a7c14a792055449077e4fc1 Mon Sep 17 00:00:00 2001 From: bolkedebruin Date: Wed, 5 Aug 2020 17:12:16 +0200 Subject: [PATCH 1/4] Set max character buffer len to 512 per MS specification The MS specs determine that the character buffer lenngths for usernames, domains, passwords, alternate shells, etc can be up to 512 characters including the mandatory null terminator. --- common/xrdp_client_info.h | 12 +++++++----- common/xrdp_constants.h | 1 + libxrdp/xrdp_sec.c | 10 +++++----- neutrinordp/xrdp-neutrinordp.h | 7 ++++--- sesman/verify_user_pam.c | 11 +++++++---- xup/xup.h | 5 +++-- 6 files changed, 27 insertions(+), 19 deletions(-) diff --git a/common/xrdp_client_info.h b/common/xrdp_client_info.h index 297cdc55..fa7df6bf 100644 --- a/common/xrdp_client_info.h +++ b/common/xrdp_client_info.h @@ -18,6 +18,8 @@ * xrdp / xserver info / caps */ +#include "xrdp_constants.h" + #if !defined(XRDP_CLIENT_INFO_H) #define XRDP_CLIENT_INFO_H @@ -57,11 +59,11 @@ struct xrdp_client_info char hostname[32]; int build; int keylayout; - char username[256]; - char password[256]; - char domain[256]; - char program[256]; - char directory[256]; + char username[INFO_CLIENT_MAX_CB_LEN]; + char password[INFO_CLIENT_MAX_CB_LEN]; + char domain[INFO_CLIENT_MAX_CB_LEN]; + char program[INFO_CLIENT_MAX_CB_LEN]; + char directory[INFO_CLIENT_MAX_CB_LEN]; int rdp_compression; int rdp_autologin; int crypt_level; /* 1, 2, 3 = low, medium, high */ diff --git a/common/xrdp_constants.h b/common/xrdp_constants.h index 93ca2086..46fffeca 100644 --- a/common/xrdp_constants.h +++ b/common/xrdp_constants.h @@ -38,6 +38,7 @@ ******************************************************************************/ #define INFO_CLIENT_NAME_BYTES 32 +#define INFO_CLIENT_MAX_CB_LEN 512 #define XRDP_MAX_BITMAP_CACHE_ID 3 #define XRDP_MAX_BITMAP_CACHE_IDX 2000 diff --git a/libxrdp/xrdp_sec.c b/libxrdp/xrdp_sec.c index c0e513e4..4ce0d509 100644 --- a/libxrdp/xrdp_sec.c +++ b/libxrdp/xrdp_sec.c @@ -735,7 +735,7 @@ xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s) } in_uint16_le(s, len_domain); - if (len_domain > 511) + if (len_domain >= INFO_CLIENT_MAX_CB_LEN) { DEBUG(("ERROR [xrdp_sec_process_logon_info()]: len_domain > 511")); return 1; @@ -757,7 +757,7 @@ xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s) self->rdp_layer->client_info.rdp_autologin = 0; } - if (len_user > 511) + if (len_user >= INFO_CLIENT_MAX_CB_LEN) { DEBUG(("ERROR [xrdp_sec_process_logon_info()]: len_user > 511")); return 1; @@ -769,7 +769,7 @@ xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s) } in_uint16_le(s, len_password); - if (len_password > 511) + if (len_password >= INFO_CLIENT_MAX_CB_LEN) { DEBUG(("ERROR [xrdp_sec_process_logon_info()]: len_password > 511")); return 1; @@ -781,7 +781,7 @@ xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s) } in_uint16_le(s, len_program); - if (len_program > 511) + if (len_program >= INFO_CLIENT_MAX_CB_LEN) { DEBUG(("ERROR [xrdp_sec_process_logon_info()]: len_program > 511")); return 1; @@ -793,7 +793,7 @@ xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s) } in_uint16_le(s, len_directory); - if (len_directory > 511) + if (len_directory >= INFO_CLIENT_MAX_CB_LEN) { DEBUG(("ERROR [xrdp_sec_process_logon_info()]: len_directory > 511")); return 1; diff --git a/neutrinordp/xrdp-neutrinordp.h b/neutrinordp/xrdp-neutrinordp.h index b68eb43a..f50a299e 100644 --- a/neutrinordp/xrdp-neutrinordp.h +++ b/neutrinordp/xrdp-neutrinordp.h @@ -24,6 +24,7 @@ #include "defines.h" #include "xrdp_rail.h" #include "xrdp_client_info.h" +#include "xrdp_constants.h" /* this is the freerdp main header */ #include @@ -196,9 +197,9 @@ struct mod int vmaj; int vmin; int vrev; - char username[256]; - char password[256]; - char domain[256]; + char username[INFO_CLIENT_MAX_CB_LEN]; + char password[INFO_CLIENT_MAX_CB_LEN]; + char domain[INFO_CLIENT_MAX_CB_LEN]; int bool_keyBoardSynced ; /* Numlock can be out of sync, we hold state here to resolve */ int keyBoardLockInfo ; /* Holds initial numlock capslock state */ diff --git a/sesman/verify_user_pam.c b/sesman/verify_user_pam.c index 05e941bb..312bcbc8 100644 --- a/sesman/verify_user_pam.c +++ b/sesman/verify_user_pam.c @@ -34,10 +34,13 @@ #include #include +/* Defines the maximum size of a username or password. With pam there is no real limit */ +#define MAX_BUF 8192 + struct t_user_pass { - char user[256]; - char pass[256]; + char user[MAX_BUF]; + char pass[MAX_BUF]; }; struct t_auth_info @@ -115,8 +118,8 @@ auth_userpass(const char *user, const char *pass, int *errorcode) get_service_name(service_name); auth_info = g_new0(struct t_auth_info, 1); - g_strncpy(auth_info->user_pass.user, user, 255); - g_strncpy(auth_info->user_pass.pass, pass, 255); + g_strncpy(auth_info->user_pass.user, user, MAX_BUF - 1); + g_strncpy(auth_info->user_pass.pass, pass, MAX_BUF - 1); auth_info->pamc.conv = &verify_pam_conv; auth_info->pamc.appdata_ptr = &(auth_info->user_pass); error = pam_start(service_name, 0, &(auth_info->pamc), &(auth_info->ph)); diff --git a/xup/xup.h b/xup/xup.h index ddd24e36..f7de3a17 100644 --- a/xup/xup.h +++ b/xup/xup.h @@ -24,6 +24,7 @@ #include "os_calls.h" #include "defines.h" #include "xrdp_client_info.h" +#include "xrdp_constants.h" #include "xrdp_rail.h" #define CURRENT_MOD_VER 4 @@ -154,8 +155,8 @@ struct mod int height; int bpp; int sck_closed; - char username[256]; - char password[256]; + char username[INFO_CLIENT_MAX_CB_LEN]; + char password[INFO_CLIENT_MAX_CB_LEN]; char ip[256]; char port[256]; int shift_state; From 47e1c5d35971a1f940afbaa7f0537728d31a8edc Mon Sep 17 00:00:00 2001 From: Bolke de Bruin Date: Wed, 19 Aug 2020 12:50:01 +0200 Subject: [PATCH 2/4] Add description --- common/xrdp_constants.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/common/xrdp_constants.h b/common/xrdp_constants.h index 46fffeca..50be8536 100644 --- a/common/xrdp_constants.h +++ b/common/xrdp_constants.h @@ -38,6 +38,10 @@ ******************************************************************************/ #define INFO_CLIENT_NAME_BYTES 32 +/** + * Maximum length of a string (two bytes + len), excluding the terminator + * [MS-RDPBCGR] TS_INFO_PACKET(2.2.1.11.1.1) + */ #define INFO_CLIENT_MAX_CB_LEN 512 #define XRDP_MAX_BITMAP_CACHE_ID 3 From 4d7b916fafd43867eebc9d805013ba9af3096b21 Mon Sep 17 00:00:00 2001 From: Bolke de Bruin Date: Fri, 21 Aug 2020 09:19:49 +0200 Subject: [PATCH 3/4] Improve description --- common/xrdp_constants.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/xrdp_constants.h b/common/xrdp_constants.h index 50be8536..7b777e3d 100644 --- a/common/xrdp_constants.h +++ b/common/xrdp_constants.h @@ -39,7 +39,7 @@ #define INFO_CLIENT_NAME_BYTES 32 /** - * Maximum length of a string (two bytes + len), excluding the terminator + * Maximum length of a string including the mandatory null terminator * [MS-RDPBCGR] TS_INFO_PACKET(2.2.1.11.1.1) */ #define INFO_CLIENT_MAX_CB_LEN 512 From e89f124afe1798c923423f4b7fa6d67df7950d56 Mon Sep 17 00:00:00 2001 From: Bolke de Bruin Date: Fri, 21 Aug 2020 14:29:01 +0200 Subject: [PATCH 4/4] Ensure copying of the whole username/password --- xup/xup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xup/xup.c b/xup/xup.c index e6321651..a235194b 100644 --- a/xup/xup.c +++ b/xup/xup.c @@ -1546,11 +1546,11 @@ lib_mod_set_param(struct mod *mod, const char *name, const char *value) { if (g_strcasecmp(name, "username") == 0) { - g_strncpy(mod->username, value, 255); + g_strncpy(mod->username, value, INFO_CLIENT_MAX_CB_LEN-1); } else if (g_strcasecmp(name, "password") == 0) { - g_strncpy(mod->password, value, 255); + g_strncpy(mod->password, value, INFO_CLIENT_MAX_CB_LEN-1); } else if (g_strcasecmp(name, "ip") == 0) {