diff --git a/crates/ironrdp-pdu/src/gcc/security_data.rs b/crates/ironrdp-pdu/src/gcc/security_data.rs index 3d770048bc..be95b21758 100644 --- a/crates/ironrdp-pdu/src/gcc/security_data.rs +++ b/crates/ironrdp-pdu/src/gcc/security_data.rs @@ -59,8 +59,14 @@ impl<'de> Decode<'de> for ClientSecurityData { fn decode(src: &mut ReadCursor<'de>) -> DecodeResult { ensure_fixed_part_size!(in: src); - let encryption_methods = EncryptionMethod::from_bits(src.read_u32()) - .ok_or_else(|| invalid_field_err!("encryptionMethods", "invalid encryption methods", in: src))?; + // [MS-RDPBCGR] 2.2.1.3.3: this field advertises the methods the + // client supports and the server "MUST select one of the methods + // specified by the client"; 3.3.5.3.3 never asks the server to + // validate the bit set, and the sibling extEncryptionMethods field + // below is already read unvalidated. Retain unknown bits (crate-wide + // policy since #1144) rather than failing the GCC exchange: the + // server simply never selects a method it does not know. + let encryption_methods = EncryptionMethod::from_bits_retain(src.read_u32()); let ext_encryption_methods = src.read_u32(); Ok(Self { @@ -147,6 +153,12 @@ impl<'de> Decode<'de> for ServerSecurityData { fn decode(src: &mut ReadCursor<'de>) -> DecodeResult { ensure_fixed_part_size!(in: src); + // Deliberately strict, unlike ClientSecurityData above: this is the + // server's SELECTED method, not an advertisement. [MS-RDPBCGR] + // 2.2.1.3.3 obliges the server to select from the client's list, so a + // conforming server never sends an unknown bit here, and a client + // cannot implement a method it does not know. The value also drives + // the conditional serverRandom/serverCertificate parse below. let encryption_method = EncryptionMethod::from_bits(src.read_u32()) .ok_or_else(|| invalid_field_err!("encryptionMethod", "invalid encryption method", in: src))?; let encryption_level = EncryptionLevel::from_u32(src.read_u32()) diff --git a/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs b/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs index e34672339c..dcee3b17c4 100644 --- a/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs +++ b/crates/ironrdp-testsuite-core/tests/pdu/gcc.rs @@ -803,6 +803,24 @@ fn from_buffer_correctly_parses_client_security_data() { assert_eq!(*CLIENT_SECURITY_DATA, decode(buffer).unwrap()); } +#[test] +fn client_security_data_with_undefined_method_bits_decodes_and_retains_them() { + // [MS-RDPBCGR] 2.2.1.3.3: encryptionMethods is an advertisement and the + // server selects only methods it knows; an unknown bit must not fail the + // GCC exchange. The bit is retained so re-encoding preserves the wire + // value. The server-selected method in ServerSecurityData stays strict. + const UNDEFINED_BIT: u32 = 0x0000_0004; // undefined in 2.2.1.3.3 + + let mut buffer = CLIENT_SECURITY_DATA_BUFFER.to_vec(); + let methods = u32::from_le_bytes(buffer[0..4].try_into().unwrap()) | UNDEFINED_BIT; + buffer[0..4].copy_from_slice(&methods.to_le_bytes()); + + let data: ClientSecurityData = decode(buffer.as_slice()).expect("undefined method bits are not fatal"); + + assert_ne!(data.encryption_methods.bits() & UNDEFINED_BIT, 0); + assert_eq!(encode_vec(&data).unwrap(), buffer); +} + #[test] fn to_buffer_correctly_serializes_client_security_data() { let security_data = CLIENT_SECURITY_DATA.clone();