diff --git a/libs/base/protos/message.proto b/libs/base/protos/message.proto index 1276deaa8..dd62942be 100644 --- a/libs/base/protos/message.proto +++ b/libs/base/protos/message.proto @@ -705,6 +705,9 @@ message TestDelay { message PublicKey { bytes asymmetric_value = 1; bytes symmetric_value = 2; + // The key exchange version the controller picked, the highest both sides speak; absent, + // i.e. 0, is the original scheme. + uint32 kx_version = 3; } message SignedId { bytes id = 1; } diff --git a/libs/hbb_common b/libs/hbb_common index d9895ff9c..229b90450 160000 --- a/libs/hbb_common +++ b/libs/hbb_common @@ -1 +1 @@ -Subproject commit d9895ff9cb4c77137fb05ce3161bcf7210177aef +Subproject commit 229b904508364c8997aad0fb5af57effac859f60 diff --git a/src/client.rs b/src/client.rs index 0fca4dfcd..ca4c87470 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1694,7 +1694,9 @@ impl Client { let bytes = res?; if let Ok(msg_in) = Message::parse_from_bytes(&bytes) { if let Some(message::Union::SignedId(si)) = msg_in.union { - if let Ok((id, their_pk_b, signed_fp)) = decode_id_pk_dtls(&si.id, &sign_pk) { + if let Ok((id, their_pk_b, signed_fp, kx_version)) = + decode_id_pk_dtls(&si.id, &sign_pk) + { if id == peer_id { // WebRTC only: bind the DTLS channel to the verified peer identity. // webrtc-rs already bound the certificate to the remote SDP, so @@ -1710,14 +1712,31 @@ impl Client { } let (asymmetric_value, symmetric_value, key) = create_symmetric_key_msg(their_pk_b); + // A WebRTC stream is encrypted by DTLS and takes no stream + // key of its own, split or not, so the pick says what runs: 0. + let picked = if is_webrtc { + 0 + } else { + hbb_common::tcp::kx_version_for(kx_version) + }; let mut msg_out = Message::new(); msg_out.set_public_key(PublicKey { - asymmetric_value, + asymmetric_value: asymmetric_value.clone(), symmetric_value, + kx_version: picked, ..Default::default() }); timeout(CONNECT_TIMEOUT, conn.send(&msg_out)).await??; - conn.set_key(key); + conn.set_negotiated_key( + key, + true, + &hbb_common::tcp::KxTranscript { + initiator_pk: &asymmetric_value, + responder_pk: &their_pk_b, + advertised: kx_version, + picked, + }, + )?; } else { if is_webrtc { bail!("WebRTC handshake id mismatch (possible MITM)"); @@ -5795,3 +5814,142 @@ mod webrtc_race_tests { assert!(err.contains("webrtc dead") && err.contains("relay dead"), "{}", err); } } + +#[cfg(test)] +mod kx_tests { + use super::*; + use hbb_common::{ + sodiumoxide::crypto::box_, + tcp::{Encrypt, FramedStream, KxTranscript, KX_VERSION_LATEST}, + }; + + const PEER_ID: &str = "123456789"; + + /// What the stand-in controlled peer saw: the version the controller picked, and whether the + /// controller's first application message decrypted. + struct Seen { + picked: u32, + decrypted: bool, + } + + fn test_delay(time: i64) -> Message { + let mut msg = Message::new(); + msg.set_test_delay(TestDelay { + time, + ..Default::default() + }); + msg + } + + fn delay_in(bytes: &[u8]) -> Option { + match Message::parse_from_bytes(bytes).ok()?.union? { + message::Union::TestDelay(t) => Some(t.time), + _ => None, + } + } + + /// A stand-in controlled peer on loopback. It advertises `advertised` in its signed identity + /// and runs the key under `run`, or under the version the controller picked when `run` is + /// `None`, then sends one message and reads one. Returns the host, its identity as the + /// rendezvous server would sign it, and what it saw. + async fn controlled_stub( + advertised: u32, + run: Option, + rs_sk: sign::SecretKey, + ) -> (String, Vec, oneshot::Receiver) { + let (sign_pk, sign_sk) = sign::gen_keypair(); + let id_pk = |pk: &[u8], kx_version| { + IdPk { + id: PEER_ID.to_owned(), + pk: pk.to_vec().into(), + kx_version, + ..Default::default() + } + .write_to_bytes() + .unwrap() + }; + let signed_id_pk = sign::sign(&id_pk(&sign_pk.0, 0), &rs_sk); + let listener = hbb_common::tcp::new_listener("127.0.0.1:0", false) + .await + .unwrap(); + let host = listener.local_addr().unwrap().to_string(); + let (tx, rx) = oneshot::channel(); + tokio::spawn(async move { + let (stream, addr) = listener.accept().await.unwrap(); + let mut s = FramedStream::from(stream, addr); + let (our_pk_b, our_sk_b) = box_::gen_keypair(); + let mut msg = Message::new(); + msg.set_signed_id(SignedId { + id: sign::sign(&id_pk(&our_pk_b.0, advertised), &sign_sk).into(), + ..Default::default() + }); + s.send(&msg).await.unwrap(); + let bytes = s.next().await.unwrap().unwrap(); + let Some(message::Union::PublicKey(pk)) = + Message::parse_from_bytes(&bytes).unwrap().union + else { + panic!("expected the controller's public key"); + }; + let key = + Encrypt::decode(&pk.symmetric_value, &pk.asymmetric_value, &our_sk_b).unwrap(); + let t = KxTranscript { + initiator_pk: &pk.asymmetric_value, + responder_pk: &our_pk_b.0, + advertised, + picked: run.unwrap_or(pk.kx_version), + }; + if t.picked == 0 { + s.set_key(key); + } else { + s.set_key_split(key, false, &t).unwrap(); + } + s.send(&test_delay(2)).await.unwrap(); + let decrypted = matches!(s.next().await, Some(Ok(b)) if delay_in(&b) == Some(1)); + tx.send(Seen { + picked: pk.kx_version, + decrypted, + }) + .ok(); + }); + (host, signed_id_pk, rx) + } + + /// The controller's handshake against the stub, then one application message each way. + /// Returns what the stub saw and whether the stub's message decrypted on this side. + async fn handshake(advertised: u32, run: Option) -> (Seen, bool) { + let (rs_pk, rs_sk) = sign::gen_keypair(); + let (host, signed_id_pk, seen) = controlled_stub(advertised, run, rs_sk).await; + let mut conn = connect_tcp(host, 3000).await.unwrap(); + let pk = + Client::secure_connection(PEER_ID, signed_id_pk, &crate::encode64(rs_pk.0), &mut conn) + .await + .unwrap(); + assert!(pk.is_some() && conn.is_secured()); + conn.send(&test_delay(1)).await.unwrap(); + let decrypted = matches!(conn.next().await, Some(Ok(b)) if delay_in(&b) == Some(2)); + (seen.await.unwrap(), decrypted) + } + + #[tokio::test] + async fn test_new_peers_pick_version_1_and_exchange_application_data() { + let (seen, decrypted) = handshake(KX_VERSION_LATEST, None).await; + assert_eq!(seen.picked, 1); + assert!(seen.decrypted && decrypted); + } + + #[tokio::test] + async fn test_a_peer_without_versions_gets_version_0() { + let (seen, decrypted) = handshake(0, Some(0)).await; + assert_eq!(seen.picked, 0); + assert!(seen.decrypted && decrypted); + } + + #[tokio::test] + async fn test_a_pick_lowered_in_transit_decrypts_nothing() { + // The controlled side takes 0 from a controller without versions, so a lowered pick is + // caught by the keys disagreeing on the first application message, not before. + let (seen, decrypted) = handshake(KX_VERSION_LATEST, Some(0)).await; + assert_eq!(seen.picked, 1); + assert!(!seen.decrypted && !decrypted); + } +} diff --git a/src/common.rs b/src/common.rs index c98627187..7daee26b8 100644 --- a/src/common.rs +++ b/src/common.rs @@ -2151,17 +2151,46 @@ async fn key_exchange(conn: &mut Stream, key: &str, log_on_success: bool) -> Res } let their_pk_b = sign::verify(&ex.keys[0], &rs_pk) .map_err(|_| anyhow!("Signature mismatch in key exchange"))?; - let (asymmetric_value, symmetric_value, key) = create_symmetric_key_msg( - get_pk(&their_pk_b) - .context("Wrong their public length in key exchange")?, - ); + let their_pk_b = get_pk(&their_pk_b) + .context("Wrong their public length in key exchange")?; + // The signed X25519 high bit marks servers that sign their parameters. + // X25519 ignores that bit, so old clients use the key unchanged; keep + // the bytes as signed, since the transcript and KxParams carry them so. + if their_pk_b[31] & 0x80 != 0 || !ex.signed_params.is_empty() { + let params = sign::verify(&ex.signed_params, &rs_pk) + .ok() + .and_then(|signed| { + let params = + signed.strip_prefix(hbb_common::tcp::KX_PARAMS_DOMAIN)?; + KxParams::parse_from_bytes(params).ok() + }) + .ok_or_else(|| { + anyhow!("Missing or invalid signed key exchange parameters") + })?; + if params.pk[..] != their_pk_b[..] || params.version != ex.version { + bail!("Key exchange version or public key does not match its signature"); + } + } + let (asymmetric_value, symmetric_value, key) = + create_symmetric_key_msg(their_pk_b); + let picked = hbb_common::tcp::kx_version_for(ex.version); let mut msg_out = RendezvousMessage::new(); msg_out.set_key_exchange(KeyExchange { - keys: vec![asymmetric_value, symmetric_value], + keys: vec![asymmetric_value.clone(), symmetric_value], + version: picked, ..Default::default() }); timeout(CONNECT_TIMEOUT, conn.send(&msg_out)).await??; - conn.set_key(key); + conn.set_negotiated_key( + key, + true, + &hbb_common::tcp::KxTranscript { + initiator_pk: &asymmetric_value, + responder_pk: &their_pk_b, + advertised: ex.version, + picked, + }, + )?; if log_on_success { log::info!("Connection secured"); } @@ -2221,21 +2250,23 @@ pub fn get_rs_pk(str_base64: &str) -> Option { } pub fn decode_id_pk(signed: &[u8], key: &sign::PublicKey) -> ResultType<(String, [u8; 32])> { - let (id, pk, _) = decode_id_pk_dtls(signed, key)?; + let (id, pk, _, _) = decode_id_pk_dtls(signed, key)?; Ok((id, pk)) } /// Like [`decode_id_pk`] but also returns the signed DTLS certificate fingerprint (empty string -/// for non-WebRTC peers), used to bind a WebRTC DTLS channel to the verified peer identity. +/// for non-WebRTC peers), used to bind a WebRTC DTLS channel to the verified peer identity, and +/// the newest key exchange version the peer speaks (0, the original scheme, for a peer from +/// before versions). pub fn decode_id_pk_dtls( signed: &[u8], key: &sign::PublicKey, -) -> ResultType<(String, [u8; 32], String)> { +) -> ResultType<(String, [u8; 32], String, u32)> { let res = IdPk::parse_from_bytes( &sign::verify(signed, key).map_err(|_| anyhow!("Signature mismatch"))?, )?; if let Some(pk) = get_pk(&res.pk) { - Ok((res.id, pk, res.dtls_fingerprint)) + Ok((res.id, pk, res.dtls_fingerprint, res.kx_version)) } else { bail!("Wrong their public length"); } @@ -3384,6 +3415,26 @@ mod tests { (encode64(pk.0), sk) } + fn signed_key_exchange( + pk: &box_::PublicKey, + sk: &sign::SecretKey, + version: u32, + ) -> KeyExchange { + let params = KxParams { + pk: pk.0.to_vec().into(), + version, + ..Default::default() + }; + let mut payload = hbb_common::tcp::KX_PARAMS_DOMAIN.to_vec(); + payload.extend_from_slice(¶ms.write_to_bytes().unwrap()); + KeyExchange { + keys: vec![sign::sign(&pk.0, sk).into()], + version, + signed_params: sign::sign(&payload, sk).into(), + ..Default::default() + } + } + async fn connect(host: &str) -> Stream { hbb_common::socket_client::connect_tcp(host.to_owned(), 3000) .await @@ -3445,6 +3496,248 @@ mod tests { assert!(conn.is_secured()); } + // The stand-in does what hbbs does at version 1: advertises it and splits the exchanged key + // over the same transcript. Neither side's unit tests can catch a client that puts a + // different byte string into the transcript than the server does; only a frame crossing + // between the two can. + #[tokio::test] + async fn test_secure_tcp_version_1_keys_match_the_server_both_ways() { + 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()], + version: 1, + ..Default::default() + }); + s.send(&msg).await.unwrap(); + 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"); + }; + let shared = + hbb_common::tcp::Encrypt::decode(&ex.keys[1], &ex.keys[0], &eph_sk).unwrap(); + s.set_key_split( + shared, + false, + &hbb_common::tcp::KxTranscript { + initiator_pk: &ex.keys[0], + responder_pk: &eph_pk.0, + advertised: 1, + picked: ex.version, + }, + ) + .unwrap(); + // Answers with the request's serial, so the client learns from its own reply that + // the request was read under the right key. + let request = s.next_timeout(3000).await.unwrap().unwrap(); + let request = RendezvousMessage::parse_from_bytes(&request).unwrap(); + let Some(rendezvous_message::Union::TestNatRequest(nat)) = request.union else { + panic!("expected the client's nat request"); + }; + let mut msg = RendezvousMessage::new(); + msg.set_test_nat_response(TestNatResponse { + port: nat.serial, + ..Default::default() + }); + s.send(&msg).await.unwrap(); + }) + .await; + let mut conn = connect(&host).await; + secure_tcp_required(&mut conn, &key).await.unwrap(); + let mut msg = RendezvousMessage::new(); + msg.set_test_nat_request(TestNatRequest { + serial: 7, + ..Default::default() + }); + conn.send(&msg).await.unwrap(); + let reply = conn.next_timeout(3000).await.unwrap().unwrap(); + let reply = RendezvousMessage::parse_from_bytes(&reply).unwrap(); + let Some(rendezvous_message::Union::TestNatResponse(nat)) = reply.union else { + panic!("expected the server's nat response"); + }; + assert_eq!(nat.port, 7); + } + + #[tokio::test] + async fn test_secure_tcp_legacy_and_signed_servers_exchange_application_data() { + for (advertised, signed) in [(0, false), (1, true), (3, true)] { + for required in [false, true] { + let (key, sk) = server_key(); + let host = rendezvous_stub(move |mut s| async move { + let (mut eph_pk, eph_sk) = box_::gen_keypair(); + let mut msg = RendezvousMessage::new(); + let ex = if signed { + eph_pk.0[31] |= 0x80; + signed_key_exchange(&eph_pk, &sk, advertised) + } else { + KeyExchange { + keys: vec![sign::sign(&eph_pk.0, &sk).into()], + ..Default::default() + } + }; + msg.set_key_exchange(ex); + s.send(&msg).await.unwrap(); + 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"); + }; + assert_eq!(ex.keys.len(), 2); + assert_eq!(ex.version, hbb_common::tcp::kx_version_for(advertised)); + let shared = + hbb_common::tcp::Encrypt::decode(&ex.keys[1], &ex.keys[0], &eph_sk) + .unwrap(); + if ex.version >= 1 { + s.set_key_split( + shared, + false, + &hbb_common::tcp::KxTranscript { + initiator_pk: &ex.keys[0], + responder_pk: &eph_pk.0, + advertised, + picked: ex.version, + }, + ) + .unwrap(); + } else { + s.set_key(shared); + } + let request = s.next_timeout(3000).await.unwrap().unwrap(); + let request = RendezvousMessage::parse_from_bytes(&request).unwrap(); + let Some(rendezvous_message::Union::TestNatRequest(nat)) = request.union else { + panic!("expected the client's nat request"); + }; + let mut msg = RendezvousMessage::new(); + msg.set_test_nat_response(TestNatResponse { + port: nat.serial, + ..Default::default() + }); + s.send(&msg).await.unwrap(); + }) + .await; + let mut conn = connect(&host).await; + if required { + secure_tcp_required(&mut conn, &key).await.unwrap(); + } else { + secure_tcp(&mut conn, &key).await.unwrap(); + } + assert!(conn.is_secured()); + let mut msg = RendezvousMessage::new(); + msg.set_test_nat_request(TestNatRequest { + serial: 7, + ..Default::default() + }); + conn.send(&msg).await.unwrap(); + let reply = conn.next_timeout(3000).await.unwrap().unwrap(); + let reply = RendezvousMessage::parse_from_bytes(&reply).unwrap(); + let Some(rendezvous_message::Union::TestNatResponse(nat)) = reply.union else { + panic!("expected the server's nat response"); + }; + assert_eq!(nat.port, 7); + } + } + } + + #[tokio::test] + async fn test_secure_tcp_rejects_unverified_versions_before_reply() { + let (key, sk) = server_key(); + let (mut eph_pk, _) = box_::gen_keypair(); + eph_pk.0[31] |= 0x80; + let (other_pk, _) = box_::gen_keypair(); + let (_, other_sk) = sign::gen_keypair(); + for case in [ + "missing_signature", + "stripped_version_and_signature", + "cleared_marker_and_stripped_fields", + "lowered_version", + "raised_version", + "replaced_public_key", + "invalid_signature", + "wrong_signer", + "unstructured_payload", + "undomained_params", + "signed_id_pk_as_params", + ] { + let mut ex = signed_key_exchange(&eph_pk, &sk, 1); + match case { + "missing_signature" => ex.signed_params = Bytes::new(), + "stripped_version_and_signature" => { + ex.signed_params = Bytes::new(); + ex.version = 0; + } + "cleared_marker_and_stripped_fields" => { + let mut signed_pk = ex.keys[0].to_vec(); + *signed_pk.last_mut().unwrap() &= 0x7f; + ex.keys[0] = signed_pk.into(); + ex.signed_params = Bytes::new(); + ex.version = 0; + } + "lowered_version" => ex.version = 0, + "raised_version" => ex.version = 2, + "replaced_public_key" => ex.keys[0] = sign::sign(&other_pk.0, &sk).into(), + "invalid_signature" => { + let mut signed = ex.signed_params.to_vec(); + signed[0] ^= 1; + ex.signed_params = signed.into(); + } + "wrong_signer" => { + ex.signed_params = signed_key_exchange(&eph_pk, &other_sk, 1).signed_params; + } + "unstructured_payload" => { + let mut payload = eph_pk.0.to_vec(); + payload.extend_from_slice(&1u32.to_le_bytes()); + ex.signed_params = sign::sign(&payload, &sk).into(); + } + "undomained_params" => { + let params = KxParams { + pk: eph_pk.0.to_vec().into(), + version: 1, + ..Default::default() + }; + ex.signed_params = sign::sign(¶ms.write_to_bytes().unwrap(), &sk).into(); + } + "signed_id_pk_as_params" => { + // The server signs IdPk with the same key; its `id` sits where `pk` does. + let mut pk = [0x2au8; box_::PUBLICKEYBYTES]; + pk[30] = 0xc2; + pk[31] = 0xaa; + let id_pk = IdPk { + id: String::from_utf8(pk.to_vec()).unwrap(), + pk: vec![7u8; box_::PUBLICKEYBYTES].into(), + ..Default::default() + }; + ex.keys[0] = sign::sign(&pk, &sk).into(); + ex.version = 0; + ex.signed_params = sign::sign(&id_pk.write_to_bytes().unwrap(), &sk).into(); + } + _ => unreachable!(), + } + for required in [false, true] { + let mut msg = RendezvousMessage::new(); + msg.set_key_exchange(ex.clone()); + let (tx, rx) = tokio::sync::oneshot::channel(); + let host = rendezvous_stub(move |mut s| async move { + s.send(&msg).await.unwrap(); + tx.send(s.next_timeout(3000).await.is_none()).unwrap(); + }) + .await; + let mut conn = connect(&host).await; + let result = if required { + secure_tcp_required(&mut conn, &key).await + } else { + secure_tcp(&mut conn, &key).await + }; + assert!(result.is_err(), "accepted {case}, required={required}"); + assert!(!conn.is_secured()); + drop(conn); + assert!(rx.await.unwrap(), "replied to {case}, required={required}"); + } + } + } + #[test] fn test_dtls_fingerprint_travels_signed_and_binds() { let (pk, sk) = sign::gen_keypair(); @@ -3461,7 +3754,7 @@ mod tests { &sk, ); - let (id, their_pk, signed_fp) = decode_id_pk_dtls(&signed, &pk).unwrap(); + 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); diff --git a/src/server.rs b/src/server.rs index 7b2cbe792..88341631b 100644 --- a/src/server.rs +++ b/src/server.rs @@ -276,12 +276,21 @@ async fn identity_handshake(stream: &mut Stream, secure: bool) -> ResultType<()> if stream.is_webrtc() && dtls_fingerprint.is_empty() { bail!("WebRTC local DTLS fingerprint unavailable"); } + // A WebRTC stream is encrypted by DTLS and takes no stream key of its own, split or + // not, so what it advertises is what it runs: 0. Saying 1 there would have a future + // controller that splits keys on WebRTC agree on a version this side never applies. + let advertised = if stream.is_webrtc() { + 0 + } else { + tcp::KX_VERSION_LATEST + }; msg_out.set_signed_id(SignedId { id: sign::sign( &IdPk { id: Config::get_id(), pk: Bytes::from(our_pk_b.0.to_vec()), dtls_fingerprint, + kx_version: advertised, ..Default::default() } .write_to_bytes() @@ -298,11 +307,28 @@ async fn identity_handshake(stream: &mut Stream, secure: bool) -> ResultType<()> if let Ok(msg_in) = Message::parse_from_bytes(&bytes) { if let Some(message::Union::PublicKey(pk)) = msg_in.union { if pk.asymmetric_value.len() == box_::PUBLICKEYBYTES { - stream.set_key(tcp::Encrypt::decode( + let key = tcp::Encrypt::decode( &pk.symmetric_value, &pk.asymmetric_value, &our_sk_b, - )?); + )?; + if pk.kx_version > advertised { + bail!( + "Handshake failed: key exchange version {} not offered, {} was", + pk.kx_version, + advertised + ); + } + stream.set_negotiated_key( + key, + false, + &tcp::KxTranscript { + initiator_pk: &pk.asymmetric_value, + responder_pk: &our_pk_b.0, + advertised, + picked: pk.kx_version, + }, + )?; } else if pk.asymmetric_value.is_empty() { Config::set_key_confirmed(false); log::info!("Force to update pk");