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 four 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. A relay response carrying an
answer, which the symmetric-NAT and forced-relay branches send on a
connection of their own, keeps the relay and loses only the answer: the
response goes without it, on a fresh socket. 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 takes
this socket straight to the punch as before, and every other punch waits for
the UDP NAT test as before. The exchange does not replace that wait, it
spends part of the same budget, which now runs from before it: what is left
is waited out, and a probe that has already answered is taken at once.

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 Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns

src/client.rs

@@ -32,7 +32,7 @@ use crate::{
     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,
     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},
 };
@@ -832,7 +832,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);
@@ -848,12 +848,44 @@ 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();
+        // Counted from before the key exchange, so the exchange spends the UDP NAT test's own
+        // wait rather than replacing it: the test runs beside both.
+        let udp_nat_wait_from = Instant::now();
+        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))?;
-        } else if let Some(udp) = udp.1.as_ref() {
+        }
-            let tm = Instant::now();
+        // A token or switch code has always taken this socket straight to the punch without
+        // waiting for the UDP NAT test. The WebRTC exchange does not replace that wait, it only
+        // spends part of the same budget, so what is left of it is waited out here and a result
+        // that has already arrived is taken at once.
+        if let Some(udp) = udp.1.as_ref().filter(|_| !legacy_secure) {
+            let tm = udp_nat_wait_from;
             // rtt is the TCP connect time. When it is too short to be a real WAN round trip it
             // says nothing about the UDP path (a TUN VPN or the LAN gateway answered the
             // handshake, not the server), so fall back to the flat grace; otherwise trust it.

src/common.rs

@@ -2079,6 +2079,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");
@@ -2107,6 +2114,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);
                     }
                     _ => {}
                 }
@@ -2114,7 +2122,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<()> {
@@ -2125,6 +2133,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 {
@@ -3263,4 +3287,88 @@ mod tests {
         assert_eq!(combined_mask & MOUSE_TYPE_MASK, MOUSE_TYPE_DOWN);
         assert_eq!(combined_mask >> 3, MOUSE_BUTTON_LEFT | MOUSE_BUTTON_RIGHT);
     }
+
+    /// 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;
+            }
+        });
+        host
+    }
+
+    fn server_key() -> (String, sign::SecretKey) {
+        let (pk, sk) = sign::gen_keypair();
+        (encode64(pk.0), sk)
+    }
+
+    async fn connect(host: &str) -> Stream {
+        hbb_common::socket_client::connect_tcp(host.to_owned(), 3000)
+            .await
+            .unwrap()
+    }
+
+    #[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

@@ -625,6 +625,20 @@ impl RendezvousMediator {
         );
 
         let mut socket = connect_tcp(&*self.host, CONNECT_TIMEOUT).await?;
+        // A relay response carrying an answer carries this machine's ICE candidates with it, so
+        // that half goes out only on an encrypted channel. A server that does not complete the
+        // exchange loses the answer, not the relay: the response goes without it, on a fresh
+        // socket since the failed exchange may have consumed a message on this one, and the
+        // controller falls back to its other transports.
+        let mut webrtc_sdp_answer = webrtc_sdp_answer;
+        if !webrtc_sdp_answer.is_empty() {
+            let key = crate::get_key(true).await;
+            if let Err(err) = crate::secure_tcp_required(&mut socket, &key).await {
+                log::warn!("relaying without the WebRTC answer, it cannot be encrypted: {err}");
+                webrtc_sdp_answer = String::new();
+                socket = connect_tcp(&*self.host, CONNECT_TIMEOUT).await?;
+            }
+        }
 
         let mut msg_out = Message::new();
         let mut rr = RelayResponse {
@@ -806,6 +820,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 {
@@ -819,7 +834,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: {}",
@@ -993,6 +1021,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(());
         }