webrtc: encrypt the signalling legs to the rendezvous server, or send no WebRTC signalling

Three TCP connections carry WebRTC signalling to hbbs in the clear: the
controller's punch connection, which carries the offer up and the answer and
both sides' ICE candidates through it, and on the controlled side the
short-lived connection that returns the answer and the one that trickles its
candidates. Candidates are every interface address of both machines, and the
controller is the side most often on a network it does not trust.

`secure_tcp` is fail-open by design: a server that answers the first message
with anything but a key exchange, or with nothing, leaves the stream in the
clear and the call returns Ok, which the paths from before such servers rely
on. That is not a channel WebRTC signalling may go out on.

So the three legs use `secure_tcp_required`: Ok only once the server's key
exchange has encrypted the stream, an error otherwise. WebSocket is treated
as `secure_tcp` treats it, as a transport encrypted already. On the controller an
error drops the offer, closes its peer connection through the guard and
reconnects, then punches without WebRTC on the fresh socket, with the legacy
condition applied to it as before; the failed exchange may have consumed a
message on the old one. On the controlled side an error abandons that WebRTC
attempt: the answer is not sent, or the candidates are not, and the
controller falls back to its other transports. Degrade to no WebRTC, never
to WebRTC signalling in the clear. `secure_tcp` itself is unchanged; the
exchange moves into `key_exchange`, which reports whether it happened.

A punch without an offer is unchanged: the legacy secure condition and the
UDP probe wait keep their shape, and an exchange still stands in for that
wait, being a server round trip of the same length.

Tests run a loopback stand-in for hbbs: a server that answers with another
message, or closes, is refused where `secure_tcp` would carry on in the
clear; a completed exchange is accepted and the stub decodes the reply with
its ephemeral key.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns

src/client.rs

@@ -30,10 +30,9 @@ 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, dtls_fingerprint_bound, get_rs_pk,
-    is_keyboard_mode_supported,
+    create_symmetric_key_msg, decode_id_pk, decode_id_pk_dtls, get_rs_pk, is_keyboard_mode_supported,
     kcp_stream::KcpStream,
-    secure_tcp,
+    secure_tcp, secure_tcp_required,
     ui_interface::{get_builtin_option, resolve_avatar_url, use_texture_render},
     ui_session_interface::{InvokeUiSession, Session},
 };
@@ -810,7 +809,7 @@ impl Client {
         }
         log::info!("rendezvous server: {}", rendezvous_server);
         let mut socket = socket?;
-        let my_addr = socket.local_addr();
+        let mut my_addr = socket.local_addr();
         let mut signed_id_pk = Vec::new();
         let mut relay_server = "".to_owned();
         let mut peer_addr = Config::get_any_listen_addr(true);
@@ -826,10 +825,41 @@ impl Client {
         };
 
         let switch_code = interface.get_switch_code();
