From 5380a019bd96699c949f243198b5caac30666ea9 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 10 Sep 2026 14:04:06 -0500 Subject: [PATCH 1/7] feat(server): add server-side UDP multitransport bootstrapping Add a MultitransportBootstrapping state to the acceptor sequence, entered right after licensing. It advertises UDP multitransport support in the GCC Server MultiTransportChannelData block when configured (set_multitransport_offer), and, once the client reciprocates the reliable-UDP flag, sends the Initiate Multitransport Request (MS-RDPBCGR 2.2.15.1) on the MCS message channel before moving straight on to capability negotiation. The acceptor does not wait for the client's Initiate Multitransport Response before continuing: MS-RDPBCGR 3.2.5.15.1 only obliges the client to send one when Soft-Sync is negotiated or the sideband attempt failed, so blocking on it would stall the handshake on the common successful path. multitransport_request() surfaces the sent request so the caller can establish the sideband UDP transport in parallel. A response that does arrive lands before the mandatory Confirm Active (the client sends it, if at all, before it ever reads Demand Active), so CapabilitiesWaitConfirm recognizes and drops it by channel rather than erroring on the unexpected payload. Adds ironrdp-testsuite-core coverage for the offer/no-offer/no-client- support paths and for the response-before-Confirm-Active ordering. --- Cargo.lock | 1 + crates/ironrdp-acceptor/Cargo.toml | 1 + crates/ironrdp-acceptor/src/connection.rs | 214 +++++++++++++++- .../tests/server/acceptor.rs | 242 +++++++++++++++++- 4 files changed, 454 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 662b8941a2..bb9ca35b28 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2600,6 +2600,7 @@ dependencies = [ "ironrdp-core 0.2.1", "ironrdp-pdu", "ironrdp-svc", + "rand 0.9.4", "tracing", ] diff --git a/crates/ironrdp-acceptor/Cargo.toml b/crates/ironrdp-acceptor/Cargo.toml index 71968d35a4..6f08e9661e 100644 --- a/crates/ironrdp-acceptor/Cargo.toml +++ b/crates/ironrdp-acceptor/Cargo.toml @@ -22,6 +22,7 @@ ironrdp-pdu = { path = "../ironrdp-pdu", version = "0.9" } # public ironrdp-svc = { path = "../ironrdp-svc", version = "0.8" } # public ironrdp-connector = { path = "../ironrdp-connector", version = "0.10" } # public ironrdp-async = { path = "../ironrdp-async", version = "0.10" } # public +rand = "0.9" tracing = { version = "0.1", features = ["log"] } [lints] diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index 182595193e..70d05c2dfa 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -16,6 +16,7 @@ use pdu::rdp::headers::ShareControlPdu; use pdu::rdp::server_error_info::{ErrorInfo, ProtocolIndependentCode, ServerSetErrorInfoPdu}; use pdu::rdp::server_license::{LicensePdu, LicensingErrorMessage}; use pdu::{gcc, mcs, nego, rdp}; +use rand::RngCore as _; use tracing::{debug, warn}; use super::channel_connection::ChannelConnectionSequence; @@ -45,6 +46,14 @@ pub struct Acceptor { received_auto_reconnect: Option, reactivation: bool, honor_client_desktop_size: Option, + /// UDP multitransport flags to advertise to the client and, if it + /// reciprocates, to offer. `None` disables the feature entirely: no + /// Server MultiTransportChannelData block is sent, and no Initiate + /// Multitransport Request follows. See `set_multitransport_offer()`. + offer_multitransport: Option, + /// The Initiate Multitransport Request sent to the client, once + /// `MultitransportBootstrapping` has run. See `multitransport_request()`. + sent_multitransport_request: Option, } /// Minimum and maximum desktop dimension honored from a client. @@ -173,6 +182,8 @@ impl Acceptor { received_auto_reconnect: None, reactivation: false, honor_client_desktop_size: None, + offer_multitransport: None, + sent_multitransport_request: None, } } @@ -220,6 +231,66 @@ impl Acceptor { self.honor_client_desktop_size = max; } + /// Advertise UDP multitransport support (MS-RDPBCGR 2.2.1.4.6) and offer + /// it to clients that reciprocate. + /// + /// Pass `Some(flags)` to send a Server MultiTransportChannelData block + /// with these flags during Basic Settings Exchange, and, once licensing + /// completes, an Initiate Multitransport Request for reliable UDP + /// (`TRANSPORT_TYPE_UDP_FECR`) if `flags` includes it and the client's + /// own Client MultiTransportChannelData reciprocated. Lossy UDP + /// (`TRANSPORT_TYPE_UDP_FECL`) is accepted in `flags` for advertisement + /// purposes but this acceptor never requests it; only the reliable + /// transport is implemented. Include `SOFT_SYNC_TCP_TO_UDP` to also + /// support switching dynamic virtual channels from TCP to UDP after the + /// sideband transport is up; see + /// [`multitransport_soft_sync_negotiated()`](Self::multitransport_soft_sync_negotiated). + /// + /// Offering requires the client to have requested an MCS message + /// channel (MS-RDPBCGR 2.2.1.3.7): both the request and, when owed, the + /// client's response travel on it. If the client never requests one, no + /// request is sent regardless of this setting. + /// + /// `None` is the default: no multitransport block is advertised and no + /// request is ever sent. + pub fn set_multitransport_offer(&mut self, flags: Option) { + self.offer_multitransport = flags; + } + + /// Returns the Initiate Multitransport Request sent to the client, if + /// [`MultitransportBootstrapping`](AcceptorState::MultitransportBootstrapping) + /// has run and decided to offer UDP multitransport. + /// + /// The caller should treat a `Some` here as the signal to begin + /// establishing the sideband UDP transport (RDPEUDP2 + TLS + RDPEMT) + /// using `request_id` and `security_cookie`, in parallel with (not + /// blocking) the rest of the acceptor sequence: this acceptor does not + /// wait for the client's Initiate Multitransport Response before + /// continuing on to capability negotiation, since MS-RDPBCGR 3.2.5.15.1 + /// only obliges the client to send one when Soft-Sync is negotiated or + /// the attempt failed, never on a plain successful bootstrap. + /// + /// `None` before `MultitransportBootstrapping` has run, when + /// multitransport was not offered + /// ([`set_multitransport_offer()`](Self::set_multitransport_offer) + /// disabled or the client did not reciprocate), or on reactivation, + /// where bootstrapping does not run again. + pub fn multitransport_request(&self) -> Option<&rdp::multitransport::MultitransportRequestPdu> { + self.sent_multitransport_request.as_ref() + } + + /// Whether both peers advertised Soft-Sync support for multitransport. + /// + /// Only meaningful once [`multitransport_request()`](Self::multitransport_request) + /// returns `Some`. + pub fn multitransport_soft_sync_negotiated(&self) -> bool { + self.offer_multitransport + .is_some_and(|offer| offer.contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP)) + && self + .multitransport_flags + .contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP) + } + pub fn new_deactivation_reactivation( mut consumed: Acceptor, static_channels: StaticChannelSet, @@ -262,6 +333,8 @@ impl Acceptor { received_auto_reconnect: consumed.received_auto_reconnect, reactivation: true, honor_client_desktop_size: consumed.honor_client_desktop_size, + offer_multitransport: consumed.offer_multitransport, + sent_multitransport_request: consumed.sent_multitransport_request, }) } @@ -413,6 +486,35 @@ pub enum AcceptorState { early_capability: Option, channels: Vec<(u16, gcc::ChannelDef)>, }, + /// After licensing, decide whether to offer UDP multitransport + /// (MS-RDPBCGR 2.2.15.1) and, if so, send the Initiate Multitransport + /// Request. + /// + /// Unlike the client's `MultitransportBootstrapping`, which is purely + /// reactive (it waits to read whatever the server sends), this state is + /// where the server actively decides and writes: it is entered with + /// nothing to read, decides based on the client's advertised + /// `multitransport_flags` and the acceptor's own configured offer, and + /// either sends the request or skips it, either way moving straight on + /// to `CapabilitiesSendServer` in the same step. + /// + /// There is deliberately no state mirroring the client's + /// `MultitransportPending`: MS-RDPBCGR 3.2.5.15.1 only obliges the client + /// to send an Initiate Multitransport Response when Soft-Sync is + /// negotiated or the sideband attempt failed, so on the common + /// successful, non-Soft-Sync path no response is ever sent. Blocking + /// here to read one would stall the handshake forever in exactly that + /// case. Establishing the actual UDP transport (RDPEUDP2 + TLS + RDPEMT) + /// is the caller's responsibility, driven out of band from this request: + /// see [`Acceptor::multitransport_request()`]. Because the client sends + /// its response, if any, before it ever reads the server's Demand + /// Active, a response that does arrive is guaranteed to precede the + /// Confirm Active on the wire; `CapabilitiesWaitConfirm` tolerates and + /// consumes it there rather than this state waiting for it. + MultitransportBootstrapping { + early_capability: Option, + channels: Vec<(u16, gcc::ChannelDef)>, + }, CapabilitiesSendServer { early_capability: Option, channels: Vec<(u16, gcc::ChannelDef)>, @@ -449,6 +551,7 @@ impl State for AcceptorState { Self::RdpSecurityCommencement { .. } => "RdpSecurityCommencement", Self::SecureSettingsExchange { .. } => "SecureSettingsExchange", Self::LicensingExchange { .. } => "LicensingExchange", + Self::MultitransportBootstrapping { .. } => "MultitransportBootstrapping", Self::CapabilitiesSendServer { .. } => "CapabilitiesSendServer", Self::MonitorLayoutSend { .. } => "MonitorLayoutSend", Self::CapabilitiesWaitConfirm { .. } => "CapabilitiesWaitConfirm", @@ -480,6 +583,9 @@ impl Sequence for Acceptor { AcceptorState::RdpSecurityCommencement { .. } => None, AcceptorState::SecureSettingsExchange { .. } => Some(&pdu::X224_HINT), AcceptorState::LicensingExchange { .. } => None, + // Nothing to read: this state decides whether to send a request, + // then moves straight on to CapabilitiesSendServer. + AcceptorState::MultitransportBootstrapping { .. } => None, AcceptorState::CapabilitiesSendServer { .. } => None, AcceptorState::MonitorLayoutSend { .. } => None, AcceptorState::CapabilitiesWaitConfirm { .. } => Some(&pdu::X224_HINT), @@ -729,6 +835,7 @@ impl Sequence for Acceptor { requested_protocol, skip_channel_join, self.message_channel_id, + self.offer_multitransport, ); let settings_response = mcs::ConnectResponse { @@ -866,6 +973,10 @@ impl Sequence for Acceptor { let written = util::encode_send_data_indication(self.user_channel_id, self.io_channel_id, &license, output)?; + // Reactivation (Deactivation-Reactivation Sequence, e.g. a + // display resize) re-enters capability negotiation directly: + // the sideband UDP transport, if any, was already bootstrapped + // once for this connection and is not torn down or re-offered. self.saved_for_reactivation = AcceptorState::CapabilitiesSendServer { early_capability, channels: channels.clone(), @@ -873,13 +984,70 @@ impl Sequence for Acceptor { ( Written::from_size(written)?, - AcceptorState::CapabilitiesSendServer { + AcceptorState::MultitransportBootstrapping { early_capability, channels, }, ) } + AcceptorState::MultitransportBootstrapping { + early_capability, + channels, + } => { + let next_state = AcceptorState::CapabilitiesSendServer { + early_capability, + channels, + }; + + let offer_udp_fecr = self + .offer_multitransport + .is_some_and(|offer| offer.contains(gcc::MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let client_supports_udp_fecr = self + .multitransport_flags + .contains(gcc::MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR); + // 2.2.15.1 requires the request to travel on the MCS message + // channel. A client can in principle advertise UDP support + // without also requesting a message channel; rather than + // failing the whole connection over a mismatch in an optional + // feature's negotiation, that is treated the same as not + // offering. + let message_channel_id = self + .message_channel_id + .filter(|_| offer_udp_fecr && client_supports_udp_fecr); + + if let Some(message_channel_id) = message_channel_id { + let mut security_cookie = [0u8; 16]; + let mut rng = rand::rng(); + rng.fill_bytes(&mut security_cookie); + let request_id = rng.next_u32(); + + let request = rdp::multitransport::MultitransportRequestPdu { + security_header: rdp::headers::BasicSecurityHeader { + flags: rdp::headers::BasicSecurityHeaderFlags::TRANSPORT_REQ, + }, + request_id, + requested_protocol: rdp::multitransport::RequestedProtocol::UdpFecR, + security_cookie, + }; + + debug!(message = ?request, "Send"); + + let written = + util::encode_send_data_indication(self.user_channel_id, message_channel_id, &request, output)?; + + self.sent_multitransport_request = Some(request); + + (Written::from_size(written)?, next_state) + } else { + debug!( + offer_udp_fecr, + client_supports_udp_fecr, "Not offering UDP multitransport" + ); + (Written::Nothing, next_state) + } + } + AcceptorState::CapabilitiesSendServer { early_capability, channels, @@ -956,6 +1124,43 @@ impl Sequence for Acceptor { } }; match message { + // An Initiate Multitransport Response can legitimately land + // here: it travels on the message channel, and the client + // sends it (when it sends one at all) while resolving its + // own multitransport bootstrapping, strictly before it ever + // reads the Demand Active that leads to Confirm Active. So + // it is checked for by channel before assuming the payload + // is a Confirm Active, and simply logged and dropped: this + // acceptor does not gate on it, per the note on + // `AcceptorState::MultitransportBootstrapping`. + mcs::McsMessage::SendDataRequest(data) + if self.sent_multitransport_request.is_some() + && Some(data.channel_id) == self.message_channel_id => + { + match decode::(data.user_data.as_ref()) { + Ok(response) => { + let expected_request_id = + self.sent_multitransport_request.as_ref().map(|r| r.request_id); + if Some(response.request_id) == expected_request_id { + debug!( + request_id = response.request_id, + success = response.is_success(), + "Received Initiate Multitransport Response" + ); + } else { + warn!( + response.request_id, + ?expected_request_id, + "Initiate Multitransport Response request ID does not match the sent request" + ); + } + } + Err(error) => warn!(?error, "Failed to decode Initiate Multitransport Response"), + } + + (Written::Nothing, prev_state) + } + mcs::McsMessage::SendDataRequest(data) => { let capabilities_confirm = decode::(data.user_data.as_ref()) .map_err(ConnectorError::decode); @@ -1039,6 +1244,7 @@ fn create_gcc_blocks( requested: SecurityProtocol, skip_channel_join: bool, message_channel_id: Option, + offer_multitransport: Option, ) -> gcc::ServerGccBlocks { gcc::ServerGccBlocks { core: gcc::ServerCoreData { @@ -1057,6 +1263,10 @@ fn create_gcc_blocks( message_channel: message_channel_id.map(|id| gcc::ServerMessageChannelData { mcs_message_channel_id: id, }), - multi_transport_channel: None, + // Only meaningful alongside a message channel: the request and any + // response it draws both travel there (MS-RDPBCGR 2.2.15.1, 2.2.15.2). + multi_transport_channel: message_channel_id + .and(offer_multitransport) + .map(|flags| gcc::MultiTransportChannelData { flags }), } } diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index ed95dd60f7..3740573c60 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -1,11 +1,16 @@ +use std::borrow::Cow; + use ironrdp_acceptor::Acceptor; use ironrdp_connector::{DesktopSize, Sequence as _, Written, encode_x224_packet}; -use ironrdp_core::{WriteBuf, decode}; -use ironrdp_pdu::gcc::ClientMessageChannelData; +use ironrdp_core::{WriteBuf, decode, encode_vec}; +use ironrdp_pdu::gcc::{ClientMessageChannelData, MultiTransportChannelData, MultiTransportFlags}; use ironrdp_pdu::mcs::{self, ConnectInitial}; use ironrdp_pdu::nego::{self, SecurityProtocol}; +use ironrdp_pdu::rdp::headers::BasicSecurityHeaderFlags; +use ironrdp_pdu::rdp::multitransport::{MultitransportRequestPdu, MultitransportResponsePdu, RequestedProtocol}; use ironrdp_pdu::x224::{X224, X224Data}; use ironrdp_testsuite_core::gcc::CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS; +use ironrdp_testsuite_core::rdp::{CLIENT_DEMAND_ACTIVE_PDU_BUFFER, CLIENT_INFO_PDU_BUFFER}; /// Build a minimal ConnectionRequest with the given protocols and encode it. fn encode_connection_request(protocol: SecurityProtocol) -> Vec { @@ -188,3 +193,236 @@ fn neg_failure_hybrid_required() { } } } + +fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { + let mut buf = WriteBuf::new(); + ironrdp_core::encode_buf( + &X224(mcs::SendDataRequest { + initiator_id, + channel_id, + user_data: Cow::Borrowed(user_data), + }), + &mut buf, + ) + .unwrap(); + buf.filled().to_vec() +} + +/// Drives `acceptor` from a fresh `InitiationWaitRequest` through +/// `SecureSettingsExchange`, i.e. everything that precedes licensing and is +/// identical regardless of what the multitransport tests below want to +/// exercise: negotiation, TLS upgrade marker, GCC exchange (with the given +/// client blocks) and the full MCS channel join sequence, then the Client +/// Info PDU. Returns `(user_channel_id, io_channel_id, message_channel_id)` +/// as actually assigned by the acceptor, so callers never have to hard-code +/// them. +fn drive_to_secure_settings_exchange( + acceptor: &mut Acceptor, + client_blocks: ironrdp_pdu::gcc::ClientGccBlocks, +) -> (u16, u16, Option) { + let request_bytes = encode_connection_request(SecurityProtocol::SSL); + acceptor.step(&request_bytes, None, &mut WriteBuf::new()).unwrap(); + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); + acceptor.mark_security_upgrade_as_done(); + + let connect_initial = ConnectInitial::with_gcc_blocks(client_blocks).unwrap(); + let mut initial_buf = WriteBuf::new(); + encode_x224_packet(&connect_initial, &mut initial_buf).unwrap(); + acceptor.step(initial_buf.filled(), None, &mut WriteBuf::new()).unwrap(); + + let mut output = WriteBuf::new(); + acceptor.step(&[], None, &mut output).unwrap(); + let payload = decode::>>(output.filled()).unwrap().0; + let response = decode::(payload.data.as_ref()).unwrap(); + let server_blocks = response.conference_create_response.gcc_blocks(); + let io_channel_id = server_blocks.network.io_channel; + let message_channel_id = server_blocks.message_channel.as_ref().map(|m| m.mcs_message_channel_id); + + // Erect Domain Request, Attach User Request, then confirm. + let mut buf = WriteBuf::new(); + ironrdp_core::encode_buf( + &X224(mcs::ErectDomainPdu { + sub_height: 0, + sub_interval: 0, + }), + &mut buf, + ) + .unwrap(); + acceptor.step(buf.filled(), None, &mut WriteBuf::new()).unwrap(); + + let mut buf = WriteBuf::new(); + ironrdp_core::encode_buf(&X224(mcs::AttachUserRequest), &mut buf).unwrap(); + acceptor.step(buf.filled(), None, &mut WriteBuf::new()).unwrap(); + + let mut output = WriteBuf::new(); + acceptor.step(&[], None, &mut output).unwrap(); + let attach_user_confirm = decode::>(output.filled()).unwrap().0; + let user_channel_id = attach_user_confirm.initiator_id; + + // Join every channel the server expects: the user and I/O channels, plus + // the message channel when one was negotiated. No other static channels + // are requested by `client_blocks.network` in these tests. + let mut to_join = vec![user_channel_id, io_channel_id]; + to_join.extend(message_channel_id); + for channel_id in to_join { + let mut buf = WriteBuf::new(); + ironrdp_core::encode_buf( + &X224(mcs::ChannelJoinRequest { + initiator_id: user_channel_id, + channel_id, + }), + &mut buf, + ) + .unwrap(); + acceptor.step(buf.filled(), None, &mut WriteBuf::new()).unwrap(); + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); + } + + // RdpSecurityCommencement (no input) -> SecureSettingsExchange, then the + // Client Info PDU on the I/O channel. + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); + let client_info = encode_send_data_request(user_channel_id, io_channel_id, &CLIENT_INFO_PDU_BUFFER); + acceptor.step(&client_info, None, &mut WriteBuf::new()).unwrap(); + + (user_channel_id, io_channel_id, message_channel_id) +} + +fn client_gcc_with_message_channel_and_multitransport( + offer: Option, +) -> ironrdp_pdu::gcc::ClientGccBlocks { + let mut blocks = CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS.clone(); + blocks.network = None; + blocks.message_channel = Some(ClientMessageChannelData); + blocks.multi_transport_channel = offer.map(|flags| MultiTransportChannelData { flags }); + blocks +} + +/// The full happy path: the acceptor offers reliable UDP multitransport, the +/// client reciprocates, so the request goes out on the message channel and +/// `multitransport_request()` surfaces it. A late Initiate Multitransport +/// Response then arrives, interleaved before the mandatory Confirm Active +/// exactly as a real client would send it (MS-RDPBCGR 3.2.5.15.1: sent while +/// resolving its own bootstrapping, strictly before it ever reads Demand +/// Active), and the acceptor must tolerate it rather than erroring out while +/// still reaching capabilities confirmation. +#[test] +fn multitransport_offered_and_client_reciprocates() { + let mut acceptor = Acceptor::new( + SecurityProtocol::SSL, + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let client_blocks = + client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let (user_channel_id, _io_channel_id, message_channel_id) = + drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + let message_channel_id = message_channel_id.expect("message channel negotiated"); + + // LicensingExchange (sends license) -> MultitransportBootstrapping. + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); + + assert!( + acceptor.multitransport_request().is_none(), + "no request sent until MultitransportBootstrapping actually runs" + ); + + // MultitransportBootstrapping: decides to offer, sends the request. + let mut output = WriteBuf::new(); + let written = acceptor.step(&[], None, &mut output).unwrap(); + assert!(!matches!(written, Written::Nothing), "expected the request to be sent"); + + let sent_request = acceptor + .multitransport_request() + .expect("request recorded after MultitransportBootstrapping") + .clone(); + assert_eq!(sent_request.requested_protocol, RequestedProtocol::UdpFecR); + assert_eq!( + sent_request.security_header.flags, + BasicSecurityHeaderFlags::TRANSPORT_REQ + ); + + let ctx = mcs::decode_send_data_indication(output.filled()).unwrap(); + assert_eq!( + ctx.channel_id, message_channel_id, + "request must go out on the message channel" + ); + let on_wire_request = decode::(ctx.user_data).unwrap(); + assert_eq!(on_wire_request, sent_request); + + // CapabilitiesSendServer: sends Demand Active, moves on to CapabilitiesWaitConfirm + // (no monitor-layout support was advertised). + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); + assert_eq!(acceptor.state().name(), "CapabilitiesWaitConfirm"); + + // The client's Initiate Multitransport Response, arriving before Confirm Active. + let response = MultitransportResponsePdu::success(sent_request.request_id); + let response_bytes = encode_send_data_request(user_channel_id, message_channel_id, &encode_vec(&response).unwrap()); + let written = acceptor.step(&response_bytes, None, &mut WriteBuf::new()).unwrap(); + assert!(matches!(written, Written::Nothing)); + assert_eq!( + acceptor.state().name(), + "CapabilitiesWaitConfirm", + "the response must not advance past capabilities waiting" + ); + + // Now the actual Confirm Active. + let confirm_active = encode_send_data_request(user_channel_id, _io_channel_id, &CLIENT_DEMAND_ACTIVE_PDU_BUFFER); + acceptor.step(&confirm_active, None, &mut WriteBuf::new()).unwrap(); + assert_eq!(acceptor.state().name(), "ConnectionFinalization"); +} + +/// Multitransport disabled (the default): no Server MultiTransportChannelData +/// block is advertised even though the client supports it, and no Initiate +/// Multitransport Request is ever sent. +#[test] +fn multitransport_not_offered_by_default() { + let mut acceptor = Acceptor::new( + SecurityProtocol::SSL, + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + + let client_blocks = + client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange + let written = acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping + assert!(matches!(written, Written::Nothing)); + assert!(acceptor.multitransport_request().is_none()); + assert!(!acceptor.multitransport_soft_sync_negotiated()); +} + +/// The acceptor offers multitransport, but the client's GCC blocks never +/// advertised reliable UDP support: nothing is sent. +#[test] +fn multitransport_not_offered_when_client_does_not_reciprocate() { + let mut acceptor = Acceptor::new( + SecurityProtocol::SSL, + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let client_blocks = client_gcc_with_message_channel_and_multitransport(None); + drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange + let written = acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping + assert!(matches!(written, Written::Nothing)); + assert!(acceptor.multitransport_request().is_none()); +} From 8545c1cf2bdcae4da7e367f401887cb585a24e61 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 14:36:50 -0500 Subject: [PATCH 2/7] fix(server): tighten and correct multitransport bootstrapping edge cases The Server MultiTransportChannelData block was advertised whenever the server's own offer was configured, regardless of whether the client actually populated its own Client MultiTransportChannelData block. MS-RDPBCGR 2.2.1.4 requires the server block to be omitted when the client did not send one. Added a client_offered_multitransport field tracking presence separately from multitransport_flags, which alone can't distinguish "absent" from "present but empty", and gated the offer on it. The Initiate Multitransport Response has no fixed position relative to the rest of the handshake (3.2.5.15.1): a conforming client can send it after Confirm Active, during ConnectionFinalization, not only before it. None of FinalizationSequence's own PDU decoders expect it, so depending which sub-state was active a late response was silently swallowed while advancing a state, propagated as a connection-ending decode error, or surfaced to the embedding application as a raw input event. Added the same tolerance CapabilitiesWaitConfirm already had to ConnectionFinalization. The late-response guard itself only checked channel and outstanding- request, not whether the payload actually decoded as a response. Since the message channel also carries Auto-Detect Response and Heartbeat PDUs (2.2.1.4.5, 2.2.8.1.1.2.1), that traffic was misclassified and dropped instead of falling through to its own handling. The guard now requires a successful strict decode, mirroring how ClientConnectorState::ConnectTimeAutoDetection demuxes the same channel client-side. Also: corrected the multitransport_request() doc, which claimed it returns None on reactivation when the carried-forward request in fact keeps it Some (intentional, needed for the late-response guard to keep working across reactivation); simplified an Option round-trip in the response-logging path down to a direct comparison, since the calling guard already guarantees a request is outstanding; and fixed two test cases constructing an S_OK response without the server advertising Soft-Sync, which 2.2.15.2 disallows. Regression tests added for the finalization tolerance and the non-response message-channel traffic case; both verified to fail against the prior behavior and pass with the fix. --- crates/ironrdp-acceptor/src/connection.rs | 199 ++++++++++++----- .../tests/server/acceptor.rs | 206 +++++++++++++++++- 2 files changed, 341 insertions(+), 64 deletions(-) diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index 70d05c2dfa..b113b7ce2b 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -37,6 +37,12 @@ pub struct Acceptor { keyboard_type: gcc::KeyboardType, ime_file_name: String, multitransport_flags: gcc::MultiTransportFlags, + /// Whether the client sent a Client MultiTransportChannelData block at all + /// (MS-RDPBCGR 2.2.1.3.8), independent of what flags it carried. The + /// server's own block MUST be omitted when the client did not populate + /// this field (2.2.1.4), which `multitransport_flags` alone can't express + /// since it collapses "absent" and "present but empty" together. + client_offered_multitransport: bool, early_capability_flags: gcc::ClientEarlyCapabilityFlags, server_capabilities: Vec, static_channels: StaticChannelSet, @@ -173,6 +179,7 @@ impl Acceptor { keyboard_type: gcc::KeyboardType(0), ime_file_name: String::new(), multitransport_flags: gcc::MultiTransportFlags::empty(), + client_offered_multitransport: false, early_capability_flags: gcc::ClientEarlyCapabilityFlags::empty(), server_capabilities: capabilities, static_channels: StaticChannelSet::new(), @@ -270,11 +277,14 @@ impl Acceptor { /// only obliges the client to send one when Soft-Sync is negotiated or /// the attempt failed, never on a plain successful bootstrap. /// - /// `None` before `MultitransportBootstrapping` has run, when + /// `None` before `MultitransportBootstrapping` has run, or when /// multitransport was not offered /// ([`set_multitransport_offer()`](Self::set_multitransport_offer) - /// disabled or the client did not reciprocate), or on reactivation, - /// where bootstrapping does not run again. + /// disabled or the client did not reciprocate). Bootstrapping does not + /// run again on reactivation, so no new request is sent then, but a + /// request from before reactivation carries forward and is still + /// returned here: `CapabilitiesWaitConfirm`'s late-response tolerance + /// needs it to remain visible across reactivation too. pub fn multitransport_request(&self) -> Option<&rdp::multitransport::MultitransportRequestPdu> { self.sent_multitransport_request.as_ref() } @@ -291,6 +301,59 @@ impl Acceptor { .contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP) } + /// If `data` (an MCS SendDataRequest already decoded from the wire) is on + /// the message channel while a multitransport request is outstanding AND + /// its payload strictly decodes as an Initiate Multitransport Response, + /// returns it. MS-RDPBCGR 3.2.5.15.1 gives this response no fixed + /// position relative to the rest of the handshake: it depends on when + /// the client resolves its own bootstrapping and whether the sideband + /// attempt failed, so both `CapabilitiesWaitConfirm` and + /// `ConnectionFinalization` tolerate it landing wherever it actually + /// shows up rather than only where `MultitransportBootstrapping`'s own + /// comment describes as typical. + /// + /// The message channel also carries Auto-Detect Response and Heartbeat + /// PDUs (2.2.1.4.5, 2.2.8.1.1.2.1), so a channel-and-outstanding-request + /// check alone would misclassify that traffic too; requiring the decode + /// to actually succeed here lets callers fall through to their own + /// handling for anything that isn't really a response, mirroring how + /// `ClientConnectorState::ConnectTimeAutoDetection` demuxes the same + /// channel client-side. + fn late_multitransport_response( + &self, + data: &mcs::SendDataRequest<'_>, + ) -> Option { + if !(self.sent_multitransport_request.is_some() && Some(data.channel_id) == self.message_channel_id) { + return None; + } + decode::(data.user_data.as_ref()).ok() + } + + /// Logs a received Initiate Multitransport Response against the + /// outstanding request, matching request IDs. Shared by the two call + /// sites `late_multitransport_response` gates; both only call this once + /// that method has confirmed a request is outstanding, so + /// `sent_multitransport_request` is always `Some` here. + fn log_multitransport_response(&self, response: &rdp::multitransport::MultitransportResponsePdu) { + let expected_request_id = self + .sent_multitransport_request + .as_ref() + .expect("late_multitransport_response only returns Some when a request is outstanding") + .request_id; + if response.request_id == expected_request_id { + debug!( + request_id = response.request_id, + success = response.is_success(), + "Received Initiate Multitransport Response" + ); + } else { + warn!( + response.request_id, + expected_request_id, "Initiate Multitransport Response request ID does not match the sent request" + ); + } + } + pub fn new_deactivation_reactivation( mut consumed: Acceptor, static_channels: StaticChannelSet, @@ -324,6 +387,7 @@ impl Acceptor { keyboard_type: consumed.keyboard_type, ime_file_name: consumed.ime_file_name, multitransport_flags: consumed.multitransport_flags, + client_offered_multitransport: consumed.client_offered_multitransport, early_capability_flags: consumed.early_capability_flags, server_capabilities: consumed.server_capabilities, static_channels, @@ -508,9 +572,13 @@ pub enum AcceptorState { /// is the caller's responsibility, driven out of band from this request: /// see [`Acceptor::multitransport_request()`]. Because the client sends /// its response, if any, before it ever reads the server's Demand - /// Active, a response that does arrive is guaranteed to precede the - /// Confirm Active on the wire; `CapabilitiesWaitConfirm` tolerates and - /// consumes it there rather than this state waiting for it. + /// Active, IronRDP's own client always sends one (if at all) before the + /// Confirm Active on the wire. That is a client behavior, not a + /// protocol guarantee 3.2.5.15.1 makes: a conforming third-party client + /// could just as legitimately send it later, during finalization. Both + /// `CapabilitiesWaitConfirm` and `ConnectionFinalization` tolerate and + /// consume it wherever it actually lands, rather than this state + /// waiting for it. MultitransportBootstrapping { early_capability: Option, channels: Vec<(u16, gcc::ChannelDef)>, @@ -727,6 +795,7 @@ impl Sequence for Acceptor { self.keyboard_layout = gcc_blocks.core.keyboard_layout; self.keyboard_type = gcc_blocks.core.keyboard_type; self.ime_file_name.clone_from(&gcc_blocks.core.ime_file_name); + self.client_offered_multitransport = gcc_blocks.multi_transport_channel.is_some(); self.multitransport_flags = gcc_blocks .multi_transport_channel .as_ref() @@ -835,7 +904,7 @@ impl Sequence for Acceptor { requested_protocol, skip_channel_join, self.message_channel_id, - self.offer_multitransport, + self.offer_multitransport.filter(|_| self.client_offered_multitransport), ); let settings_response = mcs::ConnectResponse { @@ -1123,44 +1192,30 @@ impl Sequence for Acceptor { } } }; - match message { - // An Initiate Multitransport Response can legitimately land - // here: it travels on the message channel, and the client - // sends it (when it sends one at all) while resolving its - // own multitransport bootstrapping, strictly before it ever - // reads the Demand Active that leads to Confirm Active. So - // it is checked for by channel before assuming the payload - // is a Confirm Active, and simply logged and dropped: this - // acceptor does not gate on it, per the note on - // `AcceptorState::MultitransportBootstrapping`. - mcs::McsMessage::SendDataRequest(data) - if self.sent_multitransport_request.is_some() - && Some(data.channel_id) == self.message_channel_id => - { - match decode::(data.user_data.as_ref()) { - Ok(response) => { - let expected_request_id = - self.sent_multitransport_request.as_ref().map(|r| r.request_id); - if Some(response.request_id) == expected_request_id { - debug!( - request_id = response.request_id, - success = response.is_success(), - "Received Initiate Multitransport Response" - ); - } else { - warn!( - response.request_id, - ?expected_request_id, - "Initiate Multitransport Response request ID does not match the sent request" - ); - } - } - Err(error) => warn!(?error, "Failed to decode Initiate Multitransport Response"), - } + // An Initiate Multitransport Response can legitimately land here: + // it travels on the message channel, and the client sends it + // (when it sends one at all) while resolving its own multitransport + // bootstrapping, strictly before it ever reads the Demand Active + // that leads to Confirm Active. So it is checked for by channel + // and a successful strict decode before assuming the payload is a + // Confirm Active, and simply logged and dropped: this acceptor + // does not gate on it, per the note on + // `AcceptorState::MultitransportBootstrapping`. A decode failure + // here means the message-channel traffic isn't a response at all + // (Auto-Detect Response, Heartbeat), so it falls through to the + // Confirm Active handling below instead. + let late_multitransport_response = match &message { + mcs::McsMessage::SendDataRequest(data) => self.late_multitransport_response(data), + _ => None, + }; - (Written::Nothing, prev_state) - } + if let Some(response) = late_multitransport_response { + self.log_multitransport_response(&response); + self.state = prev_state; + return Ok(Written::Nothing); + } + match message { mcs::McsMessage::SendDataRequest(data) => { let capabilities_confirm = decode::(data.user_data.as_ref()) .map_err(ConnectorError::decode); @@ -1211,23 +1266,50 @@ impl Sequence for Acceptor { channels, client_capabilities, } => { - let written = finalization.step(input, received_at, output)?; + // A late Initiate Multitransport Response can land in any + // finalization sub-state (see `late_multitransport_response`); + // none of FinalizationSequence's own PDU decoders expect it, and + // depending which sub-state is active it would otherwise be + // silently swallowed while advancing a state, propagated as a + // connection-ending decode error, or surfaced to the embedding + // application as a raw input event. Check for it here, before + // finalization ever sees the bytes, mirroring + // `CapabilitiesWaitConfirm`'s handling. + let late_multitransport_response = match decode::>>(input) { + Ok(X224(mcs::McsMessage::SendDataRequest(data))) => self.late_multitransport_response(&data), + _ => None, + }; - let state = if finalization.is_done() { - AcceptorState::Accepted { - channels, - client_capabilities, - input_events: finalization.into_input_events(), - } + if let Some(response) = late_multitransport_response { + self.log_multitransport_response(&response); + + ( + Written::Nothing, + AcceptorState::ConnectionFinalization { + finalization, + channels, + client_capabilities, + }, + ) } else { - AcceptorState::ConnectionFinalization { - finalization, - channels, - client_capabilities, - } - }; + let written = finalization.step(input, received_at, output)?; - (written, state) + let state = if finalization.is_done() { + AcceptorState::Accepted { + channels, + client_capabilities, + input_events: finalization.into_input_events(), + } + } else { + AcceptorState::ConnectionFinalization { + finalization, + channels, + client_capabilities, + } + }; + + (written, state) + } } _ => unreachable!(), @@ -1265,6 +1347,9 @@ fn create_gcc_blocks( }), // Only meaningful alongside a message channel: the request and any // response it draws both travel there (MS-RDPBCGR 2.2.15.1, 2.2.15.2). + // The caller has already filtered offer_multitransport to None when + // the client did not populate its own MultiTransportChannelData + // block, per 2.2.1.4's requirement that this block be omitted then. multi_transport_channel: message_channel_id .and(offer_multitransport) .map(|flags| gcc::MultiTransportChannelData { flags }), diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index 3740573c60..c90c4cb01b 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -6,7 +6,12 @@ use ironrdp_core::{WriteBuf, decode, encode_vec}; use ironrdp_pdu::gcc::{ClientMessageChannelData, MultiTransportChannelData, MultiTransportFlags}; use ironrdp_pdu::mcs::{self, ConnectInitial}; use ironrdp_pdu::nego::{self, SecurityProtocol}; -use ironrdp_pdu::rdp::headers::BasicSecurityHeaderFlags; +use ironrdp_pdu::rdp::client_info::CompressionType; +use ironrdp_pdu::rdp::finalization_messages::{ControlAction, ControlPdu, SynchronizePdu}; +use ironrdp_pdu::rdp::headers::{ + BasicSecurityHeaderFlags, CompressionFlags, ShareControlHeader, ShareControlPdu, ShareDataHeader, ShareDataPdu, + StreamPriority, +}; use ironrdp_pdu::rdp::multitransport::{MultitransportRequestPdu, MultitransportResponsePdu, RequestedProtocol}; use ironrdp_pdu::x224::{X224, X224Data}; use ironrdp_testsuite_core::gcc::CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS; @@ -208,6 +213,23 @@ fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8] buf.filled().to_vec() } +/// Encode a client finalization ShareData PDU (Synchronize, Control, FontList), +/// mirroring the shape `ironrdp_acceptor`'s own `wrap_share_data` uses for the +/// server's confirmations of the same messages. +fn encode_client_share_data(pdu: ShareDataPdu) -> Vec { + let header = ShareControlHeader { + share_id: 0, + pdu_source: 0, + share_control_pdu: ShareControlPdu::Data(ShareDataHeader { + share_data_pdu: pdu, + stream_priority: StreamPriority::Undefined, + compression_flags: CompressionFlags::empty(), + compression_type: CompressionType::K8, + }), + }; + encode_vec(&header).unwrap() +} + /// Drives `acceptor` from a fresh `InitiationWaitRequest` through /// `SecureSettingsExchange`, i.e. everything that precedes licensing and is /// identical regardless of what the multitransport tests below want to @@ -219,7 +241,7 @@ fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8] fn drive_to_secure_settings_exchange( acceptor: &mut Acceptor, client_blocks: ironrdp_pdu::gcc::ClientGccBlocks, -) -> (u16, u16, Option) { +) -> (u16, u16, Option, Option) { let request_bytes = encode_connection_request(SecurityProtocol::SSL); acceptor.step(&request_bytes, None, &mut WriteBuf::new()).unwrap(); acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); @@ -237,6 +259,7 @@ fn drive_to_secure_settings_exchange( let server_blocks = response.conference_create_response.gcc_blocks(); let io_channel_id = server_blocks.network.io_channel; let message_channel_id = server_blocks.message_channel.as_ref().map(|m| m.mcs_message_channel_id); + let server_multitransport = server_blocks.multi_transport_channel.as_ref().map(|m| m.flags); // Erect Domain Request, Attach User Request, then confirm. let mut buf = WriteBuf::new(); @@ -284,7 +307,12 @@ fn drive_to_secure_settings_exchange( let client_info = encode_send_data_request(user_channel_id, io_channel_id, &CLIENT_INFO_PDU_BUFFER); acceptor.step(&client_info, None, &mut WriteBuf::new()).unwrap(); - (user_channel_id, io_channel_id, message_channel_id) + ( + user_channel_id, + io_channel_id, + message_channel_id, + server_multitransport, + ) } fn client_gcc_with_message_channel_and_multitransport( @@ -320,9 +348,14 @@ fn multitransport_offered_and_client_reciprocates() { let client_blocks = client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); - let (user_channel_id, _io_channel_id, message_channel_id) = + let (user_channel_id, _io_channel_id, message_channel_id, server_multitransport) = drive_to_secure_settings_exchange(&mut acceptor, client_blocks); let message_channel_id = message_channel_id.expect("message channel negotiated"); + assert_eq!( + server_multitransport, + Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR), + "server should advertise the offer once the client reciprocated" + ); // LicensingExchange (sends license) -> MultitransportBootstrapping. acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); @@ -361,7 +394,10 @@ fn multitransport_offered_and_client_reciprocates() { assert_eq!(acceptor.state().name(), "CapabilitiesWaitConfirm"); // The client's Initiate Multitransport Response, arriving before Confirm Active. - let response = MultitransportResponsePdu::success(sent_request.request_id); + // MS-RDPBCGR 2.2.15.2: S_OK MUST only be sent to a server advertising + // SOFTSYNC_TCP_TO_UDP, which this test's offer does not include; the + // legitimate response here is a failure code. + let response = MultitransportResponsePdu::abort(sent_request.request_id); let response_bytes = encode_send_data_request(user_channel_id, message_channel_id, &encode_vec(&response).unwrap()); let written = acceptor.step(&response_bytes, None, &mut WriteBuf::new()).unwrap(); assert!(matches!(written, Written::Nothing)); @@ -377,6 +413,156 @@ fn multitransport_offered_and_client_reciprocates() { assert_eq!(acceptor.state().name(), "ConnectionFinalization"); } +/// The message channel also carries Auto-Detect Response and Heartbeat PDUs +/// (MS-RDPBCGR 2.2.1.4.5, 2.2.8.1.1.2.1), not just the Initiate Multitransport +/// Response. A guard keyed only on channel and outstanding-request, without +/// verifying the payload actually decodes as a response, would misclassify +/// that other traffic and drop it silently instead of letting the caller's +/// own (pre-existing, unrelated to this fix) handling see it. +#[test] +fn non_response_traffic_on_the_message_channel_is_not_misclassified() { + let mut acceptor = Acceptor::new( + SecurityProtocol::SSL, + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let client_blocks = + client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let (user_channel_id, _io_channel_id, message_channel_id, _) = + drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + let message_channel_id = message_channel_id.expect("message channel negotiated"); + + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping: sends request + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // CapabilitiesSendServer: sends Demand Active + assert_eq!(acceptor.state().name(), "CapabilitiesWaitConfirm"); + + // Traffic on the message channel that is not a MultitransportResponsePdu + // (its wire format is just requestId/hrResponse, so arbitrary bytes of a + // different shape and length reliably fail that specific decode). Before + // the fix, the old channel-only guard treated this as a response and + // silently dropped it (`Ok(Written::Nothing)`, no change of state, + // forever). After the fix, it falls through to the same handling any + // other unexpected traffic already gets here (a decode error), rather + // than being silently and permanently swallowed. + let not_a_response = encode_send_data_request(user_channel_id, message_channel_id, &[0xAA; 3]); + let result = acceptor.step(¬_a_response, None, &mut WriteBuf::new()); + assert!( + result.is_err(), + "non-response message-channel traffic must not be silently swallowed as a response" + ); +} + +/// MS-RDPBCGR 3.2.5.15.1 gives the Initiate Multitransport Response no fixed +/// position relative to the rest of the handshake: it depends on when the +/// client resolves its own bootstrapping and whether the sideband attempt +/// failed, so a conforming client can send it after Confirm Active, during +/// finalization, not only before it as `multitransport_offered_and_client_reciprocates` +/// exercises. `WaitRequestControl` is the sharpest of FinalizationSequence's +/// sub-states to hit: unlike `WaitSynchronize`/`WaitControlCooperate` (which +/// discard a decode error and advance anyway) or `WaitFontList` (which retries +/// on a decode error, tolerating one stray message), it propagates a decode +/// failure with `?`, so without tolerance the response's raw bytes (a bare +/// requestId/hrResponse pair, not a ShareControlHeader at all) fail to decode +/// and the connection is dropped outright. +#[test] +fn multitransport_response_arriving_during_finalization_is_tolerated() { + let mut acceptor = Acceptor::new( + SecurityProtocol::SSL, + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let client_blocks = + client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let (user_channel_id, io_channel_id, message_channel_id, _) = + drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + let message_channel_id = message_channel_id.expect("message channel negotiated"); + + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping: sends request + let sent_request = acceptor.multitransport_request().expect("request sent").clone(); + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // CapabilitiesSendServer: sends Demand Active + + let confirm_active = encode_send_data_request(user_channel_id, io_channel_id, &CLIENT_DEMAND_ACTIVE_PDU_BUFFER); + acceptor.step(&confirm_active, None, &mut WriteBuf::new()).unwrap(); + assert_eq!(acceptor.state().name(), "ConnectionFinalization"); + + // Drive the real Synchronize and ControlCooperate exchange first, reaching + // WaitRequestControl. + let synchronize = encode_send_data_request( + user_channel_id, + io_channel_id, + &encode_client_share_data(ShareDataPdu::Synchronize(SynchronizePdu { target_user_id: 0 })), + ); + acceptor.step(&synchronize, None, &mut WriteBuf::new()).unwrap(); + + let cooperate = encode_send_data_request( + user_channel_id, + io_channel_id, + &encode_client_share_data(ShareDataPdu::Control(ControlPdu { + action: ControlAction::Cooperate, + grant_id: 0, + control_id: 0, + })), + ); + acceptor.step(&cooperate, None, &mut WriteBuf::new()).unwrap(); + + // The late response, arriving while WaitRequestControl is active. + // MS-RDPBCGR 2.2.15.2: S_OK MUST only be sent to a server advertising + // SOFTSYNC_TCP_TO_UDP, which this test's offer does not include; the + // legitimate response here is a failure code. + let response = MultitransportResponsePdu::abort(sent_request.request_id); + let response_bytes = encode_send_data_request(user_channel_id, message_channel_id, &encode_vec(&response).unwrap()); + let written = acceptor + .step(&response_bytes, None, &mut WriteBuf::new()) + .expect("a late response must not drop the connection"); + assert!(matches!(written, Written::Nothing)); + assert_eq!( + acceptor.state().name(), + "ConnectionFinalization", + "a late response must not be treated as RequestControl" + ); + + // The real remainder of the sequence, undisturbed by the late response above. + let request_control = encode_send_data_request( + user_channel_id, + io_channel_id, + &encode_client_share_data(ShareDataPdu::Control(ControlPdu { + action: ControlAction::RequestControl, + grant_id: 0, + control_id: 0, + })), + ); + acceptor.step(&request_control, None, &mut WriteBuf::new()).unwrap(); + + let font_list = encode_send_data_request( + user_channel_id, + io_channel_id, + &encode_client_share_data(ShareDataPdu::FontList(Default::default())), + ); + acceptor.step(&font_list, None, &mut WriteBuf::new()).unwrap(); + + // The server's four confirmation sends, completing finalization. + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // SendSynchronizeConfirm + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // SendControlCooperateConfirm + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // SendGrantedControlConfirm + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // SendFontMap -> Accepted + + assert_eq!(acceptor.state().name(), "Connected"); +} + /// Multitransport disabled (the default): no Server MultiTransportChannelData /// block is advertised even though the client supports it, and no Initiate /// Multitransport Request is ever sent. @@ -404,7 +590,9 @@ fn multitransport_not_offered_by_default() { } /// The acceptor offers multitransport, but the client's GCC blocks never -/// advertised reliable UDP support: nothing is sent. +/// advertised reliable UDP support: nothing is sent, and per MS-RDPBCGR +/// 2.2.1.4 the server's own GCC response must omit the block entirely +/// rather than echo the offer the client never reciprocated. #[test] fn multitransport_not_offered_when_client_does_not_reciprocate() { let mut acceptor = Acceptor::new( @@ -419,7 +607,11 @@ fn multitransport_not_offered_when_client_does_not_reciprocate() { acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); let client_blocks = client_gcc_with_message_channel_and_multitransport(None); - drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + let (.., server_multitransport) = drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + assert_eq!( + server_multitransport, None, + "server must not advertise MultiTransportChannelData when the client didn't send one" + ); acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange let written = acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping From 19b37fc5d06168ea579135eafbdb7f4601130854 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 17:59:32 -0500 Subject: [PATCH 3/7] review: address automated review findings on multitransport bootstrapping Documents the interim limitation of set_multitransport_offer: this acceptor only sends the request, it does not establish the sideband UDP transport itself. Marks AcceptorState non_exhaustive, matching ClientConnectorState's convention. Changes multitransport_soft_sync_negotiated to return Option, None before a request was actually sent, rather than deriving from GCC flags alone which could report true with nothing sent. Replaces the client_offered_multitransport bool with a single multitransport_flags: Option field, removing the duplicated absent-vs-empty distinction. Merges log_multitransport_response into late_multitransport_response, removing the panic-prone two-step coupling, and folds the CapabilitiesWaitConfirm pre-check into the main match arm. Adds a multitransport_acceptor(offer) test factory, removing repeated setup across four tests. --- crates/ironrdp-acceptor/src/connection.rs | 139 +++++++++--------- .../tests/server/acceptor.rs | 76 ++++------ 2 files changed, 92 insertions(+), 123 deletions(-) diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index b113b7ce2b..5ebb346567 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -36,13 +36,12 @@ pub struct Acceptor { keyboard_layout: u32, keyboard_type: gcc::KeyboardType, ime_file_name: String, - multitransport_flags: gcc::MultiTransportFlags, - /// Whether the client sent a Client MultiTransportChannelData block at all - /// (MS-RDPBCGR 2.2.1.3.8), independent of what flags it carried. The - /// server's own block MUST be omitted when the client did not populate - /// this field (2.2.1.4), which `multitransport_flags` alone can't express - /// since it collapses "absent" and "present but empty" together. - client_offered_multitransport: bool, + /// The client's MultiTransportChannelData block flags (MS-RDPBCGR + /// 2.2.1.3.8), when it sent one. `None` when the client did not send the + /// block at all, distinct from `Some(empty())` (block present, no flags + /// set): the server's own block MUST be omitted in the former case but + /// not the latter (2.2.1.4). + multitransport_flags: Option, early_capability_flags: gcc::ClientEarlyCapabilityFlags, server_capabilities: Vec, static_channels: StaticChannelSet, @@ -178,8 +177,7 @@ impl Acceptor { keyboard_layout: 0, keyboard_type: gcc::KeyboardType(0), ime_file_name: String::new(), - multitransport_flags: gcc::MultiTransportFlags::empty(), - client_offered_multitransport: false, + multitransport_flags: None, early_capability_flags: gcc::ClientEarlyCapabilityFlags::empty(), server_capabilities: capabilities, static_channels: StaticChannelSet::new(), @@ -260,6 +258,12 @@ impl Acceptor { /// /// `None` is the default: no multitransport block is advertised and no /// request is ever sent. + /// + /// This acceptor only bootstraps and sends the request; it does not + /// itself establish the RDPEUDP2 sideband transport the request + /// promises. Enabling this without a caller that drives that + /// establishment (over `multitransport_request()`) makes every + /// reciprocating client attempt a UDP connection that cannot succeed. pub fn set_multitransport_offer(&mut self, flags: Option) { self.offer_multitransport = flags; } @@ -291,19 +295,24 @@ impl Acceptor { /// Whether both peers advertised Soft-Sync support for multitransport. /// - /// Only meaningful once [`multitransport_request()`](Self::multitransport_request) - /// returns `Some`. - pub fn multitransport_soft_sync_negotiated(&self) -> bool { - self.offer_multitransport - .is_some_and(|offer| offer.contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP)) - && self - .multitransport_flags - .contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP) + /// `None` before [`multitransport_request()`](Self::multitransport_request) + /// returns `Some`: no request was sent, so nothing was actually + /// negotiated regardless of what the GCC flags alone would suggest. + pub fn multitransport_soft_sync_negotiated(&self) -> Option { + self.sent_multitransport_request.as_ref()?; + Some( + self.offer_multitransport + .is_some_and(|offer| offer.contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP)) + && self + .multitransport_flags + .is_some_and(|flags| flags.contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP)), + ) } /// If `data` (an MCS SendDataRequest already decoded from the wire) is on /// the message channel while a multitransport request is outstanding AND /// its payload strictly decodes as an Initiate Multitransport Response, + /// logs it against the outstanding request (matching request IDs) and /// returns it. MS-RDPBCGR 3.2.5.15.1 gives this response no fixed /// position relative to the rest of the handshake: it depends on when /// the client resolves its own bootstrapping and whether the sideband @@ -323,24 +332,12 @@ impl Acceptor { &self, data: &mcs::SendDataRequest<'_>, ) -> Option { - if !(self.sent_multitransport_request.is_some() && Some(data.channel_id) == self.message_channel_id) { + let sent = self.sent_multitransport_request.as_ref()?; + if Some(data.channel_id) != self.message_channel_id { return None; } - decode::(data.user_data.as_ref()).ok() - } - - /// Logs a received Initiate Multitransport Response against the - /// outstanding request, matching request IDs. Shared by the two call - /// sites `late_multitransport_response` gates; both only call this once - /// that method has confirmed a request is outstanding, so - /// `sent_multitransport_request` is always `Some` here. - fn log_multitransport_response(&self, response: &rdp::multitransport::MultitransportResponsePdu) { - let expected_request_id = self - .sent_multitransport_request - .as_ref() - .expect("late_multitransport_response only returns Some when a request is outstanding") - .request_id; - if response.request_id == expected_request_id { + let response = decode::(data.user_data.as_ref()).ok()?; + if response.request_id == sent.request_id { debug!( request_id = response.request_id, success = response.is_success(), @@ -349,9 +346,11 @@ impl Acceptor { } else { warn!( response.request_id, - expected_request_id, "Initiate Multitransport Response request ID does not match the sent request" + expected_request_id = sent.request_id, + "Initiate Multitransport Response request ID does not match the sent request" ); } + Some(response) } pub fn new_deactivation_reactivation( @@ -387,7 +386,6 @@ impl Acceptor { keyboard_type: consumed.keyboard_type, ime_file_name: consumed.ime_file_name, multitransport_flags: consumed.multitransport_flags, - client_offered_multitransport: consumed.client_offered_multitransport, early_capability_flags: consumed.early_capability_flags, server_capabilities: consumed.server_capabilities, static_channels, @@ -489,7 +487,9 @@ impl Acceptor { keyboard_layout: self.keyboard_layout, keyboard_type: self.keyboard_type, ime_file_name: self.ime_file_name.clone(), - multitransport_flags: self.multitransport_flags, + multitransport_flags: self + .multitransport_flags + .unwrap_or_else(gcc::MultiTransportFlags::empty), client_early_capability_flags: self.early_capability_flags, reactivation: self.reactivation, credentials: self.received_credentials.take(), @@ -504,6 +504,7 @@ impl Acceptor { } #[derive(Default, Debug)] +#[non_exhaustive] pub enum AcceptorState { #[default] Consumed, @@ -795,12 +796,7 @@ impl Sequence for Acceptor { self.keyboard_layout = gcc_blocks.core.keyboard_layout; self.keyboard_type = gcc_blocks.core.keyboard_type; self.ime_file_name.clone_from(&gcc_blocks.core.ime_file_name); - self.client_offered_multitransport = gcc_blocks.multi_transport_channel.is_some(); - self.multitransport_flags = gcc_blocks - .multi_transport_channel - .as_ref() - .map(|m| m.flags) - .unwrap_or_else(gcc::MultiTransportFlags::empty); + self.multitransport_flags = gcc_blocks.multi_transport_channel.as_ref().map(|m| m.flags); // Adopt the client's requested desktop size (from its Client // Core Data) before Demand Active is sent, so the session is @@ -904,7 +900,8 @@ impl Sequence for Acceptor { requested_protocol, skip_channel_join, self.message_channel_id, - self.offer_multitransport.filter(|_| self.client_offered_multitransport), + self.offer_multitransport + .filter(|_| self.multitransport_flags.is_some()), ); let settings_response = mcs::ConnectResponse { @@ -1074,7 +1071,7 @@ impl Sequence for Acceptor { .is_some_and(|offer| offer.contains(gcc::MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); let client_supports_udp_fecr = self .multitransport_flags - .contains(gcc::MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR); + .is_some_and(|flags| flags.contains(gcc::MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); // 2.2.15.1 requires the request to travel on the MCS message // channel. A client can in principle advertise UDP support // without also requesting a message channel; rather than @@ -1192,31 +1189,27 @@ impl Sequence for Acceptor { } } }; - // An Initiate Multitransport Response can legitimately land here: - // it travels on the message channel, and the client sends it - // (when it sends one at all) while resolving its own multitransport - // bootstrapping, strictly before it ever reads the Demand Active - // that leads to Confirm Active. So it is checked for by channel - // and a successful strict decode before assuming the payload is a - // Confirm Active, and simply logged and dropped: this acceptor - // does not gate on it, per the note on - // `AcceptorState::MultitransportBootstrapping`. A decode failure - // here means the message-channel traffic isn't a response at all - // (Auto-Detect Response, Heartbeat), so it falls through to the - // Confirm Active handling below instead. - let late_multitransport_response = match &message { - mcs::McsMessage::SendDataRequest(data) => self.late_multitransport_response(data), - _ => None, - }; - - if let Some(response) = late_multitransport_response { - self.log_multitransport_response(&response); - self.state = prev_state; - return Ok(Written::Nothing); - } - match message { mcs::McsMessage::SendDataRequest(data) => { + // An Initiate Multitransport Response can legitimately land + // here: it travels on the message channel, and the client + // sends it (when it sends one at all) while resolving its + // own multitransport bootstrapping, strictly before it + // ever reads the Demand Active that leads to Confirm + // Active. So it is checked for by channel and a + // successful strict decode before assuming the payload is + // a Confirm Active, and simply logged and dropped: this + // acceptor does not gate on it, per the note on + // `AcceptorState::MultitransportBootstrapping`. A decode + // failure here means the message-channel traffic isn't a + // response at all (Auto-Detect Response, Heartbeat), so it + // falls through to the Confirm Active handling below + // instead. + if self.late_multitransport_response(&data).is_some() { + self.state = prev_state; + return Ok(Written::Nothing); + } + let capabilities_confirm = decode::(data.user_data.as_ref()) .map_err(ConnectorError::decode); let capabilities_confirm = match capabilities_confirm { @@ -1275,14 +1268,14 @@ impl Sequence for Acceptor { // application as a raw input event. Check for it here, before // finalization ever sees the bytes, mirroring // `CapabilitiesWaitConfirm`'s handling. - let late_multitransport_response = match decode::>>(input) { - Ok(X224(mcs::McsMessage::SendDataRequest(data))) => self.late_multitransport_response(&data), - _ => None, + let is_late_multitransport_response = match decode::>>(input) { + Ok(X224(mcs::McsMessage::SendDataRequest(data))) => { + self.late_multitransport_response(&data).is_some() + } + _ => false, }; - if let Some(response) = late_multitransport_response { - self.log_multitransport_response(&response); - + if is_late_multitransport_response { ( Written::Nothing, AcceptorState::ConnectionFinalization { diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index c90c4cb01b..f9b0e1a2b1 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -325,6 +325,26 @@ fn client_gcc_with_message_channel_and_multitransport( blocks } +/// Builds an `Acceptor` for the multitransport tests below, at a common +/// 1920x1080 desktop size with no static channels or credentials. `offer` is +/// passed to `set_multitransport_offer` when `Some`; pass `None` to exercise +/// the default-disabled path. +fn multitransport_acceptor(offer: Option) -> Acceptor { + let mut acceptor = Acceptor::new( + SecurityProtocol::SSL, + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + if let Some(offer) = offer { + acceptor.set_multitransport_offer(Some(offer)); + } + acceptor +} + /// The full happy path: the acceptor offers reliable UDP multitransport, the /// client reciprocates, so the request goes out on the message channel and /// `multitransport_request()` surfaces it. A late Initiate Multitransport @@ -335,16 +355,7 @@ fn client_gcc_with_message_channel_and_multitransport( /// still reaching capabilities confirmation. #[test] fn multitransport_offered_and_client_reciprocates() { - let mut acceptor = Acceptor::new( - SecurityProtocol::SSL, - DesktopSize { - width: 1920, - height: 1080, - }, - Vec::new(), - None, - ); - acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let mut acceptor = multitransport_acceptor(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); let client_blocks = client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); @@ -421,16 +432,7 @@ fn multitransport_offered_and_client_reciprocates() { /// own (pre-existing, unrelated to this fix) handling see it. #[test] fn non_response_traffic_on_the_message_channel_is_not_misclassified() { - let mut acceptor = Acceptor::new( - SecurityProtocol::SSL, - DesktopSize { - width: 1920, - height: 1080, - }, - Vec::new(), - None, - ); - acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let mut acceptor = multitransport_acceptor(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); let client_blocks = client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); @@ -473,16 +475,7 @@ fn non_response_traffic_on_the_message_channel_is_not_misclassified() { /// and the connection is dropped outright. #[test] fn multitransport_response_arriving_during_finalization_is_tolerated() { - let mut acceptor = Acceptor::new( - SecurityProtocol::SSL, - DesktopSize { - width: 1920, - height: 1080, - }, - Vec::new(), - None, - ); - acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let mut acceptor = multitransport_acceptor(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); let client_blocks = client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); @@ -568,15 +561,7 @@ fn multitransport_response_arriving_during_finalization_is_tolerated() { /// Multitransport Request is ever sent. #[test] fn multitransport_not_offered_by_default() { - let mut acceptor = Acceptor::new( - SecurityProtocol::SSL, - DesktopSize { - width: 1920, - height: 1080, - }, - Vec::new(), - None, - ); + let mut acceptor = multitransport_acceptor(None); let client_blocks = client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); @@ -586,7 +571,7 @@ fn multitransport_not_offered_by_default() { let written = acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping assert!(matches!(written, Written::Nothing)); assert!(acceptor.multitransport_request().is_none()); - assert!(!acceptor.multitransport_soft_sync_negotiated()); + assert_eq!(acceptor.multitransport_soft_sync_negotiated(), None); } /// The acceptor offers multitransport, but the client's GCC blocks never @@ -595,16 +580,7 @@ fn multitransport_not_offered_by_default() { /// rather than echo the offer the client never reciprocated. #[test] fn multitransport_not_offered_when_client_does_not_reciprocate() { - let mut acceptor = Acceptor::new( - SecurityProtocol::SSL, - DesktopSize { - width: 1920, - height: 1080, - }, - Vec::new(), - None, - ); - acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let mut acceptor = multitransport_acceptor(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); let client_blocks = client_gcc_with_message_channel_and_multitransport(None); let (.., server_multitransport) = drive_to_secure_settings_exchange(&mut acceptor, client_blocks); From 010463474c8132833656ce1fb12047a526218a15 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Wed, 16 Sep 2026 19:11:39 -0500 Subject: [PATCH 4/7] refactor(server): inject the multitransport security cookie RNG instead of calling it inline The security cookie and request ID for the Initiate Multitransport Request were generated with a direct rand::rng() call inside MultitransportBootstrapping's step() arm, a hidden side channel to global RNG state in what is otherwise a sans-I/O, deterministic sequence. Nothing was actually broken by this; it is a testability and architecture concern, not a bug, so this is a refactor rather than a fix. Added a MultitransportSecurityRng trait (fill_security_cookie, next_request_id) stored as a boxed trait object on Acceptor, defaulting to an OS-backed implementation and overridable via set_multitransport_security_rng(), matching the existing set_multitransport_offer()/set_honor_client_desktop_size() builder idiom. step() now reads through the injected source. Carried through new_deactivation_reactivation() alongside the acceptor's other injected state. Added a test injecting a fixed source and asserting the exact bytes reach the encoded wire PDU. --- crates/ironrdp-acceptor/src/connection.rs | 47 +++++++++++++++++-- crates/ironrdp-acceptor/src/lib.rs | 2 +- .../tests/server/acceptor.rs | 46 +++++++++++++++++- 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index 5ebb346567..83d896bbf1 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -59,6 +59,38 @@ pub struct Acceptor { /// The Initiate Multitransport Request sent to the client, once /// `MultitransportBootstrapping` has run. See `multitransport_request()`. sent_multitransport_request: Option, + /// Source of randomness for the Initiate Multitransport Request's security + /// cookie and request ID. See `set_multitransport_security_rng()`. + multitransport_security_rng: Box, +} + +/// Source of randomness for the security cookie and request ID the acceptor +/// sends in an Initiate Multitransport Request PDU (MS-RDPBCGR 2.2.15.1). +/// +/// [`Acceptor::step`] is otherwise a pure function of its inputs and stored +/// state, which is what makes the sans-I/O sequence deterministic and +/// testable by feeding it bytes; reading a global RNG from inside `step` +/// would be a hidden side channel breaking that. Injected instead via +/// [`Acceptor::set_multitransport_security_rng`], defaulting to an OS-backed +/// implementation. +pub trait MultitransportSecurityRng: Send { + /// Fill `cookie` with random bytes for the security cookie field. + fn fill_security_cookie(&mut self, cookie: &mut [u8; 16]); + /// Produce the request ID. + fn next_request_id(&mut self) -> u32; +} + +/// Default [`MultitransportSecurityRng`], backed by the OS RNG via `rand::rng()`. +struct OsMultitransportSecurityRng; + +impl MultitransportSecurityRng for OsMultitransportSecurityRng { + fn fill_security_cookie(&mut self, cookie: &mut [u8; 16]) { + rand::rng().fill_bytes(cookie); + } + + fn next_request_id(&mut self) -> u32 { + rand::rng().next_u32() + } } /// Minimum and maximum desktop dimension honored from a client. @@ -189,9 +221,17 @@ impl Acceptor { honor_client_desktop_size: None, offer_multitransport: None, sent_multitransport_request: None, + multitransport_security_rng: Box::new(OsMultitransportSecurityRng), } } + /// Overrides the source of randomness used for the security cookie and + /// request ID in an Initiate Multitransport Request PDU. Defaults to an + /// OS-backed RNG; intended for tests that need deterministic output. + pub fn set_multitransport_security_rng(&mut self, rng: Box) { + self.multitransport_security_rng = rng; + } + /// Adopt the desktop size requested by the client in its Client Core Data /// instead of the size this acceptor was constructed with, clamped to an /// operator-configured maximum. @@ -397,6 +437,7 @@ impl Acceptor { honor_client_desktop_size: consumed.honor_client_desktop_size, offer_multitransport: consumed.offer_multitransport, sent_multitransport_request: consumed.sent_multitransport_request, + multitransport_security_rng: consumed.multitransport_security_rng, }) } @@ -1084,9 +1125,9 @@ impl Sequence for Acceptor { if let Some(message_channel_id) = message_channel_id { let mut security_cookie = [0u8; 16]; - let mut rng = rand::rng(); - rng.fill_bytes(&mut security_cookie); - let request_id = rng.next_u32(); + self.multitransport_security_rng + .fill_security_cookie(&mut security_cookie); + let request_id = self.multitransport_security_rng.next_request_id(); let request = rdp::multitransport::MultitransportRequestPdu { security_header: rdp::headers::BasicSecurityHeader { diff --git a/crates/ironrdp-acceptor/src/lib.rs b/crates/ironrdp-acceptor/src/lib.rs index a8a709687e..e7f3636cff 100644 --- a/crates/ironrdp-acceptor/src/lib.rs +++ b/crates/ironrdp-acceptor/src/lib.rs @@ -18,7 +18,7 @@ pub use ironrdp_connector::DesktopSize; use ironrdp_pdu::nego; pub use self::channel_connection::{ChannelConnectionSequence, ChannelConnectionState}; -pub use self::connection::{Acceptor, AcceptorResult, AcceptorState}; +pub use self::connection::{Acceptor, AcceptorResult, AcceptorState, MultitransportSecurityRng}; pub use self::finalization::{FinalizationSequence, FinalizationState}; use crate::credssp::resolve_generator; diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index f9b0e1a2b1..a56d86f9b5 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -1,6 +1,6 @@ use std::borrow::Cow; -use ironrdp_acceptor::Acceptor; +use ironrdp_acceptor::{Acceptor, MultitransportSecurityRng}; use ironrdp_connector::{DesktopSize, Sequence as _, Written, encode_x224_packet}; use ironrdp_core::{WriteBuf, decode, encode_vec}; use ironrdp_pdu::gcc::{ClientMessageChannelData, MultiTransportChannelData, MultiTransportFlags}; @@ -424,6 +424,50 @@ fn multitransport_offered_and_client_reciprocates() { assert_eq!(acceptor.state().name(), "ConnectionFinalization"); } +/// A fixed `MultitransportSecurityRng` for deterministic assertions. +struct FixedMultitransportSecurityRng { + cookie: [u8; 16], + request_id: u32, +} + +impl MultitransportSecurityRng for FixedMultitransportSecurityRng { + fn fill_security_cookie(&mut self, cookie: &mut [u8; 16]) { + *cookie = self.cookie; + } + + fn next_request_id(&mut self) -> u32 { + self.request_id + } +} + +/// The security cookie and request ID in the Initiate Multitransport Request +/// come from the injected `MultitransportSecurityRng`, not from a hidden +/// global RNG read inside `step()`: the acceptor is deterministic when its +/// randomness is supplied rather than sourced internally. +#[test] +fn multitransport_request_uses_the_injected_rng() { + let mut acceptor = multitransport_acceptor(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + acceptor.set_multitransport_security_rng(Box::new(FixedMultitransportSecurityRng { + cookie: [0xAB; 16], + request_id: 0x1234_5678, + })); + + let client_blocks = + client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + + // LicensingExchange (sends license) -> MultitransportBootstrapping. + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); + // MultitransportBootstrapping: sends the request using the injected RNG. + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); + + let sent_request = acceptor + .multitransport_request() + .expect("request recorded after MultitransportBootstrapping"); + assert_eq!(sent_request.security_cookie, [0xAB; 16]); + assert_eq!(sent_request.request_id, 0x1234_5678); +} + /// The message channel also carries Auto-Detect Response and Heartbeat PDUs /// (MS-RDPBCGR 2.2.1.4.5, 2.2.8.1.1.2.1), not just the Initiate Multitransport /// Response. A guard keyed only on channel and outstanding-request, without From c9fec61df682bceba1ed593bdaf119591a18e12c Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 10 Sep 2026 14:33:49 -0500 Subject: [PATCH 5/7] feat(server): add accept_finalize_with_multitransport driver Add an async driver mirroring the client side's connect_finalize_with_multitransport: it drives the acceptor sequence to completion the same way accept_finalize already does, and awaits an app-supplied handler once, synchronously, the moment the acceptor sends an Initiate Multitransport Request, so the caller can establish the sideband UDP transport (RDPEUDP2 + TLS + RDPEMT). Unlike the client-side callback, the handler reports nothing back into the sequence: the acceptor has already sent the request and moved on by the time the handler runs, so there is no response to build from an outcome. The handler should return promptly (e.g. by spawning the actual work) rather than driving the transport to completion inline, or the handshake stalls behind it. accept_finalize becomes a thin wrapper around this with a no-op handler, matching the client side's connect_finalize/connect_finalize_with_multitransport relationship. The driver seeds its "already notified" tracking from whatever request is already present rather than starting at false: a Deactivation- Reactivation Sequence rebuilds the acceptor via new_deactivation_reactivation(), which carries the original request forward without running bootstrapping again, then this function is called a second time on the rebuilt acceptor. Without the seed, that second call's first loop iteration would treat the carried-over request as newly sent and notify the handler again. Adds integration tests in ironrdp-testsuite-core driving a real Acceptor over a tokio::io::duplex pair with a hand-rolled client script: the handler fires exactly once with the sent request and does not block the handshake, a late Initiate Multitransport Response is still tolerated ahead of Confirm Active during the async-driven path, and the handler does not fire again across a reactivation round. Building the first of these surfaced a real bug: an initial take_multitransport_request() consumed the same field CapabilitiesWaitConfirm's response tolerance depends on, breaking that check the moment the driver read the request. Removed in favor of the local flag, keeping multitransport_request() a plain borrow. --- Cargo.lock | 2 + crates/ironrdp-acceptor/src/lib.rs | 56 +++ crates/ironrdp-testsuite-core/Cargo.toml | 2 + .../tests/server/mod.rs | 1 + .../tests/server/multitransport_finalize.rs | 358 ++++++++++++++++++ 5 files changed, 419 insertions(+) create mode 100644 crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs diff --git a/Cargo.lock b/Cargo.lock index bb9ca35b28..b9032d9414 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3383,6 +3383,7 @@ dependencies = [ "expect-test", "hex", "ironrdp-acceptor", + "ironrdp-async", "ironrdp-bulk", "ironrdp-cfg", "ironrdp-cliprdr", @@ -3414,6 +3415,7 @@ dependencies = [ "ironrdp-session", "ironrdp-str", "ironrdp-svc", + "ironrdp-tokio", "ironrdp-usb", "openh264", "paste", diff --git a/crates/ironrdp-acceptor/src/lib.rs b/crates/ironrdp-acceptor/src/lib.rs index e7f3636cff..8f186029b8 100644 --- a/crates/ironrdp-acceptor/src/lib.rs +++ b/crates/ironrdp-acceptor/src/lib.rs @@ -82,19 +82,75 @@ where } pub async fn accept_finalize( + framed: Framed, + acceptor: &mut Acceptor, +) -> ConnectorResult<(Framed, AcceptorResult)> +where + S: FramedRead + FramedWrite, +{ + accept_finalize_with_multitransport(framed, acceptor, |_, _| async {}).await +} + +/// Completes the connection sequence, notifying `multitransport_handler` each +/// time the acceptor sends an Initiate Multitransport Request, so the caller +/// can establish the sideband UDP transport (RDPEUDP2 + TLS + RDPEMT). +/// +/// Unlike [`ironrdp_async::connect_finalize_with_multitransport`] on the +/// client side, `multitransport_handler` does not report an outcome back +/// into the sequence: by the time it runs, the acceptor has already sent the +/// request and moved on to capability negotiation (see the doc comment on +/// [`AcceptorState::MultitransportBootstrapping`]), and +/// nothing here waits for the sideband transport to come up. Establishing it +/// is the caller's job, driven independently of this function's return. +/// `multitransport_handler` is awaited once per request, synchronously, +/// immediately after the sequence step that sent it: it should return +/// promptly (for example, by spawning the actual UDP-accept work on the +/// caller's own runtime) rather than driving the transport to completion +/// inline, or the RDP handshake stalls behind it. +/// +/// # Panics +/// +/// Panics if `multitransport_soft_sync_negotiated()` returns `None` right +/// after `multitransport_request()` returned `Some`, which the two methods' +/// own contract does not allow. +pub async fn accept_finalize_with_multitransport( mut framed: Framed, acceptor: &mut Acceptor, + mut multitransport_handler: H, ) -> ConnectorResult<(Framed, AcceptorResult)> where S: FramedRead + FramedWrite, + H: AsyncFnMut(ironrdp_pdu::rdp::multitransport::MultitransportRequestPdu, bool), { let mut buf = WriteBuf::new(); + // `multitransport_request()` borrows rather than consumes (the field it + // reads also gates `CapabilitiesWaitConfirm`'s tolerance for a late + // Initiate Multitransport Response), so this driver tracks locally + // whether it has already notified the caller instead. Seeded from + // whatever is already present rather than `false`: a Deactivation- + // Reactivation Sequence rebuilds the acceptor via + // `Acceptor::new_deactivation_reactivation()`, which carries the + // original request forward, then this function is called again on the + // rebuilt acceptor. Bootstrapping does not run a second time, so without + // this the first loop iteration of that fresh call would treat the + // carried-over request as newly sent and notify the handler again. + let mut notified = acceptor.multitransport_request().is_some(); loop { if let Some(result) = acceptor.get_result() { return Ok((framed, result)); } + single_sequence_step(&mut framed, acceptor, &mut buf).await?; + + if !notified && let Some(request) = acceptor.multitransport_request() { + let request = request.clone(); + let soft_sync = acceptor + .multitransport_soft_sync_negotiated() + .expect("multitransport_request() just returned Some, so a request was sent"); + multitransport_handler(request, soft_sync).await; + notified = true; + } } } diff --git a/crates/ironrdp-testsuite-core/Cargo.toml b/crates/ironrdp-testsuite-core/Cargo.toml index 2c5541b7fa..2654492a8b 100644 --- a/crates/ironrdp-testsuite-core/Cargo.toml +++ b/crates/ironrdp-testsuite-core/Cargo.toml @@ -40,6 +40,8 @@ hex = "0.4" ironrdp-cliprdr-format.path = "../ironrdp-cliprdr-format" ironrdp-cliprdr = { path = "../ironrdp-cliprdr", features = ["__test"] } ironrdp-acceptor.path = "../ironrdp-acceptor" +ironrdp-async.path = "../ironrdp-async" +ironrdp-tokio.path = "../ironrdp-tokio" ironrdp-bulk.path = "../ironrdp-bulk" ironrdp-connector.path = "../ironrdp-connector" ironrdp-displaycontrol.path = "../ironrdp-displaycontrol" diff --git a/crates/ironrdp-testsuite-core/tests/server/mod.rs b/crates/ironrdp-testsuite-core/tests/server/mod.rs index ad0a24ed27..9fddc3d7de 100644 --- a/crates/ironrdp-testsuite-core/tests/server/mod.rs +++ b/crates/ironrdp-testsuite-core/tests/server/mod.rs @@ -3,6 +3,7 @@ mod autodetect; mod credential_validator; mod fast_path; mod finalize_timeout; +mod multitransport_finalize; mod rdpdr; mod rdpei; mod remotefx_entropy_coder; diff --git a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs new file mode 100644 index 0000000000..cfed9a770c --- /dev/null +++ b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs @@ -0,0 +1,358 @@ +//! Integration coverage for `accept_finalize_with_multitransport`. +//! +//! Drives a real `Acceptor` over an in-memory duplex stream, playing the role +//! of the client by hand (raw PDU bytes, not a `ClientConnector`) so the test +//! exercises the acceptor's actual async driver rather than only the +//! synchronous `Acceptor::step()` surface already covered in `acceptor.rs`. + +use std::borrow::Cow; +use std::sync::{Arc, Mutex}; + +use ironrdp_acceptor::{Acceptor, accept_finalize_with_multitransport}; +use ironrdp_async::FramedWrite as _; +use ironrdp_connector::DesktopSize; +use ironrdp_core::{WriteBuf, decode, encode_buf, encode_vec}; +use ironrdp_pdu::gcc::{ClientMessageChannelData, MultiTransportChannelData, MultiTransportFlags}; +use ironrdp_pdu::mcs::{self, ConnectInitial}; +use ironrdp_pdu::nego::{self, SecurityProtocol}; +use ironrdp_pdu::rdp::multitransport::{MultitransportRequestPdu, MultitransportResponsePdu, RequestedProtocol}; +use ironrdp_pdu::x224::{X224, X224Data}; +use ironrdp_testsuite_core::gcc::CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS; +use ironrdp_testsuite_core::rdp::{ + CLIENT_DEMAND_ACTIVE_PDU_BUFFER, CLIENT_FONT_LIST_BUFFER, CLIENT_INFO_PDU_BUFFER, CLIENT_SYNCHRONIZE_BUFFER, + CONTROL_COOPERATE_BUFFER, CONTROL_REQUEST_CONTROL_BUFFER, +}; +use ironrdp_tokio::TokioFramed; +use tokio::io::DuplexStream; + +fn encode_x224_pdu<'a, T: ironrdp_pdu::x224::X224Pdu<'a>>(pdu: T) -> Vec { + let mut buf = WriteBuf::new(); + encode_buf(&X224(pdu), &mut buf).unwrap(); + buf.filled().to_vec() +} + +fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { + encode_x224_pdu(mcs::SendDataRequest { + initiator_id, + channel_id, + user_data: Cow::Borrowed(user_data), + }) +} + +/// Reads one PDU from `framed` and returns its raw bytes (TPKT/X224 header +/// stripped, matching what `decode` in the rest of this file expects). +async fn read_pdu(framed: &mut TokioFramed) -> Vec { + let (_, bytes) = framed.read_pdu().await.expect("read one PDU"); + bytes.to_vec() +} + +/// Drives the client side of a full connection sequence up to (and through) +/// finalization, over `framed`, against an acceptor configured to offer +/// reliable UDP multitransport. Sends a (soft-sync-less) Initiate +/// Multitransport Response before the Confirm Active, which the acceptor +/// must tolerate rather than error on. Returns the same stream, still open, +/// plus the user and I/O channel IDs, for a caller that wants to keep +/// driving it (e.g. through reactivation). +async fn play_client(mut framed: TokioFramed) -> (TokioFramed, u16, u16) { + // Connection Request / Confirm. Plain RDP security (empty protocol) so no + // TLS upgrade is needed and the acceptor drives straight through. + let request = nego::ConnectionRequest { + nego_data: None, + flags: nego::RequestFlags::empty(), + protocol: SecurityProtocol::empty(), + correlation_info: None, + }; + framed.write_all(&encode_x224_pdu(request)).await.unwrap(); + let _confirm = read_pdu(&mut framed).await; + + // MCS Connect Initial, advertising both a message channel and reliable + // UDP multitransport support, so the acceptor has something to offer. + let mut blocks = CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS.clone(); + blocks.network = None; + blocks.message_channel = Some(ClientMessageChannelData); + blocks.multi_transport_channel = Some(MultiTransportChannelData { + flags: MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR, + }); + let connect_initial = ConnectInitial::with_gcc_blocks(blocks).unwrap(); + let mut initial_buf = WriteBuf::new(); + ironrdp_connector::encode_x224_packet(&connect_initial, &mut initial_buf).unwrap(); + framed.write_all(initial_buf.filled()).await.unwrap(); + + let response_bytes = read_pdu(&mut framed).await; + let payload = decode::>>(&response_bytes).unwrap().0; + let response = decode::(payload.data.as_ref()).unwrap(); + let server_blocks = response.conference_create_response.gcc_blocks(); + let io_channel_id = server_blocks.network.io_channel; + let message_channel_id = server_blocks + .message_channel + .as_ref() + .expect("acceptor must negotiate a message channel") + .mcs_message_channel_id; + + // Channel connection: Erect Domain, Attach User, then join every channel + // the server expects (user, I/O, message). + framed + .write_all(&encode_x224_pdu(mcs::ErectDomainPdu { + sub_height: 0, + sub_interval: 0, + })) + .await + .unwrap(); + framed + .write_all(&encode_x224_pdu(mcs::AttachUserRequest)) + .await + .unwrap(); + + let confirm_bytes = read_pdu(&mut framed).await; + let attach_user_confirm = decode::>(&confirm_bytes).unwrap().0; + let user_channel_id = attach_user_confirm.initiator_id; + + for channel_id in [user_channel_id, io_channel_id, message_channel_id] { + framed + .write_all(&encode_x224_pdu(mcs::ChannelJoinRequest { + initiator_id: user_channel_id, + channel_id, + })) + .await + .unwrap(); + let _join_confirm = read_pdu(&mut framed).await; + } + + // Client Info PDU on the I/O channel, then the license PDU comes back. + framed + .write_all(&encode_send_data_request( + user_channel_id, + io_channel_id, + &CLIENT_INFO_PDU_BUFFER, + )) + .await + .unwrap(); + let _license = read_pdu(&mut framed).await; + + // The Initiate Multitransport Request, on the message channel. + let request_bytes = read_pdu(&mut framed).await; + let indication = mcs::decode_send_data_indication(&request_bytes).unwrap(); + assert_eq!( + indication.channel_id, message_channel_id, + "request must go out on the message channel" + ); + let request = indication.decode_user_data::().unwrap(); + assert_eq!(request.requested_protocol, RequestedProtocol::UdpFecR); + + // Demand Active, on the I/O channel. + let _demand_active = read_pdu(&mut framed).await; + + // A response arriving ahead of Confirm Active: the acceptor must + // silently absorb it and keep waiting rather than erroring. + let response = MultitransportResponsePdu::success(request.request_id); + framed + .write_all(&encode_send_data_request( + user_channel_id, + message_channel_id, + &encode_vec(&response).unwrap(), + )) + .await + .unwrap(); + + // Confirm Active, after the multitransport response: on the wire, the + // response (if any) is sent while the client resolves its own + // MultitransportPending state, strictly before it ever reads Demand + // Active, so this ordering matches a real client. CapabilitiesWaitConfirm + // must see this before the acceptor moves on to finalization. + framed + .write_all(&encode_send_data_request( + user_channel_id, + io_channel_id, + &CLIENT_DEMAND_ACTIVE_PDU_BUFFER, + )) + .await + .unwrap(); + + // Finalization: Synchronize, Control Cooperate, Control Request Control, + // Font List, each accepted without a response until Font List lands. + for pdu_bytes in [ + &CLIENT_SYNCHRONIZE_BUFFER[..], + &CONTROL_COOPERATE_BUFFER[..], + &CONTROL_REQUEST_CONTROL_BUFFER[..], + &CLIENT_FONT_LIST_BUFFER[..], + ] { + framed + .write_all(&encode_send_data_request(user_channel_id, io_channel_id, pdu_bytes)) + .await + .unwrap(); + } + + // Four finalization responses, all on the I/O channel: Synchronize + // Confirm, Control Cooperate Confirm, Control Granted Confirm, Font Map. + for _ in 0..4 { + let _ = read_pdu(&mut framed).await; + } + + (framed, user_channel_id, io_channel_id) +} + +/// Plays a Deactivation-Reactivation round: reads the fresh Demand Active +/// (`new_deactivation_reactivation` rebuilds the acceptor straight into +/// `CapabilitiesSendServer`, skipping channel join, licensing, and +/// multitransport bootstrapping entirely, per its own doc comment), then +/// redoes Confirm Active and the finalization exchange. No multitransport +/// traffic is expected this round. +async fn play_reactivation_round(mut framed: TokioFramed, user_channel_id: u16, io_channel_id: u16) { + let _demand_active = read_pdu(&mut framed).await; + + framed + .write_all(&encode_send_data_request( + user_channel_id, + io_channel_id, + &CLIENT_DEMAND_ACTIVE_PDU_BUFFER, + )) + .await + .unwrap(); + + for pdu_bytes in [ + &CLIENT_SYNCHRONIZE_BUFFER[..], + &CONTROL_COOPERATE_BUFFER[..], + &CONTROL_REQUEST_CONTROL_BUFFER[..], + &CLIENT_FONT_LIST_BUFFER[..], + ] { + framed + .write_all(&encode_send_data_request(user_channel_id, io_channel_id, pdu_bytes)) + .await + .unwrap(); + } + + for _ in 0..4 { + let _ = read_pdu(&mut framed).await; + } +} + +/// The Initiate Multitransport Request must reach the caller through +/// `accept_finalize_with_multitransport`'s handler exactly once, with the +/// request the acceptor actually sent, and without the handler blocking the +/// rest of the handshake (which the client-side script above completes +/// concurrently). +#[tokio::test] +async fn multitransport_handler_fires_once_without_blocking_finalization() { + let (client_stream, server_stream) = tokio::io::duplex(65536); + + let handler_calls: Arc>> = Arc::new(Mutex::new(Vec::new())); + let handler_calls_for_server = Arc::clone(&handler_calls); + + // `accept_finalize_with_multitransport`'s generic handler closure is + // `!Send` for the same structural reason `RdpServer`'s own future is + // (see `finalize_timeout.rs`): a `LocalSet` is required rather than a + // plain `tokio::spawn`. + let local = tokio::task::LocalSet::new(); + local + .run_until(async move { + let server = tokio::task::spawn_local(async move { + let mut acceptor = Acceptor::new( + SecurityProtocol::empty(), + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let framed = TokioFramed::new(server_stream); + let (_, result) = accept_finalize_with_multitransport( + framed, + &mut acceptor, + |request: MultitransportRequestPdu, soft_sync: bool| { + let handler_calls = Arc::clone(&handler_calls_for_server); + async move { + handler_calls.lock().unwrap().push((request.request_id, soft_sync)); + } + }, + ) + .await + .expect("acceptor finalize with multitransport"); + + result + }); + + let client_framed = TokioFramed::new(client_stream); + let (_client_framed, _user_channel_id, _io_channel_id) = play_client(client_framed).await; + + let result = server.await.expect("server task panicked"); + assert_eq!(result.static_channels.len(), 0); + + let calls = handler_calls.lock().unwrap(); + assert_eq!(calls.len(), 1, "handler must fire exactly once"); + assert!( + !calls[0].1, + "soft-sync was not offered, so it must not be reported negotiated" + ); + }) + .await; +} + +/// A Deactivation-Reactivation Sequence rebuilds the acceptor via +/// `Acceptor::new_deactivation_reactivation()`, which carries the original +/// Initiate Multitransport Request forward without running bootstrapping +/// again. The handler must not fire a second time when the rebuilt acceptor +/// is driven through `accept_finalize_with_multitransport` for that +/// reactivation round. +#[tokio::test] +async fn multitransport_handler_does_not_fire_again_after_reactivation() { + let (client_stream, server_stream) = tokio::io::duplex(65536); + + let handler_calls: Arc>> = Arc::new(Mutex::new(Vec::new())); + let handler_calls_for_server = Arc::clone(&handler_calls); + + let local = tokio::task::LocalSet::new(); + local + .run_until(async move { + let server = tokio::task::spawn_local(async move { + let desktop_size = DesktopSize { + width: 1920, + height: 1080, + }; + let mut acceptor = Acceptor::new(SecurityProtocol::empty(), desktop_size, Vec::new(), None); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let handler = { + let handler_calls = Arc::clone(&handler_calls_for_server); + move |request: MultitransportRequestPdu, soft_sync: bool| { + let handler_calls = Arc::clone(&handler_calls); + async move { + handler_calls.lock().unwrap().push((request.request_id, soft_sync)); + } + } + }; + + let framed = TokioFramed::new(server_stream); + let (framed, result) = accept_finalize_with_multitransport(framed, &mut acceptor, handler.clone()) + .await + .expect("acceptor finalize with multitransport (first round)"); + + let mut acceptor = + Acceptor::new_deactivation_reactivation(acceptor, result.static_channels, desktop_size) + .expect("rebuild acceptor for reactivation"); + + let (_, result) = accept_finalize_with_multitransport(framed, &mut acceptor, handler) + .await + .expect("acceptor finalize with multitransport (reactivation round)"); + + result + }); + + let client_framed = TokioFramed::new(client_stream); + let (client_framed, user_channel_id, io_channel_id) = play_client(client_framed).await; + play_reactivation_round(client_framed, user_channel_id, io_channel_id).await; + + let result = server.await.expect("server task panicked"); + assert_eq!(result.static_channels.len(), 0); + + let calls = handler_calls.lock().unwrap(); + assert_eq!( + calls.len(), + 1, + "handler must not fire again for a reactivation that carries the same request forward" + ); + }) + .await; +} From 0a53022d5ad47bfdb3b4000743a5d966092a8a94 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 18:27:22 -0500 Subject: [PATCH 6/7] review: address automated review findings on the finalize driver tests Four of the eight findings were already resolved by the rebase onto #1951's own review-response commit: the late-response tolerance now applies uniformly to every FinalizationSequence sub-state, the server MultiTransportChannelData block is filtered on the client's own block presence, and two stale doc comments were already corrected. For the remaining four, deduplicated the MCS SendDataRequest encoder and the client GCC-block builder between acceptor.rs and multitransport_finalize.rs (both made pub(super) and reused), extracted a shared play_confirm_active_and_finalization helper covering the Confirm Active plus four-PDU finalization exchange that play_client and play_reactivation_round both repeated, and extracted a recording_handler factory removing duplicated Arc::clone-into-closure plumbing across the two handler tests. play_client now also returns the request_id it decoded so the first handler test can assert the handler received the exact request the acceptor sent, rather than recording it unread. Added the missing assertion to multitransport_not_offered_by_default, whose doc comment claimed the server's GCC advertisement was checked when nothing actually was. --- .../tests/server/acceptor.rs | 10 +- .../tests/server/multitransport_finalize.rs | 111 ++++++++---------- 2 files changed, 54 insertions(+), 67 deletions(-) diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index a56d86f9b5..985f27fe7a 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -199,7 +199,7 @@ fn neg_failure_hybrid_required() { } } -fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { +pub(super) fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { let mut buf = WriteBuf::new(); ironrdp_core::encode_buf( &X224(mcs::SendDataRequest { @@ -315,7 +315,7 @@ fn drive_to_secure_settings_exchange( ) } -fn client_gcc_with_message_channel_and_multitransport( +pub(super) fn client_gcc_with_message_channel_and_multitransport( offer: Option, ) -> ironrdp_pdu::gcc::ClientGccBlocks { let mut blocks = CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS.clone(); @@ -609,7 +609,11 @@ fn multitransport_not_offered_by_default() { let client_blocks = client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); - drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + let (.., server_multitransport) = drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + assert_eq!( + server_multitransport, None, + "server must not advertise MultiTransportChannelData when multitransport is disabled" + ); acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange let written = acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping diff --git a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs index cfed9a770c..89fee54831 100644 --- a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs +++ b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs @@ -5,19 +5,17 @@ //! exercises the acceptor's actual async driver rather than only the //! synchronous `Acceptor::step()` surface already covered in `acceptor.rs`. -use std::borrow::Cow; use std::sync::{Arc, Mutex}; use ironrdp_acceptor::{Acceptor, accept_finalize_with_multitransport}; use ironrdp_async::FramedWrite as _; use ironrdp_connector::DesktopSize; use ironrdp_core::{WriteBuf, decode, encode_buf, encode_vec}; -use ironrdp_pdu::gcc::{ClientMessageChannelData, MultiTransportChannelData, MultiTransportFlags}; +use ironrdp_pdu::gcc::MultiTransportFlags; use ironrdp_pdu::mcs::{self, ConnectInitial}; use ironrdp_pdu::nego::{self, SecurityProtocol}; use ironrdp_pdu::rdp::multitransport::{MultitransportRequestPdu, MultitransportResponsePdu, RequestedProtocol}; use ironrdp_pdu::x224::{X224, X224Data}; -use ironrdp_testsuite_core::gcc::CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS; use ironrdp_testsuite_core::rdp::{ CLIENT_DEMAND_ACTIVE_PDU_BUFFER, CLIENT_FONT_LIST_BUFFER, CLIENT_INFO_PDU_BUFFER, CLIENT_SYNCHRONIZE_BUFFER, CONTROL_COOPERATE_BUFFER, CONTROL_REQUEST_CONTROL_BUFFER, @@ -25,20 +23,14 @@ use ironrdp_testsuite_core::rdp::{ use ironrdp_tokio::TokioFramed; use tokio::io::DuplexStream; +use super::acceptor::{client_gcc_with_message_channel_and_multitransport, encode_send_data_request}; + fn encode_x224_pdu<'a, T: ironrdp_pdu::x224::X224Pdu<'a>>(pdu: T) -> Vec { let mut buf = WriteBuf::new(); encode_buf(&X224(pdu), &mut buf).unwrap(); buf.filled().to_vec() } -fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { - encode_x224_pdu(mcs::SendDataRequest { - initiator_id, - channel_id, - user_data: Cow::Borrowed(user_data), - }) -} - /// Reads one PDU from `framed` and returns its raw bytes (TPKT/X224 header /// stripped, matching what `decode` in the rest of this file expects). async fn read_pdu(framed: &mut TokioFramed) -> Vec { @@ -51,9 +43,11 @@ async fn read_pdu(framed: &mut TokioFramed) -> Vec { /// reliable UDP multitransport. Sends a (soft-sync-less) Initiate /// Multitransport Response before the Confirm Active, which the acceptor /// must tolerate rather than error on. Returns the same stream, still open, -/// plus the user and I/O channel IDs, for a caller that wants to keep -/// driving it (e.g. through reactivation). -async fn play_client(mut framed: TokioFramed) -> (TokioFramed, u16, u16) { +/// the user and I/O channel IDs, for a caller that wants to keep driving it +/// (e.g. through reactivation), and the request_id the acceptor's Initiate +/// Multitransport Request carried, for a caller that wants to cross-check +/// it against what a handler observed. +async fn play_client(mut framed: TokioFramed) -> (TokioFramed, u16, u16, u32) { // Connection Request / Confirm. Plain RDP security (empty protocol) so no // TLS upgrade is needed and the acceptor drives straight through. let request = nego::ConnectionRequest { @@ -67,12 +61,7 @@ async fn play_client(mut framed: TokioFramed) -> (TokioFramed) -> (TokioFramed, + user_channel_id: u16, + io_channel_id: u16, +) { framed .write_all(&encode_send_data_request( user_channel_id, @@ -185,10 +189,8 @@ async fn play_client(mut framed: TokioFramed) -> (TokioFramed) -> (TokioFramed, user_channel_id: u16, io_channel_id: u16) { let _demand_active = read_pdu(&mut framed).await; - framed - .write_all(&encode_send_data_request( - user_channel_id, - io_channel_id, - &CLIENT_DEMAND_ACTIVE_PDU_BUFFER, - )) - .await - .unwrap(); - - for pdu_bytes in [ - &CLIENT_SYNCHRONIZE_BUFFER[..], - &CONTROL_COOPERATE_BUFFER[..], - &CONTROL_REQUEST_CONTROL_BUFFER[..], - &CLIENT_FONT_LIST_BUFFER[..], - ] { - framed - .write_all(&encode_send_data_request(user_channel_id, io_channel_id, pdu_bytes)) - .await - .unwrap(); - } - - for _ in 0..4 { - let _ = read_pdu(&mut framed).await; - } + play_confirm_active_and_finalization(&mut framed, user_channel_id, io_channel_id).await; } /// The Initiate Multitransport Request must reach the caller through @@ -261,12 +240,7 @@ async fn multitransport_handler_fires_once_without_blocking_finalization() { let (_, result) = accept_finalize_with_multitransport( framed, &mut acceptor, - |request: MultitransportRequestPdu, soft_sync: bool| { - let handler_calls = Arc::clone(&handler_calls_for_server); - async move { - handler_calls.lock().unwrap().push((request.request_id, soft_sync)); - } - }, + recording_handler(handler_calls_for_server), ) .await .expect("acceptor finalize with multitransport"); @@ -275,13 +249,17 @@ async fn multitransport_handler_fires_once_without_blocking_finalization() { }); let client_framed = TokioFramed::new(client_stream); - let (_client_framed, _user_channel_id, _io_channel_id) = play_client(client_framed).await; + let (_client_framed, _user_channel_id, _io_channel_id, request_id) = play_client(client_framed).await; let result = server.await.expect("server task panicked"); assert_eq!(result.static_channels.len(), 0); let calls = handler_calls.lock().unwrap(); assert_eq!(calls.len(), 1, "handler must fire exactly once"); + assert_eq!( + calls[0].0, request_id, + "handler must receive the exact request the acceptor sent" + ); assert!( !calls[0].1, "soft-sync was not offered, so it must not be reported negotiated" @@ -290,6 +268,19 @@ async fn multitransport_handler_fires_once_without_blocking_finalization() { .await; } +/// Builds a handler for `accept_finalize_with_multitransport` that records +/// every `(request_id, soft_sync)` call into `calls`. Shared by both tests +/// below, which otherwise duplicated the identical `Arc::clone`-into-closure +/// plumbing around an inline handler. +fn recording_handler(calls: Arc>>) -> impl AsyncFnMut(MultitransportRequestPdu, bool) + Clone { + move |request: MultitransportRequestPdu, soft_sync: bool| { + let calls = Arc::clone(&calls); + async move { + calls.lock().unwrap().push((request.request_id, soft_sync)); + } + } +} + /// A Deactivation-Reactivation Sequence rebuilds the acceptor via /// `Acceptor::new_deactivation_reactivation()`, which carries the original /// Initiate Multitransport Request forward without running bootstrapping @@ -314,15 +305,7 @@ async fn multitransport_handler_does_not_fire_again_after_reactivation() { let mut acceptor = Acceptor::new(SecurityProtocol::empty(), desktop_size, Vec::new(), None); acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); - let handler = { - let handler_calls = Arc::clone(&handler_calls_for_server); - move |request: MultitransportRequestPdu, soft_sync: bool| { - let handler_calls = Arc::clone(&handler_calls); - async move { - handler_calls.lock().unwrap().push((request.request_id, soft_sync)); - } - } - }; + let handler = recording_handler(handler_calls_for_server); let framed = TokioFramed::new(server_stream); let (framed, result) = accept_finalize_with_multitransport(framed, &mut acceptor, handler.clone()) @@ -341,7 +324,7 @@ async fn multitransport_handler_does_not_fire_again_after_reactivation() { }); let client_framed = TokioFramed::new(client_stream); - let (client_framed, user_channel_id, io_channel_id) = play_client(client_framed).await; + let (client_framed, user_channel_id, io_channel_id, _request_id) = play_client(client_framed).await; play_reactivation_round(client_framed, user_channel_id, io_channel_id).await; let result = server.await.expect("server task panicked"); From d6e75ec7c8a7c6bc8c75ffa31266e664e8ed0cff Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 23:35:45 -0500 Subject: [PATCH 7/7] review: address automated review findings on the finalize driver - Document that USER_CHANNEL_ID doubles as the fixed MCS server channel ID (MS-RDPBCGR 3.3.1.5) that every server-to-client Send Data Indication in this file relies on. - Fix the finalize integration test to send an abort response instead of S_OK when SOFTSYNC_TCP_TO_UDP was not negotiated (MS-RDPBCGR 2.2.15.2), matching the existing pattern in acceptor.rs. - Rename late_multitransport_response to is_late_multitransport_response and return bool instead of an Option neither caller reads. - Simplify multitransport_acceptor to pass its Option straight through to set_multitransport_offer instead of re-wrapping it. --- crates/ironrdp-acceptor/src/connection.rs | 29 ++++++++++--------- .../tests/server/acceptor.rs | 8 ++--- .../tests/server/multitransport_finalize.rs | 5 +++- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index 83d896bbf1..848f57209e 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -24,6 +24,10 @@ use super::finalization::FinalizationSequence; use crate::util::{self, wrap_share_data}; const IO_CHANNEL_ID: u16 = 1003; +// Also the fixed MCS server channel ID (0x03EA) that MS-RDPBCGR 3.3.1.5 defines, +// which is why it doubles as the `initiator` on every server-to-client Send Data +// Indication in this file (License, Demand Active, Initiate Multitransport Request +// per 3.3.5.15.1) rather than the user's own MCS channel ID. const USER_CHANNEL_ID: u16 = 1002; pub struct Acceptor { @@ -368,15 +372,16 @@ impl Acceptor { /// handling for anything that isn't really a response, mirroring how /// `ClientConnectorState::ConnectTimeAutoDetection` demuxes the same /// channel client-side. - fn late_multitransport_response( - &self, - data: &mcs::SendDataRequest<'_>, - ) -> Option { - let sent = self.sent_multitransport_request.as_ref()?; + fn is_late_multitransport_response(&self, data: &mcs::SendDataRequest<'_>) -> bool { + let Some(sent) = self.sent_multitransport_request.as_ref() else { + return false; + }; if Some(data.channel_id) != self.message_channel_id { - return None; + return false; } - let response = decode::(data.user_data.as_ref()).ok()?; + let Ok(response) = decode::(data.user_data.as_ref()) else { + return false; + }; if response.request_id == sent.request_id { debug!( request_id = response.request_id, @@ -390,7 +395,7 @@ impl Acceptor { "Initiate Multitransport Response request ID does not match the sent request" ); } - Some(response) + true } pub fn new_deactivation_reactivation( @@ -1246,7 +1251,7 @@ impl Sequence for Acceptor { // response at all (Auto-Detect Response, Heartbeat), so it // falls through to the Confirm Active handling below // instead. - if self.late_multitransport_response(&data).is_some() { + if self.is_late_multitransport_response(&data) { self.state = prev_state; return Ok(Written::Nothing); } @@ -1301,7 +1306,7 @@ impl Sequence for Acceptor { client_capabilities, } => { // A late Initiate Multitransport Response can land in any - // finalization sub-state (see `late_multitransport_response`); + // finalization sub-state (see `is_late_multitransport_response`); // none of FinalizationSequence's own PDU decoders expect it, and // depending which sub-state is active it would otherwise be // silently swallowed while advancing a state, propagated as a @@ -1310,9 +1315,7 @@ impl Sequence for Acceptor { // finalization ever sees the bytes, mirroring // `CapabilitiesWaitConfirm`'s handling. let is_late_multitransport_response = match decode::>>(input) { - Ok(X224(mcs::McsMessage::SendDataRequest(data))) => { - self.late_multitransport_response(&data).is_some() - } + Ok(X224(mcs::McsMessage::SendDataRequest(data))) => self.is_late_multitransport_response(&data), _ => false, }; diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index 985f27fe7a..cf09144147 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -327,8 +327,8 @@ pub(super) fn client_gcc_with_message_channel_and_multitransport( /// Builds an `Acceptor` for the multitransport tests below, at a common /// 1920x1080 desktop size with no static channels or credentials. `offer` is -/// passed to `set_multitransport_offer` when `Some`; pass `None` to exercise -/// the default-disabled path. +/// passed straight through to `set_multitransport_offer`; pass `None` to +/// exercise the default-disabled path. fn multitransport_acceptor(offer: Option) -> Acceptor { let mut acceptor = Acceptor::new( SecurityProtocol::SSL, @@ -339,9 +339,7 @@ fn multitransport_acceptor(offer: Option) -> Acceptor { Vec::new(), None, ); - if let Some(offer) = offer { - acceptor.set_multitransport_offer(Some(offer)); - } + acceptor.set_multitransport_offer(offer); acceptor } diff --git a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs index 89fee54831..71ad058a7e 100644 --- a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs +++ b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs @@ -133,7 +133,10 @@ async fn play_client(mut framed: TokioFramed) -> (TokioFramed