From e3fc954e2642b31b3b455ad4cc6088b28bc67f83 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Sat, 29 Aug 2026 22:58:07 -0500 Subject: [PATCH] fix(pdu): retain unknown protocol bits in the GCC negotiation echo fields The two GCC core-data fields that echo the X.224 negotiation values are decoded strictly: serverSelectedProtocol in the client core data and clientRequestedProtocols in the server core data both rejected the whole settings exchange when the value carried a bit outside the five defined PROTOCOL_* flags. The negotiation layer itself already decodes this exact type with from_bits_retain in both the RDP Negotiation Request and Response, so a value the library accepted leniently at X.224 became connection-fatal when echoed back one phase later. The strictness also worked against the field's purpose. [MS-RDPBCGR] 3.3.5.3.3's mandated handling of serverSelectedProtocol is a value comparison: if the field does not contain the same value the server transmitted in the RDP Negotiation Response, the server SHOULD drop the connection. That comparison needs the value to survive decode; rejecting unknown bits at parse time replaces the spec's check with a decode error. The client-side echo is symmetric: the client knows what it requested and compares, rather than validating the bit set. The exposure is demonstrated by history: the PROTOCOL_* list has grown three times (RDSTLS, HYBRID_EX, RDSAAD), and each addition would have made older strict decoders refuse newer peers whose negotiation had already succeeded. Switch both sites to from_bits_retain, the crate-wide policy since #1144; re-encoding a decoded block reproduces the wire value. This continues the interop line of #1489, #1458, #1837, #1843, #1844 and #1845. --- .../ironrdp-pdu/src/gcc/core_data/client.rs | 15 +++++-- .../ironrdp-pdu/src/gcc/core_data/server.rs | 13 ++++-- .../ironrdp-testsuite-core/tests/pdu/gcc.rs | 43 +++++++++++++++++++ 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/crates/ironrdp-pdu/src/gcc/core_data/client.rs b/crates/ironrdp-pdu/src/gcc/core_data/client.rs index 2a161502ff..e75f2309cf 100644 --- a/crates/ironrdp-pdu/src/gcc/core_data/client.rs +++ b/crates/ironrdp-pdu/src/gcc/core_data/client.rs @@ -436,10 +436,17 @@ impl<'de> Decode<'de> for ClientCoreOptionalData { try_or_return!(src.try_read_u8(), optional_data); - optional_data.server_selected_protocol = Some( - SecurityProtocol::from_bits(try_or_return!(src.try_read_u32(), optional_data)) - .ok_or_else(|| invalid_field_err!("serverSelectedProtocol", "invalid security protocol", in: src))?, - ); + // Echo of the selectedProtocol from the X.224 RDP Negotiation + // Response, which nego.rs already decodes with from_bits_retain; the + // echo must not be stricter than the original. [MS-RDPBCGR] + // 3.3.5.3.3's mandated handling is a value comparison against what + // the server actually sent (SHOULD drop on mismatch), which needs + // the value to survive decode. The protocol list has grown three + // times (RDSTLS, HYBRID_EX, RDSAAD). + optional_data.server_selected_protocol = Some(SecurityProtocol::from_bits_retain(try_or_return!( + src.try_read_u32(), + optional_data + ))); optional_data.desktop_physical_width = Some(try_or_return!(src.try_read_u32(), optional_data)); // physical height must be present, if the physical width is present diff --git a/crates/ironrdp-pdu/src/gcc/core_data/server.rs b/crates/ironrdp-pdu/src/gcc/core_data/server.rs index ac5b6d421a..03d8f953d3 100644 --- a/crates/ironrdp-pdu/src/gcc/core_data/server.rs +++ b/crates/ironrdp-pdu/src/gcc/core_data/server.rs @@ -109,10 +109,15 @@ impl<'de> Decode<'de> for ServerCoreOptionalData { fn decode(src: &mut ReadCursor<'de>) -> DecodeResult { let mut optional_data = Self::default(); - optional_data.client_requested_protocols = Some( - SecurityProtocol::from_bits(try_or_return!(src.try_read_u32(), optional_data)) - .ok_or_else(|| invalid_field_err!("clientReqProtocols", "invalid server security protocol", in: src))?, - ); + // Echo of the requestedProtocols from the X.224 RDP Negotiation + // Request, which nego.rs already decodes with from_bits_retain; the + // echo must not be stricter than the original, and the client + // compares it against what it actually requested rather than + // validating the bit set. + optional_data.client_requested_protocols = Some(SecurityProtocol::from_bits_retain(try_or_return!( + src.try_read_u32(), + optional_data + ))); optional_data.early_capability_flags = Some( ServerEarlyCapabilityFlags::from_bits(try_or_return!(src.try_read_u32(), optional_data)) diff --git a/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs b/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs index e34672339c..992397a110 100644 --- a/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs +++ b/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs @@ -427,6 +427,49 @@ fn from_buffer_correctly_parses_server_core_data_without_optional_fields() { assert_eq!(*SERVER_CORE_DATA, decode(buffer).unwrap()); } +#[test] +fn client_core_data_with_undefined_protocol_echo_bits_decodes_and_retains_them() { + // serverSelectedProtocol echoes the X.224 negotiation value, which + // nego.rs already accepts with retain; the echo must not be stricter. + // [MS-RDPBCGR] 3.3.5.3.3 handles this field by value comparison, so the + // value has to survive decode. The bit is retained so re-encoding + // preserves the wire value. + const UNDEFINED_BIT: u32 = 0x0000_0020; // undefined in 2.2.1.1.1 + + let mut buffer = CLIENT_OPTIONAL_CORE_DATA_TO_SERVER_SELECTED_PROTOCOL_BUFFER.to_vec(); + let at = buffer.len() - 4; + let protocol = u32::from_le_bytes(buffer[at..].try_into().unwrap()) | UNDEFINED_BIT; + buffer[at..].copy_from_slice(&protocol.to_le_bytes()); + + let data: ClientCoreData = decode(buffer.as_slice()).expect("undefined echo bits are not fatal"); + + assert_ne!( + data.optional_data.server_selected_protocol.unwrap().bits() & UNDEFINED_BIT, + 0 + ); + assert_eq!(encode_vec(&data).unwrap(), buffer); +} + +#[test] +fn server_core_data_with_undefined_protocol_echo_bits_decodes_and_retains_them() { + // clientRequestedProtocols echoes the client's own X.224 request; the + // client compares it against what it actually requested rather than + // validating the bit set. + const UNDEFINED_BIT: u32 = 0x0000_0020; // undefined in 2.2.1.1.1 + + let mut buffer = SERVER_CORE_DATA_TO_REQUESTED_PROTOCOL_BUFFER.to_vec(); + let protocol = u32::from_le_bytes(buffer[4..8].try_into().unwrap()) | UNDEFINED_BIT; + buffer[4..8].copy_from_slice(&protocol.to_le_bytes()); + + let data: ServerCoreData = decode(buffer.as_slice()).expect("undefined echo bits are not fatal"); + + assert_ne!( + data.optional_data.client_requested_protocols.unwrap().bits() & UNDEFINED_BIT, + 0 + ); + assert_eq!(encode_vec(&data).unwrap(), buffer); +} + #[test] fn from_buffer_correctly_parses_server_core_data_without_few_optional_fields() { let buffer = SERVER_CORE_DATA_TO_REQUESTED_PROTOCOL_BUFFER;