diff --git a/src/client.rs b/src/client.rs index 10330cfd6..b58d43272 100644 --- a/src/client.rs +++ b/src/client.rs @@ -30,7 +30,8 @@ use uuid::Uuid; use crate::{ check_port, common::input::{MOUSE_BUTTON_LEFT, MOUSE_BUTTON_RIGHT, MOUSE_TYPE_DOWN, MOUSE_TYPE_UP}, - create_symmetric_key_msg, decode_id_pk, decode_id_pk_dtls, get_rs_pk, is_keyboard_mode_supported, + create_symmetric_key_msg, decode_id_pk, decode_id_pk_dtls, dtls_fingerprint_bound, get_rs_pk, + is_keyboard_mode_supported, kcp_stream::KcpStream, secure_tcp, ui_interface::{get_builtin_option, resolve_avatar_url, use_texture_render}, @@ -1648,7 +1649,7 @@ impl Client { let actual_fp = conn.dtls_fingerprint(false).await.ok_or_else( || anyhow!("WebRTC DTLS fingerprint unavailable"), )?; - if signed_fp.is_empty() || signed_fp != actual_fp { + if !dtls_fingerprint_bound(&signed_fp, &actual_fp) { bail!("WebRTC DTLS fingerprint not bound to peer identity (possible MITM)"); } } diff --git a/src/common.rs b/src/common.rs index 00a409a63..95182e232 100644 --- a/src/common.rs +++ b/src/common.rs @@ -2161,6 +2161,13 @@ pub fn decode_id_pk_dtls( } } +/// Whether the DTLS fingerprint a WebRTC peer signed into its identity is the one of the channel +/// actually negotiated. An empty signed value binds nothing: on a WebRTC channel it is either a +/// peer that could not sign one or a rendezvous/relay that stripped it, and both fail closed. +pub fn dtls_fingerprint_bound(signed_fp: &str, actual_fp: &str) -> bool { + !signed_fp.is_empty() && signed_fp == actual_fp +} + pub fn create_symmetric_key_msg(their_pk_b: [u8; 32]) -> (Bytes, Bytes, secretbox::Key) { let their_pk_b = box_::PublicKey(their_pk_b); let (our_pk_b, out_sk_b) = box_::gen_keypair(); @@ -3258,4 +3265,42 @@ mod tests { assert_eq!(combined_mask & MOUSE_TYPE_MASK, MOUSE_TYPE_DOWN); assert_eq!(combined_mask >> 3, MOUSE_BUTTON_LEFT | MOUSE_BUTTON_RIGHT); } + + #[test] + fn test_dtls_fingerprint_travels_signed_and_binds() { + let (pk, sk) = sign::gen_keypair(); + let fp = "sha-256 0A:1B:2C"; + let signed = sign::sign( + &IdPk { + id: "123456789".to_owned(), + pk: Bytes::from(vec![7u8; 32]), + dtls_fingerprint: fp.to_owned(), + ..Default::default() + } + .write_to_bytes() + .unwrap(), + &sk, + ); + + let (id, their_pk, signed_fp) = decode_id_pk_dtls(&signed, &pk).unwrap(); + assert_eq!(id, "123456789"); + assert_eq!(their_pk, [7u8; 32]); + assert_eq!(signed_fp, fp); + assert!(dtls_fingerprint_bound(&signed_fp, fp)); + assert!(!dtls_fingerprint_bound(&signed_fp, "sha-256 0A:1B:2D")); + assert!(!dtls_fingerprint_bound("", "")); + + // The fingerprint is under the signature: a blob verified with another key yields + // nothing, and one whose payload was edited in transit fails verification. + let (other_pk, _) = sign::gen_keypair(); + assert!(decode_id_pk_dtls(&signed, &other_pk).is_err()); + let mut tampered = signed.clone(); + let last = tampered.len() - 1; + tampered[last] ^= 1; + assert!(decode_id_pk_dtls(&tampered, &pk).is_err()); + + // `decode_id_pk` is the same blob minus the fingerprint, so the field is invisible to + // non-WebRTC handshakes. + assert_eq!(decode_id_pk(&signed, &pk).unwrap(), (id, their_pk)); + } } diff --git a/src/rendezvous_mediator.rs b/src/rendezvous_mediator.rs index 0436f060a..49441e74b 100644 --- a/src/rendezvous_mediator.rs +++ b/src/rendezvous_mediator.rs @@ -3,7 +3,7 @@ use std::{ hash::BuildHasher, net::SocketAddr, sync::{ - atomic::{AtomicBool, Ordering}, + atomic::{AtomicBool, AtomicUsize, Ordering}, Arc, RwLock, }, time::{Duration, Instant}, @@ -46,6 +46,7 @@ fn connection_meta( ConnectionMeta { control_permissions, controlled_context, + webrtc_pre_auth_hold: None, } } @@ -63,6 +64,33 @@ const MAX_PENDING_REMOTE_ICE: usize = 64; /// Queued candidates remembered so the controller's re-send is skipped instead of taking a slot /// of its own. Far more than an honest peer gathers, at eight bytes each. const ICE_DEDUP_WINDOW: usize = 256; +/// Answerers between an offer and an open data channel. An offer arrives before any password or +/// accept prompt, and each one builds a peer connection that binds a socket per interface and +/// runs ICE for up to `CONNECT_TIMEOUT`, where a forged TCP punch costs one connect. Past this +/// many the offer is declined, and the controller carries on over punch and relay as it does +/// for a peer without WebRTC. +const MAX_WEBRTC_ANSWERERS: usize = 8; +static WEBRTC_ANSWERERS: AtomicUsize = AtomicUsize::new(0); + +/// One of the `MAX_WEBRTC_ANSWERERS` slots, given back on drop. +struct AnswererSlot; + +impl AnswererSlot { + fn take() -> Option { + WEBRTC_ANSWERERS + .fetch_update(Ordering::AcqRel, Ordering::Acquire, |n| { + (n < MAX_WEBRTC_ANSWERERS).then(|| n + 1) + }) + .ok() + .map(|_| Self) + } +} + +impl Drop for AnswererSlot { + fn drop(&mut self) { + WEBRTC_ANSWERERS.fetch_sub(1, Ordering::AcqRel); + } +} // The rendezvous ICE route is reachable without a prior punch and the peer decides how many // candidates it sends, so these sites would let someone else set how much this machine writes to // its log file. One line a minute each, carrying the suppressed count. @@ -749,6 +777,15 @@ impl RendezvousMediator { peer_addr: SocketAddr, meta: ConnectionMeta, ) -> ResultType { + let Some(slot) = AnswererSlot::take() else { + hbb_common::throttled_log!( + ICE_LOG_INTERVAL, + warn, + "declined a WebRTC offer: {} answerers already in flight", + MAX_WEBRTC_ANSWERERS + ); + return Ok(String::new()); + }; let mut stream = WebRTCStream::new(&ph.webrtc_sdp_offer, relay_only_ice, CONNECT_TIMEOUT).await?; let answer = stream.local_endpoint().to_owned(); @@ -875,6 +912,13 @@ impl RendezvousMediator { // SESSIONS (its state handler only fires on a terminal ICE state, which a cleanly // closed session may never reach) leaking the pc, channels, and socket fds. let stream_for_cleanup = stream.clone(); + // A data channel at Open is not yet an authenticated peer: create_tcp_connection still + // runs the SignedId/PublicKey handshake, up to CONNECT_TIMEOUT, before the peer proves + // who it is. Carry the slot into that connection so a forged offer that opens a channel + // and then stalls keeps occupying one until the handshake closes it out, rather than + // freeing it here and letting the next batch of eight begin. + let mut meta = meta; + meta.webrtc_pre_auth_hold = Some(std::sync::Arc::new(slot)); if let Err(err) = crate::server::create_tcp_connection( server, Stream::WebRTC(stream), @@ -1488,9 +1532,14 @@ impl Drop for CheckIfResendPk { #[cfg(test)] mod tests { - use super::{mpsc, socket_client, tokio, IceRoute, ICE_DEDUP_WINDOW, MAX_PENDING_REMOTE_ICE}; + use super::{ + mpsc, socket_client, tokio, AnswererSlot, IceRoute, ICE_DEDUP_WINDOW, + MAX_PENDING_REMOTE_ICE, MAX_WEBRTC_ANSWERERS, + }; use hbb_common::tcp::new_listener; + use std::any::Any; use std::net::SocketAddr; + use std::sync::Arc; // A SOCKS proxy makes `connect_tcp_local` dial the proxy and ignore the local address, so // nothing these two assert can hold. Read once, from the same global config production reads. @@ -1736,4 +1785,36 @@ mod tests { "must return when the grace runs out, not a backoff later" ); } + + // The slot counter is process-global and the test harness runs tests in parallel threads, + // so every test that takes slots holds this first; a poisoned lock is still a lock. + static SLOT_TESTS: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[test] + fn test_answerer_slots_cap_and_release() { + let _serial = SLOT_TESTS.lock().unwrap_or_else(|e| e.into_inner()); + let held: Vec<_> = (0..MAX_WEBRTC_ANSWERERS) + .map(|_| AnswererSlot::take().unwrap()) + .collect(); + assert!(AnswererSlot::take().is_none()); + drop(held); + assert!(AnswererSlot::take().is_some()); + } + + // The answerer hands its slot into the connection type-erased, exactly as it reaches the + // authenticated peer; this pins that the erased hold still occupies a slot and that dropping + // it — as authorization or a dropped Connection does — is what releases it. If the slot were + // freed at data-channel Open instead, this connection could not keep the last one occupied. + #[test] + fn test_erased_slot_holds_until_dropped() { + let _serial = SLOT_TESTS.lock().unwrap_or_else(|e| e.into_inner()); + let held: Vec<_> = (0..MAX_WEBRTC_ANSWERERS - 1) + .map(|_| AnswererSlot::take().unwrap()) + .collect(); + let erased: Arc = Arc::new(AnswererSlot::take().unwrap()); + assert!(AnswererSlot::take().is_none()); + drop(erased); + assert!(AnswererSlot::take().is_some()); + drop(held); + } } diff --git a/src/server.rs b/src/server.rs index f39620f51..2e7efdb54 100644 --- a/src/server.rs +++ b/src/server.rs @@ -88,6 +88,11 @@ type ConnMap = HashMap; pub struct ConnectionMeta { pub control_permissions: Option, pub controlled_context: Option, + /// A resource permit the answerer path attaches to a WebRTC connection so it stays counted + /// until RustDesk's own authentication, not merely until the data channel opens. Type-erased + /// so this shared struct need not know the answerer's slot type; opaque everywhere else, it is + /// only carried and dropped. `None` on every other transport. + pub webrtc_pre_auth_hold: Option>, } #[cfg(any(target_os = "macos", target_os = "linux"))] diff --git a/src/server/connection.rs b/src/server/connection.rs index 8ac850958..b1fc70be2 100644 --- a/src/server/connection.rs +++ b/src/server/connection.rs @@ -264,6 +264,9 @@ pub struct Connection { port_forward_address: String, tx_to_cm: mpsc::UnboundedSender, authorized: bool, + // Held for a WebRTC answerer until authorization, then dropped to free its resource slot; + // see ConnectionMeta::webrtc_pre_auth_hold. None on every other transport. + webrtc_pre_auth_hold: Option>, require_2fa: Option, awaiting_2fa: bool, keyboard: bool, @@ -429,6 +432,7 @@ impl Connection { let super::ConnectionMeta { control_permissions, controlled_context, + webrtc_pre_auth_hold, } = meta; // Android is not supported yet, so we always set control_permissions to None. #[cfg(target_os = "android")] @@ -483,6 +487,7 @@ impl Connection { port_forward_address: "".to_owned(), tx_to_cm, authorized: false, + webrtc_pre_auth_hold, keyboard: Self::permission(keys::OPTION_ENABLE_KEYBOARD, &control_permissions), clipboard: Self::permission(keys::OPTION_ENABLE_CLIPBOARD, &control_permissions), audio: Self::permission(keys::OPTION_ENABLE_AUDIO, &control_permissions), @@ -1792,6 +1797,9 @@ impl Connection { return false; } self.authorized = true; + // The peer is authenticated now, so release any WebRTC pre-auth resource slot; a no-op + // on other transports. + self.webrtc_pre_auth_hold = None; // Releases the budget `check_id_whitelist` charges against this address: only a peer // that got this far proved more than a self-reported id. self.clear_id_whitelist_failures();