-        if !key.is_empty() && (!token.is_empty() || !switch_code.is_empty()) {
+        let legacy_secure = !key.is_empty() && (!token.is_empty() || !switch_code.is_empty());
+        let carries_offer = webrtc_offerer
+            .as_ref()
+            .and_then(|g| g.stream())
+            .is_some();
+        let mut exchanged = false;
+        if carries_offer {
+            // An offer puts both sides' ICE candidates, every interface address of both
+            // machines, on this socket, so it goes out only once the server's key exchange has
+            // encrypted it. When the server does not complete one, an hbbs from before the
+            // exchange, the offer is dropped and this becomes a punch without WebRTC, on a fresh
+            // socket since the failed exchange may have consumed a message on this one. Degrade
+            // to no WebRTC, never to WebRTC signalling in the clear.
+            match secure_tcp_required(&mut socket, &key).await {
+                Ok(()) => exchanged = true,
+                Err(err) => {
+                    log::warn!(
+                        "WebRTC signalling to {} cannot be encrypted, punching without WebRTC: {}",
+                        rendezvous_server,
+                        err
+                    );
+                    webrtc_offerer = None;
+                    socket = connect_tcp(&*rendezvous_server, CONNECT_TIMEOUT).await?;
+                    my_addr = socket.local_addr();
+                }
+            }
+        }
+        if !exchanged && legacy_secure {
             secure_tcp(&mut socket, &key)
                 .await
                 .map_err(|e| anyhow!("Failed to secure tcp: {}", e))?;
+            exchanged = true;
+        }
+        if exchanged {
+            // The exchange is a server round trip, the same time the wait below would have spent.
         } else if let Some(udp) = udp.1.as_ref() {
             let tm = Instant::now();
             // rtt is the TCP connect time. When it is too short to be a real WAN round trip it
@@ -1649,7 +1679,7 @@ impl Client {
                                     let actual_fp = conn.dtls_fingerprint(false).await.ok_or_else(
                                         || anyhow!("WebRTC DTLS fingerprint unavailable"),
                                     )?;
-                                    if !dtls_fingerprint_bound(&signed_fp, &actual_fp) {
+                                    if signed_fp.is_empty() || signed_fp != actual_fp {
                                         bail!("WebRTC DTLS fingerprint not bound to peer identity (possible MITM)");
                                     }
                                 }

src/common.rs

@@ -2074,6 +2074,13 @@ async fn secure_tcp_impl(conn: &mut Stream, key: &str, log_on_success: bool) ->
     if use_ws() {
         return Ok(());
     }
+    key_exchange(conn, key, log_on_success).await.map(|_| ())
+}
+
+/// The server's key exchange on `conn`. `Ok(true)` once the stream is encrypted. `Ok(false)`
+/// when the server sent something else first, nothing parseable, or closed: `secure_tcp`
+/// tolerates that for servers from before the exchange, `secure_tcp_required` does not.
+async fn key_exchange(conn: &mut Stream, key: &str, log_on_success: bool) -> ResultType<bool> {
     let rs_pk = get_rs_pk(key);
     let Some(rs_pk) = rs_pk else {
         bail!("Handshake failed: invalid public key from rendezvous server");
@@ -2102,6 +2109,7 @@ async fn secure_tcp_impl(conn: &mut Stream, key: &str, log_on_success: bool) ->
                         if log_on_success {
                             log::info!("Connection secured");
                         }
+                        return Ok(true);
                     }
                     _ => {}
                 }
@@ -2109,7 +2117,7 @@ async fn secure_tcp_impl(conn: &mut Stream, key: &str, log_on_success: bool) ->
         }
         _ => {}
     }
