fix(displaycontrol): decode the full headered DISPLAYCONTROL_CAPS_PDU#1442
fix(displaycontrol): decode the full headered DISPLAYCONTROL_CAPS_PDU#1442truebest wants to merge 4 commits into
Conversation
The server sends the capability set prefixed with the DISPLAYCONTROL_HEADER (Type + Length) per MS-RDPEDISP 2.2.2.1 -- DisplayControlServer::start and the testsuite golden vectors both use that shape -- but process() decoded the payload as a raw DisplayControlCapabilities body. The header bytes were read as caps fields (Type=5 as MaxNumMonitors, Length=20 as FactorA), so max_monitor_area() came out tiny and callback-produced monitor layouts were never sent. Decode the full DisplayControlPdu and reject an unexpected MonitorLayout from the server.
|
Hi truebest, nice catch. Confirmed against master: DisplayControlServer::start already assumes the headered shape, so process() was the actual outlier here, not a design choice. The fix is correct and matches the existing pattern. One suggestion: None of the existing testsuite-core coverage exercises DisplayControlClient::process() itself, only DisplayControlPdu decode/encode symmetry. A small test hitting process() directly with the headered caps bytes (plus one for the MonitorLayout rejection) would pin down the actual regression path, CBenoit tends to ask for this on behavior-changing fixes. Otherwise looks good. Nice find running against gnome-remote-desktop. |
Existing coverage only exercised DisplayControlPdu decode/encode symmetry, not process() itself -- the actual site of the headered-CAPS regression this PR fixes. Add a test decoding a real headered caps PDU through process() and asserting the callback receives the right capabilities, plus a test confirming a MONITOR_LAYOUT_PDU from the server is rejected rather than silently accepted.
|
Good point, added in 447104a: a test decoding a real headered-caps PDU through An agent will work on test coverage, let me know if it generates any issues. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 447104acd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| let caps = DisplayControlCapabilities::decode(&mut ReadCursor::new(payload)).map_err(|e| decode_err!(e))?; | ||
| // The server sends the full DISPLAYCONTROL_CAPS_PDU, i.e. the capability set prefixed | ||
| // with the DISPLAYCONTROL_HEADER (Type + Length), per MS-RDPEDISP 2.2.2.1. | ||
| let caps = match decode(payload).map_err(|e| decode_err!(e))? { |
There was a problem hiding this comment.
When the server sends a CAPS PDU with Type = 5 but an advertised Length that does not match the 20-byte caps PDU (for example 0xffff_ffff followed by a valid caps body), this new path accepts it because DisplayControlPdu::decode only subtracts the header size and never checks the advertised length against payload.len() or the caps size, even though MS-RDPEDISP defines Length as the PDU length. process then marks the channel ready and invokes the callback with capabilities from malformed server framing, so please reject a mismatched Length before accepting Caps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b00767 — DisplayControlPdu::decode now validates Length against the actual remaining payload size before decoding the body, rejecting a mismatch (e.g. 0xffff_ffff) instead of silently accepting whatever bytes follow. Added a test covering exactly this case.
DisplayControlPdu::decode computed the claimed body size from the header but never checked it against the actual payload -- a Caps PDU with a bogus Length (e.g. 0xffff_ffff) followed by a valid body decoded successfully anyway. Validate Length against the remaining bytes before decoding the body.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b007672a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| .checked_sub(Self::FIXED_PART_SIZE.try_into().expect("always in range")) | ||
| .ok_or_else(|| invalid_field_err!("Length", "Display control PDU length is too small"))?; | ||
|
|
||
| if usize::try_from(payload_length).unwrap_or(usize::MAX) != src.len() { |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new Length check only compares the advertised payload length with the bytes handed to the decoder; it does not ensure the selected inner PDU consumes that payload. A server packet with Type = 5, Length = 24, and a 16-byte body will pass this check, DisplayControlCapabilities::decode will read the fixed 12-byte caps body, and the extra 4 bytes are ignored while DisplayControlClient::process marks the channel ready. Please bound the inner decode to payload_length and reject any trailing bytes, or require the CAPS payload to be exactly 12 bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d06b30e — decode() now checks the cursor is empty after decoding the inner PDU body, rejecting trailing bytes for both Caps and MonitorLayout. Added a test with Length=24 over a 16-byte payload where the caps body only consumes 12.
The Length check only verified the header's claimed size against the total payload, not that the specific decoded variant (Caps is a fixed 12 bytes) actually consumed all of it. A Length that matched the payload size but left bytes unconsumed after decoding Caps or MonitorLayout was silently accepted. Check the cursor is empty after decoding the body.
Summary
DisplayControlClient::process()decoded the server's capability payload as a rawDisplayControlCapabilitiesbody. In reality, the server sends the capability set prefixed with aDISPLAYCONTROL_HEADER(Type + Length) as defined by MS-RDPEDISP 2.2.2.1 — the fullDISPLAYCONTROL_CAPS_PDU, not a bareDISPLAYCONTROL_MONITOR_LAYOUT_PDU-style caps body.Because the header bytes were being read as capability fields,
Type(5) was misread asMaxNumMonitorsandLength(20) was misread asMaxMonitorAreaFactorA. This mademax_monitor_area()come out far too small, which silently dropped monitor layouts produced by theon_capabilities_receivedcallback (they were rejected as exceeding the bogus max area).This is an internal inconsistency, not an intentional design choice:
DisplayControlServer::startin this same crate, and theironrdp-testsuite-coregolden vectors fordisplaycontrol, already assume/produce the headered shape. Only the client's decode path was wrong.Fix
process()now decodes the fullDisplayControlPduviadecode()and matches onDisplayControlPdu::Caps(..). ADisplayControlPdu::MonitorLayout(..)received from the server (which would be unexpected — that variant is client→server) is now rejected with an explicit error instead of being silently misparsed.How this was found
Found while running IronRDP as a client against
gnome-remote-desktopas the RDP server, which sends the full headeredDISPLAYCONTROL_CAPS_PDU. The client'smax_monitor_area()came out tiny, and dynamically-resized monitor layouts sent from the callback were silently dropped as "too large."Test plan
cargo check -p ironrdp-displaycontrolcargo test -p ironrdp-displaycontrol(crate has no unit tests of its own; golden vectors live inironrdp-testsuite-core)cargo test -p ironrdp-testsuite-core -- displaycontrol— all 10 tests pass, includingcapabilities_decodeandinvalid_capscargo clippy -p ironrdp-displaycontrol --all-targets -- -D warningscargo fmt -p ironrdp-displaycontrol -- --check