From 10025530ac83480fd03c711a0e7691ba1583724d Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 10 Sep 2026 14:04:06 -0500 Subject: [PATCH 1/5] 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 9cccb13816..538aed934a 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 3fd6d2cbbc715d317af412a23015129ab3e4fa63 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 14:36:50 -0500 Subject: [PATCH 2/5] 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 633c7c0fa4febc241519a5519315f1ba0f8c4207 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 17:59:32 -0500 Subject: [PATCH 3/5] 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 efc36ba04d13e861a612fbf03c52b286a62135ea Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Wed, 16 Sep 2026 19:11:39 -0500 Subject: [PATCH 4/5] 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 e27b3c29fd696ff62a0d986f76768c46b37e26d6 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Tue, 22 Sep 2026 17:02:30 -0500 Subject: [PATCH 5/5] review: require a strict late-response decode and snapshot the advertised offer late_multitransport_response now rejects a payload with bytes left over after the Initiate Multitransport Response (12 bytes with the Basic Security Header this acceptor's ENCRYPTION_LEVEL_NONE implies) instead of swallowing it. The request decision and multitransport_soft_sync_negotiated() read the flags actually advertised in the GCC response, not the live offer, so a late set_multitransport_offer call cannot diverge from what the client was told. --- crates/ironrdp-acceptor/src/connection.rs | 48 ++++++++--- .../tests/server/acceptor.rs | 80 +++++++++++++++++++ 2 files changed, 118 insertions(+), 10 deletions(-) diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index 83d896bbf1..d3f090ca25 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -5,7 +5,7 @@ use ironrdp_connector::{ ConnectorError, ConnectorErrorExt as _, ConnectorResult, DesktopSize, MonotonicInstant, Sequence, State, Written, encode_x224_packet, general_err, reason_err, }; -use ironrdp_core::{WriteBuf, decode}; +use ironrdp_core::{ReadCursor, WriteBuf, decode, decode_cursor}; use ironrdp_pdu as pdu; use ironrdp_pdu::nego::SecurityProtocol; use ironrdp_pdu::x224::X224; @@ -56,6 +56,13 @@ pub struct Acceptor { /// Server MultiTransportChannelData block is sent, and no Initiate /// Multitransport Request follows. See `set_multitransport_offer()`. offer_multitransport: Option, + /// The flags actually written into the Server MultiTransportChannelData + /// block during Basic Settings Exchange, or `None` if no block was sent. + /// Everything after that exchange decides from this snapshot rather than + /// from `offer_multitransport`, so a later `set_multitransport_offer()` + /// call cannot send a request, or report a Soft-Sync result, that the + /// client was never told about. + advertised_multitransport: Option, /// The Initiate Multitransport Request sent to the client, once /// `MultitransportBootstrapping` has run. See `multitransport_request()`. sent_multitransport_request: Option, @@ -220,6 +227,7 @@ impl Acceptor { reactivation: false, honor_client_desktop_size: None, offer_multitransport: None, + advertised_multitransport: None, sent_multitransport_request: None, multitransport_security_rng: Box::new(OsMultitransportSecurityRng), } @@ -299,6 +307,11 @@ impl Acceptor { /// `None` is the default: no multitransport block is advertised and no /// request is ever sent. /// + /// Only a call made before Basic Settings Exchange takes effect. The + /// flags advertised there are what the later request and + /// `multitransport_soft_sync_negotiated()` are based on; changing the + /// offer afterwards does not alter a negotiation already under way. + /// /// 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 @@ -341,7 +354,7 @@ impl Acceptor { pub fn multitransport_soft_sync_negotiated(&self) -> Option { self.sent_multitransport_request.as_ref()?; Some( - self.offer_multitransport + self.advertised_multitransport .is_some_and(|offer| offer.contains(gcc::MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP)) && self .multitransport_flags @@ -376,7 +389,16 @@ impl Acceptor { if Some(data.channel_id) != self.message_channel_id { return None; } - let response = decode::(data.user_data.as_ref()).ok()?; + // `decode` alone would accept a valid response followed by trailing + // bytes. This acceptor advertises ENCRYPTION_LEVEL_NONE, so the + // response carries the 4-byte Basic Security Header (MS-RDPBCGR + // 2.2.15.2) and is exactly 12 bytes: anything left over means the + // payload is something else. + let mut cursor = ReadCursor::new(data.user_data.as_ref()); + let response = decode_cursor::(&mut cursor).ok()?; + if !cursor.is_empty() { + return None; + } if response.request_id == sent.request_id { debug!( request_id = response.request_id, @@ -436,6 +458,7 @@ impl Acceptor { reactivation: true, honor_client_desktop_size: consumed.honor_client_desktop_size, offer_multitransport: consumed.offer_multitransport, + advertised_multitransport: consumed.advertised_multitransport, sent_multitransport_request: consumed.sent_multitransport_request, multitransport_security_rng: consumed.multitransport_security_rng, }) @@ -600,9 +623,10 @@ pub enum AcceptorState { /// 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. + /// `multitransport_flags` and the offer the server itself advertised + /// during Basic Settings Exchange, 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 @@ -935,14 +959,18 @@ impl Sequence for Acceptor { let skip_channel_join = early_capability .is_some_and(|client| client.contains(gcc::ClientEarlyCapabilityFlags::SUPPORT_SKIP_CHANNELJOIN)); + self.advertised_multitransport = self + .message_channel_id + .and(self.offer_multitransport) + .filter(|_| self.multitransport_flags.is_some()); + let server_blocks = create_gcc_blocks( self.io_channel_id, channel_ids.clone(), requested_protocol, skip_channel_join, self.message_channel_id, - self.offer_multitransport - .filter(|_| self.multitransport_flags.is_some()), + self.advertised_multitransport, ); let settings_response = mcs::ConnectResponse { @@ -1108,7 +1136,7 @@ impl Sequence for Acceptor { }; let offer_udp_fecr = self - .offer_multitransport + .advertised_multitransport .is_some_and(|offer| offer.contains(gcc::MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); let client_supports_udp_fecr = self .multitransport_flags @@ -1381,7 +1409,7 @@ 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 caller has already filtered the offer 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 diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index a56d86f9b5..4d1382d3dd 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -505,6 +505,38 @@ fn non_response_traffic_on_the_message_channel_is_not_misclassified() { ); } +/// With the Basic Security Header that the acceptor's ENCRYPTION_LEVEL_NONE +/// implies, the Initiate Multitransport Response is exactly 12 bytes +/// (MS-RDPBCGR 2.2.15.2). A message-channel payload that starts with a valid +/// response but carries trailing bytes is not one, so it must not be consumed +/// as a late response: it falls through to the same handling as any other +/// unexpected traffic. +#[test] +fn multitransport_response_with_trailing_bytes_is_not_consumed() { + 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)); + 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 + assert_eq!(acceptor.state().name(), "CapabilitiesWaitConfirm"); + + let mut payload = encode_vec(&MultitransportResponsePdu::abort(sent_request.request_id)).unwrap(); + payload.push(0x00); + let response_with_trailing_byte = encode_send_data_request(user_channel_id, message_channel_id, &payload); + let result = acceptor.step(&response_with_trailing_byte, None, &mut WriteBuf::new()); + assert!( + result.is_err(), + "a response followed by trailing bytes must not be swallowed as a late 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 @@ -618,6 +650,54 @@ fn multitransport_not_offered_by_default() { assert_eq!(acceptor.multitransport_soft_sync_negotiated(), None); } +/// The request decision follows what the server advertised during Basic +/// Settings Exchange, not the live offer: enabling multitransport only after +/// the GCC response went out without a MultiTransportChannelData block must +/// not produce a request the client was never told about (MS-RDPBCGR 2.2.1.4). +#[test] +fn multitransport_offer_enabled_after_basic_settings_is_not_requested() { + let mut acceptor = multitransport_acceptor(None); + + let client_blocks = + client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + let (.., server_multitransport) = drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + assert_eq!(server_multitransport, None); + + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + 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()); +} + +/// Likewise for Soft-Sync: adding `SOFT_SYNC_TCP_TO_UDP` to the offer after +/// the GCC response advertised reliable UDP without it must not make +/// `multitransport_soft_sync_negotiated()` report a negotiation that never +/// took place. +#[test] +fn soft_sync_added_after_basic_settings_is_not_negotiated() { + 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 | MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP, + )); + let (.., server_multitransport) = drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + assert_eq!( + server_multitransport, + Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR) + ); + + acceptor.set_multitransport_offer(Some( + MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR | MultiTransportFlags::SOFT_SYNC_TCP_TO_UDP, + )); + + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange + acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping: sends request + assert!(acceptor.multitransport_request().is_some()); + assert_eq!(acceptor.multitransport_soft_sync_negotiated(), Some(false)); +} + /// The acceptor offers multitransport, but the client's GCC blocks never /// 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