Fix bad socket creation, bad socket configuration and lock within lock

boringtun/src/device/integration_tests/mod.rs

@@ -482,7 +482,7 @@ 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 _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 +635,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();

boringtun/src/device/mod.rs

@@ -303,12 +303,12 @@ impl DeviceHandle {
         }
     }
 
-    fn new_thread_local(thread_id: usize, device_lock: &LockReadGuard<Device>) -> ThreadData {
+    fn new_thread_local(_thread_id: usize, device_lock: &LockReadGuard<Device>) -> 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,11 +904,12 @@ 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 mut p = peer.lock();
+
+                for byte in p.tunnel.peer_static_public().as_bytes() {
                     let pub_symbol = format!("{:02X}", byte);
                     public_key.push_str(&pub_symbol);
                 }
@@ -920,7 +921,6 @@ impl Device {
 
                 while let Ok(read_bytes) = udp.recv(src_buf) {
                     let mut flush = false;
-                    let mut p = peer.lock();
                     match p.tunnel.decapsulate(
                         Some(peer_addr),
                         &t.src_buf[..read_bytes],
@@ -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(),

boringtun/src/device/peer.rs

@@ -107,11 +107,7 @@ impl Peer {
         }
     }
 
-    pub fn connect_endpoint(
-        &self,
-        port: u16,
-        fwmark: Option<u32>,
-    ) -> Result<socket2::Socket, Error> {
+    pub fn connect_endpoint(&self, port: u16) -> Result<socket2::Socket, Error> {
         let mut endpoint = self.endpoint.write();
 
         if endpoint.conn.is_some() {
@@ -123,22 +119,19 @@ 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()
         } else {
             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();
+    }
+}

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;