From 8deb64b3ba83d30f26ef1437575acaca27627219 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juozapas=20Bo=C4=8Dkus?= Date: Tue, 16 Jul 2024 13:07:53 +0300 Subject: [PATCH] Fix bad socket creation, bad socket configuration and lock within lock --- boringtun/src/device/integration_tests/mod.rs | 3 +- boringtun/src/device/mod.rs | 22 ++++----- boringtun/src/device/peer.rs | 45 ++++++++++++++----- boringtun/src/noise/integration_tests/mod.rs | 4 +- 4 files changed, 44 insertions(+), 30 deletions(-) diff --git a/boringtun/src/device/integration_tests/mod.rs b/boringtun/src/device/integration_tests/mod.rs index 81e5740c..a4f47997 100644 --- a/boringtun/src/device/integration_tests/mod.rs +++ b/boringtun/src/device/integration_tests/mod.rs @@ -482,7 +482,6 @@ mod tests { fn test_wireguard_set() { let port = next_port(); let private_key = StaticSecret::random_from_rng(OsRng); - let own_public_key = PublicKey::from(&private_key); let wg = WGHandle::init("192.0.2.0".parse().unwrap(), "::2".parse().unwrap()); assert!(wg.wg_get().ends_with("errno=0\n\n")); @@ -635,7 +634,7 @@ mod tests { #[ignore] fn test_wg_start_ipv4_traffic_flows_after_reject_after_time() { let port = next_port(); - let private_key = StaticSecret::new(OsRng); + let private_key = StaticSecret::random_from_rng(OsRng); let public_key = PublicKey::from(&private_key); let addr_v4 = next_ip(); let addr_v6 = next_ip_v6(); diff --git a/boringtun/src/device/mod.rs b/boringtun/src/device/mod.rs index c02b52d9..12dbc8da 100644 --- a/boringtun/src/device/mod.rs +++ b/boringtun/src/device/mod.rs @@ -303,12 +303,12 @@ impl DeviceHandle { } } - fn new_thread_local(thread_id: usize, device_lock: &LockReadGuard) -> ThreadData { + fn new_thread_local(_thread_id: usize, device_lock: &LockReadGuard) -> ThreadData { #[cfg(target_os = "linux")] let t_local = ThreadData { src_buf: [0u8; MAX_UDP_SIZE], dst_buf: [0u8; MAX_UDP_SIZE], - iface: if thread_id == 0 || !device_lock.config.use_multi_queue { + iface: if _thread_id == 0 || !device_lock.config.use_multi_queue { // For the first thread use the original iface Arc::clone(&device_lock.iface) } else { @@ -875,7 +875,7 @@ impl Device { let ip_addr = addr.ip(); p.set_endpoint(addr); if d.config.use_connected_socket { - if let Ok(sock) = p.connect_endpoint(d.listen_port, d.fwmark) { + if let Ok(sock) = p.connect_endpoint(d.listen_port) { d.register_conn_handler(Arc::clone(peer), sock, ip_addr) .unwrap(); } @@ -904,10 +904,10 @@ impl Device { // The conn_handler handles packet received from a connected UDP socket, associated // with a known peer, this saves us the hustle of finding the right peer. If another // peer gets the same ip, it will be ignored until the socket does not expire. - let iface = &t.iface; let mut iter = MAX_ITR; let mut public_key = String::with_capacity(32); + for byte in peer.lock().tunnel.peer_static_public().as_bytes() { let pub_symbol = format!("{:02X}", byte); public_key.push_str(&pub_symbol); @@ -940,15 +940,12 @@ impl Device { } TunnResult::WriteToTunnelV4(packet, addr) => { if let Some(callback) = &d.config.firewall_process_inbound_callback { - if !callback( - &peer.lock().tunnel.peer_static_public().to_bytes(), - packet, - ) { + if !callback(&p.tunnel.peer_static_public().to_bytes(), packet) { continue; } } if p.is_allowed_ip(addr) { - iface.write4(packet); + t.iface.write4(packet); tracing::trace!( message = "Writing packet to tunnel v4", interface = ?t.iface.name(), @@ -960,15 +957,12 @@ impl Device { } TunnResult::WriteToTunnelV6(packet, addr) => { if let Some(callback) = &d.config.firewall_process_inbound_callback { - if !callback( - &peer.lock().tunnel.peer_static_public().to_bytes(), - packet, - ) { + if !callback(&p.tunnel.peer_static_public().to_bytes(), packet) { continue; } } if p.is_allowed_ip(addr) { - iface.write6(packet); + t.iface.write6(packet); tracing::trace!( message = "Writing packet to tunnel v6", interface = ?t.iface.name(), diff --git a/boringtun/src/device/peer.rs b/boringtun/src/device/peer.rs index ec48ba44..78e14190 100644 --- a/boringtun/src/device/peer.rs +++ b/boringtun/src/device/peer.rs @@ -107,11 +107,7 @@ impl Peer { } } - pub fn connect_endpoint( - &self, - port: u16, - fwmark: Option, - ) -> Result { + pub fn connect_endpoint(&self, port: u16) -> Result { let mut endpoint = self.endpoint.write(); if endpoint.conn.is_some() { @@ -123,7 +119,7 @@ impl Peer { .expect("Attempt to connect to undefined endpoint"); let udp_conn = - socket2::Socket::new(Domain::for_address(addr), Type::STREAM, Some(Protocol::UDP))?; + socket2::Socket::new(Domain::for_address(addr), Type::DGRAM, Some(Protocol::UDP))?; udp_conn.set_reuse_address(true)?; let bind_addr = if addr.is_ipv4() { SocketAddrV4::new(Ipv4Addr::UNSPECIFIED, port).into() @@ -131,14 +127,11 @@ impl Peer { SocketAddrV6::new(Ipv6Addr::UNSPECIFIED, port, 0, 0).into() }; udp_conn.bind(&bind_addr)?; - udp_conn.connect(&addr.into())?; udp_conn.set_nonblocking(true)?; + // fw_mark is being set inside make_external(), so no need to set it twice as in Cloudflare's repo. self.protect.make_external(udp_conn.as_raw_fd()); - - #[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))] - if let Some(fwmark) = fwmark { - udp_conn.set_mark(fwmark)?; - } + // Also mind that all socket setup functions should be called before .connect(). + udp_conn.connect(&addr.into())?; tracing::info!( message="Connected endpoint", @@ -204,3 +197,31 @@ impl Peer { self.index } } + +#[cfg(test)] +mod tests { + use super::*; + use rand::SeedableRng; + use x25519_dalek::{PublicKey, StaticSecret}; + + // Introduced this test to prevent LLT-5351 recurring in the future: + #[test] + fn test_connect_endpoint() { + let a_secret_key = StaticSecret::random_from_rng(&mut rand::rngs::StdRng::from_entropy()); + + let b_secret_key = StaticSecret::random_from_rng(&mut rand::rngs::StdRng::from_entropy()); + let b_public_key = PublicKey::from(&b_secret_key); + + let tunnel = Tunn::new(a_secret_key, b_public_key, None, None, 0, None).unwrap(); + let peer = Peer::new( + tunnel, + 0, + Some(SocketAddr::new(IpAddr::from([1, 2, 3, 4]), 54321)), + &[], + None, + Arc::new(crate::device::MakeExternalBoringtunNoop), + ); + + peer.connect_endpoint(12345).unwrap(); + } +} diff --git a/boringtun/src/noise/integration_tests/mod.rs b/boringtun/src/noise/integration_tests/mod.rs index 87ac4df7..41f1e9c3 100644 --- a/boringtun/src/noise/integration_tests/mod.rs +++ b/boringtun/src/noise/integration_tests/mod.rs @@ -51,10 +51,10 @@ mod tests { } async fn setup() -> (WGClient, WGClient) { - let a_secret_key = StaticSecret::new(&mut rand::rngs::StdRng::from_entropy()); + let a_secret_key = StaticSecret::random_from_rng(&mut rand::rngs::StdRng::from_entropy()); let a_public_key = PublicKey::from(&a_secret_key); - let b_secret_key = StaticSecret::new(&mut rand::rngs::StdRng::from_entropy()); + let b_secret_key = StaticSecret::random_from_rng(&mut rand::rngs::StdRng::from_entropy()); let b_public_key = PublicKey::from(&b_secret_key); let a = WGClient::new(a_secret_key, b_public_key).await;