fix(pdu): tolerate an empty Server Font Map body - #1472
Closed
GlassOnTin wants to merge 1 commit into
Closed
Conversation
`FontPdu::decode` requires the full 8-byte fixed part (number, total_number, flags, entry_size). Some RDP servers — notably VirtualBox's VRDP — end the connection finalization sequence with a Server Font Map PDU that carries an *empty* body. Against those servers `decode` fails with `NotEnoughBytes`, and since the Font Map is the final step of finalization (MS-RDPBCGR 1.3.1.1) the whole connection is aborted right after a successful logon. The Font Map's fields are unused by the client — it only signals that finalization is complete and the server may begin sending graphics. So an empty body now decodes to the spec-recommended defaults (the existing `FontPdu::default()`: number 0, total_number 0, flags FIRST|LAST, entry_size 4) instead of erroring. This matches mstsc and FreeRDP, both of which accept a short/empty Font Map. A present-but-truncated body still errors via `ensure_fixed_part_size!`. Adds unit tests for the full-body and empty-body decode paths.
| // signals the end of the connection finalization sequence (MS-RDPBCGR | ||
| // 1.3.1.1) — so an empty body decodes to the recommended defaults rather | ||
| // than failing the whole connection, matching mstsc/FreeRDP leniency. | ||
| if src.is_empty() { |
Contributor
There was a problem hiding this comment.
FontPdu is also the decoder for ShareDataPdu::FontList (headers.rs dispatches both FontList and FontMap here). This makes a header-only client Font List decode successfully, and ironrdp-acceptor then treats it as successfully processed and advances finalization. The requested tolerance is only for a Server Font Map; keep the empty-body exception in the FontMap dispatch path (or use a distinct decode entry point) so malformed Font Lists still fail. Please add coverage for accepting a header-only Font Map while rejecting the same Font List.
This was referenced Jul 30, 2026
Marc-André Moreau (mamoreau-devolutions)
pushed a commit
that referenced
this pull request
Jul 31, 2026
…tionResult (#1488) Per [MS-RDPBCGR] 2.2.8.1.2, a client must not send fast-path input events unless the server advertised `INPUT_FLAG_FASTPATH_INPUT` or `INPUT_FLAG_FASTPATH_INPUT2` in its Input Capability Set. Today the connector discards the server's capability sets after Demand Active, so client code has no way to honour that requirement. This PR captures the server's Input capability flags during capabilities exchange, carries them through the `ConnectionFinalization`/`Finalized` activation states (so they are refreshed correctly across a Deactivation-Reactivation Sequence too), and surfaces them as `ConnectionResult::input_flags`. The session layer can then choose between fast-path and slow-path input per server. ### Motivation / real-world interop This is not theoretical: VirtualBox's VRDP server closes the connection outright on receiving a fast-path input PDU — its `VBox.log` reports ``` VRDP: Network packet length is incorrect 0x0004. Closing connection. ``` (a single fast-path scancode event is a 4-byte packet). VirtualBox never advertises fast-path input; its Demand Active offers `InputFlags(SCANCODES)` alone. mstsc and FreeRDP honour the negotiation and fall back to slow-path `TS_INPUT_PDU`s, which is why they work against VRDE. Haven (Android RDP client built on IronRDP) has been shipping this exact change as a vendored-connector patch since v5.86.1, with the slow-path fallback keyed off `ConnectionResult::input_flags`. Verified against a real VirtualBox 7.2.6 VRDE server: before the gate, the first arrow-key press killed the session with the log line above; with the gate, extended input sessions run clean, and a fast-path-capable server on the same host still takes the fast-path branch. (Discussed in #1158; this is the third and last piece Haven carries in its connector fork, alongside #1237 and #1472.) ### Changes - `connection_activation.rs`: capture `input_flags` from the `CapabilitySet::Input` in Server Demand Active (empty if absent); add the field to `ConnectionActivationState::{ConnectionFinalization, Finalized}`. - `connection.rs`: add `ConnectionResult::input_flags`, populated from the `Finalized` state. - Call sites in `ironrdp-client`, `ironrdp-web`, ffi, and the e2e test updated for the new variant field (all currently ignore it). ### Testing - Two new integration tests in `ironrdp-testsuite-core/tests/session/connection_activation.rs`: the fixture's Demand Active yields `SCANCODES | MOUSEX | UNICODE | FASTPATH_INPUT_2` in the `ConnectionFinalization` state, and a Demand Active with the Input capability stripped yields `InputFlags::empty()`. - `cargo check --workspace --all-targets`, `cargo clippy --workspace --all-targets`, and `cargo fmt --check` are clean; the 7 activation tests pass. Co-authored-by: GlassOnTin <GlassOnTin@users.noreply.github.com>
Contributor
|
fixed by #1506 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
FontPdu::decoderequires the full 8-byte fixed part (number,total_number,flags,entry_size). Some RDP servers — notably VirtualBox's built-in VRDP server — end the connection finalization sequence with a Server Font Map PDU that carries an empty body. Against those serversdecodefails withNotEnoughBytes, and because the Font Map is the final step of finalization (MS-RDPBCGR 1.3.1.1), the whole connection is aborted immediately after a successful logon.The failure surfaces downstream as a connect that authenticates and then dies at finalization.
Fix
The Server Font Map's fields are unused by the client — it only signals that finalization is complete and the server may begin sending graphics. So an empty body now decodes to the spec-recommended defaults (the existing
FontPdu::default():number 0,total_number 0,flags FIRST|LAST,entry_size 4) instead of erroring. A present-but-truncated body still errors viaensure_fixed_part_size!, so this only relaxes the fully-empty case.This matches
mstscand FreeRDP, both of which accept a short/empty Font Map.Scope note
FontPduis shared byFontList(client→server, encode-only) andFontMap(server→client, decode-only), so this decode-side leniency only affects the received Font Map in practice. If you'd rather scope it strictly to theShareDataPduType::FontMaparm inrdp/headers.rsand keepFontPdu::decodeitself strict, I'm happy to move it there — just let me know your preference.Tests
Adds unit tests for both decode paths (full 8-byte body → parsed fields; empty body → defaults).
cargo test -p ironrdp-pduis green (374 passing);cargo clippy -p ironrdp-pdu --all-targetsclean.Context
Sibling to #1237 — both are small connector/pdu changes that let a downstream (an Android RDP client) drop a vendored
ironrdp-connectorfork it currently carries solely to work around VirtualBox VRDP quirks. This one removes the Font Map workaround; #1237 removes the EGFX early-capability workaround.