diff --git a/Cargo.lock b/Cargo.lock index 7e3280ae9..f70a30b56 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3435,6 +3435,7 @@ dependencies = [ "proptest", "rstest", "tokio", + "tokio-rustls", "visibility", ] diff --git a/crates/ironrdp-server/src/builder.rs b/crates/ironrdp-server/src/builder.rs index 66b624ab6..d1d3e93f6 100644 --- a/crates/ironrdp-server/src/builder.rs +++ b/crates/ironrdp-server/src/builder.rs @@ -151,6 +151,7 @@ impl RdpServerBuilder { where D: RdpServerDisplay + 'static, { + let connection_policy = ConnectionPolicy::default_for(&self.state.security); RdpServerBuilder { state: BuilderDone { addr: self.state.addr, @@ -177,7 +178,7 @@ impl RdpServerBuilder { autodetect_bandwidth: None, autodetect_bandwidth_generation: None, honor_client_desktop_size: None, - connection_policy: ConnectionPolicy::default(), + connection_policy, auto_reconnect_cookie: None, remotefx_quant: Quant::default(), remotefx_entropy_coder: None, @@ -187,6 +188,7 @@ impl RdpServerBuilder { } pub fn with_no_display(self) -> RdpServerBuilder { + let connection_policy = ConnectionPolicy::default_for(&self.state.security); RdpServerBuilder { state: BuilderDone { addr: self.state.addr, @@ -213,7 +215,7 @@ impl RdpServerBuilder { autodetect_bandwidth: None, autodetect_bandwidth_generation: None, honor_client_desktop_size: None, - connection_policy: ConnectionPolicy::default(), + connection_policy, auto_reconnect_cookie: None, remotefx_quant: Quant::default(), remotefx_entropy_coder: None, @@ -354,10 +356,13 @@ impl RdpServerBuilder { /// Choose what [`RdpServer::run`] does with a second connection that /// arrives while a session is already being served: leave it in the backlog - /// ([`ConnectionPolicy::Queue`], the default), close it immediately + /// ([`ConnectionPolicy::Queue`]), close it immediately /// ([`ConnectionPolicy::Reject`]), or let a fully-authenticated newcomer /// take the session over ([`ConnectionPolicy::Preempt`]). /// + /// The default follows the security mode already chosen on this builder; + /// see [`ConnectionPolicy::default_for`]. + /// /// `Preempt`'s takeover is only authentication-gated under /// [`RdpServerSecurity::Hybrid`]; see [`ConnectionPolicy::Preempt`] for the /// per-mode security table. `Reject` closes a newcomer without consulting diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index e4bd48275..37af84767 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -325,18 +325,22 @@ impl CredentialValidator for ExactMatchCredentialValidator { /// What [`RdpServer::run`] does with a second connection that arrives while a /// session is already being served. /// -/// [`RdpServer`] serves one connection at a time. By default a second -/// connection accepted while one is live is left unserved in the OS listen -/// backlog -- from that client's point of view, a silent hang until the first -/// session ends. That is `ironrdp-server`'s pre-existing behaviour, kept as the -/// default ([`Queue`](ConnectionPolicy::Queue)) so an embedder that already -/// relies on it is not surprised by upgrading. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +/// [`RdpServer`] serves one connection at a time, so a second connection +/// accepted while one is live has to go somewhere: left in the OS listen +/// backlog ([`Queue`](ConnectionPolicy::Queue)), closed at once +/// ([`Reject`](ConnectionPolicy::Reject)), or served in place of the running +/// session ([`Preempt`](ConnectionPolicy::Preempt)). +/// +/// The default depends on the security mode -- see +/// [`ConnectionPolicy::default_for`]. There is deliberately no +/// mode-independent [`Default`]: whether takeover is safe out of the box is +/// decided by whether the client is authenticated before it could evict +/// anything, and only the security mode knows that. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ConnectionPolicy { /// Leave the extra connection in the OS listen backlog until the running /// session ends. The pre-existing behaviour: the second client is not /// answered and appears to hang until the first leaves. - #[default] Queue, /// Close the extra connection immediately. The running session is never /// interrupted; the new client fails fast and can retry rather than @@ -389,6 +393,37 @@ pub enum ConnectionPolicy { Preempt, } +impl ConnectionPolicy { + /// The policy [`RdpServerBuilder`](crate::RdpServerBuilder) starts from + /// for a given security mode. + /// + /// Takeover is the least-surprising behaviour for the single-session + /// servers `ironrdp-server` typically backs (one mirroring one desktop): a + /// newly connecting client should replace a stale or abandoned one, not + /// hang behind it. But takeover is only *safe* out of the box where the + /// newcomer is authenticated before it could evict anything, and per the + /// table on [`Preempt`](ConnectionPolicy::Preempt) that is + /// [`RdpServerSecurity::Hybrid`] alone -- under `Tls` or `None` any peer + /// able to complete the handshake clears the bar, and the anti-storm + /// cooldown bars the victim rather than the attacker. So: + /// + /// | Security mode | Default | + /// |---|---| + /// | [`Hybrid`](RdpServerSecurity::Hybrid) | [`Preempt`](ConnectionPolicy::Preempt) -- CredSSP/NLA gates every takeover | + /// | [`Tls`](RdpServerSecurity::Tls), [`None`](RdpServerSecurity::None) | [`Queue`](ConnectionPolicy::Queue) -- the pre-existing behaviour; an unauthenticated takeover is an explicit opt-in | + /// + /// Either can be overridden with + /// [`RdpServerBuilder::with_connection_policy`](crate::RdpServerBuilder::with_connection_policy). + #[must_use] + pub fn default_for(security: &RdpServerSecurity) -> Self { + if authenticates_before_eviction(security) { + Self::Preempt + } else { + Self::Queue + } + } +} + /// Tunnel payloads held while a Soft-Sync response is pending; beyond this the /// client is sending far more than the handful of messages the race allows. const MAX_EARLY_TUNNEL_PAYLOADS: usize = 64; @@ -410,7 +445,8 @@ pub struct RdpServerOptions { /// [`RdpServerBuilder::with_honor_client_desktop_size`](crate::RdpServerBuilder::with_honor_client_desktop_size). pub honor_client_desktop_size: Option, /// What to do with a second connection while a session is being served. - /// Defaults to [`ConnectionPolicy::Queue`]. Set via + /// Defaults to [`ConnectionPolicy::default_for`] the selected security + /// mode. Set via /// [`RdpServerBuilder::with_connection_policy`](crate::RdpServerBuilder::with_connection_policy). pub connection_policy: ConnectionPolicy, /// Quantization values the RemoteFX encoder uses once selected. Defaults @@ -858,6 +894,11 @@ impl ErrorInfoDisconnectHandle { /// The disconnect takes effect only after the server handles this event. /// Unlike [`ServerEvent::Quit`], the client is told why: it decodes the /// PDU and can surface `error` to the user before the connection drops. + /// + /// The one exception is a client that did not set + /// `RNS_UD_CS_SUPPORT_ERRINFO_PDU` in its Client Core Data: MS-RDPBCGR + /// 3.3.5.7.1 forbids sending it the PDU, so it is disconnected without + /// the reason. #[expect( clippy::result_large_err, reason = "SendError hands the whole event back on a closed channel; ServerEvent's size is \ @@ -882,6 +923,13 @@ pub enum ServerEvent { /// client that replaced it, and the two ping-pong indefinitely. Telling /// the loser WHY it was disconnected is what makes it stay away. /// + /// The PDU is only sent to a client that set + /// `RNS_UD_CS_SUPPORT_ERRINFO_PDU` in its Client Core Data (MS-RDPBCGR + /// 3.3.5.7.1 forbids it otherwise); one that did not is dropped without + /// the reason, and the anti-storm cooldown on [`RdpServer::run`] is then + /// the only thing standing between it and the ping-pong above. (mstsc and + /// FreeRDP both set the flag.) + /// /// A more general version of the same PDU/mechanism exists as /// [`Self::Disconnect`] (upstream, `ErrorInfoDisconnectHandle`) for an /// embedder-chosen [`ErrorInfo`]; this variant stays separate because its @@ -1164,6 +1212,10 @@ struct ConnectionState { /// Whether the client advertised `SUPPORT_HEART_BEAT_PDU` in its GCC /// Client Core Data. client_supports_heartbeat: bool, + /// Whether the client advertised `SUPPORT_ERR_INFO_PDU` in its GCC Client + /// Core Data. MS-RDPBCGR 3.3.5.7.1 forbids sending it a Server Set Error + /// Info PDU unless it did. + client_supports_errinfo: bool, /// Auto-detect state, present when the server has auto-detect enabled. /// /// Probes in flight, RTT samples and the session-lifetime lowest RTT all @@ -2884,6 +2936,11 @@ impl RdpServer { Ok((RunState::Continue, encoder)) } + /// `conn.client_supports_errinfo` is the client's `RNS_UD_CS_SUPPORT_ERRINFO_PDU` + /// early-capability opt-in: MS-RDPBCGR 3.3.5.7.1 forbids sending a Server + /// Set Error Info PDU to a client that did not set it, so the two arms below + /// that carry a disconnect reason drop the PDU (and just disconnect) when it + /// is `false`. #[expect( clippy::too_many_arguments, reason = "private per-connection dispatch; the parameters are the connection's negotiated identifiers and transports" @@ -2933,14 +2990,14 @@ impl RdpServer { // against the preempting client — see the variant's docs). ServerEvent::EvictedByOtherConnection => { debug!("evicting this connection -- another client took the session over"); - // KNOWN GAP: MS-RDPBCGR 3.3.5.7.1 says the Set Error Info - // PDU MUST NOT be sent to a client that did not set - // RNS_UD_CS_SUPPORT_ERRINFO_PDU in its Client Core Data - // `earlyCapabilityFlags`, and this sends it unconditionally. - // `AcceptorResult` exposes no early-capability field today, - // so the check is not currently expressible here; the - // pre-existing `send_access_denied` has the identical gap. - // Closing it needs an ironrdp-acceptor API addition. + if !conn.client_supports_errinfo { + // MS-RDPBCGR 3.3.5.7.1: the client did not set + // RNS_UD_CS_SUPPORT_ERRINFO_PDU, so it MUST NOT be + // sent a Set Error Info PDU. Such a client cannot be + // told why it is going away; it just goes. + debug!("client did not opt into Set Error Info PDUs; dropping it without the eviction reason"); + return Ok(RunState::Disconnect); + } let pdu = rdp::headers::ShareDataPdu::ServerSetErrorInfo(ServerSetErrorInfoPdu( ErrorInfo::ProtocolIndependentCode(ProtocolIndependentCode::DisconnectedByOtherconnection), )); @@ -2963,6 +3020,11 @@ impl RdpServer { } ServerEvent::Disconnect(error) => { debug!(?error, "Got disconnect event"); + if !conn.client_supports_errinfo { + // Same MS-RDPBCGR 3.3.5.7.1 rule as the eviction arm. + debug!("client did not opt into Set Error Info PDUs; disconnecting without the reason"); + return Ok(RunState::Disconnect); + } let pdu = rdp::headers::ShareDataPdu::ServerSetErrorInfo(ServerSetErrorInfoPdu(error)); // pduSource=0, not user_channel_id -- same MS-RDPBCGR // 2.2.5.1.1 requirement as the EvictedByOtherConnection @@ -4127,10 +4189,16 @@ impl RdpServer { { debug!("Client accepted"); + // MS-RDPBCGR 3.3.5.7.1: a Set Error Info PDU MUST NOT be sent to a client that did not + // set `SUPPORT_ERR_INFO_PDU`; such a client is just disconnected. + let supports_err_info = result + .client_early_capability_flags + .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_ERR_INFO_PDU); + let is_auto_reconnect = if let Some(reconnect) = result.auto_reconnect.as_ref() { if !self.verify_auto_reconnect_cookie(reconnect) { warn!("Auto-reconnect cookie validation rejected"); - send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; + send_access_denied(result.io_channel_id, result.user_channel_id, supports_err_info, writer).await?; return Err(ServerError::reason("auto-reconnect validation", "cookie rejected")); } @@ -4152,12 +4220,14 @@ impl RdpServer { } Ok(CredentialDecision::Reject) => { warn!("Credential validation rejected"); - send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; + send_access_denied(result.io_channel_id, result.user_channel_id, supports_err_info, writer) + .await?; return Err(ServerError::reason("credential validation", "rejected by validator")); } Err(e) => { error!(error = %e, "Credential validator backend error"); - send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; + send_access_denied(result.io_channel_id, result.user_channel_id, supports_err_info, writer) + .await?; return Err(ServerError::custom("credential validation", e)); } } @@ -4196,6 +4266,9 @@ impl RdpServer { conn.client_supports_heartbeat = result .client_early_capability_flags .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_HEART_BEAT_PDU); + conn.client_supports_errinfo = result + .client_early_capability_flags + .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_ERR_INFO_PDU); if !result.reactivation { for (_channel_key, channel, channel_id) in conn.static_channels.iter_by_key_mut() { debug!(?channel, ?channel_id, "Start"); @@ -4982,11 +5055,18 @@ fn with_connection_handler( /// /// Used to deny a connection after credential validation rejects it, mirroring the /// acceptor's exact-match denial so both paths refuse the same spec-defined way. +/// +/// Sends nothing when the client did not opt in via `SUPPORT_ERR_INFO_PDU` +/// (MS-RDPBCGR 3.3.5.7.1); the caller still closes the connection. async fn send_access_denied( io_channel_id: u16, user_channel_id: u16, + supports_err_info: bool, writer: &mut impl FramedWrite, ) -> ServerResult<()> { + if !supports_err_info { + return Ok(()); + } let info = ServerSetErrorInfoPdu(ErrorInfo::ProtocolIndependentCode( ProtocolIndependentCode::ServerDeniedConnection, )); @@ -5857,7 +5937,10 @@ mod cliprdr_error_tests { // Left in its initial state, so `require_ready` refuses the request // below -- the cheapest reproduction of "the channel said no". let cliprdr: CliprdrServer = Cliprdr::new(Box::new(SilentBackend)); - let mut conn = ConnectionState::default(); + let mut conn = ConnectionState { + client_supports_errinfo: true, + ..ConnectionState::default() + }; conn.static_channels.insert(cliprdr); conn.static_channels .attach_channel_id(TypeId::of::(), 1004); diff --git a/crates/ironrdp-testsuite-core/Cargo.toml b/crates/ironrdp-testsuite-core/Cargo.toml index ff7f30597..eb974a35a 100644 --- a/crates/ironrdp-testsuite-core/Cargo.toml +++ b/crates/ironrdp-testsuite-core/Cargo.toml @@ -76,6 +76,7 @@ pretty_assertions = "1.4" proptest.workspace = true rstest.workspace = true tokio = { version = "1", features = ["macros", "rt", "io-util", "sync", "test-util"] } +tokio-rustls = "0.26" [lints] workspace = true diff --git a/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs b/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs index e21f193ee..32c1ebbbc 100644 --- a/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs +++ b/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs @@ -1,19 +1,25 @@ //! Coverage for [`ConnectionPolicy`] in `RdpServer::run`. //! -//! `run` serves one connection at a time. `Queue` (the default) leaves a -//! second connection unanswered in the listen backlog until the first ends; -//! `Reject` closes it at once so the client fails fast instead of appearing to -//! hang. +//! `run` serves one connection at a time. `Queue` (the default outside +//! `Hybrid` security) leaves a second connection unanswered in the listen +//! backlog until the first ends; `Reject` closes it at once so the client +//! fails fast instead of appearing to hang. use core::net::SocketAddr; use core::sync::atomic::{AtomicUsize, Ordering}; use core::time::Duration; use std::sync::Arc; -use ironrdp_server::{ConnectionHandler, ConnectionPolicy, RdpServer, ServerEvent}; +use ironrdp_server::{ + ConnectionHandler, ConnectionPolicy, DesktopSize, DisplayUpdate, RdpServer, RdpServerDisplay, + RdpServerDisplayUpdates, RdpServerSecurity, ServerEvent, ServerResult, +}; use tokio::io::AsyncReadExt as _; use tokio::net::TcpStream; use tokio::sync::oneshot; +use tokio_rustls::TlsAcceptor; +use tokio_rustls::rustls::server::{ClientHello, ResolvesServerCert}; +use tokio_rustls::rustls::sign::CertifiedKey; async fn bound_addr(sender: &tokio::sync::mpsc::UnboundedSender) -> SocketAddr { // Poll until the accept loop has bound and can answer GetLocalAddr. @@ -71,11 +77,9 @@ async fn reject_closes_a_second_connection_during_a_session() { .await; } -/// With the default `Queue`, a second connection is left unanswered while the +/// Drive `server` and assert a second connection is left unanswered while the /// session runs: the read does not complete within the window. -#[tokio::test] -async fn queue_leaves_a_second_connection_waiting_during_a_session() { - let mut server = build(ConnectionPolicy::Queue); +async fn assert_second_connection_is_left_waiting(mut server: RdpServer) { let sender = server.event_sender().clone(); let local = tokio::task::LocalSet::new(); @@ -102,6 +106,102 @@ async fn queue_leaves_a_second_connection_waiting_during_a_session() { .await; } +/// With `Queue`, a second connection is left unanswered while the session +/// runs. +#[tokio::test] +async fn queue_leaves_a_second_connection_waiting_during_a_session() { + assert_second_connection_is_left_waiting(build(ConnectionPolicy::Queue)).await; +} + +/// Pins the out-of-the-box policy under `with_no_security` end to end: with no +/// `with_connection_policy` call, a second connection is left waiting (`Queue`), +/// NOT served in place of the live one. `None` authenticates nothing, so a +/// `Preempt` default here would let any peer that can reach the port evict the +/// live session. +#[tokio::test] +async fn the_default_under_no_security_leaves_a_second_connection_waiting() { + let server = RdpServer::builder() + .with_addr(([127, 0, 0, 1], 0)) + .with_no_security() + .with_no_input() + .with_no_display() + .build(); + assert_second_connection_is_left_waiting(server).await; +} + +struct StubUpdates; + +#[async_trait::async_trait] +impl RdpServerDisplayUpdates for StubUpdates { + async fn next_update(&mut self) -> ServerResult> { + core::future::pending().await + } +} + +struct StubDisplay; + +#[async_trait::async_trait] +impl RdpServerDisplay for StubDisplay { + async fn size(&mut self) -> DesktopSize { + DesktopSize { width: 64, height: 64 } + } + + async fn updates(&mut self) -> ServerResult> { + Ok(Box::new(StubUpdates)) + } +} + +/// The `with_display_handler` initializer resolves the default separately from +/// `with_no_display`; pin it too, so the two copies cannot drift apart. +#[tokio::test] +async fn the_default_under_no_security_with_a_display_handler_leaves_a_second_connection_waiting() { + let server = RdpServer::builder() + .with_addr(([127, 0, 0, 1], 0)) + .with_no_security() + .with_no_input() + .with_display_handler(StubDisplay) + .build(); + assert_second_connection_is_left_waiting(server).await; +} + +/// A `TlsAcceptor` that can be constructed without any certificate material. +/// It would fail the first handshake, but nothing here handshakes: the tests +/// only need a value of each `RdpServerSecurity` variant. +#[derive(Debug)] +struct NoCert; + +impl ResolvesServerCert for NoCert { + fn resolve(&self, _client_hello: ClientHello<'_>) -> Option> { + None + } +} + +fn tls_acceptor() -> TlsAcceptor { + let config = tokio_rustls::rustls::ServerConfig::builder() + .with_no_client_auth() + .with_cert_resolver(Arc::new(NoCert)); + TlsAcceptor::from(Arc::new(config)) +} + +/// The default policy follows the security mode: takeover out of the box only +/// where the newcomer is authenticated before it could evict anything +/// (`Hybrid`); the pre-existing queue-behind everywhere else. +#[test] +fn the_default_policy_follows_the_security_mode() { + assert_eq!( + ConnectionPolicy::default_for(&RdpServerSecurity::None), + ConnectionPolicy::Queue + ); + assert_eq!( + ConnectionPolicy::default_for(&RdpServerSecurity::Tls(tls_acceptor())), + ConnectionPolicy::Queue + ); + assert_eq!( + ConnectionPolicy::default_for(&RdpServerSecurity::Hybrid((tls_acceptor(), Vec::new()))), + ConnectionPolicy::Preempt + ); +} + struct CountingHandler { accepted: Arc, } diff --git a/crates/ironrdp-testsuite-extra/tests/e2e.rs b/crates/ironrdp-testsuite-extra/tests/e2e.rs index 679b1ba8f..e9b1c435d 100644 --- a/crates/ironrdp-testsuite-extra/tests/e2e.rs +++ b/crates/ironrdp-testsuite-extra/tests/e2e.rs @@ -1792,3 +1792,74 @@ async fn connection_handler_sees_every_hook_across_a_preemption() { })) .await; } + +/// Under `Hybrid` the out-of-the-box policy is `Preempt`: with no +/// `with_connection_policy` call, a second client that completes CredSSP takes +/// the session over, and the first client's connection is closed. +#[tokio::test] +async fn the_default_under_hybrid_lets_an_authenticated_newcomer_take_over() { + const HYBRID_USER: &str = "user"; + const HYBRID_PASSWORD: &str = "password"; + + let cert_path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/certs/server-cert.pem"); + let key_path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/certs/server-key.pem"); + let identity = TlsIdentityCtx::init_from_paths(&cert_path, &key_path).expect("failed to init TLS identity"); + let acceptor = identity.make_acceptor().expect("failed to build TLS acceptor"); + + // Keep the display sender alive: once it drops, the server closes the session. + let (_display_tx, display_rx) = mpsc::unbounded_channel(); + let mut server = RdpServer::builder() + .with_addr(([127, 0, 0, 1], 0)) + .with_hybrid(acceptor, identity.pub_key.clone()) + .with_input_handler(TestInputHandler) + .with_display_handler(TestDisplay { + rx: Arc::new(Mutex::new(display_rx)), + }) + .build(); + server.set_credentials(Some(server::Credentials { + username: HYBRID_USER.into(), + password: HYBRID_PASSWORD.into(), + domain: None, + })); + let ev = server.event_sender().clone(); + + let client_config = || connector::Config { + credentials: connector::Credentials::UsernamePassword { + username: HYBRID_USER.into(), + password: HYBRID_PASSWORD.into(), + }, + ..default_client_config() + }; + + let local = tokio::task::LocalSet::new(); + Box::pin(local.run_until(async move { + let server_task = tokio::task::spawn_local(async move { + let _ = server.run().await; + }); + let server_addr = local_addr_of(&ev).await; + let connect = || { + connect_client(server_addr, |client_addr| { + connector::ClientConnector::new(client_config(), client_addr) + }) + }; + + let (mut first, _) = connect().await; + + // The newcomer can only finish its handshake if the server serves it, + // which under `Queue` it would not do while `first` is live. + let (second, _) = tokio::time::timeout(Duration::from_secs(10), connect()) + .await + .expect("the authenticated newcomer must be served (Preempt), not queued"); + + // The incumbent is evicted: its transport ends rather than staying open. + let evicted = tokio::time::timeout(Duration::from_secs(5), async { + while first.read_pdu().await.is_ok() {} + }) + .await; + assert!(evicted.is_ok(), "the incumbent's connection must be closed"); + + drop(second); + server_task.abort(); + })) + .await; +}