fix(pdu): don't reject a Share Data PDU whose totalLength is under-declared

[GlassOnTin]

The problem

VirtualBox's built-in RDP server (VRDP) cannot complete a connection with IronRDP. It fails on the last message of the handshake:

ironrdp_connector::connection_finalization: Server Control (Granted Control)
ironrdp_blocking::connector: Wait for PDU connector.state="ConnectionFinalization" hint=X224Hint
RDP protocol decode error: Error {
    context: "<ironrdp_pdu::rdp::headers::ShareControlHeader as ironrdp_core::decode::Decode<'_>>::decode",
    kind: NotEnoughBytes { received: 18, expected: 26 },
}

Nothing is missing. Those two numbers come from the total_length cross-check at the end of ShareControlHeader::decode, so received is the length the server declared and expected is the size the PDU actually decoded to — neither is a byte count off the wire:

if total_length < header_length {
    return Err(not_enough_bytes_err!(total_length, header_length));
}

For the Server Font Map PDU those sizes are 18 and 26: SHARE_CONTROL_HEADER_SIZE (10) + ShareDataHeader::FIXED_PART_SIZE (8) + FontPdu (8). VRDP declares totalLength as the two headers and never counts its own 8-byte body. All 26 bytes arrive; only the number is wrong.

The inner PDU has already decoded successfully out of those bytes by the time this check runs, which is what makes the rejection avoidable — the check is a consistency assertion, not a bounds check.

The change

Only a server declaring more than was decoded leaves anything to do, and that case already has a handler — the trailing-padding path added for Windows. So the check collapses to:

if total_length > header_length {
    let padding = total_length - header_length;
    ensure_size!(in: src, size: padding);
    read_padding!(src, padding);
}

The is_empty_output_pdu special case goes with it: a total_length of 0 now falls through as the no-op it already was.

Genuine truncation is unaffected

This is the part worth checking rather than taking on trust. A short buffer fails earlier — inside the inner PDU's own Decode impl, via ensure_fixed_part_size! — and never reaches this check. The existing test proves it and still passes:

fn from_header_only_buffer_rejects_rdp_pdu_client_font_list() {
    assert!(decode::<ShareControlHeader>(&CLIENT_FONT_LIST_BUFFER[..18]).is_err());
}

Tests

Added one that fails without this change with the reported error verbatim:

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());
}

It sits next to from_header_only_buffer_defaults_rdp_pdu_server_font_map, which already tolerates an 18-byte buffer — this covers the neighbouring case of a 26-byte buffer with an 18-byte declaration.

cargo test -p ironrdp-testsuite-core: 1049 passed, 0 failed.

Where this was found

