fix(server): recognize the Initiate Multitransport Response on the message channel - #1964
Conversation
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Active
github-actions
Bot
added
kind/protocol
|
Adding detail: I reproduced this independently today from a different client while testing an unrelated encoder issue. Raw bytes: 04 00 00 00 b9 22 37 e0 00 00 00 00, decoding to MultitransportResponsePdu { security_header: TRANSPORT_RSP, request_id: 0xe03722b9, hr_response: S_OK }. This is the first case I have seen with hr_response=S_OK rather than E_ABORT: the same decode gap drops a successful Initiate Multitransport Response exactly the same way it drops a failed one, so the impact is not limited to the failure path described above. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Malformed multitransport responses currently surface misleading auto-detect decode errors.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Recognizes multitransport responses received on the server’s MCS message channel.
Changes:
- Adds message-channel PDU dispatch.
- Handles and logs multitransport outcomes.
- Adds decoding regression tests.
| File | Description |
|---|---|
crates/ironrdp-server/src/server.rs |
Adds multitransport response handling and tests. |
…ssage channel handle_message_channel_data unconditionally decoded every PDU on the MCS message channel as an AutoDetectRspPdu, per a comment claiming the channel "currently carries only the auto-detect response". That's no longer accurate: once a server sends an Initiate Multitransport Request, the client answers on this same channel with an Initiate Multitransport Response (MS-RDPBCGR 2.2.15.2) if it could not establish the sideband transport. MultitransportResponsePdu already has full Encode/Decode support in ironrdp-pdu, but had zero consumers anywhere in ironrdp-server, so every such response failed to decode (its securityHeader carries SEC_TRANSPORT_RSP, not SEC_AUTODETECT_RSP) and was dropped as an "Unhandled MCS message channel PDU" warning instead of being recognized as the ordinary protocol traffic it is. Extracted the dispatch into decode_message_channel_pdu, tried against both PDU types (auto-detect first, since it is by far the more common one) instead of just one, and given handle_message_channel_data a proper Multitransport arm that logs success/failure at debug level instead of warning on legitimate input.
51c10be to
05ccafa
Compare
GitHub Actions
Active
github-actions
Bot
added
risk/low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly extends MCS message-channel handling to recognize the Client Initiate Multitransport Response alongside auto-detect responses. Independently verified: AutoDetectRspPdu::decode requires flags to contain SEC_AUTODETECT_RSP (0x2000) while MultitransportResponsePdu::decode masks RESET/IGNORE_SEQNO and requires flags to equal SEC_TRANSPORT_RSP (0x0004), so the two decodes are mutually exclusive and the fallback ordering is safe; previously every multitransport response was dropped as an 'Unhandled' warning. Logging-only handling with the session continuing on the main transport matches MS-RDPBCGR 2.2.15.2/3.3.5.15.2, and surfacing the auto-detect error on double failure is sensible and tested. Three new tests cover both decode paths and the error case. Only a low-severity optional cosmetic compression candidate remains; refined before publishing.
…dp-pdu Moves the MCS message-channel demux into ironrdp-pdu as ClientMessageChannelPdu, which dispatches on the Basic Security Header flags so a malformed Initiate Multitransport Response is reported as one rather than as a failed auto-detect decode. Its tests live in ironrdp-testsuite-core; the ones previously added inline in ironrdp-server never ran there. The server logs the response once, in the acceptor's wording.
GitHub Actions
Active
github-actions
Bot
added
risk/high
|
This pull request may overlap with #1954. Both PRs concern the server side of the UDP multitransport exchange on the MCS message channel. This PR decodes the client's Initiate Multitransport Response (MS-RDPBCGR 2.2.15.2) in ironrdp-server's handle_message_channel_data, and This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
There was a problem hiding this comment.
The PR correctly fixes the server dropping client Initiate Multitransport Responses: a flag-dispatched ClientMessageChannelPdu is added in ironrdp-pdu and server.rs now handles the new variant, preserving auto-detect behavior; protocol review found no defect. Two low-severity skeptical findings are published: the speculative #[non_exhaustive] on a spec-complete two-variant enum, and the unreachable Ok catch-all warn arm it forces in server.rs. The contains-based dispatch finding was rejected: the inner MultitransportResponsePdu decoder enforces the masked-equality flag check, so dispatch-on-contains cannot mis-accept traffic, the existing decode_io_channel demux uses the same contains idiom, and AutoDetectRspPdu::decode itself uses contains; residual impact is limited to incomplete error attribution on malformed doubly-flagged input. The code-compressor specialist failed and required no dispositions.
Reduced coverage: optional reviewer code-compressor was unavailable.
github-actions
Bot
added
ai-reviewed/2
MS-RDPBCGR fixes the client-to-server message channel to these two PDUs, and ironrdp-pdu's dispatch enums are exhaustive, so the attribute only forced an unreachable catch-all arm in the server. Both removed.
GitHub Actions
Active
github-actions
Bot
added
the
needs-review
Windows clients answer the Initiate Multitransport Request with E_ABORT about 2.7 s after connecting, after finalization has completed, so the earlier check never saw it and the accept still ran to its 15 s timeout. Now built on Devolutions#1964, which decodes that late response on the message channel: keep the pending accept's abort handle and stop the accept when the response reports failure, and report a cancelled accept as a decline rather than a panic.
Windows clients answer the Initiate Multitransport Request with E_ABORT about 2.7 s after connecting, after finalization has completed, so the earlier check never saw it and the accept still ran to its 15 s timeout. Now built on Devolutions#1964, which decodes that late response on the message channel: keep the pending accept's abort handle and stop the accept when the response reports failure, and report a cancelled accept as a decline rather than a panic.
6aebe6d
into
Devolutions:master
Windows clients answer the Initiate Multitransport Request with E_ABORT about 2.7 s after connecting, after finalization has completed, so the earlier check never saw it and the accept still ran to its 15 s timeout. Now built on Devolutions#1964, which decodes that late response on the message channel: keep the pending accept's abort handle and stop the accept when the response reports failure, and report a cancelled accept as a decline rather than a panic.
Windows clients answer the Initiate Multitransport Request with E_ABORT about 2.7 s after connecting, after finalization has completed, so the earlier check never saw it and the accept still ran to its 15 s timeout. Now built on Devolutions#1964, which decodes that late response on the message channel: keep the pending accept's abort handle and stop the accept when the response reports failure, and report a cancelled accept as a decline rather than a panic.
Windows clients answer the Initiate Multitransport Request with E_ABORT about 2.7 s after connecting, after finalization has completed, so the earlier check never saw it and the accept still ran to its 15 s timeout. Now built on Devolutions#1964, which decodes that late response on the message channel: keep the pending accept's abort handle and stop the accept when the response reports failure, and report a cancelled accept as a decline rather than a panic.
Windows clients answer the Initiate Multitransport Request with E_ABORT about 2.7 s after connecting, after finalization has completed, so the earlier check never saw it and the accept still ran to its 15 s timeout. Now built on Devolutions#1964, which decodes that late response on the message channel: keep the pending accept's abort handle and stop the accept when the response reports failure, and report a cancelled accept as a decline rather than a panic.
Summary
handle_message_channel_data unconditionally decoded every PDU received on the MCS message channel as an AutoDetectRspPdu, per a comment claiming the channel "currently carries only the auto-detect response." That is no longer accurate: once a server sends an Initiate Multitransport Request, the client answers on this same channel with an Initiate Multitransport Response (MS-RDPBCGR 2.2.15.2) to report whether it could establish the sideband transport.
MultitransportResponsePdu already has full Encode/Decode support in ironrdp-pdu (with success()/abort() constructors), but has zero consumers anywhere in ironrdp-server. In practice every such response from a real client failed to decode as AutoDetectRspPdu (its securityHeader carries SEC_TRANSPORT_RSP, not SEC_AUTODETECT_RSP) and was dropped with an "Unhandled MCS message channel PDU" warning: legitimate protocol traffic logged as an error.
Reproduced against a real Windows client (mstsc) that offered UDP multitransport but could not establish it: the raw bytes it sent decode exactly as MultitransportResponsePdu { security_header: TRANSPORT_RSP, request_id, hr_response: E_ABORT }.
Changes
ironrdp_pdu::rdp::message_channel::ClientMessageChannelPdu, an enum with Encode/Decode covering the two PDUs a client sends on this channel. Decode peeks at the Basic Security Header flags and dispatches: SEC_TRANSPORT_RSP to MultitransportResponsePdu, anything else to AutoDetectRspPdu, so a malformed PDU is reported against the type its header names.Test plan
cargo xtask check fmt/lints/tests/typos/locks all pass.