From 35389365b73377cb4b0215420bc975b308cea563 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Mon, 14 Sep 2026 22:17:47 +0800 Subject: [PATCH] webrtc: cap concurrent answerer setups, pin the DTLS fingerprint binding with a test A WebRTC offer reaches the controlled side before any password or accept prompt, and answering one builds a peer connection that binds a socket per interface and runs ICE for up to CONNECT_TIMEOUT. A forged TCP punch reuses the mediator's local port for one connect; a forged offer costs all of that, and nothing bounded how many could be in flight at once. SESSIONS dedups by offer fingerprint, which only stops replays of one offer. spawn_webrtc_answerer now takes one of 16 slots before building the peer connection. The wait for the data channel is bounded by CONNECT_TIMEOUT, and what the slot stands for is the peer connection an unauthenticated offer had this machine build, ICE, DTLS and SCTP: on an open channel it is given back at once, and on a failed one it goes with the pc into the detached teardown and comes back when that has finished. pc.close() has no timeout of its own, so a slot freed where the task gives up would let a teardown that never finished pile pcs up unbounded with the count reading zero; held, a stuck teardown costs WebRTC capacity and the offers past the cap degrade to punch and relay. Every failure before the pc exists releases the slot through the guard's drop. From the open channel on the connection is one like any other, and the connection layer bounds unauthenticated connections in number and in time for every transport alike (#16237), a peer that stalls in the identity handshake or after it included. So this guard stays inside the WebRTC path, sized above what legitimate controllers reach at once in the seconds ICE takes. Past the cap the offer is declined with an empty answer, the reply the controller already gets from a peer without WebRTC, so it carries on over punch and relay. Declines log through the throttled-log macro. At the cap a re-sent PunchHole for a live session also gets an empty answer rather than the cached one, since the slot is taken before the cache is consulted; only reachable at the cap, where degrading is the point. The other change is regression coverage for the signed DTLS fingerprint binding, which is unchanged. The controller's defence against a rendezvous or relay that swaps SDP fingerprints is the fingerprint the controlled side signs into IdPk and the comparison in secure_connection, and neither had a test. The comparison moves into dtls_fingerprint_bound so it can have one, along with decode_id_pk_dtls: the fingerprint round-trips under the signature, another key or an edited payload yields nothing, empty never binds, and decode_id_pk still sees the same id and pk. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns --- src/client.rs | 5 +-- src/common.rs | 45 ++++++++++++++++++++++++ src/rendezvous_mediator.rs | 70 +++++++++++++++++++++++++++++++++++--- 3 files changed, 114 insertions(+), 6 deletions(-) diff --git a/src/client.rs b/src/client.rs index 3710d2639..0e92796a1 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}, @@ -1671,7 +1672,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 9dadb1c86..88ddea191 100644 --- a/src/common.rs +++ b/src/common.rs @@ -2166,6 +2166,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(); @@ -3263,4 +3270,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..71ee9e83b 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}, @@ -63,6 +63,36 @@ 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. A guard against pathological setup concurrency, above what +/// legitimate controllers reach at once in the seconds ICE takes; once the channel is open the +/// connection is one like any other, and the connection layer bounds unauthenticated +/// connections in number and in time for every transport alike. +const MAX_WEBRTC_ANSWERERS: usize = 16; +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 +779,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(); @@ -865,10 +904,20 @@ impl RendezvousMediator { if let Err(err) = result { log::warn!("webrtc wait_connected failed: {}", err); // Release the pc now rather than waiting for the ICE agent to time out into a - // terminal state (~30s); this also drops the SESSIONS entry promptly. - stream.close().await; + // terminal state (~30s); this also drops the SESSIONS entry promptly. The slot + // goes with it and comes back when the teardown has finished, not when this task + // gives up on the offer: what it stands for is a peer connection built for an + // unauthenticated offer, and one that will not die still costs what it costs. + // `pc.close()` has no timeout of its own, so were the slot freed here a teardown + // that never finished would leave the pcs to pile up unbounded, with the count + // reading zero. Detached, the wait is on WEBRTC_RT, which owns the pc, and not on + // this task. + stream.close_detached_with(slot); return; } + // The channel is open: from here the session is a connection like any other, and the + // connection layer's own limits apply to it. + drop(slot); // create_tcp_connection takes ownership of the stream; keep a handle to close the pc // once the session returns. It runs the whole session and returns Ok on normal end, // Err on setup failure — either way the pc must be closed, else it lingers forever in @@ -1488,7 +1537,10 @@ 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::net::SocketAddr; @@ -1736,4 +1788,14 @@ mod tests { "must return when the grace runs out, not a backoff later" ); } + + #[test] + fn test_answerer_slots_cap_and_release() { + let held: Vec<_> = (0..MAX_WEBRTC_ANSWERERS) + .map(|_| AnswererSlot::take().unwrap()) + .collect(); + assert!(AnswererSlot::take().is_none()); + drop(held); + assert!(AnswererSlot::take().is_some()); + } }