From d65649aebaef7b398c2db35443320fc032397e20 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Tue, 4 Aug 2026 11:33:49 +0100 Subject: [PATCH 1/4] fix(pdu): don't reject a Share Data PDU whose totalLength is under-declared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VirtualBox's VRDP sets the Server Font Map PDU's totalLength to 18 — the two headers — and never counts the 8-byte Font Map body that follows it. The PDU is complete on the wire; only the declared length is wrong. Decoding it failed with NotEnoughBytes { received: 18, expected: 26 } which killed the connection during Connection Finalization, every time, against that server. The name of the error is the tell that nothing was missing: the inner PDU had already decoded successfully, out of bytes that were really there, before the consistency check on totalLength ran at all. A genuinely short buffer fails earlier, inside from_type — as the client-font-list truncation test still shows. So the only case that leaves anything to do is a server declaring *more* than we consumed, which is the existing Windows trailing-padding path. That collapses the check to `total_length > header_length`, and the empty-output special case with it: a zero totalLength now falls through as the no-op it already was. Reported against Haven on VirtualBox: GlassHaven/Haven#422 --- crates/ironrdp-pdu/src/rdp/headers.rs | 23 +++++++------------ .../ironrdp-testsuite-core/tests/pdu/rdp.rs | 10 ++++++++ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/crates/ironrdp-pdu/src/rdp/headers.rs b/crates/ironrdp-pdu/src/rdp/headers.rs index 17a02f60d..7a8b342f1 100644 --- a/crates/ironrdp-pdu/src/rdp/headers.rs +++ b/crates/ironrdp-pdu/src/rdp/headers.rs @@ -1,7 +1,7 @@ use bitflags::bitflags; use ironrdp_core::{ Decode, DecodeResult, Encode, EncodeResult, ReadCursor, WriteBuf, WriteCursor, cast_length, decode, - ensure_fixed_part_size, ensure_size, invalid_field_err, not_enough_bytes_err, other_err, read_padding, + ensure_fixed_part_size, ensure_size, invalid_field_err, other_err, read_padding, unsupported_value_err, write_padding, }; use num_derive::FromPrimitive; @@ -321,22 +321,15 @@ impl<'de> Decode<'de> for ShareControlHeader { }; if pdu_type == ShareControlPduType::DataPdu { + // Servers get totalLength wrong in both directions. Some Windows versions append + // padding past the inner unit; VirtualBox's VRDP declares only the two headers of a + // Server Font Map PDU (18) and never counts its 8-byte body. An under-declared + // length is not a truncation — the inner PDU above has already decoded from bytes + // that were really present, and a genuinely short buffer fails there instead — so + // only a server claiming *more* than we consumed leaves anything to skip. let header_length = header.size(); - let is_empty_output_pdu = matches!( - &header.share_control_pdu, - ShareControlPdu::Data(ShareDataHeader { - share_data_pdu: ShareDataPdu::Update(data) | ShareDataPdu::Pointer(data), - .. - }) 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)); - } - - // Some Windows versions append padding that is not part of the inner unit. + if total_length > header_length { let padding = total_length - header_length; ensure_size!(in: src, size: padding); read_padding!(src, padding); diff --git a/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs b/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs index 55fda5538..2c97ee40d 100644 --- a/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs +++ b/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs @@ -78,6 +78,16 @@ 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()); +} + #[test] fn from_header_only_buffer_rejects_rdp_pdu_client_font_list() { assert!(decode::(&CLIENT_FONT_LIST_BUFFER[..18]).is_err()); From c5fa9fac8f36affdc2add12adae37391974a99ee Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Tue, 4 Aug 2026 11:56:47 +0100 Subject: [PATCH 2/4] style: rustfmt the shortened import list --- crates/ironrdp-pdu/src/rdp/headers.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ironrdp-pdu/src/rdp/headers.rs b/crates/ironrdp-pdu/src/rdp/headers.rs index 7a8b342f1..17bc5ad63 100644 --- a/crates/ironrdp-pdu/src/rdp/headers.rs +++ b/crates/ironrdp-pdu/src/rdp/headers.rs @@ -1,8 +1,8 @@ use bitflags::bitflags; use ironrdp_core::{ Decode, DecodeResult, Encode, EncodeResult, ReadCursor, WriteBuf, WriteCursor, cast_length, decode, - ensure_fixed_part_size, ensure_size, invalid_field_err, other_err, read_padding, - unsupported_value_err, write_padding, + ensure_fixed_part_size, ensure_size, invalid_field_err, other_err, read_padding, unsupported_value_err, + write_padding, }; use num_derive::FromPrimitive; use num_traits::FromPrimitive as _; From 3405208bab76b344585358b0d8a0c9c01f6a13a7 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Tue, 4 Aug 2026 20:41:35 +0100 Subject: [PATCH 3/4] fix(pdu): keep rejecting a zero totalLength on a non-output Data PDU MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review was right: dropping the check wholesale relaxed more than the compatibility case needs. `reject_zero_length_non_output_data_pdu` is an explicit regression test and my change made it pass through. I missed it because I ran `-p ironrdp-testsuite-core` rather than the workspace, and that test lives in ironrdp-pdu's own `mod tests`. The workspace run reproduces it immediately. The distinction that matters: a server undercounting its own body (VirtualBox declaring 18 for a 26-byte Server Font Map) has bytes that really arrived and an inner PDU that already decoded from them. A zero totalLength has none of that — it is a length field never filled in, and the only legitimate use is the no-op empty Update / Pointer PDU. So that case keeps its carve-out and everything else with a zero length is still rejected, exactly as before. Workspace tests: 0 failures. Both behaviours are pinned — `from_buffer_with_under_declared_total_length_parses_rdp_pdu_server_font_map` for the relaxation, `reject_zero_length_non_output_data_pdu` for its limit. --- crates/ironrdp-pdu/src/rdp/headers.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/crates/ironrdp-pdu/src/rdp/headers.rs b/crates/ironrdp-pdu/src/rdp/headers.rs index 17bc5ad63..477ca4a5e 100644 --- a/crates/ironrdp-pdu/src/rdp/headers.rs +++ b/crates/ironrdp-pdu/src/rdp/headers.rs @@ -1,8 +1,8 @@ use bitflags::bitflags; use ironrdp_core::{ Decode, DecodeResult, Encode, EncodeResult, ReadCursor, WriteBuf, WriteCursor, cast_length, decode, - ensure_fixed_part_size, ensure_size, invalid_field_err, other_err, read_padding, unsupported_value_err, - write_padding, + ensure_fixed_part_size, ensure_size, invalid_field_err, not_enough_bytes_err, other_err, read_padding, + unsupported_value_err, write_padding, }; use num_derive::FromPrimitive; use num_traits::FromPrimitive as _; @@ -325,14 +325,28 @@ impl<'de> Decode<'de> for ShareControlHeader { // padding past the inner unit; VirtualBox's VRDP declares only the two headers of a // Server Font Map PDU (18) and never counts its 8-byte body. An under-declared // length is not a truncation — the inner PDU above has already decoded from bytes - // that were really present, and a genuinely short buffer fails there instead — so - // only a server claiming *more* than we consumed leaves anything to skip. + // that were really present, and a genuinely short buffer fails there instead — so a + // server claiming *more* than we consumed is the only case with anything left to skip. + // + // Zero stays rejected. That is not a server undercounting its own body, it is a + // length field never filled in; the one legitimate use is the no-op empty Update / + // Pointer PDU, so that case is carved out rather than the check dropped. let header_length = header.size(); + let is_empty_output_pdu = matches!( + &header.share_control_pdu, + ShareControlPdu::Data(ShareDataHeader { + share_data_pdu: ShareDataPdu::Update(data) | ShareDataPdu::Pointer(data), + .. + }) if data.is_empty() + ); + if total_length > header_length { let padding = total_length - header_length; ensure_size!(in: src, size: padding); read_padding!(src, padding); + } else if total_length == 0 && !is_empty_output_pdu { + return Err(not_enough_bytes_err!(total_length, header_length)); } } From 79ec46fe8ad266be9859692a64c1ea89ff425a46 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sun, 9 Aug 2026 21:12:07 +0100 Subject: [PATCH 4/4] fix(pdu): narrow the under-declared length exception to the Server Font Map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review: the previous revision left every positive `totalLength < header_length` accepted, including a header-only non-output PDU declaring 1..17, which MS-RDPBCGR 3.2.5.2 asks a receiver to reject. The tolerance is now keyed on the PDU type instead of being open-ended. What is tolerated is exactly one server bug: VirtualBox's VRDP declares the two headers of a Server Font Map (18) and never counts the 8-byte body it does send. Everything else that under-declares is rejected again, so the change is a net tightening against the branch under review. Why the exception is keyed on the type rather than on arithmetic, since the obvious alternative does not work: `header_length` is `header.size()`, the *re-encoded* size, not the bytes that were on the wire. `ShareDataPdu::from_type` substitutes `FontPdu::default()` when the cursor is empty, so a header-only Font Map re-encodes to 26 while only 18 bytes existed. A conformant server declaring 18 was therefore measured against 26 and refused — master rejects a spec-correct PDU today, and any comparison against the re-encoded size reproduces that. I did try deriving the answer instead, accepting `totalLength` when it equalled the bytes actually consumed. It reads better and it is dead code: the Font Map is the only PDU whose re-encoded size exceeds what it consumed, so the type check always got there first. Mutating that clause away changed no test result, so it is not in this diff. The over-declared branch is untouched, including its `ensure_size!`, so the Windows padding case behaves exactly as before. Two tests, both confirmed able to fail: from_header_only_buffer_with_matching_total_length... an 18-byte Font Map declaring 18 — the conformant encoding master refuses. reject_under_declared_non_output_data_pdu a ShutdownDenied declaring 17, the case raised in review. Removing the Font Map exception fails both Font Map tests; restoring the open-ended acceptance fails both rejection tests, including the existing `reject_zero_length_non_output_data_pdu`. `cargo test --workspace` passes (17 suites), clippy clean. --- crates/ironrdp-pdu/src/rdp/headers.rs | 56 +++++++++++++++---- .../ironrdp-testsuite-core/tests/pdu/rdp.rs | 17 ++++++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/crates/ironrdp-pdu/src/rdp/headers.rs b/crates/ironrdp-pdu/src/rdp/headers.rs index 477ca4a5e..87ac67969 100644 --- a/crates/ironrdp-pdu/src/rdp/headers.rs +++ b/crates/ironrdp-pdu/src/rdp/headers.rs @@ -321,18 +321,15 @@ impl<'de> Decode<'de> for ShareControlHeader { }; if pdu_type == ShareControlPduType::DataPdu { - // Servers get totalLength wrong in both directions. Some Windows versions append - // padding past the inner unit; VirtualBox's VRDP declares only the two headers of a - // Server Font Map PDU (18) and never counts its 8-byte body. An under-declared - // length is not a truncation — the inner PDU above has already decoded from bytes - // that were really present, and a genuinely short buffer fails there instead — so a - // server claiming *more* than we consumed is the only case with anything left to skip. - // - // Zero stays rejected. That is not a server undercounting its own body, it is a - // length field never filled in; the one legitimate use is the no-op empty Update / - // Pointer PDU, so that case is carved out rather than the check dropped. + // 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 { @@ -341,11 +338,25 @@ impl<'de> Decode<'de> for ShareControlHeader { }) if data.is_empty() ); + // 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(_), + .. + }) + ); + 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 == 0 && !is_empty_output_pdu { + } else if total_length < header_length && !is_font_map && !is_empty_output_pdu { return Err(not_enough_bytes_err!(total_length, header_length)); } } @@ -954,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 2c97ee40d..7f82e61d5 100644 --- a/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs +++ b/crates/ironrdp-testsuite-core/tests/pdu/rdp.rs @@ -88,6 +88,23 @@ fn from_buffer_with_under_declared_total_length_parses_rdp_pdu_server_font_map() 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());