From 85e74baf934fedc323f9a9b99f536cf3b549a99b Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Sat, 29 Aug 2026 22:19:33 -0500 Subject: [PATCH] fix(pdu): retain unknown GCC flag bits instead of failing the settings exchange Three GCC settings-block decoders rejected the whole Basic Settings Exchange when a flags field carried a bit outside the defined list: RedirectionFlags in Client Cluster Data, the per-monitor flags in Client Monitor Data, and transportFlags in the Multitransport Channel Data. [MS-RDPBCGR] 3.3.5.3.3's processing rules ask the server to validate the Client Network Data bounds among the settings blocks and nothing else, so the strictness was this library's own, and it failed the connection at GCC, before any channel was joined. The exposure is real in each case. The transportFlags list has grown before (TRANSPORTTYPE_UDP_PREFERRED and SOFTSYNC_TCP_TO_UDP are later additions), so an older IronRDP would have refused every client that advertised them. The monitor flags list defines only TS_MONITOR_PRIMARY, so any vendor or future bit refused a multimon client outright. The redirection flags sit next to a version mask that already anticipates growth. Switch all three sites to from_bits_retain, the crate-wide policy since #1144: unknown bits are kept rather than dropped, so re-encoding a decoded block reproduces the wire value. The cluster version mask handling is untouched, and the separately parsed RedirectionVersion stays strict since it is a discriminant, not a bit set. This continues the interop line of #1489 (unknown GCC user-data blocks), #1458 (unknown security header flags), #1837 (undefined channel option bits) and #1843 (unknown ClientInfoFlags bits). --- crates/ironrdp-pdu/src/gcc/cluster_data.rs | 7 ++- crates/ironrdp-pdu/src/gcc/monitor_data.rs | 7 ++- .../src/gcc/multi_transport_channel_data.rs | 12 +++-- .../ironrdp-testsuite-core/tests/pdu/gcc.rs | 54 +++++++++++++++++++ 4 files changed, 71 insertions(+), 9 deletions(-) diff --git a/crates/ironrdp-pdu/src/gcc/cluster_data.rs b/crates/ironrdp-pdu/src/gcc/cluster_data.rs index 9e2f499b6d..6f5c6ef4b9 100644 --- a/crates/ironrdp-pdu/src/gcc/cluster_data.rs +++ b/crates/ironrdp-pdu/src/gcc/cluster_data.rs @@ -52,8 +52,11 @@ impl<'de> Decode<'de> for ClientClusterData { let flags_with_version = src.read_u32(); let redirected_session_id = src.read_u32(); - let flags = RedirectionFlags::from_bits(flags_with_version & !REDIRECTION_VERSION_MASK) - .ok_or_else(|| invalid_field_err!("flags", "invalid redirection flags", in: src))?; + // [MS-RDPBCGR] 3.3.5.3.3 asks the server to validate only the Client + // Network Data bounds among the settings blocks, never this bit set; + // retain unknown bits (crate-wide policy since #1144) rather than + // refusing the client at GCC. + let flags = RedirectionFlags::from_bits_retain(flags_with_version & !REDIRECTION_VERSION_MASK); let redirection_version = RedirectionVersion::from_u32((flags_with_version & REDIRECTION_VERSION_MASK) >> 2) .ok_or_else(|| invalid_field_err!("redirVersion", "invalid redirection version", in: src))?; diff --git a/crates/ironrdp-pdu/src/gcc/monitor_data.rs b/crates/ironrdp-pdu/src/gcc/monitor_data.rs index cbb3243a49..d60f6cbdca 100644 --- a/crates/ironrdp-pdu/src/gcc/monitor_data.rs +++ b/crates/ironrdp-pdu/src/gcc/monitor_data.rs @@ -116,8 +116,11 @@ impl<'de> Decode<'de> for Monitor { let top = src.read_i32(); let right = src.read_i32(); let bottom = src.read_i32(); - let flags = MonitorFlags::from_bits(src.read_u32()) - .ok_or_else(|| invalid_field_err!("flags", "invalid monitor flags", in: src))?; + // [MS-RDPBCGR] 2.2.1.3.6.1 defines only TS_MONITOR_PRIMARY here, and + // 3.3.5.3.3 never asks the server to validate the bit set; retain + // unknown bits (crate-wide policy since #1144) rather than refusing a + // multimon client at GCC. + let flags = MonitorFlags::from_bits_retain(src.read_u32()); Ok(Self { left, diff --git a/crates/ironrdp-pdu/src/gcc/multi_transport_channel_data.rs b/crates/ironrdp-pdu/src/gcc/multi_transport_channel_data.rs index 0949c0a04c..c5d5806bb1 100644 --- a/crates/ironrdp-pdu/src/gcc/multi_transport_channel_data.rs +++ b/crates/ironrdp-pdu/src/gcc/multi_transport_channel_data.rs @@ -1,7 +1,5 @@ use bitflags::bitflags; -use ironrdp_core::{ - Decode, DecodeResult, Encode, EncodeResult, ReadCursor, WriteCursor, ensure_fixed_part_size, invalid_field_err, -}; +use ironrdp_core::{Decode, DecodeResult, Encode, EncodeResult, ReadCursor, WriteCursor, ensure_fixed_part_size}; #[derive(Debug, Clone, PartialEq, Eq)] #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] @@ -37,8 +35,12 @@ impl<'de> Decode<'de> for MultiTransportChannelData { fn decode(src: &mut ReadCursor<'de>) -> DecodeResult { ensure_fixed_part_size!(in: src); - let flags = MultiTransportFlags::from_bits(src.read_u32()) - .ok_or_else(|| invalid_field_err!("flags", "invalid multitransport flags", in: src))?; + // [MS-RDPBCGR] 2.2.1.3.8's transportFlags list has grown before + // (UDP_PREFERRED and SOFTSYNC_TCP_TO_UDP are later additions), and + // 3.3.5.3.3 never asks the receiver to validate it; retain unknown + // bits (crate-wide policy since #1144) rather than failing the GCC + // exchange. + let flags = MultiTransportFlags::from_bits_retain(src.read_u32()); Ok(Self { flags }) } diff --git a/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs b/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs index e34672339c..acde4242cb 100644 --- a/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs +++ b/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs @@ -248,6 +248,23 @@ fn from_buffer_correctly_parses_client_cluster_data() { assert_eq!(*CLUSTER_DATA, decode(buffer).unwrap()); } +#[test] +fn cluster_data_with_undefined_flag_bits_decodes_and_retains_them() { + // [MS-RDPBCGR] 3.3.5.3.3 never asks the server to validate this bit set; + // an unknown flag must not fail the GCC exchange. The bit is retained so + // re-encoding preserves the wire value. + const UNDEFINED_BIT: u32 = 0x0000_0080; // undefined in 2.2.1.3.5 + + let mut buffer = CLUSTER_DATA_BUFFER.to_vec(); + let flags = u32::from_le_bytes(buffer[0..4].try_into().unwrap()) | UNDEFINED_BIT; + buffer[0..4].copy_from_slice(&flags.to_le_bytes()); + + let data: ClientClusterData = decode(buffer.as_slice()).expect("undefined redirection bits are not fatal"); + + assert_ne!(data.flags.bits() & UNDEFINED_BIT, 0); + assert_eq!(encode_vec(&data).unwrap(), buffer); +} + #[test] fn to_buffer_correctly_serializes_client_cluster_data() { let data = CLUSTER_DATA.clone(); @@ -555,6 +572,25 @@ fn from_buffer_correctly_parses_client_monitor_data_with_monitors() { ); } +#[test] +fn monitor_with_undefined_flag_bits_decodes_and_retains_them() { + // [MS-RDPBCGR] 2.2.1.3.6.1 defines only TS_MONITOR_PRIMARY; a vendor or + // future bit must not refuse a multimon client at GCC. The bit is + // retained so re-encoding preserves the wire value. + const UNDEFINED_BIT: u32 = 0x0000_0002; // undefined in 2.2.1.3.6.1 + + let mut buffer = ironrdp_testsuite_core::monitor_data::MONITOR_DATA_WITH_MONITORS_BUFFER.to_vec(); + // First monitor starts after the 8-byte header; its flags are the fifth + // dword of the 20-byte TS_MONITOR_DEF. + let flags = u32::from_le_bytes(buffer[24..28].try_into().unwrap()) | UNDEFINED_BIT; + buffer[24..28].copy_from_slice(&flags.to_le_bytes()); + + let data: ClientMonitorData = decode(buffer.as_slice()).expect("undefined monitor bits are not fatal"); + + assert_ne!(data.monitors[0].flags.bits() & UNDEFINED_BIT, 0); + assert_eq!(encode_vec(&data).unwrap(), buffer); +} + #[test] fn to_buffer_correctly_serializes_client_monitor_data_without_monitors() { let data = ironrdp_testsuite_core::monitor_data::MONITOR_DATA_WITHOUT_MONITORS.clone(); @@ -666,6 +702,24 @@ fn from_buffer_correctly_parses_server_multi_transport_channel_data() { ); } +#[test] +fn multi_transport_with_undefined_flag_bits_decodes_and_retains_them() { + // [MS-RDPBCGR] 2.2.1.3.8's transportFlags list has grown before; an + // unknown bit must not fail the GCC exchange. The bit is retained so + // re-encoding preserves the wire value. + const UNDEFINED_BIT: u32 = 0x0000_0002; // undefined in 2.2.1.3.8 + + let mut buffer = + ironrdp_testsuite_core::multi_transport_channel_data::SERVER_GCC_MULTI_TRANSPORT_CHANNEL_BLOCK_BUFFER.to_vec(); + let flags = u32::from_le_bytes(buffer[0..4].try_into().unwrap()) | UNDEFINED_BIT; + buffer[0..4].copy_from_slice(&flags.to_le_bytes()); + + let data: MultiTransportChannelData = decode(buffer.as_slice()).expect("undefined transport bits are not fatal"); + + assert_ne!(data.flags.bits() & UNDEFINED_BIT, 0); + assert_eq!(encode_vec(&data).unwrap(), buffer); +} + #[test] fn to_buffer_correctly_serializes_server_multi_transport_channel_data() { let data = ironrdp_testsuite_core::multi_transport_channel_data::SERVER_GCC_MULTI_TRANSPORT_CHANNEL_BLOCK.clone();