Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions boringtun/src/device/integration_tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down Expand Up @@ -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();
Expand Down
22 changes: 8 additions & 14 deletions boringtun/src/device/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down
45 changes: 33 additions & 12 deletions boringtun/src/device/peer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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",
Expand Down Expand Up @@ -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();
}
}
4 changes: 2 additions & 2 deletions boringtun/src/noise/integration_tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading