From 885d3aa56fae68b5b47b825bc3e1955e6c807f02 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 13 Mar 2025 11:12:44 +0100 Subject: [PATCH 1/7] [codec,region] fix noinline warning --- libfreerdp/codec/region.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libfreerdp/codec/region.c b/libfreerdp/codec/region.c index 73c8b2f3c..334888589 100644 --- a/libfreerdp/codec/region.c +++ b/libfreerdp/codec/region.c @@ -204,7 +204,7 @@ void region16_clear(REGION16* region) } WINPR_ATTR_MALLOC(freeRegion, 1) -static INLINE REGION16_DATA* allocateRegion(size_t nbItems) +static REGION16_DATA* allocateRegion(size_t nbItems) { REGION16_DATA* data = calloc(1, sizeof(REGION16_DATA)); if (!data) From 3ecbb7120e6da586e41a27860bb5abc39e80c98f Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 13 Mar 2025 10:52:22 +0100 Subject: [PATCH 2/7] [core,gateway] add rts parser checks Add some additional checks to [MS-RPCH] command parsing --- libfreerdp/core/gateway/rts.c | 132 ++++++++++++++++++++++------------ 1 file changed, 88 insertions(+), 44 deletions(-) diff --git a/libfreerdp/core/gateway/rts.c b/libfreerdp/core/gateway/rts.c index 67258f026..1677f9f2a 100644 --- a/libfreerdp/core/gateway/rts.c +++ b/libfreerdp/core/gateway/rts.c @@ -591,12 +591,6 @@ static BOOL rts_write_syntax_id(wStream* s, const p_syntax_id_t* syntax_id) return TRUE; } -static p_cont_elem_t* rts_context_elem_new(size_t count) -{ - p_cont_elem_t* ctx = calloc(count, sizeof(p_cont_elem_t)); - return ctx; -} - static void rts_context_elem_free(p_cont_elem_t* ptr) { if (!ptr) @@ -605,6 +599,13 @@ static void rts_context_elem_free(p_cont_elem_t* ptr) free(ptr); } +WINPR_ATTR_MALLOC(rts_context_elem_free, 1) +static p_cont_elem_t* rts_context_elem_new(size_t count) +{ + p_cont_elem_t* ctx = calloc(count, sizeof(p_cont_elem_t)); + return ctx; +} + static BOOL rts_read_context_elem(wStream* s, p_cont_elem_t* element, BOOL silent) { WINPR_ASSERT(s); @@ -1240,21 +1241,32 @@ static BOOL rts_write_pdu_header(wStream* s, const rpcconn_rts_hdr_t* header) return TRUE; } -static BOOL rts_receive_window_size_command_read(WINPR_ATTR_UNUSED rdpRpc* rpc, wStream* buffer, - UINT64* ReceiveWindowSize) +/* [MS-RPCH] 2.2.3.5.1 ReceiveWindowSize */ +static BOOL rts_receive_window_size_command_read(rdpRpc* rpc, wStream* buffer, + UINT32* ReceiveWindowSize) { WINPR_ASSERT(rpc); WINPR_ASSERT(buffer); if (!Stream_CheckAndLogRequiredLength(TAG, buffer, 8)) return FALSE; - const UINT64 val = Stream_Get_UINT64(buffer); + const uint32_t CommandType = Stream_Get_UINT32(buffer); + if (CommandType != RTS_CMD_RECEIVE_WINDOW_SIZE) + { + WLog_Print(rpc->log, WLOG_ERROR, + "[MS-RPCH] 2.2.3.5.1 ReceiveWindowSize::CommandType must be 0x08" PRIx32 ", got " + "0x%08" PRIx32, + RTS_CMD_RECEIVE_WINDOW_SIZE, CommandType); + return FALSE; + } + const UINT32 val = Stream_Get_UINT32(buffer); if (ReceiveWindowSize) - *ReceiveWindowSize = val; /* ReceiveWindowSize (8 bytes) */ + *ReceiveWindowSize = val; /* ReceiveWindowSize (4 bytes) */ return TRUE; } +/* [MS-RPCH] 2.2.3.5.1 ReceiveWindowSize */ static BOOL rts_receive_window_size_command_write(wStream* s, UINT32 ReceiveWindowSize) { WINPR_ASSERT(s); @@ -1268,6 +1280,7 @@ static BOOL rts_receive_window_size_command_write(wStream* s, UINT32 ReceiveWind return TRUE; } +/* [MS-RPCH] 2.2.3.5.2 FlowControlAck */ static int rts_flow_control_ack_command_read(rdpRpc* rpc, wStream* buffer, UINT32* BytesReceived, UINT32* AvailableWindow, BYTE* ChannelCookie) { @@ -1310,6 +1323,7 @@ static int rts_flow_control_ack_command_read(rdpRpc* rpc, wStream* buffer, UINT3 return 24; } +/* [MS-RPCH] 2.2.3.5.2 FlowControlAck */ static BOOL rts_flow_control_ack_command_write(wStream* s, UINT32 BytesReceived, UINT32 AvailableWindow, BYTE* ChannelCookie) { @@ -1326,8 +1340,9 @@ static BOOL rts_flow_control_ack_command_write(wStream* s, UINT32 BytesReceived, return TRUE; } +/* [MS-RPCH] 2.2.3.5.3 ConnectionTimeout */ static BOOL rts_connection_timeout_command_read(WINPR_ATTR_UNUSED rdpRpc* rpc, wStream* buffer, - UINT64* ConnectionTimeout) + UINT32* ConnectionTimeout) { WINPR_ASSERT(rpc); WINPR_ASSERT(buffer); @@ -1335,9 +1350,18 @@ static BOOL rts_connection_timeout_command_read(WINPR_ATTR_UNUSED rdpRpc* rpc, w if (!Stream_CheckAndLogRequiredLength(TAG, buffer, 8)) return FALSE; - UINT64 val = Stream_Get_UINT64(buffer); + const uint32_t CommandType = Stream_Get_UINT32(buffer); + if (CommandType != RTS_CMD_CONNECTION_TIMEOUT) + { + WLog_Print(rpc->log, WLOG_ERROR, + "[MS-RPCH] 2.2.3.5.3 ConnectionTimeout::CommandType must be 0x08" PRIx32 ", got " + "0x%08" PRIx32, + RTS_CMD_CONNECTION_TIMEOUT, CommandType); + return FALSE; + } + const UINT32 val = Stream_Get_UINT32(buffer); if (ConnectionTimeout) - *ConnectionTimeout = val; /* ConnectionTimeout (8 bytes) */ + *ConnectionTimeout = val; /* ConnectionTimeout (4 bytes) */ return TRUE; } @@ -1385,19 +1409,38 @@ static BOOL rts_client_keepalive_command_write(wStream* s, UINT32 ClientKeepaliv return TRUE; } -static BOOL rts_version_command_read(WINPR_ATTR_UNUSED rdpRpc* rpc, wStream* buffer) +/* [MS-RPCH] 2.2.3.5.7 Version */ +static BOOL rts_version_command_read(rdpRpc* rpc, wStream* buffer, uint32_t* pversion) { WINPR_ASSERT(rpc); WINPR_ASSERT(buffer); - if (!Stream_SafeSeek(buffer, 8)) + if (!Stream_EnsureRemainingCapacity(buffer, 8)) return FALSE; - /* command (4 bytes) */ - /* Version (4 bytes) */ + const uint32_t CommandType = Stream_Get_UINT32(buffer); /* CommandType (4 bytes) */ + if (CommandType != RTS_CMD_VERSION) + { + WLog_Print(rpc->log, WLOG_ERROR, + "[MS-RPCH] 2.2.3.5.7 Version::CommandType must be 0x08" PRIx32 ", got " + "0x%08" PRIx32, + RTS_CMD_VERSION, CommandType); + return FALSE; + } + const uint32_t version = Stream_Get_UINT32(buffer); /* Version (4 bytes) */ + if (version != 1) + { + WLog_Print(rpc->log, WLOG_WARN, + "[MS-RPCH] 2.2.3.5.7 Version::Version should be 0x00000001, got 0x%08" PRIx32, + version); + } + if (pversion) + *pversion = version; + return TRUE; } +/* [MS-RPCH] 2.2.3.5.7 Version */ static BOOL rts_version_command_write(wStream* buffer) { WINPR_ASSERT(buffer); @@ -1603,24 +1646,21 @@ fail: BOOL rts_recv_CONN_A3_pdu(rdpRpc* rpc, wStream* buffer) { - BOOL rc = 0; - UINT64 ConnectionTimeout = 0; + UINT32 ConnectionTimeout = 0; if (!Stream_SafeSeek(buffer, 20)) return FALSE; - rc = rts_connection_timeout_command_read(rpc, buffer, &ConnectionTimeout); - if (!rc || (ConnectionTimeout > UINT32_MAX)) - return rc; + if (!rts_connection_timeout_command_read(rpc, buffer, &ConnectionTimeout)) + return FALSE; - WLog_DBG(TAG, "Receiving CONN/A3 RTS PDU: ConnectionTimeout: %" PRIu64 "", ConnectionTimeout); + WLog_DBG(TAG, "Receiving CONN/A3 RTS PDU: ConnectionTimeout: %" PRIu32 "", ConnectionTimeout); WINPR_ASSERT(rpc); WINPR_ASSERT(rpc->VirtualConnection); WINPR_ASSERT(rpc->VirtualConnection->DefaultInChannel); - rpc->VirtualConnection->DefaultInChannel->PingOriginator.ConnectionTimeout = - (UINT32)ConnectionTimeout; + rpc->VirtualConnection->DefaultInChannel->PingOriginator.ConnectionTimeout = ConnectionTimeout; return TRUE; } @@ -1682,32 +1722,32 @@ fail: return status; } -/* CONN/C Sequence */ +/* [MS-RPCH] 2.2.4.9 CONN/C2 RTS PDU */ BOOL rts_recv_CONN_C2_pdu(rdpRpc* rpc, wStream* buffer) { BOOL rc = FALSE; - UINT64 ReceiveWindowSize = 0; - UINT64 ConnectionTimeout = 0; + UINT32 ReceiveWindowSize = 0; + UINT32 ConnectionTimeout = 0; WINPR_ASSERT(rpc); WINPR_ASSERT(buffer); - if (!Stream_SafeSeek(buffer, 20)) - return FALSE; + rpcconn_hdr_t header = { 0 }; + if (!rts_read_pdu_header(buffer, &header)) + goto fail; - rc = rts_version_command_read(rpc, buffer); - if (!rc) - return rc; - rc = rts_receive_window_size_command_read(rpc, buffer, &ReceiveWindowSize); - if (!rc || (ReceiveWindowSize > UINT32_MAX)) - return rc; - rc = rts_connection_timeout_command_read(rpc, buffer, &ConnectionTimeout); - if (!rc || (ConnectionTimeout > UINT32_MAX)) - return rc; + if (!rts_version_command_read(rpc, buffer, NULL)) + goto fail; + + if (!rts_receive_window_size_command_read(rpc, buffer, &ReceiveWindowSize)) + goto fail; + + if (!rts_connection_timeout_command_read(rpc, buffer, &ConnectionTimeout)) + goto fail; WLog_DBG(TAG, - "Receiving CONN/C2 RTS PDU: ConnectionTimeout: %" PRIu64 " ReceiveWindowSize: %" PRIu64 + "Receiving CONN/C2 RTS PDU: ConnectionTimeout: %" PRIu32 " ReceiveWindowSize: %" PRIu32 "", ConnectionTimeout, ReceiveWindowSize); @@ -1715,10 +1755,14 @@ BOOL rts_recv_CONN_C2_pdu(rdpRpc* rpc, wStream* buffer) WINPR_ASSERT(rpc->VirtualConnection); WINPR_ASSERT(rpc->VirtualConnection->DefaultInChannel); - rpc->VirtualConnection->DefaultInChannel->PingOriginator.ConnectionTimeout = - (UINT32)ConnectionTimeout; - rpc->VirtualConnection->DefaultInChannel->PeerReceiveWindow = (UINT32)ReceiveWindowSize; - return TRUE; + rpc->VirtualConnection->DefaultInChannel->PingOriginator.ConnectionTimeout = ConnectionTimeout; + rpc->VirtualConnection->DefaultInChannel->PeerReceiveWindow = ReceiveWindowSize; + + rc = TRUE; + +fail: + rts_free_pdu_header(&header, FALSE); + return rc; } /* Out-of-Sequence PDUs */ From 37b5fa6562846e50f98bd096135fc9a18b17a015 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 13 Mar 2025 11:39:15 +0100 Subject: [PATCH 3/7] [codec,dsp] fix missing integer casts --- libfreerdp/codec/dsp.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/libfreerdp/codec/dsp.c b/libfreerdp/codec/dsp.c index 31d30e8e2..cf0d70a09 100644 --- a/libfreerdp/codec/dsp.c +++ b/libfreerdp/codec/dsp.c @@ -624,7 +624,8 @@ static BOOL freerdp_dsp_decode_opus(FREERDP_DSP_CONTEXT* WINPR_RESTRICT context, if (!Stream_EnsureRemainingCapacity(context->common.buffer, max_size)) return FALSE; - frames = opus_decode(context->opus_decoder, src, size, Stream_Pointer(out), OPUS_MAX_FRAMES, 0); + frames = opus_decode(context->opus_decoder, src, WINPR_ASSERTING_INT_CAST(opus_int32, size), + Stream_Pointer(out), OPUS_MAX_FRAMES, 0); if (frames < 0) return FALSE; @@ -645,10 +646,11 @@ static BOOL freerdp_dsp_encode_opus(FREERDP_DSP_CONTEXT* WINPR_RESTRICT context, if (!Stream_EnsureRemainingCapacity(context->common.buffer, max_size)) return FALSE; - const int src_frames = size / sizeof(opus_int16) / context->common.format.nChannels; + const size_t src_frames = size / sizeof(opus_int16) / context->common.format.nChannels; const opus_int16* src_data = (const opus_int16*)src; - const int frames = - opus_encode(context->opus_encoder, src_data, src_frames, Stream_Pointer(out), max_size); + const int frames = opus_encode( + context->opus_encoder, src_data, WINPR_ASSERTING_INT_CAST(opus_int32, src_frames), + Stream_Pointer(out), WINPR_ASSERTING_INT_CAST(opus_int32, max_size)); if (frames < 0) return FALSE; return Stream_SafeSeek(out, frames * context->common.format.nChannels * sizeof(int16_t)); From d42cdd656dd2e55706299f3955ad73074a6d708e Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 13 Mar 2025 11:47:14 +0100 Subject: [PATCH 4/7] [winpr,utils] limit debug string length --- winpr/libwinpr/utils/debug.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/winpr/libwinpr/utils/debug.c b/winpr/libwinpr/utils/debug.c index 96890f84d..5d6a337b3 100644 --- a/winpr/libwinpr/utils/debug.c +++ b/winpr/libwinpr/utils/debug.c @@ -58,7 +58,9 @@ WINPR_PRAGMA_DIAG_POP #define TAG "com.winpr.utils.debug" -static const char* support_msg = "Invalid stacktrace buffer! check if platform is supported!"; +#define LINE_LENGTH_MAX 2048 + +static const char support_msg[] = "Invalid stacktrace buffer! check if platform is supported!"; void winpr_backtrace_free(void* buffer) { @@ -93,7 +95,7 @@ void* winpr_backtrace(DWORD size) WLog_FATAL(TAG, "%s", support_msg); /* return a non NULL buffer to allow the backtrace function family to succeed without failing */ - return _strdup(support_msg); + return strndup(support_msg, sizeof(support_msg)); #endif } @@ -125,7 +127,7 @@ char** winpr_backtrace_symbols(void* buffer, size_t* used) * 2. The first sizeof(char*) bytes contain the pointer to the string following the pointer. * 3. The at data + sizeof(char*) contains the actual string */ - size_t len = strlen(support_msg); + size_t len = strnlen(support_msg, sizeof(support_msg)); char* ppmsg = calloc(sizeof(char*) + len + 1, sizeof(char)); if (!ppmsg) return NULL; @@ -158,7 +160,7 @@ void winpr_backtrace_symbols_fd(void* buffer, int fd) return; for (size_t i = 0; i < used; i++) - (void)_write(fd, lines[i], (unsigned)strnlen(lines[i], UINT32_MAX)); + (void)_write(fd, lines[i], (unsigned)strnlen(lines[i], LINE_LENGTH_MAX)); free((void*)lines); } #else From 264e8e8ab22a78d53e275bf4882ab4a2b567a406 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 13 Mar 2025 11:53:10 +0100 Subject: [PATCH 5/7] [core,info] limit user and domain length to UNLEN --- libfreerdp/core/info.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libfreerdp/core/info.c b/libfreerdp/core/info.c index 2f8e6694c..835718b77 100644 --- a/libfreerdp/core/info.c +++ b/libfreerdp/core/info.c @@ -1451,11 +1451,11 @@ static BOOL rdp_write_logon_info_v2(wStream* s, logon_info* info) */ Stream_Write_UINT32(s, logonInfoV2Size); Stream_Write_UINT32(s, info->sessionId); - domainLen = strnlen(info->domain, UINT32_MAX); + domainLen = strnlen(info->domain, 256); /* lmcons.h UNLEN */ if (domainLen >= UINT32_MAX / sizeof(WCHAR)) return FALSE; Stream_Write_UINT32(s, (UINT32)(domainLen + 1) * sizeof(WCHAR)); - usernameLen = strnlen(info->username, UINT32_MAX); + usernameLen = strnlen(info->username, 256); /* lmcons.h UNLEN */ if (usernameLen >= UINT32_MAX / sizeof(WCHAR)) return FALSE; Stream_Write_UINT32(s, (UINT32)(usernameLen + 1) * sizeof(WCHAR)); From da049b2f96beded13f891623dd136093ca6621c1 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 13 Mar 2025 11:59:20 +0100 Subject: [PATCH 6/7] [winpr,registry] limit REG_SZ value length Add a (sensible) upper limit for entries and add a warning should some value be truncated --- winpr/libwinpr/registry/registry.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/winpr/libwinpr/registry/registry.c b/winpr/libwinpr/registry/registry.c index 761572f33..89c41fcb5 100644 --- a/winpr/libwinpr/registry/registry.c +++ b/winpr/libwinpr/registry/registry.c @@ -35,6 +35,7 @@ #include #include +#include #include "registry_reg.h" @@ -43,6 +44,24 @@ static Reg* instance = NULL; +static size_t regsz_length(const char* key, const void* value, bool unicode) +{ + /* https://learn.microsoft.com/en-us/windows/win32/sysinfo/registry-element-size-limits + * + * while not strictly limited to this size larger values should be stored to a + * file. + */ + const size_t limit = 16383; + size_t length = 0; + if (unicode) + length = _wcsnlen((const WCHAR*)value, limit); + else + length = strnlen((const char*)value, limit); + if (length >= limit) + WLog_WARN(TAG, "REG_SZ[%s] truncated to size %" PRIuz, key, length); + return length; +} + static Reg* RegGetInstance(void) { if (!instance) @@ -436,7 +455,8 @@ LONG RegQueryValueExW(HKEY hKey, LPCWSTR lpValueName, LPDWORD lpReserved, LPDWOR goto end; case REG_SZ: { - const size_t length = strnlen(pValue->data.string, UINT32_MAX) * sizeof(WCHAR); + const size_t length = + regsz_length(pValue->name, pValue->data.string, TRUE) * sizeof(WCHAR); status = ERROR_SUCCESS; if (lpData != NULL) @@ -508,7 +528,8 @@ LONG RegQueryValueExA(HKEY hKey, LPCSTR lpValueName, LPDWORD lpReserved, LPDWORD return reg_read_int(pValue, lpData, lpcbData); case REG_SZ: { - const size_t length = strnlen(pValue->data.string, UINT32_MAX); + const size_t length = regsz_length(pValue->name, pValue->data.string, FALSE); + char* pData = (char*)lpData; if (pData != NULL) From ee7e6075623d96f5d46a314c2f892bddf5ac6e20 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 13 Mar 2025 12:18:17 +0100 Subject: [PATCH 7/7] [channels,rdpear] Enforce kerberos string length limits --- channels/rdpear/client/rdpear_main.c | 30 ++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/channels/rdpear/client/rdpear_main.c b/channels/rdpear/client/rdpear_main.c index 32f07e4e6..31b7cc25b 100644 --- a/channels/rdpear/client/rdpear_main.c +++ b/channels/rdpear/client/rdpear_main.c @@ -42,6 +42,10 @@ #define TAG CHANNELS_TAG("rdpear.client") +#ifndef MAX_KEYTAB_NAME_LEN +#define MAX_KEYTAB_NAME_LEN 1100 /* Defined in MIT krb5.h */ +#endif + /* defined in libkrb5 */ krb5_error_code encode_krb5_authenticator(const krb5_authenticator* rep, krb5_data** code_out); krb5_error_code encode_krb5_ap_rep(const krb5_ap_rep* rep, krb5_data** code_out); @@ -515,15 +519,33 @@ static BOOL rdpear_kerb_CreateApReqAuthenticator(RDPEAR_PLUGIN* rdpear, NdrConte for (int i = 0; i < client.length; i++) { - client.data[i].data = KERB_RPC_UNICODESTR_to_charptr(&req.ClientName->Names[i]); - if (!client.data[i].data) + krb5_data* cur = &client.data[i]; + cur->data = KERB_RPC_UNICODESTR_to_charptr(&req.ClientName->Names[i]); + if (!cur->data) goto out; - client.data[i].length = (unsigned int)strnlen(client.data[i].data, UINT32_MAX); + const size_t len = strnlen(cur->data, MAX_KEYTAB_NAME_LEN + 1); + if (len > MAX_KEYTAB_NAME_LEN) + { + WLog_ERR(TAG, + "Invalid ClientName length %" PRIuz + ", limited to %u characters. ClientName: (%s)", + MAX_KEYTAB_NAME_LEN, len, cur->data); + goto out; + } + cur->length = (unsigned int)len; } client.realm.data = KERB_RPC_UNICODESTR_to_charptr(req.ClientRealm); if (!client.realm.data) goto out; - client.realm.length = (unsigned int)strnlen(client.realm.data, UINT32_MAX); + + const size_t len = strnlen(client.realm.data, MAX_KEYTAB_NAME_LEN + 1); + if (len > MAX_KEYTAB_NAME_LEN) + { + WLog_ERR(TAG, "Invalid realm length %" PRIuz ", limited to %u characters. Realm: (%s)", + MAX_KEYTAB_NAME_LEN, len, client.realm.data); + goto out; + } + client.realm.length = (unsigned int)len; authent.client = &client; krb5_checksum checksum;