diff --git a/src/client.rs b/src/client.rs index 10330cfd6..f75aefe7f 100644 --- a/src/client.rs +++ b/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}, }; @@ -809,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); @@ -825,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 diff --git a/src/common.rs b/src/common.rs index 00a409a63..0a9a1a63c 100644 --- a/src/common.rs +++ b/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 { 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 { @@ -3258,4 +3282,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(serve: F) -> String + where + F: FnOnce(hbb_common::tcp::FramedStream) -> Fut + Send + 'static, + Fut: std::future::Future + 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()); + } } diff --git a/src/rendezvous_mediator.rs b/src/rendezvous_mediator.rs index 0436f060a..bb34c81ac 100644 --- a/src/rendezvous_mediator.rs +++ b/src/rendezvous_mediator.rs @@ -806,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 { @@ -819,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: {}", @@ -993,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(()); }