Downstream in Haven, from a user who could not connect to VirtualBox at all (GlassHaven/Haven#422). The same fix also resolves an earlier report on that issue where VRDP under-declares a Pointer update mid-session as 24 bytes on an 8565-byte frame — same defect, different PDU, and one we had been working around client-side because it happened after connect. This one happens during finalization, where there is no session yet to work around it in.

I have not captured the VRDP bytes myself, so which PDU the server sends at that point rests on sequence position — it follows Granted Control, where the client waits for Font Map — rather than on a capture. The size arithmetic matches Font Map exactly, but a Control PDU is also 26 bytes, so I would not claim more than that. The fix does not depend on which it is.

[Marc-André Moreau (mamoreau-devolutions)]

Request changes: the compatibility relaxation must not accept arbitrary under-declared Data PDUs. The existing zero-length non-output regression test now fails, contrary to the Share Control length consistency requirement in MS-RDPBCGR.

[Marc-André Moreau (mamoreau-devolutions)]

MS-RDPBCGR 2.2.8.1.1.1.1 defines totalLength as the packet length including the Share Control Header, and 3.2.5.2 requires checking it for consistency (a discrepancy should cause the connection to be dropped). This condition now accepts totalLength < header_length for every Data PDU, including totalLength == 0; the existing rdp::headers::tests::reject_zero_length_non_output_data_pdu fails because a ShutdownDenied PDU with a zero length is accepted. Please retain the prior exception only for empty Update/Pointer PDUs, or narrow the compatibility handling to a complete known VRDP Font Map body rather than accepting arbitrary under-declared lengths.

[Marc-André Moreau (mamoreau-devolutions)]

Request changes: the new zero-length guard fixes the specific regression, but the generalized under-declared-length acceptance remains too broad for protocol correctness.

[Marc-André Moreau (mamoreau-devolutions)]

Still request changes: this condition leaves every positive totalLength < header_length accepted. That includes malformed non-output Data PDUs (for example, a header-only PDU declared with 1–17 bytes), not just the complete VRDP Font Map compatibility case. MS-RDPBCGR §3.2.5.2 requires the server to validate totalLength consistency; please narrow the under-declared exception to the validated Server Font Map case, or otherwise prove that the complete inner PDU is present before accepting it.

[github-actions]

The diff restructures totalLength handling in ShareControlHeader::decode to accept a non-zero under-declared value. Traced against the pre-change condition, over-declared padding-skip (still guarded by ensure_size!) and zero-length rejection with its empty Update/Pointer carve-out are preserved exactly, so only the intended relaxation changes. The protocol analysis missed that header_length is header.size(), the re-encoded size, not bytes consumed: for a Font Map it counts a body from_type defaults in, so master rejects a self-consistent 18-byte header-only Font Map. The change fixes that real defect. Narrower alternatives fail: remaining() does not advance the cursor, so consumed-bytes or is_empty() guards misfire on read-to-end variants, and ironrdp-pdu has no tracing. No blocking findings; callers decode one PDU per slice and the fuzz oracle only asserts re-decodability. Two non-blocking items: an untested acceptance boundary, and a comment understating server-role reach.

Protocol analysis: partially_accepted — Kept: the relaxation drops the totalLength examination of 3.2.5.2/3.3.5.2 and applies in the server role despite a comment about servers only — my second finding. Refined: the handoff reads header_length as bytes consumed; it is header.size(), which for a Font Map counts a body from_type defaults in on an empty cursor, so master rejects a conformant 18-byte header-only Font Map. The removed check was not a working consistency check. Rejected as out of scope: the zero-length carve-out, FlowPDU 0x8000, and unchecked non-Data PDUs are pre-existing and behaviourally unchanged (0x8000 fails ensure_size before and after). The residue concern is real but low impact: callers decode one PDU per slice and remaining() does not advance the cursor. I accept one suggested test, refined.

[github-actions]

non_blocking / medium: The new test pins only the "body present, totalLength under-declared" case. The subtler half of the fix is untested: because header.size() counts the FontPdu body that ShareDataPdu::from_type defaults in when the cursor is empty (headers.rs:625-633), master rejects an 18-byte header-only Font Map that declares a fully self-consistent totalLength of 18 (18 < 26 hits the removed branch). That is a spec-conformant encoding the old code refused, and it is the case most likely to be hit in the field. The existing from_header_only_buffer_defaults test does not cover it — its fixture still declares 26. Adding a case with an 18-byte buffer and buf[0] = 18 would pin the acceptance boundary and document why comparing against the re-encoded size, rather than bytes consumed, is what made the check unusable.

[github-actions]

non_blocking / medium: The comment justifies the relaxation entirely in terms of server misbehaviour ("Servers get totalLength wrong in both directions"), but this decoder is shared with ironrdp-server and ironrdp-acceptor, which decode client-originated Data PDUs through the same path. The leniency therefore also applies to inbound client PDUs such as Input Event PDUs, widening hostile-input tolerance on a length field that MS-RDPBCGR 3.3.5.2 asks receivers to examine. I checked the narrower alternatives and none are workable here: ReadCursor::remaining() does not advance pos, so a consumed-bytes or is_empty() guard would misfire on the read-to-end variants, and ironrdp-pdu has no tracing dependency to record the inconsistency instead. The blast radius is not itself exploitable — the padding branch is unchanged and still guarded by ensure_size!, and bodies stay bounded by the transport frame — so this is a documentation gap rather than a defect: one sentence noting the relaxation is deliberate in the server role too would stop a future reader assuming it is client-only.

[GlassOnTin]

Thanks — the second round was right, and I've taken the first of your two options: the exception is now keyed on the Server Font Map PDU type rather than on a length comparison. A header-only non-output PDU declaring 1–17 is rejected again, so relative to the branch you reviewed this is a net tightening, not a further relaxation.

There is one thing worth surfacing, because it is a defect in master independent of VRDP.

header_length is header.size() — the re-encoded size, not the bytes that were on the wire. For a Font Map the two differ, because ShareDataPdu::from_type substitutes FontPdu::default() when the cursor is empty:

ShareDataPduType::FontMap => {
    let font_pdu = if src.is_empty() { FontPdu::default() } else { FontPdu::decode(src)? };

So a header-only Server Font Map re-encodes to 26 while only 18 bytes ever existed. A server sending 18 bytes and honestly declaring totalLength = 18 is measured against 26, and master refuses it. That is a spec-conformant encoding being rejected, and it is why I could not narrow this by comparing lengths — the quantity being compared against is not the one totalLength describes.

I did try the derived version you'd expect, accepting totalLength when it equals the bytes actually consumed. It is not in the diff, because it turned out to be dead code: the Font Map is the only PDU whose re-encoded size exceeds what it consumed, so the type check always reached the case first. I mutated the clause away and no test changed result, so I removed it rather than ship an unreachable branch with a confident comment on it.

The over-declared branch is untouched, ensure_size! included.

Two tests, and I checked both can actually fail rather than assuming:

• from_header_only_buffer_with_matching_total_length_parses_rdp_pdu_server_font_map — the 18-byte Font Map declaring 18, i.e. the conformant case master refuses. This is the test your automation suggested; it was a good catch.
• reject_under_declared_non_output_data_pdu — a ShutdownDenied declaring 17, the exact example from your 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 that my first revision broke. cargo test --workspace passes (17 suites) and clippy is clean.

If you would rather not carry a per-server exception at all, the alternative I can offer is to drop the VRDP tolerance and keep only the header-only fix — that alone makes master accept the conformant 18-byte Font Map, and it leaves VirtualBox broken. I have a preference but not a strong one; it is your call which side of that line the crate should sit on.