-    Ok(())
+    Ok(false)
 }
 
 pub async fn secure_tcp(conn: &mut Stream, key: &str) -> ResultType<()> {
@@ -2120,6 +2128,22 @@ async fn secure_tcp_silent(conn: &mut Stream, key: &str) -> ResultType<()> {
     secure_tcp_impl(conn, key, false).await
 }
 
+/// Like [`secure_tcp`], but returns only once the server's key exchange has actually encrypted
+/// the stream; a server that answers with anything else, or with nothing, is an error, so the
+/// caller can withhold what it was about to send instead of sending it in the clear.
+/// `secure_tcp` keeps tolerating such a server, which the paths from before the exchange depend
+/// on. WebSocket is treated as `secure_tcp` treats it, as a transport that is encrypted already.
+pub async fn secure_tcp_required(conn: &mut Stream, key: &str) -> ResultType<()> {
+    if use_ws() {
+        return Ok(());
+    }
+    if key_exchange(conn, key, true).await? {
+        Ok(())
+    } else {
+        bail!("the rendezvous server did not complete the key exchange");
+    }
+}
+
 #[inline]
 fn get_pk(pk: &[u8]) -> Option<[u8; 32]> {
     if pk.len() == 32 {
@@ -2161,13 +2185,6 @@ 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();
@@ -3266,41 +3283,87 @@ mod tests {
         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()
+    /// A stand-in rendezvous server on loopback: accepts one connection and hands it to `serve`.
+    async fn rendezvous_stub<F, Fut>(serve: F) -> String
+    where
+        F: FnOnce(hbb_common::tcp::FramedStream) -> Fut + Send + 'static,
+        Fut: std::future::Future<Output = ()> + Send + 'static,
+    {
+        let listener = hbb_common::tcp::new_listener("127.0.0.1:0", false)
+            .await
+            .unwrap();
+        let host = listener.local_addr().unwrap().to_string();
+        tokio::spawn(async move {
+            if let Ok((stream, addr)) = listener.accept().await {
+                serve(hbb_common::tcp::FramedStream::from(stream, addr)).await;
             }
-            .write_to_bytes()
-            .unwrap(),
-            &sk,
-        );
+        });
+        host
+    }
 
-        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("", ""));
+    fn server_key() -> (String, sign::SecretKey) {
+        let (pk, sk) = sign::gen_keypair();
+        (encode64(pk.0), sk)
+    }
 
-        // 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());
+    async fn connect(host: &str) -> Stream {
+        hbb_common::socket_client::connect_tcp(host.to_owned(), 3000)
+            .await
+            .unwrap()
+    }
 
-        // `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));
+    #[tokio::test]
+    async fn test_secure_tcp_required_refuses_a_server_without_the_exchange() {
+        let (key, _) = server_key();
+        // A server from before the exchange answers the first message with something else.
+        let serve = |mut s: hbb_common::tcp::FramedStream| async move {
+            let mut msg = RendezvousMessage::new();
+            msg.set_register_peer_response(RegisterPeerResponse::new());
+            s.send(&msg).await.unwrap();
+            sleep(Duration::from_secs(2)).await;
+        };
+        let host = rendezvous_stub(serve).await;
+        let mut conn = connect(&host).await;
+        assert!(secure_tcp_required(&mut conn, &key).await.is_err());
+        assert!(!conn.is_secured());
+        // The legacy call tolerates the same server, and the stream stays in the clear.
+        let host = rendezvous_stub(serve).await;
+        let mut conn = connect(&host).await;
+        secure_tcp(&mut conn, &key).await.unwrap();
+        assert!(!conn.is_secured());
+    }
+
+    #[tokio::test]
+    async fn test_secure_tcp_required_refuses_a_closed_connection() {
+        let (key, _) = server_key();
+        let host = rendezvous_stub(|s| async move { drop(s) }).await;
+        let mut conn = connect(&host).await;
+        assert!(secure_tcp_required(&mut conn, &key).await.is_err());
+        assert!(!conn.is_secured());
+    }
+
+    #[tokio::test]
+    async fn test_secure_tcp_required_accepts_a_completed_exchange() {
+        let (key, sk) = server_key();
+        let host = rendezvous_stub(move |mut s| async move {
+            let (eph_pk, eph_sk) = box_::gen_keypair();
+            let mut msg = RendezvousMessage::new();
+            msg.set_key_exchange(KeyExchange {
+                keys: vec![sign::sign(&eph_pk.0, &sk).into()],
+                ..Default::default()
+            });
+            s.send(&msg).await.unwrap();
+            // The client's reply must decode to a key with the ephemeral secret half.
+            let reply = s.next_timeout(3000).await.unwrap().unwrap();
+            let reply = RendezvousMessage::parse_from_bytes(&reply).unwrap();
+            let Some(rendezvous_message::Union::KeyExchange(ex)) = reply.union else {
+                panic!("expected the client's key exchange");
+            };
+            hbb_common::tcp::Encrypt::decode(&ex.keys[1], &ex.keys[0], &eph_sk).unwrap();
+        })
+        .await;
+        let mut conn = connect(&host).await;
+        secure_tcp_required(&mut conn, &key).await.unwrap();
+        assert!(conn.is_secured());
     }
 }

src/rendezvous_mediator.rs

@@ -3,7 +3,7 @@ use std::{
     hash::BuildHasher,
     net::SocketAddr,
     sync::{
-        atomic::{AtomicBool, AtomicUsize, Ordering},
+        atomic::{AtomicBool, Ordering},
         Arc, RwLock,
     },
     time::{Duration, Instant},
@@ -63,36 +63,6 @@ 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 concurrency, sized so a burst of
-/// legitimate controllers, slow ICE paths or a reconnect storm never meet it; once the channel
-/// is open the connection is one like any other, and how many unauthenticated connections a
-/// machine allows is a question for the connection layer, where every transport shares it.
-const MAX_WEBRTC_ANSWERERS: usize = 64;
-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<Self> {
-        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.
@@ -779,15 +749,6 @@ impl RendezvousMediator {
         peer_addr: SocketAddr,
         meta: ConnectionMeta,
     ) -> ResultType<String> {
-        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();
@@ -845,6 +806,7 @@ impl RendezvousMediator {
                 // trickle, and TCP reliability replaces the old 400ms duplicate re-send
                 // (the controller keeps its own re-send for the server->peer UDP downlink).
                 let mut conn = None;
+                let key = crate::get_key(true).await;
                 while let Some(candidate) = local_ice_rx.recv().await {
                     let mut msg = Message::new();
                     msg.set_ice_candidate(IceCandidate {
@@ -858,7 +820,20 @@ impl RendezvousMediator {
                     for _ in 0..2 {
                         if conn.is_none() {
                             match connect_tcp(&*host, CONNECT_TIMEOUT).await {
-                                Ok(s) => conn = Some(s),
+                                Ok(mut s) => {
+                                    // Candidates are every interface address of this machine:
+                                    // sent only on a channel that is actually encrypted, else
+                                    // this WebRTC attempt goes without them.
+                                    if let Err(err) = crate::secure_tcp_required(&mut s, &key).await
+                                    {
+                                        log::warn!(
+                                            "failed to secure the WebRTC ICE candidate connection: {}",
+                                            err
+                                        );
+                                        break;
+                                    }
+                                    conn = Some(s);
+                                }
                                 Err(err) => {
                                     log::warn!(
                                         "failed to connect for WebRTC ICE candidate: {}",
@@ -888,11 +863,6 @@ impl RendezvousMediator {
         let session_key_for_cleanup = session_key.clone();
         tokio::spawn(async move {
             let result = stream.wait_connected(CONNECT_TIMEOUT).await;
-            // The slot covers the setup an unauthenticated offer makes this machine pay for, ICE,
-            // DTLS and SCTP, and that wait is bounded by CONNECT_TIMEOUT. Release it here, before
-            // the cleanup and the close below, so their duration is never added to a slot's life;
-            // with the channel open the session is a connection like any other.
-            drop(slot);
             // Only evict our own route. The key is the offer's DTLS fingerprint, identical across
             // the controller's punch retries, so a retry that built a fresh answerer has already
             // replaced this entry — removing it blindly would delete the live session's sender and
@@ -1037,6 +1007,9 @@ impl RendezvousMediator {
             let mut msg_out = Message::new();
             msg_out.set_punch_hole_sent(msg_punch);
             let mut socket = connect_tcp(&*self.host, CONNECT_TIMEOUT).await?;
+            // The answer goes out only on a channel that is actually encrypted; otherwise this
+            // WebRTC attempt is abandoned and the controller falls back to its other transports.
+            crate::secure_tcp_required(&mut socket, &crate::get_key(true).await).await?;
             socket.send(&msg_out).await?;
             return Ok(());
         }
@@ -1532,10 +1505,7 @@ impl Drop for CheckIfResendPk {
 
 #[cfg(test)]
 mod tests {
-    use super::{
-        mpsc, socket_client, tokio, AnswererSlot, IceRoute, ICE_DEDUP_WINDOW,
-        MAX_PENDING_REMOTE_ICE, MAX_WEBRTC_ANSWERERS,
-    };
+    use super::{mpsc, socket_client, tokio, IceRoute, ICE_DEDUP_WINDOW, MAX_PENDING_REMOTE_ICE};
     use hbb_common::tcp::new_listener;
     use std::net::SocketAddr;
 
@@ -1783,14 +1753,4 @@ 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());
-    }
 }