diff --git a/crates/ironrdp-pdu/src/rdp/headers.rs b/crates/ironrdp-pdu/src/rdp/headers.rs index 17a02f60d..87ac67969 100644 --- a/crates/ironrdp-pdu/src/rdp/headers.rs +++ b/crates/ironrdp-pdu/src/rdp/headers.rs @@ -321,8 +321,15 @@ impl<'de> Decode<'de> for ShareControlHeader { }; if pdu_type == ShareControlPduType::DataPdu { + // Note this is the *re-encoded* size, not the bytes that were on the wire. The two + // diverge for a header-only Server Font Map: `ShareDataPdu::from_type` substitutes + // `FontPdu::default()` on an empty cursor, so the re-encoded size counts an 8-byte + // body that was never sent, and a conformant server declaring 18 was measured + // against 26. That is why an honest header-only Font Map was rejected, and it is + // the reason the exception below is keyed on the PDU type rather than on arithmetic. let header_length = header.size(); + // An empty Update/Pointer PDU is a legitimate no-op carrying a zero length. let is_empty_output_pdu = matches!( &header.share_control_pdu, ShareControlPdu::Data(ShareDataHeader { @@ -331,15 +338,26 @@ impl<'de> Decode<'de> for ShareControlHeader { }) if data.is_empty() ); - if header_length != total_length && !(total_length == 0 && is_empty_output_pdu) { - if total_length < header_length { - return Err(not_enough_bytes_err!(total_length, header_length)); - } + // VirtualBox's VRDP declares only the two headers of a Server Font Map (18) and + // never counts the 8-byte body that follows, so it under-declares by exactly the + // body it did send. Narrowed to this PDU type rather than allowed for Data PDUs at + // large, so a malformed non-output PDU declaring 1..17 is still rejected. + let is_font_map = matches!( + &header.share_control_pdu, + ShareControlPdu::Data(ShareDataHeader { + share_data_pdu: ShareDataPdu::FontMap(_), + .. + }) + ); - // Some Windows versions append padding that is not part of the inner unit. + if total_length > header_length { + // Over-declared: some Windows versions append padding past the inner unit. + // Unchanged, and still bounded by `ensure_size!`. let padding = total_length - header_length; ensure_size!(in: src, size: padding); read_padding!(src, padding); + } else if total_length < header_length && !is_font_map && !is_empty_output_pdu { + return Err(not_enough_bytes_err!(total_length, header_length)); } } @@ -947,6 +965,29 @@ mod tests { )); } + /// The under-declaration carve-out must not extend past the Server Font Map case. + /// + /// A `ShutdownDenied` PDU is header-only, so 18 bytes were read; declaring 17 is neither + /// self-consistent with the wire nor the VRDP Font Map, and MS-RDPBCGR 3.2.5.2 asks for it + /// to be rejected. Guards the exact example raised in review — "a header-only PDU declared + /// with 1–17 bytes" — which an earlier revision of this check accepted. + #[test] + fn reject_under_declared_non_output_data_pdu() { + let mut encoded = zero_length_empty_data_pdu(0x25); + encoded[0] = 17; + + let error = decode::(&encoded) + .expect_err("an under-declared length is only tolerated for a Server Font Map"); + + assert!(matches!( + error.kind(), + ironrdp_core::DecodeErrorKind::NotEnoughBytes { + received: 17, + expected: 18 + } + )); + } + #[test] fn share_data_context_retains_compression_metadata() { let mut user_data = encode_vec(&ShareControlHeader { diff --git a/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs b/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs index 55fda5538..7f82e61d5 100644 --- a/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs +++ b/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs @@ -78,6 +78,33 @@ fn from_header_only_buffer_defaults_rdp_pdu_server_font_map() { assert_eq!(SERVER_FONT_MAP.clone(), decode(buf).unwrap()); } +/// VirtualBox's VRDP declares `totalLength` as the size of the two headers (18) and does not +/// count the 8-byte Font Map body that follows it, so the PDU is complete but under-declared. +#[test] +fn from_buffer_with_under_declared_total_length_parses_rdp_pdu_server_font_map() { + let mut buf = SERVER_FONT_MAP_BUFFER; + buf[0] = 18; + + assert_eq!(SERVER_FONT_MAP.clone(), decode(buf.as_ref()).unwrap()); +} + +/// A header-only Server Font Map that declares its own length honestly: 18 bytes sent, 18 +/// declared. Nothing about it is malformed, and it is the encoding a server is most likely to +/// send in the field. +/// +/// It is pinned separately because it is the case the old check got wrong, for a reason that is +/// easy to reintroduce: `ShareDataPdu::from_type` defaults a `FontPdu` in when the cursor is +/// empty, so the re-encoded size is 26 and comparing 18 against it rejected a conformant PDU. +/// Anyone reinstating a comparison against the re-encoded size will fail here. The neighbouring +/// `from_header_only_buffer_defaults` test does not cover it — its fixture still declares 26. +#[test] +fn from_header_only_buffer_with_matching_total_length_parses_rdp_pdu_server_font_map() { + let mut buf = SERVER_FONT_MAP_BUFFER[..18].to_vec(); + buf[0] = 18; + + assert_eq!(SERVER_FONT_MAP.clone(), decode(buf.as_slice()).unwrap()); +} + #[test] fn from_header_only_buffer_rejects_rdp_pdu_client_font_list() { assert!(decode::(&CLIENT_FONT_LIST_BUFFER[..18]).is_err());